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

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

[Thomas Hellström]

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?

/Thomas


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

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

[Thomas Hellström]

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
> > > 
> 

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

[Rodrigo Vivi]

On Fri, Aug 23, 2024 at 05:20:52PM +0200, Thomas Hellström wrote:
> 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?

Probably my mistake when refactoring it. I believe it makes sense to
move that to the init function and then keep as it is now here.

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

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

[Matthew Auld]

On 23/08/2024 16:20, Thomas Hellström wrote:
> 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?

hmm, don't we want to record the locks that are held when calling into 
the sync rpm get? We then make sure those locks are never grabbed in 
either of the callbacks. At least that was the original intention.

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

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

[Thomas Hellström]

On Fri, 2024-08-23 at 16:40 +0100, Matthew Auld wrote:
> On 23/08/2024 16:20, Thomas Hellström wrote:
> > 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?
> 
> hmm, don't we want to record the locks that are held when calling
> into 
> the sync rpm get? We then make sure those locks are never grabbed in 
> either of the callbacks. At least that was the original intention.

Ah, yes, so it's more of a might_lock() type of function rather than
priming? I should have noticed since it really didn't explicitly order
any locks....

Would it be ok then If I added a new module_init prime function that
looks like the prime function in the patch, and rename the existing
prime to something like

xe_might_enter_rpm_callback()?

/Thomas





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

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

[Matthew Auld]

On 23/08/2024 16:54, Thomas Hellström wrote:
> On Fri, 2024-08-23 at 16:40 +0100, Matthew Auld wrote:
>> On 23/08/2024 16:20, Thomas Hellström wrote:
>>> 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?
>>
>> hmm, don't we want to record the locks that are held when calling
>> into
>> the sync rpm get? We then make sure those locks are never grabbed in
>> either of the callbacks. At least that was the original intention.
> 
> Ah, yes, so it's more of a might_lock() type of function rather than
> priming? I should have noticed since it really didn't explicitly order
> any locks....
> 
> Would it be ok then If I added a new module_init prime function that
> looks like the prime function in the patch, and rename the existing
> prime to something like
> 
> xe_might_enter_rpm_callback()?

Sure, that sounds better than prime.

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

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

[Thomas Hellström]

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.

v2:
- Rename lockmap acquire- and release functions. (Rodrigo Vivi).
- Reinstate the old xe_pm_runtime_lockdep_prime() function and
  rename it to xe_rpm_might_enter_cb(). (Matthew Auld).
- Introduce a separate xe_pm_runtime_lockdep_prime function
  called from module init for known required locking orders.

Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "Auld, Matthew" <matthew.auld@intel.com>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/xe/xe_pm.c | 79 +++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/xe/xe_pm.h |  1 +
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
index 9f3c14fd9f33..747fb96453d3 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_rpm_lockmap_acquire(const struct xe_device *xe)
+{
+	lock_map_acquire(xe->d3cold.capable ?
+			 &xe_pm_runtime_d3cold_map :
+			 &xe_pm_runtime_nod3cold_map);
+}
+
+static void xe_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_release(xe);
 	xe_pm_write_callback_task(xe, NULL);
 	return err;
 }
@@ -451,15 +469,37 @@ int xe_pm_runtime_resume(struct xe_device *xe)
  * stuff that can happen inside the runtime_resume callback by acquiring
  * a dummy lock (it doesn't protect anything and gets compiled out on
  * non-debug builds).  Lockdep then only needs to see the
- * xe_pm_runtime_lockdep_map -> runtime_resume callback once, and then can
- * hopefully validate all the (callers_locks) -> xe_pm_runtime_lockdep_map.
+ * xe_pm_runtime_xxx_map -> runtime_resume callback once, and then can
+ * hopefully validate all the (callers_locks) -> xe_pm_runtime_xxx_map.
  * For example if the (callers_locks) are ever grabbed in the
  * runtime_resume callback, lockdep should give us a nice splat.
  */
