Re: [PATCH v2 3/9] drm/xe/multi_queue: Store primary LRC and position info in LRC

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Tue, May 05, 2026 at 11:35:23AM -0700, Umesh Nerlige Ramappa wrote:
>On Mon, May 04, 2026 at 08:46:10PM -0700, Niranjana Vishwanathapura wrote:
>>On Fri, May 01, 2026 at 05:53:36PM -0700, Umesh Nerlige Ramappa wrote:
>>>Given an LRC belonging to the secondary queue, in order to check if its
>>>context group is active, we need to check the LRC of the primary queue.
>>>In addition to that we want to compare the secondary queue position to
>>>CSMQDEBUG register to check if the queue itself is active.
>>>
>>>To do so, store primary LRC and position information in the LRC as well
>>>as take a reference to the primary LRC from each LRC in the queue group.
>>>
>>>A note on references involved:
>>>
>>>- In general the Queue takes a ref on its LRC.
>>>- In addition, for multi-queue,
>>>a. Primary Queue takes a ref for each Secondary LRC.
>>>b. Each Secondary Queue takes a ref to the Primary Queue
>>>
>>>In the current patch, each LRC in the queue group is storing a pointer
>>>to Primary LRC. There is a small window of time in the primary queue
>>>free path where the primary LRC may be freed before the secondary LRC.
>>>
>>>__xe_exec_queue_fini(q); // frees|puts primary q LRCs
>>>...
>>>window where secondary Q LRC is pointing to invalid primary LRC
>>>...
>>>__xe_exec_queue_free(q); // frees|puts secondary q LRCs in multi-Q case
>>>
>>>In this window the reference in Secondary LRC is invalid. While there
>>>may be nothing accessing the secondary LRCs reference, to be safe, this
>>>patch is taking a reference to Primary LRC from the secondary LRC.
>>>
>>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>---
>>>v2:
>>>- Store primary LRC instead of primary queue (Niranjana)
>>>- Drop the valid flag and check if primary_lrc is NULL (Niranjana)
>>>- Document/Revisit references (Matt/Umesh)
>>>---
>>>drivers/gpu/drm/xe/xe_exec_queue.c | 23 ++++++++++++++++++++---
>>>drivers/gpu/drm/xe/xe_lrc.h        |  5 +++++
>>>drivers/gpu/drm/xe/xe_lrc_types.h  |  8 ++++++++
>>>3 files changed, 33 insertions(+), 3 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>>>index b287d0e0e60a..e34601d28520 100644
>>>--- a/drivers/gpu/drm/xe/xe_exec_queue.c
>>>+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>>>@@ -129,8 +129,14 @@ static void xe_exec_queue_group_cleanup(struct xe_exec_queue *q)
>>>		return;
>>>
>>>	/* Primary queue cleanup */
>>>-	xa_for_each(&group->xa, idx, lrc)
>>>+	xa_for_each(&group->xa, idx, lrc) {
>>>+		/* drop secondary lrc ref to primary lrc */
>>>+		xe_lrc_put(lrc->multi_queue.primary_lrc);
>>>+		/* drop primary queue ref to secondary lrc */
>>>		xe_lrc_put(lrc);
>>>+	}
>>>+	/* drop primary lrc ref to itself */
>>>+	xe_lrc_put(q->lrc[0]);
>>>
>>>	xa_destroy(&group->xa);
>>>	mutex_destroy(&group->list_lock);
>>>@@ -275,8 +281,15 @@ static void xe_exec_queue_set_lrc(struct xe_exec_queue *q, struct xe_lrc *lrc, u
>>>{
>>>	xe_assert(gt_to_xe(q->gt), idx < q->width);
>>>
>>>-	scoped_guard(spinlock, &q->lrc_lookup_lock)
>>>+	scoped_guard(spinlock, &q->lrc_lookup_lock) {
>>>		q->lrc[idx] = lrc;
>>>+		if (xe_exec_queue_is_multi_queue(q)) {
>>>+			struct xe_lrc *primary_lrc = q->multi_queue.group->primary->lrc[0];
>>>+
>>>+			lrc->multi_queue.pos = q->multi_queue.pos;
>>
>>I think q->multi_queue.pos is not yet set for secondary queues at this point.
>>It is set later in the xe_exec_queue_group_add() call.
>
>Hmm, I recall seeing that a while ago and that's why I leaned toward 
>having a lrc->q (back reference to the queue that the LRC belongs to, 
>but that's prohibited). Anyways, I will try to move this logic outsize 
>to a late point then.
>
>>
>>>+			lrc->multi_queue.primary_lrc = xe_lrc_get(primary_lrc);
>>
>>I think we don't need to get/put the primary_lrc reference.
>>Each queue holds a reference to its LRC. The secondary queues holds a reference
>>to the primary queue. So, essentially, the secondary LRCs are holding a reference
>>to primary lrc. So, I think, we don't need to hold reference again.
>
>I would look at the queue and LRC as different objects in memory.
>
>In taking a reference here, I am addressing only one specific corner 
>case:
>
>- Consider all secondary queues are freed
>- Since primary queue holds a reference to all secondary LRCs, all   
>secondary LRCs are still in memory.
>- When all references to primary queue are released, we end up in   
>xe_exec_queue_fini(primary_q).
>
>In this function the LRC for primary_q is freed first, and then the 
>secondary_q lrcs are freed, so there is a small window of time where 
>the reference from secondary LRC to primary LRC is invalid. IMO, that 
>small windows of time is enough for strange issues.
>

By that time, all secondary queues have been deregistered and destroyed.
So, there shouldn't be any secondary LRC trying to access the primary
LRC. May be we can set primary_lrc to NULL while releaseing LRC reference
which should be sane enough and we really don't need primary_lrc reference
taking.

Niranjana

>Umesh
>>
>>Niranjana
>>
>>>+		}
>>>+	}
>>>}
>>>
>>>/**
>>>@@ -388,8 +401,12 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q, u32 exec_queue_flags)
>>>
>>>			xe_exec_queue_set_lrc(q, lrc, i);
>>>
>>>-			if (__lrc)
>>>+			if (__lrc) {
>>>+				if (xe_exec_queue_is_multi_queue(q))
>>>+					xe_lrc_put(__lrc->multi_queue.primary_lrc);
>>>+
>>>				xe_lrc_put(__lrc);
>>>+			}
>>>			__lrc = lrc;
>>>
>>>		} while (marker != xe_vf_migration_fixups_complete_count(q->gt));
>>>diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
>>>index 97aef0327fc8..3d0bf4a7bfa0 100644
>>>--- a/drivers/gpu/drm/xe/xe_lrc.h
>>>+++ b/drivers/gpu/drm/xe/xe_lrc.h
>>>@@ -91,6 +91,11 @@ static inline size_t xe_lrc_ring_size(void)
>>>	return SZ_16K;
>>>}
>>>
>>>+static inline bool xe_lrc_is_multi_queue(struct xe_lrc *lrc)
>>>+{
>>>+	return lrc->multi_queue.primary_lrc;
>>>+}
>>>+
>>>size_t xe_gt_lrc_hang_replay_size(struct xe_gt *gt, enum xe_engine_class class);
>>>size_t xe_gt_lrc_size(struct xe_gt *gt, enum xe_engine_class class);
>>>u32 xe_lrc_pphwsp_offset(struct xe_lrc *lrc);
>>>diff --git a/drivers/gpu/drm/xe/xe_lrc_types.h b/drivers/gpu/drm/xe/xe_lrc_types.h
>>>index 5a718f759ed6..0a5c13ec2ad7 100644
>>>--- a/drivers/gpu/drm/xe/xe_lrc_types.h
>>>+++ b/drivers/gpu/drm/xe/xe_lrc_types.h
>>>@@ -63,6 +63,14 @@ struct xe_lrc {
>>>
>>>	/** @ctx_timestamp: readout value of CTX_TIMESTAMP on last update */
>>>	u64 ctx_timestamp;
>>>+
>>>+	/** @multi_queue: Multi queue LRC related information */
>>>+	struct {
>>>+		/** @multi_queue.primary_lrc: Primary lrc of this multi-queue group*/
>>>+		struct xe_lrc *primary_lrc;
>>>+		/** @multi_queue.pos: Position of LRC within the multi-queue group */
>>>+		u8 pos;
>>>+	} multi_queue;
>>>};
>>>
>>>struct xe_lrc_snapshot;
>>>-- 
>>>2.43.0
>>>