Re: Polymorphic to_i915()

Dave Gordon
2016-04-18T11:11:10Z

On 18/04/16 10:18, Jani Nikula wrote:
> On Fri, 15 Apr 2016, Chris Wilson <chris@chris-wilson.co.uk> 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.