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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@xxxxxxxxx>]

On 2/17/2022 3:52 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.
>
> 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 | 45 ++++++++++++-------
>   1 file changed, 28 insertions(+), 17 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..0ab2d1a24bf6 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,17 +2237,13 @@ 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));
> @@ -2259,8 +2255,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);
>   
> @@ -2308,6 +2302,21 @@ 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;
> +
> +	context_registered = ctx_id_mapped(guc, desc_idx);
> +
> +	prepare_context_registration_info(ce);
>   
>   	/*
>   	 * The context_lookup xarray is used to determine if the hardware
> @@ -3145,7 +3154,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 +3642,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 +3670,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;
>