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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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