[RFC PATCH 0/4] drm/i915: implement NOA mux reprogramming at ctx-switch

[RFC PATCH 0/4] drm/i915: implement NOA mux reprogramming at ctx-switch

[Lionel Landwerlin]

Hi all,

This little series implements NOA muxes reprogramming on context
switch through the per context batch buffer.

NOA muxes resides in slices & subslices, which makes their
configuration subject to loss when a slice or subslice is shutdown.
The goal of this series is to restore the configuration on context
switch. It currently reloads all configurations on context switch but
we may be able to be a bit more clever by copying the last RPCS
configuration in memory and compare the current configuration before
executing all the MI_LRIs using a MI_PREDICATE.

Looking forward to comments!

Cheers,

Lionel Landwerlin (4):
  drm/i915: use same define size for wa_bb pin/allocation
  drm/i915: extract per-ctx/indirect bb programming
  drm/i915: pass wa_ctx as argument
  drm/i915: reprogram NOA muxes on context switch when using perf

 drivers/gpu/drm/i915/i915_drv.h  |   2 +
 drivers/gpu/drm/i915/i915_perf.c |  77 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c | 110 ++++++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_lrc.h |   1 +
 4 files changed, 166 insertions(+), 24 deletions(-)

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

[RFC PATCH 1/4] drm/i915: use same define size for wa_bb pin/allocation

[Lionel Landwerlin]

If we have CTX_WA_BB_OBJ_SIZE we should use it everywhere we want to
refer to the workaround batchbuffer object rather than using
PAGE_SIZE.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3758ff81928d..5b96b1e2353d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1141,7 +1141,7 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
 		goto err;
 	}
 
-	err = i915_vma_pin(vma, 0, PAGE_SIZE, PIN_GLOBAL | PIN_HIGH);
+	err = i915_vma_pin(vma, 0, CTX_WA_BB_OBJ_SIZE, PIN_GLOBAL | PIN_HIGH);
 	if (err)
 		goto err;
 
-- 
2.14.1

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

[RFC PATCH 2/4] drm/i915: extract per-ctx/indirect bb programming

[Lionel Landwerlin]

Let's put this in its own function to reuse it later.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5b96b1e2353d..6da2b4f0c5a5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1906,6 +1906,23 @@ static u32 intel_lr_indirect_ctx_offset(struct intel_engine_cs *engine)
 	return indirect_ctx_offset;
 }
 
