Re: [PATCH] drm/bridge: anx7625: Use pm_runtime_force_{suspend, resume}

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



)

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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux