[PATCH] drm/xe: Use separate rpm lockdep map for non-d3cold-capable devices
Matthew Auld
matthew.auld at intel.com
Fri Aug 23 16:17:30 UTC 2024
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 at intel.com>
>>>>>
>>>>>>
>>>>>> Cc: "Vivi, Rodrigo" <rodrigo.vivi at intel.com>
>>>>>> Signed-off-by: Thomas Hellström
>>>>>> <thomas.hellstrom at 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
>>>>>>
>>>>
>>>
>
More information about the Intel-xe
mailing list