[PATCH 1/3] drm: rockchip: hdmi: remove vop_crtc_mode_fixup to fix clock handling

[Doug Anderson]

Hi,

On Tue, Sep 22, 2020 at 12:10 PM Vicente Bergas <vicencb at gmail.com> wrote:
>
> On Tuesday, September 22, 2020 5:26:17 PM CEST, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Sep 22, 2020 at 7:52 AM Vicente Bergas <vicencb at gmail.com> wrote:
> >> On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson
> >> <dianders at chromium.org> wrote: ...
> >
> > Here's the code:
> >
> >   rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
> >   adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
> >
> > Input clock is in kHz and DRM always rounds down (last I checked--I
> > guess you could confirm if this is still true).
> >
> > Imagine that you want an input clock of 999999 kHz and the PLL can
> > actually make this.
> >
> > DRM will request a clock of 999 kHz because it always rounds down.
> >
> > First:
> >   rate = 999 * 1000 + 999 = 999999 Hz
> >
> > Now we'll ask the clock framework if it can make this.  It can, so
> > clk_round_rate() will return 999999 kHz.  Note that, at least on all
> > Rockchip platforms I looked at in the past, clk_round_rate() and
> > clk_set_rate() always round down.  Thus, if we _hadn't_ added the 999
> > here we would not have gotten back 999999 Hz.
> >
> > We have to return a rate in terms of kHz.  While we could round down
> > like DRM does, it seemed better at the time to do the rounding here.
> > Thus, I now rounded up.  We should end up storing
> >
> >   (999999 + 999) / 1000 = 1000 kHz
> >
> > Then, when we use it in vop_crtc_atomic_enable() we don't have to do
> > any more rounding.
> >
> > I guess it's possible that the problem is that the function is
> > starting with an input where it knows that "adjusted_mode->clock" was
> > rounded down and it ends with it rounded up.  That shouldn't cause
> > problems unless somehow the function is being called twice or someone
> > else is making assumptions about the rounding.  You could,
> > potentially, change this to:
> >
> >   adjusted_mode->clock = rate / 1000;
> >
> > ...and then in vop_crtc_atomic_enable() you add the "999" back in, like:
> >
> >   clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
> >
> > That would make it more consistent / stable.  Does it work for you?
>
> Hi Douglas,
>
> i've tested this as suggested:
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1181,7 +1181,7 @@
>          *    this in the actual clk_set_rate().
>          */
>         rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
> -       adjusted_mode->clock = DIV_ROUND_UP(rate, 1000);
> +       adjusted_mode->clock = rate / 1000;

You'll also want to change the comment above.  Specifically it says
that we're storing the rounded up state.


>         return true;
>  }
> @@ -1380,7 +1380,7 @@
>
>         VOP_REG_SET(vop, intr, line_flag_num[0], vact_end);
>
> -       clk_set_rate(vop->dclk, adjusted_mode->clock * 1000);
> +       clk_set_rate(vop->dclk, adjusted_mode->clock * 1000 + 999);
>
>         VOP_REG_SET(vop, common, standby, 0);
>         mutex_unlock(&vop->vop_lock);
> and it also works fine.
> Should i sent a V2 of this patch series including your approach?

That would be good w/ me.

-Doug