Re: [PATCH] drm/xe/display: Remove hpd cancel work sync from runtime pm path

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

On Thu, Feb 13, 2025 at 03:04:44PM +0200, Upadhyay, Tejas wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Rodrigo
> > Vivi
> > Sent: Thursday, February 13, 2025 12:55 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Deak, Imre <imre.deak@intel.com>
> > Subject: [PATCH] drm/xe/display: Remove hpd cancel work sync from runtime
> > pm path
> >
> > This function will synchronously cancel and wait for many display work queue
> > items, which might try to take the runtime pm reference causing a bad
> > deadlock. So, remove it from the runtime_pm suspend patch.
> >
> > Reported-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/display/xe_display.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/xe/display/xe_display.c
> > b/drivers/gpu/drm/xe/display/xe_display.c
> > index 651799c946ac..f0f427689a56 100644
> > --- a/drivers/gpu/drm/xe/display/xe_display.c
> > +++ b/drivers/gpu/drm/xe/display/xe_display.c
> > @@ -311,7 +311,8 @@ static void __xe_display_pm_suspend(struct
> > xe_device *xe, bool runtime)
> >
> >       xe_display_flush_cleanup_work(xe);
> >
> > -     intel_hpd_cancel_work(xe);
> > +     if (!runtime)
> > +             intel_hpd_cancel_work(xe);
> 
> So the before suspend still need to make sure hpd work is cancelled smoothly, no?

Not sure what you mean (the patch is merged already). The point is that
there is no point in canceling them with the IRQs being enabled, because
right after canceling they could be rescheduled again (via a new IRQ).
And here the IRQs are still enabled. In addition, even if IRQs were
disabled, a synchornous cancel could lead to a deadlock as the commit
log explains. There is also no reason for canceling the hpd work
during runtime suspend.

> 
> Tejas
> >
> >       if (!runtime && has_display(xe)) {
> >               intel_display_driver_suspend_access(display);
> > --
> > 2.48.1
>