Re: [Intel-gfx] [PATCH v3] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Chris Wilson]

Quoting Rafael Antognolli (2018-02-05 23:33:30)
> This workaround should prevent a bug that can be hit on a context
> restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
> (0x7a04 0x0010 0x 0x) followed by 12DW's of
> NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
> idle prior to programming 3DSTATE_SAMPLE_PATTERN.
> 
> It's also not clear whether we should add those extra dwords because of
> the workaround itself, or if that's just padding for the WA BB (and next
> commands could come right after the PIPE_CONTROL). We keep them for now.
> 
> References: HSD#1939868
> 
>  v2: More descriptive changelog and comments.
>  v3: Explain that PIPE_CONTROL is actually 6 dwords, and that we advance
>  10 more dwords because of that.
> 
> Signed-off-by: Rafael Antognolli 
> Cc: Chris Wilson 
> Acked-by: Chris Wilson 

Thanks for the clear comment elucidating our doubt about what is
actually required for the w/a. Pushed.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915/cnl: WaPipeControlBefore3DStateSamplePattern

[Rafael Antognolli]

This workaround should prevent a bug that can be hit on a context
restore. To avoid the issue, we must emit a PIPE_CONTROL with CS stall
(0x7a04 0x0010 0x 0x) followed by 12DW's of
NOOP(0x0) in the indirect context batch buffer, to ensure the engine is
idle prior to programming 3DSTATE_SAMPLE_PATTERN.

It's also not clear whether we should add those extra dwords because of
the workaround itself, or if that's just padding for the WA BB (and next
commands could come right after the PIPE_CONTROL). We keep them for now.

References: HSD#1939868

 v2: More descriptive changelog and comments.
 v3: Explain that PIPE_CONTROL is actually 6 dwords, and that we advance
 10 more dwords because of that.

Signed-off-by: Rafael Antognolli 
Cc: Chris Wilson 
Acked-by: Chris Wilson 
---
 drivers/gpu/drm/i915/intel_lrc.c | 38 +-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index adf257dfa5e0..4bca4bc47cca 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1328,6 +1328,40 @@ static u32 *gen9_init_indirectctx_bb(struct 
intel_engine_cs *engine, u32 *batch)
return batch;
 }
 
+static u32 *gen10_init_indirectctx_bb(struct intel_engine_cs *engine, u32 
*batch)
+{
+   int i;
+
+   /*
+* WaPipeControlBefore3DStateSamplePattern: cnl
+*
+* Ensure the engine is idle prior to programming a
+* 3DSTATE_SAMPLE_PATTERN during a context restore.
+*/
+   batch = gen8_emit_pipe_control(batch,
+  PIPE_CONTROL_CS_STALL,
+  0);
+   /*
+* WaPipeControlBefore3DStateSamplePattern says we need 4 dwords for
+* the PIPE_CONTROL followed by 12 dwords of 0x0, so 16 dwords in
+* total. However, a PIPE_CONTROL is 6 dwords long, not 4, which is
+* confusing. Since gen8_emit_pipe_control() already advances the
+* batch by 6 dwords, we advance the other 10 here, completing a
+* cacheline. It's not clear if the workaround requires this padding
+* before other commands, or if it's just the regular padding we would
+* already have for the workaround bb, so leave it here for now.
+*/
+   for (i = 0; i < 10; i++)
+   *batch++ = MI_NOOP;
+
+   /* Pad to end of cacheline */
+   while ((unsigned long)batch % CACHELINE_BYTES)
+   *batch++ = MI_NOOP;
+
+   return batch;
+}
+
+
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
@@ -1381,7 +1415,9 @@ static int intel_init_workaround_bb(struct 
intel_engine_cs *engine)
 
switch (INTEL_GEN(engine->i915)) {
case 10:
-   return 0;
+   wa_bb_fn[0] = gen10_init_indirectctx_bb;
+   wa_bb_fn[1] = NULL;
+   break;
case 9:
wa_bb_fn[0] = gen9_init_indirectctx_bb;
wa_bb_fn[1] = NULL;
-- 
2.14.3

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