On 02/12/15 13:46, Chris Wilson wrote:
On Wed, Dec 02, 2015 at 01:29:21PM +0000, Dave Gordon wrote:
On 25/11/15 09:23, Daniel Vetter wrote:
On Tue, Nov 24, 2015 at 11:47:26PM +0000, Chris Wilson wrote:
On Tue, Nov 24, 2015 at 10:26:01PM +0000, Chris Wilson wrote:
On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote:
/* Iterate over initialised rings */
#define for_each_ring(ring__, dev_priv__, i__) \
for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
- if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
+ for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
Idly wondering if we would be happy with
for_each_ring(ring__, dev_priv__)
for ((ring__) = &(dev_priv__)->ring[0];
(ring__) <= &(dev_priv__)->ring[I915_NUM_RINGS];
(ring__)++)
for_each_if(intel_ring_initialized(ring__))
?
The downside is that we have used i__ in several places rather than
ring->id.
Fwiw, 13 files changed, 113 insertions(+), 140 deletions(-)
Seems a reasonable shrinkage.
Maybe for_each_engine even, and phase out for_each_ring completely?
-Daniel
Wouldn't it be nicer (and safer) not to build macros that fold the
loop structure into the macro (in contravention of kernel
programming guidelines).
So how about NOT including the actual for() inside the macro, so
that instead of writing
for_each_engine(engine, dev_priv)
do_stuff(engine)
we would write it as
for (EACH_ENGINE(engine, dev_priv))
initialise(engine)
for (EACH_ACTIVE_ENGINE(engine, dev_priv)) {
service(engine)
restart(engine)
}
etc. The for-loop is visible and the scope doesn't give you any surprises.
[The EACH_ENGINE() macros expands to a semicolon-separated triplet
of expressions; still a violation of the "don't use macros to
redefine C syntax" guideline, but much less egregious than macros
that contain embedded 'for's and 'if's.
for_each() is common practice in the kernel, so hiding the for() inside
the macro isn't that egregious. The problem is defining EACH_ACTIVE_ENGINE()
simply, preferrably without the use of another loop inside the for(;;).
Nothing wrong with another loop :) Although, to keep it tidy, it can be
inside an inline helper function that skips over the unwanted items in
the iteration. I think it's better to have the if-ready condition test
inside the iterator (and therefore clearly bounded) than let it dangle
at the end of the macro.
One is to pack the i915->engines[] and have i915->num_engines and
intel_lookup_engine (or an i915->engine_for_id[]) for the occasional
case where we look up e.g. &i915->engines[BCS].
-Chris
Or, put the active ones on a linked list, or keep a bitmask of which
ones have been initialised inside the dev_priv structure, so you don't
have to even dereference the engine[] array to work out whether a
particular engine is initialised. Apropos which, wouldn't it be much
more efficient to do that, because intel_ring_initialized() is quite
heavyweight and the results surely don't change often, if at all, during
normal operation. So we should only evaluate it when something has
changed, and cache the bool result for use in all those for_each() loops!
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
