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

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

On Thu, Feb 26, 2026 at 06:26:29PM +0100, Lis, Tomasz wrote:
> ack to almost all requests, but comment to one remaining below.
> 
> On 2/26/2026 2:57 AM, Matthew Brost wrote:
> > 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?
> 
> We've added another getter and setter. It would be very un-structured if we
> had one getter, then a lot of different code, and then the 2nd getter.
> 
> Where no other restrictions apply, we should keep functions of similar
> level in one place within source files.
> 
> When someone looks at a code which contains a setter and a getter, I believe
> they should find any other getters nearby. They definitely should not expect
> there may be another getter of the same resource much further within the
> file.
> 
> For why move up instead of inserting new ones down - it is to avoid
> unnecessary prototype (I believe checkpatch would otherwise complain).
> 

Fair enough. We lose a bit of git history, but the function you’re
moving isn’t exactly splitting the atom, so I don’t think the history is
a concern here.

Matt

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