Hi Javier,
On Thu, Oct 27, 2022 at 05:25:49PM +0200, Javier Martinez Canillas wrote:
> On 10/21/22 15:13, maxime@xxxxxxxxxx wrote:
> > Commit ae71ab585c81 ("drm/vc4: hdmi: Enforce the minimum rate at
> > runtime_resume") reintroduced the call to clk_set_min_rate in an attempt
> > to fix the boot without a monitor connected on the RaspberryPi3.
> >
> > However, that introduced a regression breaking the display output
> > entirely (black screen but no vblank timeout) on the Pi4.
> >
> > This is due to the fact that we now have in a typical modeset at boot,
> > in vc4_hdmi_encoder_pre_crtc_configure(), we have a first call to
> > clk_set_min_rate() asking for the minimum rate of the HSM clock for our
> > given resolution, and then a call to pm_runtime_resume_and_get(). We
> > will thus execute vc4_hdmi_runtime_resume() which, since the commit
> > mentioned above, will call clk_set_min_rate() a second time with the
> > absolute minimum rate we want to enforce on the HSM clock.
> >
> > We're thus effectively erasing the minimum mandated by the mode we're
> > trying to set. The fact that only the Pi4 is affected is due to the fact
> > that it uses a different clock driver that tries to minimize the HSM
> > clock at all time. It will thus lower the HSM clock rate to 120MHz on
> > the second clk_set_min_rate() call.
> >
> > The Pi3 doesn't use the same driver and will not change the frequency on
> > the second clk_set_min_rate() call since it's still within the new
> > boundaries and it doesn't have the code to minimize the clock rate as
> > needed. So even though the boundaries are still off, the clock rate is
> > still the right one for our given mode, so everything works.
> >
> > There is a lot of moving parts, so I couldn't find any obvious
> > solution:
> >
> > - Reverting the original is not an option, as that would break the Pi3
> > again.
> >
> > - We can't move the clk_set_min_rate() call in _pre_crtc_configure()
> > since because, on the Pi3, the HSM clock has the CLK_SET_RATE_GATE
> > flag which prevents the clock rate from being changed after it's
> > been enabled. Our calls to clk_set_min_rate() can change it, so they
> > need to be done before clk_prepare_enable().
> >
> > - We can't remove the call to clk_prepare_enable() from the
> > runtime_resume hook to put it into _pre_crtc_configure() either,
> > since we need that clock to be enabled to access the registers, and
> > we can't count on the fact that the display will be active in all
> > situations (doing any CEC operation, or listing the modes while
> > inactive are valid for example()).
> >
> > - We can't drop the call to clk_set_min_rate() in
> > _pre_crtc_configure() since we would need to still enforce the
> > minimum rate for a given resolution, and runtime_resume doesn't have
> > access to the current mode, if there's any.
> >
> > - We can't copy the TMDS character rate into vc4_hdmi and reuse it
> > since, because it's part of the KMS atomic state, it needs to be
> > protected by a mutex. Unfortunately, some functions (CEC operations,
> > mostly) can be reentrant (through the CEC framework) and still need
> > a pm_runtime_get.
> >
> > However, we can work around this issue by leveraging the fact that the
> > clk_set_min_rate() calls set boundaries for its given struct clk, and
> > that each different clk_get() call will return a different instance of
> > struct clk. The clock framework will then aggregate the boundaries for
> > each struct clk instances linked to a given clock, plus its hardware
> > boundaries, and will use that.
> >
> > We can thus get an extra HSM clock user for runtime_pm use only, and use
> > our different clock instances depending on the context: runtime_pm will
> > use its own to set the absolute minimum clock setup so that we never
> > lock the CPU waiting for a register access, and the modeset part will
> > set its requirement for the current resolution. And we let the CCF do
> > the coordination.
> >
> > It's not an ideal solution, but it's fairly unintrusive and doesn't
> > really change any part of the logic so it looks like a rather safe fix.
> >
>
> What a great commit message. I learned a couple of things from it :)
>
> [...]
>
> Maybe adding some comments here explaining why two instances of the
> same clock are getting by the driver? It's very clear after reading
> your commit message, but it may not be for someone looking the code.
Yeah, possibly. I have every intention on switching to the same clock
driver for the Pi 0-3 and Pi4 (patches have been sent already), and
reverting that commit and a few others in a few releases.
I've already tested that everything works with the firmware clocks + the
reverts, so it's just a matter of letting enough time for everybody to
catch up. So that patch is just transitory and is just meant to fix the
code right now.
Maxime
Attachment:
signature.asc
Description: PGP signature