+static void execlists_init_reg_state_wa_bb(u32 *regs,
+					   struct intel_engine_cs *engine)
+{
+	struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
+	u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
+
+	regs[CTX_RCS_INDIRECT_CTX + 1] =
+		(ggtt_offset + wa_ctx->indirect_ctx.offset) |
+		(wa_ctx->indirect_ctx.size / CACHELINE_BYTES);
+
+	regs[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
+		intel_lr_indirect_ctx_offset(engine) << 6;
+
+	regs[CTX_BB_PER_CTX_PTR + 1] =
+		(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
+}
+
 static void execlists_init_reg_state(u32 *regs,
 				     struct i915_gem_context *ctx,
 				     struct intel_engine_cs *engine,
@@ -1948,20 +1965,8 @@ static void execlists_init_reg_state(u32 *regs,
 		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;
-			u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);
-
-			regs[CTX_RCS_INDIRECT_CTX + 1] =
-				(ggtt_offset + wa_ctx->indirect_ctx.offset) |
-				(wa_ctx->indirect_ctx.size / CACHELINE_BYTES);
-
-			regs[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
-				intel_lr_indirect_ctx_offset(engine) << 6;
-
-			regs[CTX_BB_PER_CTX_PTR + 1] =
-				(ggtt_offset + wa_ctx->per_ctx.offset) | 0x01;
-		}
+		if (engine->wa_ctx.vma)
+			execlists_init_reg_state_wa_bb(regs, engine);
 	}
 
 	regs[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9) | MI_LRI_FORCE_POSTED;
-- 
2.14.1

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

[RFC PATCH 3/4] drm/i915: pass wa_ctx as argument

[Lionel Landwerlin]

Rather than accessing it from the engine structure. This will be used
for reprogramming later.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6da2b4f0c5a5..c7e7c355b0a7 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1125,7 +1125,8 @@ static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
 
 #define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
 
-static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
+static int lrc_setup_wa_ctx(struct intel_engine_cs *engine,
+			    struct i915_ctx_workarounds *wa_ctx)
 {
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
@@ -1145,7 +1146,7 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
 	if (err)
 		goto err;
 
-	engine->wa_ctx.vma = vma;
+	wa_ctx->vma = vma;
 	return 0;
 
 err:
@@ -1160,9 +1161,9 @@ static void lrc_destroy_wa_ctx(struct intel_engine_cs *engine)
 
 typedef u32 *(*wa_bb_func_t)(struct intel_engine_cs *engine, u32 *batch);
 
-static int intel_init_workaround_bb(struct intel_engine_cs *engine)
+static int intel_init_workaround_bb(struct intel_engine_cs *engine,
+				    struct i915_ctx_workarounds *wa_ctx)
 {
-	struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
 	struct i915_wa_ctx_bb *wa_bb[2] = { &wa_ctx->indirect_ctx,
 					    &wa_ctx->per_ctx };
 	wa_bb_func_t wa_bb_fn[2];
@@ -1190,7 +1191,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine)
 		return 0;
 	}
 
-	ret = lrc_setup_wa_ctx(engine);
+	ret = lrc_setup_wa_ctx(engine, wa_ctx);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Failed to setup context WA page: %d\n", ret);
 		return ret;
@@ -1829,7 +1830,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 	if (ret)
 		return ret;
 
-	ret = intel_init_workaround_bb(engine);
+	ret = intel_init_workaround_bb(engine, &engine->wa_ctx);
 	if (ret) {
 		/*
 		 * We continue even if we fail to initialize WA batch
-- 
2.14.1

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

[RFC PATCH 4/4] drm/i915: reprogram NOA muxes on context switch when using perf

[Lionel Landwerlin]

If some of the contexts submitting workloads to the GPU have been
configured to shutdown slices/subslices, we might loose the NOA
configurations written in the NOA muxes. We need to reprogram then at
context switch.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/i915_perf.c | 77 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c | 64 ++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_lrc.h |  1 +
 4 files changed, 140 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0003b46b6840..d4b3e5da9009 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3685,6 +3685,8 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
 void i915_oa_init_reg_state(struct intel_engine_cs *engine,
 			    struct i915_gem_context *ctx,
 			    uint32_t *reg_state);
+u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv);
+u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch);
 
 /* i915_gem_evict.c */
 int __must_check i915_gem_evict_something(struct i915_address_space *vm,
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 94185d610673..b74ffbb47879 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1687,6 +1687,74 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req,
 	return 0;
 }
 
+#define MAX_LRI_SIZE (125U)
+
+u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv)
+{
+	struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	/* Perf not supported. */
+	if (!dev_priv->perf.initialized)
+		return 0;
+
+	/* OA not currently configured. */
+	if (!stream)
+		return 0;
+
+	/* Very unlikely but possible that we have no muxes to configure. */
+	if (!stream->oa_config->mux_regs_len)
+		return 0;
+
+	/* Return the size of MI_LOAD_REGISTER_IMMs. */
+	return (stream->oa_config->mux_regs_len / MAX_LRI_SIZE) * 4 + 4 +
+		stream->oa_config->mux_regs_len * 8;
+}
+
+u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
+	u32 n_lri, n_mux_regs;
+	u32 i;
+
+	lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+	/* We only care about RCS. */
+	if (engine->id != RCS)
+		return batch;
+
+	/* Perf not supported. */
+	if (!dev_priv->perf.initialized)
+		return batch;
+
+	/* OA not currently configured. */
+	if (!stream)
+		return batch;
+
+	/* It's very unlikely, but possible that we're dealing with a config
+	 * with no mux to configure.
+	 */
+	if (!stream->oa_config->mux_regs_len)
+		return batch;
+
+	n_mux_regs = stream->oa_config->mux_regs_len;
+	n_lri = (n_mux_regs / MAX_LRI_SIZE) + (n_mux_regs % MAX_LRI_SIZE) != 0;
+
+	for (i = 0; i < n_mux_regs; i++) {
+		if ((i % MAX_LRI_SIZE) == 0) {
+			n_lri = min(n_mux_regs - i, MAX_LRI_SIZE);
+			*batch++ = MI_LOAD_REGISTER_IMM(n_lri);
+		}
+
+		*batch++ = i915_mmio_reg_offset(stream->oa_config->mux_regs[i].addr);
+		*batch++ = stream->oa_config->mux_regs[i].value;
+	}
+
+	return batch;
+}
+
 static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_priv,
 						 const struct i915_oa_config *oa_config)
 {
@@ -1793,6 +1861,15 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
 	if (ret)
 		goto out;
 
+	/*
+	 * Reload the workaround batchbuffer to include NOA muxes
+	 * reprogramming on context-switch, so we don't loose configurations
+	 * after switch-from a context with disabled slices/subslices.
+	 */
+	ret = logical_render_ring_reload_wa_bb(dev_priv->engine[RCS]);
+	if (ret)
+		return ret;
+
 	/* Update all contexts now that we've stalled the submission. */
 	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
 		struct intel_context *ce = &ctx->engine[RCS];
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c7e7c355b0a7..60639624045b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -217,6 +217,8 @@ static void execlists_init_reg_state(u32 *reg_state,
 				     struct i915_gem_context *ctx,
 				     struct intel_engine_cs *engine,
 				     struct intel_ring *ring);
+static void execlists_init_reg_state_wa_bb(u32 *reg_state,
+					   struct intel_engine_cs *engine);
 
 /**
  * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
@@ -1055,6 +1057,8 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
  */
 static u32 *gen8_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
 {
+	batch = i915_oa_emit_perctx_bb(engine, batch);
+
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
 	*batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
 	*batch++ = MI_BATCH_BUFFER_END;
@@ -1118,21 +1122,27 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
 
 static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
 {
+	batch = i915_oa_emit_perctx_bb(engine, batch);
+
 	*batch++ = MI_BATCH_BUFFER_END;
 
 	return batch;
 }
 
-#define CTX_WA_BB_OBJ_SIZE (PAGE_SIZE)
+/* Reserve 200 dwords for indirect & per-ctx bb */
+#define CTX_WA_BB_MIN_DWORDS (200)
 
 static int lrc_setup_wa_ctx(struct intel_engine_cs *engine,
 			    struct i915_ctx_workarounds *wa_ctx)
 {
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
+	u32 size = DIV_ROUND_UP(i915_oa_get_perctx_bb_size(engine->i915) +
+				4 * CTX_WA_BB_MIN_DWORDS,
+				PAGE_SIZE) * PAGE_SIZE;
 	int err;
 
-	obj = i915_gem_object_create(engine->i915, CTX_WA_BB_OBJ_SIZE);
+	obj = i915_gem_object_create(engine->i915, size);
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
@@ -1142,7 +1152,7 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine,
 		goto err;
 	}
 
-	err = i915_vma_pin(vma, 0, CTX_WA_BB_OBJ_SIZE, PIN_GLOBAL | PIN_HIGH);
+	err = i915_vma_pin(vma, 0, size, PIN_GLOBAL | PIN_HIGH);
 	if (err)
 		goto err;
 
@@ -1215,7 +1225,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine,
 		wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset);
 	}
 
