On 22/06/16 17:59, Chris Wilson wrote:
On Wed, Jun 22, 2016 at 05:35:48PM +0100, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
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.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
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;
-
- if (HAS_BSD(dev)) {
- ret = logical_bsd_ring_init(dev);
- if (ret)
- goto cleanup_render_ring;
- }
-
- if (HAS_BLT(dev)) {
- ret = logical_blt_ring_init(dev);
- if (ret)
- goto cleanup_bsd_ring;
- }
-
- if (HAS_VEBOX(dev)) {
- ret = logical_vebox_ring_init(dev);
- if (ret)
- goto cleanup_blt_ring;
- }
-
- if (HAS_BSD2(dev)) {
- ret = logical_bsd2_ring_init(dev);
- if (ret)
- goto cleanup_vebox_ring;
+ for (i = 0; i < I915_NUM_ENGINES; i++) {
One more thing to consider is that logical_rings[] has an unspecified
size. Either we set NUM_ENGINES there or use ARRAY_SIZE here, either way
we risk not initialising all engines we expect.
I think we need:
unsigned mask = 0;
+ if (HAS_ENGINE(dev_priv, i)) {
+ engine = logical_ring_setup(dev_priv, i);
+ ret = logical_rings[i].init(engine);
+ if (ret)
+ goto cleanup;
mask |= intel_engine_flag(engine);
+ }
}
if (WARN_ON(mask != dev_priv->info.rings_mask))
mkwrite_intel_info(dev_priv)->rings_mask = mask;
To catch any future forgotten additions without exploding.
Hm, if logical_rings does not contain all required engines it can either
crash or, if somehow it magically manages to initialize it from random
data, still pass your test.
Perhaps we just need:
BUILD_BUG_ON(ARRAY_SIZE(logical_rings) == I915_NUM_ENGINES)
?
What is this mkwrite_intel_info thing? There is a facility nowadays to
write to the rodata section? :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
