[PATCH] drm/xe: Use separate rpm lockdep map for non-d3cold-capable devices
Matthew Auld
matthew.auld at intel.com
Fri Aug 23 15:40:10 UTC 2024
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.
>
> /Thomas
>
>
>
>>
>> /Thomas
>>
>>
>>>
>>>> }
>>>>
>>>> /**
>>>> --
>>>> 2.44.0
>>>>
>>
>
More information about the Intel-xe
mailing list