* [PATCH] drm/i915: stop command parser before sync flush
@ 2014-03-04 5:16 naresh.kumar.kachhi
2014-03-04 7:39 ` Chris Wilson
0 siblings, 1 reply; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-04 5:16 UTC (permalink / raw)
To: intel-gfx; +Cc: Naresh Kumar Kachhi
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Based on Bspec the command parser must be stopped prior to
issuing sync flush. Only after observing Rings Idle set in
MI_MODE can a Sync Flush be issued. Once sync flush has
finished the command parser is re-enabled by clearing Stop Rings.
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 3 +++
drivers/gpu/drm/i915/intel_ringbuffer.c | 13 +++++++++++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++++
3 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 2f564ce..97d872a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -720,6 +720,7 @@
#define RING_INSTPS(base) ((base)+0x70)
#define RING_DMA_FADD(base) ((base)+0x78)
#define RING_INSTPM(base) ((base)+0xc0)
+#define RING_MI_MODE(base) ((base)+0x9c)
#define INSTPS 0x02070 /* 965+ only */
#define INSTDONE1 0x0207c /* 965+ only */
#define ACTHD_I965 0x02074
@@ -796,6 +797,8 @@
# define VS_TIMER_DISPATCH (1 << 6)
# define MI_FLUSH_ENABLE (1 << 12)
# define ASYNC_FLIP_PERF_DISABLE (1 << 14)
+# define MODE_STOP (1 << 8)
+# define MODE_IDLE (1 << 9)
#define GEN6_GT_MODE 0x20d0
#define GEN6_GT_MODE_HI (1 << 9)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index b340c75..477b5dc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -982,6 +982,15 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
/* Flush the TLB for this page */
if (INTEL_INFO(dev)->gen >= 6) {
u32 reg = RING_INSTPM(ring->mmio_base);
+
+ /* before issuing a sync flush request the ring to go idle */
+ I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(MODE_STOP));
+
+ /* Wait for idle */
+ if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
+ /* don't fail here, try to invalidate TLB */
+ DRM_ERROR("%s :timed out trying to stop ring", ring->name);
+
I915_WRITE(reg,
_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
INSTPM_SYNC_FLUSH));
@@ -989,6 +998,10 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
1000))
DRM_ERROR("%s: wait for SyncFlush to complete for TLB invalidation timed out\n",
ring->name);
+
+ /* Clear the MI_MODE stop bit */
+ I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(MODE_STOP));
+ reg = I915_READ_MODE(ring); /* Barrier read */
}
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 08b91c6..1bb4a01 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -33,6 +33,11 @@ struct intel_hw_status_page {
#define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
#define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
+#define I915_READ_MODE(ring) \
+ I915_READ(RING_MI_MODE((ring)->mmio_base))
+#define I915_WRITE_MODE(ring, val) \
+ I915_WRITE(RING_MI_MODE((ring)->mmio_base), val)
+
enum intel_ring_hangcheck_action {
HANGCHECK_IDLE = 0,
HANGCHECK_WAIT,
--
1.8.1.2
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] drm/i915: stop command parser before sync flush
2014-03-04 5:16 [PATCH] drm/i915: stop command parser before sync flush naresh.kumar.kachhi
@ 2014-03-04 7:39 ` Chris Wilson
2014-03-12 6:50 ` [PATCH v1 0/2] disable rings " naresh.kumar.kachhi
0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2014-03-04 7:39 UTC (permalink / raw)
To: naresh.kumar.kachhi; +Cc: intel-gfx
On Tue, Mar 04, 2014 at 10:46:38AM +0530, naresh.kumar.kachhi@intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> Based on Bspec the command parser must be stopped prior to
> issuing sync flush. Only after observing Rings Idle set in
> MI_MODE can a Sync Flush be issued. Once sync flush has
> finished the command parser is re-enabled by clearing Stop Rings.
I am intrigued as to how the rings can be active when they are shut
down. Oh, I see, a slight misordering...
Can you please also send a patch to issue the ring disable before we
setup the status page? I wonder if there are other steps that should be
placed into that critical section where the ring is disabled?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v1 0/2] disable rings before sync flush
2014-03-04 7:39 ` Chris Wilson
@ 2014-03-12 6:50 ` naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 1/2] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 6:50 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
initial version: stop rings before sync flush
v1: address the comments from Chris Wilson on
http://www.spinics.net/lists/intel-gfx/msg41025.html.
Rather than adding stop and start ring, we are moving
ring disable before hw_status_page_setup (sync flush).
Naresh Kumar Kachhi (2):
drm/i915: disable rings before HW status page setup
drm/i915: warn if ring is active before sync flush
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 13 ++++++++-----
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
3 files changed, 12 insertions(+), 5 deletions(-)
--
1.8.5.3
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v1 1/2] drm/i915: disable rings before HW status page setup
2014-03-12 6:50 ` [PATCH v1 0/2] disable rings " naresh.kumar.kachhi
@ 2014-03-12 6:50 ` naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 2/2] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
2014-03-12 8:06 ` [PATCH v1 0/2] disable rings " Chris Wilson
2 siblings, 0 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 6:50 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Rings should be idle before issuing sync_flush
(in intel_ring_setup_status_page). This patch moves the ring
disabling before doing the HW status page setup.
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8590921..42b4001 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -440,16 +440,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
- if (I915_NEED_GFX_HWS(dev))
- intel_ring_setup_status_page(ring);
- else
- ring_setup_phys_status_page(ring);
-
/* Stop the ring if it's running. */
I915_WRITE_CTL(ring, 0);
I915_WRITE_HEAD(ring, 0);
ring->write_tail(ring, 0);
+ if (I915_NEED_GFX_HWS(dev))
+ intel_ring_setup_status_page(ring);
+ else
+ ring_setup_phys_status_page(ring);
+
head = I915_READ_HEAD(ring) & HEAD_ADDR;
/* G45 ring initialization fails to reset head to zero */
--
1.8.5.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v1 2/2] drm/i915: warn if ring is active before sync flush
2014-03-12 6:50 ` [PATCH v1 0/2] disable rings " naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 1/2] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
@ 2014-03-12 6:50 ` naresh.kumar.kachhi
2014-03-12 8:06 ` [PATCH v1 0/2] disable rings " Chris Wilson
2 siblings, 0 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 6:50 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Based on Bspec the command parser must be stopped prior to
issuing sync flush. This should be done by the caller of
intel_ring_setup_status_page. Patch adds a warning if it is
not done.
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +++
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
3 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 146609a..6174fda 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -748,6 +748,7 @@ enum punit_power_well {
#define RING_INSTPS(base) ((base)+0x70)
#define RING_DMA_FADD(base) ((base)+0x78)
#define RING_INSTPM(base) ((base)+0xc0)
+#define RING_MI_MODE(base) ((base)+0x9c)
#define INSTPS 0x02070 /* 965+ only */
#define INSTDONE1 0x0207c /* 965+ only */
#define ACTHD_I965 0x02074
@@ -824,6 +825,7 @@ enum punit_power_well {
# define VS_TIMER_DISPATCH (1 << 6)
# define MI_FLUSH_ENABLE (1 << 12)
# define ASYNC_FLIP_PERF_DISABLE (1 << 14)
+# define MODE_IDLE (1 << 9)
#define GEN6_GT_MODE 0x20d0
#define GEN7_GT_MODE 0x7008
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 42b4001..1bd3b58 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -982,6 +982,9 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
/* Flush the TLB for this page */
if (INTEL_INFO(dev)->gen >= 6) {
u32 reg = RING_INSTPM(ring->mmio_base);
+ /* ring should be idle before issuing a sync flush*/
+ WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
+
I915_WRITE(reg,
_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
INSTPM_SYNC_FLUSH));
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 09af920..f11ceb2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -33,6 +33,8 @@ struct intel_hw_status_page {
#define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
#define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
+#define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
+
enum intel_ring_hangcheck_action {
HANGCHECK_IDLE = 0,
HANGCHECK_WAIT,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v1 0/2] disable rings before sync flush
2014-03-12 6:50 ` [PATCH v1 0/2] disable rings " naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 1/2] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 2/2] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
@ 2014-03-12 8:06 ` Chris Wilson
2014-03-12 8:12 ` Chris Wilson
2 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2014-03-12 8:06 UTC (permalink / raw)
To: naresh.kumar.kachhi; +Cc: intel-gfx
On Wed, Mar 12, 2014 at 12:20:09PM +0530, naresh.kumar.kachhi@intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> initial version: stop rings before sync flush
>
> v1: address the comments from Chris Wilson on
> http://www.spinics.net/lists/intel-gfx/msg41025.html.
> Rather than adding stop and start ring, we are moving
> ring disable before hw_status_page_setup (sync flush).
>
> Naresh Kumar Kachhi (2):
> drm/i915: disable rings before HW status page setup
> drm/i915: warn if ring is active before sync flush
That looks much safer in general, thanks.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 0/2] disable rings before sync flush
2014-03-12 8:06 ` [PATCH v1 0/2] disable rings " Chris Wilson
@ 2014-03-12 8:12 ` Chris Wilson
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
0 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2014-03-12 8:12 UTC (permalink / raw)
To: naresh.kumar.kachhi, intel-gfx
On Wed, Mar 12, 2014 at 08:06:26AM +0000, Chris Wilson wrote:
> On Wed, Mar 12, 2014 at 12:20:09PM +0530, naresh.kumar.kachhi@intel.com wrote:
> > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> >
> > initial version: stop rings before sync flush
> >
> > v1: address the comments from Chris Wilson on
> > http://www.spinics.net/lists/intel-gfx/msg41025.html.
> > Rather than adding stop and start ring, we are moving
> > ring disable before hw_status_page_setup (sync flush).
> >
> > Naresh Kumar Kachhi (2):
> > drm/i915: disable rings before HW status page setup
> > drm/i915: warn if ring is active before sync flush
>
> That looks much safer in general, thanks.
And it raises the spectre of what if the rings didn't stop
immediately... Maybe we should throw in a wait_for(ring_idle(ring));
That could explain some of our mysterious problems when trying to start
rings and the random retry behaviour we resort to...
Make that patch 1.5 :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 0/3] disable rings before sync flush
2014-03-12 8:12 ` Chris Wilson
@ 2014-03-12 11:09 ` naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 1/3] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 11:09 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
initial version: stop rings before sync flush
v1: address the comments from Chris Wilson on
http://www.spinics.net/lists/intel-gfx/msg41025.html.
Rather than adding stop and start ring, we are moving
ring disable before hw_status_page_setup (sync flush).
v2: Based on Chris's suggestion. Added one more patch to wait
until the rings become idle after disabling. In case of timeout
we log a message.
Naresh Kumar Kachhi (3):
drm/i915: disable rings before HW status page setup
drm/i915: wait for rings to become idle once disabled
drm/i915: warn if ring is active before sync flush
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 18 +++++++++++++-----
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
3 files changed, 17 insertions(+), 5 deletions(-)
--
1.8.5.3
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v2 1/3] drm/i915: disable rings before HW status page setup
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
@ 2014-03-12 11:09 ` naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled naresh.kumar.kachhi
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 11:09 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Rings should be idle before issuing sync_flush
(in intel_ring_setup_status_page). This patch moves the ring
disabling before doing the HW status page setup.
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8590921..42b4001 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -440,16 +440,16 @@ static int init_ring_common(struct intel_ring_buffer *ring)
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
- if (I915_NEED_GFX_HWS(dev))
- intel_ring_setup_status_page(ring);
- else
- ring_setup_phys_status_page(ring);
-
/* Stop the ring if it's running. */
I915_WRITE_CTL(ring, 0);
I915_WRITE_HEAD(ring, 0);
ring->write_tail(ring, 0);
+ if (I915_NEED_GFX_HWS(dev))
+ intel_ring_setup_status_page(ring);
+ else
+ ring_setup_phys_status_page(ring);
+
head = I915_READ_HEAD(ring) & HEAD_ADDR;
/* G45 ring initialization fails to reset head to zero */
--
1.8.5.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 1/3] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
@ 2014-03-12 11:09 ` naresh.kumar.kachhi
2014-03-12 11:18 ` Chris Wilson
2014-03-12 11:09 ` [PATCH v2 3/3] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
2014-03-12 11:28 ` [PATCH v2 0/3] disable rings " Chris Wilson
3 siblings, 1 reply; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 11:09 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
make sure we wait for rings to become idle once they are
disabled. In case of timeout print an error message
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 2 ++
drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++++
drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
3 files changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 146609a..6174fda 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -748,6 +748,7 @@ enum punit_power_well {
#define RING_INSTPS(base) ((base)+0x70)
#define RING_DMA_FADD(base) ((base)+0x78)
#define RING_INSTPM(base) ((base)+0xc0)
+#define RING_MI_MODE(base) ((base)+0x9c)
#define INSTPS 0x02070 /* 965+ only */
#define INSTDONE1 0x0207c /* 965+ only */
#define ACTHD_I965 0x02074
@@ -824,6 +825,7 @@ enum punit_power_well {
# define VS_TIMER_DISPATCH (1 << 6)
# define MI_FLUSH_ENABLE (1 << 12)
# define ASYNC_FLIP_PERF_DISABLE (1 << 14)
+# define MODE_IDLE (1 << 9)
#define GEN6_GT_MODE 0x20d0
#define GEN7_GT_MODE 0x7008
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 42b4001..4da2211 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -445,6 +445,11 @@ static int init_ring_common(struct intel_ring_buffer *ring)
I915_WRITE_HEAD(ring, 0);
ring->write_tail(ring, 0);
+ /* Wait for idle */
+ if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
+ /* don't fail here, log a message to keep track */
+ DRM_ERROR("%s :timed out trying to stop ring", ring->name);
+
if (I915_NEED_GFX_HWS(dev))
intel_ring_setup_status_page(ring);
else
@@ -982,6 +987,7 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
/* Flush the TLB for this page */
if (INTEL_INFO(dev)->gen >= 6) {
u32 reg = RING_INSTPM(ring->mmio_base);
+
I915_WRITE(reg,
_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
INSTPM_SYNC_FLUSH));
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 09af920..f11ceb2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -33,6 +33,8 @@ struct intel_hw_status_page {
#define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
#define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
+#define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
+
enum intel_ring_hangcheck_action {
HANGCHECK_IDLE = 0,
HANGCHECK_WAIT,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled
2014-03-12 11:09 ` [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled naresh.kumar.kachhi
@ 2014-03-12 11:18 ` Chris Wilson
0 siblings, 0 replies; 14+ messages in thread
From: Chris Wilson @ 2014-03-12 11:18 UTC (permalink / raw)
To: naresh.kumar.kachhi; +Cc: intel-gfx
On Wed, Mar 12, 2014 at 04:39:41PM +0530, naresh.kumar.kachhi@intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> make sure we wait for rings to become idle once they are
> disabled. In case of timeout print an error message
>
> Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 6 ++++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
> 3 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 146609a..6174fda 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -748,6 +748,7 @@ enum punit_power_well {
> #define RING_INSTPS(base) ((base)+0x70)
> #define RING_DMA_FADD(base) ((base)+0x78)
> #define RING_INSTPM(base) ((base)+0xc0)
> +#define RING_MI_MODE(base) ((base)+0x9c)
> #define INSTPS 0x02070 /* 965+ only */
> #define INSTDONE1 0x0207c /* 965+ only */
> #define ACTHD_I965 0x02074
> @@ -824,6 +825,7 @@ enum punit_power_well {
> # define VS_TIMER_DISPATCH (1 << 6)
> # define MI_FLUSH_ENABLE (1 << 12)
> # define ASYNC_FLIP_PERF_DISABLE (1 << 14)
> +# define MODE_IDLE (1 << 9)
>
> #define GEN6_GT_MODE 0x20d0
> #define GEN7_GT_MODE 0x7008
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 42b4001..4da2211 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -445,6 +445,11 @@ static int init_ring_common(struct intel_ring_buffer *ring)
> I915_WRITE_HEAD(ring, 0);
> ring->write_tail(ring, 0);
>
> + /* Wait for idle */
This comment is too close to "i++; /* post-increment the variable i */"
Personally, I would remove the newline here so that this adjoins the
/* Stop the rings if it's running */ block, then that comment would seem
to naturally apply.
> + if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
> + /* don't fail here, log a message to keep track */
> + DRM_ERROR("%s :timed out trying to stop ring", ring->name);
Missed the trailing '\n'.
> +
> if (I915_NEED_GFX_HWS(dev))
> intel_ring_setup_status_page(ring);
> else
> @@ -982,6 +987,7 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
> /* Flush the TLB for this page */
> if (INTEL_INFO(dev)->gen >= 6) {
> u32 reg = RING_INSTPM(ring->mmio_base);
> +
Spurious whitespace change.
> I915_WRITE(reg,
> _MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
> INSTPM_SYNC_FLUSH));
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 09af920..f11ceb2 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -33,6 +33,8 @@ struct intel_hw_status_page {
> #define I915_READ_IMR(ring) I915_READ(RING_IMR((ring)->mmio_base))
> #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val)
>
> +#define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base))
> +
> enum intel_ring_hangcheck_action {
> HANGCHECK_IDLE = 0,
> HANGCHECK_WAIT,
> --
> 1.8.5.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] drm/i915: warn if ring is active before sync flush
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 1/3] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled naresh.kumar.kachhi
@ 2014-03-12 11:09 ` naresh.kumar.kachhi
2014-03-12 11:28 ` [PATCH v2 0/3] disable rings " Chris Wilson
3 siblings, 0 replies; 14+ messages in thread
From: naresh.kumar.kachhi @ 2014-03-12 11:09 UTC (permalink / raw)
To: intel-gfx
From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Based on Bspec the command parser must be stopped prior to
issuing sync flush. This should be done by the caller of
intel_ring_setup_status_page. Patch adds a warning if it is
not done.
v2: rebased based on new patch (wait for ring to become idle)
Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
---
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 4da2211..6ac6569 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -987,6 +987,8 @@ void intel_ring_setup_status_page(struct intel_ring_buffer *ring)
/* Flush the TLB for this page */
if (INTEL_INFO(dev)->gen >= 6) {
u32 reg = RING_INSTPM(ring->mmio_base);
+ /* ring should be idle before issuing a sync flush*/
+ WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
I915_WRITE(reg,
_MASKED_BIT_ENABLE(INSTPM_TLB_INVALIDATE |
--
1.8.5.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] disable rings before sync flush
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
` (2 preceding siblings ...)
2014-03-12 11:09 ` [PATCH v2 3/3] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
@ 2014-03-12 11:28 ` Chris Wilson
2014-03-12 15:07 ` Daniel Vetter
3 siblings, 1 reply; 14+ messages in thread
From: Chris Wilson @ 2014-03-12 11:28 UTC (permalink / raw)
To: naresh.kumar.kachhi; +Cc: intel-gfx
On Wed, Mar 12, 2014 at 04:39:39PM +0530, naresh.kumar.kachhi@intel.com wrote:
> From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>
> initial version: stop rings before sync flush
>
> v1: address the comments from Chris Wilson on
> http://www.spinics.net/lists/intel-gfx/msg41025.html.
> Rather than adding stop and start ring, we are moving
> ring disable before hw_status_page_setup (sync flush).
>
> v2: Based on Chris's suggestion. Added one more patch to wait
> until the rings become idle after disabling. In case of timeout
> we log a message.
>
> Naresh Kumar Kachhi (3):
> drm/i915: disable rings before HW status page setup
> drm/i915: wait for rings to become idle once disabled
> drm/i915: warn if ring is active before sync flush
>
> drivers/gpu/drm/i915/i915_reg.h | 2 ++
> drivers/gpu/drm/i915/intel_ringbuffer.c | 18 +++++++++++++-----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
> 3 files changed, 17 insertions(+), 5 deletions(-)
With the minor changes in patch 2,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
and the recommendation for stable@
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 0/3] disable rings before sync flush
2014-03-12 11:28 ` [PATCH v2 0/3] disable rings " Chris Wilson
@ 2014-03-12 15:07 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2014-03-12 15:07 UTC (permalink / raw)
To: Chris Wilson, naresh.kumar.kachhi, intel-gfx
On Wed, Mar 12, 2014 at 11:28:03AM +0000, Chris Wilson wrote:
> On Wed, Mar 12, 2014 at 04:39:39PM +0530, naresh.kumar.kachhi@intel.com wrote:
> > From: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> >
> > initial version: stop rings before sync flush
> >
> > v1: address the comments from Chris Wilson on
> > http://www.spinics.net/lists/intel-gfx/msg41025.html.
> > Rather than adding stop and start ring, we are moving
> > ring disable before hw_status_page_setup (sync flush).
> >
> > v2: Based on Chris's suggestion. Added one more patch to wait
> > until the rings become idle after disabling. In case of timeout
> > we log a message.
> >
> > Naresh Kumar Kachhi (3):
> > drm/i915: disable rings before HW status page setup
> > drm/i915: wait for rings to become idle once disabled
> > drm/i915: warn if ring is active before sync flush
> >
> > drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 18 +++++++++++++-----
> > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++
> > 3 files changed, 17 insertions(+), 5 deletions(-)
>
> With the minor changes in patch 2,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> and the recommendation for stable@
All 3 merged with the request polish applied to the 2nd patch.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-03-12 15:07 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-04 5:16 [PATCH] drm/i915: stop command parser before sync flush naresh.kumar.kachhi
2014-03-04 7:39 ` Chris Wilson
2014-03-12 6:50 ` [PATCH v1 0/2] disable rings " naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 1/2] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
2014-03-12 6:50 ` [PATCH v1 2/2] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
2014-03-12 8:06 ` [PATCH v1 0/2] disable rings " Chris Wilson
2014-03-12 8:12 ` Chris Wilson
2014-03-12 11:09 ` [PATCH v2 0/3] " naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 1/3] drm/i915: disable rings before HW status page setup naresh.kumar.kachhi
2014-03-12 11:09 ` [PATCH v2 2/3] drm/i915: wait for rings to become idle once disabled naresh.kumar.kachhi
2014-03-12 11:18 ` Chris Wilson
2014-03-12 11:09 ` [PATCH v2 3/3] drm/i915: warn if ring is active before sync flush naresh.kumar.kachhi
2014-03-12 11:28 ` [PATCH v2 0/3] disable rings " Chris Wilson
2014-03-12 15:07 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).