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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 22/03/16 11:20, Dave Gordon wrote:
> 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?

One example is maybe:

struct drm_i915_error_state {
	struct drm_i915_error_ring

So per-ring state of the total error state, which sounds equivalent to 
per-engine state of a context.

Or:

struct intel_fbc {
	struct intel_fbc_state_cache {

More verbose version like "struct intel_context_engine_state"?

"struct intel_engine_context" reads like a context of an engine to me, 
which sounds opposite to what it should be - state of an engine for a 
context.

?

Regards,

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