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

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

On Tue, 2026-03-03 at 14:42 -0800, Matthew Brost wrote:
> 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.

Agreed..

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

Ah right..

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

Yeah ok makes sense and I appreciate the explanation :)

Anyway I think with that I agree with the direction here.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

Thanks,
Stuart

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