Re: [PATCH v3 03/25] drm/xe: Decouple exec queue idle check from LRC

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

On Tue, Mar 03, 2026 at 02:26:56PM -0700, Summers, Stuart wrote:
> On Mon, 2026-03-02 at 13:02 -0800, Matthew Brost wrote:
> > > On Mon, Mar 02, 2026 at 01:50:11PM -0700, Summers, Stuart wrote:
> > > > > On Fri, 2026-02-27 at 17:34 -0800, Matthew Brost wrote:
> > > > > > > We already maintain a job count for each exec queue, so
> > > > > > > simplify > > > the
> > > > > > > idle
> > > > > > > check to rely on the job count rather than the LRC state.
> > > > > > > This
> > > > > > > decouples
> > > > > > > exec queues from LRC-based backends and avoids
> > > > > > > unnecessarily > > > coupling
> > > > > > > idle
> > > > > > > detection to backend-specific implementation details.
> > > > > > > 
> > > > > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/xe/xe_exec_queue.c | 15 +--------------
> > > > > > >  1 file changed, 1 insertion(+), 14 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > > > > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > > > > index 2d0e73a6a6ee..b3f700a9d425 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > > > > > @@ -1382,20 +1382,7 @@ bool xe_exec_queue_is_lr(struct > >
> > > > > > > > xe_exec_queue
> > > > > > > *q)
> > > > > > >   */
> > > > > > >  bool xe_exec_queue_is_idle(struct xe_exec_queue *q)
> > > > > > >  {
> > > > > > > -       if (xe_exec_queue_is_parallel(q)) {
> > > > > > > -               int i;
> > > > > > > -
> > > > > > > -               for (i = 0; i < q->width; ++i) {
> > > > > > > -                       if (xe_lrc_seqno(q->lrc[i]) !=
> > > > > > > -                           q->lrc[i]->fence_ctx.next_seqno
> > > > > > > - 1)
> > > > > > > -                               return false;
> > > > > > > -               }
> > > > > > > -
> > > > > > > -               return true;
> > > > > > > -       }
> > > > > > > -
> > > > > > > -       return xe_lrc_seqno(q->lrc[0]) ==
> > > > > > > -               q->lrc[0]->fence_ctx.next_seqno - 1;
> > > > > > > +       return !atomic_read(&q->job_cnt);
> > > > > 
> > > > > Still looking through the series, so might be handled
> > > > > elsewhere, > > but
> > > > > just looking at this patch alone, I'm a little worried this
> > > > > will > > cause
> > > > > unexpected issues in the exec queue cleanup. This function > >
> > > > > currently
> > > > > ensures that the job is idle from the hardware level. The
> > > > > change > > you
> > > 
> > > The current check is actually incorrect if, for example, a queue is
> > > reset and the LRC head != tail. However, I believe the only places
> > > we
> > > use xe_exec_queue_is_idle are cases where a queue hasn’t been
> > > reset, > so
> > > it happens to work in practice. It’s also just an advisory check,
> > > so
> > > nothing bad happens if it incorrectly reports “not idle".
> 
> So reset case aside (which not taking into consideration anything you
> said below :) I'd consider a bug here), it does give a false sense of
> things being actually idle on the hardware IMO that might be extended
> out to other areas without realizing in the future. I agree that the
> current use cases match what you said.
> 

Yes, so I would say this patch is actually improving things and opening
up this function to other possible use cases.

> > > 
> > > > > make here moves that to a software level check. And this is
> > > > > getting
> > > > > decremented and checked before we tear down the exec queue. So
> > > > > presumably, GuC and the command streamer could still be doing >
> > > > > > something
> > > > > here and we're falsely telling other parts of the driver that
> > > > > rely > > on
> > > > > the engine to really be idle to trust us here.
> > > > > 
> > > 
> > > See above for part of the explanation, but the other part involves
> > > reference counting and fence signaling. A job can only have its
> > > last
> > > reference dropped when its fence is signaled.
> > > 
> > > A fence can only signal under the following conditions:
> > > 
> > > - Its seqno is incremented via ring instructions (which corresponds
> > > > to
> > >   the LRC head == tail if it’s the last job on the queue).
> 
> Right, so technically I guess we could have a hardware hang after the
> sequence number was written since that isn't the last instruction
> there, but seems very unlikely. And if we did hit that case, the reset
> handler would cover that.
> 
> Maybe this should be obvious... but just so I'm not missing something
> here..
> 
> So I think the signaling here we're talking about is via the
> MI_USER_INT in:
> xe_hw_engine_handle_irq -> xe_hw_fence_rq_run

This is where fences are signaled or if we time them out in
guc_exec_queue_timedout_job via xe_sched_job_set_error.

> 
> And that dependency you're talking about is here (xe_exec, although I
> know there are a few in xe_migrate, xe_pt, etc)?
>         /* Wait behind rebinds */
>         if (!xe_vm_in_lr_mode(vm)) {
>                 err = xe_sched_job_add_deps(job,
>                                             xe_vm_resv(vm),
>                                             DMA_RESV_USAGE_KERNEL);
>                 if (err)
>                         goto err_put_job;
>         }
> 
> What is the expectation for LR jobs?
> 

This is completely unrelated but in dma-fence mode (!xe_vm_in_lr_mode)
we can't fault the device so we issue rebinds in the current exec
IOCTL for anything that moved since the last exec IOCTL - this ordering
exec IOCTL submission behind moving memory back into place + rebinding
it.

LR mode we either:
 - Rebind in preempt rebind worker
 - Let the device take a page fault and rebind

Because of this we don't even take the dma-resv lock for LR VMs in the
exec IOCTL.

Matt 

> Thanks,
> Stuart
> 
> > > - We time out jobs on the queue and signal their fences in
> > > software. > We
> > >   only signal fences in software once the queue has been kicked off
> > > > the
> > >   hardware (i.e., scheduling-disable H2G triggers a G2H response).
> > > 
> > > > > For reference, I'm looking at xe_sched_job_destroy() where we
> > > > > do > > the
> > > > > decrement and then the exec queue put.
> > > > > 
> > > > > So my question is, how are we guaranteeing that hardware is
> > > > > indeed > > idle
> > > > > after this change? Are we moving the sequence number check > >
> > > > > somewhere
> > > > > else?
> > > > > 
> > > 
> > > I think above explains this.
> > > 
> > > Matt
> > > 
> > > > > Thanks,
> > > > > Stuart
> > > > > 
> > > > > > >  }
> > > > > > >  
> > > > > > >  /**
> > > > > 
>