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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Thu, Jun 23, 2016 at 02:16:55PM +0100, Tvrtko Ursulin wrote:
> 
> On 23/06/16 13:11, Chris Wilson wrote:
> >On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 23/06/16 12:25, Chris Wilson wrote:
> >>>On Thu, Jun 23, 2016 at 12:12:29PM +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.
> >>>>
> >>>>v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> >>>>     ENGINE_MASK and HAS_ENGINE macros.
> >>>>     Also removed the forward declarations by shuffling functions
> >>>>     around.
> >>>>
> >>>>v3: Warn when logical_rings table does not contain enough data
> >>>>     and disable the engines which could not be initialized.
> >>>>     (Chris Wilson)
> >>>
> >>>I was happy with the BUILD_BUG suggestion :)
> >>
> >>I've changed my mind later. :)
> >>
> >>>>+	for (i = 0;
> >>>>+		i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
> >>>
> >>>HAS_ENGINE() == false if i >= I915_NUM_ENGINES
> >>
> >>Don't follow. :) Why is v3 not good enough?
> >
> >Both (all three) is overkill.
> >
> >I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
> >succinctly. For belt and braces,
> >
> >WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));
> 
> I don't think this works - you meant testing that bits higher than
> BIT(I915_NUM_ENGINES) were not set in ring_mask?

Yes. Higher than BIT(NUM_ENGINES-1), just to make sure we don't end up
setting garbage at some point.
 
> And it probably belongs somewhere else, in common code which
> initializes intel_device_info I think.

Possibly. I think the engine initialisation path is a good choice
though, since that has the knowlege of what will be setup.

Either here (with the goal of using this as the basis for future
unification), or in the caller.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx