[PATCH 1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[PATCH 1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[Chris Wilson]

The per-context and per-batch workaround buffers are optional, yet we
tell the GPU to execute them even if they contain no instructions. Doing
so incurs the dispatch latency, which we can avoid if we don't ask the
GPU to execute the no-op buffers. Allow ourselves to skip setup of empty
buffer, and then to only enable non-empty buffers in the context image.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 86fed9f1f1ae..297c9c1564e5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1265,7 +1265,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 			ret = -EINVAL;
 			break;
 		}
-		batch_ptr = wa_bb_fn[i](engine, batch_ptr);
+		if (wa_bb_fn[i])
+			batch_ptr = wa_bb_fn[i](engine, batch_ptr);
 		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
 	}
 
@@ -1994,13 +1995,12 @@ static void execlists_init_reg_state(u32 *regs,
 	CTX_REG(regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
 	CTX_REG(regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
 	if (rcs) {
-		CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
+		struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
+
 		CTX_REG(regs, CTX_RCS_INDIRECT_CTX, RING_INDIRECT_CTX(base), 0);
 		CTX_REG(regs, CTX_RCS_INDIRECT_CTX_OFFSET,
 			RING_INDIRECT_CTX_OFFSET(base), 0);
-
-		if (engine->wa_ctx.vma) {
-			struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
+		if (wa_ctx->indirect_ctx.size) {
 			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
 
 			regs[CTX_RCS_INDIRECT_CTX + 1] =
@@ -2009,6 +2009,11 @@ static void execlists_init_reg_state(u32 *regs,
 
 			regs[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
 				intel_lr_indirect_ctx_offset(engine) << 6;
+		}
+
+		CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
+		if (wa_ctx->per_ctx.size) {
+			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
 
 			regs[CTX_BB_PER_CTX_PTR + 1] =
 				(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
-- 
2.14.1

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

[PATCH 2/2] drm/i915/lrc: Skip no-op per-bb buffer on gen9

[Chris Wilson]

Since we inherited the context image setup from gen8 which needed a
per-bb workaround (for GPGPU), we are submitting an empty per-bb buffer
on gen9. Now that we can skip adding the buffer to the context image,
remove the dangling per-bb. This slightly improves execution latency,
most notably on an idle engine.

References: https://bugs.freedesktop.org/show_bug.cgi?id=87725
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 297c9c1564e5..91a5411bb9da 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1171,13 +1171,6 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 	return batch;
 }
 
-static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
-{
-	*batch++ = MI_BATCH_BUFFER_END;
-
-	return batch;
-}
-
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
@@ -1234,7 +1227,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 		return 0;
 	case 9:
 		wa_bb_fn[0] = gen9_init_indirectctx_bb;
-		wa_bb_fn[1] = gen9_init_perctx_bb;
+		wa_bb_fn[1] = NULL;
 		break;
 	case 8:
 		wa_bb_fn[0] = gen8_init_indirectctx_bb;
-- 
2.14.1

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

Re: [PATCH 2/2] drm/i915/lrc: Skip no-op per-bb buffer on gen9

[Tvrtko Ursulin]

On 21/09/2017 14:54, Chris Wilson wrote:
> Since we inherited the context image setup from gen8 which needed a
> per-bb workaround (for GPGPU), we are submitting an empty per-bb buffer
> on gen9. Now that we can skip adding the buffer to the context image,
> remove the dangling per-bb. This slightly improves execution latency,
> most notably on an idle engine.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=87725

How much of the 7% we get back? :)

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 9 +--------
>   1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 297c9c1564e5..91a5411bb9da 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1171,13 +1171,6 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>   	return batch;
>   }
>   
> -static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
> -{
> -	*batch++ = MI_BATCH_BUFFER_END;
> -
> -	return batch;
> -}
> -
>   #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
>   
>   static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
> @@ -1234,7 +1227,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>   		return 0;
>   	case 9:
>   		wa_bb_fn[0] = gen9_init_indirectctx_bb;
> -		wa_bb_fn[1] = gen9_init_perctx_bb;
> +		wa_bb_fn[1] = NULL;
>   		break;
>   	case 8:
>   		wa_bb_fn[0] = gen8_init_indirectctx_bb;
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

Re: [PATCH 2/2] drm/i915/lrc: Skip no-op per-bb buffer on gen9

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-09-21 16:12:21)
> 
> On 21/09/2017 14:54, Chris Wilson wrote:
> > Since we inherited the context image setup from gen8 which needed a
> > per-bb workaround (for GPGPU), we are submitting an empty per-bb buffer
> > on gen9. Now that we can skip adding the buffer to the context image,
> > remove the dangling per-bb. This slightly improves execution latency,
> > most notably on an idle engine.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=87725
> 
> How much of the 7% we get back? :)

Not enough. The difference in execution latency between ringbuffer
submission and execlists for this type of workload is roughly an order of
magnitude (~5us to ~30us, using gem_sync as a reasonable proxy). The
per-bb accounts for around 6us of that on bdw, so a big chunk but still
a few times slower. Not that we do move the GPGPU workaround on bdw
just yet, I left that for when we do play with preemption and
MI_ARB_ON_OFF. (Side note, the remaining difference between ringbuffer
and execlists seems to be related to MI arbitration...)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set
URL   : https://patchwork.freedesktop.org/series/30701/
State : success

== Summary ==

Series 30701v1 series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set
https://patchwork.freedesktop.org/api/1.0/series/30701/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-glk-1) fdo#102777 +1

fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#102777 https://bugs.freedesktop.org/show_bug.cgi?id=102777

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:437s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:415s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:520s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:507s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:491s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:495s
fi-cfl-s         total:289  pass:223  dwarn:34  dfail:0   fail:0   skip:32  time:536s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:416s
fi-glk-1         total:289  pass:258  dwarn:2   dfail:0   fail:0   skip:29  time:622s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:426s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:404s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:433s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:460s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:470s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:572s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:545s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:449s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:741s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:475s
fi-snb-2600      total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:419s
fi-snb-2520m failed to connect after reboot

01a2040bb790263c0d32ec30d83bd2ddf3b922c2 drm-tip: 2017y-09m-21d-13h-23m-06s UTC integration manifest
d02479d1940a drm/i915/lrc: Skip no-op per-bb buffer on gen9
894bcf3f84c6 drm/i915/lrc: Only enable per-context and per-bb buffers if set

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5780/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[Tvrtko Ursulin]

On 21/09/2017 14:54, Chris Wilson wrote:
> The per-context and per-batch workaround buffers are optional, yet we
> tell the GPU to execute them even if they contain no instructions. Doing
> so incurs the dispatch latency, which we can avoid if we don't ask the
> GPU to execute the no-op buffers. Allow ourselves to skip setup of empty
> buffer, and then to only enable non-empty buffers in the context image.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 86fed9f1f1ae..297c9c1564e5 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1265,7 +1265,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
>   			ret = -EINVAL;
>   			break;
>   		}
> -		batch_ptr = wa_bb_fn[i](engine, batch_ptr);
> +		if (wa_bb_fn[i])
> +			batch_ptr = wa_bb_fn[i](engine, batch_ptr);
>   		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
>   	}
>   
> @@ -1994,13 +1995,12 @@ static void execlists_init_reg_state(u32 *regs,
>   	CTX_REG(regs, CTX_SECOND_BB_HEAD_L, RING_SBBADDR(base), 0);
>   	CTX_REG(regs, CTX_SECOND_BB_STATE, RING_SBBSTATE(base), 0);
>   	if (rcs) {
> -		CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
> +		struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
> +
>   		CTX_REG(regs, CTX_RCS_INDIRECT_CTX, RING_INDIRECT_CTX(base), 0);
>   		CTX_REG(regs, CTX_RCS_INDIRECT_CTX_OFFSET,
>   			RING_INDIRECT_CTX_OFFSET(base), 0);
> -
> -		if (engine->wa_ctx.vma) {
> -			struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
> +		if (wa_ctx->indirect_ctx.size) {
>   			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
>   
>   			regs[CTX_RCS_INDIRECT_CTX + 1] =
> @@ -2009,6 +2009,11 @@ static void execlists_init_reg_state(u32 *regs,
>   
>   			regs[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
>   				intel_lr_indirect_ctx_offset(engine) << 6;
> +		}
> +
> +		CTX_REG(regs, CTX_BB_PER_CTX_PTR, RING_BB_PER_CTX_PTR(base), 0);
> +		if (wa_ctx->per_ctx.size) {
> +			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
>   
>   			regs[CTX_BB_PER_CTX_PTR + 1] =
>   				(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
> 

Looks OK. Does it make sense to fetch the ggtt offset only once ("if 
(size || size) ggtt_offset = ...")? Meh.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set
URL   : https://patchwork.freedesktop.org/series/30701/
State : failure

== Summary ==

Test gem_exec_schedule:
        Subgroup wide-blt:
                skip       -> INCOMPLETE (shard-hsw)
Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912

fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-hsw        total:2429 pass:1306 dwarn:4   dfail:0   fail:11  skip:1060 time:9689s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5780/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: failure for series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set

[Chris Wilson]

Quoting Patchwork (2017-09-21 17:38:05)
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915/lrc: Only enable per-context and per-bb buffers if set
> URL   : https://patchwork.freedesktop.org/series/30701/
> State : failure
> 
> == Summary ==
> 
> Test gem_exec_schedule:
>         Subgroup wide-blt:
>                 skip       -> INCOMPLETE (shard-hsw)
> Test kms_setmode:
>         Subgroup basic:
>                 pass       -> FAIL       (shard-hsw) fdo#99912
> 
> fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
> 
> shard-hsw        total:2429 pass:1306 dwarn:4   dfail:0   fail:11  skip:1060 time:9689s
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5780/shards.html

Pushed this pair to avoid having to send them again in the larger
series!

Thanks for the review,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx