[PATCH 00/27] Parallel submission aka multi-bb execbuf

On Wed, Sep 15, 2021 at 09:24:15AM +0100, Tvrtko Ursulin wrote:

On 14/09/2021 19:04, Matthew Brost wrote:

quoted

On Tue, Sep 14, 2021 at 09:34:08AM +0100, Tvrtko Ursulin wrote:

quoted

8<

quoted

quoted

Today we have:

for_each intel_engines: // intel_engines is a flat list of all engines
	intel_engine_setup()

You propose to change it to:

for_each engine_class:
    for 0..max_global_engine_instance:
       for_each intel_engines:
          skip engine not present
          skip class not matching

          count logical instance

    for_each intel_engines:
       skip engine not present
       skip wrong class

       intel_engine_setup()


I propose:

// Leave as is:

for_each intel_engines:
    intel_engine_setup()

// Add:

for_each engine_class:
    logical = 0
    for_each gt->engine_class[class]:
       skip engine not present

       engine->logical_instance = logical++


When code which actually needs a preturbed "map" arrives you add that in to
this second loop.

See above, why introduce an algorithm that doesn't work for future parts
+ future patches are land imminently? It makes zero sense whatsoever.
With your proposal we would literally land code to just throw it away a
couple of months from now + break patches we intend to land soon. This

It sure works, it just walks the per class list instead of walking the flat
list skipping one class at the time.

Just add the map based transformation to the second pass later, when it
becomes required.

I can flatten the algorithm if that helps alleviate your concerns but
with that being said, I've played around this locally and IMO makes the
code way more ugly. Sure it eliminates some iterations of the loop but
who really cares about that in a one time setup function?

quoted

algorithm works and has no reason whatsoever to be optimal as it a one
time setup call. I really don't understand why we are still talking
about this paint color.

I don't think bike shedding is not an appropriate term when complaint is how
proposed algorithm is needlessly complicated.

Are you just ignoring the fact that the algorithm (map) is needed in
pending patches? IMO it is more complicated to write throw away code
when the proper algorithm is already written. If the logical mapping was
straight forward on all platforms as the ones currently upstream I would
100% agree with your suggestion, but it isn't on unembargoed platforms
eminently going upstream. The algorithm I have works for the current
platforms + the pending platforms. IMO is 100% acceptable to merge
something looking towards a known future.

Matt

Regards,

Tvrtko

On 8/20/2021 15:44, Matthew Brost wrote:

These should be in the 'guc_state' sub-struct? Would be good to keep all GuC specific content in one self-contained struct. Especially given the other child/parent fields are no going to be guc_ prefixed any more.

Maybe 'page in context record'? Otherwise, exactly what 'context' is meant here? It isn't the 'struct intel_context'. The contetx record is saved as 'ce->state' / 'ce->lrc_reg_state', yes? Is it possible to link to either of those field? Probably not given that they don't appear to have any kerneldoc description :(. Maybe add that in too :).

+		 * work queue descriptor

Later on, it is described as 'process descriptor and work queue'. It would be good to be consistent.

Is this size actually dictated by the GuC API? Or is it just a driver level decision? If the latter, shouldn't this be below instead?

Can this not be derived from GUC_WQ_SIZE given that the two are fundamentally linked? E.g. '#define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'? And maybe have a '#define WQ_TOTAL_SIZE PAGE_SIZE' and use that in all of WQ_OFFSET, GUC_WQ_SIZE and the allocation itself in intel_lrc.c?

Also, the process descriptor is actually an array of descriptors sized by the number of children? Or am I misunderstanding the code below? In so, shouldn't there be a 'COMPILE_BUG_ON((MAX_ENGINE_INSTANCE * sizeof(descriptor)) < (WQ_DESC_SIZE)' where WQ_DESC_SIZE is WQ_TOTAL_SIZE - WQ_SIZE?

Where did the LRC_STATE_OFFSET come from? Is that built in to the lrg_reg_state pointer itself? That needs to be documented somewhere.

Technically, this should say 'If the context is a parent'. Or just move it to be inside the if block.

Should this not be inherited from the ce? And same below. Or are we not using this priority in that way?

John.

On 8/20/2021 15:44, Matthew Brost wrote:

Isn't that now two? One uni-LRC holding the timeline mutex and one multi-LRC holding the parallel submit mutex?

John.

On Wed, Sep 15, 2021 at 12:21:35PM -0700, John Harrison wrote:

On 8/20/2021 15:44, Matthew Brost wrote:

quoted

Add multi-lrc context registration H2G. In addition a workqueue and
process descriptor are setup during multi-lrc context registration as
these data structures are needed for multi-lrc submission.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
  drivers/gpu/drm/i915/gt/intel_context_types.h |  12 ++
  drivers/gpu/drm/i915/gt/intel_lrc.c           |   5 +
  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   2 +-
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 109 +++++++++++++++++-
  4 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 0fafc178cf2c..6f567ebeb039 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h

@@ -232,8 +232,20 @@ struct intel_context {
  		/** @parent: pointer to parent if child */
  		struct intel_context *parent;
+
+		/** @guc_wqi_head: head pointer in work queue */
+		u16 guc_wqi_head;
+		/** @guc_wqi_tail: tail pointer in work queue */
+		u16 guc_wqi_tail;
+

These should be in the 'guc_state' sub-struct? Would be good to keep all GuC
specific content in one self-contained struct. Especially given the other
child/parent fields are no going to be guc_ prefixed any more.

Right now I have everything in guc_state protected by guc_state.lock,
these fields are not protected by this lock. IMO it is better to use a
different sub-structure for the parallel fields (even if anonymous).

quoted

  		/** @guc_number_children: number of children if parent */
  		u8 guc_number_children;
+
+		/**
+		 * @parent_page: page in context used by parent for work queue,

Maybe 'page in context record'? Otherwise, exactly what 'context' is meant
here? It isn't the 'struct intel_context'. The contetx record is saved as
'ce->state' / 'ce->lrc_reg_state', yes? Is it possible to link to either of

It is the page in ce->state / page minus LRC reg offset in
ce->lrg_reg_state. Will update the commit to make that clear.

those field? Probably not given that they don't appear to have any kerneldoc
description :(. Maybe add that in too :).

quoted

+		 * work queue descriptor

Later on, it is described as 'process descriptor and work queue'. It would
be good to be consistent.

Yep. Will fix.

quoted

+		 */
+		u8 parent_page;
  	};
  #ifdef CONFIG_DRM_I915_SELFTEST

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index bb4af4977920..0ddbad4e062a 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c

@@ -861,6 +861,11 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)
  		context_size += PAGE_SIZE;
  	}
+	if (intel_context_is_parent(ce)) {
+		ce->parent_page = context_size / PAGE_SIZE;
+		context_size += PAGE_SIZE;
+	}
+
  	obj = i915_gem_object_create_lmem(engine->i915, context_size, 0);
  	if (IS_ERR(obj))
  		obj = i915_gem_object_create_shmem(engine->i915, context_size);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index fa4be13c8854..0e600a3b8f1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h

@@ -52,7 +52,7 @@
  #define GUC_DOORBELL_INVALID		256
