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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux