Re: [PATCH] drm/xe: Use separate rpm lockdep map for non-d3cold-capable devices

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

On Fri, 2024-08-23 at 17:15 +0200, Thomas Hellström wrote:
> Hi, Rodrigo.
> 
> On Fri, 2024-08-23 at 10:43 -0400, Rodrigo Vivi wrote:
> > On Fri, Aug 23, 2024 at 03:59:06PM +0200, Thomas Hellström wrote:
> > 
> > Hi Thomas, first of all, please notice that you used the wrong
> > address from our mailing list ;)
> 
> You're right. Well it's friday afternoon here...
> 
> > 
> > but a few more comments below...
> > 
> > > For non-d3cold-capable devices we'd like to be able to wake up
> > > the
> > > device from reclaim. In particular, for Lunar Lake we'd like to
> > > be
> > > able to blit CCS metadata to system at shrink time; at least from
> > > kswapd where it's reasonable OK to wait for rpm resume and a
> > > preceding rpm suspend.
> > > 
> > > Therefore use a separate lockdep map for such devices and prime
> > > it
> > > reclaim-tainted.
> > 
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > 
> > > 
> > > Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
> > > Signed-off-by: Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pm.c | 45 +++++++++++++++++++++++++++++++-
> > > --
> > > ----
> > >  1 file changed, 37 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c
> > > b/drivers/gpu/drm/xe/xe_pm.c
> > > index 9f3c14fd9f33..2e9fdb5da8bb 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > @@ -70,11 +70,29 @@
> > >   */
> > >  
> > >  #ifdef CONFIG_LOCKDEP
> > > -static struct lockdep_map xe_pm_runtime_lockdep_map = {
> > > -	.name = "xe_pm_runtime_lockdep_map"
> > > +static struct lockdep_map xe_pm_runtime_d3cold_map = {
> > > +	.name = "xe_rpm_d3cold_map"
> > > +};
> > > +
> > > +static struct lockdep_map xe_pm_runtime_nod3cold_map = {
> > > +	.name = "xe_rpm_nod3cold_map"
> > >  };
> > >  #endif
> > >  
> > > +static void xe_pm_runtime_acquire(const struct xe_device *xe)
> > 
> > I believe this should have a different name.
> > runtime_acquire and runtime_release sounds like runtime_get and
> > runtime_put...
> > 
> > since it is static perhaps we should only call it
> > xe_rpm_lockmap_acquire
> > and xe_rpm_lockmap_release
> 
> Sure, I'll fix
> 
> > 
> > or
> > d3_lockmap_acquire
> > d3_lockmap_release ?
> > 
> > or something like that...
> > 
> > > +{
> > > +	lock_map_acquire(xe->d3cold.capable ?
> > > +			 &xe_pm_runtime_d3cold_map :
> > > +			 &xe_pm_runtime_nod3cold_map);
> > > +}
> > > +
> > > +static void xe_pm_runtime_release(const struct xe_device *xe)
> > > +{
> > > +	lock_map_release(xe->d3cold.capable ?
> > > +			 &xe_pm_runtime_d3cold_map :
> > > +			 &xe_pm_runtime_nod3cold_map);
> > > +}
> > > +
> > >  /**
> > >   * xe_pm_suspend - Helper for System suspend, i.e. S0->S3 / S0-
> > > > S2idle
> > >   * @xe: xe device instance
> > > @@ -354,7 +372,7 @@ int xe_pm_runtime_suspend(struct xe_device
> > > *xe)
> > >  	 * annotation here and in xe_pm_runtime_get() lockdep
> > > will
> > > see
> > >  	 * the potential lock inversion and give us a nice
> > > splat.
> > >  	 */
> > > -	lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > > +	xe_pm_runtime_acquire(xe);
> > >  
> > >  	/*
> > >  	 * Applying lock for entire list op as xe_ttm_bo_destroy
> > > and xe_bo_move_notify
> > > @@ -386,7 +404,7 @@ int xe_pm_runtime_suspend(struct xe_device
> > > *xe)
> > >  out:
> > >  	if (err)
> > >  		xe_display_pm_resume(xe, true);
> > > -	lock_map_release(&xe_pm_runtime_lockdep_map);
> > > +	xe_pm_runtime_release(xe);
> > >  	xe_pm_write_callback_task(xe, NULL);
> > >  	return err;
> > >  }
> > > @@ -407,7 +425,7 @@ int xe_pm_runtime_resume(struct xe_device
> > > *xe)
> > >  	/* Disable access_ongoing asserts and prevent recursive
> > > pm
> > > calls */
> > >  	xe_pm_write_callback_task(xe, current);
> > >  
> > > -	lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > > +	xe_pm_runtime_acquire(xe);
> > >  
> > >  	if (xe->d3cold.allowed) {
> > >  		err = xe_pcode_ready(xe, true);
> > > @@ -437,7 +455,7 @@ int xe_pm_runtime_resume(struct xe_device
> > > *xe)
> > >  			goto out;
> > >  	}
> > >  out:
> > > -	lock_map_release(&xe_pm_runtime_lockdep_map);
> > > +	xe_pm_runtime_release(xe);
> > >  	xe_pm_write_callback_task(xe, NULL);
> > >  	return err;
> > >  }
> > > @@ -458,8 +476,19 @@ int xe_pm_runtime_resume(struct xe_device
> > > *xe)
> > >   */
> > >  static void pm_runtime_lockdep_prime(void)
> > >  {
> > > -	lock_map_acquire(&xe_pm_runtime_lockdep_map);
> > > -	lock_map_release(&xe_pm_runtime_lockdep_map);
> > > +	struct dma_resv lockdep_resv;
> > > +
> > > +	dma_resv_init(&lockdep_resv);
> > > +	lock_map_acquire(&xe_pm_runtime_d3cold_map);
> > > +	/* D3Cold takes the dma_resv locks to evict bos */
> > > +	dma_resv_lock(&lockdep_resv, NULL);
> > > +	fs_reclaim_acquire(GFP_KERNEL);
> > > +	/* Shrinkers like to wake up the device under reclaim.
> > > */
> > > +	lock_map_acquire(&xe_pm_runtime_nod3cold_map);
> > > +	lock_map_release(&xe_pm_runtime_nod3cold_map);
> > > +	fs_reclaim_release(GFP_KERNEL);
> > > +	dma_resv_unlock(&lockdep_resv);
> > > +	lock_map_release(&xe_pm_runtime_d3cold_map);
> > 
> > do we really need this entire sequence? or checking for d3capable
> > here could have 2 different smaller sequences?
> 
> Hm. I forgot to check when this function was called. I was assuming
> it
> was called only once per driver instance and in that case we need it
> all since we can have multiple devices with different capabilities,
> but
> it seems to be called on each runtime_get(). Is that intentional?

Otherwise I'd like to change that to be called at module init?

/Thomas



> 
> /Thomas
> 
> 
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.44.0
> > > 
>