Re: [Intel-gfx] [PATCH] drm/i915/guc: Initialize GuC submission locks and queues early

[John Harrison <john.c.harrison@intel.com>]

On 2/14/2022 17:11, Daniele Ceraolo Spurio wrote:
> Move initialization of submission-related spinlock, lists and workers to
> init_early. This fixes an issue where if the GuC init fails we might
> still try to get the lock in the context cleanup code. Note that it is
> safe to call the GuC context cleanup code even if the init failed
> because all contexts are initialized with an invalid GuC ID, which will
> cause the GuC side of the cleanup to be skipped, so it is easier to just
> make sure the variables are initialized than to special case the cleanup
> to handle the case when they're not.
>
> References: https://gitlab.freedesktop.org/drm/intel/-/issues/4932
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>

> ---
>   .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 27 ++++++++++---------
>   1 file changed, 14 insertions(+), 13 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 b3a429a92c0da..2160da2c83cbf 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1818,24 +1818,11 @@ int intel_guc_submission_init(struct intel_guc *guc)
>   	 */
>   	GEM_BUG_ON(!guc->lrc_desc_pool);
>   
> -	xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
> -
> -	spin_lock_init(&guc->submission_state.lock);
> -	INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
> -	ida_init(&guc->submission_state.guc_ids);
> -	INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> -	INIT_WORK(&guc->submission_state.destroyed_worker,
> -		  destroyed_worker_func);
> -	INIT_WORK(&guc->submission_state.reset_fail_worker,
> -		  reset_fail_worker_func);
> -
>   	guc->submission_state.guc_ids_bitmap =
>   		bitmap_zalloc(NUMBER_MULTI_LRC_GUC_ID(guc), GFP_KERNEL);
>   	if (!guc->submission_state.guc_ids_bitmap)
>   		return -ENOMEM;
>   
> -	spin_lock_init(&guc->timestamp.lock);
> -	INIT_DELAYED_WORK(&guc->timestamp.work, guc_timestamp_ping);
>   	guc->timestamp.ping_delay = (POLL_TIME_CLKS / gt->clock_frequency + 1) * HZ;
>   	guc->timestamp.shift = gpm_timestamp_shift(gt);
>   
> @@ -3831,6 +3818,20 @@ static bool __guc_submission_selected(struct intel_guc *guc)
>   
>   void intel_guc_submission_init_early(struct intel_guc *guc)
>   {
> +	xa_init_flags(&guc->context_lookup, XA_FLAGS_LOCK_IRQ);
> +
> +	spin_lock_init(&guc->submission_state.lock);
> +	INIT_LIST_HEAD(&guc->submission_state.guc_id_list);
> +	ida_init(&guc->submission_state.guc_ids);
> +	INIT_LIST_HEAD(&guc->submission_state.destroyed_contexts);
> +	INIT_WORK(&guc->submission_state.destroyed_worker,
> +		  destroyed_worker_func);
> +	INIT_WORK(&guc->submission_state.reset_fail_worker,
> +		  reset_fail_worker_func);
> +
> +	spin_lock_init(&guc->timestamp.lock);
> +	INIT_DELAYED_WORK(&guc->timestamp.work, guc_timestamp_ping);
> +
>   	guc->submission_state.num_guc_ids = GUC_MAX_LRC_DESCRIPTORS;
>   	guc->submission_supported = __guc_submission_supported(guc);
>   	guc->submission_selected = __guc_submission_selected(guc);