[PATCH v2 0/7] Add Per-context WA using WA batch buffers

[PATCH v2 0/7] Add Per-context WA using WA batch buffers

[Arun Siluvery]

From Gen8+ we have some workarounds that are applied Per context and
they are applied using special batch buffers called as WA batch buffers.
HW executes them at specific stages during context save/restore.
The patches in this series adds this framework to i915.

I did some basic testing on BDW by running glmark2 and didn't see any issues.
These WA are mainly required when preemption is enabled.

These patches were sent to mailing list before and this series incorporates
the initial review feedback.

[v1] http://lists.freedesktop.org/archives/intel-gfx/2015-February/060707.html
[v2] Review feedback from v1, new split-up with one workaround per patch.

I could probably squash patches 1 and 3 if that is preferred.
Please review and give your comments.

Arun Siluvery (7):
  drm/i915/gen8: Add infrastructure to initialize WA batch buffers
  drm/i915/gen8: Re-order init pipe_control in lrc mode
  drm/i915/gen8: Enable WA batch buffers during ctx save/restore
  drm/i915/gen8: Add WaDisableCtxRestoreArbitration workaround
  drm/i915/gen8: Add WaFlushCoherentL3CacheLinesAtContextSwitch
    workaround
  drm/i915/gen8: Add WaClearSlmSpaceAtContextSwitch workaround
  drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

 drivers/gpu/drm/i915/i915_drv.h         |   3 +
 drivers/gpu/drm/i915/i915_reg.h         |  28 ++++
 drivers/gpu/drm/i915/intel_lrc.c        | 278 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |   3 +
 4 files changed, 305 insertions(+), 7 deletions(-)

-- 
2.3.0

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

[PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Arun Siluvery]

This patch adds functions to setup WA batch buffers but they are not yet
enabled in this patch. Some of the WA are to be applied during context save
but before restore and some at the end of context save/restore but before
executing the instructions in the ring, WA batch buffers are created for
this purpose and these WA cannot be applied using normal means.

Signed-off-by: Namrta <namrta.salonie@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |   3 ++
 drivers/gpu/drm/i915/intel_lrc.c | 101 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 731b5ce..dd4b31d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -814,6 +814,9 @@ struct intel_context {
 
 	/* Execlists */
 	bool rcs_initialized;
+	struct intel_ringbuffer *indirect_ctx_wa_bb;
+	struct intel_ringbuffer *per_ctx_wa_bb;
+
 	struct {
 		struct drm_i915_gem_object *state;
 		struct intel_ringbuffer *ringbuf;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0413b8f..50e1b37 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1077,6 +1077,107 @@ static int intel_logical_ring_workarounds_emit(struct intel_engine_cs *ring,
 	return 0;
 }
 
+static struct intel_ringbuffer *
+create_wa_bb(struct intel_engine_cs *ring, uint32_t bb_size)
+{
+	struct drm_device *dev = ring->dev;
+	struct intel_ringbuffer *ringbuf;
+	int ret;
+
+	ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
+	if (!ringbuf)
+		return NULL;
+
+	ringbuf->ring = ring;
+	ringbuf->size = roundup(bb_size, PAGE_SIZE);
+	ringbuf->effective_size = ringbuf->size;
+	ringbuf->head = 0;
+	ringbuf->tail = 0;
+	ringbuf->space = ringbuf->size;
+	ringbuf->last_retired_head = -1;
+
+	ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
+	if (ret) {
+		DRM_DEBUG_DRIVER(
+		"Failed to allocate ringbuf obj for wa_bb%s: %d\n",
+		ring->name, ret);
+		kfree(ringbuf);
+		return NULL;
+	}
+
+        ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
+        if (ret) {
+                DRM_ERROR("Failed to pin and map %s w/a batch: %d\n",
+                          ring->name, ret);
+                intel_destroy_ringbuffer_obj(ringbuf);
+                kfree(ringbuf);
+                return NULL;
+        }
+
+	return ringbuf;
+}
+
+static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
+				    struct intel_context *ctx)
+{
+	int i;
+	struct intel_ringbuffer *ringbuf = NULL;
+
+	ringbuf = create_wa_bb(ring, PAGE_SIZE);
+	if (!ringbuf)
+		return -ENOMEM;
+
+	ctx->indirect_ctx_wa_bb = ringbuf;
+
+	/* FIXME: fill one cache line with NOOPs.
+	 * Replace these instructions with WA
+	 */
+	for (i = 0; i < 16; ++i)
+		intel_logical_ring_emit(ringbuf, MI_NOOP);
+
+	/*
+	 * MI_BATCH_BUFFER_END is not required in Indirect ctx BB because
+	 * execution depends on the size defined in CTX_RCS_INDIRECT_CTX
+	 */
+
+	return 0;
+}
+
+static int gen8_init_perctx_bb(struct intel_engine_cs *ring,
+			       struct intel_context *ctx)
+{
+	int i;
+	struct intel_ringbuffer *ringbuf = NULL;
+
+	ringbuf = create_wa_bb(ring, PAGE_SIZE);
+	if (!ringbuf)
+		return -ENOMEM;
+
+	ctx->per_ctx_wa_bb = ringbuf;
+
+	/* FIXME: Replace these instructions with WA */
+	for (i = 0; i < 15; ++i)
+		intel_logical_ring_emit(ringbuf, MI_NOOP);
+
+	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_END);
+
+	return 0;
+}
+
+static int intel_init_workaround_bb(struct intel_engine_cs *ring,
+				    struct intel_context *ctx)
+{
+	int ret;
+	struct drm_device *dev = ring->dev;
+
+	if (WARN_ON(ring->id != RCS))
+		return -EINVAL;
+
+	/* FIXME: Add Gen specific init functions */
+
+	return 0;
+}
+
 static int gen8_init_common_ring(struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = ring->dev;
-- 
2.3.0

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

[PATCH v2 2/7] drm/i915/gen8: Re-order init pipe_control in lrc mode

[Arun Siluvery]

Some of the WA applied using WA batch buffers perform writes to scratch page.
In the current flow WA are initialized before scratch obj is allocated.
This patch reorders intel_init_pipe_control() to have a valid scratch obj
before we initialize WA.

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 50e1b37..d124636 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1571,11 +1571,16 @@ static int logical_render_ring_init(struct drm_device *dev)
 	ring->emit_bb_start = gen8_emit_bb_start;
 
 	ring->dev = dev;
+
+	ret = intel_init_pipe_control(ring);
+	if (ret)
+		return ret;
+
 	ret = logical_ring_init(dev, ring);
 	if (ret)
 		return ret;
 
-	return intel_init_pipe_control(ring);
+	return 0;
 }
 
 static int logical_bsd_ring_init(struct drm_device *dev)
