[PATCH 1/5] drm/i915: Remove redundant TLB invalidate on switching contexts

[PATCH 1/5] drm/i915: Remove redundant TLB invalidate on switching contexts

[Chris Wilson]

We are required to reload the TLBs around context switches
(MI_SET_CONTEXT specifically) and the recommendation is do that before
the MI_SET_CONTEXT so that it is serialised with the switch and not
forgotten:

[DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s
responsibility to invalidate the TLBs at least once after the previous
context switch after any GTT mappings changed (including new GTT entries).
This can be done by a pipeline PIPE_CONTROL with TLB inv bit set
immediately before MI_SET_CONTEXT.

However, we already do an unconditional TLB invalidate before every
batch so this condition is satifisfied.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 99c46f4dbde6..521e6f4705b1 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -607,17 +607,6 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 		0;
 	int len;
 
-	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
-	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
-	 * explicitly, so we rely on the value at ring init, stored in
-	 * itlb_before_ctx_switch.
-	 */
-	if (IS_GEN6(dev_priv)) {
-		int ret = engine->emit_flush(req, EMIT_INVALIDATE);
-		if (ret)
-			return ret;
-	}
-
 	/* These flags are for resource streamer on HSW+ */
 	if (IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8)
 		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
-- 
2.11.0

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

[PATCH 2/5] drm/i915: Remove redundant TLB invalidate on switching ppgtt

[Chris Wilson]

We are required to reload the TLBs around ppgtt switches. However, we
already do an unconditional TLB invalidate before every batch and a flush
afterwards, so this condition is already satisfied without extra flushes
around the LRI instructions.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 057239ab3f70..999f15455f48 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1431,13 +1431,8 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
 {
 	struct intel_engine_cs *engine = req->engine;
 	u32 *cs;
-	int ret;
 
 	/* NB: TLBs must be flushed and invalidated before a switch */
-	ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
-	if (ret)
-		return ret;
-
 	cs = intel_ring_begin(req, 6);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
@@ -1458,13 +1453,8 @@ static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 {
 	struct intel_engine_cs *engine = req->engine;
 	u32 *cs;
-	int ret;
 
 	/* NB: TLBs must be flushed and invalidated before a switch */
-	ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
-	if (ret)
-		return ret;
-
 	cs = intel_ring_begin(req, 6);
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
@@ -1477,13 +1467,6 @@ static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	*cs++ = MI_NOOP;
 	intel_ring_advance(req, cs);
 
-	/* XXX: RCS is the only one to auto invalidate the TLBs? */
-	if (engine->id != RCS) {
-		ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
-		if (ret)
-			return ret;
-	}
-
 	return 0;
 }
 
-- 
2.11.0

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

Re: [PATCH 2/5] drm/i915: Remove redundant TLB invalidate on switching ppgtt

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We are required to reload the TLBs around ppgtt switches. However, we
> already do an unconditional TLB invalidate before every batch and a flush
> afterwards, so this condition is already satisfied without extra flushes
> around the LRI instructions.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 -----------------
>  1 file changed, 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 057239ab3f70..999f15455f48 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1431,13 +1431,8 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
>  {
>  	struct intel_engine_cs *engine = req->engine;
>  	u32 *cs;
> -	int ret;
>  
>  	/* NB: TLBs must be flushed and invalidated before a switch */
> -	ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);

The pattern slightly changes as you dont get the flush on the first
switch.

But what is there to flush on first switch anyways.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -	if (ret)
> -		return ret;
> -
>  	cs = intel_ring_begin(req, 6);
>  	if (IS_ERR(cs))
>  		return PTR_ERR(cs);
> @@ -1458,13 +1453,8 @@ static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
>  {
>  	struct intel_engine_cs *engine = req->engine;
>  	u32 *cs;
> -	int ret;
>  
>  	/* NB: TLBs must be flushed and invalidated before a switch */
> -	ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
> -	if (ret)
> -		return ret;
> -
>  	cs = intel_ring_begin(req, 6);
>  	if (IS_ERR(cs))
>  		return PTR_ERR(cs);
> @@ -1477,13 +1467,6 @@ static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
>  	*cs++ = MI_NOOP;
>  	intel_ring_advance(req, cs);
>  
> -	/* XXX: RCS is the only one to auto invalidate the TLBs? */
> -	if (engine->id != RCS) {
> -		ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: Remove redundant TLB invalidate on switching ppgtt

[Chris Wilson]

On Fri, Feb 24, 2017 at 11:47:56AM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > We are required to reload the TLBs around ppgtt switches. However, we
> > already do an unconditional TLB invalidate before every batch and a flush
> > afterwards, so this condition is already satisfied without extra flushes
> > around the LRI instructions.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 -----------------
> >  1 file changed, 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 057239ab3f70..999f15455f48 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1431,13 +1431,8 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
> >  {
> >  	struct intel_engine_cs *engine = req->engine;
> >  	u32 *cs;
> > -	int ret;
> >  
> >  	/* NB: TLBs must be flushed and invalidated before a switch */
> > -	ret = engine->emit_flush(req, EMIT_INVALIDATE | EMIT_FLUSH);
> 
> The pattern slightly changes as you dont get the flush on the first
> switch.
> 
> But what is there to flush on first switch anyways.

Those flushes onto the empty ring have always left me questioning my
sanity ;)
-Chris

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

[PATCH 3/5] drm/i915: Reduce context alignment

[Chris Wilson]

No hardware was ever shipped that needed more than 4096 byte alignment
and future hardware will not use this legacy path. So reduce the
alignment to make it easier and quicker to launch workloads.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 17 -----------------
 drivers/gpu/drm/i915/i915_gem_context.h |  2 --
 drivers/gpu/drm/i915/intel_ringbuffer.c |  3 ++-
 3 files changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 521e6f4705b1..baceca14f5e0 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -92,21 +92,6 @@
 
 #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
 
-/* This is a HW constraint. The value below is the largest known requirement
- * I've seen in a spec to date, and that was a workaround for a non-shipping
- * part. It should be safe to decrease this, but it's more future proof as is.
- */
-#define GEN6_CONTEXT_ALIGN (64<<10)
-#define GEN7_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
-
-static size_t get_context_alignment(struct drm_i915_private *dev_priv)
-{
-	if (IS_GEN6(dev_priv))
-		return GEN6_CONTEXT_ALIGN;
-
-	return GEN7_CONTEXT_ALIGN;
-}
-
 static int get_context_size(struct drm_i915_private *dev_priv)
 {
 	int ret;
@@ -281,8 +266,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
 	list_add_tail(&ctx->link, &dev_priv->context_list);
 	ctx->i915 = dev_priv;
 
-	ctx->ggtt_alignment = get_context_alignment(dev_priv);
-
 	if (dev_priv->hw_context_size) {
 		struct drm_i915_gem_object *obj;
 		struct i915_vma *vma;
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 0ac750b90f3d..81268c9770a6 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -140,8 +140,6 @@ struct i915_gem_context {
 	 */
 	int priority;
 
-	/** ggtt_alignment: alignment restriction for context objects */
-	u32 ggtt_alignment;
 	/** ggtt_offset_bias: placement restriction for context objects */
 	u32 ggtt_offset_bias;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index f62afffef682..4a864f8c9387 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1431,7 +1431,8 @@ static int context_pin(struct i915_gem_context *ctx)
 			return ret;
 	}
 
-	return i915_vma_pin(vma, 0, ctx->ggtt_alignment, PIN_GLOBAL | PIN_HIGH);
+	return i915_vma_pin(vma, 0, I915_GTT_MIN_ALIGNMENT,
+			    PIN_GLOBAL | PIN_HIGH);
 }
 
 static int intel_ring_context_pin(struct intel_engine_cs *engine,
-- 
2.11.0

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

Re: [PATCH 3/5] drm/i915: Reduce context alignment

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> No hardware was ever shipped that needed more than 4096 byte alignment
> and future hardware will not use this legacy path. So reduce the
> alignment to make it easier and quicker to launch workloads.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 17 -----------------
>  drivers/gpu/drm/i915/i915_gem_context.h |  2 --
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  3 ++-
>  3 files changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 521e6f4705b1..baceca14f5e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -92,21 +92,6 @@
>  
>  #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1
>  
> -/* This is a HW constraint. The value below is the largest known requirement
> - * I've seen in a spec to date, and that was a workaround for a non-shipping
> - * part. It should be safe to decrease this, but it's more future proof as is.
> - */
> -#define GEN6_CONTEXT_ALIGN (64<<10)
> -#define GEN7_CONTEXT_ALIGN I915_GTT_MIN_ALIGNMENT
> -
> -static size_t get_context_alignment(struct drm_i915_private *dev_priv)
> -{
> -	if (IS_GEN6(dev_priv))
> -		return GEN6_CONTEXT_ALIGN;
> -
> -	return GEN7_CONTEXT_ALIGN;
> -}
> -
>  static int get_context_size(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
> @@ -281,8 +266,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
>  	list_add_tail(&ctx->link, &dev_priv->context_list);
>  	ctx->i915 = dev_priv;
>  
> -	ctx->ggtt_alignment = get_context_alignment(dev_priv);
> -
>  	if (dev_priv->hw_context_size) {
>  		struct drm_i915_gem_object *obj;
>  		struct i915_vma *vma;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> index 0ac750b90f3d..81268c9770a6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.h
> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> @@ -140,8 +140,6 @@ struct i915_gem_context {
>  	 */
>  	int priority;
>  
> -	/** ggtt_alignment: alignment restriction for context objects */
> -	u32 ggtt_alignment;
>  	/** ggtt_offset_bias: placement restriction for context objects */
>  	u32 ggtt_offset_bias;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index f62afffef682..4a864f8c9387 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1431,7 +1431,8 @@ static int context_pin(struct i915_gem_context *ctx)
>  			return ret;
>  	}
>  
> -	return i915_vma_pin(vma, 0, ctx->ggtt_alignment, PIN_GLOBAL | PIN_HIGH);
> +	return i915_vma_pin(vma, 0, I915_GTT_MIN_ALIGNMENT,
> +			    PIN_GLOBAL | PIN_HIGH);
>  }
>  
>  static int intel_ring_context_pin(struct intel_engine_cs *engine,
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Chris Wilson]

If we are setting the context and do inhibit the restoration from the
context image, also forgo applying the set-context w/a.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index baceca14f5e0..25e8db73542a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 }
 
 static inline int
-mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
+mi_set_context(struct drm_i915_gem_request *req, u32 flags)
 {
 	struct drm_i915_private *dev_priv = req->i915;
 	struct intel_engine_cs *engine = req->engine;
 	enum intel_engine_id id;
-	u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
 	const int num_rings =
 		/* Use an extended w/a on ivb+ if signalling from other rings */
 		i915.semaphores ?
 		INTEL_INFO(dev_priv)->num_rings - 1 :
 		0;
+	u32 *cs;
 	int len;
 
 	/* These flags are for resource streamer on HSW+ */
@@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
 	else if (INTEL_GEN(dev_priv) < 8)
 		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
-
+	flags |= MI_MM_SPACE_GTT;
 
 	len = 4;
-	if (INTEL_GEN(dev_priv) >= 7)
+	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))
 		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
 
 	cs = intel_ring_begin(req, len);
@@ -606,7 +606,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 		return PTR_ERR(cs);
 
 	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
-	if (INTEL_GEN(dev_priv) >= 7) {
+	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
 		*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
 		if (num_rings) {
 			struct intel_engine_cs *signaller;
@@ -633,7 +633,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
 	 */
 	*cs++ = MI_NOOP;
 
-	if (INTEL_GEN(dev_priv) >= 7) {
+	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
 		if (num_rings) {
 			struct intel_engine_cs *signaller;
 			i915_reg_t last_reg = {}; /* keep gcc quiet */
-- 
2.11.0

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

Re: [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> If we are setting the context and do inhibit the restoration from the
> context image, also forgo applying the set-context w/a.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index baceca14f5e0..25e8db73542a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  }
>  
>  static inline int
> -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
>  {
>  	struct drm_i915_private *dev_priv = req->i915;
>  	struct intel_engine_cs *engine = req->engine;
>  	enum intel_engine_id id;
> -	u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
>  	const int num_rings =
>  		/* Use an extended w/a on ivb+ if signalling from other rings */
>  		i915.semaphores ?
>  		INTEL_INFO(dev_priv)->num_rings - 1 :
>  		0;
> +	u32 *cs;
>  	int len;
>  
>  	/* These flags are for resource streamer on HSW+ */
> @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
>  	else if (INTEL_GEN(dev_priv) < 8)
>  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> -
> +	flags |= MI_MM_SPACE_GTT;
>  
>  	len = 4;
> -	if (INTEL_GEN(dev_priv) >= 7)
> +	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))

If we are restoring from the context image, we dont need the
workarounds. So should this be 

if (INTEL_GEN(dev_priv) >= 7 && flags & MI_RESTORE_INHIBIT)

?

-Mika

>  		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
>  
>  	cs = intel_ring_begin(req, len);
> @@ -606,7 +606,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  		return PTR_ERR(cs);
>  
>  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> -	if (INTEL_GEN(dev_priv) >= 7) {
> +	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
>  		if (num_rings) {
>  			struct intel_engine_cs *signaller;
> @@ -633,7 +633,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  	 */
>  	*cs++ = MI_NOOP;
>  
> -	if (INTEL_GEN(dev_priv) >= 7) {
> +	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT)) {
>  		if (num_rings) {
>  			struct intel_engine_cs *signaller;
>  			i915_reg_t last_reg = {}; /* keep gcc quiet */
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Chris Wilson]

On Fri, Feb 24, 2017 at 12:45:54PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If we are setting the context and do inhibit the restoration from the
> > context image, also forgo applying the set-context w/a.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index baceca14f5e0..25e8db73542a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  }
> >  
> >  static inline int
> > -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> > +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
> >  {
> >  	struct drm_i915_private *dev_priv = req->i915;
> >  	struct intel_engine_cs *engine = req->engine;
> >  	enum intel_engine_id id;
> > -	u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
> >  	const int num_rings =
> >  		/* Use an extended w/a on ivb+ if signalling from other rings */
> >  		i915.semaphores ?
> >  		INTEL_INFO(dev_priv)->num_rings - 1 :
> >  		0;
> > +	u32 *cs;
> >  	int len;
> >  
> >  	/* These flags are for resource streamer on HSW+ */
> > @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> >  		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
> >  	else if (INTEL_GEN(dev_priv) < 8)
> >  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > -
> > +	flags |= MI_MM_SPACE_GTT;
> >  
> >  	len = 4;
> > -	if (INTEL_GEN(dev_priv) >= 7)
> > +	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))
> 
> If we are restoring from the context image, we dont need the
> workarounds. So should this be 
> 
> if (INTEL_GEN(dev_priv) >= 7 && flags & MI_RESTORE_INHIBIT)

If we are restoring, we need the w/a. Afaik, it's the execution of the
context image itself that's troublesome.
-Chris

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

Re: [PATCH 4/5] drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT

[Chris Wilson]

On Fri, Feb 24, 2017 at 12:45:54PM +0200, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > If we are setting the context and do inhibit the restoration from the
> > context image, also forgo applying the set-context w/a.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index baceca14f5e0..25e8db73542a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -577,17 +577,17 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
> >  }
> >  
> >  static inline int
> > -mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> > +mi_set_context(struct drm_i915_gem_request *req, u32 flags)
> >  {
> >  	struct drm_i915_private *dev_priv = req->i915;
> >  	struct intel_engine_cs *engine = req->engine;
> >  	enum intel_engine_id id;
> > -	u32 *cs, flags = hw_flags | MI_MM_SPACE_GTT;
> >  	const int num_rings =
> >  		/* Use an extended w/a on ivb+ if signalling from other rings */
> >  		i915.semaphores ?
> >  		INTEL_INFO(dev_priv)->num_rings - 1 :
> >  		0;
> > +	u32 *cs;
> >  	int len;
> >  
> >  	/* These flags are for resource streamer on HSW+ */
> > @@ -595,10 +595,10 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> >  		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
> >  	else if (INTEL_GEN(dev_priv) < 8)
> >  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > -
> > +	flags |= MI_MM_SPACE_GTT;
> >  
> >  	len = 4;
> > -	if (INTEL_GEN(dev_priv) >= 7)
> > +	if (INTEL_GEN(dev_priv) >= 7 && !(flags & MI_RESTORE_INHIBIT))
> 
> If we are restoring from the context image, we dont need the
> workarounds. So should this be 

I merged up to the previous patch as we ran into an uncertainity as to
whether some of the w/a applied to restore or to save.

The next patch is still interesting, but merits some soak testing to see
if cures the last full-ppgtt corruption on hsw.
-Chris

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

[PATCH 5/5] drm/i915: Suppress switch_mm emission between the same aliasing_ppgtt

[Chris Wilson]

When switching between contexts using the aliasing_ppgtt, the VM is
shared. We don't need to reload the PD registers unless they are dirty.

Martin Peres reported an issue that looks like corruption between
Haswell context switches, bisecting to commit f9326be5f1d3 ("drm/i915:
Rearrange switch_context to load the aliasing ppgtt on first use").
Switching between the same mm (the aliasing_ppgtt is used for all
contexts in this case) should be a nop, but appears to trigger some
side-effects in the context switch. However, as we know the switch
is redundant in this case, we can skip it and continue to ignore the
issue until somebody feels strong enough to investigate full-ppgtt on
gen7 again!

Except.. Martin was using full-ppgtt which is not supported as it
doesn't work correctly yet. So whilst the bisect did yield valuable
information about the failures, the fix should not have any user impact
under default settings.

Fixes: f9326be5f1d3 ("drm/i915: Rearrange switch_context to load the aliasing ppgtt on first use")
Reported-by: Martin Peres <martin.peres@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Martin Peres <martin.peres@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 25e8db73542a..41d4fa569bcf 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -708,10 +708,10 @@ static inline bool skip_rcs_switch(struct i915_hw_ppgtt *ppgtt,
 }
 
 static bool
-needs_pd_load_pre(struct i915_hw_ppgtt *ppgtt,
-		  struct intel_engine_cs *engine,
-		  struct i915_gem_context *to)
+needs_pd_load_pre(struct i915_hw_ppgtt *ppgtt, struct intel_engine_cs *engine)
 {
+	struct i915_hw_ppgtt *last_ppgtt;
+
 	if (!ppgtt)
 		return false;
 
@@ -720,7 +720,9 @@ needs_pd_load_pre(struct i915_hw_ppgtt *ppgtt,
 		return true;
 
 	/* Same context without new entries, skip */
-	if (engine->legacy_active_context == to &&
+	last_ppgtt =
+		engine->legacy_active_context->ppgtt ?: engine->i915->mm.aliasing_ppgtt;
+	if (last_ppgtt == ppgtt &&
 	    !(intel_engine_flag(engine) & ppgtt->pd_dirty_rings))
 		return false;
 
@@ -764,7 +766,7 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
 	if (skip_rcs_switch(ppgtt, engine, to))
 		return 0;
 
-	if (needs_pd_load_pre(ppgtt, engine, to)) {
+	if (needs_pd_load_pre(ppgtt, engine)) {
 		/* Older GENs and non render rings still want the load first,
 		 * "PP_DCLV followed by PP_DIR_BASE register through Load
 		 * Register Immediate commands in Ring Buffer before submitting
@@ -861,7 +863,7 @@ int i915_switch_context(struct drm_i915_gem_request *req)
 		struct i915_hw_ppgtt *ppgtt =
 			to->ppgtt ?: req->i915->mm.aliasing_ppgtt;
 
-		if (needs_pd_load_pre(ppgtt, engine, to)) {
+		if (needs_pd_load_pre(ppgtt, engine)) {
 			int ret;
 
 			trace_switch_mm(engine, to);
-- 
2.11.0

_______________________________________________
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/5] drm/i915: Remove redundant TLB invalidate on switching contexts

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Remove redundant TLB invalidate on switching contexts
URL   : https://patchwork.freedesktop.org/series/20155/
State : success

== Summary ==

Series 20155v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20155/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:108  pass:95   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29 

53a95c930ad728b11cc2b21e42b4bd9dcd306400 drm-tip: 2017y-02m-23d-16h-19m-22s UTC integration manifest
971af51 drm/i915: Suppress switch_mm emission between the same aliasing_ppgtt
5df0efd drm/i915: Suppress context restore w/a when using MI_RESTORE_INHIBIT
44910de drm/i915: Reduce context alignment
914fc5a drm/i915: Remove redundant TLB invalidate on switching ppgtt
4120c91 drm/i915: Remove redundant TLB invalidate on switching contexts

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3950/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Remove redundant TLB invalidate on switching contexts

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> We are required to reload the TLBs around context switches
> (MI_SET_CONTEXT specifically) and the recommendation is do that before
> the MI_SET_CONTEXT so that it is serialised with the switch and not
> forgotten:
>
> [DevSNB] If Flush TLB invalidation Mode is enabled it’s the driver’s
> responsibility to invalidate the TLBs at least once after the previous
> context switch after any GTT mappings changed (including new GTT entries).
> This can be done by a pipeline PIPE_CONTROL with TLB inv bit set
> immediately before MI_SET_CONTEXT.
>
> However, we already do an unconditional TLB invalidate before every
> batch so this condition is satifisfied.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 99c46f4dbde6..521e6f4705b1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -607,17 +607,6 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  		0;
>  	int len;
>  
> -	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
> -	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
> -	 * explicitly, so we rely on the value at ring init, stored in
> -	 * itlb_before_ctx_switch.
> -	 */
> -	if (IS_GEN6(dev_priv)) {
> -		int ret = engine->emit_flush(req, EMIT_INVALIDATE);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	/* These flags are for resource streamer on HSW+ */
>  	if (IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8)
>  		flags |= (HSW_MI_RS_SAVE_STATE_EN | HSW_MI_RS_RESTORE_STATE_EN);
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx