* [PATCH v2 0/3] Rendering specific Hw workarounds for VLV
@ 2014-02-07 12:22 akash.goel
2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: akash.goel @ 2014-02-07 12:22 UTC (permalink / raw)
To: intel-gfx; +Cc: 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
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' 2014-02-07 12:22 [PATCH v2 0/3] Rendering specific Hw workarounds for VLV akash.goel @ 2014-02-07 12:22 ` akash.goel 2014-03-21 11:50 ` Gupta, Sourab 2014-03-21 11:59 ` Damien Lespiau 2014-02-07 12:22 ` [PATCH v2 2/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaReadAfterWriteHazard' akash.goel 2014-02-07 12:22 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation akash.goel 2 siblings, 2 replies; 20+ messages in thread From: akash.goel @ 2014-02-07 12:22 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' 2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel @ 2014-03-21 11:50 ` Gupta, Sourab 2014-03-21 11:59 ` Damien Lespiau 1 sibling, 0 replies; 20+ messages in thread From: Gupta, Sourab @ 2014-03-21 11:50 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' 2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel 2014-03-21 11:50 ` Gupta, Sourab @ 2014-03-21 11:59 ` Damien Lespiau 1 sibling, 0 replies; 20+ messages in thread From: Damien Lespiau @ 2014-03-21 11:59 UTC (permalink / raw) To: akash.goel; +Cc: intel-gfx, sourab.gupta 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaReadAfterWriteHazard' 2014-02-07 12:22 [PATCH v2 0/3] Rendering specific Hw workarounds for VLV akash.goel 2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel @ 2014-02-07 12:22 ` akash.goel 2014-02-07 12:22 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation akash.goel 2 siblings, 0 replies; 20+ messages in thread From: akash.goel @ 2014-02-07 12:22 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation 2014-02-07 12:22 [PATCH v2 0/3] Rendering specific Hw workarounds for VLV akash.goel 2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel 2014-02-07 12:22 ` [PATCH v2 2/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaReadAfterWriteHazard' akash.goel @ 2014-02-07 12:22 ` akash.goel 2014-02-07 12:31 ` Chris Wilson 2014-02-07 14:44 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation Ville Syrjälä 2 siblings, 2 replies; 20+ messages in thread From: akash.goel @ 2014-02-07 12:22 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation 2014-02-07 12:22 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation akash.goel @ 2014-02-07 12:31 ` Chris Wilson 2014-02-07 14:34 ` Goel, Akash 2014-03-21 12:35 ` [PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg sourab.gupta 2014-02-07 14:44 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation Ville Syrjälä 1 sibling, 2 replies; 20+ messages in thread From: Chris Wilson @ 2014-02-07 12:31 UTC (permalink / raw) To: akash.goel; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation 2014-02-07 12:31 ` Chris Wilson @ 2014-02-07 14:34 ` Goel, Akash 2014-03-21 12:35 ` [PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg sourab.gupta 1 sibling, 0 replies; 20+ messages in thread From: Goel, Akash @ 2014-02-07 14:34 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg 2014-02-07 12:31 ` Chris Wilson 2014-02-07 14:34 ` Goel, Akash @ 2014-03-21 12:35 ` sourab.gupta 2014-03-21 12:35 ` [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register sourab.gupta 1 sibling, 1 reply; 20+ messages in thread From: sourab.gupta @ 2014-03-21 12:35 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, Akash Goel, Chris Wilson 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 12:35 ` [PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg sourab.gupta @ 2014-03-21 12:35 ` sourab.gupta 2014-03-21 12:58 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: sourab.gupta @ 2014-03-21 12:35 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, Akash Goel, Chris Wilson 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 12:35 ` [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register sourab.gupta @ 2014-03-21 12:58 ` Chris Wilson 2014-03-21 13:09 ` Gupta, Sourab 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2014-03-21 12:58 UTC (permalink / raw) To: sourab.gupta; +Cc: Akash Goel, Daniel Vetter, intel-gfx, 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 12:58 ` Chris Wilson @ 2014-03-21 13:09 ` Gupta, Sourab 2014-03-21 13:17 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: Gupta, Sourab @ 2014-03-21 13:09 UTC (permalink / raw) To: Chris Wilson; +Cc: Goel, Akash, Daniel Vetter, intel-gfx, Wilson, Chris 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 13:09 ` Gupta, Sourab @ 2014-03-21 13:17 ` Chris Wilson 2014-03-21 13:31 ` Gupta, Sourab 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2014-03-21 13:17 UTC (permalink / raw) To: Gupta, Sourab; +Cc: Goel, Akash, Daniel Vetter, intel-gfx, Wilson, Chris 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 13:17 ` Chris Wilson @ 2014-03-21 13:31 ` Gupta, Sourab 2014-03-21 13:45 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: Gupta, Sourab @ 2014-03-21 13:31 UTC (permalink / raw) To: Chris Wilson; +Cc: Goel, Akash, Daniel Vetter, intel-gfx, Wilson, Chris 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 13:31 ` Gupta, Sourab @ 2014-03-21 13:45 ` Chris Wilson 2014-03-21 15:28 ` [PATCH v2] " sourab.gupta 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2014-03-21 13:45 UTC (permalink / raw) To: Gupta, Sourab; +Cc: Goel, Akash, Daniel Vetter, intel-gfx, Wilson, Chris 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 13:45 ` Chris Wilson @ 2014-03-21 15:28 ` sourab.gupta 2014-03-21 16:52 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: sourab.gupta @ 2014-03-21 15:28 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, Akash Goel, 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 15:28 ` [PATCH v2] " sourab.gupta @ 2014-03-21 16:52 ` Chris Wilson 2014-03-22 4:25 ` Gupta, Sourab 0 siblings, 1 reply; 20+ messages in thread From: Chris Wilson @ 2014-03-21 16:52 UTC (permalink / raw) To: sourab.gupta; +Cc: Daniel Vetter, intel-gfx, Akash Goel 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-21 16:52 ` Chris Wilson @ 2014-03-22 4:25 ` Gupta, Sourab 2014-03-22 9:20 ` Chris Wilson 0 siblings, 1 reply; 20+ messages in thread From: Gupta, Sourab @ 2014-03-22 4:25 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register 2014-03-22 4:25 ` Gupta, Sourab @ 2014-03-22 9:20 ` Chris Wilson 0 siblings, 0 replies; 20+ messages in thread From: Chris Wilson @ 2014-03-22 9:20 UTC (permalink / raw) To: Gupta, Sourab; +Cc: Daniel Vetter, intel-gfx, Goel, Akash 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation 2014-02-07 12:22 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation akash.goel 2014-02-07 12:31 ` Chris Wilson @ 2014-02-07 14:44 ` Ville Syrjälä 1 sibling, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2014-02-07 14:44 UTC (permalink / raw) To: akash.goel; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2014-03-22 9:20 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-02-07 12:22 [PATCH v2 0/3] Rendering specific Hw workarounds for VLV akash.goel 2014-02-07 12:22 ` [PATCH v2 1/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaTlbInvalidateStoreDataBefore' akash.goel 2014-03-21 11:50 ` Gupta, Sourab 2014-03-21 11:59 ` Damien Lespiau 2014-02-07 12:22 ` [PATCH v2 2/3] drm/i915/vlv: Added a rendering specific Hw WA 'WaReadAfterWriteHazard' akash.goel 2014-02-07 12:22 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation akash.goel 2014-02-07 12:31 ` Chris Wilson 2014-02-07 14:34 ` Goel, Akash 2014-03-21 12:35 ` [PATCH 1/2] drm/i915/vlv: Remove the enabling of VS_TIMER_DISPATCH bit in MI MODE reg sourab.gupta 2014-03-21 12:35 ` [PATCH 2/2] drm/i915/vlv: Enabling the TLB invalidate bit in GFX Mode register sourab.gupta 2014-03-21 12:58 ` Chris Wilson 2014-03-21 13:09 ` Gupta, Sourab 2014-03-21 13:17 ` Chris Wilson 2014-03-21 13:31 ` Gupta, Sourab 2014-03-21 13:45 ` Chris Wilson 2014-03-21 15:28 ` [PATCH v2] " sourab.gupta 2014-03-21 16:52 ` Chris Wilson 2014-03-22 4:25 ` Gupta, Sourab 2014-03-22 9:20 ` Chris Wilson 2014-02-07 14:44 ` [PATCH v2 3/3] drm/i915/vlv: Modified the programming of 2 regs in Ring initialisation Ville Syrjälä
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.