Hi,
On Tue, Sep 22, 2020 at 12:10 PM Vicente Bergas vicencb@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@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;
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