-static void pm_runtime_lockdep_prime(void)
+static void xe_rpm_might_enter_cb(const struct xe_device *xe)
+{
+	xe_rpm_lockmap_acquire(xe);
+	xe_rpm_lockmap_release(xe);
+}
+
+/*
+ * Prime the lockdep maps for known locking orders that need to
+ * be supported but that may not always occur on all systems.
+ */
+static void xe_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);
+	dma_resv_unlock(&lockdep_resv);
+	lock_map_release(&xe_pm_runtime_d3cold_map);
+
+	/* Shrinkers might like to wake up the device under reclaim. */
+	fs_reclaim_acquire(GFP_KERNEL);
+	lock_map_acquire(&xe_pm_runtime_nod3cold_map);
+	lock_map_release(&xe_pm_runtime_nod3cold_map);
+	fs_reclaim_release(GFP_KERNEL);
 }
 
 /**
@@ -474,7 +514,7 @@ void xe_pm_runtime_get(struct xe_device *xe)
 	if (xe_pm_read_callback_task(xe) == current)
 		return;
 
-	pm_runtime_lockdep_prime();
+	xe_rpm_might_enter_cb(xe);
 	pm_runtime_resume(xe->drm.dev);
 }
 
@@ -506,7 +546,7 @@ int xe_pm_runtime_get_ioctl(struct xe_device *xe)
 	if (WARN_ON(xe_pm_read_callback_task(xe) == current))
 		return -ELOOP;
 
-	pm_runtime_lockdep_prime();
+	xe_rpm_might_enter_cb(xe);
 	return pm_runtime_get_sync(xe->drm.dev);
 }
 
@@ -574,7 +614,7 @@ bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
 		return true;
 	}
 
-	pm_runtime_lockdep_prime();
+	xe_rpm_might_enter_cb(xe);
 	return pm_runtime_resume_and_get(xe->drm.dev) >= 0;
 }
 
@@ -666,3 +706,14 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
 	drm_dbg(&xe->drm,
 		"d3cold: allowed=%s\n", str_yes_no(xe->d3cold.allowed));
 }
+
+/**
+ * xe_pm_module_init() - Perform xe_pm specific module initialization.
+ *
+ * Return: 0 on success. Currently doesn't fail.
+ */
+int __init xe_pm_module_init(void)
+{
+	xe_pm_runtime_lockdep_prime();
+	return 0;
+}
diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
index 104a21ae6dfd..9aef673b1c8a 100644
--- a/drivers/gpu/drm/xe/xe_pm.h
+++ b/drivers/gpu/drm/xe/xe_pm.h
@@ -32,5 +32,6 @@ void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
 int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
 void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
 struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
+int xe_pm_module_init(void);
 
 #endif
-- 
2.44.0

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

[Matthew Auld]

On 23/08/2024 17:58, Thomas Hellström wrote:
> 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.
> 
> v2:
> - Rename lockmap acquire- and release functions. (Rodrigo Vivi).
> - Reinstate the old xe_pm_runtime_lockdep_prime() function and
>    rename it to xe_rpm_might_enter_cb(). (Matthew Auld).
> - Introduce a separate xe_pm_runtime_lockdep_prime function
>    called from module init for known required locking orders.
> 
> Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
> Cc: "Auld, Matthew" <matthew.auld@intel.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>   drivers/gpu/drm/xe/xe_pm.c | 79 +++++++++++++++++++++++++++++++-------
>   drivers/gpu/drm/xe/xe_pm.h |  1 +
>   2 files changed, 66 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index 9f3c14fd9f33..747fb96453d3 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_rpm_lockmap_acquire(const struct xe_device *xe)
> +{
> +	lock_map_acquire(xe->d3cold.capable ?
> +			 &xe_pm_runtime_d3cold_map :
> +			 &xe_pm_runtime_nod3cold_map);
> +}
> +
> +static void xe_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_release(xe);
>   	xe_pm_write_callback_task(xe, NULL);
>   	return err;
>   }
> @@ -451,15 +469,37 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>    * stuff that can happen inside the runtime_resume callback by acquiring
>    * a dummy lock (it doesn't protect anything and gets compiled out on
>    * non-debug builds).  Lockdep then only needs to see the
> - * xe_pm_runtime_lockdep_map -> runtime_resume callback once, and then can
> - * hopefully validate all the (callers_locks) -> xe_pm_runtime_lockdep_map.
> + * xe_pm_runtime_xxx_map -> runtime_resume callback once, and then can
> + * hopefully validate all the (callers_locks) -> xe_pm_runtime_xxx_map.
>    * For example if the (callers_locks) are ever grabbed in the
>    * runtime_resume callback, lockdep should give us a nice splat.
>    */
> -static void pm_runtime_lockdep_prime(void)
> +static void xe_rpm_might_enter_cb(const struct xe_device *xe)
> +{
> +	xe_rpm_lockmap_acquire(xe);
> +	xe_rpm_lockmap_release(xe);
> +}
> +
> +/*
> + * Prime the lockdep maps for known locking orders that need to
> + * be supported but that may not always occur on all systems.
> + */
> +static void xe_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);
> +	dma_resv_unlock(&lockdep_resv);
> +	lock_map_release(&xe_pm_runtime_d3cold_map);
> +
> +	/* Shrinkers might like to wake up the device under reclaim. */
> +	fs_reclaim_acquire(GFP_KERNEL);
> +	lock_map_acquire(&xe_pm_runtime_nod3cold_map);
> +	lock_map_release(&xe_pm_runtime_nod3cold_map);
> +	fs_reclaim_release(GFP_KERNEL);

