• maxime@cerno.tech's avatar
    drm/vc4: hdmi: Fix HSM clock too low on Pi4 · 3bc6a37f
    maxime@cerno.tech authored
    Commit ae71ab58 ("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.
    
    Link: https://bugzilla.redhat.com/show_bug.cgi?id=2136234
    Fixes: ae71ab58 ("drm/vc4: hdmi: Enforce the minimum rate at runtime_resume")
    Reported-by: default avatarPeter Robinson <pbrobinson@gmail.com>
    Reviewed-by: default avatarJavier Martinez Canillas <javierm@redhat.com>
    Tested-by: default avatarPeter Robinson <pbrobinson@gmail.com>
    Link: https://lore.kernel.org/r/20221021131339.2203291-1-maxime@cerno.techSigned-off-by: default avatarMaxime Ripard <maxime@cerno.tech>
    3bc6a37f
vc4_hdmi.h 7.08 KB