-#define GUC_WQ_SIZE			(PAGE_SIZE * 2)
+#define GUC_WQ_SIZE			(PAGE_SIZE / 2)

Is this size actually dictated by the GuC API? Or is it just a driver level
decision? If the latter, shouldn't this be below instead?

Driver level decision. What exactly do you mean by below?
 

quoted

  /* Work queue item header definitions */
  #define WQ_STATUS_ACTIVE		1

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 14b24298cdd7..dbcb9ab28a9a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c

@@ -340,6 +340,39 @@ static struct i915_priolist *to_priolist(struct rb_node *rb)
  	return rb_entry(rb, struct i915_priolist, node);
  }
+/*
+ * When using multi-lrc submission an extra page in the context state is
+ * reserved for the process descriptor and work queue.
+ *
+ * The layout of this page is below:
+ * 0						guc_process_desc
+ * ...						unused
+ * PAGE_SIZE / 2				work queue start
+ * ...						work queue
+ * PAGE_SIZE - 1				work queue end
+ */
+#define WQ_OFFSET	(PAGE_SIZE / 2)

Can this not be derived from GUC_WQ_SIZE given that the two are
fundamentally linked? E.g. '#define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'?

Yes. I like 'define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'. Will change.

And maybe have a '#define WQ_TOTAL_SIZE PAGE_SIZE' and use that in all of
WQ_OFFSET, GUC_WQ_SIZE and the allocation itself in intel_lrc.c?

Also, the process descriptor is actually an array of descriptors sized by
the number of children? Or am I misunderstanding the code below? In so,

No, it is fixed size descriptor.

A later patch in the series uses the space of above the process
descriptor for insertation of preeemption points handshake. That does
depend on the number of children. I will add a COMPILE_BUG_ON for that
to ensure everything fits in the memory layout.

shouldn't there be a 'COMPILE_BUG_ON((MAX_ENGINE_INSTANCE *
sizeof(descriptor)) < (WQ_DESC_SIZE)' where WQ_DESC_SIZE is WQ_TOTAL_SIZE -
WQ_SIZE?

quoted

+static u32 __get_process_desc_offset(struct intel_context *ce)
+{
+	GEM_BUG_ON(!ce->parent_page);
+
+	return ce->parent_page * PAGE_SIZE;
+}
+
+static u32 __get_wq_offset(struct intel_context *ce)
+{
+	return __get_process_desc_offset(ce) + WQ_OFFSET;
+}
+
+static struct guc_process_desc *
+__get_process_desc(struct intel_context *ce)
+{
+	return (struct guc_process_desc *)
+		(ce->lrc_reg_state +
+		 ((__get_process_desc_offset(ce) -
+		   LRC_STATE_OFFSET) / sizeof(u32)));

Where did the LRC_STATE_OFFSET come from? Is that built in to the
lrg_reg_state pointer itself? That needs to be documented somewhere.

In gt/intel_lrc.c (lrc_pin) ce->lrc_reg_state is assigned to
mmap(ce->state) + LRC_STATE_OFFSET. I can update the kerneldoc for that
field in this patch.

quoted

+}
+
  static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
  {
  	struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;

@@ -1342,6 +1375,30 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce)
  	spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  }
