| Message ID | 20210916202907.18394-1-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/rockchip: Update crtc fixup to account for fractional clk change | expand |
Hi, On Thu, Sep 16, 2021 at 1:29 PM Chris Morgan <macroalpha82@gmail.com> wrote: > > From: Chris Morgan <macromorgan@hotmail.com> > > After commit 928f9e268611 ("clk: fractional-divider: Hide > clk_fractional_divider_ops from wide audience") was merged it appears > that the DSI panel on my Odroid Go Advance stopped working. Upon closer > examination of the problem, it looks like it was the fixup in the > rockchip_drm_vop.c file was causing the issue. The changes made to the > clk driver appear to change some assumptions made in the fixup. > > After debugging the working 5.14 kernel and the no-longer working > 5.15 kernel, it looks like this was broken all along but still > worked, whereas after the fractional clock change it stopped > working despite the issue (it went from sort-of broken to very broken). > > In the 5.14 kernel the dclk_vopb_frac was being requested to be set to > 17000999 on my board. The clock driver was taking the value of the > parent clock and attempting to divide the requested value from it > (17000000/17000999 = 0), then subtracting 1 from it (making it -1), > and running it through fls_long to get 64. It would then subtract > the value of fd->mwidth from it to get 48, and then bit shift > 17000999 to the left by 48, coming up with a very large number of > 7649082492112076800. This resulted in a numerator of 65535 and a > denominator of 1 from the clk driver. The driver seemingly would > try again and get a correct 1:1 value later, and then move on. > > Output from my 5.14 kernel (with some printfs for good measure): > [ 2.830066] rockchip-drm display-subsystem: bound ff460000.vop (ops vop_component_ops) > [ 2.839431] rockchip-drm display-subsystem: bound ff450000.dsi (ops dw_mipi_dsi_rockchip_ops) > [ 2.855980] Clock is dclk_vopb_frac > [ 2.856004] Scale 64, Rate 7649082492112076800, Oldrate 17000999, Parent Rate 17000000, Best Numerator 65535, Best Denominator 1, fd->mwidth 16 > [ 2.903529] Clock is dclk_vopb_frac > [ 2.903556] Scale 0, Rate 17000000, Oldrate 17000000, Parent Rate 17000000, Best Numerator 1, Best Denominator 1, fd->mwidth 16 > [ 2.903579] Clock is dclk_vopb_frac > [ 2.903583] Scale 0, Rate 17000000, Oldrate 17000000, Parent Rate 17000000, Best Numerator 1, Best Denominator 1, fd->mwidth 16 > > Contrast this with 5.15 after the clk change where the rate of 17000999 > was getting passed and resulted in numerators/denomiators of 17001/ > 17000. > > Output from my 5.15 kernel (with some printfs added for good measure): > [ 2.817571] rockchip-drm display-subsystem: bound ff460000.vop (ops vop_component_ops) > [ 2.826975] rockchip-drm display-subsystem: bound ff450000.dsi (ops dw_mipi_dsi_rockchip_ops) > [ 2.843430] Rate 17000999, Parent Rate 17000000, Best Numerator 17018, Best Denominator 17017 > [ 2.891073] Rate 17001000, Parent Rate 17000000, Best Numerator 17001, Best Denominator 17000 > [ 2.891269] Rate 17001000, Parent Rate 17000000, Best Numerator 17001, Best Denominator 17000 > [ 2.891281] Rate 17001000, Parent Rate 17000000, Best Numerator 17001, Best Denominator 17000 > > I have tested the change extensively on my Odroid Go Advance (Rockchip > RK3326) and it appears to work well. However, this change will affect > all Rockchip SoCs that use this driver so I believe further testing > is warranted. Please note that without this change I can confirm > at least all PX30s with DSI panels will stop working with the 5.15 > kernel. > > Upon advice from Doug Anderson <dianders@chromium.org> it was decided > that we would first check if the clock rate can be set exactly as > requested, and only if it could not would we then add 999 to it and > attempt the process again. This way we can preserve the behavior for > clocks that still need it while resolving the specific issue for the > PX30 and DSI panels (since it is using a fractional clock). > > Changes since v2: > - Moved fixes to correct location. > > Changes since v1: > - Made the addition of 999 conditional based on whether the clock > subsystem can set the actual clock rate as requested. > - Updated the notes in the fixup routine to reflect this new behavior. > - Added reference to original commit, as this has technically been > broken since then however only now is it an issue due to the clock > changes. > > Fixes: 287422a95fe2 ("drm/rockchip: Round up _before_ giving to the clock framework") > > Signed-off-by: Chris Morgan <macromorgan@hotmail.com> Not worth making a v4 since it can be fixed when applying, but normally there is no blank line between the "Fixes" and "Signed-off-by" lines. Also, for trivial small fixes like this you're expected to carry forward reviews to the next versions. Here I'll re-add it for you, though: Reviewed-by: Douglas Anderson <dianders@chromium.org>
On Thu, Sep 16, 2021 at 03:29:07PM -0500, Chris Morgan wrote: > After commit 928f9e268611 ("clk: fractional-divider: Hide > clk_fractional_divider_ops from wide audience") was merged it appears What does that commit have to do with anything? Are you perhaps interested in its parent, where the rockchip clock driver was actually changed? 4e7cf74fa3b2 clk: fractional-divider: Export approximation algorithm to the CCF users I'm just trying to follow along what's going on here. Brian
On Thu, Sep 16, 2021 at 01:49:07PM -0700, Brian Norris wrote: > On Thu, Sep 16, 2021 at 03:29:07PM -0500, Chris Morgan wrote: > > After commit 928f9e268611 ("clk: fractional-divider: Hide > > clk_fractional_divider_ops from wide audience") was merged it appears > > What does that commit have to do with anything? Are you perhaps > interested in its parent, where the rockchip clock driver was actually > changed? It was part of a series of 4 commits, basically there were 2 commits from the series that had to be rolled back to make things work again and this was one of the two. Sorry for the confusion. It turns out that there was a change made some time ago (referenced by the Fixes:) that broke how the clock was getting set for a specific clock, but it wasn't until the patch series about the fractional dividers where it went from "works despite being broken in this limited case" to "doesn't work anymore in this limited case." Thank you. > > 4e7cf74fa3b2 clk: fractional-divider: Export approximation algorithm to the CCF users > > I'm just trying to follow along what's going on here. > > Brian
On Thu, 16 Sep 2021 15:29:07 -0500, Chris Morgan wrote: > After commit 928f9e268611 ("clk: fractional-divider: Hide > clk_fractional_divider_ops from wide audience") was merged it appears > that the DSI panel on my Odroid Go Advance stopped working. Upon closer > examination of the problem, it looks like it was the fixup in the > rockchip_drm_vop.c file was causing the issue. The changes made to the > clk driver appear to change some assumptions made in the fixup. > > [...] Applied, thanks! [1/1] drm/rockchip: Update crtc fixup to account for fractional clk change commit: e93a3ccf76996df53ff6c2c03626b70bb0b0d55d Best regards,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index ba9e14da41b4..a25b98b7f5bd 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -1174,26 +1174,24 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc, * * Action plan: * - * 1. When DRM gives us a mode, we should add 999 Hz to it. That way - * if the clock we need is 60000001 Hz (~60 MHz) and DRM tells us to - * make 60000 kHz then the clock framework will actually give us - * the right clock. + * 1. Try to set the exact rate first, and confirm the clock framework + * can provide it. * - * NOTE: if the PLL (maybe through a divider) could actually make - * a clock rate 999 Hz higher instead of the one we want then this - * could be a problem. Unfortunately there's not much we can do - * since it's baked into DRM to use kHz. It shouldn't matter in - * practice since Rockchip PLLs are controlled by tables and - * even if there is a divider in the middle I wouldn't expect PLL - * rates in the table that are just a few kHz different. + * 2. If the clock framework cannot provide the exact rate, we should + * add 999 Hz to the requested rate. That way if the clock we need + * is 60000001 Hz (~60 MHz) and DRM tells us to make 60000 kHz then + * the clock framework will actually give us the right clock. * - * 2. Get the clock framework to round the rate for us to tell us + * 3. Get the clock framework to round the rate for us to tell us * what it will actually make. * - * 3. Store the rounded up rate so that we don't need to worry about + * 4. Store the rounded up rate so that we don't need to worry about * this in the actual clk_set_rate(). */ - rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000 + 999); + rate = clk_round_rate(vop->dclk, adjusted_mode->clock * 1000); + if (rate / 1000 != adjusted_mode->clock) + rate = clk_round_rate(vop->dclk, + adjusted_mode->clock * 1000 + 999); adjusted_mode->clock = DIV_ROUND_UP(rate, 1000); return true;