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

[Imre Deak <imre.deak@intel.com>]

On Fri, Aug 23, 2024 at 11:43:47AM +0300, 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.

Not sure what you mean by this. i915 (and xe) does use HPD polling to
detect display hotplug events. Interrupts for this work only if the
given output type has a physical HPD interrupt signal for this (so for
DP, HDMI yes, but for TV and some VGA outputs no) and the device is not
runtime suspended. Because of the former reason (outputs w/o an HPD
interrupt signal) the polling functionality provided by DRM core is
enabled while the system is not suspended (and so gets disabled here
during system suspend). Because of the latter reason (interrupts
disabled when the device is runtime suspended) polling for output types
that have an HPD interrupt signal is enabled when runtime suspending and
for these same outputs polling is disabled (switched back to interrupt
based detection) when runtime resuming.

> 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.
>
> > >
> > > > -               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
> -------------------