diff mbox

drm/i915: Update wa_ctx_emit() macro as per kernel coding guidelines

Message ID 1436347625-8800-1-git-send-email-arun.siluvery@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

arun.siluvery@linux.intel.com July 8, 2015, 9:27 a.m. UTC
wa_ctx_emit() depends on the name of a local variable; if the name of that
variable is changed then we get compile errors. In this case it is unlikely
to be changed as this macro is only used in this set of functions but
Kernel coding guidelines doesn't recommend doing this. It was my mistake
as I should have corrected it at the beginning but missed so correct
this before there are more usages of this macro (Bob Beckett).

https://www.kernel.org/doc/Documentation/CodingStyle,
Chapter 12, "Things to avoid when using macros", point 2):

"
2) macros that depend on having a local variable with a magic name:

   #define FOO(val) bar(index, val)

might look like a good thing, but it's confusing as hell when one reads the
code and it's prone to breakage from seemingly innocent changes.
"

v2: Optimization to avoid multiple evaluation of 'index' in the macro.
Since we invoke it multiple times, compiler, if it can, should be able to coalesce
them into a single condition and remove multiple WARN_ON checks (Chris).

Suggested-by: Robert Beckett <robert.beckett@intel.com>
Cc: Robert Beckett <robert.beckett@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 79 ++++++++++++++++++++--------------------
 1 file changed, 40 insertions(+), 39 deletions(-)

Comments

Chris Wilson July 8, 2015, 9:35 a.m. UTC | #1
On Wed, Jul 08, 2015 at 10:27:05AM +0100, Arun Siluvery wrote:
> wa_ctx_emit() depends on the name of a local variable; if the name of that
> variable is changed then we get compile errors. In this case it is unlikely
> to be changed as this macro is only used in this set of functions but
> Kernel coding guidelines doesn't recommend doing this. It was my mistake
> as I should have corrected it at the beginning but missed so correct
> this before there are more usages of this macro (Bob Beckett).
> 
> https://www.kernel.org/doc/Documentation/CodingStyle,
> Chapter 12, "Things to avoid when using macros", point 2):
> 
> "
> 2) macros that depend on having a local variable with a magic name:
> 
>    #define FOO(val) bar(index, val)
> 
> might look like a good thing, but it's confusing as hell when one reads the
> code and it's prone to breakage from seemingly innocent changes.
> "
> 
> v2: Optimization to avoid multiple evaluation of 'index' in the macro.
> Since we invoke it multiple times, compiler, if it can, should be able to coalesce
> them into a single condition and remove multiple WARN_ON checks (Chris).
> 
> Suggested-by: Robert Beckett <robert.beckett@intel.com>
> Cc: Robert Beckett <robert.beckett@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Simple mechanical change,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 23ff018..b8a3482 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1087,12 +1087,13 @@  static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request *req)
 	return 0;
 }
 
-#define wa_ctx_emit(batch, cmd)						\
+#define wa_ctx_emit(batch, index, cmd)					\
 	do {								\
-		if (WARN_ON(index >= (PAGE_SIZE / sizeof(uint32_t)))) {	\
+		int __index = (index)++;				\
+		if (WARN_ON(__index >= (PAGE_SIZE / sizeof(uint32_t)))) { \
 			return -ENOSPC;					\
 		}							\
-		batch[index++] = (cmd);					\
+		batch[__index] = (cmd);					\
 	} while (0)
 
 
@@ -1118,29 +1119,29 @@  static inline int gen8_emit_flush_coherentl3_wa(struct intel_engine_cs *ring,
 {
 	uint32_t l3sqc4_flush = (0x40400000 | GEN8_LQSC_FLUSH_COHERENT_LINES);
 
-	wa_ctx_emit(batch, (MI_STORE_REGISTER_MEM_GEN8(1) |
-			    MI_SRM_LRM_GLOBAL_GTT));
-	wa_ctx_emit(batch, GEN8_L3SQCREG4);
-	wa_ctx_emit(batch, ring->scratch.gtt_offset + 256);
-	wa_ctx_emit(batch, 0);
-
-	wa_ctx_emit(batch, MI_LOAD_REGISTER_IMM(1));
-	wa_ctx_emit(batch, GEN8_L3SQCREG4);
-	wa_ctx_emit(batch, l3sqc4_flush);
-
-	wa_ctx_emit(batch, GFX_OP_PIPE_CONTROL(6));
-	wa_ctx_emit(batch, (PIPE_CONTROL_CS_STALL |
-			    PIPE_CONTROL_DC_FLUSH_ENABLE));
-	wa_ctx_emit(batch, 0);
-	wa_ctx_emit(batch, 0);
-	wa_ctx_emit(batch, 0);
-	wa_ctx_emit(batch, 0);
-
-	wa_ctx_emit(batch, (MI_LOAD_REGISTER_MEM_GEN8(1) |
-			    MI_SRM_LRM_GLOBAL_GTT));
-	wa_ctx_emit(batch, GEN8_L3SQCREG4);
-	wa_ctx_emit(batch, ring->scratch.gtt_offset + 256);
-	wa_ctx_emit(batch, 0);
+	wa_ctx_emit(batch, index, (MI_STORE_REGISTER_MEM_GEN8(1) |
+				   MI_SRM_LRM_GLOBAL_GTT));
+	wa_ctx_emit(batch, index, GEN8_L3SQCREG4);
+	wa_ctx_emit(batch, index, ring->scratch.gtt_offset + 256);
+	wa_ctx_emit(batch, index, 0);
+
+	wa_ctx_emit(batch, index, MI_LOAD_REGISTER_IMM(1));
+	wa_ctx_emit(batch, index, GEN8_L3SQCREG4);
+	wa_ctx_emit(batch, index, l3sqc4_flush);
+
+	wa_ctx_emit(batch, index, GFX_OP_PIPE_CONTROL(6));
+	wa_ctx_emit(batch, index, (PIPE_CONTROL_CS_STALL |
+				   PIPE_CONTROL_DC_FLUSH_ENABLE));
+	wa_ctx_emit(batch, index, 0);
+	wa_ctx_emit(batch, index, 0);
+	wa_ctx_emit(batch, index, 0);
+	wa_ctx_emit(batch, index, 0);
+
+	wa_ctx_emit(batch, index, (MI_LOAD_REGISTER_MEM_GEN8(1) |
+				   MI_SRM_LRM_GLOBAL_GTT));
+	wa_ctx_emit(batch, index, GEN8_L3SQCREG4);
+	wa_ctx_emit(batch, index, ring->scratch.gtt_offset + 256);
+	wa_ctx_emit(batch, index, 0);
 
 	return index;
 }
@@ -1201,7 +1202,7 @@  static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 	uint32_t index = wa_ctx_start(wa_ctx, *offset, CACHELINE_DWORDS);
 
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
-	wa_ctx_emit(batch, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+	wa_ctx_emit(batch, index, MI_ARB_ON_OFF | MI_ARB_DISABLE);
 
 	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw */
 	if (IS_BROADWELL(ring->dev)) {
@@ -1214,19 +1215,19 @@  static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 	/* Actual scratch location is at 128 bytes offset */
 	scratch_addr = ring->scratch.gtt_offset + 2*CACHELINE_BYTES;
 
-	wa_ctx_emit(batch, GFX_OP_PIPE_CONTROL(6));
-	wa_ctx_emit(batch, (PIPE_CONTROL_FLUSH_L3 |
-			    PIPE_CONTROL_GLOBAL_GTT_IVB |
-			    PIPE_CONTROL_CS_STALL |
-			    PIPE_CONTROL_QW_WRITE));
-	wa_ctx_emit(batch, scratch_addr);
-	wa_ctx_emit(batch, 0);
-	wa_ctx_emit(batch, 0);
-	wa_ctx_emit(batch, 0);
+	wa_ctx_emit(batch, index, GFX_OP_PIPE_CONTROL(6));
+	wa_ctx_emit(batch, index, (PIPE_CONTROL_FLUSH_L3 |
+				   PIPE_CONTROL_GLOBAL_GTT_IVB |
+				   PIPE_CONTROL_CS_STALL |
+				   PIPE_CONTROL_QW_WRITE));
+	wa_ctx_emit(batch, index, scratch_addr);
+	wa_ctx_emit(batch, index, 0);
+	wa_ctx_emit(batch, index, 0);
+	wa_ctx_emit(batch, index, 0);
 
 	/* Pad to end of cacheline */
 	while (index % CACHELINE_DWORDS)
-		wa_ctx_emit(batch, MI_NOOP);
+		wa_ctx_emit(batch, index, MI_NOOP);
 
 	/*
 	 * MI_BATCH_BUFFER_END is not required in Indirect ctx BB because
@@ -1262,9 +1263,9 @@  static int gen8_init_perctx_bb(struct intel_engine_cs *ring,
 	uint32_t index = wa_ctx_start(wa_ctx, *offset, CACHELINE_DWORDS);
 
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
-	wa_ctx_emit(batch, MI_ARB_ON_OFF | MI_ARB_ENABLE);
+	wa_ctx_emit(batch, index, MI_ARB_ON_OFF | MI_ARB_ENABLE);
 
-	wa_ctx_emit(batch, MI_BATCH_BUFFER_END);
+	wa_ctx_emit(batch, index, MI_BATCH_BUFFER_END);
 
 	return wa_ctx_end(wa_ctx, *offset = index, 1);
 }