Re: [PATCH v3 06/10] drm/xe/xe_late_bind_fw: Reload late binding fw in rpm resume

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 6/23/2025 9:11 AM, Nilawar, Badal wrote:
>
> On 23-06-2025 20:56, Daniele Ceraolo Spurio wrote:
>>
>>
>> On 6/18/2025 10:52 PM, Nilawar, Badal wrote:
>>>
>>> On 19-06-2025 02:35, Daniele Ceraolo Spurio wrote:
>>>>
>>>>
>>>> On 6/18/2025 12:00 PM, Badal Nilawar wrote:
>>>>> Reload late binding fw during runtime resume.
>>>>>
>>>>> v2: Flush worker during runtime suspend
>>>>>
>>>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/xe/xe_late_bind_fw.c | 2 +-
>>>>>   drivers/gpu/drm/xe/xe_late_bind_fw.h | 1 +
>>>>>   drivers/gpu/drm/xe/xe_pm.c           | 6 ++++++
>>>>>   3 files changed, 8 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c 
>>>>> b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>>>>> index 54aa08c6bdfd..c0be9611c73b 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>>>>> @@ -58,7 +58,7 @@ static int xe_late_bind_fw_num_fans(struct 
>>>>> xe_late_bind *late_bind)
>>>>>           return 0;
>>>>>   }
>>>>>   -static void xe_late_bind_wait_for_worker_completion(struct 
>>>>> xe_late_bind *late_bind)
>>>>> +void xe_late_bind_wait_for_worker_completion(struct xe_late_bind 
>>>>> *late_bind)
>>>>>   {
>>>>>       struct xe_device *xe = late_bind_to_xe(late_bind);
>>>>>       struct xe_late_bind_fw *lbfw;
>>>>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.h 
>>>>> b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>>>>> index 28d56ed2bfdc..07e437390539 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>>>>> @@ -12,5 +12,6 @@ struct xe_late_bind;
>>>>>     int xe_late_bind_init(struct xe_late_bind *late_bind);
>>>>>   int xe_late_bind_fw_load(struct xe_late_bind *late_bind);
>>>>> +void xe_late_bind_wait_for_worker_completion(struct xe_late_bind 
>>>>> *late_bind);
>>>>>     #endif
>>>>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>>>>> index ff749edc005b..91923fd4af80 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_pm.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>>>>> @@ -20,6 +20,7 @@
>>>>>   #include "xe_gt.h"
>>>>>   #include "xe_guc.h"
>>>>>   #include "xe_irq.h"
>>>>> +#include "xe_late_bind_fw.h"
>>>>>   #include "xe_pcode.h"
>>>>>   #include "xe_pxp.h"
>>>>>   #include "xe_trace.h"
>>>>> @@ -460,6 +461,8 @@ int xe_pm_runtime_suspend(struct xe_device *xe)
>>>>>       if (err)
>>>>>           goto out;
>>>>>   + xe_late_bind_wait_for_worker_completion(&xe->late_bind);
>>>>
>>>> I thing this can deadlock, because you do an rpm_put from within 
>>>> the worker and if that's the last put it'll end up here and wait 
>>>> for the worker to complete.
>>>> We could probably just skip this wait, because the worker can 
>>>> handle rpm itself. What we might want to be careful about is to nor 
>>>> re-queue it (from xe_late_bind_fw_load below) if it's currently 
>>>> being executed; we could also just let the fw be loaded twice if we 
>>>> hit that race condition, that shouldn't be an issue apart from 
>>>> doing something not needed.
>>>
>>> In xe_pm_runtime_get/_put, deadlocks are avoided by verifying the 
>>> condition (xe_pm_read_callback_task(xe) == current).
>>
>> Isn't that for calls to rpm_get/put done from within the 
>> rpm_suspend/resume code? This is not the case here, we're not 
>> deadlocking on the rpm lock, we're deadlocking on the worker.
>>
>> The error flow as I see it here would be as follow:
>>
>>     rpm refcount is 1, owned by thread X
>>     worker starts
>>     worker takes rpm [rpm refcount now 2]
>>     thread X releases rpm [rpm refcount now 1]
>>     worker releases rpm [rpm refcount now 0]
>>         rpm_suspend is called from within the worker
> rpm_suspend is not called within worker. First device will move to 
> idle state, then via auto suspend flow it will be runtime suspended, 
> all run time pm state changes will happen from rpm worker.
>
>>     xe_pm_write_callback_task is called
>>             flush_work is called -> deadlock
>>
>> I don't see how the callback_task() code can block the flush_work 
>> from deadlocking here.
>
> flush_work, as per my understanding, will wait for work to finish 
> executing last queuing instance. It runs the worker from the same 
> thread it is being flushed.  So how deadlock will happen?

My bad, for some reason I was convinced that we were doing a sync 
suspend, instead of going through the rpm worker. You're correct, no 
deadlock here.

Daniele

>
>>
>> Also, what happens if when the worker starts the rpm refcount is 0? 
>> Assuming the deadlock issue is not there.
>>
>>     worker starts
>>     worker takes rpm [rpm refcount now 1]
>>         rpm_resume is called
>>             worker is re-queued
>>     worker releases rpm [rpm refcount now 0]
>>     worker exits
>>     worker re-starts -> go back to beginning
>>
>> This second issue should be easily fixed by using pm_get_if_in_use 
>> from the worker, to not load the late_bind table if we're 
>> rpm_suspended since we'll do it when someone else resumes the device.
>
> Yes this makes sense, I will take care of this in next revision.
>
> Badal
>
>>
>> Daniele
>>
>>>
>>> Badal
>>>
>>>>
>>>> Daniele
>>>>
>>>>> +
>>>>>       /*
>>>>>        * Applying lock for entire list op as xe_ttm_bo_destroy and 
>>>>> xe_bo_move_notify
>>>>>        * also checks and deletes bo entry from user fault list.
>>>>> @@ -550,6 +553,9 @@ int xe_pm_runtime_resume(struct xe_device *xe)
>>>>>         xe_pxp_pm_resume(xe->pxp);
>>>>>   +    if (xe->d3cold.allowed)
>>>>> +        xe_late_bind_fw_load(&xe->late_bind);
>>>>> +
>>>>>   out:
>>>>>       xe_rpm_lockmap_release(xe);
>>>>>       xe_pm_write_callback_task(xe, NULL);
>>>>
>>