-	BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE);
+	BUG_ON(batch_ptr - batch > wa_ctx->vma->obj->base.size);
 
 	kunmap_atomic(batch);
 	if (ret)
@@ -1844,6 +1854,52 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 	return logical_ring_init(engine);
 }
 
+int logical_render_ring_reload_wa_bb(struct intel_engine_cs *engine)
+{
+	struct drm_i915_private *dev_priv = engine->i915;
+	struct i915_ctx_workarounds new_wa_ctx;
+	struct i915_gem_context *ctx;
+	int ret;
+
+	if (WARN_ON(engine->id != RCS))
+		return -EINVAL;
+
+	memset(&new_wa_ctx, 0, sizeof(new_wa_ctx));
+	ret = intel_init_workaround_bb(engine, &new_wa_ctx);
+	if (ret)
+		return ret;
+
+	if (engine->wa_ctx.vma)
+		lrc_destroy_wa_ctx(engine);
+
+	memcpy(&engine->wa_ctx, &new_wa_ctx, sizeof(engine->wa_ctx));
+
+	list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
+		struct intel_context *ce = &ctx->engine[RCS];
+		u32 *regs;
+
+		/* Settings will be set upon first use. */
+		if (!ce->state)
+			continue;
+
+		regs = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
+		if (IS_ERR(regs)) {
+			ret = PTR_ERR(regs);
+			break;
+		}
+
+		ce->state->obj->mm.dirty = true;
+		regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
+
+		if (engine->wa_ctx.vma)
+			execlists_init_reg_state_wa_bb(regs, engine);
+
+		i915_gem_object_unpin_map(ce->state->obj);
+	}
+
+	return ret;
+}
+
 int logical_xcs_ring_init(struct intel_engine_cs *engine)
 {
 	logical_ring_setup(engine);
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 4ef6a6143f5d..bbaf9f563ad5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -65,6 +65,7 @@ enum {
 /* Logical Rings */
 void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
 int logical_render_ring_init(struct intel_engine_cs *engine);
+int logical_render_ring_reload_wa_bb(struct intel_engine_cs *engine);
 int logical_xcs_ring_init(struct intel_engine_cs *engine);
 
 /* Logical Ring Contexts */
-- 
2.14.1

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

Re: [RFC PATCH 0/4] drm/i915: implement NOA mux reprogramming at ctx-switch

[Lionel Landwerlin]

I forgot to mentioned this was tested on SKL GT4 using 2 OA/NOA 
configurations that exposes :

      1. signals of the state of power on each slice/subslice
      2. signals of the number of sampler cache misses on each slice

We verified that the counters incrementing on those signals return to 
normal values when you switch between 2 different context with 2 
different RPCS configurations (as opposed to running the same 
configurations without this series).
We used Chris's series to expose RPCS configuration to userspace : 
https://github.com/djdeath/linux/commit/b2ea9b16ef1377f808be4d4c60f5f23596517f49
You can pull the branch with those changes here : 
https://github.com/djdeath/linux/commits/wip/djdeath/oa-next-slice-control

Cheers,

-
Lionel

On 30/08/17 19:20, Lionel Landwerlin wrote:
> Hi all,
>
> This little series implements NOA muxes reprogramming on context
> switch through the per context batch buffer.
>
> NOA muxes resides in slices & subslices, which makes their
> configuration subject to loss when a slice or subslice is shutdown.
> The goal of this series is to restore the configuration on context
> switch. It currently reloads all configurations on context switch but
> we may be able to be a bit more clever by copying the last RPCS
> configuration in memory and compare the current configuration before
> executing all the MI_LRIs using a MI_PREDICATE.
>
> Looking forward to comments!
>
> Cheers,
>
> Lionel Landwerlin (4):
>    drm/i915: use same define size for wa_bb pin/allocation
>    drm/i915: extract per-ctx/indirect bb programming
>    drm/i915: pass wa_ctx as argument
>    drm/i915: reprogram NOA muxes on context switch when using perf
>
>   drivers/gpu/drm/i915/i915_drv.h  |   2 +
>   drivers/gpu/drm/i915/i915_perf.c |  77 +++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_lrc.c | 110 ++++++++++++++++++++++++++++++---------
>   drivers/gpu/drm/i915/intel_lrc.h |   1 +
>   4 files changed, 166 insertions(+), 24 deletions(-)
>
> --
> 2.14.1
>

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

Re: [RFC PATCH 1/4] drm/i915: use same define size for wa_bb pin/allocation

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-30 19:20:03)
> If we have CTX_WA_BB_OBJ_SIZE we should use it everywhere we want to
> refer to the workaround batchbuffer object rather than using
> PAGE_SIZE.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 3758ff81928d..5b96b1e2353d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1141,7 +1141,7 @@ static int lrc_setup_wa_ctx(struct intel_engine_cs *engine)
>                 goto err;
>         }
>  
> -       err = i915_vma_pin(vma, 0, PAGE_SIZE, PIN_GLOBAL | PIN_HIGH);
> +       err = i915_vma_pin(vma, 0, CTX_WA_BB_OBJ_SIZE, PIN_GLOBAL | PIN_HIGH);

