Re: [PATCH 6/7] drm/xe: Clean up GuC software state after a wedge

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Oct 20, 2025 at 09:45:28PM +0000, Stuart Summers wrote:
> When the driver is wedged during a hardware failure, there
> is a chance the queue kill coming from those events can
> race with either the scheduler teardown or the queue
> deregistration with GuC. Basically the following two
> scenarios can occur (from event trace):
> 
> Scheduler start missing:
>   xe_exec_queue_create

The queues should be initialized in a started state unless a GT reset or
VF migration is in progress. In both cases, upon successful completion,
all queues will be restarted.

I did spot a bug in GT resets — if those fail, we don’t properly restart
the queues. That should be fixed.

Also, I think xe_guc_declare_wedged is incorrect now that I’m looking at
it.

It probably should be:

void xe_guc_declare_wedged(struct xe_guc *guc)
{
        xe_gt_assert(guc_to_gt(guc), guc_to_xe(guc)->wedged.mode);

        xe_guc_ct_stop(&guc->ct);
        xe_guc_submit_wedge(guc);
	xe_guc_sanitize(guc);
}

>   xe_exec_queue_kill
>   xe_guc_exec_queue_kill
>   xe_exec_queue_destroy
> 
> GuC CT response missing:
>   xe_exec_queue_create
>   xe_exec_queue_register
>   xe_exec_queue_scheduling_enable
>   xe_exec_queue_scheduling_done
>   xe_exec_queue_kill
>   xe_guc_exec_queue_kill
>   xe_exec_queue_close
>   xe_exec_queue_destroy
>   xe_exec_queue_cleanup_entity
>   xe_exec_queue_scheduling_disable

The ref count should be zero here — xe_exec_queue_scheduling_disable is
called after this series [1]. I think we need to get this series in
before making changes to the GuC submission state machine. Technically,
all we need are the last three patches from that series, as they
simplify some things. I believe an upcoming Xe3 feature would also
benefit from getting these patches in too.

So that means in xe_guc_submit_wedge() the below if statement is going
to fail.

1006         mutex_lock(&guc->submission_state.lock);
1007         xa_for_each(&guc->submission_state.exec_queue_lookup, index, q)
1008                 if (xe_exec_queue_get_unless_zero(q))
1009                         set_exec_queue_wedged(q);
1010         mutex_unlock(&guc->submission_state.lock);

I think we need...

else if (exec_queue_register(q)) 
	__guc_exec_queue_destroy(guc, q);

We also need to cleanup suspend fences too as those could be lost under
the right race condition.

So prior to existing if statement, we also need:

if (q->guc->suspend_pending)
	suspend_fence_signal(q);

[1] https://patchwork.freedesktop.org/series/155315/

> 
> The above traces depend also on inclusion of [1].
> 
> In the first scenario, the queue is created, but killed
> prior to completing the message cleanup. In the second,
> we go through a full registration before killing. The
> CT communication happens in that last call to
> xe_exec_queue_scheduling_disable.
> 
> We expect to then get a call to xe_guc_exec_queue_destroy
> in both cases if the aforementioned scheduler/GuC CT communication
> had happened, which we are missing here, hence missing any
> LRC/BO cleanup in the exec queues in question.
> 
> Since this sequence seems specific to the wedge case
> as described above, add a targeted scheduler start
> and guc deregistration handler to the wedged_fini()
> routine.
> 
> Without this change, if we inject wedges in the above scenarios
> we can expect the following when the DRM memory tracking is
> enabled (see CONFIG_DRM_DEBUG_MM):
> [  129.600285] [drm:drm_mm_takedown] *ERROR* node [00647000 + 00008000]: inserted at
>                 drm_mm_insert_node_in_range+0x2ec/0x4b0
>                 __xe_ggtt_insert_bo_at+0x10f/0x360 [xe]
>                 __xe_bo_create_locked+0x184/0x520 [xe]
>                 xe_bo_create_pin_map_at_aligned+0x3b/0x180 [xe]
>                 xe_bo_create_pin_map+0x13/0x20 [xe]
>                 xe_lrc_create+0x139/0x18e0 [xe]
>                 xe_exec_queue_create+0x22f/0x3e0 [xe]
>                 xe_exec_queue_create_ioctl+0x4e9/0xbf0 [xe]
>                 drm_ioctl_kernel+0x9f/0xf0
>                 drm_ioctl+0x20f/0x440
>                 xe_drm_ioctl+0x121/0x150 [xe]
>                 __x64_sys_ioctl+0x8c/0xe0
>                 do_syscall_64+0x4c/0x1d0
>                 entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [  129.601966] [drm:drm_mm_takedown] *ERROR* node [0064f000 + 00008000]: inserted at
>                 drm_mm_insert_node_in_range+0x2ec/0x4b0
>                 __xe_ggtt_insert_bo_at+0x10f/0x360 [xe]
>                 __xe_bo_create_locked+0x184/0x520 [xe]
>                 xe_bo_create_pin_map_at_aligned+0x3b/0x180 [xe]
>                 xe_bo_create_pin_map+0x13/0x20 [xe]
>                 xe_lrc_create+0x139/0x18e0 [xe]
>                 xe_exec_queue_create+0x22f/0x3e0 [xe]
>                 xe_exec_queue_create_bind+0x7f/0xd0 [xe]
>                 xe_vm_create+0x4aa/0x8b0 [xe]
>                 xe_vm_create_ioctl+0x17b/0x420 [xe]
>                 drm_ioctl_kernel+0x9f/0xf0
>                 drm_ioctl+0x20f/0x440
>                 xe_drm_ioctl+0x121/0x150 [xe]
>                 __x64_sys_ioctl+0x8c/0xe0
>                 do_syscall_64+0x4c/0x1d0
>                 entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> 
> [1] https://patchwork.freedesktop.org/patch/680852/?series=155352&rev=4
> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 5ec1e4a83d68..a11ae4e70809 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -287,6 +287,8 @@ static void guc_submit_fini(struct drm_device *drm, void *arg)
>  	xa_destroy(&guc->submission_state.exec_queue_lookup);
>  }
>  
> +static void __guc_exec_queue_destroy(struct xe_guc *guc, struct xe_exec_queue *q);
> +
>  static void guc_submit_wedged_fini(void *arg)
>  {
>  	struct xe_guc *guc = arg;
> @@ -299,6 +301,16 @@ static void guc_submit_wedged_fini(void *arg)
>  			mutex_unlock(&guc->submission_state.lock);
>  			xe_exec_queue_put(q);
>  			mutex_lock(&guc->submission_state.lock);

With everything above I don't think this new code below is needed.

But to make sure we know what we are doing, how about this from [2]
before the xe_exec_queue_put.

xe_gt_assert(..., !drm_sched_is_stopped(sched));

Wanna try out these suggestions? It is always possible I made a mistake
here.

Matt

[2] https://patchwork.freedesktop.org/patch/681606/?series=155315&rev=3

> +		} else {
> +			/*
> +			 * Make sure queues which were killed as part of a
> +			 * wedge are cleaned up properly. Clean up any
> +			 * dangling scheduler tasks and pending exec queue
> +			 * deregistration.
> +			 */
> +			xe_sched_submission_start(&q->guc->sched);
> +			if (exec_queue_pending_disable(q))
> +				__guc_exec_queue_destroy(guc, q);
>  		}
>  	}
>  	mutex_unlock(&guc->submission_state.lock);
> -- 
> 2.34.1
>