-- 
2.3.0

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

[PATCH v2 3/7] drm/i915/gen8: Enable WA batch buffers during ctx save/restore

[Arun Siluvery]

Populate Ctx offset pointer registers with WA batch buffer address;
Let the HW know about these batch buffers so that it can execute them
during ctx save/restore.

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 74 +++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h |  3 ++
 2 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d124636..ee968e1 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -211,6 +211,7 @@ enum {
 	FAULT_AND_CONTINUE /* Unsupported */
 };
 #define GEN8_CTX_ID_SHIFT 32
+#define CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT  0x17
 
 static int intel_lr_context_pin(struct intel_engine_cs *ring,
 		struct intel_context *ctx);
@@ -1173,7 +1174,18 @@ static int intel_init_workaround_bb(struct intel_engine_cs *ring,
 	if (WARN_ON(ring->id != RCS))
 		return -EINVAL;
 
-	/* FIXME: Add Gen specific init functions */
+	if (IS_GEN8(dev)) {
+		ret = gen8_init_indirectctx_bb(ring, ctx);
+		if (ret)
+			return ret;
+
+		ret = gen8_init_perctx_bb(ring, ctx);
+		if (ret)
+			return ret;
+	} else {
+		WARN_ONCE(INTEL_INFO(dev)->gen > 8,
+			  "WA batch buffers are not initialized\n");
+	}
 
 	return 0;
 }
@@ -1561,6 +1573,7 @@ static int logical_render_ring_init(struct drm_device *dev)
 	else
 		ring->init_hw = gen8_init_render_ring;
 	ring->init_context = gen8_init_rcs_context;
+	ring->init_context_bb = intel_init_workaround_bb;
 	ring->cleanup = intel_fini_pipe_control;
 	ring->get_seqno = gen8_get_seqno;
 	ring->set_seqno = gen8_set_seqno;
@@ -1860,15 +1873,29 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o
 	reg_state[CTX_SECOND_BB_STATE] = ring->mmio_base + 0x118;
 	reg_state[CTX_SECOND_BB_STATE+1] = 0;
 	if (ring->id == RCS) {
-		/* TODO: according to BSpec, the register state context
-		 * for CHV does not have these. OTOH, these registers do
-		 * exist in CHV. I'm waiting for a clarification */
 		reg_state[CTX_BB_PER_CTX_PTR] = ring->mmio_base + 0x1c0;
-		reg_state[CTX_BB_PER_CTX_PTR+1] = 0;
+
+		if (ctx->per_ctx_wa_bb)
+			reg_state[CTX_BB_PER_CTX_PTR + 1] =
+				i915_gem_obj_ggtt_offset(
+					ctx->per_ctx_wa_bb->obj) | 0x01;
+		else
+			reg_state[CTX_BB_PER_CTX_PTR+1] = 0;
+
 		reg_state[CTX_RCS_INDIRECT_CTX] = ring->mmio_base + 0x1c4;
-		reg_state[CTX_RCS_INDIRECT_CTX+1] = 0;
 		reg_state[CTX_RCS_INDIRECT_CTX_OFFSET] = ring->mmio_base + 0x1c8;
-		reg_state[CTX_RCS_INDIRECT_CTX_OFFSET+1] = 0;
+
+		if (ctx->indirect_ctx_wa_bb) {
+			reg_state[CTX_RCS_INDIRECT_CTX + 1] =
+				i915_gem_obj_ggtt_offset(
+				ctx->indirect_ctx_wa_bb->obj) | 0x01;
+
+			reg_state[CTX_RCS_INDIRECT_CTX_OFFSET + 1] =
+				CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT << 6;
+		} else {
+			reg_state[CTX_RCS_INDIRECT_CTX+1] = 0;
+			reg_state[CTX_RCS_INDIRECT_CTX_OFFSET+1] = 0;
+		}
 	}
 	reg_state[CTX_LRI_HEADER_1] = MI_LOAD_REGISTER_IMM(9);
 	reg_state[CTX_LRI_HEADER_1] |= MI_LRI_FORCE_POSTED;
@@ -1935,6 +1962,18 @@ void intel_lr_context_free(struct intel_context *ctx)
 			drm_gem_object_unreference(&ctx_obj->base);
 		}
 	}
+
+	if (ctx->indirect_ctx_wa_bb) {
+		intel_unpin_ringbuffer_obj(ctx->indirect_ctx_wa_bb);
+		intel_destroy_ringbuffer_obj(ctx->indirect_ctx_wa_bb);
+		kfree(ctx->indirect_ctx_wa_bb);
+	}
+
+	if (ctx->per_ctx_wa_bb) {
+		intel_unpin_ringbuffer_obj(ctx->per_ctx_wa_bb);
+		intel_destroy_ringbuffer_obj(ctx->per_ctx_wa_bb);
+		kfree(ctx->per_ctx_wa_bb);
+	}
 }
 
 static uint32_t get_lr_context_size(struct intel_engine_cs *ring)
@@ -2060,6 +2099,16 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 
 	}
 
+	if (ring->id == RCS && !ctx->rcs_initialized) {
+		if (ring->init_context_bb) {
+			ret = ring->init_context_bb(ring, ctx);
+			if (ret) {
+				DRM_ERROR("ring init context bb: %d\n", ret);
+				goto error;
+			}
+		}
+	}
+
 	ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
@@ -2088,6 +2137,17 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	return 0;
 
 error:
+	if (ctx->indirect_ctx_wa_bb) {
+		intel_unpin_ringbuffer_obj(ctx->indirect_ctx_wa_bb);
+		intel_destroy_ringbuffer_obj(ctx->indirect_ctx_wa_bb);
+		kfree(ctx->indirect_ctx_wa_bb);
+	}
+	if (ctx->per_ctx_wa_bb) {
+		intel_unpin_ringbuffer_obj(ctx->per_ctx_wa_bb);
+		intel_destroy_ringbuffer_obj(ctx->per_ctx_wa_bb);
+		kfree(ctx->per_ctx_wa_bb);
+	}
+
 	if (is_global_default_ctx)
 		intel_unpin_ringbuffer_obj(ringbuf);
 error_destroy_rbuf:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 39f6dfc..ddb8421 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -154,6 +154,9 @@ struct  intel_engine_cs {
 	int		(*init_context)(struct intel_engine_cs *ring,
 					struct intel_context *ctx);
 
+	int		(*init_context_bb)(struct intel_engine_cs *ring,
+					   struct intel_context *ctx);
+
 	void		(*write_tail)(struct intel_engine_cs *ring,
 				      u32 value);
 	int __must_check (*flush)(struct intel_engine_cs *ring,
-- 
2.3.0

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

[PATCH v2 4/7] drm/i915/gen8: Add WaDisableCtxRestoreArbitration workaround

[Arun Siluvery]

In Indirect and Per context w/a batch buffer,
+WaDisableCtxRestoreArbitration

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index ee968e1..8ed6433 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1121,7 +1121,6 @@ create_wa_bb(struct intel_engine_cs *ring, uint32_t bb_size)
 static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 				    struct intel_context *ctx)
 {
-	int i;
 	struct intel_ringbuffer *ringbuf = NULL;
 
 	ringbuf = create_wa_bb(ring, PAGE_SIZE);
@@ -1130,10 +1129,11 @@ static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 
 	ctx->indirect_ctx_wa_bb = ringbuf;
 
-	/* FIXME: fill one cache line with NOOPs.
-	 * Replace these instructions with WA
-	 */
-	for (i = 0; i < 16; ++i)
+	/* WaDisableCtxRestoreArbitration:bdw,chv */
+	intel_logical_ring_emit(ringbuf, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+
+	/* padding */
+	while (((unsigned long) ringbuf->tail % CACHELINE_BYTES) != 0)
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
 
 	/*
@@ -1156,8 +1156,11 @@ static int gen8_init_perctx_bb(struct intel_engine_cs *ring,
 
 	ctx->per_ctx_wa_bb = ringbuf;
 
-	/* FIXME: Replace these instructions with WA */
-	for (i = 0; i < 15; ++i)
+	/* WaDisableCtxRestoreArbitration:bdw,chv */
+	intel_logical_ring_emit(ringbuf, MI_ARB_ON_OFF | MI_ARB_ENABLE);
+
+	/* padding */
+	for (i = 0; i < 14; ++i)
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
 
 	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_END);
-- 
2.3.0

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

[PATCH v2 5/7] drm/i915/gen8: Add WaFlushCoherentL3CacheLinesAtContextSwitch workaround

[Arun Siluvery]

In Indirect context w/a batch buffer,
+WaFlushCoherentL3CacheLinesAtContextSwitch

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 1 +
 drivers/gpu/drm/i915/intel_lrc.c | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 84af255..5203c79 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -426,6 +426,7 @@
 #define   PIPE_CONTROL_INDIRECT_STATE_DISABLE		(1<<9)
 #define   PIPE_CONTROL_NOTIFY				(1<<8)
 #define   PIPE_CONTROL_FLUSH_ENABLE			(1<<7) /* gen7+ */
+#define   PIPE_CONTROL_DC_FLUSH_ENABLE			(1<<5)
 #define   PIPE_CONTROL_VF_CACHE_INVALIDATE		(1<<4)
 #define   PIPE_CONTROL_CONST_CACHE_INVALIDATE		(1<<3)
 #define   PIPE_CONTROL_STATE_CACHE_INVALIDATE		(1<<2)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 8ed6433..24a7dcd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1132,6 +1132,14 @@ static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
 	intel_logical_ring_emit(ringbuf, MI_ARB_ON_OFF | MI_ARB_DISABLE);
 
+	/* WaFlushCoherentL3CacheLinesAtContextSwitch:bdw,chv */
+	intel_logical_ring_emit(ringbuf, GFX_OP_PIPE_CONTROL(6));
+	intel_logical_ring_emit(ringbuf, PIPE_CONTROL_DC_FLUSH_ENABLE);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf, 0);
+
 	/* padding */
 	while (((unsigned long) ringbuf->tail % CACHELINE_BYTES) != 0)
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
-- 
2.3.0

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

[PATCH v2 6/7] drm/i915/gen8: Add WaClearSlmSpaceAtContextSwitch workaround

[Arun Siluvery]

In Indirect context w/a batch buffer,
WaClearSlmSpaceAtContextSwitch

v2: s/PIPE_CONTROL_FLUSH_RO_CACHES/PIPE_CONTROL_FLUSH_L3 (Ville)

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_lrc.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5203c79..33b0ff1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -410,6 +410,7 @@
 #define   DISPLAY_PLANE_A           (0<<20)
 #define   DISPLAY_PLANE_B           (1<<20)
 #define GFX_OP_PIPE_CONTROL(len)	((0x3<<29)|(0x3<<27)|(0x2<<24)|(len-2))
+#define   PIPE_CONTROL_FLUSH_L3				(1<<27)
 #define   PIPE_CONTROL_GLOBAL_GTT_IVB			(1<<24) /* gen7+ */
 #define   PIPE_CONTROL_MMIO_WRITE			(1<<23)
 #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 24a7dcd..c49f2ce 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1121,8 +1121,15 @@ create_wa_bb(struct intel_engine_cs *ring, uint32_t bb_size)
 static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 				    struct intel_context *ctx)
 {
+	u32 scratch_addr;
+	unsigned long flags = 0;
 	struct intel_ringbuffer *ringbuf = NULL;
 
+	if (ring->scratch.obj == NULL) {
+		DRM_ERROR("scratch page not allocated for %s\n", ring->name);
+		return -EINVAL;
+	}
+
 	ringbuf = create_wa_bb(ring, PAGE_SIZE);
 	if (!ringbuf)
 		return -ENOMEM;
@@ -1140,6 +1147,23 @@ static int gen8_init_indirectctx_bb(struct intel_engine_cs *ring,
 	intel_logical_ring_emit(ringbuf, 0);
 	intel_logical_ring_emit(ringbuf, 0);
 
+	/* WaClearSlmSpaceAtContextSwitch:bdw,chv */
+	flags = PIPE_CONTROL_FLUSH_L3 |
+		PIPE_CONTROL_GLOBAL_GTT_IVB |
+		PIPE_CONTROL_CS_STALL |
+		PIPE_CONTROL_QW_WRITE;
+
+	/* Actual scratch location is at 128 bytes offset */
+	scratch_addr = ring->scratch.gtt_offset + 2*CACHELINE_BYTES;
+	scratch_addr |= PIPE_CONTROL_GLOBAL_GTT;
+
+	intel_logical_ring_emit(ringbuf, GFX_OP_PIPE_CONTROL(6));
+	intel_logical_ring_emit(ringbuf, flags);
+	intel_logical_ring_emit(ringbuf, scratch_addr);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf, 0);
+
 	/* padding */
 	while (((unsigned long) ringbuf->tail % CACHELINE_BYTES) != 0)
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
-- 
2.3.0

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

[PATCH v2 7/7] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

[Arun Siluvery]

In Per context w/a batch buffer,
WaRsRestoreWithPerCtxtBb

v2: This patches modifies definitions of MI_LOAD_REGISTER_MEM and
MI_LOAD_REGISTER_REG; Add GEN8 specific defines for these instructions
so as to not break any future users of existing definitions (Michel)

Signed-off-by: Rafael Barbalho <rafael.barbalho@intel.com>
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 26 ++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c | 65 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 90 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 33b0ff1..6928162 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -347,6 +347,26 @@
 #define   MI_INVALIDATE_BSD		(1<<7)
 #define   MI_FLUSH_DW_USE_GTT		(1<<2)
 #define   MI_FLUSH_DW_USE_PPGTT		(0<<2)
+#define MI_ATOMIC(len)	MI_INSTR(0x2F, (len-2))
+#define   MI_ATOMIC_MEMORY_TYPE_GGTT	(1<<22)
+#define   MI_ATOMIC_INLINE_DATA		(1<<18)
+#define   MI_ATOMIC_CS_STALL		(1<<17)
+#define   MI_ATOMIC_RETURN_DATA_CTL	(1<<16)
+#define MI_ATOMIC_OP_MASK(op)  ((op) << 8)
+#define MI_ATOMIC_AND	MI_ATOMIC_OP_MASK(0x01)
+#define MI_ATOMIC_OR	MI_ATOMIC_OP_MASK(0x02)
+#define MI_ATOMIC_XOR	MI_ATOMIC_OP_MASK(0x03)
+#define MI_ATOMIC_MOVE	MI_ATOMIC_OP_MASK(0x04)
+#define MI_ATOMIC_INC	MI_ATOMIC_OP_MASK(0x05)
+#define MI_ATOMIC_DEC	MI_ATOMIC_OP_MASK(0x06)
+#define MI_ATOMIC_ADD	MI_ATOMIC_OP_MASK(0x07)
+#define MI_ATOMIC_SUB	MI_ATOMIC_OP_MASK(0x08)
+#define MI_ATOMIC_RSUB	MI_ATOMIC_OP_MASK(0x09)
+#define MI_ATOMIC_IMAX	MI_ATOMIC_OP_MASK(0x0A)
+#define MI_ATOMIC_IMIN	MI_ATOMIC_OP_MASK(0x0B)
+#define MI_ATOMIC_UMAX	MI_ATOMIC_OP_MASK(0x0C)
+#define MI_ATOMIC_UMIN	MI_ATOMIC_OP_MASK(0x0D)
+
 #define MI_BATCH_BUFFER		MI_INSTR(0x30, 1)
 #define   MI_BATCH_NON_SECURE		(1)
 /* for snb/ivb/vlv this also means "batch in ppgtt" when ppgtt is enabled. */
@@ -453,6 +473,10 @@
 #define   MI_REPORT_PERF_COUNT_GGTT (1<<0)
 #define MI_LOAD_REGISTER_MEM    MI_INSTR(0x29, 0)
 #define MI_LOAD_REGISTER_REG    MI_INSTR(0x2A, 0)
+#define MI_LOAD_REGISTER_MEM_GEN8 MI_INSTR(0x29, 2)
+#define   MI_LRM_USE_GLOBAL_GTT (1<<22)
+#define   MI_LRM_ASYNC_MODE_ENABLE (1<<21)
+#define MI_LOAD_REGISTER_REG_GEN8 MI_INSTR(0x2A, 1)
 #define MI_RS_STORE_DATA_IMM    MI_INSTR(0x2B, 0)
 #define MI_LOAD_URB_MEM         MI_INSTR(0x2C, 0)
 #define MI_STORE_URB_MEM        MI_INSTR(0x2D, 0)
@@ -1799,6 +1823,8 @@ enum skl_disp_power_wells {
 #define   GEN8_RC_SEMA_IDLE_MSG_DISABLE	(1 << 12)
 #define   GEN8_FF_DOP_CLOCK_GATE_DISABLE	(1<<10)
 
+#define GEN8_RS_PREEMPT_STATUS		0x215C
+
 /* Fuse readout registers for GT */
 #define CHV_FUSE_GT			(VLV_DISPLAY_BASE + 0x2168)
 #define   CHV_FGT_DISABLE_SS0		(1 << 10)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c49f2ce..ca2da11 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1180,19 +1180,82 @@ static int gen8_init_perctx_bb(struct intel_engine_cs *ring,
 			       struct intel_context *ctx)
 {
 	int i;
+	u32 scratch_addr;
+	unsigned long flags = 0;
 	struct intel_ringbuffer *ringbuf = NULL;
 
+	if (ring->scratch.obj == NULL) {
+		DRM_ERROR("scratch page not allocated for %s\n", ring->name);
+		return -EINVAL;
+	}
+
 	ringbuf = create_wa_bb(ring, PAGE_SIZE);
 	if (!ringbuf)
 		return -ENOMEM;
 
 	ctx->per_ctx_wa_bb = ringbuf;
 
+	/* Actual scratch location is at 128 bytes offset */
+	scratch_addr = ring->scratch.gtt_offset + 2*CACHELINE_BYTES;
+	scratch_addr |= PIPE_CONTROL_GLOBAL_GTT;
+
 	/* WaDisableCtxRestoreArbitration:bdw,chv */
 	intel_logical_ring_emit(ringbuf, MI_ARB_ON_OFF | MI_ARB_ENABLE);
 
+	/*
+	 * As per Bspec, to workaround a known HW issue, SW must perform the
+	 * below programming sequence prior to programming MI_BATCH_BUFFER_END.
+	 *
+	 * This is only applicable for Gen8.
+	 */
+
+	/* WaRsRestoreWithPerCtxtBb:bdw,chv */
+	intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(1));
+	intel_logical_ring_emit(ringbuf, INSTPM);
+	intel_logical_ring_emit(ringbuf,
+				_MASKED_BIT_DISABLE(INSTPM_FORCE_ORDERING));
+
+	flags = MI_ATOMIC_MEMORY_TYPE_GGTT |
+		MI_ATOMIC_INLINE_DATA |
+		MI_ATOMIC_CS_STALL |
+		MI_ATOMIC_RETURN_DATA_CTL |
+		MI_ATOMIC_MOVE;
+
+	intel_logical_ring_emit(ringbuf, MI_ATOMIC(5) | flags);
+	intel_logical_ring_emit(ringbuf, scratch_addr);
+	intel_logical_ring_emit(ringbuf, 0);
+	intel_logical_ring_emit(ringbuf,
+				_MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
+	intel_logical_ring_emit(ringbuf,
+				_MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
+
+	/*
+	 * BSpec says MI_LOAD_REGISTER_MEM, MI_LOAD_REGISTER_REG and
+	 * MI_BATCH_BUFFER_END instructions in this sequence need to be
+	 * in the same cacheline.
+	 */
+	while (((unsigned long) ringbuf->tail % CACHELINE_BYTES) != 0)
+		intel_logical_ring_emit(ringbuf, MI_NOOP);
+
+	intel_logical_ring_emit(ringbuf,
+				MI_LOAD_REGISTER_MEM_GEN8 |
+				MI_LRM_USE_GLOBAL_GTT |
+				MI_LRM_ASYNC_MODE_ENABLE);
+	intel_logical_ring_emit(ringbuf, INSTPM);
+	intel_logical_ring_emit(ringbuf, scratch_addr);
+	intel_logical_ring_emit(ringbuf, 0);
+
+	/*
+	 * BSpec says there should not be any commands programmed
+	 * between MI_LOAD_REGISTER_REG and MI_BATCH_BUFFER_END so
+	 * do not add any new commands
+	 */
+	intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_REG_GEN8);
+	intel_logical_ring_emit(ringbuf, GEN8_RS_PREEMPT_STATUS);
+	intel_logical_ring_emit(ringbuf, GEN8_RS_PREEMPT_STATUS);
+
 	/* padding */
-	for (i = 0; i < 14; ++i)
+	for (i = 0; i < 8; ++i)
 		intel_logical_ring_emit(ringbuf, MI_NOOP);
 
 	intel_logical_ring_emit(ringbuf, MI_BATCH_BUFFER_END);
-- 
2.3.0

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Chris Wilson]

On Fri, May 29, 2015 at 07:03:19PM +0100, Arun Siluvery wrote:
> This patch adds functions to setup WA batch buffers but they are not yet
> enabled in this patch. Some of the WA are to be applied during context save
> but before restore and some at the end of context save/restore but before
> executing the instructions in the ring, WA batch buffers are created for
> this purpose and these WA cannot be applied using normal means.
> 
> Signed-off-by: Namrta <namrta.salonie@intel.com>
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   3 ++
>  drivers/gpu/drm/i915/intel_lrc.c | 101 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 731b5ce..dd4b31d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -814,6 +814,9 @@ struct intel_context {
>  
>  	/* Execlists */
>  	bool rcs_initialized;
> +	struct intel_ringbuffer *indirect_ctx_wa_bb;
> +	struct intel_ringbuffer *per_ctx_wa_bb;

Eh? They are only command sequences whose starting addresses you encode
into the execlists context. Why have you allocated a ringbuf not an
object? Why have you allocated 2 pages when you only need one, and could
even find space elsewhere in the context....

And these should be pinned alongside the context *not permanently*.

I want a debug mode that limits us to say 16M of GGTT space so that
these address space leaks are easier to demonstrate in practice.
-Chris

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

Re: [PATCH v2 7/7] drm/i915/gen8: Add WaRsRestoreWithPerCtxtBb workaround

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6506
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  303/303              303/303
SNB                 -1              315/315              314/315
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                 -1              320/320              319/320
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*SNB  igt@pm_rpm@dpms-mode-unset-non-lpsp      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
*BDW  igt@gem_flink_race@flink_name      PASS(1)      DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_display.c:#assert_plane[i915]()@WARNING:.* at .* assert_plane
assertion_failure@assertion failure
WARNING:at_drivers/gpu/drm/drm_irq.c:#drm_wait_one_vblank[drm]()@WARNING:.* at .* drm_wait_one_vblank+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Siluvery, Arun]

On 29/05/2015 19:16, Chris Wilson wrote:
> On Fri, May 29, 2015 at 07:03:19PM +0100, Arun Siluvery wrote:
>> This patch adds functions to setup WA batch buffers but they are not yet
>> enabled in this patch. Some of the WA are to be applied during context save
>> but before restore and some at the end of context save/restore but before
>> executing the instructions in the ring, WA batch buffers are created for
>> this purpose and these WA cannot be applied using normal means.
>>
>> Signed-off-by: Namrta <namrta.salonie@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |   3 ++
>>   drivers/gpu/drm/i915/intel_lrc.c | 101 +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 104 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 731b5ce..dd4b31d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -814,6 +814,9 @@ struct intel_context {
>>
>>   	/* Execlists */
>>   	bool rcs_initialized;
>> +	struct intel_ringbuffer *indirect_ctx_wa_bb;
>> +	struct intel_ringbuffer *per_ctx_wa_bb;
>
> Eh? They are only command sequences whose starting addresses you encode
> into the execlists context. Why have you allocated a ringbuf not an
> object? Why have you allocated 2 pages when you only need one, and could
> even find space elsewhere in the context....

ringbuf is only used so that I can use logical_ring_*(), object can also 
be used.
Single page is enough but since we have two batch buffers and need to 
provide offsets in two different registers, two pages are used for 
simplifying things, I guess we can manage with single page, I will try this.

Your idea of using space in context itself simplifies many things but 
the context size varies across Gens, is it safe to pick last page or 
increase the size by one more page and use that to load these 
instructions? I think using an additional page is safe to avoid the risk 
of HW overwriting that page or do you have any other recommendation? I 
will first try and see if it works.

>
> And these should be pinned alongside the context *not permanently*.
right, I will correct this but this won't be required if we use the 
space in context.

>
> I want a debug mode that limits us to say 16M of GGTT space so that
> these address space leaks are easier to demonstrate in practice.
> -Chris
>

regards
Arun

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Chris Wilson]

On Mon, Jun 01, 2015 at 11:01:08AM +0100, Siluvery, Arun wrote:
> On 29/05/2015 19:16, Chris Wilson wrote:
> >On Fri, May 29, 2015 at 07:03:19PM +0100, Arun Siluvery wrote:
> >>This patch adds functions to setup WA batch buffers but they are not yet
> >>enabled in this patch. Some of the WA are to be applied during context save
> >>but before restore and some at the end of context save/restore but before
> >>executing the instructions in the ring, WA batch buffers are created for
> >>this purpose and these WA cannot be applied using normal means.
> >>
> >>Signed-off-by: Namrta <namrta.salonie@intel.com>
> >>Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/i915_drv.h  |   3 ++
> >>  drivers/gpu/drm/i915/intel_lrc.c | 101 +++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 104 insertions(+)
> >>
> >>diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>index 731b5ce..dd4b31d 100644
> >>--- a/drivers/gpu/drm/i915/i915_drv.h
> >>+++ b/drivers/gpu/drm/i915/i915_drv.h
> >>@@ -814,6 +814,9 @@ struct intel_context {
> >>
> >>  	/* Execlists */
> >>  	bool rcs_initialized;
> >>+	struct intel_ringbuffer *indirect_ctx_wa_bb;
> >>+	struct intel_ringbuffer *per_ctx_wa_bb;
> >
> >Eh? They are only command sequences whose starting addresses you encode
> >into the execlists context. Why have you allocated a ringbuf not an
> >object? Why have you allocated 2 pages when you only need one, and could
> >even find space elsewhere in the context....
> 
> ringbuf is only used so that I can use logical_ring_*(), object can
> also be used.
> Single page is enough but since we have two batch buffers and need
> to provide offsets in two different registers, two pages are used
> for simplifying things, I guess we can manage with single page, I
> will try this.
> 
> Your idea of using space in context itself simplifies many things
> but the context size varies across Gens, is it safe to pick last
> page or increase the size by one more page and use that to load
> these instructions? I think using an additional page is safe to
> avoid the risk of HW overwriting that page or do you have any other
> recommendation? I will first try and see if it works.
> 
> >
> >And these should be pinned alongside the context *not permanently*.
> right, I will correct this but this won't be required if we use the
> space in context.

Indeed, allocating an extra scratch page in the context would simplify
vma/mm management. A trick might be to allocate the scratch page at the
start, then offset the lrc regs etc - that would then be consistent
amongst gen and be easy enough to extend if we need more per-context
scratch space in future.
-Chris

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Daniel, Thomas]

> 
> Indeed, allocating an extra scratch page in the context would simplify
> vma/mm management. A trick might be to allocate the scratch page at the
> start, then offset the lrc regs etc - that would then be consistent
> amongst gen and be easy enough to extend if we need more per-context
> scratch space in future.
> -Chris

Yes, I think we already have another use for more per-context space at the start.  The GuC is planning to do this.  Arun, you probably should work with Alex Dai and Dave Gordon to avoid conflicts here.

Thomas.

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Siluvery, Arun]

On 01/06/2015 11:22, Daniel, Thomas wrote:
>>
>> Indeed, allocating an extra scratch page in the context would simplify
>> vma/mm management. A trick might be to allocate the scratch page at the
>> start, then offset the lrc regs etc - that would then be consistent
>> amongst gen and be easy enough to extend if we need more per-context
>> scratch space in future.
>> -Chris
>
> Yes, I think we already have another use for more per-context space at the start.  The GuC is planning to do this.  Arun, you probably should work with Alex Dai and Dave Gordon to avoid conflicts here.
>
> Thomas.
>

Thanks for the heads-up Thomas.
I have discussed with Dave and agreed to share this page;
GuC probably doesn't need whole page so first half is reserved for it's 
use and second half is used for WA.

I have modified my patches to use context page for applying these WA and 
don't see any issues.

During the discussions Dave proposed another approach. Even though these 
WA are called per context they are only initialized once and not changed 
afterwards, same set of WA are applied for each context so instead of 
adding them in each context, does it make sense to create a separate 
page and share across all contexts? but ofcourse GuC will anyway add a 
new page to context so I might as well share that page.

Chris/Dave, do you see any problems with sharing page with GuC or you 
prefer to allocate a separate page for these WA and share across all 
contexts? Please give your comments.

regards
Arun


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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Dave Gordon]

