On Thu, Nov 20, 2025 at 11:50:32AM -0800, Niranjana Vishwanathapura wrote:
> On Wed, Nov 19, 2025 at 02:41:04PM -0800, Matthew Brost wrote:
> > Deregistering queues in the TDR introduces unnecessary complexity,
> > requiring reference-counting techniques to function correctly,
> > particularly to prevent use-after-free (UAF) issues while a
> > deregistration initiated from the TDR is in progress.
> >
> > All that's needed in the TDR is to kick the queue off the hardware,
> > which is achieved by disabling scheduling. Queue deregistration should
> > be handled in a single, well-defined point in the cleanup path, tied to
> > the queue's reference count.
> >
> > v4:
> > - Explain why extra ref were needed prior to this patch (Niranjana)
> >
> > Signed-off-by: Matthew Brost <[email protected]>
> > ---
> > drivers/gpu/drm/xe/xe_guc_submit.c | 65 +++++-------------------------
> > 1 file changed, 9 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c
> > b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 648c9ea06749..5de300b66767 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
> > #define EXEC_QUEUE_STATE_WEDGED (1 << 8)
> > #define EXEC_QUEUE_STATE_BANNED (1 << 9)
> > #define EXEC_QUEUE_STATE_CHECK_TIMEOUT (1 << 10)
> > -#define EXEC_QUEUE_STATE_EXTRA_REF (1 << 11)
> > -#define EXEC_QUEUE_STATE_PENDING_RESUME (1 << 12)
> > -#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT (1 << 13)
> > +#define EXEC_QUEUE_STATE_PENDING_RESUME (1 << 11)
> > +#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT (1 << 12)
> >
> > static bool exec_queue_registered(struct xe_exec_queue *q)
> > {
> > @@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct
> > xe_exec_queue *q)
> > atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
> > }
> >
> > -static bool exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > - return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
> > -}
> > -
> > -static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > - atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > -}
> > -
> > -static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > - atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > -}
> > -
> > static bool exec_queue_pending_resume(struct xe_exec_queue *q)
> > {
> > return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
> > @@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue
> > *q, bool immediate)
> > G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
> > }
> >
> > -static void __deregister_exec_queue(struct xe_guc *guc, struct
> > xe_exec_queue *q)
> > -{
> > - u32 action[] = {
> > - XE_GUC_ACTION_DEREGISTER_CONTEXT,
> > - q->guc->id,
> > - };
> > -
> > - xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
> > - xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
> > - xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
> > - xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
> > -
> > - set_exec_queue_destroyed(q);
> > - trace_xe_exec_queue_deregister(q);
> > -
> > - xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
> > - G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
> > -}
> > -
> > static enum drm_gpu_sched_stat
> > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > {
> > @@ -1225,6 +1190,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job
> > *drm_job)
> > bool wedged = false, skip_timeout_check;
> >
> > xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
> > + xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
>
> Is it always guaranteed? What if we get here because TDR is triggered
> by some error notification from the GuC and befor we get here, the
> exec_queue gets destroyed in the CLEANUP message handler? I am not
> sure we can we be sure here that it will be race proof.
>
Jobs hold a reference to the queue. We have a job here, and the CLEANUP
message (which sets destroyed) is tied to the reference count. So if
this pops, we have a problem. I use asserts in GuC submission to ensure
the state machine (which is fairly complicated) works as designed-this
would be one of those cases.
Matt
> Niranjana
>
> >
> > /*
> > * TDR has fired before free job worker. Common if exec queue
> > @@ -1241,8 +1207,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job
> > *drm_job)
> >
> > /* Must check all state after stopping scheduler */
> > skip_timeout_check = exec_queue_reset(q) ||
> > - exec_queue_killed_or_banned_or_wedged(q) ||
> > - exec_queue_destroyed(q);
> > + exec_queue_killed_or_banned_or_wedged(q);
> >
> > /*
> > * If devcoredump not captured and GuC capture for the job is not ready
> > @@ -1271,13 +1236,13 @@ guc_exec_queue_timedout_job(struct drm_sched_job
> > *drm_job)
> > wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
> >
> > /* Engine state now stable, disable scheduling to check timestamp */
> > - if (!wedged && exec_queue_registered(q)) {
> > + if (!wedged && (exec_queue_enabled(q) ||
> > exec_queue_pending_disable(q))) {
> > int ret;
> >
> > if (exec_queue_reset(q))
> > err = -EIO;
> >
> > - if (!exec_queue_destroyed(q) && xe_uc_fw_is_running(&guc->fw)) {
> > + if (xe_uc_fw_is_running(&guc->fw)) {
> > /*
> > * Wait for any pending G2H to flush out before
> > * modifying state
> > @@ -1327,8 +1292,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job
> > *drm_job)
> > xe_devcoredump(q, job,
> > "Schedule disable failed to respond,
> > guc_id=%d, ret=%d, guc_read=%d",
> > q->guc->id, ret,
> > xe_guc_read_stopped(guc));
> > - set_exec_queue_extra_ref(q);
> > - xe_exec_queue_get(q); /* GT reset owns this */
> > set_exec_queue_banned(q);
> > xe_gt_reset_async(q->gt);
> > xe_sched_tdr_queue_imm(sched);
> > @@ -1381,13 +1344,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job
> > *drm_job)
> > }
> > }
> >
> > - /* Finish cleaning up exec queue via deregister */
> > set_exec_queue_banned(q);
> > - if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
> > - set_exec_queue_extra_ref(q);
> > - xe_exec_queue_get(q);
> > - __deregister_exec_queue(guc, q);
> > - }
> >
> > /* Mark all outstanding jobs as bad, thus completing them */
> > xe_sched_job_set_error(job, err);
> > @@ -1933,7 +1890,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc,
> > struct xe_exec_queue *q)
> >
> > /* Clean up lost G2H + reset engine state */
> > if (exec_queue_registered(q)) {
> > - if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > + if (xe_exec_queue_is_lr(q))
> > xe_exec_queue_put(q);
> > else if (exec_queue_destroyed(q))
> > __guc_exec_queue_destroy(guc, q);
> > @@ -2067,11 +2024,7 @@ static void
> > guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
> >
> > if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
> > clear_exec_queue_destroyed(q);
> > - if (exec_queue_extra_ref(q))
> > - xe_exec_queue_put(q);
> > - else
> > - q->guc->needs_cleanup = true;
> > - clear_exec_queue_extra_ref(q);
> > + q->guc->needs_cleanup = true;
> > xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
> > q->guc->id);
> > }
> > @@ -2488,7 +2441,7 @@ static void handle_deregister_done(struct xe_guc
> > *guc, struct xe_exec_queue *q)
> >
> > clear_exec_queue_registered(q);
> >
> > - if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > + if (xe_exec_queue_is_lr(q))
> > xe_exec_queue_put(q);
> > else
> > __guc_exec_queue_destroy(guc, q);
> > --
> > 2.34.1
> >
