[Intel-gfx] [PATCH 12/45] drm/i915: Move the engine->destroy() vfunc onto the engine
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Apr 25 11:01:36 UTC 2019
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 at chris-wilson.co.uk>
> ---
> 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
More information about the Intel-gfx
mailing list