Re: [PATCH v2] drm/i915: Name the anonymous per-engine context struct

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

On 22/03/16 09:28, Chris Wilson wrote:
> On Mon, Mar 21, 2016 at 03:23:57PM +0000, Dave Gordon wrote:
>> On 18/03/16 17:26, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> This anonymous struct was causing a good amount of overly
>>> verbose code. Also, if we name it and cache the pointer locally
>>> when there are multiple accesses to it, not only the code is
>>> more readable, but the compiler manages to generate smaller
>>> binary.
>>>
>>> Along the way I also shortened access to dev_priv and eliminated
>>> some unused variables and cached some where I spotted the
>>> opportunity.
>>>
>>> Name for the structure, intel_context_engine, and the local
>>> variable name were borrowed from a similar patch by Chris Wilson.
>>>
>>> v2: Hate the engine->dev surprises, really do.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>   drivers/gpu/drm/i915/i915_drv.h  |  2 +-
>>>   drivers/gpu/drm/i915/intel_lrc.c | 94 +++++++++++++++++++++-------------------
>>>   2 files changed, 50 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 00c41a4bde2a..480639c39543 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -840,7 +840,7 @@ struct intel_context {
>>>   	} legacy_hw_ctx;
>>>
>>>   	/* Execlists */
>>> -	struct {
>>> +	struct intel_context_engine {
>>
>> Good idea, I had a version of this too, derived from Chris' patch
>> [157/190] drm/i915: Tidy execlists by using intel_context_engine locals.
>>
>> The only thing to disagree with is the actual name; it should be
>> "intel_engine_context" (or some abbreviation thereof), because in
>
> We have been using object_subobject.
> -Chris

No we haven't. Examples of the above convention for structs include:

* "struct intel_device_info" - information about an intel device
* "struct i915_ctx_hang_stats" - statistics about hangs, per context
* "struct intel_uncore_funcs" - functions exported from uncore
* "struct i915_power_well_ops" - operations on power wells
* "struct i915_suspend_saved_registers" - registers saved at suspend

and for instance names:

* "struct list_head request_list" - a list of requests
* "struct i915_vma *lrc_vma" - the VMA for an LRC
* "uint32_t *lrc_reg_state" - the state of the registers in an LRC

Indeed it's quite difficult to find any compound names that do *not* 
follow this convention. Maybe your version would be more natural in a 
language where adjectives (or possessives) normally follow the noun they 
describe?

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx