Re: [PATCH v7 11/32] drm/xe: Move GGTT lock init to alloc

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Oct 07, 2025 at 01:47:27PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 10/7/2025 1:26 PM, Matthew Brost wrote:
> > The GGTT lock is needed very early during GT initialization for a VF;
> > move the GGTT lock initialization to the allocation phase.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_ggtt.c | 36 ++++++++++++++++++++++--------------
> >  1 file changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> > index 7fdd0a97a628..f7717057cd14 100644
> > --- a/drivers/gpu/drm/xe/xe_ggtt.c
> > +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> > @@ -159,6 +159,16 @@ static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
> >  	}
> >  }
> >  
> > +static void primelockdep(struct xe_ggtt *ggtt)
> > +{
> > +	if (!IS_ENABLED(CONFIG_LOCKDEP))
> > +		return;
> > +
> > +	fs_reclaim_acquire(GFP_KERNEL);
> > +	might_lock(&ggtt->lock);
> > +	fs_reclaim_release(GFP_KERNEL);
> > +}
> > +
> >  /**
> >   * xe_ggtt_alloc - Allocate a GGTT for a given &xe_tile
> >   * @tile: &xe_tile
> > @@ -170,8 +180,19 @@ static void xe_ggtt_clear(struct xe_ggtt *ggtt, u64 start, u64 size)
> >  struct xe_ggtt *xe_ggtt_alloc(struct xe_tile *tile)
> >  {
> >  	struct xe_ggtt *ggtt = drmm_kzalloc(&tile_to_xe(tile)->drm, sizeof(*ggtt), GFP_KERNEL);
> > -	if (ggtt)
> > +
> > +	if (ggtt) {
> > +		int err;
> > +
> >  		ggtt->tile = tile;
> > +
> > +		err = drmm_mutex_init(&tile_to_xe(tile)->drm, &ggtt->lock);
> > +		if (err)
> > +			ggtt = NULL;
> > +		else
> > +			primelockdep(ggtt);
> > +	}
> > +
> 
> all this looks over-complicated and against the rule to exit early on error, so maybe:
> 
> 	struct xe_device *xe = tile_to_xe(tile);
> 	struct xe_ggtt *ggtt;
> 
> 	ggtt = drmm_kzalloc(&xe->drm, sizeof(*ggtt), GFP_KERNEL);
> 	if (!ggtt)
> 		return NULL;
> 	if (drmm_mutex_init(&xe->drm, &ggtt->lock))
> 		return NULL;
> 	if (IS_ENABLED(CONFIG_LOCKDEP))
> 		primelockdep(ggtt);
> 
> 	ggtt->tile = tile;	
> 

Sure, I have CI issue so I need to respin this series now.

Matt

> >  	return ggtt;
> >  }
> >  
> > @@ -180,7 +201,6 @@ static void ggtt_fini_early(struct drm_device *drm, void *arg)
> >  	struct xe_ggtt *ggtt = arg;
> >  
> >  	destroy_workqueue(ggtt->wq);
> > -	mutex_destroy(&ggtt->lock);
> >  	drm_mm_takedown(&ggtt->mm);
> >  }
> >  
> > @@ -198,16 +218,6 @@ void xe_ggtt_might_lock(struct xe_ggtt *ggtt)
> >  }
> >  #endif
> >  
> > -static void primelockdep(struct xe_ggtt *ggtt)
> > -{
> > -	if (!IS_ENABLED(CONFIG_LOCKDEP))
> > -		return;
> > -
> > -	fs_reclaim_acquire(GFP_KERNEL);
> > -	might_lock(&ggtt->lock);
> > -	fs_reclaim_release(GFP_KERNEL);
> > -}
> > -
> >  static const struct xe_ggtt_pt_ops xelp_pt_ops = {
> >  	.pte_encode_flags = xelp_ggtt_pte_flags,
> >  	.ggtt_set_pte = xe_ggtt_set_pte,
> > @@ -227,8 +237,6 @@ static void __xe_ggtt_init_early(struct xe_ggtt *ggtt, u32 reserved)
> >  {
> >  	drm_mm_init(&ggtt->mm, reserved,
> >  		    ggtt->size - reserved);
> > -	mutex_init(&ggtt->lock);
> > -	primelockdep(ggtt);
> >  }
> >  
> >  int xe_ggtt_init_kunit(struct xe_ggtt *ggtt, u32 reserved, u32 size)
>