Re: [Intel-xe] [PATCH v2] drm/xe/guc_submit: fixup deregister in job timeout

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

On 08/08/2023 19:09, Matthew Brost wrote:
> On Tue, Aug 08, 2023 at 10:19:04AM +0100, Matthew Auld wrote:
>> Rather check if the engine is still registered before proceeding with
>> deregister steps. Also the engine being marked as disabled doesn't mean
>> the engine has been disabled or deregistered from GuC pov, and here we
>> are signalling fences so we need to be sure GuC is not still using this
>> context.
>>
>> v2:
>>   - Drop the read_stopped() for this path. Since we are signalling
>>     fences on error here, best play it safe and wait for the GT reset to
>>     mark the engine as disabled, rather than it just being queued.
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_submit.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index 52c61f78b083..6126ddf2fdd5 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -881,15 +881,17 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>>   	}
>>   
>>   	/* Engine state now stable, disable scheduling if needed */
>> -	if (exec_queue_enabled(q)) {
>> +	if (exec_queue_registered(q)) {
>>   		struct xe_guc *guc = exec_queue_to_guc(q);
>>   		int ret;
>>   
>>   		if (exec_queue_reset(q))
>>   			err = -EIO;
>>   		set_exec_queue_banned(q);
>> -		xe_exec_queue_get(q);
>> -		disable_scheduling_deregister(guc, q);
>> +		if (!exec_queue_destroyed(q)) {
>> +			xe_exec_queue_get(q);
>> +			disable_scheduling_deregister(guc, q);
>> +		}
>>   
>>   		/*
>>   		 * Must wait for scheduling to be disabled before signalling
>> @@ -901,8 +903,8 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>>   		 */
>>   		smp_rmb();
>>   		ret = wait_event_timeout(guc->ct.wq,
>> -					 !exec_queue_pending_disable(q) ||
>> -					 guc_read_stopped(guc), HZ * 5);
> 
> I think we want the guc_read_stopped here as we want to pop out of the
> wait if a GT reset is scheduled.
> 
>> +					 !exec_queue_pending_disable(q),
>> +					 HZ * 5);
>>   		if (!ret) {
> 
> Then here we want the check to be:
> 
> !ret || guc_read_stopped
> 
> As we want delay the signaling of fences behind the GT reset.

OK, that sounds better. Let me try that.

> 
> Matt
> 
>>   			XE_WARN_ON("Schedule disable failed to respond");
>>   			sched->timeout = MIN_SCHED_TIMEOUT;
>> -- 
>> 2.41.0
>>