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@xxxxxxxxx> wrote:
On Tue, Sep 22, 2020 at 4:28 PM Doug Anderson
<dianders@xxxxxxxxxxxx> 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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