On 02/06/15 19:36, Siluvery, Arun wrote:
> On 01/06/2015 11:22, Daniel, Thomas wrote:
>>>
>>> Indeed, allocating an extra scratch page in the context would simplify
>>> vma/mm management. A trick might be to allocate the scratch page at the
>>> start, then offset the lrc regs etc - that would then be consistent
>>> amongst gen and be easy enough to extend if we need more per-context
>>> scratch space in future.
>>> -Chris
>>
>> Yes, I think we already have another use for more per-context space at
>> the start.  The GuC is planning to do this.  Arun, you probably should
>> work with Alex Dai and Dave Gordon to avoid conflicts here.
>>
>> Thomas.
>>
> 
> Thanks for the heads-up Thomas.
> I have discussed with Dave and agreed to share this page;
> GuC probably doesn't need whole page so first half is reserved for it's
> use and second half is used for WA.
> 
> I have modified my patches to use context page for applying these WA and
> don't see any issues.
> 
> During the discussions Dave proposed another approach. Even though these
> WA are called per context they are only initialized once and not changed
> afterwards, same set of WA are applied for each context so instead of
> adding them in each context, does it make sense to create a separate
> page and share across all contexts? but of course GuC will anyway add a
> new page to context so I might as well share that page.
> 
> Chris/Dave, do you see any problems with sharing page with GuC or you
> prefer to allocate a separate page for these WA and share across all
> contexts? Please give your comments.
> 
> regards
> Arun

I think we have to consider which is more future-proof i.e. which is
least likely:
(1) the area shared with the GuC grows (definitely still in flux), or
(2) workarounds need to be context-specific (possible, but unlikely)

So I'd prefer a single area set up just once to contain the pre- and
post-context restore workaround batches. If necessary, the one area
could contain multiple batches at different offsets, so we could point
different contexts at different (shared) batches as required. I think
they're unlikely to actually need per-context customisation[*], but
there might be a need for different workarounds according to workload
type or privilege level or some other criterion ... ?

.Dave.

[*] unless they need per-context memory addresses coded into them?

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Siluvery, Arun]

On 02/06/2015 19:47, Dave Gordon wrote:
> On 02/06/15 19:36, Siluvery, Arun wrote:
>> On 01/06/2015 11:22, Daniel, Thomas wrote:
>>>>
>>>> Indeed, allocating an extra scratch page in the context would simplify
>>>> vma/mm management. A trick might be to allocate the scratch page at the
>>>> start, then offset the lrc regs etc - that would then be consistent
>>>> amongst gen and be easy enough to extend if we need more per-context
>>>> scratch space in future.
>>>> -Chris
>>>
>>> Yes, I think we already have another use for more per-context space at
>>> the start.  The GuC is planning to do this.  Arun, you probably should
>>> work with Alex Dai and Dave Gordon to avoid conflicts here.
>>>
>>> Thomas.
>>>
>>
>> Thanks for the heads-up Thomas.
>> I have discussed with Dave and agreed to share this page;
>> GuC probably doesn't need whole page so first half is reserved for it's
>> use and second half is used for WA.
>>
>> I have modified my patches to use context page for applying these WA and
>> don't see any issues.
>>
>> During the discussions Dave proposed another approach. Even though these
>> WA are called per context they are only initialized once and not changed
>> afterwards, same set of WA are applied for each context so instead of
>> adding them in each context, does it make sense to create a separate
>> page and share across all contexts? but of course GuC will anyway add a
>> new page to context so I might as well share that page.
>>
>> Chris/Dave, do you see any problems with sharing page with GuC or you
>> prefer to allocate a separate page for these WA and share across all
>> contexts? Please give your comments.
>>
>> regards
>> Arun
>
> I think we have to consider which is more future-proof i.e. which is
> least likely:
> (1) the area shared with the GuC grows (definitely still in flux), or
> (2) workarounds need to be context-specific (possible, but unlikely)
>
> So I'd prefer a single area set up just once to contain the pre- and
> post-context restore workaround batches. If necessary, the one area
> could contain multiple batches at different offsets, so we could point
> different contexts at different (shared) batches as required. I think
> they're unlikely to actually need per-context customisation[*], but
> there might be a need for different workarounds according to workload
> type or privilege level or some other criterion ... ?
>
> .Dave.
>
> [*] unless they need per-context memory addresses coded into them?
>

Considering these WA are initialized only once and not changed 
afterwards and GuC area probably grows in future which may run into the 
space used by WA, independent single area setup makes senses.
I also checked spec and it is not clear whether any customization is 
going to be required for different contexts.
I have modified patches to setup a single page with WA when 
default_context is initialized and this is used by all contexts.

I will send patches but please let me know if there are any other comments.

regards
Arun
>
>

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

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Daniel Vetter]

On Fri, May 29, 2015 at 07:16:38PM +0100, Chris Wilson wrote:
> On Fri, May 29, 2015 at 07:03:19PM +0100, Arun Siluvery wrote:
> > This patch adds functions to setup WA batch buffers but they are not yet
> > enabled in this patch. Some of the WA are to be applied during context save
> > but before restore and some at the end of context save/restore but before
> > executing the instructions in the ring, WA batch buffers are created for
> > this purpose and these WA cannot be applied using normal means.
> > 
> > Signed-off-by: Namrta <namrta.salonie@intel.com>
> > Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |   3 ++
> >  drivers/gpu/drm/i915/intel_lrc.c | 101 +++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 731b5ce..dd4b31d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -814,6 +814,9 @@ struct intel_context {
> >  
> >  	/* Execlists */
> >  	bool rcs_initialized;
> > +	struct intel_ringbuffer *indirect_ctx_wa_bb;
> > +	struct intel_ringbuffer *per_ctx_wa_bb;
> 
> Eh? They are only command sequences whose starting addresses you encode
> into the execlists context. Why have you allocated a ringbuf not an
> object? Why have you allocated 2 pages when you only need one, and could
> even find space elsewhere in the context....
> 
> And these should be pinned alongside the context *not permanently*.
> 
> I want a debug mode that limits us to say 16M of GGTT space so that
> these address space leaks are easier to demonstrate in practice.

Yeah the "fix up execlist context to use active list tracking" series
is still pending too ... And artificially limiting ggtt space would indeed
be a neat debug trick. Maybe we'd need to have different limits for
display and other buffers just to avoid making 4k completely unuseable
with this debug knob enabled.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Daniel Vetter]

