[PATCH 1/3] drm/i915: Invalidate media caches on gen7

[PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Chris Wilson]

In the gen7 pipe control there is an extra bit to flush the media
caches, so let's set it during cache invalidation flushes.

Cc: Simon Farnsworth <simon@farnz.org.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_reg.h         | 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 93fdad8a7447..0ddef7256d02 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -400,6 +400,7 @@
 #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
 #define   PIPE_CONTROL_CS_STALL				(1<<20)
 #define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
+#define   PIPE_CONTROL_MEDIA_CACHE_INVALIDATE		(1<<16)
 #define   PIPE_CONTROL_QW_WRITE				(1<<14)
 #define   PIPE_CONTROL_POST_SYNC_OP_MASK                (3<<14)
 #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4702e7bcd71c..282279b83ca4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -373,6 +373,7 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
 		flags |= PIPE_CONTROL_VF_CACHE_INVALIDATE;
 		flags |= PIPE_CONTROL_CONST_CACHE_INVALIDATE;
 		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
+		flags |= PIPE_CONTROL_MEDIA_CACHE_INVALIDATE;
 		/*
 		 * TLB invalidate requires a post-sync write.
 		 */
-- 
2.1.3

[PATCH 2/3] drm/i915: Force the CS stall for invalidate flushes

[Chris Wilson]

In order to act as a full command barrier by itself, we need to tell the
pipecontrol to actually stall the command streamer while the flush runs.
We require the full command barrier before operations like
MI_SET_CONTEXT, which currently rely on a prior invalidate flush.

References: https://bugs.freedesktop.org/show_bug.cgi?id=83677
Cc: Simon Farnsworth <simon@farnz.org.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 282279b83ca4..02fb478a2867 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -380,6 +380,8 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
 		flags |= PIPE_CONTROL_QW_WRITE;
 		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
 
+		flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
+
 		/* Workaround: we must issue a pipe_control with CS-stall bit
 		 * set before a pipe_control command that has the state cache
 		 * invalidate bit set. */
-- 
2.1.3

[PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Chris Wilson]

