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

Tvrtko Ursulin
2016-06-22T16:21:03Z

On 22/06/16 17:10, Chris Wilson wrote:
> 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?

No I was trying to protect against someone changing the definitions of
RENDER_RING & co since the loop below this depends on that. Maybe it was
too paranoid. Or maybe better, I could add HAS_ENGINE(id) and cement
that in one place instead of this many BUILD_BUG_ONs.

I'll respin anyway to remove forward decls which can be avoided with
some reshuffle.

Regards,

Tvrtko