[Intel-gfx] [PATCH 1/4] drm/i915/gt: Cleanup partial engine discovery failures

[Janusz Krzysztofik]

Hi Matt,

On Thursday, 15 September 2022 00:04:24 CEST Matt Roper wrote:
> From: Chris Wilson <chris.p.wilson at intel.com>
> 
> If we abort driver initialisation in the middle of gt/engine discovery,
> some engines will be fully setup and some not. Those incompletely setup
> engines only have 'engine->release == NULL' and so will leak any of the
> common objects allocated.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 1f7188129cd1..bff12b4ec314 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -1196,6 +1196,12 @@ void intel_engine_destroy_pinned_context(struct intel_context *ce)
>  	intel_context_put(ce);
>  }
>  
> +static void destroy_pinned_context(struct intel_context *ce)
> +{
> +	if (ce)
> +		intel_engine_destroy_pinned_context(ce);
> +}
> +
>  static struct intel_context *
>  create_kernel_context(struct intel_engine_cs *engine)
>  {
> @@ -1274,8 +1280,13 @@ int intel_engines_init(struct intel_gt *gt)
>  			return err;
>  
>  		err = setup(engine);
> -		if (err)
> +		if (err) {
> +			intel_engine_cleanup_common(engine);
>  			return err;
> +		}
> +
> +		/* The backend should now be responsible for cleanup */
> +		GEM_BUG_ON(engine->release == NULL);

LGTM

>  
>  		err = engine_init_common(engine);
>  		if (err)
> @@ -1307,8 +1318,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  	if (engine->default_state)
>  		fput(engine->default_state);
>  
> -	if (engine->kernel_context)
> -		intel_engine_destroy_pinned_context(engine->kernel_context);
> +	destroy_pinned_context(engine->kernel_context);

I think there is no point for this hunk, and consequently for the first one.  
A version of intel_engine_destroy_pinned_context() that accepts NULL can be 
added later, when we see need for similar code optimisation in more places, if 
ever.

Thanks,
Janusz

>  
>  	GEM_BUG_ON(!llist_empty(&engine->barrier_tasks));
>  	cleanup_status_page(engine);
>