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.