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

["Summers, Stuart" <stuart.summers@intel.com>]

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.

> > 
> > > > 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

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?

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
> > > > 
> > > > > >  }
> > > > > >  
> > > > > >  /**
> > > >