Re: [PATCH 12/45] drm/i915: Move the engine->destroy() vfunc onto the engine

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 25/04/2019 10:19, Chris Wilson wrote:
> Make the engine responsible for cleaning itself up!

Why? (Just a hint to explain in the commit message.)

> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine.h       |  4 ++
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c    | 63 ++++++++++----------
>   drivers/gpu/drm/i915/gt/intel_engine_types.h |  2 +-
>   drivers/gpu/drm/i915/gt/intel_lrc.c          | 30 ++++------
>   drivers/gpu/drm/i915/gt/intel_ringbuffer.c   | 35 +++++------
>   drivers/gpu/drm/i915/i915_drv.h              |  6 --
>   drivers/gpu/drm/i915/i915_gem.c              | 19 +-----
>   7 files changed, 66 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index 3e53f53bc52b..f5b0f27cecb6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -362,7 +362,11 @@ __intel_ring_space(unsigned int head, unsigned int tail, unsigned int size)
>   	return (head - tail - CACHELINE_BYTES) & (size - 1);
>   }
>   
> +int intel_engines_init_mmio(struct drm_i915_private *i915);
>   int intel_engines_setup(struct drm_i915_private *i915);
> +int intel_engines_init(struct drm_i915_private *i915);
> +void intel_engines_cleanup(struct drm_i915_private *i915);
> +
>   int intel_engine_init_common(struct intel_engine_cs *engine);
>   void intel_engine_cleanup_common(struct intel_engine_cs *engine);
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 4e30f3720eb7..65cdd0c4accc 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -303,6 +303,12 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   	engine->class = info->class;
>   	engine->instance = info->instance;
>   
> +	/*
> +	 * To be overridden by the backend on setup, however to facilitate
> +	 * cleanup on error during setup we always provide the destroy vfunc.
> +	 */
> +	engine->destroy = (typeof(engine->destroy))kfree;

Scheme overall is nice but has one fragile point in it.

I think we want a wrapper and then add some WARN_ON type safety if the 
engine has state allocated and the destroy vfunc accidentally still 
points to this one.

I suspect easiest would be to set an engine flag once it has been 
initialized.

> +
>   	engine->uabi_class = intel_engine_classes[info->class].uabi_class;
>   
>   	engine->context_size = __intel_engine_context_size(dev_priv,
> @@ -327,18 +333,31 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   	return 0;
>   }
>   
> +/**
> + * intel_engines_cleanup() - free the resources allocated for Command Streamers
> + * @dev_priv: i915 device private
> + */
> +void intel_engines_cleanup(struct drm_i915_private *i915)
> +{
> +	struct intel_engine_cs *engine;
> +	enum intel_engine_id id;
> +
> +	for_each_engine(engine, i915, id) {
> +		engine->destroy(engine);
> +		i915->engine[id] = NULL;
> +	}
> +}
> +
>   /**
>    * intel_engines_init_mmio() - allocate and prepare the Engine Command Streamers
>    * @dev_priv: i915 device private
>    *
>    * Return: non-zero if the initialization failed.
>    */
> -int intel_engines_init_mmio(struct drm_i915_private *dev_priv)
> +int intel_engines_init_mmio(struct drm_i915_private *i915)
>   {
> -	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
> -	const unsigned int engine_mask = INTEL_INFO(dev_priv)->engine_mask;
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> +	struct intel_device_info *device_info = mkwrite_device_info(i915);
> +	const unsigned int engine_mask = INTEL_INFO(i915)->engine_mask;
>   	unsigned int mask = 0;
>   	unsigned int i;
>   	int err;
> @@ -351,10 +370,10 @@ int intel_engines_init_mmio(struct drm_i915_private *dev_priv)
>   		return -ENODEV;
>   
>   	for (i = 0; i < ARRAY_SIZE(intel_engines); i++) {
> -		if (!HAS_ENGINE(dev_priv, i))
> +		if (!HAS_ENGINE(i915, i))
>   			continue;
>   
> -		err = intel_engine_setup(dev_priv, i);
> +		err = intel_engine_setup(i915, i);

While nosing around I noticed there is mmio access in setup. :(

>   		if (err)
>   			goto cleanup;
>   
> @@ -370,20 +389,19 @@ int intel_engines_init_mmio(struct drm_i915_private *dev_priv)
>   		device_info->engine_mask = mask;
>   
>   	/* We always presume we have at least RCS available for later probing */
> -	if (WARN_ON(!HAS_ENGINE(dev_priv, RCS0))) {
> +	if (WARN_ON(!HAS_ENGINE(i915, RCS0))) {
>   		err = -ENODEV;
>   		goto cleanup;
>   	}
>   
> -	RUNTIME_INFO(dev_priv)->num_engines = hweight32(mask);
> +	RUNTIME_INFO(i915)->num_engines = hweight32(mask);
>   
> -	i915_check_and_clear_faults(dev_priv);
> +	i915_check_and_clear_faults(i915);
>   
>   	return 0;
>   
>   cleanup:
> -	for_each_engine(engine, dev_priv, id)
> -		kfree(engine);
> +	intel_engines_cleanup(i915);
>   	return err;
>   }
>   
> @@ -397,7 +415,7 @@ int intel_engines_init(struct drm_i915_private *i915)
>   {
>   	int (*init)(struct intel_engine_cs *engine);
>   	struct intel_engine_cs *engine;
> -	enum intel_engine_id id, err_id;
> +	enum intel_engine_id id;
>   	int err;
>   
>   	if (HAS_EXECLISTS(i915))
> @@ -406,8 +424,6 @@ int intel_engines_init(struct drm_i915_private *i915)
>   		init = intel_ring_submission_init;
>   
>   	for_each_engine(engine, i915, id) {
> -		err_id = id;
> -
>   		err = init(engine);
>   		if (err)
>   			goto cleanup;
> @@ -416,14 +432,7 @@ int intel_engines_init(struct drm_i915_private *i915)
>   	return 0;
>   
>   cleanup:
> -	for_each_engine(engine, i915, id) {
> -		if (id >= err_id) {
> -			kfree(engine);
> -			i915->engine[id] = NULL;
> -		} else {
> -			i915->gt.cleanup_engine(engine);
> -		}
> -	}
> +	intel_engines_cleanup(i915);
>   	return err;
>   }
>   
> @@ -609,13 +618,7 @@ int intel_engines_setup(struct drm_i915_private *i915)
>   	return 0;
>   
>   cleanup:
> -	for_each_engine(engine, i915, id) {
> -		if (engine->cops)
> -			i915->gt.cleanup_engine(engine);
> -		else
> -			kfree(engine);
> -		i915->engine[id] = NULL;
> -	}
> +	intel_engines_cleanup(i915);
>   	return err;
>   }
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index d972c339309c..9d64e33f8427 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -420,7 +420,7 @@ struct intel_engine_cs {
>   	 */
>   	void		(*cancel_requests)(struct intel_engine_cs *engine);
>   
> -	void		(*cleanup)(struct intel_engine_cs *engine);
> +	void		(*destroy)(struct intel_engine_cs *engine);
>   
>   	struct intel_engine_execlists execlists;
>   
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index ea7794d368d1..2e74c792104e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -2336,26 +2336,6 @@ static void execlists_park(struct intel_engine_cs *engine)
>   	tasklet_kill(&engine->execlists.tasklet);
>   }
>   
> -/**
> - * intel_logical_ring_cleanup() - deallocate the Engine Command Streamer
> - * @engine: Engine Command Streamer.
> - */
> -void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *i915 = engine->i915;
> -
> -	if (engine->cleanup)
> -		engine->cleanup(engine);
> -
> -	intel_engine_cleanup_common(engine);
> -
> -	lrc_destroy_wa_ctx(engine);
> -
> -	engine->i915 = NULL;
> -	i915->engine[engine->id] = NULL;
> -	kfree(engine);
> -}
> -
>   void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
>   {
>   	engine->submit_request = execlists_submit_request;
> @@ -2378,10 +2358,20 @@ void intel_execlists_set_default_submission(struct intel_engine_cs *engine)
>   		engine->flags |= I915_ENGINE_HAS_PREEMPTION;
>   }
>   
> +static void execlists_destroy(struct intel_engine_cs *engine)
> +{
> +	intel_engine_cleanup_common(engine);
> +
> +	lrc_destroy_wa_ctx(engine);
> +	kfree(engine);
> +}
> +
>   static void
>   logical_ring_default_vfuncs(struct intel_engine_cs *engine)
>   {
>   	/* Default vfuncs which can be overriden by each engine. */
> +
> +	engine->destroy = execlists_destroy;
>   	engine->resume = execlists_resume;
>   
>   	engine->reset.prepare = execlists_reset_prepare;
> diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> index 5bcd3034a101..7a4804c47466 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
> @@ -1520,25 +1520,6 @@ static const struct intel_context_ops ring_context_ops = {
>   	.destroy = ring_context_destroy,
>   };
>   
> -void intel_engine_cleanup(struct intel_engine_cs *engine)
> -{
> -	struct drm_i915_private *dev_priv = engine->i915;
> -
> -	WARN_ON(INTEL_GEN(dev_priv) > 2 &&
> -		(ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
> -
> -	intel_ring_unpin(engine->buffer);
> -	intel_ring_put(engine->buffer);
> -
> -	if (engine->cleanup)
> -		engine->cleanup(engine);
> -
> -	intel_engine_cleanup_common(engine);
> -
> -	dev_priv->engine[engine->id] = NULL;
> -	kfree(engine);
> -}
> -
>   static int load_pd_dir(struct i915_request *rq,
>   		       const struct i915_hw_ppgtt *ppgtt)
>   {
> @@ -2130,6 +2111,20 @@ static void gen6_bsd_set_default_submission(struct intel_engine_cs *engine)
>   	engine->submit_request = gen6_bsd_submit_request;
>   }
>   
> +static void ring_destroy(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *dev_priv = engine->i915;
> +
> +	WARN_ON(INTEL_GEN(dev_priv) > 2 &&
> +		(ENGINE_READ(engine, RING_MI_MODE) & MODE_IDLE) == 0);
> +
> +	intel_ring_unpin(engine->buffer);
> +	intel_ring_put(engine->buffer);
> +
> +	intel_engine_cleanup_common(engine);
> +	kfree(engine);
> +}
> +
>   static void setup_irq(struct intel_engine_cs *engine)
>   {
>   	struct drm_i915_private *i915 = engine->i915;
> @@ -2158,6 +2153,8 @@ static void setup_common(struct intel_engine_cs *engine)
>   
>   	setup_irq(engine);
>   
> +	engine->destroy = ring_destroy;
> +
>   	engine->resume = xcs_resume;
>   	engine->reset.prepare = reset_prepare;
>   	engine->reset.reset = reset_ring;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5c77bf5b735b..5ca4df9a7428 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1995,8 +1995,6 @@ struct drm_i915_private {
>   
>   	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
>   	struct {
> -		void (*cleanup_engine)(struct intel_engine_cs *engine);
> -
>   		struct i915_gt_timelines {
>   			struct mutex mutex; /* protects list, tainted by GPU */
>   			struct list_head active_list;
> @@ -2722,9 +2720,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
>   extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
>   int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>   
> -int intel_engines_init_mmio(struct drm_i915_private *dev_priv);
> -int intel_engines_init(struct drm_i915_private *dev_priv);
> -
>   u32 intel_calculate_mcr_s_ss_select(struct drm_i915_private *dev_priv);
>   
>   /* intel_hotplug.c */
> @@ -3132,7 +3127,6 @@ int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
>   int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
>   void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
>   void i915_gem_fini(struct drm_i915_private *dev_priv);
> -void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>   			   unsigned int flags, long timeout);
>   void i915_gem_suspend(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4c1793b1012e..2fcec1bfb038 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4476,11 +4476,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   
>   	dev_priv->mm.unordered_timeline = dma_fence_context_alloc(1);
>   
> -	if (HAS_LOGICAL_RING_CONTEXTS(dev_priv))
> -		dev_priv->gt.cleanup_engine = intel_logical_ring_cleanup;
> -	else
> -		dev_priv->gt.cleanup_engine = intel_engine_cleanup;
> -
>   	i915_timelines_init(dev_priv);
>   
>   	ret = i915_gem_init_userptr(dev_priv);
> @@ -4601,7 +4596,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   err_pm:
>   	if (ret != -EIO) {
>   		intel_cleanup_gt_powersave(dev_priv);
> -		i915_gem_cleanup_engines(dev_priv);
> +		intel_engines_cleanup(dev_priv);
>   	}
>   err_context:
>   	if (ret != -EIO)
> @@ -4661,7 +4656,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>   	mutex_lock(&dev_priv->drm.struct_mutex);
>   	intel_uc_fini_hw(dev_priv);
>   	intel_uc_fini(dev_priv);
> -	i915_gem_cleanup_engines(dev_priv);
> +	intel_engines_cleanup(dev_priv);
>   	i915_gem_contexts_fini(dev_priv);
>   	i915_gem_fini_scratch(dev_priv);
>   	mutex_unlock(&dev_priv->drm.struct_mutex);
> @@ -4684,16 +4679,6 @@ void i915_gem_init_mmio(struct drm_i915_private *i915)
>   	i915_gem_sanitize(i915);
>   }
>   
> -void
> -i915_gem_cleanup_engines(struct drm_i915_private *dev_priv)
> -{
> -	struct intel_engine_cs *engine;
> -	enum intel_engine_id id;
> -
> -	for_each_engine(engine, dev_priv, id)
> -		dev_priv->gt.cleanup_engine(engine);
> -}
> -
>   void
>   i915_gem_load_init_fences(struct drm_i915_private *dev_priv)
>   {

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx