Hi, On Tue, Sep 22, 2020 at 12:10 PM Vicente Bergas <vicencb@xxxxxxxxx> 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@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; 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