It shouldn't be passed there at all; that parameter is for overriding
the vma->size.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/4] drm/i915: extract per-ctx/indirect bb programming

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-30 19:20:04)
> Let's put this in its own function to reuse it later.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 33 +++++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5b96b1e2353d..6da2b4f0c5a5 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1906,6 +1906,23 @@ static u32 intel_lr_indirect_ctx_offset(struct intel_engine_cs *engine)
>         return indirect_ctx_offset;
>  }
>  
> +static void execlists_init_reg_state_wa_bb(u32 *regs,
> +                                          struct intel_engine_cs *engine)
> +{
> +       struct i915_ctx_workarounds *wa_ctx = &engine->wa_ctx;
> +       u32 ggtt_offset = i915_ggtt_offset(wa_ctx->vma);

You are going to repeat the !wa_ctx->vma so you might as well pull it
in.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 4/4] drm/i915: reprogram NOA muxes on context switch when using perf

[Chris Wilson]

Quoting Lionel Landwerlin (2017-08-30 19:20:06)
> If some of the contexts submitting workloads to the GPU have been
> configured to shutdown slices/subslices, we might loose the NOA
> configurations written in the NOA muxes. We need to reprogram then at
> context switch.
> 
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>  drivers/gpu/drm/i915/i915_perf.c | 77 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c | 64 ++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_lrc.h |  1 +
>  4 files changed, 140 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0003b46b6840..d4b3e5da9009 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3685,6 +3685,8 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
>  void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>                             struct i915_gem_context *ctx,
>                             uint32_t *reg_state);
> +u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv);
> +u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch);
>  
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct i915_address_space *vm,
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 94185d610673..b74ffbb47879 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1687,6 +1687,74 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req,
>         return 0;
>  }
>  
> +#define MAX_LRI_SIZE (125U)
> +
> +u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv)
> +{
> +       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
> +
> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);

Still not happy by this coupling to struct_mutex. :-p

Missed RCS check.

> +
> +       /* Perf not supported. */
> +       if (!dev_priv->perf.initialized)
> +               return 0;
> +
> +       /* OA not currently configured. */
> +       if (!stream)
> +               return 0;
> +
> +       /* Very unlikely but possible that we have no muxes to configure. */
> +       if (!stream->oa_config->mux_regs_len)
> +               return 0;
> +
> +       /* Return the size of MI_LOAD_REGISTER_IMMs. */
> +       return (stream->oa_config->mux_regs_len / MAX_LRI_SIZE) * 4 + 4 +
> +               stream->oa_config->mux_regs_len * 8;
> +}
> +
> +u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
> +       u32 n_lri, n_mux_regs;
> +       u32 i;
> +
> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> +       /* We only care about RCS. */
> +       if (engine->id != RCS)
> +               return batch;
> +
> +       /* Perf not supported. */
> +       if (!dev_priv->perf.initialized)
> +               return batch;
> +
> +       /* OA not currently configured. */
> +       if (!stream)
> +               return batch;
> +
> +       /* It's very unlikely, but possible that we're dealing with a config
> +        * with no mux to configure.
> +        */
> +       if (!stream->oa_config->mux_regs_len)
> +               return batch;

The above could be condensed into
if (i915_oa_get_perctx_bb_size() == 0)
	return;

> +
> +       n_mux_regs = stream->oa_config->mux_regs_len;
> +       n_lri = (n_mux_regs / MAX_LRI_SIZE) + (n_mux_regs % MAX_LRI_SIZE) != 0;
> +
> +       for (i = 0; i < n_mux_regs; i++) {
> +               if ((i % MAX_LRI_SIZE) == 0) {
> +                       n_lri = min(n_mux_regs - i, MAX_LRI_SIZE);
> +                       *batch++ = MI_LOAD_REGISTER_IMM(n_lri);
> +               }
> +
> +               *batch++ = i915_mmio_reg_offset(stream->oa_config->mux_regs[i].addr);
> +               *batch++ = stream->oa_config->mux_regs[i].value;
> +       }

I would have personally used a double loop. But at least kill that first
n_lri, that was a moment of confusion spent trying to work out what you
were using it for.

> +
> +       return batch;
> +}

>  /**
>   * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
> @@ -1055,6 +1057,8 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>   */
>  static u32 *gen8_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
>  {
> +       batch = i915_oa_emit_perctx_bb(engine, batch);
> +
>         /* WaDisableCtxRestoreArbitration:bdw,chv */
>         *batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>         *batch++ = MI_BATCH_BUFFER_END;
> @@ -1118,21 +1122,27 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>  
>  static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
>  {
> +       batch = i915_oa_emit_perctx_bb(engine, batch);

Wrong wa_bb. This is emitted at the start of every bb, you want
indirectctx_bb which is emitted after a context switch. Or at least I
hope you don't need such a heavy handed approach. If you do, can you
convince me that you are really not in conflict with the application?
(Earlier you said it only needs ctx switch.)

> +int logical_render_ring_reload_wa_bb(struct intel_engine_cs *engine)

intel_lrc_update_wa_bb().

> +{
> +       struct drm_i915_private *dev_priv = engine->i915;
> +       struct i915_ctx_workarounds new_wa_ctx;
> +       struct i915_gem_context *ctx;
> +       int ret;
> +
> +       if (WARN_ON(engine->id != RCS))
> +               return -EINVAL;
> +
> +       memset(&new_wa_ctx, 0, sizeof(new_wa_ctx));
> +       ret = intel_init_workaround_bb(engine, &new_wa_ctx);
> +       if (ret)
> +               return ret;
> +
> +       if (engine->wa_ctx.vma)
> +               lrc_destroy_wa_ctx(engine);

Couldn't we at least try to reuse the existing vma first? 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 4/4] drm/i915: reprogram NOA muxes on context switch when using perf

[Lionel Landwerlin]

On 30/08/17 20:15, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-08-30 19:20:06)
>> If some of the contexts submitting workloads to the GPU have been
>> configured to shutdown slices/subslices, we might loose the NOA
>> configurations written in the NOA muxes. We need to reprogram then at
>> context switch.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  2 ++
>>   drivers/gpu/drm/i915/i915_perf.c | 77 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_lrc.c | 64 ++++++++++++++++++++++++++++++---
>>   drivers/gpu/drm/i915/intel_lrc.h |  1 +
>>   4 files changed, 140 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0003b46b6840..d4b3e5da9009 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3685,6 +3685,8 @@ int i915_perf_remove_config_ioctl(struct drm_device *dev, void *data,
>>   void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>>                              struct i915_gem_context *ctx,
>>                              uint32_t *reg_state);
>> +u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv);
>> +u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch);
>>   
>>   /* i915_gem_evict.c */
>>   int __must_check i915_gem_evict_something(struct i915_address_space *vm,
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 94185d610673..b74ffbb47879 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1687,6 +1687,74 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req,
>>          return 0;
>>   }
>>   
>> +#define MAX_LRI_SIZE (125U)
>> +
>> +u32 i915_oa_get_perctx_bb_size(struct drm_i915_private *dev_priv)
>> +{
>> +       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
>> +
>> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);
> Still not happy by this coupling to struct_mutex. :-p
>
> Missed RCS check.
>
>> +
>> +       /* Perf not supported. */
>> +       if (!dev_priv->perf.initialized)
>> +               return 0;
>> +
>> +       /* OA not currently configured. */
>> +       if (!stream)
>> +               return 0;
>> +
>> +       /* Very unlikely but possible that we have no muxes to configure. */
>> +       if (!stream->oa_config->mux_regs_len)
>> +               return 0;
>> +
>> +       /* Return the size of MI_LOAD_REGISTER_IMMs. */
>> +       return (stream->oa_config->mux_regs_len / MAX_LRI_SIZE) * 4 + 4 +
>> +               stream->oa_config->mux_regs_len * 8;
>> +}
>> +
>> +u32 *i915_oa_emit_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
>> +{
>> +       struct drm_i915_private *dev_priv = engine->i915;
>> +       struct i915_perf_stream *stream = dev_priv->perf.oa.exclusive_stream;
>> +       u32 n_lri, n_mux_regs;
>> +       u32 i;
>> +
>> +       lockdep_assert_held(&dev_priv->drm.struct_mutex);
>> +
>> +       /* We only care about RCS. */
>> +       if (engine->id != RCS)
>> +               return batch;
>> +
>> +       /* Perf not supported. */
>> +       if (!dev_priv->perf.initialized)
>> +               return batch;
>> +
>> +       /* OA not currently configured. */
>> +       if (!stream)
>> +               return batch;
>> +
>> +       /* It's very unlikely, but possible that we're dealing with a config
>> +        * with no mux to configure.
>> +        */
>> +       if (!stream->oa_config->mux_regs_len)
>> +               return batch;
> The above could be condensed into
> if (i915_oa_get_perctx_bb_size() == 0)
> 	return;
>
>> +
>> +       n_mux_regs = stream->oa_config->mux_regs_len;
>> +       n_lri = (n_mux_regs / MAX_LRI_SIZE) + (n_mux_regs % MAX_LRI_SIZE) != 0;
>> +
>> +       for (i = 0; i < n_mux_regs; i++) {
>> +               if ((i % MAX_LRI_SIZE) == 0) {
>> +                       n_lri = min(n_mux_regs - i, MAX_LRI_SIZE);
>> +                       *batch++ = MI_LOAD_REGISTER_IMM(n_lri);
>> +               }
>> +
>> +               *batch++ = i915_mmio_reg_offset(stream->oa_config->mux_regs[i].addr);
>> +               *batch++ = stream->oa_config->mux_regs[i].value;
>> +       }
> I would have personally used a double loop. But at least kill that first
> n_lri, that was a moment of confusion spent trying to work out what you
> were using it for.
>
>> +
>> +       return batch;
>> +}
>>   /**
>>    * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists
>> @@ -1055,6 +1057,8 @@ static u32 *gen8_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>>    */
>>   static u32 *gen8_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
>>   {
>> +       batch = i915_oa_emit_perctx_bb(engine, batch);
>> +
>>          /* WaDisableCtxRestoreArbitration:bdw,chv */
>>          *batch++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>>          *batch++ = MI_BATCH_BUFFER_END;
>> @@ -1118,21 +1122,27 @@ static u32 *gen9_init_indirectctx_bb(struct intel_engine_cs *engine, u32 *batch)
>>   
>>   static u32 *gen9_init_perctx_bb(struct intel_engine_cs *engine, u32 *batch)
>>   {
>> +       batch = i915_oa_emit_perctx_bb(engine, batch);
> Wrong wa_bb. This is emitted at the start of every bb, you want
> indirectctx_bb which is emitted after a context switch. Or at least I
> hope you don't need such a heavy handed approach. If you do, can you
> convince me that you are really not in conflict with the application?
> (Earlier you said it only needs ctx switch.)

Thanks, I'll rerun my tests with indirectctx_bb.

>
>> +int logical_render_ring_reload_wa_bb(struct intel_engine_cs *engine)
> intel_lrc_update_wa_bb().
>
>> +{
>> +       struct drm_i915_private *dev_priv = engine->i915;
>> +       struct i915_ctx_workarounds new_wa_ctx;
>> +       struct i915_gem_context *ctx;
>> +       int ret;
>> +
>> +       if (WARN_ON(engine->id != RCS))
>> +               return -EINVAL;
>> +
>> +       memset(&new_wa_ctx, 0, sizeof(new_wa_ctx));
>> +       ret = intel_init_workaround_bb(engine, &new_wa_ctx);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (engine->wa_ctx.vma)
>> +               lrc_destroy_wa_ctx(engine);
> Couldn't we at least try to reuse the existing vma first?
>

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

✓ Fi.CI.BAT: success for drm/i915: implement NOA mux reprogramming at ctx-switch

[Patchwork]

== Series Details ==

Series: drm/i915: implement NOA mux reprogramming at ctx-switch
URL   : https://patchwork.freedesktop.org/series/29564/
State : success

== Summary ==

Series 29564v1 drm/i915: implement NOA mux reprogramming at ctx-switch
https://patchwork.freedesktop.org/api/1.0/series/29564/revisions/1/mbox/

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
        Subgroup basic-flip-after-cursor-varying-size:
                pass       -> FAIL       (fi-hsw-4770) fdo#102402 +1
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                skip       -> PASS       (fi-skl-x1585l) fdo#101781

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

fi-bdw-5557u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-bdw-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:440s
fi-blb-e6850     total:288  pass:224  dwarn:1   dfail:0   fail:0   skip:63  time:360s
fi-bsw-n3050     total:288  pass:243  dwarn:0   dfail:0   fail:0   skip:45  time:559s
fi-bwr-2160      total:288  pass:184  dwarn:0   dfail:0   fail:0   skip:104 time:255s
fi-bxt-j4205     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:522s
fi-byt-j1900     total:288  pass:254  dwarn:1   dfail:0   fail:0   skip:33  time:521s
fi-byt-n2820     total:288  pass:250  dwarn:1   dfail:0   fail:0   skip:37  time:512s
fi-elk-e7500     total:288  pass:230  dwarn:0   dfail:0   fail:0   skip:58  time:447s
fi-glk-2a        total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:610s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:2   skip:25  time:465s
fi-hsw-4770r     total:288  pass:263  dwarn:0   dfail:0   fail:0   skip:25  time:425s
fi-ilk-650       total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:419s
fi-ivb-3520m     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:507s
fi-ivb-3770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:478s
fi-kbl-7500u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:477s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:598s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-skl-6260u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:469s
fi-skl-6700k     total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:538s
fi-skl-6770hq    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:491s
fi-skl-gvtdvm    total:288  pass:266  dwarn:0   dfail:0   fail:0   skip:22  time:446s
fi-skl-x1585l    total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:507s
fi-snb-2520m     total:288  pass:251  dwarn:0   dfail:0   fail:0   skip:37  time:555s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:2   skip:38  time:411s
fi-pnv-d510 failed to connect after reboot

c399d43adc55a49d028d24ce7cdacc1823a4f159 drm-tip: 2017y-08m-31d-07h-25m-28s UTC integration manifest
64fc27dd0b93 drm/i915: reprogram NOA muxes on context switch when using perf
2714213be61c drm/i915: pass wa_ctx as argument
a66d0a2b9c18 drm/i915: extract per-ctx/indirect bb programming
91d1ad1d720f drm/i915: use same define size for wa_bb pin/allocation

== Logs ==

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

✗ Fi.CI.IGT: warning for drm/i915: implement NOA mux reprogramming at ctx-switch

[Patchwork]

== Series Details ==

Series: drm/i915: implement NOA mux reprogramming at ctx-switch
URL   : https://patchwork.freedesktop.org/series/29564/
State : warning

== Summary ==

Test kms_setmode:
        Subgroup basic:
                pass       -> FAIL       (shard-hsw) fdo#99912
Test kms_atomic_transition:
        Subgroup plane-all-modeset-transition:
                pass       -> DMESG-WARN (shard-hsw)
Test perf:
        Subgroup blocking:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_flip:
        Subgroup plain-flip-fb-recreate:
                fail       -> PASS       (shard-hsw)

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

shard-hsw        total:2265 pass:1229 dwarn:1   dfail:0   fail:19  skip:1016 time:9658s

== Logs ==

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