[PATCH v3 2/3] drm/xe: Handle polling only for system s/r in xe_display_pm_suspend/resume()

[Murthy, Arun R]

> -----Original Message-----
> From: Govindapillai, Vinod <vinod.govindapillai at intel.com>
> Sent: Friday, August 23, 2024 2:38 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-xe at lists.freedesktop.org
> Cc: Shankar, Uma <uma.shankar at intel.com>; Vivi, Rodrigo
> <rodrigo.vivi at intel.com>; Deak, Imre <imre.deak at intel.com>;
> ville.syrala at intel.com
> Subject: Re: [PATCH v3 2/3] drm/xe: Handle polling only for system s/r in
> xe_display_pm_suspend/resume()
> 
> On Fri, 2024-08-23 at 08:43 +0000, Murthy, Arun R wrote:
> > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display.c
> > > > > b/drivers/gpu/drm/xe/display/xe_display.c
> > > > > index ad7fc5137b42..b2a0b4b5c45c 100644
> > > > > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > > > > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > > > > @@ -320,15 +320,13 @@ void xe_display_pm_suspend(struct
> > > > > xe_device *xe, bool runtime)
> > > > >          * properly.
> > > > >          */
> > > > >         intel_power_domains_disable(xe);
> > > > > +
> > > > Un-necessary change.
> > > >
> > > > >         intel_fbdev_set_suspend(&xe->drm,
> > > > > FBINFO_STATE_SUSPENDED, true);
> > > > > -       if (has_display(xe)) {
> > > > > +       if (!runtime && has_display(xe)) {
> > > > >                 drm_kms_helper_poll_disable(&xe->drm);
> > > >
> > > > Can we get rid of this API as we don't reply on device polling and
> > > > use interrupt
> > > based.
> > >
> > > Not sure if I understand the point! Wonder if it is relevant at this
> > > context! But as mentioned in the series patch description, there
> > > could be few other steps missing in the xe runtime_suspend handling
> > > and a better refactoring/changes are being planned. So I guess you could
> take this up at that time?
> > >
> > Nothing missing as such, rather this
> function(drm_kms_helper_poll_enable/disable) is not required.
> > I915 uses interrupt based detection and not polling. Even if called
> > when polling is not enabled, will have no impact and just return with warning.
> > If to be taken later can we have a TODO or Re-visit so that we don’t miss.
> 
> "While xe_display_pm_suspend/resume() performs steps during runtime
> suspend/resume that shouldn't happen, like suspending MST and they are
> missing other steps like enabling DC9, this patchset is meant to keep the current
> behavior wrt. these, leaving the corresponding updates for a follow-up"
> 
> "drm_kms_helper_poll_init/drm_kms_helper_poll_disable/drm_kms_helper_po
> ll_enable" if you feel is not needed, i guess it should be taken up as separate
> task. I don't think a adding a TODO in this patch / series context is required.
> 
Apart from this patch looks good to me.

Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>

Thanks and Regards,
Arun R Murthy
--------------------

> BR
> Vinod
> 
> >
> > > >
> > > > > -               if (!runtime)
> > > > > -
> > > > > intel_display_driver_disable_user_access(xe);
> > > > > -       }
> > > > > -
> > > > > -       if (!runtime)
> > > > > +               intel_display_driver_disable_user_access(xe);
> > > > >                 intel_display_driver_suspend(xe);
> > > > > +       }
> > > > >
> > > > >         xe_display_flush_cleanup_work(xe);
> > > > >
> > > > > @@ -387,15 +385,12 @@ void xe_display_pm_resume(struct xe_device
> > > > > *xe, bool runtime)
> > > > >
> > > > >         /* MST sideband requires HPD interrupts enabled */
> > > > >         intel_dp_mst_resume(xe);
> > > > > -       if (!runtime)
> > > > > +       if (!runtime && has_display(xe)) {
> > > > >                 intel_display_driver_resume(xe);
> > > > > -
> > > > > -       if (has_display(xe)) {
> > > > >                 drm_kms_helper_poll_enable(&xe->drm);
> > > > > -               if (!runtime)
> > > > > -
> > > > > intel_display_driver_enable_user_access(xe);
> > > > > +               intel_display_driver_enable_user_access(xe);
> > > > > +               intel_hpd_poll_disable(xe);
> > > > Do we need this disable here as we are enabling this only in
> > > > xe_display_pm_runtime_suspend() and hence disable only in
> > > > xe_display_pm_runtime_resume()
> > >
> > > To quote Imre,
> > >
> > > "intel_hpd_poll_disable() is needed during system resume as well,
> > > since it does an explicit connector probing. This probing is needed
> > > also when you resume from S3 etc, for monitors that got connected
> > > during the system was suspended."
> > >
> > Thanks for the clarification.
> >
> > Thanks and Regards,
> > Arun R Murthy
> > -------------------