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