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

Thu, 25 Apr 2019 04:02:25 -0700 


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 <ch...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx






















					Reply via email to