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

Ceraolo Spurio, Daniele
2022-02-23T01:12:49Z

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