Re: [PATCH v2 4/8] drm/i915/guc: Split guc_lrc_desc_pin apart

"Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx> · Fri, 25 Feb 2022 16:10:44 -0800

On 2/24/2022 4:06 PM, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

The LRC descriptor pool is going away. Further, the function that was
populating it was also doing a bunch of logic about the context
registration sequence. So, split that code apart into separate state
setup and try to register functions. Note that some of those 'try to
register' code paths actually undo the state setup and leave it to be
redone again later (with potentially different values). This is
inefficient. The next patch will correct this.

Also, move a comment about ignoring return values to the place where
the return values are actually ignored.

v2: Move some more splitting from a later patch (and do it correctly).

Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

Daniele

---
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 54 +++++++++++--------
  1 file changed, 33 insertions(+), 21 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 ad784e8068c7..e41e309b9e7e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -634,7 +634,7 @@ int intel_guc_wait_for_idle(struct intel_guc *guc, long timeout)
  					      true, timeout);
  }
  
-static int guc_lrc_desc_pin(struct intel_context *ce, bool loop);
+static int try_context_registration(struct intel_context *ce, bool loop);
  
  static int __guc_add_request(struct intel_guc *guc, struct i915_request *rq)
  {
@@ -932,7 +932,7 @@ static int guc_dequeue_one_context(struct intel_guc *guc)
  
  		if (unlikely(!ctx_id_mapped(guc, ce->guc_id.id) &&
  			     !intel_context_is_banned(ce))) {
-			ret = guc_lrc_desc_pin(ce, false);
+			ret = try_context_registration(ce, false);
  			if (unlikely(ret == -EPIPE)) {
  				goto deadlk;
  			} else if (ret == -EBUSY) {
@@ -2237,20 +2237,15 @@ static void guc_context_policy_init(struct intel_engine_cs *engine,
  	desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
  }
  
-static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
+static void prepare_context_registration_info(struct intel_context *ce)
  {
  	struct intel_engine_cs *engine = ce->engine;
-	struct intel_runtime_pm *runtime_pm = engine->uncore->rpm;
  	struct intel_guc *guc = &engine->gt->uc.guc;
  	u32 desc_idx = ce->guc_id.id;
  	struct guc_lrc_desc *desc;
-	bool context_registered;
-	intel_wakeref_t wakeref;
  	struct intel_context *child;
-	int ret = 0;
  
  	GEM_BUG_ON(!engine->mask);
-	GEM_BUG_ON(!sched_state_is_init(ce));
  
  	/*
  	 * Ensure LRC + CT vmas are is same region as write barrier is done
@@ -2259,11 +2254,6 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
  	GEM_BUG_ON(i915_gem_object_is_lmem(guc->ct.vma->obj) !=
  		   i915_gem_object_is_lmem(ce->ring->vma->obj));
  
-	context_registered = ctx_id_mapped(guc, desc_idx);
-
-	clr_ctx_id_mapping(guc, desc_idx);
-	set_ctx_id_mapping(guc, desc_idx, ce);
-
  	desc = __get_lrc_desc(guc, desc_idx);
  	desc->engine_class = engine_class_to_guc_class(engine->class);
  	desc->engine_submit_mask = engine->logical_mask;
@@ -2308,6 +2298,26 @@ static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
  
  		clear_children_join_go_memory(ce);
  	}
+}
+
+static int try_context_registration(struct intel_context *ce, bool loop)
+{
+	struct intel_engine_cs *engine = ce->engine;
+	struct intel_runtime_pm *runtime_pm = engine->uncore->rpm;
+	struct intel_guc *guc = &engine->gt->uc.guc;
+	intel_wakeref_t wakeref;
+	u32 desc_idx = ce->guc_id.id;
+	bool context_registered;
+	int ret = 0;
+
+	GEM_BUG_ON(!sched_state_is_init(ce));
+
+	context_registered = ctx_id_mapped(guc, desc_idx);
+
+	clr_ctx_id_mapping(guc, desc_idx);
+	set_ctx_id_mapping(guc, desc_idx, ce);
+
+	prepare_context_registration_info(ce);
  
  	/*
  	 * The context_lookup xarray is used to determine if the hardware
@@ -3145,7 +3155,7 @@ static int guc_request_alloc(struct i915_request *rq)
  	if (unlikely(ret < 0))
  		return ret;
  	if (context_needs_register(ce, !!ret)) {
-		ret = guc_lrc_desc_pin(ce, true);
+		ret = try_context_registration(ce, true);
  		if (unlikely(ret)) {	/* unwind */
  			if (ret == -EPIPE) {
  				disable_submission(guc);
@@ -3633,9 +3643,17 @@ static void guc_set_default_submission(struct intel_engine_cs *engine)
  static inline void guc_kernel_context_pin(struct intel_guc *guc,
  					  struct intel_context *ce)
  {
+	/*
+	 * Note: we purposefully do not check the returns below because
+	 * the registration can only fail if a reset is just starting.
+	 * This is called at the end of reset so presumably another reset
+	 * isn't happening and even it did this code would be run again.
+	 */
+
  	if (context_guc_id_invalid(ce))
  		pin_guc_id(guc, ce);
-	guc_lrc_desc_pin(ce, true);
+
+	try_context_registration(ce, true);
  }
  
  static inline void guc_init_lrc_mapping(struct intel_guc *guc)
@@ -3653,13 +3671,7 @@ static inline void guc_init_lrc_mapping(struct intel_guc *guc)
  	 * Also, after a reset the of the GuC we want to make sure that the
  	 * information shared with GuC is properly reset. The kernel LRCs are
  	 * not attached to the gem_context, so they need to be added separately.
-	 *
-	 * Note: we purposefully do not check the return of guc_lrc_desc_pin,
-	 * because that function can only fail if a reset is just starting. This
-	 * is at the end of reset so presumably another reset isn't happening
-	 * and even it did this code would be run again.
  	 */
-
  	for_each_engine(engine, gt, id) {
  		struct intel_context *ce;
  