There exists a current workaround to prevent a hang on context switch
should the ring go to sleep in the middle of the restore,
WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
spite of disabling arbitration (which prevents the ring from powering
down during the critical section) we were still hitting hangs that had
the hallmarks of the known erratum. That is we are still seeing hangs
"on the last instruction in the context restore". By comparing -nightly
(broken) with requests (working), we were able to deduce that it was the
semaphore LRI cross-talk that reproduced the original failure.
Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
the rings had to be awake to prevent the hangs. Fortunately, we can
reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
should be able to limit the extra power implications.

Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
products, we should apply this extra hammer for all of the same
platforms despite so far that we have only been able to reproduce the
hang on certain ivb and hsw models. The last question is whether we want
to always use the extra hammer or only when we know semaphores are in
operation. At the moment, we only use LRI on non-RCS rings for
semaphores, but that may change in the future with the possibility of
reintroducing this bug under subtle conditions.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
Cc: Simon Farnsworth <simon@farnz.org.uk>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_context.c | 39 ++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_reg.h         |  2 ++
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2acf5803cf32..724ccecea06a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -484,7 +484,9 @@ mi_set_context(struct intel_engine_cs *ring,
 	       u32 hw_flags)
 {
 	u32 flags = hw_flags | MI_MM_SPACE_GTT;
-	int ret;
+	struct intel_engine_cs *engine;
+	const int num_rings = i915_semaphore_is_enabled(ring->dev) ? hweight32(INTEL_INFO(ring->dev)->ring_mask) : 0;
+	int len, i, ret;
 
 	/* 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
@@ -501,15 +503,26 @@ mi_set_context(struct intel_engine_cs *ring,
 	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
 		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
 
-	ret = intel_ring_begin(ring, 6);
+
+	len = 4;
+	if (INTEL_INFO(ring->dev)->gen >= 7)
+		len += 4*num_rings + 2;
+
+	ret = intel_ring_begin(ring, len);
 	if (ret)
 		return ret;
 
 	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
-	if (INTEL_INFO(ring->dev)->gen >= 7)
-		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
-	else
-		intel_ring_emit(ring, MI_NOOP);
+	if (INTEL_INFO(ring->dev)->gen >= 7) {
+		if (num_rings) {
+			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
+			for_each_ring(engine, to_i915(ring->dev), i) {
+				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
+				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
+			}
+		} else
+			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
+	}
 
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
@@ -521,10 +534,16 @@ mi_set_context(struct intel_engine_cs *ring,
 	 */
 	intel_ring_emit(ring, MI_NOOP);
 
-	if (INTEL_INFO(ring->dev)->gen >= 7)
-		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
-	else
-		intel_ring_emit(ring, MI_NOOP);
+	if (INTEL_INFO(ring->dev)->gen >= 7) {
+		if (num_rings) {
+			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
+			for_each_ring(engine, to_i915(ring->dev), i) {
+				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
+				intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
+			}
+		} else
+			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
+	}
 
 	intel_ring_advance(ring);
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0ddef7256d02..1ce303cb8852 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1134,6 +1134,7 @@ enum punit_power_well {
 #define GEN6_VERSYNC	(RING_SYNC_1(VEBOX_RING_BASE))
 #define GEN6_VEVSYNC	(RING_SYNC_2(VEBOX_RING_BASE))
 #define GEN6_NOSYNC 0
+#define RING_PSMI_CTL(base)	((base)+0x50)
 #define RING_MAX_IDLE(base)	((base)+0x54)
 #define RING_HWS_PGA(base)	((base)+0x80)
 #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
@@ -1464,6 +1465,7 @@ enum punit_power_well {
 #define   GEN6_BLITTER_FBC_NOTIFY			(1<<3)
 
 #define GEN6_RC_SLEEP_PSMI_CONTROL	0x2050
+#define   GEN6_PSMI_SLEEP_MSG_DISABLE	(1 << 0)
 #define   GEN8_RC_SEMA_IDLE_MSG_DISABLE	(1 << 12)
 #define   GEN8_FF_DOP_CLOCK_GATE_DISABLE	(1<<10)
 
-- 
2.1.3

Re: [PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Ville Syrjälä]

On Thu, Dec 11, 2014 at 08:16:59AM +0000, Chris Wilson wrote:
> In the gen7 pipe control there is an extra bit to flush the media
> caches, so let's set it during cache invalidation flushes.

Bspec is telling me this bit is already present in snb, and calls it
'Generic Media State Clear'. Older Bspec seems to suggest we should set
it here, and maybe that we should also issue another PIPE_CONTROL with
the bit set after MI_SET_CONTEXT when switching from media to 3D context.
These notes don't seem to be present in the current BSpec, so I'm not
sure what the deal is.

> 
> Cc: Simon Farnsworth <simon@farnz.org.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 93fdad8a7447..0ddef7256d02 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -400,6 +400,7 @@
>  #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
>  #define   PIPE_CONTROL_CS_STALL				(1<<20)
>  #define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
> +#define   PIPE_CONTROL_MEDIA_CACHE_INVALIDATE		(1<<16)
>  #define   PIPE_CONTROL_QW_WRITE				(1<<14)
>  #define   PIPE_CONTROL_POST_SYNC_OP_MASK                (3<<14)
>  #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4702e7bcd71c..282279b83ca4 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -373,6 +373,7 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
>  		flags |= PIPE_CONTROL_VF_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_CONST_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
> +		flags |= PIPE_CONTROL_MEDIA_CACHE_INVALIDATE;
>  		/*
>  		 * TLB invalidate requires a post-sync write.
>  		 */
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1                 364/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(7, M26)PASS(22, M26M37)      PASS(1, M37)
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: [Intel-gfx] [PATCH 2/3] drm/i915: Force the CS stall for invalidate flushes

[Ville Syrjälä]

On Thu, Dec 11, 2014 at 08:17:00AM +0000, Chris Wilson wrote:
> In order to act as a full command barrier by itself, we need to tell the
> pipecontrol to actually stall the command streamer while the flush runs.
> We require the full command barrier before operations like
> MI_SET_CONTEXT, which currently rely on a prior invalidate flush.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> Cc: Simon Farnsworth <simon@farnz.org.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 282279b83ca4..02fb478a2867 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -380,6 +380,8 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
>  		flags |= PIPE_CONTROL_QW_WRITE;
>  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
>  
> +		flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
> +

Hmm. BSpec says that the render cache won't be flushed when this bit
is set. Is that going to cause problems for the gpu_cache_dirty cases
where seem to do invalidate+flush with a single PIPE_CONTROL?

>  		/* Workaround: we must issue a pipe_control with CS-stall bit
>  		 * set before a pipe_control command that has the state cache
>  		 * invalidate bit set. */
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Force the CS stall for invalidate flushes

[Chris Wilson]

On Fri, Dec 12, 2014 at 11:09:15AM +0200, Ville Syrjälä wrote:
> On Thu, Dec 11, 2014 at 08:17:00AM +0000, Chris Wilson wrote:
> > In order to act as a full command barrier by itself, we need to tell the
> > pipecontrol to actually stall the command streamer while the flush runs.
> > We require the full command barrier before operations like
> > MI_SET_CONTEXT, which currently rely on a prior invalidate flush.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 282279b83ca4..02fb478a2867 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -380,6 +380,8 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
> >  		flags |= PIPE_CONTROL_QW_WRITE;
> >  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> >  
> > +		flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
> > +
> 
> Hmm. BSpec says that the render cache won't be flushed when this bit
> is set. Is that going to cause problems for the gpu_cache_dirty cases
> where seem to do invalidate+flush with a single PIPE_CONTROL?

I thought it was DEPTH_STALL that disabled the write flush. It is
redundant in the case where the write flush is taking place though and
you can do:

/* bspec is not entirely clear when the render target cache flush is
 * disabled with other stall bits set, so don't set any additional
 * stalls if we are already using the cache flush.
 */
if ((flags & PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH) == 0)
   flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Chris Wilson]

On Thu, Dec 11, 2014 at 02:24:53PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 11, 2014 at 08:16:59AM +0000, Chris Wilson wrote:
> > In the gen7 pipe control there is an extra bit to flush the media
> > caches, so let's set it during cache invalidation flushes.
> 
> Bspec is telling me this bit is already present in snb, and calls it
> 'Generic Media State Clear'. Older Bspec seems to suggest we should set
> it here, and maybe that we should also issue another PIPE_CONTROL with
> the bit set after MI_SET_CONTEXT when switching from media to 3D context.
> These notes don't seem to be present in the current BSpec, so I'm not
> sure what the deal is.

In an alternative universe, I do the barrier + context switch first, invalidate
later. However, there is also a benefit to doing the invalidate before
context switch as bspec talks about not reloading invalidated indirect
state. Not sure if that ever applies to us though.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Force the CS stall for invalidate flushes

[Ville Syrjälä]

On Fri, Dec 12, 2014 at 09:20:49AM +0000, Chris Wilson wrote:
> On Fri, Dec 12, 2014 at 11:09:15AM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 11, 2014 at 08:17:00AM +0000, Chris Wilson wrote:
> > > In order to act as a full command barrier by itself, we need to tell the
> > > pipecontrol to actually stall the command streamer while the flush runs.
> > > We require the full command barrier before operations like
> > > MI_SET_CONTEXT, which currently rely on a prior invalidate flush.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > index 282279b83ca4..02fb478a2867 100644
> > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > @@ -380,6 +380,8 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
> > >  		flags |= PIPE_CONTROL_QW_WRITE;
> > >  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> > >  
> > > +		flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
> > > +
> > 
> > Hmm. BSpec says that the render cache won't be flushed when this bit
> > is set. Is that going to cause problems for the gpu_cache_dirty cases
> > where seem to do invalidate+flush with a single PIPE_CONTROL?
> 
> I thought it was DEPTH_STALL that disabled the write flush.

Hmm. Yeah, the previous sentence talks about the depth stall bit. So I
suppose it could still be referring to the depth stall bit when it says
the render cache flush won't be flushed.

> It is
> redundant in the case where the write flush is taking place though and
> you can do:
> 
> /* bspec is not entirely clear when the render target cache flush is
>  * disabled with other stall bits set, so don't set any additional
>  * stalls if we are already using the cache flush.
>  */
> if ((flags & PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH) == 0)
>    flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Force the CS stall for invalidate flushes

[Daniel Vetter]

On Fri, Dec 12, 2014 at 09:20:49AM +0000, Chris Wilson wrote:
> On Fri, Dec 12, 2014 at 11:09:15AM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 11, 2014 at 08:17:00AM +0000, Chris Wilson wrote:
> > > In order to act as a full command barrier by itself, we need to tell the
> > > pipecontrol to actually stall the command streamer while the flush runs.
> > > We require the full command barrier before operations like
> > > MI_SET_CONTEXT, which currently rely on a prior invalidate flush.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > index 282279b83ca4..02fb478a2867 100644
> > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > @@ -380,6 +380,8 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
> > >  		flags |= PIPE_CONTROL_QW_WRITE;
> > >  		flags |= PIPE_CONTROL_GLOBAL_GTT_IVB;
> > >  
> > > +		flags |= PIPE_CONTROL_STALL_AT_SCOREBOARD;
> > > +
> > 
> > Hmm. BSpec says that the render cache won't be flushed when this bit
> > is set. Is that going to cause problems for the gpu_cache_dirty cases
> > where seem to do invalidate+flush with a single PIPE_CONTROL?
> 
> I thought it was DEPTH_STALL that disabled the write flush. It is
> redundant in the case where the write flush is taking place though and
> you can do:

My bspec here concures that only the depth stall affects flushes.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Daniel Vetter]

On Thu, Dec 11, 2014 at 08:16:59AM +0000, Chris Wilson wrote:
> In the gen7 pipe control there is an extra bit to flush the media
> caches, so let's set it during cache invalidation flushes.
> 
> Cc: Simon Farnsworth <simon@farnz.org.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/i915/i915_reg.h         | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 93fdad8a7447..0ddef7256d02 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -400,6 +400,7 @@
>  #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
>  #define   PIPE_CONTROL_CS_STALL				(1<<20)
>  #define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
> +#define   PIPE_CONTROL_MEDIA_CACHE_INVALIDATE		(1<<16)

Maybe call it STATE_CLEAR since that's what Bspec calls it. libva doesn't
use this, so I have no idea what it means. I also wonder whether we should
nuke indirect state (bit9), too? Just to make sure the context switch
doesn't fall over between 3d and media pipeline.

Anyway if it helps it must be good. With the rename applied:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  #define   PIPE_CONTROL_QW_WRITE				(1<<14)
>  #define   PIPE_CONTROL_POST_SYNC_OP_MASK                (3<<14)
>  #define   PIPE_CONTROL_DEPTH_STALL			(1<<13)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 4702e7bcd71c..282279b83ca4 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -373,6 +373,7 @@ gen7_render_ring_flush(struct intel_engine_cs *ring,
>  		flags |= PIPE_CONTROL_VF_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_CONST_CACHE_INVALIDATE;
>  		flags |= PIPE_CONTROL_STATE_CACHE_INVALIDATE;
> +		flags |= PIPE_CONTROL_MEDIA_CACHE_INVALIDATE;
>  		/*
>  		 * TLB invalidate requires a post-sync write.
>  		 */
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Daniel Vetter]

