Re: [PATCH v3 05/11] drm/xe/multi_queue: Store primary LRC and position info in LRC

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

On Tue, May 05, 2026 at 04:44:14PM -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. Both Primary and secondary LRCs are freed only when
>primary queue is destroyed. At this time there all secondary queues are
>already destroyed, so there is no one using secondary LRCs. We should be
>good without taking any additional references.
>
>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)
>
>v3:
>- Drop the reference logic since it's not needed (Niranjana)
>- Move lrc->multi_queue initialization to a later point (Niranjana)
>---
> drivers/gpu/drm/xe/xe_exec_queue.c | 5 +++++
> drivers/gpu/drm/xe/xe_lrc.h        | 5 +++++
> drivers/gpu/drm/xe/xe_lrc_types.h  | 8 ++++++++
> 3 files changed, 18 insertions(+)
>
>diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>index 9b36ff5b09a6..bc8110a9b054 100644
>--- a/drivers/gpu/drm/xe/xe_exec_queue.c
>+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>@@ -1389,6 +1389,11 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> 				goto put_exec_queue;
> 		}
>
>+		if (xe_exec_queue_is_multi_queue(q)) {
>+			q->lrc[0]->multi_queue.pos = q->multi_queue.pos;
>+			q->lrc[0]->multi_queue.primary_lrc = q->multi_queue.group->primary->lrc[0];
>+		}

This is fine, but looks bit odd without proper comment in the code.
I can think of two better ways to do it,

1. Do it here as below

/* Update lrc multi-queue information after secondary queues is added to group */
if (xe_exec_queue_is_multi_queue(q))
      xe_exec_queue_set_multi_queue_lrc(q);

Yes, xe_exec_queue_set_multi_queue_lrc() will be new static function.

2. Remove the code here, and...
    a. set q->lrc[0].multi_queue.primary_lrc in xe_exec_queue_set_lrc() if queue is
       multi-queue, so that it gets set for both primary and secondary.
    b. set q->lrc[0].multi_queue.pos in xe_exec_queue_group_add() for secondary queues,
       (for primary, pos is 0 anyway).

Niranjana

>+
> 		if (xe_vm_in_preempt_fence_mode(vm)) {
> 			q->lr.context = dma_fence_context_alloc(1);
>
>diff --git a/drivers/gpu/drm/xe/xe_lrc.h b/drivers/gpu/drm/xe/xe_lrc.h
>index d280e6572398..557dce004d48 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.51.0
>