Would it make sense to also prime the d3cold wrt reclaim, but with the 
opposite ordering?

Anyway,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

>   }
>   
>   /**
> @@ -474,7 +514,7 @@ void xe_pm_runtime_get(struct xe_device *xe)
>   	if (xe_pm_read_callback_task(xe) == current)
>   		return;
>   
> -	pm_runtime_lockdep_prime();
> +	xe_rpm_might_enter_cb(xe);
>   	pm_runtime_resume(xe->drm.dev);
>   }
>   
> @@ -506,7 +546,7 @@ int xe_pm_runtime_get_ioctl(struct xe_device *xe)
>   	if (WARN_ON(xe_pm_read_callback_task(xe) == current))
>   		return -ELOOP;
>   
> -	pm_runtime_lockdep_prime();
> +	xe_rpm_might_enter_cb(xe);
>   	return pm_runtime_get_sync(xe->drm.dev);
>   }
>   
> @@ -574,7 +614,7 @@ bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
>   		return true;
>   	}
>   
> -	pm_runtime_lockdep_prime();
> +	xe_rpm_might_enter_cb(xe);
>   	return pm_runtime_resume_and_get(xe->drm.dev) >= 0;
>   }
>   
> @@ -666,3 +706,14 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
>   	drm_dbg(&xe->drm,
>   		"d3cold: allowed=%s\n", str_yes_no(xe->d3cold.allowed));
>   }
> +
> +/**
> + * xe_pm_module_init() - Perform xe_pm specific module initialization.
> + *
> + * Return: 0 on success. Currently doesn't fail.
> + */
> +int __init xe_pm_module_init(void)
> +{
> +	xe_pm_runtime_lockdep_prime();
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> index 104a21ae6dfd..9aef673b1c8a 100644
> --- a/drivers/gpu/drm/xe/xe_pm.h
> +++ b/drivers/gpu/drm/xe/xe_pm.h
> @@ -32,5 +32,6 @@ void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
>   int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
>   void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
>   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
> +int xe_pm_module_init(void);
>   
>   #endif

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

[Matthew Brost]

On Fri, Aug 23, 2024 at 06:13:02PM +0100, Matthew Auld wrote:
> On 23/08/2024 17:58, Thomas Hellström wrote:
> > 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.
> > 
> > v2:
> > - Rename lockmap acquire- and release functions. (Rodrigo Vivi).
> > - Reinstate the old xe_pm_runtime_lockdep_prime() function and
> >    rename it to xe_rpm_might_enter_cb(). (Matthew Auld).
> > - Introduce a separate xe_pm_runtime_lockdep_prime function
> >    called from module init for known required locking orders.
> > 
> > Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
> > Cc: "Auld, Matthew" <matthew.auld@intel.com>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_pm.c | 79 +++++++++++++++++++++++++++++++-------
> >   drivers/gpu/drm/xe/xe_pm.h |  1 +
> >   2 files changed, 66 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > index 9f3c14fd9f33..747fb96453d3 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_rpm_lockmap_acquire(const struct xe_device *xe)
> > +{
> > +	lock_map_acquire(xe->d3cold.capable ?
> > +			 &xe_pm_runtime_d3cold_map :
> > +			 &xe_pm_runtime_nod3cold_map);
> > +}
> > +
> > +static void xe_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_release(xe);
> >   	xe_pm_write_callback_task(xe, NULL);
> >   	return err;
> >   }
> > @@ -451,15 +469,37 @@ int xe_pm_runtime_resume(struct xe_device *xe)
> >    * stuff that can happen inside the runtime_resume callback by acquiring
> >    * a dummy lock (it doesn't protect anything and gets compiled out on
> >    * non-debug builds).  Lockdep then only needs to see the
> > - * xe_pm_runtime_lockdep_map -> runtime_resume callback once, and then can
> > - * hopefully validate all the (callers_locks) -> xe_pm_runtime_lockdep_map.
> > + * xe_pm_runtime_xxx_map -> runtime_resume callback once, and then can
> > + * hopefully validate all the (callers_locks) -> xe_pm_runtime_xxx_map.
> >    * For example if the (callers_locks) are ever grabbed in the
> >    * runtime_resume callback, lockdep should give us a nice splat.
> >    */
> > -static void pm_runtime_lockdep_prime(void)
> > +static void xe_rpm_might_enter_cb(const struct xe_device *xe)
> > +{
> > +	xe_rpm_lockmap_acquire(xe);
> > +	xe_rpm_lockmap_release(xe);
> > +}
> > +
> > +/*
> > + * Prime the lockdep maps for known locking orders that need to
> > + * be supported but that may not always occur on all systems.
> > + */
> > +static void xe_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);
> > +	dma_resv_unlock(&lockdep_resv);
> > +	lock_map_release(&xe_pm_runtime_d3cold_map);
> > +
> > +	/* Shrinkers might like to wake up the device under reclaim. */
> > +	fs_reclaim_acquire(GFP_KERNEL);
> > +	lock_map_acquire(&xe_pm_runtime_nod3cold_map);
> > +	lock_map_release(&xe_pm_runtime_nod3cold_map);
> > +	fs_reclaim_release(GFP_KERNEL);
> 
> Would it make sense to also prime the d3cold wrt reclaim, but with the
> opposite ordering?

