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

[Govindapillai, Vinod]

On Fri, 2024-08-23 at 12:08 +0300, Govindapillai, Vinod wrote:
> 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.

I Missed this earlier! Okay! Can do this before pushing the change if RB-ed

+Corrected Ville's email Id...

Vinod

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