Re: [PATCH v3] drm/xe/uc: Add stop on hardware initialization error

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2025-10-28 6:36 p.m., Dong, Zhanjun wrote:
> 
> 
> On 2025-10-28 3:57 p.m., Matthew Brost wrote:
>> On Tue, Oct 28, 2025 at 11:38:20AM -0400, Zhanjun Dong wrote:
>>> On hardware init fail, the hardware might no longer response, add GuC 
>>> stop
>>> to clean up exec_queue items.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5466
>>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/5530
>>> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
>>> ---
>>> v3: Switch to xe_guc_stop
>>> v2: Switch to xe_guc_ct_stop
>>> ---
>>>   drivers/gpu/drm/xe/xe_uc.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
>>> index 465bda355443..00ca5883e006 100644
>>> --- a/drivers/gpu/drm/xe/xe_uc.c
>>> +++ b/drivers/gpu/drm/xe/xe_uc.c
>>> @@ -173,6 +173,7 @@ static int vf_uc_load_hw(struct xe_uc *uc)
>>>       return 0;
>>>   err_out:
>>> +    xe_guc_stop(&uc->guc);
>>
>> If exec queues are destroyed later—after the submission backend has been
>> stopped—the final put on the queue may be lost, leading to dangling
>> memory when aborting the driver load or unloading it.
>>
>> I think you'll need to call xe_guc_submit_pause_abort somewhere to
>> ensure the final put cleanup messages are processed by the queues. Maybe
>> we add this call in guc_submit_fini before wait_event_timeout?
>>
>> Matt
> Thanks for review.
> My original thought is through xe_guc_stop/xe_guc_submit_stop/ 
> guc_exec_queue_stop, where will do clean up, might be not covers all 
> conditions, let me try.
Tested with call xe_guc_submit_pause_abort in guc_submit_fini before 
wait_event_timeout, works in some condition, while there is 1 condition 
might not cover: for lr queues, it won't clear, so I'm thinking of:

@@ -2375,7 +2382,9 @@ void xe_guc_submit_pause_abort(struct xe_guc *guc)
                         continue;

                 xe_sched_submission_start(sched);
-               if (exec_queue_killed_or_banned_or_wedged(q))
+               if (exec_queue_killed_or_banned_or_wedged(q) || \
		    exec_queue_registered(q))
                         xe_guc_exec_queue_trigger_cleanup(q);
         }
         mutex_unlock(&guc->submission_state.lock);

@Matthew Brost <matthew.brost@intel.com>, Do you think this change has 
side effect to migration worker? I can make it another function if true.

Regards,
Zhanjun Dong

> 
> Regards,
> Zhanjun Dong
> 
>>
>>>       xe_guc_sanitize(&uc->guc);
>>>       return err;
>>>   }
>>> @@ -228,6 +229,7 @@ int xe_uc_load_hw(struct xe_uc *uc)
>>>       return 0;
>>>   err_out:
>>> +    xe_guc_stop(&uc->guc);
>>>       xe_guc_sanitize(&uc->guc);
>>>       return ret;
>>>   }
>>> -- 
>>> 2.34.1
>>>
>