+static int __guc_action_register_multi_lrc(struct intel_guc *guc,
+					   struct intel_context *ce,
+					   u32 guc_id,
+					   u32 offset,
+					   bool loop)
+{
+	struct intel_context *child;
+	u32 action[4 + MAX_ENGINE_INSTANCE];
+	int len = 0;
+
+	GEM_BUG_ON(ce->guc_number_children > MAX_ENGINE_INSTANCE);
+
+	action[len++] = INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC;
+	action[len++] = guc_id;
+	action[len++] = ce->guc_number_children + 1;
+	action[len++] = offset;
+	for_each_child(ce, child) {
+		offset += sizeof(struct guc_lrc_desc);
+		action[len++] = offset;
+	}
+
+	return guc_submission_send_busy_loop(guc, action, len, 0, loop);
+}
+
  static int __guc_action_register_context(struct intel_guc *guc,
  					 u32 guc_id,
  					 u32 offset,

@@ -1364,9 +1421,15 @@ static int register_context(struct intel_context *ce, bool loop)
  		ce->guc_id.id * sizeof(struct guc_lrc_desc);
  	int ret;
+	GEM_BUG_ON(intel_context_is_child(ce));
  	trace_intel_context_register(ce);
-	ret = __guc_action_register_context(guc, ce->guc_id.id, offset, loop);
+	if (intel_context_is_parent(ce))
+		ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id,
+						      offset, loop);
+	else
+		ret = __guc_action_register_context(guc, ce->guc_id.id, offset,
+						    loop);
  	if (likely(!ret)) {
  		unsigned long flags;

@@ -1396,6 +1459,7 @@ static int deregister_context(struct intel_context *ce, u32 guc_id, bool loop)
  {
  	struct intel_guc *guc = ce_to_guc(ce);
+	GEM_BUG_ON(intel_context_is_child(ce));
  	trace_intel_context_deregister(ce);
  	return __guc_action_deregister_context(guc, guc_id, loop);

@@ -1423,6 +1487,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
  	struct guc_lrc_desc *desc;
  	bool context_registered;
  	intel_wakeref_t wakeref;
+	struct intel_context *child;
  	int ret = 0;
  	GEM_BUG_ON(!engine->mask);

@@ -1448,6 +1513,42 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
  	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
  	guc_context_policy_init(engine, desc);
+	/*
+	 * Context is a parent, we need to register a process descriptor
+	 * describing a work queue and register all child contexts.

Technically, this should say 'If the context is a parent'. Or just move it
to be inside the if block.

I will add the "If the".

quoted

+	 */
+	if (intel_context_is_parent(ce)) {
+		struct guc_process_desc *pdesc;
+
+		ce->guc_wqi_tail = 0;
+		ce->guc_wqi_head = 0;
+
+		desc->process_desc = i915_ggtt_offset(ce->state) +
+			__get_process_desc_offset(ce);
+		desc->wq_addr = i915_ggtt_offset(ce->state) +
+			__get_wq_offset(ce);
+		desc->wq_size = GUC_WQ_SIZE;
+
+		pdesc = __get_process_desc(ce);
+		memset(pdesc, 0, sizeof(*(pdesc)));
+		pdesc->stage_id = ce->guc_id.id;
+		pdesc->wq_base_addr = desc->wq_addr;
+		pdesc->wq_size_bytes = desc->wq_size;
+		pdesc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;

Should this not be inherited from the ce? And same below. Or are we not
using this priority in that way?

Honestly I don't think this field is used or maybe doesn't even exist
anymore. I'll check the GuC code and likely delete this or if it is
still present I'll inherited this from the ce.

Matt

John.

quoted

+		pdesc->wq_status = WQ_STATUS_ACTIVE;
+
+		for_each_child(ce, child) {
+			desc = __get_lrc_desc(guc, child->guc_id.id);
+
+			desc->engine_class =
+				engine_class_to_guc_class(engine->class);
+			desc->hw_context_desc = child->lrc.lrca;
+			desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
+			desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
+			guc_context_policy_init(engine, desc);
+		}
+	}
+
  	/*
  	 * The context_lookup xarray is used to determine if the hardware
  	 * context is currently registered. There are two cases in which it

@@ -2858,6 +2959,12 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx)
  		return NULL;
  	}
+	if (unlikely(intel_context_is_child(ce))) {
+		drm_err(&guc_to_gt(guc)->i915->drm,
+			"Context is child, desc_idx %u", desc_idx);
+		return NULL;
+	}
+
  	return ce;
  }

On Wed, Sep 15, 2021 at 12:24:41PM -0700, John Harrison wrote:

On 8/20/2021 15:44, Matthew Brost wrote:

quoted

In GuC parent-child contexts the parent context controls the scheduling,
ensure only the parent does the scheduling operations.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 24 ++++++++++++++-----
  1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index dbcb9ab28a9a..00d54bb00bfb 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c

@@ -320,6 +320,12 @@ static void decr_context_committed_requests(struct intel_context *ce)
  	GEM_BUG_ON(ce->guc_state.number_committed_requests < 0);
  }
+static struct intel_context *
+request_to_scheduling_context(struct i915_request *rq)
+{
+	return intel_context_to_parent(rq->context);
+}
+
  static bool context_guc_id_invalid(struct intel_context *ce)
  {
  	return ce->guc_id.id == GUC_INVALID_LRC_ID;

@@ -1684,6 +1690,7 @@ static void __guc_context_sched_disable(struct intel_guc *guc,
  	GEM_BUG_ON(guc_id == GUC_INVALID_LRC_ID);
+	GEM_BUG_ON(intel_context_is_child(ce));
  	trace_intel_context_sched_disable(ce);
  	guc_submission_send_busy_loop(guc, action, ARRAY_SIZE(action),

@@ -1898,6 +1905,8 @@ static void guc_context_sched_disable(struct intel_context *ce)
  	u16 guc_id;
  	bool enabled;
+	GEM_BUG_ON(intel_context_is_child(ce));
+
  	if (submission_disabled(guc) || context_guc_id_invalid(ce) ||
  	    !lrc_desc_registered(guc, ce->guc_id.id)) {
  		spin_lock_irqsave(&ce->guc_state.lock, flags);

@@ -2286,6 +2295,8 @@ static void guc_signal_context_fence(struct intel_context *ce)
  {
  	unsigned long flags;
+	GEM_BUG_ON(intel_context_is_child(ce));
+
  	spin_lock_irqsave(&ce->guc_state.lock, flags);
  	clr_context_wait_for_deregister_to_register(ce);
  	__guc_signal_context_fence(ce);

@@ -2315,7 +2326,7 @@ static void guc_context_init(struct intel_context *ce)
  static int guc_request_alloc(struct i915_request *rq)
  {
-	struct intel_context *ce = rq->context;
+	struct intel_context *ce = request_to_scheduling_context(rq);
  	struct intel_guc *guc = ce_to_guc(ce);
  	unsigned long flags;
  	int ret;

@@ -2358,11 +2369,12 @@ static int guc_request_alloc(struct i915_request *rq)
  	 * exhausted and return -EAGAIN to the user indicating that they can try
  	 * again in the future.
  	 *
-	 * There is no need for a lock here as the timeline mutex ensures at
-	 * most one context can be executing this code path at once. The
-	 * guc_id_ref is incremented once for every request in flight and
-	 * decremented on each retire. When it is zero, a lock around the
-	 * increment (in pin_guc_id) is needed to seal a race with unpin_guc_id.
+	 * There is no need for a lock here as the timeline mutex (or
+	 * parallel_submit mutex in the case of multi-lrc) ensures at most one
+	 * context can be executing this code path at once. The guc_id_ref is

Isn't that now two? One uni-LRC holding the timeline mutex and one multi-LRC
holding the parallel submit mutex?

This is actually a stale comment and need scrub this. The
parallel_submit mutex is gone, now we grab the ce->timeline locks
starting at the parent and then all children in a loop. I think the
original comment is sufficient.

Matt

John.

quoted

+	 * incremented once for every request in flight and decremented on each
+	 * retire. When it is zero, a lock around the increment (in pin_guc_id)
+	 * is needed to seal a race with unpin_guc_id.
  	 */
  	if (atomic_add_unless(&ce->guc_id.ref, 1, 0))
  		goto out;

On 8/20/2021 15:44, Matthew Brost wrote:

The 'hence' clause seems to be attached to the wrong reason. The id space is partition because of the contiguous vs random requirements of multi vs single LRC, not because a different allocator is used in one partion vs the other.

Is there any check against adding/removing children when the guc_ids are allocated? Presumably it shouldn't ever happen but if it did then the bitmap_release would not match the allocation. Maybe add BUG_ON(ce->guc_id) to the parent/child link functions (if it's not there already?).

Leaving this as 'ce' and calling the input parameter 'ce_in' would have made for significantly easier to read diffs!

As far as I can tell, the only actual change to this function (beyond 'ce_in->id = id' vs 'return id' and adding anti-family asserts) is that this spinlock was dropped. However, I'm not seeing any replacement for it or any comment about why the spinlock is no longer necessary.

John.

On 9/15/2021 12:31, Matthew Brost wrote:

On Wed, Sep 15, 2021 at 12:21:35PM -0700, John Harrison wrote:

quoted

On 8/20/2021 15:44, Matthew Brost wrote:

quoted

Add multi-lrc context registration H2G. In addition a workqueue and
process descriptor are setup during multi-lrc context registration as
these data structures are needed for multi-lrc submission.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
   drivers/gpu/drm/i915/gt/intel_context_types.h |  12 ++
   drivers/gpu/drm/i915/gt/intel_lrc.c           |   5 +
   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   2 +-
   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 109 +++++++++++++++++-
   4 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 0fafc178cf2c..6f567ebeb039 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h

@@ -232,8 +232,20 @@ struct intel_context {
   		/** @parent: pointer to parent if child */
   		struct intel_context *parent;
+
+		/** @guc_wqi_head: head pointer in work queue */
+		u16 guc_wqi_head;
+		/** @guc_wqi_tail: tail pointer in work queue */
+		u16 guc_wqi_tail;
+

These should be in the 'guc_state' sub-struct? Would be good to keep all GuC
specific content in one self-contained struct. Especially given the other
child/parent fields are no going to be guc_ prefixed any more.

Right now I have everything in guc_state protected by guc_state.lock,
these fields are not protected by this lock. IMO it is better to use a
different sub-structure for the parallel fields (even if anonymous).

Hmm, I still think it is bad to be scattering back-end specific fields amongst regular fields. The GuC patches include a whole bunch of complaints about execlist back-end specific stuff leaking through to the higher levels, we really shouldn't be guilty of doing the same with GuC if at all possible. At the very least, the GuC specific fields should be grouped together at the end of the struct rather than inter-mingled.

quoted

quoted

   		/** @guc_number_children: number of children if parent */
   		u8 guc_number_children;
+
+		/**
+		 * @parent_page: page in context used by parent for work queue,

Maybe 'page in context record'? Otherwise, exactly what 'context' is meant
here? It isn't the 'struct intel_context'. The contetx record is saved as
'ce->state' / 'ce->lrc_reg_state', yes? Is it possible to link to either of

It is the page in ce->state / page minus LRC reg offset in
ce->lrg_reg_state. Will update the commit to make that clear.

quoted

those field? Probably not given that they don't appear to have any kerneldoc
description :(. Maybe add that in too :).

quoted

+		 * work queue descriptor

Later on, it is described as 'process descriptor and work queue'. It would
be good to be consistent.

Yep. Will fix.

quoted

quoted

+		 */
+		u8 parent_page;
   	};
   #ifdef CONFIG_DRM_I915_SELFTEST

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index bb4af4977920..0ddbad4e062a 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c

@@ -861,6 +861,11 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)
   		context_size += PAGE_SIZE;
   	}
