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

[Matthew Auld <matthew.auld@intel.com>]

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