Do you under the dma_resv_lock? dma_resv_lockdep already does that.

Patch also looks good to me btw.

Matt

> 
> Anyway,
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> 
> >   }
> >   /**
> > @@ -474,7 +514,7 @@ void xe_pm_runtime_get(struct xe_device *xe)
> >   	if (xe_pm_read_callback_task(xe) == current)
> >   		return;
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	pm_runtime_resume(xe->drm.dev);
> >   }
> > @@ -506,7 +546,7 @@ int xe_pm_runtime_get_ioctl(struct xe_device *xe)
> >   	if (WARN_ON(xe_pm_read_callback_task(xe) == current))
> >   		return -ELOOP;
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	return pm_runtime_get_sync(xe->drm.dev);
> >   }
> > @@ -574,7 +614,7 @@ bool xe_pm_runtime_resume_and_get(struct xe_device *xe)
> >   		return true;
> >   	}
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	return pm_runtime_resume_and_get(xe->drm.dev) >= 0;
> >   }
> > @@ -666,3 +706,14 @@ void xe_pm_d3cold_allowed_toggle(struct xe_device *xe)
> >   	drm_dbg(&xe->drm,
> >   		"d3cold: allowed=%s\n", str_yes_no(xe->d3cold.allowed));
> >   }
> > +
> > +/**
> > + * xe_pm_module_init() - Perform xe_pm specific module initialization.
> > + *
> > + * Return: 0 on success. Currently doesn't fail.
> > + */
> > +int __init xe_pm_module_init(void)
> > +{
> > +	xe_pm_runtime_lockdep_prime();
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > index 104a21ae6dfd..9aef673b1c8a 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.h
> > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > @@ -32,5 +32,6 @@ void xe_pm_assert_unbounded_bridge(struct xe_device *xe);
> >   int xe_pm_set_vram_threshold(struct xe_device *xe, u32 threshold);
> >   void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
> >   struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
> > +int xe_pm_module_init(void);
> >   #endif

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

[Thomas Hellström]

Hi, Matthew, Thanks for reviewing.

On Fri, 2024-08-23 at 18:13 +0100, Matthew Auld wrote:
> On 23/08/2024 17:58, Thomas Hellström wrote:
> > 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.
> > 
> > v2:
> > - Rename lockmap acquire- and release functions. (Rodrigo Vivi).
> > - Reinstate the old xe_pm_runtime_lockdep_prime() function and
> >    rename it to xe_rpm_might_enter_cb(). (Matthew Auld).
> > - Introduce a separate xe_pm_runtime_lockdep_prime function
> >    called from module init for known required locking orders.
> > 
> > Cc: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
> > Cc: "Auld, Matthew" <matthew.auld@intel.com>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_pm.c | 79 +++++++++++++++++++++++++++++++--
> > -----
> >   drivers/gpu/drm/xe/xe_pm.h |  1 +
> >   2 files changed, 66 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_pm.c
> > b/drivers/gpu/drm/xe/xe_pm.c
> > index 9f3c14fd9f33..747fb96453d3 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_rpm_lockmap_acquire(const struct xe_device *xe)
> > +{
> > +	lock_map_acquire(xe->d3cold.capable ?
> > +			 &xe_pm_runtime_d3cold_map :
> > +			 &xe_pm_runtime_nod3cold_map);
> > +}
> > +
> > +static void xe_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_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_rpm_lockmap_release(xe);
> >   	xe_pm_write_callback_task(xe, NULL);
> >   	return err;
> >   }
> > @@ -451,15 +469,37 @@ int xe_pm_runtime_resume(struct xe_device
> > *xe)
> >    * stuff that can happen inside the runtime_resume callback by
> > acquiring
> >    * a dummy lock (it doesn't protect anything and gets compiled
> > out on
> >    * non-debug builds).  Lockdep then only needs to see the
> > - * xe_pm_runtime_lockdep_map -> runtime_resume callback once, and
> > then can
> > - * hopefully validate all the (callers_locks) ->
> > xe_pm_runtime_lockdep_map.
> > + * xe_pm_runtime_xxx_map -> runtime_resume callback once, and then
> > can
> > + * hopefully validate all the (callers_locks) ->
> > xe_pm_runtime_xxx_map.
> >    * For example if the (callers_locks) are ever grabbed in the
> >    * runtime_resume callback, lockdep should give us a nice splat.
> >    */
> > -static void pm_runtime_lockdep_prime(void)
> > +static void xe_rpm_might_enter_cb(const struct xe_device *xe)
> > +{
> > +	xe_rpm_lockmap_acquire(xe);
> > +	xe_rpm_lockmap_release(xe);
> > +}
> > +
> > +/*
> > + * Prime the lockdep maps for known locking orders that need to
> > + * be supported but that may not always occur on all systems.
> > + */
> > +static void xe_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);
> > +	dma_resv_unlock(&lockdep_resv);
> > +	lock_map_release(&xe_pm_runtime_d3cold_map);
> > +
> > +	/* Shrinkers might like to wake up the device under
> > reclaim. */
> > +	fs_reclaim_acquire(GFP_KERNEL);
> > +	lock_map_acquire(&xe_pm_runtime_nod3cold_map);
> > +	lock_map_release(&xe_pm_runtime_nod3cold_map);
> > +	fs_reclaim_release(GFP_KERNEL);
> 
> Would it make sense to also prime the d3cold wrt reclaim, but with
> the 
> opposite ordering?

