Re: Polymorphic to_i915()

[Dave Gordon <david.s.gordon@intel.com>]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx