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

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

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

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