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

[Vicente Bergas <vicencb@gmail.com>]

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@gmail.com> wrote:
>> On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson 
>> <dianders@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;
 
 	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?

For completeness i've added some printks to the original code to show the
clock values:
1.- Provided adjusted_mode->clock
adjusted_mode->clock (before) = 148500KHz
rate = 148500998Hz
adjusted_mode->clock (after) = 148501KHz <= this is the problematic clock

2.- Overwrite adjusted_mode->clock with the comment's value of 60000.001KHz
adjusted_mode->clock (before) = 60000KHz
rate = 60000998Hz
adjusted_mode->clock (after) = 60001KHz

3.- Overwrite adjusted_mode->clock with your mentioned value of 999.999KHz
adjusted_mode->clock (before) = 999KHz
rate = 999999Hz
adjusted_mode->clock (after) = 1000KHz

Regards,
  Vicente.


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip