Re: [PATCH v3 2/4] drm/xe/queue: Wrappers for setting and getting LRC references

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

On Wed, Feb 25, 2026 at 05:42:20PM -0800, Matthew Brost wrote:

Opps, one mistake in my review, corrected below.

> On Thu, Feb 26, 2026 at 12:54:45AM +0100, Tomasz Lis wrote:
> 
> s/'drm/xe/queue'/'drm/xe' in the patch prefix I think.
> 
> > There is a small but non-zero chance that VF post migration fixups
> > are running on an exec queue during teardown. The chances are
> > decreased by starting the teardown by releasing guc_id, but remain
> > non-zero. On the other hand the sync between fixups and EQ creation
> > (wait_valid_ggtt) drastically increases the chance for such parallel
> > teardown if queue creation error path is entered (err_lrc label).
> > 
> > The exec queue itself is not going to cause an issue, but LRCs have
> > a small chance of getting freed during the fixups.
> > 
> > Creating a setter and a getter makes it easier to protect the fixup
> > operations with a lock. For other driver activities, the original
> > access method (without any protection) can still be used.
> > 
> > Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c | 71 +++++++++++++++++++++---------
> >  drivers/gpu/drm/xe/xe_exec_queue.h |  1 +
> >  2 files changed, 52 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index b4ef725a682d..2cb37af42021 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -270,6 +270,54 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe,
> >  	return q;
> >  }
> >  
> > +static void xe_exec_queue_set_lrc(struct xe_exec_queue *q, struct xe_lrc *lrc, u16 idx)
> > +{
> > +	xe_assert(gt_to_xe(q->gt), idx < q->width);
> > +
> > +	scoped_guard(spinlock, &q->multi_queue.lock)
> > +		q->lrc[idx] = lrc;
> > +}
> > +
> > +/**
> > + * xe_exec_queue_get_lrc() - Get the LRC from exec queue.
> > + * @q: The exec_queue.
> > + * @idx: Index within multi-LRC array.
> > + *
> > + * Retrieves LRC of given index for the exec queue
> 
> under lock and takes reference.
> 
> 
> > + *
> > + * Return: Pointer to LRC on success, error on failure
> 
> NULL on lookup failure.
> 
> > + */
> > +struct xe_lrc *xe_exec_queue_get_lrc(struct xe_exec_queue *q, u16 idx)
> > +{
> > +	struct xe_lrc *lrc;
> 
> struct xe_lrc *lrc = NULL;
> 

Actually not needed as lrc is always assigned and will either be present
and valid or NULL.

Matt

> > +
> > +	xe_assert(gt_to_xe(q->gt), idx < q->width);
> > +
> > +	scoped_guard(spinlock, &q->multi_queue.lock) {
> 
> Hmm, this isn't what 'q->multi_queue.lock' was designed for.
> 
> Can we get a dedicated spinlock? Maybe 'q->lrc_lookup_lock'?
> 
> Open to better naming.
> 
> > +		lrc = q->lrc[idx];
> > +		if (lrc)
> > +			xe_lrc_get(lrc);
> > +	}
> > +
> > +	return lrc;
> > +}
> > +
> > +/**
> > + * xe_exec_queue_lrc() - Get the LRC from exec queue.
> > + * @q: The exec_queue.
> > + *
> > + * Retrieves the primary LRC for the exec queue. Note that this function
> > + * returns only the first LRC instance, even when multiple parallel LRCs
> > + * are configured. This function does not increment reference count,
> > + * so the reference can be just forgotten after use.
> > + *
> > + * Return: Pointer to LRC on success, error on failure
> > + */
> > +struct xe_lrc *xe_exec_queue_lrc(struct xe_exec_queue *q)
> > +{
> > +	return q->lrc[0];
> > +}
> 
> Why move this code?
> 
> > +
> >  static void __xe_exec_queue_fini(struct xe_exec_queue *q)
> >  {
> >  	int i;
> > @@ -327,8 +375,7 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q, u32 exec_queue_flags)
> >  			goto err_lrc;
> >  		}
> >  
> > -		/* Pairs with READ_ONCE to xe_exec_queue_contexts_hwsp_rebase */
> > -		WRITE_ONCE(q->lrc[i], lrc);
> > +		xe_exec_queue_set_lrc(q, lrc, i);
> >  	}
> >  
> >  	return 0;
> > @@ -1288,21 +1335,6 @@ int xe_exec_queue_get_property_ioctl(struct drm_device *dev, void *data,
> >  	return ret;
> >  }
> >  
> > -/**
> > - * xe_exec_queue_lrc() - Get the LRC from exec queue.
> > - * @q: The exec_queue.
> > - *
> > - * Retrieves the primary LRC for the exec queue. Note that this function
> > - * returns only the first LRC instance, even when multiple parallel LRCs
> > - * are configured.
> > - *
> > - * Return: Pointer to LRC on success, error on failure
> > - */
> > -struct xe_lrc *xe_exec_queue_lrc(struct xe_exec_queue *q)
> > -{
> > -	return q->lrc[0];
> > -}
> > -
> >  /**
> >   * xe_exec_queue_is_lr() - Whether an exec_queue is long-running
> >   * @q: The exec_queue
> > @@ -1662,14 +1694,13 @@ int xe_exec_queue_contexts_hwsp_rebase(struct xe_exec_queue *q, void *scratch)
> >  	for (i = 0; i < q->width; ++i) {
> >  		struct xe_lrc *lrc;
> >  
> > -		/* Pairs with WRITE_ONCE in __xe_exec_queue_init  */
> > -		lrc = READ_ONCE(q->lrc[i]);
> > +		lrc = xe_exec_queue_get_lrc(q, i);
> >  		if (!lrc)
> >  			continue;
> > -
> 
> Unrelated.
> 
> Matt
> 
> >  		xe_lrc_update_memirq_regs_with_address(lrc, q->hwe, scratch);
> >  		xe_lrc_update_hwctx_regs_with_address(lrc);
> >  		err = xe_lrc_setup_wa_bb_with_scratch(lrc, q->hwe, scratch);
> > +		xe_lrc_put(lrc);
> >  		if (err)
> >  			break;
> >  	}
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
> > index c9e3a7c2d249..a82d99bd77bc 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> > @@ -160,6 +160,7 @@ void xe_exec_queue_update_run_ticks(struct xe_exec_queue *q);
> >  int xe_exec_queue_contexts_hwsp_rebase(struct xe_exec_queue *q, void *scratch);
> >  
> >  struct xe_lrc *xe_exec_queue_lrc(struct xe_exec_queue *q);
> > +struct xe_lrc *xe_exec_queue_get_lrc(struct xe_exec_queue *q, u16 idx);
> >  
> >  /**
> >   * xe_exec_queue_idle_skip_suspend() - Can exec queue skip suspend
> > -- 
> > 2.25.1
> >