+	if (intel_context_is_parent(ce)) {
+		ce->parent_page = context_size / PAGE_SIZE;
+		context_size += PAGE_SIZE;
+	}
+
   	obj = i915_gem_object_create_lmem(engine->i915, context_size, 0);
   	if (IS_ERR(obj))
   		obj = i915_gem_object_create_shmem(engine->i915, context_size);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index fa4be13c8854..0e600a3b8f1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h

@@ -52,7 +52,7 @@
   #define GUC_DOORBELL_INVALID		256
-#define GUC_WQ_SIZE			(PAGE_SIZE * 2)
+#define GUC_WQ_SIZE			(PAGE_SIZE / 2)

Is this size actually dictated by the GuC API? Or is it just a driver level
decision? If the latter, shouldn't this be below instead?

Driver level decision. What exactly do you mean by below?

The next chunk of the patch - where WQ_OFFSET is defined and the whole caboodle is described.

quoted

quoted

   /* Work queue item header definitions */
   #define WQ_STATUS_ACTIVE		1

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 14b24298cdd7..dbcb9ab28a9a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c

@@ -340,6 +340,39 @@ static struct i915_priolist *to_priolist(struct rb_node *rb)
   	return rb_entry(rb, struct i915_priolist, node);
   }
+/*
+ * When using multi-lrc submission an extra page in the context state is
+ * reserved for the process descriptor and work queue.
+ *
+ * The layout of this page is below:
+ * 0						guc_process_desc
+ * ...						unused
+ * PAGE_SIZE / 2				work queue start
+ * ...						work queue
+ * PAGE_SIZE - 1				work queue end
+ */
+#define WQ_OFFSET	(PAGE_SIZE / 2)

Can this not be derived from GUC_WQ_SIZE given that the two are
fundamentally linked? E.g. '#define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'?

Yes. I like 'define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'. Will change.

quoted

And maybe have a '#define WQ_TOTAL_SIZE PAGE_SIZE' and use that in all of
WQ_OFFSET, GUC_WQ_SIZE and the allocation itself in intel_lrc.c?

Also, the process descriptor is actually an array of descriptors sized by
the number of children? Or am I misunderstanding the code below? In so,

No, it is fixed size descriptor.

Yeah, I think I was getting confused between pdesc and desc in the code below.

I still think it would be a good idea to have everything explicitly named and the only mention of PAGE_SIZE is in the 'total size' definition.

John.

A later patch in the series uses the space of above the process
descriptor for insertation of preeemption points handshake. That does
depend on the number of children. I will add a COMPILE_BUG_ON for that
to ensure everything fits in the memory layout.

quoted

shouldn't there be a 'COMPILE_BUG_ON((MAX_ENGINE_INSTANCE *
sizeof(descriptor)) < (WQ_DESC_SIZE)' where WQ_DESC_SIZE is WQ_TOTAL_SIZE -
WQ_SIZE?

quoted

+static u32 __get_process_desc_offset(struct intel_context *ce)
+{
+	GEM_BUG_ON(!ce->parent_page);
+
+	return ce->parent_page * PAGE_SIZE;
+}
+
+static u32 __get_wq_offset(struct intel_context *ce)
+{
+	return __get_process_desc_offset(ce) + WQ_OFFSET;
+}
+
+static struct guc_process_desc *
+__get_process_desc(struct intel_context *ce)
+{
+	return (struct guc_process_desc *)
+		(ce->lrc_reg_state +
+		 ((__get_process_desc_offset(ce) -
+		   LRC_STATE_OFFSET) / sizeof(u32)));

Where did the LRC_STATE_OFFSET come from? Is that built in to the
lrg_reg_state pointer itself? That needs to be documented somewhere.

In gt/intel_lrc.c (lrc_pin) ce->lrc_reg_state is assigned to
mmap(ce->state) + LRC_STATE_OFFSET. I can update the kerneldoc for that
field in this patch.

quoted

quoted

+}
+
   static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
   {
   	struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;

@@ -1342,6 +1375,30 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce)
   	spin_unlock_irqrestore(&guc->submission_state.lock, flags);
   }
+static int __guc_action_register_multi_lrc(struct intel_guc *guc,
+					   struct intel_context *ce,
+					   u32 guc_id,
+					   u32 offset,
+					   bool loop)
+{
+	struct intel_context *child;
+	u32 action[4 + MAX_ENGINE_INSTANCE];
+	int len = 0;
+
+	GEM_BUG_ON(ce->guc_number_children > MAX_ENGINE_INSTANCE);
+
+	action[len++] = INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC;
+	action[len++] = guc_id;
+	action[len++] = ce->guc_number_children + 1;
+	action[len++] = offset;
+	for_each_child(ce, child) {
+		offset += sizeof(struct guc_lrc_desc);
+		action[len++] = offset;
+	}
+
+	return guc_submission_send_busy_loop(guc, action, len, 0, loop);
+}
+
   static int __guc_action_register_context(struct intel_guc *guc,
   					 u32 guc_id,
   					 u32 offset,

@@ -1364,9 +1421,15 @@ static int register_context(struct intel_context *ce, bool loop)
   		ce->guc_id.id * sizeof(struct guc_lrc_desc);
   	int ret;
+	GEM_BUG_ON(intel_context_is_child(ce));
   	trace_intel_context_register(ce);
-	ret = __guc_action_register_context(guc, ce->guc_id.id, offset, loop);
+	if (intel_context_is_parent(ce))
+		ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id,
+						      offset, loop);
+	else
+		ret = __guc_action_register_context(guc, ce->guc_id.id, offset,
+						    loop);
   	if (likely(!ret)) {
   		unsigned long flags;

@@ -1396,6 +1459,7 @@ static int deregister_context(struct intel_context *ce, u32 guc_id, bool loop)
   {
   	struct intel_guc *guc = ce_to_guc(ce);
+	GEM_BUG_ON(intel_context_is_child(ce));
   	trace_intel_context_deregister(ce);
   	return __guc_action_deregister_context(guc, guc_id, loop);

@@ -1423,6 +1487,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
   	struct guc_lrc_desc *desc;
   	bool context_registered;
   	intel_wakeref_t wakeref;
+	struct intel_context *child;
   	int ret = 0;
   	GEM_BUG_ON(!engine->mask);

@@ -1448,6 +1513,42 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
   	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
   	guc_context_policy_init(engine, desc);
+	/*
+	 * Context is a parent, we need to register a process descriptor
+	 * describing a work queue and register all child contexts.

Technically, this should say 'If the context is a parent'. Or just move it
to be inside the if block.

I will add the "If the".

quoted

quoted

+	 */
+	if (intel_context_is_parent(ce)) {
+		struct guc_process_desc *pdesc;
+
+		ce->guc_wqi_tail = 0;
+		ce->guc_wqi_head = 0;
+
+		desc->process_desc = i915_ggtt_offset(ce->state) +
+			__get_process_desc_offset(ce);
+		desc->wq_addr = i915_ggtt_offset(ce->state) +
+			__get_wq_offset(ce);
+		desc->wq_size = GUC_WQ_SIZE;
+
+		pdesc = __get_process_desc(ce);
+		memset(pdesc, 0, sizeof(*(pdesc)));
+		pdesc->stage_id = ce->guc_id.id;
+		pdesc->wq_base_addr = desc->wq_addr;
+		pdesc->wq_size_bytes = desc->wq_size;
+		pdesc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;

Should this not be inherited from the ce? And same below. Or are we not
using this priority in that way?

Honestly I don't think this field is used or maybe doesn't even exist
anymore. I'll check the GuC code and likely delete this or if it is
still present I'll inherited this from the ce.

Matt

quoted

John.

quoted

+		pdesc->wq_status = WQ_STATUS_ACTIVE;
+
+		for_each_child(ce, child) {
+			desc = __get_lrc_desc(guc, child->guc_id.id);
+
+			desc->engine_class =
+				engine_class_to_guc_class(engine->class);
+			desc->hw_context_desc = child->lrc.lrca;
+			desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
+			desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
+			guc_context_policy_init(engine, desc);
+		}
+	}
+
   	/*
   	 * The context_lookup xarray is used to determine if the hardware
   	 * context is currently registered. There are two cases in which it

@@ -2858,6 +2959,12 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx)
   		return NULL;
   	}
+	if (unlikely(intel_context_is_child(ce))) {
+		drm_err(&guc_to_gt(guc)->i915->drm,
+			"Context is child, desc_idx %u", desc_idx);
+		return NULL;
+	}
+
   	return ce;
   }

On Wed, Sep 15, 2021 at 01:23:19PM -0700, John Harrison wrote:

On 9/15/2021 12:31, Matthew Brost wrote:

quoted

On Wed, Sep 15, 2021 at 12:21:35PM -0700, John Harrison wrote:

quoted

On 8/20/2021 15:44, Matthew Brost wrote:

quoted

Add multi-lrc context registration H2G. In addition a workqueue and
process descriptor are setup during multi-lrc context registration as
these data structures are needed for multi-lrc submission.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
   drivers/gpu/drm/i915/gt/intel_context_types.h |  12 ++
   drivers/gpu/drm/i915/gt/intel_lrc.c           |   5 +
   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   2 +-
   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 109 +++++++++++++++++-
   4 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 0fafc178cf2c..6f567ebeb039 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h

@@ -232,8 +232,20 @@ struct intel_context {
   		/** @parent: pointer to parent if child */
   		struct intel_context *parent;
+
+		/** @guc_wqi_head: head pointer in work queue */
+		u16 guc_wqi_head;
+		/** @guc_wqi_tail: tail pointer in work queue */
+		u16 guc_wqi_tail;
+

These should be in the 'guc_state' sub-struct? Would be good to keep all GuC
specific content in one self-contained struct. Especially given the other
child/parent fields are no going to be guc_ prefixed any more.

Right now I have everything in guc_state protected by guc_state.lock,
these fields are not protected by this lock. IMO it is better to use a
different sub-structure for the parallel fields (even if anonymous).

Hmm, I still think it is bad to be scattering back-end specific fields
amongst regular fields. The GuC patches include a whole bunch of complaints
about execlist back-end specific stuff leaking through to the higher levels,
we really shouldn't be guilty of doing the same with GuC if at all possible.
At the very least, the GuC specific fields should be grouped together at the
end of the struct rather than inter-mingled.

How 2 different sub-structures - parallel (shared) & guc_parallel (guc specific)?

quoted

quoted

quoted

   		/** @guc_number_children: number of children if parent */
   		u8 guc_number_children;
+
+		/**
+		 * @parent_page: page in context used by parent for work queue,

Maybe 'page in context record'? Otherwise, exactly what 'context' is meant
here? It isn't the 'struct intel_context'. The contetx record is saved as
'ce->state' / 'ce->lrc_reg_state', yes? Is it possible to link to either of

It is the page in ce->state / page minus LRC reg offset in
ce->lrg_reg_state. Will update the commit to make that clear.

quoted

those field? Probably not given that they don't appear to have any kerneldoc
description :(. Maybe add that in too :).

quoted

+		 * work queue descriptor

Later on, it is described as 'process descriptor and work queue'. It would
be good to be consistent.

Yep. Will fix.

quoted

quoted

+		 */
+		u8 parent_page;
   	};
   #ifdef CONFIG_DRM_I915_SELFTEST

diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index bb4af4977920..0ddbad4e062a 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c

@@ -861,6 +861,11 @@ __lrc_alloc_state(struct intel_context *ce, struct intel_engine_cs *engine)
   		context_size += PAGE_SIZE;
   	}
+	if (intel_context_is_parent(ce)) {
+		ce->parent_page = context_size / PAGE_SIZE;
+		context_size += PAGE_SIZE;
+	}
+
   	obj = i915_gem_object_create_lmem(engine->i915, context_size, 0);
   	if (IS_ERR(obj))
   		obj = i915_gem_object_create_shmem(engine->i915, context_size);

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index fa4be13c8854..0e600a3b8f1e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h

@@ -52,7 +52,7 @@
   #define GUC_DOORBELL_INVALID		256
-#define GUC_WQ_SIZE			(PAGE_SIZE * 2)
+#define GUC_WQ_SIZE			(PAGE_SIZE / 2)

Is this size actually dictated by the GuC API? Or is it just a driver level
decision? If the latter, shouldn't this be below instead?

Driver level decision. What exactly do you mean by below?

The next chunk of the patch - where WQ_OFFSET is defined and the whole
caboodle is described.

quoted

quoted

quoted

   /* Work queue item header definitions */
   #define WQ_STATUS_ACTIVE		1

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 14b24298cdd7..dbcb9ab28a9a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c

@@ -340,6 +340,39 @@ static struct i915_priolist *to_priolist(struct rb_node *rb)
   	return rb_entry(rb, struct i915_priolist, node);
   }
+/*
+ * When using multi-lrc submission an extra page in the context state is
+ * reserved for the process descriptor and work queue.
+ *
+ * The layout of this page is below:
+ * 0						guc_process_desc
+ * ...						unused
+ * PAGE_SIZE / 2				work queue start
+ * ...						work queue
+ * PAGE_SIZE - 1				work queue end
+ */
+#define WQ_OFFSET	(PAGE_SIZE / 2)

Can this not be derived from GUC_WQ_SIZE given that the two are
fundamentally linked? E.g. '#define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'?

Yes. I like 'define WQ_OFFSET (PAGE_SIZE - GUC_WQ_SIZE)'. Will change.

quoted

And maybe have a '#define WQ_TOTAL_SIZE PAGE_SIZE' and use that in all of
WQ_OFFSET, GUC_WQ_SIZE and the allocation itself in intel_lrc.c?

Also, the process descriptor is actually an array of descriptors sized by
the number of children? Or am I misunderstanding the code below? In so,

No, it is fixed size descriptor.

Yeah, I think I was getting confused between pdesc and desc in the code
below.

I still think it would be a good idea to have everything explicitly named
and the only mention of PAGE_SIZE is in the 'total size' definition.

#define PARENT_SCRATCH_SIZE 	PAGE_SIZE?

Matt 

John.

quoted

A later patch in the series uses the space of above the process
descriptor for insertation of preeemption points handshake. That does
depend on the number of children. I will add a COMPILE_BUG_ON for that
to ensure everything fits in the memory layout.

quoted

shouldn't there be a 'COMPILE_BUG_ON((MAX_ENGINE_INSTANCE *
sizeof(descriptor)) < (WQ_DESC_SIZE)' where WQ_DESC_SIZE is WQ_TOTAL_SIZE -
WQ_SIZE?

quoted

+static u32 __get_process_desc_offset(struct intel_context *ce)
+{
+	GEM_BUG_ON(!ce->parent_page);
+
+	return ce->parent_page * PAGE_SIZE;
+}
+
+static u32 __get_wq_offset(struct intel_context *ce)
+{
+	return __get_process_desc_offset(ce) + WQ_OFFSET;
+}
+
+static struct guc_process_desc *
+__get_process_desc(struct intel_context *ce)
+{
+	return (struct guc_process_desc *)
+		(ce->lrc_reg_state +
+		 ((__get_process_desc_offset(ce) -
+		   LRC_STATE_OFFSET) / sizeof(u32)));

Where did the LRC_STATE_OFFSET come from? Is that built in to the
lrg_reg_state pointer itself? That needs to be documented somewhere.

In gt/intel_lrc.c (lrc_pin) ce->lrc_reg_state is assigned to
mmap(ce->state) + LRC_STATE_OFFSET. I can update the kerneldoc for that
field in this patch.

quoted

quoted

+}
+
   static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
   {
   	struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;

@@ -1342,6 +1375,30 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce)
   	spin_unlock_irqrestore(&guc->submission_state.lock, flags);
   }
+static int __guc_action_register_multi_lrc(struct intel_guc *guc,
+					   struct intel_context *ce,
+					   u32 guc_id,
+					   u32 offset,
+					   bool loop)
+{
+	struct intel_context *child;
+	u32 action[4 + MAX_ENGINE_INSTANCE];
+	int len = 0;
+
+	GEM_BUG_ON(ce->guc_number_children > MAX_ENGINE_INSTANCE);
+
+	action[len++] = INTEL_GUC_ACTION_REGISTER_CONTEXT_MULTI_LRC;
+	action[len++] = guc_id;
+	action[len++] = ce->guc_number_children + 1;
+	action[len++] = offset;
+	for_each_child(ce, child) {
+		offset += sizeof(struct guc_lrc_desc);
+		action[len++] = offset;
+	}
+
+	return guc_submission_send_busy_loop(guc, action, len, 0, loop);
+}
+
   static int __guc_action_register_context(struct intel_guc *guc,
   					 u32 guc_id,
   					 u32 offset,

@@ -1364,9 +1421,15 @@ static int register_context(struct intel_context *ce, bool loop)
   		ce->guc_id.id * sizeof(struct guc_lrc_desc);
   	int ret;
+	GEM_BUG_ON(intel_context_is_child(ce));
   	trace_intel_context_register(ce);
-	ret = __guc_action_register_context(guc, ce->guc_id.id, offset, loop);
+	if (intel_context_is_parent(ce))
+		ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id,
+						      offset, loop);
+	else
+		ret = __guc_action_register_context(guc, ce->guc_id.id, offset,
+						    loop);
   	if (likely(!ret)) {
   		unsigned long flags;

@@ -1396,6 +1459,7 @@ static int deregister_context(struct intel_context *ce, u32 guc_id, bool loop)
   {
   	struct intel_guc *guc = ce_to_guc(ce);
+	GEM_BUG_ON(intel_context_is_child(ce));
   	trace_intel_context_deregister(ce);
   	return __guc_action_deregister_context(guc, guc_id, loop);

@@ -1423,6 +1487,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
   	struct guc_lrc_desc *desc;
   	bool context_registered;
   	intel_wakeref_t wakeref;
+	struct intel_context *child;
   	int ret = 0;
   	GEM_BUG_ON(!engine->mask);

@@ -1448,6 +1513,42 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
   	desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
   	guc_context_policy_init(engine, desc);
+	/*
+	 * Context is a parent, we need to register a process descriptor
+	 * describing a work queue and register all child contexts.

Technically, this should say 'If the context is a parent'. Or just move it
to be inside the if block.

I will add the "If the".

quoted

quoted

+	 */
+	if (intel_context_is_parent(ce)) {
+		struct guc_process_desc *pdesc;
+
+		ce->guc_wqi_tail = 0;
+		ce->guc_wqi_head = 0;
+
+		desc->process_desc = i915_ggtt_offset(ce->state) +
+			__get_process_desc_offset(ce);
+		desc->wq_addr = i915_ggtt_offset(ce->state) +
+			__get_wq_offset(ce);
+		desc->wq_size = GUC_WQ_SIZE;
+
+		pdesc = __get_process_desc(ce);
+		memset(pdesc, 0, sizeof(*(pdesc)));
+		pdesc->stage_id = ce->guc_id.id;
+		pdesc->wq_base_addr = desc->wq_addr;
+		pdesc->wq_size_bytes = desc->wq_size;
+		pdesc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;

Should this not be inherited from the ce? And same below. Or are we not
using this priority in that way?

Honestly I don't think this field is used or maybe doesn't even exist
anymore. I'll check the GuC code and likely delete this or if it is
still present I'll inherited this from the ce.

Matt

quoted

John.

quoted

+		pdesc->wq_status = WQ_STATUS_ACTIVE;
+
+		for_each_child(ce, child) {
+			desc = __get_lrc_desc(guc, child->guc_id.id);
+
+			desc->engine_class =
+				engine_class_to_guc_class(engine->class);
+			desc->hw_context_desc = child->lrc.lrca;
+			desc->priority = GUC_CLIENT_PRIORITY_KMD_NORMAL;
+			desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
+			guc_context_policy_init(engine, desc);
+		}
+	}
+
   	/*
   	 * The context_lookup xarray is used to determine if the hardware
   	 * context is currently registered. There are two cases in which it

@@ -2858,6 +2959,12 @@ g2h_context_lookup(struct intel_guc *guc, u32 desc_idx)
   		return NULL;
   	}
+	if (unlikely(intel_context_is_child(ce))) {
+		drm_err(&guc_to_gt(guc)->i915->drm,
+			"Context is child, desc_idx %u", desc_idx);
+		return NULL;
+	}
+
   	return ce;
   }

On Wed, Sep 15, 2021 at 01:04:45PM -0700, John Harrison wrote:

On 8/20/2021 15:44, Matthew Brost wrote:

quoted

Assign contexts in parent-child relationship consecutive guc_ids. This
is accomplished by partitioning guc_id space between ones that need to
be consecutive (1/16 available guc_ids) and ones that do not (15/16 of
available guc_ids). The consecutive search is implemented via the bitmap
API.

This is a precursor to the full GuC multi-lrc implementation but aligns
to how GuC mutli-lrc interface is defined - guc_ids must be consecutive
when using the GuC multi-lrc interface.

v2:
  (Daniel Vetter)
   - Explictly state why we assign consecutive guc_ids

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |   6 +-
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 107 +++++++++++++-----
  2 files changed, 86 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index 023953e77553..3f95b1b4f15c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h

@@ -61,9 +61,13 @@ struct intel_guc {
  		 */
  		spinlock_t lock;
  		/**
-		 * @guc_ids: used to allocate new guc_ids
+		 * @guc_ids: used to allocate new guc_ids, single-lrc
  		 */
  		struct ida guc_ids;
+		/**
+		 * @guc_ids_bitmap: used to allocate new guc_ids, multi-lrc
+		 */
+		unsigned long *guc_ids_bitmap;
  		/** @num_guc_ids: number of guc_ids that can be used */
  		u32 num_guc_ids;
  		/** @max_guc_ids: max number of guc_ids that can be used */

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 00d54bb00bfb..e9dfd43d29a0 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c

@@ -125,6 +125,18 @@ guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count);
  #define GUC_REQUEST_SIZE 64 /* bytes */
+/*
+ * We reserve 1/16 of the guc_ids for multi-lrc as these need to be contiguous
+ * per the GuC submission interface. A different allocation algorithm is used
+ * (bitmap vs. ida) between multi-lrc and single-lrc hence the reason to

The 'hence' clause seems to be attached to the wrong reason. The id space is
partition because of the contiguous vs random requirements of multi vs
single LRC, not because a different allocator is used in one partion vs the
other.

Kinda? The reason I partitioned it because to algorithms are different,
we could a unified space with a single algorithm, right? It was just
easier split the space and use 2 already existing data structures rather
cook up an algorithm in a unified space. There isn't a requirement from
the GuC that the space is partitioned, the only requirement is multi-lrc
IDs are contiguous. All this being said, I think comment is correct.

quoted

+ * partition the guc_id space. We believe the number of multi-lrc contexts in
+ * use should be low and 1/16 should be sufficient. Minimum of 32 guc_ids for
+ * multi-lrc.
+ */
+#define NUMBER_MULTI_LRC_GUC_ID(guc) \
+	((guc)->submission_state.num_guc_ids / 16 > 32 ? \
+	 (guc)->submission_state.num_guc_ids / 16 : 32)
+
  /*
   * Below is a set of functions which control the GuC scheduling state which
   * require a lock.

@@ -1176,6 +1188,10 @@ int intel_guc_submission_init(struct intel_guc *guc)
  	INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
  	intel_gt_pm_unpark_work_init(&guc->submission_state.destroyed_worker,
  				     destroyed_worker_func);
+	guc->submission_state.guc_ids_bitmap =
+		bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
+	if (!guc->submission_state.guc_ids_bitmap)
+		return -ENOMEM;
  	return 0;
  }

@@ -1188,6 +1204,7 @@ void intel_guc_submission_fini(struct intel_guc *guc)
  	guc_lrc_desc_pool_destroy(guc);
  	guc_flush_destroyed_contexts(guc);
  	i915_sched_engine_put(guc->sched_engine);
+	bitmap_free(guc->submission_state.guc_ids_bitmap);
  }
  static void queue_request(struct i915_sched_engine *sched_engine,

@@ -1239,18 +1256,43 @@ static void guc_submit_request(struct i915_request *rq)
  	spin_unlock_irqrestore(&sched_engine->lock, flags);
  }
-static int new_guc_id(struct intel_guc *guc)
+static int new_guc_id(struct intel_guc *guc, struct intel_context *ce)
  {
-	return ida_simple_get(&guc->submission_state.guc_ids, 0,
-			      guc->submission_state.num_guc_ids, GFP_KERNEL |
-			      __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
+	int ret;
+
+	GEM_BUG_ON(intel_context_is_child(ce));
+
+	if (intel_context_is_parent(ce))
+		ret = bitmap_find_free_region(guc->submission_state.guc_ids_bitmap,
+					      NUMBER_MULTI_LRC_GUC_ID(guc),
+					      order_base_2(ce->guc_number_children
+							   + 1));
+	else
+		ret = ida_simple_get(&guc->submission_state.guc_ids,
+				     NUMBER_MULTI_LRC_GUC_ID(guc),
+				     guc->submission_state.num_guc_ids,
+				     GFP_KERNEL | __GFP_RETRY_MAYFAIL |
+				     __GFP_NOWARN);
+	if (unlikely(ret < 0))
+		return ret;
+
+	ce->guc_id.id = ret;
+	return 0;
  }
  static void __release_guc_id(struct intel_guc *guc, struct intel_context *ce)
  {
+	GEM_BUG_ON(intel_context_is_child(ce));
+
  	if (!context_guc_id_invalid(ce)) {
-		ida_simple_remove(&guc->submission_state.guc_ids,
-				  ce->guc_id.id);
+		if (intel_context_is_parent(ce))
+			bitmap_release_region(guc->submission_state.guc_ids_bitmap,
+					      ce->guc_id.id,
+					      order_base_2(ce->guc_number_children
+							   + 1));

Is there any check against adding/removing children when the guc_ids are
allocated? Presumably it shouldn't ever happen but if it did then the

I don't have any protection for that but adding something like this
isn't bad idea.

bitmap_release would not match the allocation. Maybe add BUG_ON(ce->guc_id)
to the parent/child link functions (if it's not there already?).

Do you something like below in this function?

GEM_BUG_ON(guc_id_not_is_use());

quoted

+		else
+			ida_simple_remove(&guc->submission_state.guc_ids,
+					  ce->guc_id.id);
  		reset_lrc_desc(guc, ce->guc_id.id);
  		set_context_guc_id_invalid(ce);
  	}

@@ -1267,49 +1309,60 @@ static void release_guc_id(struct intel_guc *guc, struct intel_context *ce)
  	spin_unlock_irqrestore(&guc->submission_state.lock, flags);
  }
-static int steal_guc_id(struct intel_guc *guc)
+static int steal_guc_id(struct intel_guc *guc, struct intel_context *ce)
  {
-	struct intel_context *ce;
-	int guc_id;
+	struct intel_context *cn;

Leaving this as 'ce' and calling the input parameter 'ce_in' would have made
for significantly easier to read diffs!

Yea probably but I think we should change the style to make diff easier
to read.

quoted

  	lockdep_assert_held(&guc->submission_state.lock);
+	GEM_BUG_ON(intel_context_is_child(ce));
+	GEM_BUG_ON(intel_context_is_parent(ce));
  	if (!list_empty(&guc->submission_state.guc_id_list)) {
-		ce = list_first_entry(&guc->submission_state.guc_id_list,
+		cn = list_first_entry(&guc->submission_state.guc_id_list,
  				      struct intel_context,
  				      guc_id.link);
-		GEM_BUG_ON(atomic_read(&ce->guc_id.ref));
-		GEM_BUG_ON(context_guc_id_invalid(ce));
-
-		list_del_init(&ce->guc_id.link);
-		guc_id = ce->guc_id.id;
+		GEM_BUG_ON(atomic_read(&cn->guc_id.ref));
+		GEM_BUG_ON(context_guc_id_invalid(cn));
+		GEM_BUG_ON(intel_context_is_child(cn));
+		GEM_BUG_ON(intel_context_is_parent(cn));
-		spin_lock(&ce->guc_state.lock);

As far as I can tell, the only actual change to this function (beyond
'ce_in->id = id' vs 'return id' and adding anti-family asserts) is that this
spinlock was dropped. However, I'm not seeing any replacement for it or any
comment about why the spinlock is no longer necessary.

Good catch, the lock shouldn't be dropped.

Matt
 

John.

quoted

-		clr_context_registered(ce);
-		spin_unlock(&ce->guc_state.lock);
+		list_del_init(&cn->guc_id.link);
+		ce->guc_id = cn->guc_id;
+		clr_context_registered(cn);
+		set_context_guc_id_invalid(cn);
-		set_context_guc_id_invalid(ce);
-		return guc_id;
+		return 0;
  	} else {
  		return -EAGAIN;
  	}
  }
-static int assign_guc_id(struct intel_guc *guc, u16 *out)
+static int assign_guc_id(struct intel_guc *guc, struct intel_context *ce)
  {
  	int ret;
  	lockdep_assert_held(&guc->submission_state.lock);
+	GEM_BUG_ON(intel_context_is_child(ce));
-	ret = new_guc_id(guc);
+	ret = new_guc_id(guc, ce);
  	if (unlikely(ret < 0)) {
-		ret = steal_guc_id(guc);
+		if (intel_context_is_parent(ce))
+			return -ENOSPC;
+
+		ret = steal_guc_id(guc, ce);
  		if (ret < 0)
  			return ret;
  	}
-	*out = ret;
+	if (intel_context_is_parent(ce)) {
+		struct intel_context *child;
+		int i = 1;
+
+		for_each_child(ce, child)
+			child->guc_id.id = ce->guc_id.id + i++;
+	}
+
  	return 0;
  }

@@ -1327,7 +1380,7 @@ static int pin_guc_id(struct intel_guc *guc, struct intel_context *ce)
  	might_lock(&ce->guc_state.lock);
  	if (context_guc_id_invalid(ce)) {
-		ret = assign_guc_id(guc, &ce->guc_id.id);
+		ret = assign_guc_id(guc, ce);
  		if (ret)
  			goto out_unlock;
  		ret = 1;	/* Indidcates newly assigned guc_id */

@@ -1369,8 +1422,10 @@ static void unpin_guc_id(struct intel_guc *guc, struct intel_context *ce)
  	unsigned long flags;
  	GEM_BUG_ON(atomic_read(&ce->guc_id.ref) < 0);
+	GEM_BUG_ON(intel_context_is_child(ce));
-	if (unlikely(context_guc_id_invalid(ce)))
+	if (unlikely(context_guc_id_invalid(ce) ||
+		     intel_context_is_parent(ce)))
  		return;
  	spin_lock_irqsave(&guc->submission_state.lock, flags);

On 15/09/2021 17:58, Matthew Brost wrote:

On Wed, Sep 15, 2021 at 09:24:15AM +0100, Tvrtko Ursulin wrote:

quoted

On 14/09/2021 19:04, Matthew Brost wrote:

quoted

On Tue, Sep 14, 2021 at 09:34:08AM +0100, Tvrtko Ursulin wrote:

quoted

8<

quoted

quoted

Today we have:

for_each intel_engines: // intel_engines is a flat list of all engines
	intel_engine_setup()

You propose to change it to:

for_each engine_class:
     for 0..max_global_engine_instance:
        for_each intel_engines:
           skip engine not present
           skip class not matching

           count logical instance

     for_each intel_engines:
        skip engine not present
        skip wrong class

        intel_engine_setup()


I propose:

// Leave as is:

for_each intel_engines:
     intel_engine_setup()

// Add:

for_each engine_class:
     logical = 0
     for_each gt->engine_class[class]:
        skip engine not present

        engine->logical_instance = logical++


When code which actually needs a preturbed "map" arrives you add that in to
this second loop.

See above, why introduce an algorithm that doesn't work for future parts
+ future patches are land imminently? It makes zero sense whatsoever.
With your proposal we would literally land code to just throw it away a
couple of months from now + break patches we intend to land soon. This

It sure works, it just walks the per class list instead of walking the flat
list skipping one class at the time.

Just add the map based transformation to the second pass later, when it
becomes required.

I can flatten the algorithm if that helps alleviate your concerns but
with that being said, I've played around this locally and IMO makes the
code way more ugly. Sure it eliminates some iterations of the loop but
who really cares about that in a one time setup function?

quoted

quoted

algorithm works and has no reason whatsoever to be optimal as it a one
time setup call. I really don't understand why we are still talking
about this paint color.

I don't think bike shedding is not an appropriate term when complaint is how
proposed algorithm is needlessly complicated.

Are you just ignoring the fact that the algorithm (map) is needed in
pending patches? IMO it is more complicated to write throw away code
when the proper algorithm is already written. If the logical mapping was
straight forward on all platforms as the ones currently upstream I would
100% agree with your suggestion, but it isn't on unembargoed platforms
eminently going upstream. The algorithm I have works for the current
platforms + the pending platforms. IMO is 100% acceptable to merge
something looking towards a known future.

FWIW my 2c is that unused bits detract from review. And my gut feeling still is that code can be written in a simpler way and that the map transform can still plug in easily on top in a later series.

I said FWIW since even if I am right you can still view my comments as external/community inputs at this point and proceed however you wish.

Regards,

Tvrtko

On 8/20/2021 15:44, Matthew Brost wrote:

Granted, this patch is just moving code from one file to another not changing it. However, I think it would be worth adding a blank line in here. Otherwise the 'this register' comment below can be confusingly read as referring to the send_regs.fw_domain entry above.

And maybe add a comment why it is a bug for the send_regs value to be set? I'm not seeing any obvious connection between it and the reset of this code.

Presumably all of these API changes are not actually new? They really came in with the reset of the v40 re-write? It's just that this is the first time we are using them and therefore need to finally update the defines?

Would be better to say '|| is_parent(ce)' rather than blocked? The reason for reason for claiming enabled when not is because it's a multi-LRC parent, right? Or can there be a parent that is neither enabled nor blocked for which we don't want to do the processing? But why would that make sense/be possible?

WQE or WQI?

This should have a BUG_ON check that the requested size fits within the WQ_LEN field?

Indeed, would be better to use the FIELD macros as they do that kind of thing for you.

As above, would be better to use FIELD macros instead of manual shifting.

John.

On 8/20/2021 15:44, Matthew Brost wrote: