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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-25 12:01:36)
> 
> 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.)

... so we can remove the vfunc stuck away inside the i915->gt that has
been annoying the reader for the last several years.

> > 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.

Hmm, still fragile since you rely on the user? And if you don't rely on
the user, we just put a GEM_BUG_ON(engine->destroy == kfree) at the
point where we expect the backend to take control, i.e. after a
successful call to setup() in intel_engines_setup.

@@ -617,6 +617,9 @@ int intel_engines_setup(struct drm_i915_private *i915)
                if (err)
                        goto cleanup;

+               /* We expect the backend to take control over its state */
+               GEM_BUG_ON(engine->destroy == (typeof(engine->destroy))kfree);
+
                GEM_BUG_ON(!engine->cops);
> 
> > +
> >       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. :(

Yeah, we can take another step or two to split up the phases between sw
setup and HW setup.
-Chris