Re: [PATCH 1/2] drm/xe: Separate the d3cold and non-d3cold runtime PM handling

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

On Mon, Oct 07, 2024 at 09:28:52PM +0300, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre Deak
> Sent: Monday, October 7, 2024 7:06 AM
> To: intel-xe@lists.freedesktop.org; intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: [PATCH 1/2] drm/xe: Separate the d3cold and non-d3cold runtime PM handling
> >
> > For clarity separate the d3cold and non-d3cold runtime PM handling. The
> > only change in behavior is disabling polling later during runtime
> > resume. This shouldn't make a difference, since the poll disabling is
> > handled from a work, which could run at any point wrt. the runtime
> > resume handler. The work will also require a runtime PM reference,
> > syncing it with the resume handler.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> It seems a bit weird to me that we're enabling hpd polling during the suspend
> path and disabling it during the resume path, especially given the second patch's
> commit message mentioning that HPD interrupts are getting disabled in the
> suspend path.

Yes, the above summary is correct: interrupts must be disabled while the
device is runtime suspended, so polling should be used instead to detect
display hotplug events. Accordingly runtime resume reverses this,
disabling polling and enabling interrupts.

> However, I just looked, and it seems like this is the way it has been
> since the beginning, so I'm going to trust that it's supposed to be this way.
> 
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>

Thanks.

> -Jonathan Cavitt
> 
> > ---
> >  drivers/gpu/drm/xe/display/xe_display.c | 19 ++++++++++++++-----
> >  1 file changed, 14 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> > index ca00a365080fb..cb2449b7921ac 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -345,6 +345,9 @@ static void __xe_display_pm_suspend(struct xe_device *xe, bool runtime)
> >       intel_opregion_suspend(display, s2idle ? PCI_D1 : PCI_D3cold);
> >
> >       intel_dmc_suspend(display);
> > +
> > +     if (runtime && has_display(xe))
> > +             intel_hpd_poll_enable(xe);
> >  }
> >
> >  void xe_display_pm_suspend(struct xe_device *xe)
> > @@ -387,8 +390,10 @@ void xe_display_pm_runtime_suspend(struct xe_device *xe)
> >       if (!xe->info.probe_display)
> >               return;
> >
> > -     if (xe->d3cold.allowed)
> > +     if (xe->d3cold.allowed) {
> >               __xe_display_pm_suspend(xe, true);
> > +             return;
> > +     }
> >
> >       intel_hpd_poll_enable(xe);
> >  }
> > @@ -453,9 +458,11 @@ static void __xe_display_pm_resume(struct xe_device *xe, bool runtime)
> >               intel_display_driver_resume(xe);
> >               drm_kms_helper_poll_enable(&xe->drm);
> >               intel_display_driver_enable_user_access(xe);
> > -             intel_hpd_poll_disable(xe);
> >       }
> >
> > +     if (has_display(xe))
> > +             intel_hpd_poll_disable(xe);
> > +
> >       intel_opregion_resume(display);
> >
> >       if (!runtime)
> > @@ -474,10 +481,12 @@ void xe_display_pm_runtime_resume(struct xe_device *xe)
> >       if (!xe->info.probe_display)
> >               return;
> >
> > -     intel_hpd_poll_disable(xe);
> > -
> > -     if (xe->d3cold.allowed)
> > +     if (xe->d3cold.allowed) {
> >               __xe_display_pm_resume(xe, true);
> > +             return;
> > +     }
> > +
> > +     intel_hpd_poll_disable(xe);
> >  }
> >
> >
> > --
> > 2.44.2
> >
> >