Re: [PATCH] drm/bridge: anx7625: Use pm_runtime_force_{suspend,resume}
From: Pi-Hsun Shih
Date: Fri Jul 16 2021 - 04:27:26 EST
)
On Wed, Jul 14, 2021 at 6:32 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Wed, Jul 14, 2021 at 02:01:59PM +0800, Pi-Hsun Shih wrote:
> > Use pm_runtime_force_suspend and pm_runtime_force_resume to ensure that
> > anx7625 would always be powered off when suspended. Also update the
> > bridge enable hook to always ensure that the anx7625 is powered on
> > before starting DP operations.
> >
> > Fixes: 409776fa3c42 ("drm/bridge: anx7625: add suspend / resume hooks")
> >
> > Signed-off-by: Pi-Hsun Shih <pihsun@xxxxxxxxxxxx>
> >
> > ---
> >
> > An issue was found that the anx7625 driver won't power off when used as
> > eDP bridge on Asurada board if suspend is entered via VT2.
> >
> > The reason is that in this case, anx7625_suspend won't power off anx7625
> > (since intp_irq is not set). And anx7625_bridge_disable is only called
> > indirectly by other driver's (mediatek-drm) suspend.
> > pm_runtime_put_sync won't do anything since it's already in system
> > suspend.
> >
> > If not in VT2, the bridge disable is indirectly called when Chrome
> > stops, so anx7625 will be powered off correctly.
> >
> > To fix the issue, the suspend resume hooks are changed to
> > pm_runtime_force_{suspend,resume} to ensure the runtime suspend / resume
> > is always called correctly when system suspend / resume.
> > (Note that IRQ no longer needs to be disabled on suspend after commit
> > f03ab6629c7b ("drm/bridge: anx7625: Make hpd workqueue freezable"))
> >
> > Since bridge disable is called indirectly by mediatek-drm driver's
> > suspend, it might happens after anx7625 suspend is called. So a check
> > if the driver is already suspended via pm_runtime_force_suspend is also
> > added, to ensure that the anx7625_dp_stop won't be called when power
> > is off. And also since bridge enable might happens before anx7625 resume
> > is called, a check to that is also added, and would force resume the
> > device in this case.
> >
> > I'm not sure if the approach to fix this is the most appropriate way,
> > since using pm_runtime_force_resume in bridge enable kinda feels hacky
> > to me. I'm open to any suggestions.
>
> I thought the real fix was to create device links between the bridge and
> the other parts of the overall drm driver, so that the driver core can
> resume devices in the right order.
>
> Unfortunately those device link patches haven't made it in yet. Quick
> search on lore didn't find anything, maybe I was just dreaming, or maybe
> the patches only existed for panels.
>
> Either way, this is a drm_bridge.c problem that needs to be fixed there,
> not individually in each driver.
> -Daniel
Hi,
Thanks for the response, I did find some discussion about this in 2018 for
drm_panel
(https://patchwork.kernel.org/project/dri-devel/patch/b53584fd988d045c13de22d81825395b0ae0aad7.1524727888.git.jsarha@xxxxxx/),
which also mentioned drm_bridge.
>From that thread it seems that linking all bridges with the previous one would
break some drivers, and there was no conclusion on how this should be done.
I have some ideas on how to solve this issue for the anx7625 driver without
affecting other drivers, are patches that do one of the following acceptable?
* Add some opt-in flag to drm_bridge which, if set, would create a stateless
device link between the bridge and the encoder in drm_bridge_attach. And use
the flag in anx7625 driver.
* Add the stateless device link in the anx7625 driver inside
anx7625_bridge_attach
(We can remove the link if a general solution for drm_bridge comes out later).
Or is it still preferred to have some general solution in drm_bridge without
explicit opt-in?
Regards,
Pi-Hsun
>
> >
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 55 +++++++++--------------
> > 1 file changed, 20 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index a3d82377066b..9d0f5dc88b16 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -1559,7 +1559,20 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
> >
> > DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
> >
> > - pm_runtime_get_sync(dev);
> > + /*
> > + * The only case where pm_runtime is disabled here is when the function
> > + * is called other driver's resume hook by
> > + * drm_mode_config_helper_resume, but when the pm_runtime_force_resume
> > + * hasn't been called on this device.
> > + *
> > + * pm_runtime_get_sync won't power on anx7625 in this case since we're
> > + * in system resume, so instead we force resume anx7625 to make sure
> > + * the following anx7625_dp_start would succeed.
> > + */
> > + if (pm_runtime_enabled(dev))
> > + pm_runtime_get_sync(dev);
> > + else
> > + pm_runtime_force_resume(dev);
> >
> > anx7625_dp_start(ctx);
> > }
> > @@ -1571,9 +1584,10 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
> >
> > DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
> >
> > - anx7625_dp_stop(ctx);
> > -
> > - pm_runtime_put_sync(dev);
> > + if (pm_runtime_enabled(dev)) {
> > + anx7625_dp_stop(ctx);
> > + pm_runtime_put_sync(dev);
> > + }
> > }
> >
> > static enum drm_connector_status
> > @@ -1705,38 +1719,9 @@ static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
> > return 0;
> > }
> >
> > -static int __maybe_unused anx7625_resume(struct device *dev)
> > -{
> > - struct anx7625_data *ctx = dev_get_drvdata(dev);
> > -
> > - if (!ctx->pdata.intp_irq)
> > - return 0;
> > -
> > - if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev)) {
> > - enable_irq(ctx->pdata.intp_irq);
> > - anx7625_runtime_pm_resume(dev);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > -static int __maybe_unused anx7625_suspend(struct device *dev)
> > -{
> > - struct anx7625_data *ctx = dev_get_drvdata(dev);
> > -
> > - if (!ctx->pdata.intp_irq)
> > - return 0;
> > -
> > - if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev)) {
> > - anx7625_runtime_pm_suspend(dev);
> > - disable_irq(ctx->pdata.intp_irq);
> > - }
> > -
> > - return 0;
> > -}
> > -
> > static const struct dev_pm_ops anx7625_pm_ops = {
> > - SET_SYSTEM_SLEEP_PM_OPS(anx7625_suspend, anx7625_resume)
> > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > + pm_runtime_force_resume)
> > SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
> > anx7625_runtime_pm_resume, NULL)
> > };
> >
> > base-commit: c0d438dbc0b74901f1901d97a6c84f38daa0c831
> > --
> > 2.32.0.93.g670b81a890-goog
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch