Re: [Intel-gfx] [PATCH 5/8] drm/i915/guc: Move lrc desc setup to where it is needed

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

On 2/17/2022 3:52 PM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> The LRC descriptor was being initialised early on in the context
> registration sequence. It could then be determined that the actual
> registration needs to be delayed and the descriptor would be wiped
> out. This is inefficient, so move the setup to later in the process
> after the point of no return.
>
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 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 0ab2d1a24bf6..aa74ec74194a 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2153,6 +2153,8 @@ static int __guc_action_register_context(struct intel_guc *guc,
>   					     0, loop);
>   }
>   
> +static void prepare_context_registration_info(struct intel_context *ce);
> +
>   static int register_context(struct intel_context *ce, bool loop)
>   {
>   	struct intel_guc *guc = ce_to_guc(ce);
> @@ -2163,6 +2165,8 @@ static int register_context(struct intel_context *ce, bool loop)
>   	GEM_BUG_ON(intel_context_is_child(ce));
>   	trace_intel_context_register(ce);
>   
> +	prepare_context_registration_info(ce);
> +
>   	if (intel_context_is_parent(ce))
>   		ret = __guc_action_register_multi_lrc(guc, ce, ce->guc_id.id,
>   						      offset, loop);
> @@ -2246,7 +2250,6 @@ static void prepare_context_registration_info(struct intel_context *ce)
>   	struct intel_context *child;
>   
>   	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
> @@ -2314,9 +2317,13 @@ static int try_context_registration(struct intel_context *ce, bool loop)
>   	bool context_registered;
>   	int ret = 0;
>   
> +	GEM_BUG_ON(!sched_state_is_init(ce));
> +
>   	context_registered = ctx_id_mapped(guc, desc_idx);
>   
> -	prepare_context_registration_info(ce);
> +	if (context_registered)
> +		clr_ctx_id_mapping(guc, desc_idx);
> +	set_ctx_id_mapping(guc, desc_idx, ce);

I think we can do the clr unconditionally. Also, should we drop the 
clr/set pair in prepare_context_registration_info? it shouldn't be 
needed, unless I'm missing a path where we don;t pass through here.

Daniele

>   
>   	/*
>   	 * The context_lookup xarray is used to determine if the hardware