[PATCH 23/33] drm/vc4: hdmi: Move HDMI reset to pm_resume

[Dave Stevenson]

Hi Florian

On Tue, 9 Aug 2022 at 20:02, Florian Fainelli <f.fainelli at gmail.com> wrote:
>
> On 8/4/22 16:11, Florian Fainelli wrote:
> > On 6/13/22 07:47, Maxime Ripard wrote:
> >> From: Dave Stevenson <dave.stevenson at raspberrypi.com>
> >>
> >> The BCM2835-37 found in the RaspberryPi 0 to 3 have a power domain
> >> attached to the HDMI block, handled in Linux through runtime_pm.
> >>
> >> That power domain is shared with the VEC block, so even if we put our
> >> runtime_pm reference in the HDMI driver it would keep being on. If the
> >> VEC is disabled though, the power domain would be disabled and we would
> >> lose any initialization done in our bind implementation.
> >>
> >> That initialization involves calling the reset function and initializing
> >> the CEC registers.
> >>
> >> Let's move the initialization to our runtime_resume implementation so
> >> that we initialize everything properly if we ever need to.
> >>
> >> Fixes: c86b41214362 ("drm/vc4: hdmi: Move the HSM clock enable to
> >> runtime_pm")
> >> Signed-off-by: Dave Stevenson <dave.stevenson at raspberrypi.com>
> >> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> >
> > After seeing the same warning as Stefan reported in the link below, but
> > on the Raspberry Pi 4B:
> >
> > https://www.spinics.net/lists/dri-devel/msg354170.html
> >
> > a separate bisection effort led me to this commit, before is fine, after
> > produces 4 warnings during boot, see attached log.
> >
> > Is there a fix that we can try that would also cover the Raspberry Pi
> > 4B? Is it possible that this series precipitates the problem:
> >
> > https://www.spinics.net/lists/arm-kernel/msg984638.html
>
> Maxime, Dave, anything you would want me to try? Still seeing these
> warnings with net-next-6.0-11220-g15205c2829ca

Strange as we don't see this warning on the vendor kernel which is
doing exactly the same. We are largely still on 5.15 as LTS though, so
5.19 hasn't had much bashing in that regard.

Your callstack implies it's only sequencing.
vc4_hdmi_bind is manually calling vc4_hdmi_runtime_resume (and hence
initialising registers) before the call to pm_runtime_set_active and
pm_runtime_enable, hence the pm accounting check in vc4_hdmi_write
fails.

pm_runtime always seems like black magic to me :-/
Do we need to manually power up here, or can we call pm_runtime_enable
without touching the state, and then resume in the normal manner?
ie something simple like
pm_runtime_enable(dev);
pm_runtime_resume_and_get(dev);
The resume_and_get will call vc4_hdmi_runtime_resume and hence
initialise the block, but it will have sorted the pm accounting first.

Otherwise we mess with the order to be:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
ret = vc4_hdmi_runtime_resume(dev);
if (ret)
   goto err_put_ddc; //This error handling needs to be checked
pm_runtime_enable(dev);

I have no feel for which is the "correct" approach in terms of
pm_runtime, so will defer to others in that regard.

  Dave

> Would be nice to see those fixes before 6.0 final, thanks!
> --
> Florian