As Matt Brost pointed out that's already implied by the ordering wrt
dma-resv.

> 
> Anyway,
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

Thanks. I sen't out a v3, somehow the change that hooked up the module
init function got lost. Please indicate if that part is R-B:d as well.

Thanks,
Thomas


> 
> >   }
> >   
> >   /**
> > @@ -474,7 +514,7 @@ void xe_pm_runtime_get(struct xe_device *xe)
> >   	if (xe_pm_read_callback_task(xe) == current)
> >   		return;
> >   
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	pm_runtime_resume(xe->drm.dev);
> >   }
> >   
> > @@ -506,7 +546,7 @@ int xe_pm_runtime_get_ioctl(struct xe_device
> > *xe)
> >   	if (WARN_ON(xe_pm_read_callback_task(xe) == current))
> >   		return -ELOOP;
> >   
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	return pm_runtime_get_sync(xe->drm.dev);
> >   }
> >   
> > @@ -574,7 +614,7 @@ bool xe_pm_runtime_resume_and_get(struct
> > xe_device *xe)
> >   		return true;
> >   	}
> >   
> > -	pm_runtime_lockdep_prime();
> > +	xe_rpm_might_enter_cb(xe);
> >   	return pm_runtime_resume_and_get(xe->drm.dev) >= 0;
> >   }
> >   
> > @@ -666,3 +706,14 @@ void xe_pm_d3cold_allowed_toggle(struct
> > xe_device *xe)
> >   	drm_dbg(&xe->drm,
> >   		"d3cold: allowed=%s\n", str_yes_no(xe-
> > >d3cold.allowed));
> >   }
> > +
> > +/**
> > + * xe_pm_module_init() - Perform xe_pm specific module
> > initialization.
> > + *
> > + * Return: 0 on success. Currently doesn't fail.
> > + */
> > +int __init xe_pm_module_init(void)
> > +{
> > +	xe_pm_runtime_lockdep_prime();
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_pm.h
> > b/drivers/gpu/drm/xe/xe_pm.h
> > index 104a21ae6dfd..9aef673b1c8a 100644
> > --- a/drivers/gpu/drm/xe/xe_pm.h
> > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > @@ -32,5 +32,6 @@ void xe_pm_assert_unbounded_bridge(struct
> > xe_device *xe);
> >   int xe_pm_set_vram_threshold(struct xe_device *xe, u32
> > threshold);
> >   void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
> >   struct task_struct *xe_pm_read_callback_task(struct xe_device
> > *xe);
> > +int xe_pm_module_init(void);
> >   
> >   #endif