Re: [PATCH] drm/xe: Add missing runtime outer protection

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Thu, 2024-05-30 at 05:42 +0000, Matthew Brost wrote:
> On Wed, May 29, 2024 at 05:56:03PM -0400, Rodrigo Vivi wrote:
> > TTM BO destroy is called unlocked as a ref count worker when it
> > gets 0 users. When that happens we could be runtime suspended,
> > and waking up from inner locked places like ggtt_remove_node,
> > could potentially lead to deadlocks. Our warning system against
> > this case hit this case:
> > 
> > [ 2295.891269] xe 0000:03:00.0: Missing outer runtime PM protection
> > [snip]
> > [ 2295.891604]  ? xe_pm_runtime_get_noresume+0x5c/0x70 [xe]
> > [ 2295.891717]  ? report_bug+0x18d/0x1c0
> > [ 2295.891722]  ? handle_bug+0x3c/0x80
> > [ 2295.891724]  ? exc_invalid_op+0x13/0x60
> > [ 2295.891726]  ? asm_exc_invalid_op+0x16/0x20
> > [ 2295.891730]  ? xe_pm_runtime_get_noresume+0x5c/0x70 [xe]
> > [ 2295.891816]  xe_ggtt_remove_node+0x93/0xf0 [xe]
> > [ 2295.891870]  xe_ttm_bo_destroy+0xe9/0xf0 [xe]
> > [ 2295.891935]  process_one_work+0x225/0x730
> > [ 2295.891940]  worker_thread+0x1d8/0x3c0
> > 
> > Add this outer protection to avoid any potential deadlock.
> > 
> > Reported-by: Paulo Zanoni <paulo.zanoni@intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_bo.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c
> > b/drivers/gpu/drm/xe/xe_bo.c
> > index 2bae01ce4e5b..a902f23bec0c 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1066,6 +1066,8 @@ static void xe_ttm_bo_destroy(struct
> > ttm_buffer_object *ttm_bo)
> >  	struct xe_bo *bo = ttm_to_xe_bo(ttm_bo);
> >  	struct xe_device *xe = ttm_to_xe_device(ttm_bo->bdev);
> >  
> > +	xe_pm_runtime_get(xe);
> 
> Should we only do this if we are in a kthread? i.e. !current->mm

First, I think the xe_ggtt_remove_node() should be moved to
delete_mem_notify(), because all backing memory is released at that
point.

But I guess this needs to be a bit carefully considered. 
First, if we only do this from a kthread, then all instances of
xe_bo_put() needs to have a runtime pm ref, but OTOH putting a bo from
reclaim context would, if calling runtime_pm_get synchronously case a
lockdep splat?

I figure we need something like

if (xe_pm_runtime_get_if_active() || (current->flags & PF_WQ_WORKER))
	xe_ggtt_remove_node()
else
	remove_ggtt_node_from_worker()


Unless we can queue ggtt manipulations if device inactive and only
execute them at wakeup.

/Thomas

> 
> Matt
> 
> > +
> >  	if (bo->ttm.base.import_attach)
> >  		drm_prime_gem_destroy(&bo->ttm.base, NULL);
> >  	drm_gem_object_release(&bo->ttm.base);
> > @@ -1089,6 +1091,8 @@ static void xe_ttm_bo_destroy(struct
> > ttm_buffer_object *ttm_bo)
> >  	mutex_unlock(&xe->mem_access.vram_userfault.lock);
> >  
> >  	kfree(bo);
> > +
> > +	xe_pm_runtime_put(xe);
> >  }
> >  
> >  static void xe_gem_object_free(struct drm_gem_object *obj)
> > -- 
> > 2.45.1
> >