On Thu, Dec 11, 2014 at 08:17:01AM +0000, Chris Wilson wrote:
> There exists a current workaround to prevent a hang on context switch
> should the ring go to sleep in the middle of the restore,
> WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> spite of disabling arbitration (which prevents the ring from powering
> down during the critical section) we were still hitting hangs that had
> the hallmarks of the known erratum. That is we are still seeing hangs
> "on the last instruction in the context restore". By comparing -nightly
> (broken) with requests (working), we were able to deduce that it was the
> semaphore LRI cross-talk that reproduced the original failure.
> Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
> the rings had to be awake to prevent the hangs. Fortunately, we can
> reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
> should be able to limit the extra power implications.
> 
> Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> products, we should apply this extra hammer for all of the same
> platforms despite so far that we have only been able to reproduce the
> hang on certain ivb and hsw models. The last question is whether we want
> to always use the extra hammer or only when we know semaphores are in
> operation. At the moment, we only use LRI on non-RCS rings for
> semaphores, but that may change in the future with the possibility of
> reintroducing this bug under subtle conditions.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> Cc: Simon Farnsworth <simon@farnz.org.uk>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org

Oh dear, but awesome piece of debugging!

First question: How does this work on vlv with the split forcewake domain?
Any bad side effects like longer ctx switch times?

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 39 ++++++++++++++++++++++++---------
>  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 2acf5803cf32..724ccecea06a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -484,7 +484,9 @@ mi_set_context(struct intel_engine_cs *ring,
>  	       u32 hw_flags)
>  {
>  	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> -	int ret;
> +	struct intel_engine_cs *engine;
> +	const int num_rings = i915_semaphore_is_enabled(ring->dev) ? hweight32(INTEL_INFO(ring->dev)->ring_mask) : 0;
> +	int len, i, ret;
>  
>  	/* 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
> @@ -501,15 +503,26 @@ mi_set_context(struct intel_engine_cs *ring,
>  	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
>  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
>  
> -	ret = intel_ring_begin(ring, 6);
> +
> +	len = 4;
> +	if (INTEL_INFO(ring->dev)->gen >= 7)
> +		len += 4*num_rings + 2;
> +
> +	ret = intel_ring_begin(ring, len);
>  	if (ret)
>  		return ret;
>  
>  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> -	if (INTEL_INFO(ring->dev)->gen >= 7)
> -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> -	else
> -		intel_ring_emit(ring, MI_NOOP);
> +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> +		if (num_rings) {
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> +			for_each_ring(engine, to_i915(ring->dev), i) {
> +				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
> +				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +			}
> +		} else
> +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);

For added paranoia, shouldn't we keep this one here? Or is the only thing
this does frob the idle notifications like PSMI? I kinda prefer we just
keep it if there's no harmful effects ...

Otherwise lgtm.
-Daniel

> +	}
>  
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
> @@ -521,10 +534,16 @@ mi_set_context(struct intel_engine_cs *ring,
>  	 */
>  	intel_ring_emit(ring, MI_NOOP);
>  
> -	if (INTEL_INFO(ring->dev)->gen >= 7)
> -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
> -	else
> -		intel_ring_emit(ring, MI_NOOP);
> +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> +		if (num_rings) {
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> +			for_each_ring(engine, to_i915(ring->dev), i) {
> +				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
> +				intel_ring_emit(ring, _MASKED_BIT_DISABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> +			}
> +		} else
> +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_ENABLE);
> +	}
>  
>  	intel_ring_advance(ring);
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0ddef7256d02..1ce303cb8852 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1134,6 +1134,7 @@ enum punit_power_well {
>  #define GEN6_VERSYNC	(RING_SYNC_1(VEBOX_RING_BASE))
>  #define GEN6_VEVSYNC	(RING_SYNC_2(VEBOX_RING_BASE))
>  #define GEN6_NOSYNC 0
> +#define RING_PSMI_CTL(base)	((base)+0x50)
>  #define RING_MAX_IDLE(base)	((base)+0x54)
>  #define RING_HWS_PGA(base)	((base)+0x80)
>  #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
> @@ -1464,6 +1465,7 @@ enum punit_power_well {
>  #define   GEN6_BLITTER_FBC_NOTIFY			(1<<3)
>  
>  #define GEN6_RC_SLEEP_PSMI_CONTROL	0x2050
> +#define   GEN6_PSMI_SLEEP_MSG_DISABLE	(1 << 0)
>  #define   GEN8_RC_SEMA_IDLE_MSG_DISABLE	(1 << 12)
>  #define   GEN8_FF_DOP_CLOCK_GATE_DISABLE	(1<<10)
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Chris Wilson]

On Mon, Dec 15, 2014 at 10:41:52AM +0100, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 08:17:01AM +0000, Chris Wilson wrote:
> > There exists a current workaround to prevent a hang on context switch
> > should the ring go to sleep in the middle of the restore,
> > WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> > spite of disabling arbitration (which prevents the ring from powering
> > down during the critical section) we were still hitting hangs that had
> > the hallmarks of the known erratum. That is we are still seeing hangs
> > "on the last instruction in the context restore". By comparing -nightly
> > (broken) with requests (working), we were able to deduce that it was the
> > semaphore LRI cross-talk that reproduced the original failure.
> > Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
> > the rings had to be awake to prevent the hangs. Fortunately, we can
> > reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
> > should be able to limit the extra power implications.
> > 
> > Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> > products, we should apply this extra hammer for all of the same
> > platforms despite so far that we have only been able to reproduce the
> > hang on certain ivb and hsw models. The last question is whether we want
> > to always use the extra hammer or only when we know semaphores are in
> > operation. At the moment, we only use LRI on non-RCS rings for
> > semaphores, but that may change in the future with the possibility of
> > reintroducing this bug under subtle conditions.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> 
> Oh dear, but awesome piece of debugging!
> 
> First question: How does this work on vlv with the split forcewake domain?
> Any bad side effects like longer ctx switch times?

It will work on vlv as well since the split power domains do not matter
here: the LRI is on an active ring, and the result of the LRI is to
temporary wake up the other domain.
The negative impact is that we do wake up other power domains and rings
during the context switch, a small but definite power increase.

> > ---
> >  drivers/gpu/drm/i915/i915_gem_context.c | 39 ++++++++++++++++++++++++---------
> >  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
> >  2 files changed, 31 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 2acf5803cf32..724ccecea06a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -484,7 +484,9 @@ mi_set_context(struct intel_engine_cs *ring,
> >  	       u32 hw_flags)
> >  {
> >  	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> > -	int ret;
> > +	struct intel_engine_cs *engine;
> > +	const int num_rings = i915_semaphore_is_enabled(ring->dev) ? hweight32(INTEL_INFO(ring->dev)->ring_mask) : 0;
> > +	int len, i, ret;
> >  
> >  	/* 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
> > @@ -501,15 +503,26 @@ mi_set_context(struct intel_engine_cs *ring,
> >  	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> >  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> >  
> > -	ret = intel_ring_begin(ring, 6);
> > +
> > +	len = 4;
> > +	if (INTEL_INFO(ring->dev)->gen >= 7)
> > +		len += 4*num_rings + 2;
> > +
> > +	ret = intel_ring_begin(ring, len);
> >  	if (ret)
> >  		return ret;
> >  
> >  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> > -	if (INTEL_INFO(ring->dev)->gen >= 7)
> > -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > -	else
> > -		intel_ring_emit(ring, MI_NOOP);
> > +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> > +		if (num_rings) {
> > +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> > +			for_each_ring(engine, to_i915(ring->dev), i) {
> > +				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
> > +				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> > +			}
> > +		} else
> > +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> 
> For added paranoia, shouldn't we keep this one here? Or is the only thing
> this does frob the idle notifications like PSMI? I kinda prefer we just
> keep it if there's no harmful effects ...

Yeah, MI_ARB_ON_OFF is just a magic command used to do the same as the
PSMI twiddling on the local ring.

We can do MI_ARB_ON_OFF + LRI(!RCS) which is slightly shorter
instruction wise, closer to the original w/a and looks slightly neater.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Ville Syrjälä]

On Mon, Dec 15, 2014 at 04:24:48PM +0000, Chris Wilson wrote:
> On Mon, Dec 15, 2014 at 10:41:52AM +0100, Daniel Vetter wrote:
> > On Thu, Dec 11, 2014 at 08:17:01AM +0000, Chris Wilson wrote:
> > > There exists a current workaround to prevent a hang on context switch
> > > should the ring go to sleep in the middle of the restore,
> > > WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> > > spite of disabling arbitration (which prevents the ring from powering
> > > down during the critical section) we were still hitting hangs that had
> > > the hallmarks of the known erratum. That is we are still seeing hangs
> > > "on the last instruction in the context restore". By comparing -nightly
> > > (broken) with requests (working), we were able to deduce that it was the
> > > semaphore LRI cross-talk that reproduced the original failure.
> > > Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
> > > the rings had to be awake to prevent the hangs. Fortunately, we can
> > > reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
> > > should be able to limit the extra power implications.
> > > 
> > > Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> > > products, we should apply this extra hammer for all of the same
> > > platforms despite so far that we have only been able to reproduce the
> > > hang on certain ivb and hsw models. The last question is whether we want
> > > to always use the extra hammer or only when we know semaphores are in
> > > operation. At the moment, we only use LRI on non-RCS rings for
> > > semaphores, but that may change in the future with the possibility of
> > > reintroducing this bug under subtle conditions.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: stable@vger.kernel.org
> > 
> > Oh dear, but awesome piece of debugging!
> > 
> > First question: How does this work on vlv with the split forcewake domain?
> > Any bad side effects like longer ctx switch times?
> 
> It will work on vlv as well since the split power domains do not matter
> here: the LRI is on an active ring, and the result of the LRI is to
> temporary wake up the other domain.
> The negative impact is that we do wake up other power domains and rings
> during the context switch, a small but definite power increase.

I don't think there's any point in worrying about that for context
switches as long as we still have the "semaphore LRI wakes up all
power wells for every single batch" issue around.

> 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_context.c | 39 ++++++++++++++++++++++++---------
> > >  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
> > >  2 files changed, 31 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > index 2acf5803cf32..724ccecea06a 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > @@ -484,7 +484,9 @@ mi_set_context(struct intel_engine_cs *ring,
> > >  	       u32 hw_flags)
> > >  {
> > >  	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> > > -	int ret;
> > > +	struct intel_engine_cs *engine;
> > > +	const int num_rings = i915_semaphore_is_enabled(ring->dev) ? hweight32(INTEL_INFO(ring->dev)->ring_mask) : 0;
> > > +	int len, i, ret;
> > >  
> > >  	/* 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
> > > @@ -501,15 +503,26 @@ mi_set_context(struct intel_engine_cs *ring,
> > >  	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > >  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > >  
> > > -	ret = intel_ring_begin(ring, 6);
> > > +
> > > +	len = 4;
> > > +	if (INTEL_INFO(ring->dev)->gen >= 7)
> > > +		len += 4*num_rings + 2;
> > > +
> > > +	ret = intel_ring_begin(ring, len);
> > >  	if (ret)
> > >  		return ret;
> > >  
> > >  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> > > -	if (INTEL_INFO(ring->dev)->gen >= 7)
> > > -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > > -	else
> > > -		intel_ring_emit(ring, MI_NOOP);
> > > +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> > > +		if (num_rings) {
> > > +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> > > +			for_each_ring(engine, to_i915(ring->dev), i) {
> > > +				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
> > > +				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> > > +			}
> > > +		} else
> > > +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > 
> > For added paranoia, shouldn't we keep this one here? Or is the only thing
> > this does frob the idle notifications like PSMI? I kinda prefer we just
> > keep it if there's no harmful effects ...
> 
> Yeah, MI_ARB_ON_OFF is just a magic command used to do the same as the
> PSMI twiddling on the local ring.
> 
> We can do MI_ARB_ON_OFF + LRI(!RCS) which is slightly shorter
> instruction wise, closer to the original w/a and looks slightly neater.
> -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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Chris Wilson]

On Mon, Dec 15, 2014 at 10:34:45AM +0100, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 08:16:59AM +0000, Chris Wilson wrote:
> > In the gen7 pipe control there is an extra bit to flush the media
> > caches, so let's set it during cache invalidation flushes.
> > 
> > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         | 1 +
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
> >  2 files changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 93fdad8a7447..0ddef7256d02 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -400,6 +400,7 @@
> >  #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
> >  #define   PIPE_CONTROL_CS_STALL				(1<<20)
> >  #define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
> > +#define   PIPE_CONTROL_MEDIA_CACHE_INVALIDATE		(1<<16)
> 
> Maybe call it STATE_CLEAR since that's what Bspec calls it. libva doesn't
> use this, so I have no idea what it means. I also wonder whether we should
> nuke indirect state (bit9), too? Just to make sure the context switch
> doesn't fall over between 3d and media pipeline.
> 

I am not sure if we should use "indirect state pointers disable" as that
affects the context image. If userspace is using indirect state pointers
and contexts, it will expect the GPU state to be maintain across the
unforseeable context switches. Userspace would have to zap the pointers
itself if it does not want them preserved.

> Anyway if it helps it must be good. With the rename applied:

I don't have anything to suggest this improved matters, it was just
mentioned in conjunction with context switches in the bspec and seemed
appropriate when thinking about libva...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Daniel Vetter]

On Mon, Dec 15, 2014 at 07:03:43PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 15, 2014 at 04:24:48PM +0000, Chris Wilson wrote:
> > On Mon, Dec 15, 2014 at 10:41:52AM +0100, Daniel Vetter wrote:
> > > On Thu, Dec 11, 2014 at 08:17:01AM +0000, Chris Wilson wrote:
> > > > There exists a current workaround to prevent a hang on context switch
> > > > should the ring go to sleep in the middle of the restore,
> > > > WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> > > > spite of disabling arbitration (which prevents the ring from powering
> > > > down during the critical section) we were still hitting hangs that had
> > > > the hallmarks of the known erratum. That is we are still seeing hangs
> > > > "on the last instruction in the context restore". By comparing -nightly
> > > > (broken) with requests (working), we were able to deduce that it was the
> > > > semaphore LRI cross-talk that reproduced the original failure.
> > > > Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
> > > > the rings had to be awake to prevent the hangs. Fortunately, we can
> > > > reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
> > > > should be able to limit the extra power implications.
> > > > 
> > > > Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> > > > products, we should apply this extra hammer for all of the same
> > > > platforms despite so far that we have only been able to reproduce the
> > > > hang on certain ivb and hsw models. The last question is whether we want
> > > > to always use the extra hammer or only when we know semaphores are in
> > > > operation. At the moment, we only use LRI on non-RCS rings for
> > > > semaphores, but that may change in the future with the possibility of
> > > > reintroducing this bug under subtle conditions.
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: stable@vger.kernel.org
> > > 
> > > Oh dear, but awesome piece of debugging!
> > > 
> > > First question: How does this work on vlv with the split forcewake domain?
> > > Any bad side effects like longer ctx switch times?
> > 
> > It will work on vlv as well since the split power domains do not matter
> > here: the LRI is on an active ring, and the result of the LRI is to
> > temporary wake up the other domain.
> > The negative impact is that we do wake up other power domains and rings
> > during the context switch, a small but definite power increase.
> 
> I don't think there's any point in worrying about that for context
> switches as long as we still have the "semaphore LRI wakes up all
> power wells for every single batch" issue around.

Hm right, there have been patches floating around for that one ... A short
mention in the commit message about this would be useful.

> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_context.c | 39 ++++++++++++++++++++++++---------
> > > >  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
> > > >  2 files changed, 31 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > index 2acf5803cf32..724ccecea06a 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > @@ -484,7 +484,9 @@ mi_set_context(struct intel_engine_cs *ring,
> > > >  	       u32 hw_flags)
> > > >  {
> > > >  	u32 flags = hw_flags | MI_MM_SPACE_GTT;
> > > > -	int ret;
> > > > +	struct intel_engine_cs *engine;
> > > > +	const int num_rings = i915_semaphore_is_enabled(ring->dev) ? hweight32(INTEL_INFO(ring->dev)->ring_mask) : 0;
> > > > +	int len, i, ret;
> > > >  
> > > >  	/* 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
> > > > @@ -501,15 +503,26 @@ mi_set_context(struct intel_engine_cs *ring,
> > > >  	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
> > > >  		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
> > > >  
> > > > -	ret = intel_ring_begin(ring, 6);
> > > > +
> > > > +	len = 4;
> > > > +	if (INTEL_INFO(ring->dev)->gen >= 7)
> > > > +		len += 4*num_rings + 2;
> > > > +
> > > > +	ret = intel_ring_begin(ring, len);
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > >  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> > > > -	if (INTEL_INFO(ring->dev)->gen >= 7)
> > > > -		intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > > > -	else
> > > > -		intel_ring_emit(ring, MI_NOOP);
> > > > +	if (INTEL_INFO(ring->dev)->gen >= 7) {
> > > > +		if (num_rings) {
> > > > +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> > > > +			for_each_ring(engine, to_i915(ring->dev), i) {
> > > > +				intel_ring_emit(ring, RING_PSMI_CTL(engine->mmio_base));
> > > > +				intel_ring_emit(ring, _MASKED_BIT_ENABLE(GEN6_PSMI_SLEEP_MSG_DISABLE));
> > > > +			}
> > > > +		} else
> > > > +			intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> > > 
> > > For added paranoia, shouldn't we keep this one here? Or is the only thing
> > > this does frob the idle notifications like PSMI? I kinda prefer we just
> > > keep it if there's no harmful effects ...
> > 
> > Yeah, MI_ARB_ON_OFF is just a magic command used to do the same as the
> > PSMI twiddling on the local ring.
> > 
> > We can do MI_ARB_ON_OFF + LRI(!RCS) which is slightly shorter
> > instruction wise, closer to the original w/a and looks slightly neater.

Tbh without confirmation from a hw uarch that they're equivalent I prefer
we keep the mi_arb dance for the local ring, just in case that has some
other magic sauce going on. Means a bit more testing unfortunately, but
this has been going on for a long time already anyway.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Invalidate media caches on gen7

[Daniel Vetter]

On Tue, Dec 16, 2014 at 08:26:55AM +0000, Chris Wilson wrote:
> On Mon, Dec 15, 2014 at 10:34:45AM +0100, Daniel Vetter wrote:
> > On Thu, Dec 11, 2014 at 08:16:59AM +0000, Chris Wilson wrote:
> > > In the gen7 pipe control there is an extra bit to flush the media
> > > caches, so let's set it during cache invalidation flushes.
> > > 
> > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h         | 1 +
> > >  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
> > >  2 files changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 93fdad8a7447..0ddef7256d02 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -400,6 +400,7 @@
> > >  #define   PIPE_CONTROL_STORE_DATA_INDEX			(1<<21)
> > >  #define   PIPE_CONTROL_CS_STALL				(1<<20)
> > >  #define   PIPE_CONTROL_TLB_INVALIDATE			(1<<18)
> > > +#define   PIPE_CONTROL_MEDIA_CACHE_INVALIDATE		(1<<16)
> > 
> > Maybe call it STATE_CLEAR since that's what Bspec calls it. libva doesn't
> > use this, so I have no idea what it means. I also wonder whether we should
> > nuke indirect state (bit9), too? Just to make sure the context switch
> > doesn't fall over between 3d and media pipeline.
> > 
> 
> I am not sure if we should use "indirect state pointers disable" as that
> affects the context image. If userspace is using indirect state pointers
> and contexts, it will expect the GPU state to be maintain across the
> unforseeable context switches. Userspace would have to zap the pointers
> itself if it does not want them preserved.

Well userspace has to assume that already since indirect state can be
relocated by the kernel between batches. So I don't think it should be
harmful, and might help to prevent loading state data in some cases.
Anyway, separate patch.

> > Anyway if it helps it must be good. With the rename applied:
> 
> I don't have anything to suggest this improved matters, it was just
> mentioned in conjunction with context switches in the bspec and seemed
> appropriate when thinking about libva...

Yeah makes sense if just as a stopgap for libva leaking state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Chris Wilson]

On Tue, Dec 16, 2014 at 10:19:47AM +0100, Daniel Vetter wrote:
> On Mon, Dec 15, 2014 at 07:03:43PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 15, 2014 at 04:24:48PM +0000, Chris Wilson wrote:
> > > On Mon, Dec 15, 2014 at 10:41:52AM +0100, Daniel Vetter wrote:
> > > > On Thu, Dec 11, 2014 at 08:17:01AM +0000, Chris Wilson wrote:
> > > > > There exists a current workaround to prevent a hang on context switch
> > > > > should the ring go to sleep in the middle of the restore,
> > > > > WaProgramMiArbOnOffAroundMiSetContext (applicable to all gen7+). In
> > > > > spite of disabling arbitration (which prevents the ring from powering
> > > > > down during the critical section) we were still hitting hangs that had
> > > > > the hallmarks of the known erratum. That is we are still seeing hangs
> > > > > "on the last instruction in the context restore". By comparing -nightly
> > > > > (broken) with requests (working), we were able to deduce that it was the
> > > > > semaphore LRI cross-talk that reproduced the original failure.
> > > > > Explicitly disabling PMSI sleep on the RCS ring was insufficient, all
> > > > > the rings had to be awake to prevent the hangs. Fortunately, we can
> > > > > reduce the wakelock to the MI_SET_CONTEXT operation itself, and so
> > > > > should be able to limit the extra power implications.
> > > > > 
> > > > > Since the MI_ARB_ON_OFF workaround is listed for all gen7 and above
> > > > > products, we should apply this extra hammer for all of the same
> > > > > platforms despite so far that we have only been able to reproduce the
> > > > > hang on certain ivb and hsw models. The last question is whether we want
> > > > > to always use the extra hammer or only when we know semaphores are in
> > > > > operation. At the moment, we only use LRI on non-RCS rings for
> > > > > semaphores, but that may change in the future with the possibility of
> > > > > reintroducing this bug under subtle conditions.
> > > > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80660
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=83677
> > > > > Cc: Simon Farnsworth <simon@farnz.org.uk>
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: stable@vger.kernel.org
> > > > 
> > > > Oh dear, but awesome piece of debugging!
> > > > 
> > > > First question: How does this work on vlv with the split forcewake domain?
> > > > Any bad side effects like longer ctx switch times?
> > > 
> > > It will work on vlv as well since the split power domains do not matter
> > > here: the LRI is on an active ring, and the result of the LRI is to
> > > temporary wake up the other domain.
> > > The negative impact is that we do wake up other power domains and rings
> > > during the context switch, a small but definite power increase.
> > 
> > I don't think there's any point in worrying about that for context
> > switches as long as we still have the "semaphore LRI wakes up all
> > power wells for every single batch" issue around.
> 
> Hm right, there have been patches floating around for that one ... A short
> mention in the commit message about this would be useful.

If you notice, it was hinted at in the commit message. requests does
deferred semaphore signalling and that was key to realising the trigger
for the bug. I expanded upon that in v2.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3] drm/i915: Disable PMSI sleep messages on all rings around context switches

[Daniel Vetter]

On Tue, Dec 16, 2014 at 09:55:23AM +0000, Chris Wilson wrote:
> On Tue, Dec 16, 2014 at 10:19:47AM +0100, Daniel Vetter wrote:
> > Hm right, there have been patches floating around for that one ... A short
> > mention in the commit message about this would be useful.
> 
> If you notice, it was hinted at in the commit message. requests does
> deferred semaphore signalling and that was key to realising the trigger
> for the bug. I expanded upon that in v2.

I meant the power implications of waking up other gt power domains. But
really was just a minor thing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx