Re: [PATCH 20/26] drm/i915/guc: Implement no mid batch preemption for multi-lrc

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/4/2021 15:06, Matthew Brost wrote:
For some users of multi-lrc, e.g. split frame, it isn't safe to preempt
mid BB. To safely enable preemption at the BB boundary, a handshake
between to parent and child is needed. This is implemented via custom
between to parent -> between parent
emit_bb_start & emit_fini_breadcrumb functions and enabled via by
via by -> by

I'm also not seeing any mention of the forced re-group behavioural change in either the comments or commit description.

default if a context is configured by set parallel extension.

v2:
  (John Harrison)
   - Fix a few comments wording
   - Add struture for parent page layout

Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
---
  drivers/gpu/drm/i915/gt/intel_context.c       |   2 +-
  drivers/gpu/drm/i915/gt/intel_context_types.h |   2 +
  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   |   2 +-
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 330 +++++++++++++++++-
  4 files changed, 324 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 3b340eb59ada..ee84259959d0 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -569,7 +569,7 @@ void intel_context_bind_parent_child(struct intel_context *parent,
  	GEM_BUG_ON(intel_context_is_child(child));
  	GEM_BUG_ON(intel_context_is_parent(child));
- parent->parallel.number_children++;
+	parent->parallel.child_index = parent->parallel.number_children++;
  	list_add_tail(&child->parallel.child_link,
  		      &parent->parallel.child_list);
  	child->parallel.parent = parent;
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 1d880303a7e4..95a5b94b4ece 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -250,6 +250,8 @@ struct intel_context {
  		struct i915_request *last_rq;
  		/** @number_children: number of children if parent */
  		u8 number_children;
+		/** @child_index: index into child_list if child */
+		u8 child_index;
  		/** @guc: GuC specific members for parallel submission */
  		struct {
  			/** @wqi_head: head pointer in work queue */
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 a00eeddc1449..663950d3badc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -181,7 +181,7 @@ struct guc_process_desc {
  	u32 wq_status;
  	u32 engine_presence;
  	u32 priority;
-	u32 reserved[30];
+	u32 reserved[36];
Not seeing the promised explanation of this bug fix.

  } __packed;
#define CONTEXT_REGISTRATION_FLAG_KMD BIT(0)
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 12ee8ca76249..f28e36aa77c2 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -11,6 +11,7 @@
  #include "gt/intel_context.h"
  #include "gt/intel_engine_pm.h"
  #include "gt/intel_engine_heartbeat.h"
+#include "gt/intel_gpu_commands.h"
  #include "gt/intel_gt.h"
  #include "gt/intel_gt_irq.h"
  #include "gt/intel_gt_pm.h"
@@ -368,10 +369,16 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
/*
   * When using multi-lrc submission an extra page in the context state is
- * reserved for the process descriptor and work queue.
+ * reserved for the process descriptor, work queue, and handshake between the
+ * parent + childlren contexts to insert safe preemption points between each set
+ * of BBs.
   *
   * The layout of this page is below:
   * 0						guc_process_desc
+ * + sizeof(struct guc_process_desc)		child go
+ * + CACHELINE_BYTES				child join[0]
+ * ...
+ * + CACHELINE_BYTES				child join[n - 1]
   * ...						unused
   * PAGE_SIZE / 2				work queue start
   * ...						work queue
@@ -379,7 +386,25 @@ static inline struct i915_priolist *to_priolist(struct rb_node *rb)
   */
  #define WQ_SIZE			(PAGE_SIZE / 2)
  #define WQ_OFFSET		(PAGE_SIZE - WQ_SIZE)
-static u32 __get_process_desc_offset(struct intel_context *ce)
+
+struct parent_page {
+	struct guc_process_desc pdesc;
+
+	u32 child_go_memory;
+	u8 unused0[CACHELINE_BYTES - sizeof(u32)];
+
+	struct {
+		u32 child_join_memory;
+		u8 unused1[CACHELINE_BYTES - sizeof(u32)];
+	} join[MAX_ENGINE_INSTANCE + 1];
Could have a common structure for these. Call the u32 'semaphore_memory' or something then just have:
  struct sync_semaphore go;
  struct sync_semaphore go[MAX + 1];

+
+	u8 unused2[(WQ_OFFSET - sizeof(struct guc_process_desc) -
+		    CACHELINE_BYTES * (MAX_ENGINE_INSTANCE + 2))];
And this bit could be 'sizeof(struct sync_semaphore) * MAX + 2' to be clearer what it refers to.

And to be totally paranoid about it, could also add 'BUILD_BUG_ON(sizeof(struct sync_semaphore) != CACHELINE_BYTES'.

And BUILD_BUG_ON(sizeof(parent_page) == PARENT_PAGE_SIZE)'.

+
+	u32 wq[WQ_SIZE / sizeof(u32)];
+};
+
+static u32 __get_parent_page_offset(struct intel_context *ce)
  {
  	GEM_BUG_ON(!ce->parallel.guc.parent_page);
@@ -388,23 +413,35 @@ static u32 __get_process_desc_offset(struct intel_context *ce) static u32 __get_wq_offset(struct intel_context *ce)
  {
-	return __get_process_desc_offset(ce) + WQ_OFFSET;
+	BUILD_BUG_ON(offsetof(struct parent_page, wq) != WQ_OFFSET);
+
+	return __get_parent_page_offset(ce) + WQ_OFFSET;
  }
-static struct guc_process_desc *
-__get_process_desc(struct intel_context *ce)
+static struct parent_page *
+__get_parent_page(struct intel_context *ce)
  {
+	BUILD_BUG_ON(sizeof(struct parent_page) != PAGE_SIZE);
+
  	/*
  	 * Need to subtract LRC_STATE_OFFSET here as the
  	 * parallel.guc.parent_page is the offset into ce->state while
  	 * ce->lrc_reg_reg is ce->state + LRC_STATE_OFFSET.
  	 */
-	return (struct guc_process_desc *)
+	return (struct parent_page *)
  		(ce->lrc_reg_state +
-		 ((__get_process_desc_offset(ce) -
+		 ((__get_parent_page_offset(ce) -
  		   LRC_STATE_OFFSET) / sizeof(u32)));
  }
+static struct guc_process_desc *
+__get_process_desc(struct intel_context *ce)
+{
+	struct parent_page *pp = __get_parent_page(ce);
+
+	return &pp->pdesc;
+}
+
  static u32 *get_wq_pointer(struct guc_process_desc *desc,
  			   struct intel_context *ce,
  			   u32 wqi_size)
@@ -424,8 +461,7 @@ static u32 *get_wq_pointer(struct guc_process_desc *desc,
  	}
  #undef AVAILABLE_SPACE
- return ((u32 *)__get_process_desc(ce)) +
-		((WQ_OFFSET + ce->parallel.guc.wqi_tail) / sizeof(u32));
+	return &__get_parent_page(ce)->wq[ce->parallel.guc.wqi_tail / sizeof(u32)];
  }
static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
@@ -1829,6 +1865,26 @@ static int deregister_context(struct intel_context *ce, u32 guc_id)
  	return __guc_action_deregister_context(guc, guc_id);
  }
+static inline void clear_children_join_go_memory(struct intel_context *ce)
+{
+	u32 *mem = (u32 *)(&__get_parent_page(ce)->child_go_memory);
+	u8 i;
+
+	for (i = 0; i < ce->parallel.number_children + 1; ++i)
+		mem[i * (CACHELINE_BYTES / sizeof(u32))] = 0;
Can't this be written as:
  pp->child_go_memory = 0;
  for(i = 0 to number_children)
    pp->child_join_memory = 0;

Seems like that would be much clearer than this magic casting and offsetting. I mean, that was the whole point of creating the parent_page structure.


+}
+
+static inline u32 get_children_go_value(struct intel_context *ce)
+{
+	return __get_parent_page(ce)->child_go_memory;
+}
+
+static inline u32 get_children_join_value(struct intel_context *ce,
+					  u8 child_index)
+{
+	return __get_parent_page(ce)->join[child_index].child_join_memory;
+}
+
  static void guc_context_policy_init(struct intel_engine_cs *engine,
  				    struct guc_lrc_desc *desc)
  {
@@ -1888,7 +1944,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
  		ce->parallel.guc.wqi_head = 0;
desc->process_desc = i915_ggtt_offset(ce->state) +
-			__get_process_desc_offset(ce);
+			__get_parent_page_offset(ce);
  		desc->wq_addr = i915_ggtt_offset(ce->state) +
  			__get_wq_offset(ce);
  		desc->wq_size = WQ_SIZE;
@@ -1910,6 +1966,8 @@ 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);
  		}
+
+		clear_children_join_go_memory(ce);
  	}
/*
@@ -2976,6 +3034,31 @@ static const struct intel_context_ops virtual_child_context_ops = {
  	.get_sibling = guc_virtual_get_sibling,
  };
+/*
+ * The below override of the breadcrumbs is enabled when the user configures a
+ * context for parallel submission (multi-lrc, parent-child).
+ *
+ * The overridden breadcrumbs implements an algorithm which allows the GuC to
+ * safely preempt all the hw contexts configured for parallel submission
+ * between each BB. The contract between the i915 and GuC is if the parent
+ * context can be preempted, all the children can be preempted, and the GuC will
+ * always try to preempt the parent before the children. A handshake between the
+ * parent / children breadcrumbs ensures the i915 holds up its end of the deal
+ * creating a window to preempt between each set of BBs.
+ */
+static int emit_bb_start_parent_no_preempt_mid_batch(struct i915_request *rq,
+						     u64 offset, u32 len,
+						     const unsigned int flags);
+static int emit_bb_start_child_no_preempt_mid_batch(struct i915_request *rq,
+						    u64 offset, u32 len,
+						    const unsigned int flags);
+static u32 *
+emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq,
+						 u32 *cs);
+static u32 *
+emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq,
+						u32 *cs);
+
  static struct intel_context *
  guc_create_parallel(struct intel_engine_cs **engines,
  		    unsigned int num_siblings,
@@ -3011,6 +3094,20 @@ guc_create_parallel(struct intel_engine_cs **engines,
  		}
  	}
+ parent->engine->emit_bb_start =
+		emit_bb_start_parent_no_preempt_mid_batch;
+	parent->engine->emit_fini_breadcrumb =
+		emit_fini_breadcrumb_parent_no_preempt_mid_batch;
+	parent->engine->emit_fini_breadcrumb_dw =
+		12 + 4 * parent->parallel.number_children;
+	for_each_child(parent, ce) {
+		ce->engine->emit_bb_start =
+			emit_bb_start_child_no_preempt_mid_batch;
+		ce->engine->emit_fini_breadcrumb =
+			emit_fini_breadcrumb_child_no_preempt_mid_batch;
+		ce->engine->emit_fini_breadcrumb_dw = 16;
+	}
+
  	kfree(siblings);
  	return parent;
@@ -3840,6 +3937,17 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc,
  			drm_printf(p, "\t\tWQI Status: %u\n\n",
  				   READ_ONCE(desc->wq_status));
+ if (ce->engine->emit_bb_start ==
+			    emit_bb_start_parent_no_preempt_mid_batch) {
+				u8 i;
+
+				drm_printf(p, "\t\tChildren Go: %u\n\n",
+					   get_children_go_value(ce));
+				for (i = 0; i < ce->parallel.number_children; ++i)
+					drm_printf(p, "\t\tChildren Join: %u\n",
+						   get_children_join_value(ce, i));
+			}
+
  			for_each_child(ce, child)
  				guc_log_context(p, child);
  		}
@@ -3847,6 +3955,208 @@ void intel_guc_submission_print_context_info(struct intel_guc *guc,
  	xa_unlock_irqrestore(&guc->context_lookup, flags);
  }
+static inline u32 get_children_go_addr(struct intel_context *ce)
+{
+	GEM_BUG_ON(!intel_context_is_parent(ce));
+	BUILD_BUG_ON(offsetof(struct parent_page, child_go_memory) !=
+		     sizeof(struct guc_process_desc));
+
+	return i915_ggtt_offset(ce->state) +
+		__get_parent_page_offset(ce) +
+		sizeof(struct guc_process_desc);
Rather than relying on the BUILD_BUG to make sure that the magic calculation matches the structure definition, can't this just say "ggtt_offset + pp_offset + offsetof(pp, child_go)"?

+}
+
+static inline u32 get_children_join_addr(struct intel_context *ce,
+					 u8 child_index)
+{
+	GEM_BUG_ON(!intel_context_is_parent(ce));
+
+	return get_children_go_addr(ce) + (child_index + 1) * CACHELINE_BYTES;
"ggtt_offset + pp_offset + offsetof(pp, child_join[i])"?


+}
+
+#define PARENT_GO_BB			1
+#define PARENT_GO_FINI_BREADCRUMB	0
+#define CHILD_GO_BB			1
+#define CHILD_GO_FINI_BREADCRUMB	0
+static int emit_bb_start_parent_no_preempt_mid_batch(struct i915_request *rq,
+						     u64 offset, u32 len,
+						     const unsigned int flags)
+{
+	struct intel_context *ce = rq->context;
+	u32 *cs;
+	u8 i;
+
+	GEM_BUG_ON(!intel_context_is_parent(ce));
+
+	cs = intel_ring_begin(rq, 10 + 4 * ce->parallel.number_children);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	/* Wait on children */
+	for (i = 0; i < ce->parallel.number_children; ++i) {
+		*cs++ = (MI_SEMAPHORE_WAIT |
+			 MI_SEMAPHORE_GLOBAL_GTT |
+			 MI_SEMAPHORE_POLL |
+			 MI_SEMAPHORE_SAD_EQ_SDD);
+		*cs++ = PARENT_GO_BB;
+		*cs++ = get_children_join_addr(ce, i);
+		*cs++ = 0;
+	}
+
+	/* Turn off preemption */
+	*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
+	*cs++ = MI_NOOP;
+
+	/* Tell children go */
+	cs = gen8_emit_ggtt_write(cs,
+				  CHILD_GO_BB,
+				  get_children_go_addr(ce),
+				  0);
+
+	/* Jump to batch */
+	*cs++ = MI_BATCH_BUFFER_START_GEN8 |
+		(flags & I915_DISPATCH_SECURE ? 0 : BIT(8));
+	*cs++ = lower_32_bits(offset);
+	*cs++ = upper_32_bits(offset);
+	*cs++ = MI_NOOP;
+
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
+static int emit_bb_start_child_no_preempt_mid_batch(struct i915_request *rq,
+						    u64 offset, u32 len,
+						    const unsigned int flags)
+{
+	struct intel_context *ce = rq->context;
+	struct intel_context *parent = intel_context_to_parent(ce);
+	u32 *cs;
+
+	GEM_BUG_ON(!intel_context_is_child(ce));
+
+	cs = intel_ring_begin(rq, 12);
+	if (IS_ERR(cs))
+		return PTR_ERR(cs);
+
+	/* Signal parent */
+	cs = gen8_emit_ggtt_write(cs,
+				  PARENT_GO_BB,
+				  get_children_join_addr(parent,
+							 ce->parallel.child_index),
+				  0);
+
+	/* Wait on parent for go */
+	*cs++ = (MI_SEMAPHORE_WAIT |
+		 MI_SEMAPHORE_GLOBAL_GTT |
+		 MI_SEMAPHORE_POLL |
+		 MI_SEMAPHORE_SAD_EQ_SDD);
+	*cs++ = CHILD_GO_BB;
+	*cs++ = get_children_go_addr(parent);
+	*cs++ = 0;
+
+	/* Turn off preemption */
+	*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
+
+	/* Jump to batch */
+	*cs++ = MI_BATCH_BUFFER_START_GEN8 |
+		(flags & I915_DISPATCH_SECURE ? 0 : BIT(8));
+	*cs++ = lower_32_bits(offset);
+	*cs++ = upper_32_bits(offset);
+
+	intel_ring_advance(rq, cs);
+
+	return 0;
+}
+
+static u32 *
+emit_fini_breadcrumb_parent_no_preempt_mid_batch(struct i915_request *rq,
+						 u32 *cs)
+{
+	struct intel_context *ce = rq->context;
+	u8 i;
+
+	GEM_BUG_ON(!intel_context_is_parent(ce));
+
+	/* Wait on children */
+	for (i = 0; i < ce->parallel.number_children; ++i) {
+		*cs++ = (MI_SEMAPHORE_WAIT |
+			 MI_SEMAPHORE_GLOBAL_GTT |
+			 MI_SEMAPHORE_POLL |
+			 MI_SEMAPHORE_SAD_EQ_SDD);
+		*cs++ = PARENT_GO_FINI_BREADCRUMB;
+		*cs++ = get_children_join_addr(ce, i);
+		*cs++ = 0;
+	}
+
+	/* Turn on preemption */
+	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	*cs++ = MI_NOOP;
+
You mentioned possibly needing to add an MI_ARB_CHECK in here but I'm not seeing it. Did the testing happen? I don't see that it should be necessary. Once you execute the MI_ARB_ENABLE, the CS can preempt anywhere, I thought? Even if it can't there should be an MI_ARB_CHECK added at the next level up after the breadcrumb code. Or do we not have those in between batches any more?

John.


+	/* Tell children go */
+	cs = gen8_emit_ggtt_write(cs,
+				  CHILD_GO_FINI_BREADCRUMB,
+				  get_children_go_addr(ce),
+				  0);
+
+	/* Emit fini breadcrumb */
+	cs = gen8_emit_ggtt_write(cs,
+				  rq->fence.seqno,
+				  i915_request_active_timeline(rq)->hwsp_offset,
+				  0);
+
+	/* User interrupt */
+	*cs++ = MI_USER_INTERRUPT;
+	*cs++ = MI_NOOP;
+
+	rq->tail = intel_ring_offset(rq, cs);
+
+	return cs;
+}
+
+static u32 *
+emit_fini_breadcrumb_child_no_preempt_mid_batch(struct i915_request *rq, u32 *cs)
+{
+	struct intel_context *ce = rq->context;
+	struct intel_context *parent = intel_context_to_parent(ce);
+
+	GEM_BUG_ON(!intel_context_is_child(ce));
+
+	/* Turn on preemption */
+	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
+	*cs++ = MI_NOOP;
+
+	/* Signal parent */
+	cs = gen8_emit_ggtt_write(cs,
+				  PARENT_GO_FINI_BREADCRUMB,
+				  get_children_join_addr(parent,
+							 ce->parallel.child_index),
+				  0);
+
+	/* Wait parent on for go */
+	*cs++ = (MI_SEMAPHORE_WAIT |
+		 MI_SEMAPHORE_GLOBAL_GTT |
+		 MI_SEMAPHORE_POLL |
+		 MI_SEMAPHORE_SAD_EQ_SDD);
+	*cs++ = CHILD_GO_FINI_BREADCRUMB;
+	*cs++ = get_children_go_addr(parent);
+	*cs++ = 0;
+
+	/* Emit fini breadcrumb */
+	cs = gen8_emit_ggtt_write(cs,
+				  rq->fence.seqno,
+				  i915_request_active_timeline(rq)->hwsp_offset,
+				  0);
+
+	/* User interrupt */
+	*cs++ = MI_USER_INTERRUPT;
+	*cs++ = MI_NOOP;
+
+	rq->tail = intel_ring_offset(rq, cs);
+
+	return cs;
+}
+
  static struct intel_context *
  guc_create_virtual(struct intel_engine_cs **siblings, unsigned int count,
  		   unsigned long flags)




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux