Re: [Intel-gfx] [PATCH v6] drm/i915: Tidy workaround batch buffer emission

[Chris Wilson]

On Fri, Feb 17, 2017 at 07:58:59AM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Use the "*batch++ = " style as in the ring emission for better
> readability and also simplify the logic a bit by consolidating
> the offset and size calculations and overflow checking. The
> latter is a programming error so it is not required to check
> for it after each write to the object, but rather do it once the
> whole state has been written and fail the driver if something
> went wrong.
> 
> v2: Rebase.
> 
> v3: Keep track of offsets and sizes in bytes for simplicity
> and rename function pointer variable to _fn suffix.
> (Chris Wilson)
> 
> v4: Fix size calc broken in v3 and add alignment warning. (Chris Wilson)
> 
> v5: Fix return code.
> 
> v6: I added an exit from loop in v5 but forgot to put back
> the object teardown.
> 
> Signed-off-by: Tvrtko Ursulin 
> Reviewed-by: Chris Wilson  (v5)
> Cc: Chris Wilson 

> + /*
> +  * Emit the two workaround batch buffers, recording the offset from the
> +  * start of the workaround batch buffer object for each and their
> +  * respective sizes.
> +  */
> + for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) {
> + wa_bb[i]->offset = batch_ptr - batch;
> + if (WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, CACHELINE_BYTES))) {
> + ret = -EINVAL;
> + break;
> + }
> + batch_ptr = wa_bb_fn[i](engine, batch_ptr);
> + wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
>   }
>  
> -out:
> + BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);

Ok, I didn't spot it last time, so hopefully not missing anything this
time!
Reviewed-by: Chris Wilson 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v6] drm/i915: Tidy workaround batch buffer emission

[Tvrtko Ursulin]

From: Tvrtko Ursulin 

Use the "*batch++ = " style as in the ring emission for better
readability and also simplify the logic a bit by consolidating
the offset and size calculations and overflow checking. The
latter is a programming error so it is not required to check
for it after each write to the object, but rather do it once the
whole state has been written and fail the driver if something
went wrong.

v2: Rebase.

v3: Keep track of offsets and sizes in bytes for simplicity
and rename function pointer variable to _fn suffix.
(Chris Wilson)

v4: Fix size calc broken in v3 and add alignment warning. (Chris Wilson)

v5: Fix return code.

v6: I added an exit from loop in v5 but forgot to put back
the object teardown.

Signed-off-by: Tvrtko Ursulin 
Reviewed-by: Chris Wilson  (v5)
Cc: Chris Wilson 
---
 drivers/gpu/drm/i915/intel_lrc.c | 318 ++-
 1 file changed, 117 insertions(+), 201 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ee431d39ce06..5154661a38cf 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -890,18 +890,6 @@ static int execlists_request_alloc(struct 
drm_i915_gem_request *request)
return ret;
 }
 
-#define wa_ctx_emit(batch, index, cmd) \
-   do {\
-   int __index = (index)++;\
-   if (WARN_ON(__index >= (PAGE_SIZE / sizeof(uint32_t { \
-   return -ENOSPC; \
-   }   \
-   batch[__index] = (cmd); \
-   } while (0)
-
-#define wa_ctx_emit_reg(batch, index, reg) \
-   wa_ctx_emit((batch), (index), i915_mmio_reg_offset(reg))
-
 /*
  * In this WA we need to set GEN8_L3SQCREG4[21:21] and reset it after
  * PIPE_CONTROL instruction. This is required for the flush to happen correctly
@@ -918,56 +906,31 @@ static int execlists_request_alloc(struct 
drm_i915_gem_request *request)
  * This WA is also required for Gen9 so extracting as a function avoids
  * code duplication.
  */
-static inline int gen8_emit_flush_coherentl3_wa(struct intel_engine_cs *engine,
-   uint32_t *batch,
-   uint32_t index)
-{
-   uint32_t l3sqc4_flush = (0x4040 | GEN8_LQSC_FLUSH_COHERENT_LINES);
-
-   wa_ctx_emit(batch, index, (MI_STORE_REGISTER_MEM_GEN8 |
-  MI_SRM_LRM_GLOBAL_GTT));
-   wa_ctx_emit_reg(batch, index, GEN8_L3SQCREG4);
-   wa_ctx_emit(batch, index, i915_ggtt_offset(engine->scratch) + 256);
-   wa_ctx_emit(batch, index, 0);
-
-   wa_ctx_emit(batch, index, MI_LOAD_REGISTER_IMM(1));
-   wa_ctx_emit_reg(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 |
-  MI_SRM_LRM_GLOBAL_GTT));
-   wa_ctx_emit_reg(batch, index, GEN8_L3SQCREG4);
-   wa_ctx_emit(batch, index, i915_ggtt_offset(engine->scratch) + 256);
-   wa_ctx_emit(batch, index, 0);
-
-   return index;
-}
-
-static inline uint32_t wa_ctx_start(struct i915_wa_ctx_bb *wa_ctx,
-   uint32_t offset,
-   uint32_t start_alignment)
+static u32 *
+gen8_emit_flush_coherentl3_wa(struct intel_engine_cs *engine, u32 *batch)
 {
-   return wa_ctx->offset = ALIGN(offset, start_alignment);
-}
-
-static inline int wa_ctx_end(struct i915_wa_ctx_bb *wa_ctx,
-uint32_t offset,
-uint32_t size_alignment)
-{
-   wa_ctx->size = offset - wa_ctx->offset;
-
-   WARN(wa_ctx->size % size_alignment,
-"wa_ctx_bb failed sanity checks: size %d is not aligned to %d\n",
-wa_ctx->size, size_alignment);
-   return 0;
+   *batch++ = MI_STORE_REGISTER_MEM_GEN8 | MI_SRM_LRM_GLOBAL_GTT;
+   *batch++ = i915_mmio_reg_offset(GEN8_L3SQCREG4);
+   *batch++ = i915_ggtt_offset(engine->scratch) + 256;
+   *batch++ = 0;
+
+   *batch++ = MI_LOAD_REGISTER_IMM(1);
+   *batch++ = i915_mmio_reg_offset(GEN8_L3SQCREG4);
+   *batch++ = 0x4040 | GEN8_LQSC_FLUSH_COHERENT_LINES;
+
+   *batch++ = GFX_OP_PIPE_CONTROL(6);
+   *batch++ =