On Thu, Jun 04, 2015 at 03:30:56PM +0100, Siluvery, Arun wrote:
> On 02/06/2015 19:47, Dave Gordon wrote:
> >On 02/06/15 19:36, Siluvery, Arun wrote:
> >>On 01/06/2015 11:22, Daniel, Thomas wrote:
> >>>>
> >>>>Indeed, allocating an extra scratch page in the context would simplify
> >>>>vma/mm management. A trick might be to allocate the scratch page at the
> >>>>start, then offset the lrc regs etc - that would then be consistent
> >>>>amongst gen and be easy enough to extend if we need more per-context
> >>>>scratch space in future.
> >>>>-Chris
> >>>
> >>>Yes, I think we already have another use for more per-context space at
> >>>the start.  The GuC is planning to do this.  Arun, you probably should
> >>>work with Alex Dai and Dave Gordon to avoid conflicts here.
> >>>
> >>>Thomas.
> >>>
> >>
> >>Thanks for the heads-up Thomas.
> >>I have discussed with Dave and agreed to share this page;
> >>GuC probably doesn't need whole page so first half is reserved for it's
> >>use and second half is used for WA.
> >>
> >>I have modified my patches to use context page for applying these WA and
> >>don't see any issues.
> >>
> >>During the discussions Dave proposed another approach. Even though these
> >>WA are called per context they are only initialized once and not changed
> >>afterwards, same set of WA are applied for each context so instead of
> >>adding them in each context, does it make sense to create a separate
> >>page and share across all contexts? but of course GuC will anyway add a
> >>new page to context so I might as well share that page.
> >>
> >>Chris/Dave, do you see any problems with sharing page with GuC or you
> >>prefer to allocate a separate page for these WA and share across all
> >>contexts? Please give your comments.
> >>
> >>regards
> >>Arun
> >
> >I think we have to consider which is more future-proof i.e. which is
> >least likely:
> >(1) the area shared with the GuC grows (definitely still in flux), or
> >(2) workarounds need to be context-specific (possible, but unlikely)
> >
> >So I'd prefer a single area set up just once to contain the pre- and
> >post-context restore workaround batches. If necessary, the one area
> >could contain multiple batches at different offsets, so we could point
> >different contexts at different (shared) batches as required. I think
> >they're unlikely to actually need per-context customisation[*], but
> >there might be a need for different workarounds according to workload
> >type or privilege level or some other criterion ... ?
> >
> >.Dave.
> >
> >[*] unless they need per-context memory addresses coded into them?
> >
> 
> Considering these WA are initialized only once and not changed afterwards
> and GuC area probably grows in future which may run into the space used by
> WA, independent single area setup makes senses.
> I also checked spec and it is not clear whether any customization is going
> to be required for different contexts.
> I have modified patches to setup a single page with WA when default_context
> is initialized and this is used by all contexts.
> 
> I will send patches but please let me know if there are any other comments.

Yeah if the wa batches aren't ctx specific, then there's really no need to
allocate one of them per ctx. One global buffer with all the wa combined
should really be all we need.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/7] drm/i915/gen8: Add infrastructure to initialize WA batch buffers

[Siluvery, Arun]

On 15/06/2015 11:41, Daniel Vetter wrote:
> On Thu, Jun 04, 2015 at 03:30:56PM +0100, Siluvery, Arun wrote:
>> On 02/06/2015 19:47, Dave Gordon wrote:
>>> On 02/06/15 19:36, Siluvery, Arun wrote:
>>>> On 01/06/2015 11:22, Daniel, Thomas wrote:
>>>>>>
>>>>>> Indeed, allocating an extra scratch page in the context would simplify
>>>>>> vma/mm management. A trick might be to allocate the scratch page at the
>>>>>> start, then offset the lrc regs etc - that would then be consistent
>>>>>> amongst gen and be easy enough to extend if we need more per-context
>>>>>> scratch space in future.
>>>>>> -Chris
>>>>>
>>>>> Yes, I think we already have another use for more per-context space at
>>>>> the start.  The GuC is planning to do this.  Arun, you probably should
>>>>> work with Alex Dai and Dave Gordon to avoid conflicts here.
>>>>>
>>>>> Thomas.
>>>>>
>>>>
>>>> Thanks for the heads-up Thomas.
>>>> I have discussed with Dave and agreed to share this page;
>>>> GuC probably doesn't need whole page so first half is reserved for it's
>>>> use and second half is used for WA.
>>>>
>>>> I have modified my patches to use context page for applying these WA and
>>>> don't see any issues.
>>>>
>>>> During the discussions Dave proposed another approach. Even though these
>>>> WA are called per context they are only initialized once and not changed
>>>> afterwards, same set of WA are applied for each context so instead of
>>>> adding them in each context, does it make sense to create a separate
>>>> page and share across all contexts? but of course GuC will anyway add a
>>>> new page to context so I might as well share that page.
>>>>
>>>> Chris/Dave, do you see any problems with sharing page with GuC or you
>>>> prefer to allocate a separate page for these WA and share across all
>>>> contexts? Please give your comments.
>>>>
>>>> regards
>>>> Arun
>>>
>>> I think we have to consider which is more future-proof i.e. which is
>>> least likely:
>>> (1) the area shared with the GuC grows (definitely still in flux), or
>>> (2) workarounds need to be context-specific (possible, but unlikely)
>>>
>>> So I'd prefer a single area set up just once to contain the pre- and
>>> post-context restore workaround batches. If necessary, the one area
>>> could contain multiple batches at different offsets, so we could point
>>> different contexts at different (shared) batches as required. I think
>>> they're unlikely to actually need per-context customisation[*], but
>>> there might be a need for different workarounds according to workload
>>> type or privilege level or some other criterion ... ?
>>>
>>> .Dave.
>>>
>>> [*] unless they need per-context memory addresses coded into them?
>>>
>>
>> Considering these WA are initialized only once and not changed afterwards
>> and GuC area probably grows in future which may run into the space used by
>> WA, independent single area setup makes senses.
>> I also checked spec and it is not clear whether any customization is going
>> to be required for different contexts.
>> I have modified patches to setup a single page with WA when default_context
>> is initialized and this is used by all contexts.
>>
>> I will send patches but please let me know if there are any other comments.
>
> Yeah if the wa batches aren't ctx specific, then there's really no need to
> allocate one of them per ctx. One global buffer with all the wa combined
> should really be all we need.
> -Daniel
>
Hi Daniel,

Agree, this is already taken into account in the next revision v3 
(already sent to mailing list).

I can see you are still going through the list but when you get there, 
please let me know if you have any other comments.

regards
Arun

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