[PATCH v2 0/3] Rendering specific Hw workarounds for VLV

[PATCH v2 0/3] Rendering specific Hw workarounds for VLV

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

The following patches leads to stable behavior on VLV, especially
when playing 3D Apps, benchmarks.
Addressed review comments from Ville.

Akash Goel (3):
  drm/i915/vlv: Added a rendering specific Hw WA
    'WaTlbInvalidateStoreDataBefore'
  drm/i915/vlv: Added a rendering specific Hw WA
    'WaReadAfterWriteHazard'
  drm/i915/vlv: Modified the programming of 2 regs in Ring
    initialisation

 drivers/gpu/drm/i915/intel_ringbuffer.c | 68 ++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 5 deletions(-)

-- 
1.8.5.2

[PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore'

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new rendering specific Workaround 'WaTlbInvalidateStoreDataBefore'.
In this WA, before pipecontrol with TLB invalidate set, need to add 2 MI
Store data commands.

v2: Modified the WA comment. (Ville)

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d897a19..2ac6600 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2183,6 +2183,29 @@ intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring)
 	uint32_t flush_domains;
 	int ret;
 
+	if (IS_VALLEYVIEW(ring->dev)) {
+		/*
+		 * WaTlbInvalidateStoreDataBefore
+		 * Before pipecontrol with TLB invalidate set, need 2 store
+		 * data commands (such as MI_STORE_DATA_IMM or MI_STORE_DATA_INDEX)
+		 * Without this, hardware cannot guarantee the command after the
+		 * PIPE_CONTROL with TLB inv will not use the old TLB values.
+		 * FIXME, should also apply to snb, ivb
+		 */
+		int i;
+		ret = intel_ring_begin(ring, 4 * 2);
+		if (ret)
+			return ret;
+		for (i = 0; i < 2; i++) {
+			intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
+			intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_INDEX <<
+						MI_STORE_DWORD_INDEX_SHIFT);
+			intel_ring_emit(ring, 0);
+			intel_ring_emit(ring, MI_NOOP);
+		}
+		intel_ring_advance(ring);
+	}
+
 	flush_domains = 0;
 	if (ring->gpu_caches_dirty)
 		flush_domains = I915_GEM_GPU_DOMAINS;
-- 
1.8.5.2

Re: [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore'

[Gupta, Sourab]

On Fri, 2014-02-07 at 12:22 +0000, Goel, Akash wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new rendering specific Workaround 'WaTlbInvalidateStoreDataBefore'.
> In this WA, before pipecontrol with TLB invalidate set, need to add 2 MI
> Store data commands.
> 
> v2: Modified the WA comment. (Ville)
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d897a19..2ac6600 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2183,6 +2183,29 @@ intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring)
>         uint32_t flush_domains;
>         int ret;
> 
> +       if (IS_VALLEYVIEW(ring->dev)) {
> +               /*
> +                * WaTlbInvalidateStoreDataBefore
> +                * Before pipecontrol with TLB invalidate set, need 2 store
> +                * data commands (such as MI_STORE_DATA_IMM or MI_STORE_DATA_INDEX)
> +                * Without this, hardware cannot guarantee the command after the
> +                * PIPE_CONTROL with TLB inv will not use the old TLB values.
> +                * FIXME, should also apply to snb, ivb
> +                */
> +               int i;
> +               ret = intel_ring_begin(ring, 4 * 2);
> +               if (ret)
> +                       return ret;
> +               for (i = 0; i < 2; i++) {
> +                       intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
> +                       intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_INDEX <<
> +                                               MI_STORE_DWORD_INDEX_SHIFT);
> +                       intel_ring_emit(ring, 0);
> +                       intel_ring_emit(ring, MI_NOOP);
> +               }
> +               intel_ring_advance(ring);
> +       }
> +
>         flush_domains = 0;
>         if (ring->gpu_caches_dirty)
>                 flush_domains = I915_GEM_GPU_DOMAINS;
> --
> 1.8.5.2
> 
Hi Ville,

Can you please let us know the status of this patch, as there are no
further comments to address here.

Regards,
Sourab

Re: [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore'

[Damien Lespiau]

On Fri, Feb 07, 2014 at 05:52:10PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Added a new rendering specific Workaround 'WaTlbInvalidateStoreDataBefore'.
> In this WA, before pipecontrol with TLB invalidate set, need to add 2 MI
> Store data commands.
> 
> v2: Modified the WA comment. (Ville)
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d897a19..2ac6600 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2183,6 +2183,29 @@ intel_ring_invalidate_all_caches(struct intel_ring_buffer *ring)
>  	uint32_t flush_domains;
>  	int ret;
>  
> +	if (IS_VALLEYVIEW(ring->dev)) {
> +		/*
> +		 * WaTlbInvalidateStoreDataBefore
> +		 * Before pipecontrol with TLB invalidate set, need 2 store
> +		 * data commands (such as MI_STORE_DATA_IMM or MI_STORE_DATA_INDEX)
> +		 * Without this, hardware cannot guarantee the command after the
> +		 * PIPE_CONTROL with TLB inv will not use the old TLB values.
> +		 * FIXME, should also apply to snb, ivb
> +		 */

We have a small syntax to indicate for which platform a W/A has been
implemented so a script in intel-gpu-tool can pick them up and make a
list. It's a bit low-fi, but has proven to be handy to quickly check
what we implement for a specific platform.

So this should be WaTlbInvalidateStoreDataBefore:vlv

This script works like this:

$ /path/to/intel-gpu-tools/scripts/list-workarounds -p vlv /path/to/kernel
WaCatErrorRejectionIssue
WaDisableAsyncFlipPerfMode
WaDisableBackToBackFlipFix
WaDisableDopClockGating
WaDisableEarlyCull
WaDisableL3Bank2xClockGate
WaDisablePSDDualDispatchEnable
...

-- 
Damien

[PATCH v2 2/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaReadAfterWriteHazard'

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Added a new rendering specific Workaround 'WaReadAfterWriteHazard'.
In this WA, need to add 12 MI Store Dword commands to ensure proper
flush of h/w pipeline.

v2: Modified the WA comment (Ville)

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2ac6600..49370a1 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2173,6 +2173,32 @@ intel_ring_flush_all_caches(struct intel_ring_buffer *ring)
 
 	trace_i915_gem_ring_flush(ring, 0, I915_GEM_GPU_DOMAINS);
 
+	if (IS_VALLEYVIEW(ring->dev)) {
+		/*
+		 * WaReadAfterWriteHazard
+		 * Send a number of Store Data commands here to finish
+		 * flushing hardware pipeline.This is needed in the case
+		 * where the next workload tries reading from the same
+		 * surface that this batch writes to. Without these StoreDWs,
+		 * not all of the data will actually be flushd to the surface
+		 * by the time the next batch starts reading it, possibly
+		 * causing a small amount of corruption.
+		 * FIXME, should also apply to snb, ivb.
+		 */
+		int i;
+		ret = intel_ring_begin(ring, 4 * 12);
+		if (ret)
+			return ret;
+		for (i = 0; i < 12; i++) {
+			intel_ring_emit(ring, MI_STORE_DWORD_INDEX);
+			intel_ring_emit(ring, I915_GEM_HWS_SCRATCH_INDEX <<
+							MI_STORE_DWORD_INDEX_SHIFT);
+			intel_ring_emit(ring, 0);
+			intel_ring_emit(ring, MI_NOOP);
+		}
+		intel_ring_advance(ring);
+	}
+
 	ring->gpu_caches_dirty = false;
 	return 0;
 }
-- 
1.8.5.2

[PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation

[akash.goel]

From: Akash Goel <akash.goel@intel.com>

Modified programming of following 2 regs in Render ring initialisation fn.
1. GFX_MODE_GEN7 (Enabling TLB invalidate)
2. MI_MODE (Enabling MI Flush)

v2: Removed the enabling of MI_FLUSH (Ville)
    Added new comments (Ville).

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 49370a1..0d7d927b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -563,7 +563,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 	int ret = init_ring_common(ring);
 
 	if (INTEL_INFO(dev)->gen > 3)
-		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
+		/* FIXME, should also apply to ivb */
+		if (!IS_VALLEYVIEW(dev))
+			I915_WRITE(MI_MODE,
+				_MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
 
 	/* We need to disable the AsyncFlip performance optimisations in order
 	 * to use MI_WAIT_FOR_EVENT within the CS. It should already be
@@ -579,10 +582,16 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 		I915_WRITE(GFX_MODE,
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS));
 
-	if (IS_GEN7(dev))
-		I915_WRITE(GFX_MODE_GEN7,
-			   _MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
-			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	if (IS_GEN7(dev)) {
+		if (IS_VALLEYVIEW(dev)) {
+			/* FIXME, should also apply to ivb */
+			I915_WRITE(GFX_MODE_GEN7,
+				_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+		} else
+			I915_WRITE(GFX_MODE_GEN7,
+				_MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
+				_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	}
 
 	if (INTEL_INFO(dev)->gen >= 5) {
 		ret = init_pipe_control(ring);
-- 
1.8.5.2

Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation

[Chris Wilson]

On Fri, Feb 07, 2014 at 05:52:12PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Modified programming of following 2 regs in Render ring initialisation fn.
> 1. GFX_MODE_GEN7 (Enabling TLB invalidate)

The changelog needs to explain why. According to the spec this is a
pessimisation.

> 2. MI_MODE (Enabling MI Flush)

And this is out-of-date. Doesn't describe the actual change nor why.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation

[Goel, Akash]

> 1. GFX_MODE_GEN7 (Enabling TLB invalidate)
>> The changelog needs to explain why. According to the spec this is a pessimisation.
Ok, Will look into this. 

> 2. MI_MODE (Enabling MI Flush)
>> And this is out-of-date. Doesn't describe the actual change nor why.
Sorry I did not update the commit message properly.
Actually I have reverted the change, enabling of MI_FLUSH, in this new version of the patch, as you also said this is obsolete. 

Best Regards
Akash

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, February 07, 2014 6:01 PM
To: Goel, Akash
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation

On Fri, Feb 07, 2014 at 05:52:12PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Modified programming of following 2 regs in Render ring initialisation fn.
> 1. GFX_MODE_GEN7 (Enabling TLB invalidate)

The changelog needs to explain why. According to the spec this is a pessimisation.

-Chris

--
Chris Wilson, Intel Open Source Technology Centre

[PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg

[sourab.gupta]

From: Akash Goel <akash.goel@intel.com>

Removing the VS_TIMER_DISPATCH bit enable for MI MODE reg for
VLV platform as it is not required.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 1c734ab..7da5774 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -567,7 +567,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 	int ret = init_ring_common(ring);
 
 	if (INTEL_INFO(dev)->gen > 3)
-		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
+		/* FIXME, should also apply to ivb */
+		if (!IS_VALLEYVIEW(dev))
+			I915_WRITE(MI_MODE,
+					_MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
 
 	/* We need to disable the AsyncFlip performance optimisations in order
 	 * to use MI_WAIT_FOR_EVENT within the CS. It should already be
-- 
1.8.5.1

[PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[sourab.gupta]

From: Akash Goel <akash.goel@intel.com>

This patch Enables the bit for TLB invalidate in GFX Mode register.

According to bspec,  When enabled this bit limits the invalidation
of the TLB only to batch buffer boundaries, to pipe_control
commands which have the TLB invalidation bit set and sync flushes.
If disabled, the TLB caches are flushed for every full flush of
the pipeline.

Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7da5774..320637b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -586,10 +586,16 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 		I915_WRITE(GFX_MODE,
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS));
 
-	if (IS_GEN7(dev))
-		I915_WRITE(GFX_MODE_GEN7,
-			   _MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
-			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	if (IS_GEN7(dev)) {
+		if (IS_VALLEYVIEW(dev)) {
+			/* FIXME, should also apply to ivb */
+			I915_WRITE(GFX_MODE_GEN7,
+					_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+		} else
+			I915_WRITE(GFX_MODE_GEN7,
+					_MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
+					_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	}
 
 	if (INTEL_INFO(dev)->gen >= 5) {
 		ret = init_pipe_control(ring);
-- 
1.8.5.1

Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Chris Wilson]

On Fri, Mar 21, 2014 at 06:05:04PM +0530, sourab.gupta@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch Enables the bit for TLB invalidate in GFX Mode register.
> 
> According to bspec,  When enabled this bit limits the invalidation
> of the TLB only to batch buffer boundaries, to pipe_control
> commands which have the TLB invalidation bit set and sync flushes.
> If disabled, the TLB caches are flushed for every full flush of
> the pipeline.

So why do we want to not disable it?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Gupta, Sourab]

On Fri, 2014-03-21 at 12:58 +0000, Chris Wilson wrote:
> On Fri, Mar 21, 2014 at 06:05:04PM +0530, sourab.gupta@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > 
> > According to bspec,  When enabled this bit limits the invalidation
> > of the TLB only to batch buffer boundaries, to pipe_control
> > commands which have the TLB invalidation bit set and sync flushes.
> > If disabled, the TLB caches are flushed for every full flush of
> > the pipeline.
> 
> So why do we want to not disable it?
> -Chris
> 
Hi Chris,
As per the description, enabling this bit will make the TLB invalidation
more optimal. Otherwise, TLB invalidation will happen for every full
pipeline flush. Thats why we are enabling this bit.

Regards,
Sourab

Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Chris Wilson]

On Fri, Mar 21, 2014 at 01:09:12PM +0000, Gupta, Sourab wrote:
> On Fri, 2014-03-21 at 12:58 +0000, Chris Wilson wrote:
> > On Fri, Mar 21, 2014 at 06:05:04PM +0530, sourab.gupta@intel.com wrote:
> > > From: Akash Goel <akash.goel@intel.com>
> > > 
> > > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > > 
> > > According to bspec,  When enabled this bit limits the invalidation
> > > of the TLB only to batch buffer boundaries, to pipe_control
> > > commands which have the TLB invalidation bit set and sync flushes.
> > > If disabled, the TLB caches are flushed for every full flush of
> > > the pipeline.
> > 
> > So why do we want to not disable it?
> > -Chris
> > 
> Hi Chris,
> As per the description, enabling this bit will make the TLB invalidation
> more optimal. Otherwise, TLB invalidation will happen for every full
> pipeline flush. Thats why we are enabling this bit.

You are not enabling the bit either, you simply do not disable it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Gupta, Sourab]

On Fri, 2014-03-21 at 13:17 +0000, Chris Wilson wrote:
> On Fri, Mar 21, 2014 at 01:09:12PM +0000, Gupta, Sourab wrote:
> > On Fri, 2014-03-21 at 12:58 +0000, Chris Wilson wrote:
> > > On Fri, Mar 21, 2014 at 06:05:04PM +0530, sourab.gupta@intel.com wrote:
> > > > From: Akash Goel <akash.goel@intel.com>
> > > > 
> > > > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > > > 
> > > > According to bspec,  When enabled this bit limits the invalidation
> > > > of the TLB only to batch buffer boundaries, to pipe_control
> > > > commands which have the TLB invalidation bit set and sync flushes.
> > > > If disabled, the TLB caches are flushed for every full flush of
> > > > the pipeline.
> > > 
> > > So why do we want to not disable it?
> > > -Chris
> > > 
> > Hi Chris,
> > As per the description, enabling this bit will make the TLB invalidation
> > more optimal. Otherwise, TLB invalidation will happen for every full
> > pipeline flush. Thats why we are enabling this bit.
> 
> You are not enabling the bit either, you simply do not disable it.
> -Chris
> 
Hi Chris,
According to spec, the default value of this bit will be 1 after reset.
So, we are letting the default value remain as 1 and not disabling it.
Probably the commit message should have better reflected this as 'not
resetting the bit for TLB invalidate from its default value of 1'.

If required, we can explicitly set the value to 1 (without assuming any
defaults).

Regards,
Sourab

Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Chris Wilson]

On Fri, Mar 21, 2014 at 01:31:56PM +0000, Gupta, Sourab wrote:
> On Fri, 2014-03-21 at 13:17 +0000, Chris Wilson wrote:
> > On Fri, Mar 21, 2014 at 01:09:12PM +0000, Gupta, Sourab wrote:
> > > On Fri, 2014-03-21 at 12:58 +0000, Chris Wilson wrote:
> > > > On Fri, Mar 21, 2014 at 06:05:04PM +0530, sourab.gupta@intel.com wrote:
> > > > > From: Akash Goel <akash.goel@intel.com>
> > > > > 
> > > > > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > > > > 
> > > > > According to bspec,  When enabled this bit limits the invalidation
> > > > > of the TLB only to batch buffer boundaries, to pipe_control
> > > > > commands which have the TLB invalidation bit set and sync flushes.
> > > > > If disabled, the TLB caches are flushed for every full flush of
> > > > > the pipeline.
> > > > 
> > > > So why do we want to not disable it?
> > > > -Chris
> > > > 
> > > Hi Chris,
> > > As per the description, enabling this bit will make the TLB invalidation
> > > more optimal. Otherwise, TLB invalidation will happen for every full
> > > pipeline flush. Thats why we are enabling this bit.
> > 
> > You are not enabling the bit either, you simply do not disable it.
> > -Chris
> > 
> Hi Chris,
> According to spec, the default value of this bit will be 1 after reset.
> So, we are letting the default value remain as 1 and not disabling it.
> Probably the commit message should have better reflected this as 'not
> resetting the bit for TLB invalidate from its default value of 1'.
> 
> If required, we can explicitly set the value to 1 (without assuming any
> defaults).

I would indeed explicitly set it according to how we've implemented TLB
flushes. And you can then reference the commit that adds the
pre-requisite pipe-control invalidation as if we trust our history then
at some point in the past relying on an invalidate on
MI_BATCH_BUFFER_START was insufficient.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[sourab.gupta]

From: Akash Goel <akash.goel@intel.com>

This patch Enables the bit for TLB invalidate in GFX Mode register.

According to bspec,  When enabled this bit limits the invalidation
of the TLB only to batch buffer boundaries, to pipe_control
commands which have the TLB invalidation bit set and sync flushes.
If disabled, the TLB caches are flushed for every full flush of
the pipeline.

v2: Explicitly enabling TLB invalidate bit instead of assuming
default 1 (Chris Wilson)

Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 4eb3e06..dca8c6d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -583,10 +583,17 @@ static int init_render_ring(struct intel_ring_buffer *ring)
 		I915_WRITE(GFX_MODE,
 			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS));
 
-	if (IS_GEN7(dev))
-		I915_WRITE(GFX_MODE_GEN7,
-			   _MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
-			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	if (IS_GEN7(dev)) {
+		if (IS_VALLEYVIEW(dev)) {
+			/* FIXME, should also apply to ivb */
+			I915_WRITE(GFX_MODE_GEN7,
+					_MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS) |
+					_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+		} else
+			I915_WRITE(GFX_MODE_GEN7,
+					_MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
+					_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
+	}
 
 	if (INTEL_INFO(dev)->gen >= 5) {
 		ret = init_pipe_control(ring);
-- 
1.8.5.1

Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Chris Wilson]

On Fri, Mar 21, 2014 at 08:58:08PM +0530, sourab.gupta@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> This patch Enables the bit for TLB invalidate in GFX Mode register.
> 
> According to bspec,  When enabled this bit limits the invalidation
> of the TLB only to batch buffer boundaries, to pipe_control
> commands which have the TLB invalidation bit set and sync flushes.
> If disabled, the TLB caches are flushed for every full flush of
> the pipeline.
> 
> v2: Explicitly enabling TLB invalidate bit instead of assuming
> default 1 (Chris Wilson)

Right, but there is nothing special about this code for vlv, all of gen7
share the same TLB invalidation code, and there is no documented reason
not to do the switch. So do a cursory test on ivb/hsw and send a patch
that doesn't say FIXME.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Gupta, Sourab]

On Fri, 2014-03-21 at 16:52 +0000, Chris Wilson wrote:
> On Fri, Mar 21, 2014 at 08:58:08PM +0530, sourab.gupta@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> > 
> > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > 
> > According to bspec,  When enabled this bit limits the invalidation
> > of the TLB only to batch buffer boundaries, to pipe_control
> > commands which have the TLB invalidation bit set and sync flushes.
> > If disabled, the TLB caches are flushed for every full flush of
> > the pipeline.
> > 
> > v2: Explicitly enabling TLB invalidate bit instead of assuming
> > default 1 (Chris Wilson)
> 
> Right, but there is nothing special about this code for vlv, all of gen7
> share the same TLB invalidation code, and there is no documented reason
> not to do the switch. So do a cursory test on ivb/hsw and send a patch
> that doesn't say FIXME.
> -Chris
> 
Hi Chris,
I agree, this could be applicable to ivb also. But we are constrained by
the availability of ivb/hsw machines and we're not able to test it on
those. We didn't want to cause any regression on those platforms. Thats
why we were limiting our patch to vlv only with a FIXME comment. We've
done this for other WAs also.

If required we can put this patch for full GEN7 (not tested on ivb/hsw)
and keep our fingers crossed! :)

Regards,
Sourab

Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register

[Chris Wilson]

On Sat, Mar 22, 2014 at 04:25:31AM +0000, Gupta, Sourab wrote:
> On Fri, 2014-03-21 at 16:52 +0000, Chris Wilson wrote:
> > On Fri, Mar 21, 2014 at 08:58:08PM +0530, sourab.gupta@intel.com wrote:
> > > From: Akash Goel <akash.goel@intel.com>
> > > 
> > > This patch Enables the bit for TLB invalidate in GFX Mode register.
> > > 
> > > According to bspec,  When enabled this bit limits the invalidation
> > > of the TLB only to batch buffer boundaries, to pipe_control
> > > commands which have the TLB invalidation bit set and sync flushes.
> > > If disabled, the TLB caches are flushed for every full flush of
> > > the pipeline.
> > > 
> > > v2: Explicitly enabling TLB invalidate bit instead of assuming
> > > default 1 (Chris Wilson)
> > 
> > Right, but there is nothing special about this code for vlv, all of gen7
> > share the same TLB invalidation code, and there is no documented reason
> > not to do the switch. So do a cursory test on ivb/hsw and send a patch
> > that doesn't say FIXME.
> > -Chris
> > 
> Hi Chris,
> I agree, this could be applicable to ivb also. But we are constrained by
> the availability of ivb/hsw machines and we're not able to test it on
> those. We didn't want to cause any regression on those platforms. Thats
> why we were limiting our patch to vlv only with a FIXME comment. We've
> done this for other WAs also.
> 
> If required we can put this patch for full GEN7 (not tested on ivb/hsw)
> and keep our fingers crossed! :)

How about for the generalised patch,
Tested-by: Chris Wilson <chris@chris-wilson.co.uk> # ivb, hsw

Happy now? :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation

[Ville Syrjälä]

On Fri, Feb 07, 2014 at 05:52:12PM +0530, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@intel.com>
> 
> Modified programming of following 2 regs in Render ring initialisation fn.
> 1. GFX_MODE_GEN7 (Enabling TLB invalidate)
> 2. MI_MODE (Enabling MI Flush)
> 
> v2: Removed the enabling of MI_FLUSH (Ville)
>     Added new comments (Ville).
> 
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 49370a1..0d7d927b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -563,7 +563,10 @@ static int init_render_ring(struct intel_ring_buffer *ring)
>  	int ret = init_ring_common(ring);
>  
>  	if (INTEL_INFO(dev)->gen > 3)
> -		I915_WRITE(MI_MODE, _MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));
> +		/* FIXME, should also apply to ivb */
> +		if (!IS_VALLEYVIEW(dev))
> +			I915_WRITE(MI_MODE,
> +				_MASKED_BIT_ENABLE(VS_TIMER_DISPATCH));

Was this supposed to be here? I guess it should be a separate patch.

>  
>  	/* We need to disable the AsyncFlip performance optimisations in order
>  	 * to use MI_WAIT_FOR_EVENT within the CS. It should already be
> @@ -579,10 +582,16 @@ static int init_render_ring(struct intel_ring_buffer *ring)
>  		I915_WRITE(GFX_MODE,
>  			   _MASKED_BIT_ENABLE(GFX_TLB_INVALIDATE_ALWAYS));
>  
> -	if (IS_GEN7(dev))
> -		I915_WRITE(GFX_MODE_GEN7,
> -			   _MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
> -			   _MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
> +	if (IS_GEN7(dev)) {
> +		if (IS_VALLEYVIEW(dev)) {
> +			/* FIXME, should also apply to ivb */
> +			I915_WRITE(GFX_MODE_GEN7,
> +				_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
> +		} else
> +			I915_WRITE(GFX_MODE_GEN7,
> +				_MASKED_BIT_DISABLE(GFX_TLB_INVALIDATE_ALWAYS) |
> +				_MASKED_BIT_ENABLE(GFX_REPLAY_MODE));
> +	}
>  
>  	if (INTEL_INFO(dev)->gen >= 5) {
>  		ret = init_pipe_control(ring);
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC