Re: [PATCH] drm/i915: Small compaction of the engine init code

Chris Wilson
2016-06-22T16:10:42Z

On Wed, Jun 22, 2016 at 04:55:51PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Effectively removes one layer of indirection between the mask of
> possible engines and the engine constructors. Instead of spelling
> out in code the mapping of HAS_<engine> to constructors, makes
> more use of the recently added data driven approach by putting
> engine constructor vfuncs into the table as well.
>
> Effect is fewer lines of source and smaller binary.
>
> At the same time simplify the error handling since engine
> destructors can run on unitialized engines anyway.
>
> Similar approach could be done for legacy submission is wanted.

Yup, long term plan is to reduce as much as the needless duplication
between the two/three (and kill of the dev_priv->gt.init_rings and
friends). Muttering was even afoot to seperate the legacy submission
code from the ring handling.

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
> /**
> * intel_logical_rings_init() - allocate, populate and init the Engine Command Streamers
> * @dev: DRM device.
> *
> - * This function inits the engines for an Execlists submission style (the equivalent in the
> - * legacy ringbuffer submission world would be i915_gem_init_engines). It does it only for
> - * those engines that are present in the hardware.
> + * This function inits the engines for an Execlists submission style (the
> + * equivalent in the legacy ringbuffer submission world would be
> + * i915_gem_init_engines). It does it only for those engines that are present in
> + * the hardware.
> *
> * Return: non-zero if the initialization failed.
> */
> int intel_logical_rings_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_engine_cs *engine;
> + unsigned int i;
> int ret;
>
> - ret = logical_render_ring_init(dev);
> - if (ret)
> - return ret;
> + BUILD_BUG_ON((1 << RCS) != RENDER_RING);
> + BUILD_BUG_ON((1 << BCS) != BLT_RING);
> + BUILD_BUG_ON((1 << VCS) != BSD_RING);
> + BUILD_BUG_ON((1 << VCS2) != BSD2_RING);
> + BUILD_BUG_ON((1 << VECS) != VEBOX_RING);

Heh, isn't that the very definition of those in the header.
Planning for some array compaction?
-Chris

--
Chris Wilson, Intel Open Source Technology Centre