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 05:30 +0000, Murthy, Arun R wrote:

Hi,

Thanks for the review!

> > -----Original Message-----
> > From: Govindapillai, Vinod <vinod.govindapillai@intel.com>
> > Sent: Tuesday, August 20, 2024 10:44 PM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>; Vivi,
> > Rodrigo <rodrigo.vivi@intel.com>; Shankar, Uma <uma.shankar@intel.com>;
> > ville.syrala@intel.com
> > Subject: [PATCH v3 2/3] drm/xe: Handle polling only for system s/r in
> > xe_display_pm_suspend/resume()
> > 
> > From: Imre Deak <imre.deak@intel.com>
> > 
> > This is a preparation for the follow-up patch where polling will be handled
> > properly for all cases during runtime suspend/resume.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > ---
> >  drivers/gpu/drm/xe/display/xe_display.c | 19 +++++++------------
> >  1 file changed, 7 insertions(+), 12 deletions(-)
> > 
> > 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? 

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

BR
Vinod

> 
> Thanks and Regards,
> Arun R Murthy
> -------------------
> 
> >         }
> > -       intel_hpd_poll_disable(xe);
> > 
> >         intel_opregion_resume(display);
> > 
> > --
> > 2.34.1
>