On 18/04/16 10:18, Jani Nikula wrote:
On Fri, 15 Apr 2016, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
Final canvas for opinions for using a magic macro to reduce typing in
the common operation of getting our drm_i915_private from the object.
21 files changed, 333 insertions(+), 392 deletions(-)
Not to mention the ease it makes for later patches to reduce the pointer
dance.
I've expressed my reservations about this the last time.
My compromise proposal is this: let's add the to_i915()
"superconvenience macro", but let's not embed that into other
macros. Instead, move away from convenience macros in them, explicitly
requiring dev_priv.
This would make just one macro special, and would keep the rest less
surprising and "C-like". We already need dev_priv all over the place, so
I don't think having a local variable or an explicit to_i915() is a big
burden.
BR,
Jani.
I'm quite happy with the first patch of this series (various renames),
but not so keen on the second (pass anything to for_each_engine(). The
reason for this is that the dev_priv parameter to this macro is used
repeatedly in the resulting loop, so the caller should be encouraged to
supply as an actual parameter the expression that needs the least
evaluation on each iteration i.e. a local variable. Allowing such things
as to_i915(dev) or, worse, to_i915(engine) may encourage uses which end
up generating four levels of memory indirection :(
e.g. engine->dev->dev_priv->engine(!)
So I think it should be regarded as better (more efficient) style to write
struct drm_i915_private *i915 = to_i915(...);
...
for_each_engine(engine, i915) {
...
}
rather than
for_each_engine(engine, to_i915(...)) {
...
}
OTOH I have no objection to
if (USES_FULL_PPGTT(to_i915(obj)) ...
(not a loop) but I'm not so keen on
if (USES_FULL_PPGTT(obj))
as this suggests that it's a property of the object, and different
objects might therefore return different values.
The GuC changes are OK, and we might as well convert "dev_priv" to
"i915" at the same time, if that's now the preferred name.
In the end, I think I agree with Jani: let's have a thoroughly
polymorphic to_i915() that works on anything, and then have everything
else require that.
Regards,
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
