Re: [PATCH 2/3] drm/xe: Allow the pm notifier to continue on failure

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Aug 29, 2025 at 04:50:01PM +0100, Matthew Auld wrote:
> On 29/08/2025 12:33, Thomas Hellström wrote:
> > Its actions are opportunistic anyway and will be completed
> > on device suspend.
> > 
> > Also restrict the scope of the pm runtime reference to
> > the notifier callback itself to make it easier to
> > follow.
> > 
> > Marking as a fix to simplify backporting of the fix
> > that follows in the series.
> > 
> > Fixes: c6a4d46ec1d7 ("drm/xe: evict user memory in PM notifier")
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: <stable@vger.kernel.org> # v6.16+
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_pm.c | 14 ++++----------
> >   1 file changed, 4 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index a2e85030b7f4..b57b46ad9f7c 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.c
> > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > @@ -308,28 +308,22 @@ static int xe_pm_notifier_callback(struct notifier_block *nb,
> >   	case PM_SUSPEND_PREPARE:
> >   		xe_pm_runtime_get(xe);
> >   		err = xe_bo_evict_all_user(xe);
> > -		if (err) {
> > +		if (err)
> >   			drm_dbg(&xe->drm, "Notifier evict user failed (%d)\n", err);
> > -			xe_pm_runtime_put(xe);
> > -			break;
> > -		}
> >   		err = xe_bo_notifier_prepare_all_pinned(xe);
> > -		if (err) {
> > +		if (err)
> >   			drm_dbg(&xe->drm, "Notifier prepare pin failed (%d)\n", err);
> > -			xe_pm_runtime_put(xe);
> > -		}
> > +		xe_pm_runtime_put(xe);
> 
> IIRC I was worried that this ends up triggering runtime suspend at some
> later point and then something wakes it up again before the actual forced
> suspend triggers, which looks like it would undo all the
> prepare_all_pinned() work, so I opted for keeping it held over the entire
> sequence. Is that not a concern here?

I was seeing this more like a umbrella to shut-up our inner callers warnings
since runtime pm references will be taken prior to pm state transitions
by base/power.

However on a quick look I couldn't connect the core code that takes the
runtime pm directly with this notify now. So, perhaps let's indeed play
safe and keep our own references here?!...

> 
> >   		break;
> >   	case PM_POST_HIBERNATION:
> >   	case PM_POST_SUSPEND:
> > +		xe_pm_runtime_get(xe);
> >   		xe_bo_notifier_unprepare_all_pinned(xe);
> >   		xe_pm_runtime_put(xe);
> >   		break;
> >   	}
> > -	if (err)
> > -		return NOTIFY_BAD;
> > -
> >   	return NOTIFY_DONE;
> >   }
>