Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Wed, Mar 26, 2014 at 05:09:53PM -0700, Ben Widawsky wrote:
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
v3: Use a WARN_ON rather than loop indefinitely on an unstable
register read.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 13 -
drivers/gpu/drm/i915/i915_gpu_error.c |2 +-
drivers/gpu/drm/i915/i915_irq.c |8 +---
drivers/gpu/drm/i915/i915_reg.h |1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 22 --
drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++---
6 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 5418253..4c09fb2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
@@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private
*dev_priv, int fw_engine);
#define I915_WRITE64(reg, val)
dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
#define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv,
(reg), true)
+#define I915_READ64_2x32(lower_reg, upper_reg) ({ \
+ u32 upper = I915_READ(upper_reg); \
+ u32 lower = I915_READ(lower_reg); \
+ u32 tmp = I915_READ(upper_reg); \
+ if (upper != tmp) { \
+ upper = tmp;\
+ lower = I915_READ(lower_reg); \
+ WARN_ON(I915_READ(upper_reg) != upper); \
+ } \
+ (u64)upper 32 | lower; })
+
May as well get the most recent value of upper:
WARN_ON((tmp = I915_READ(upper_reg)) != upper);
}
return (u64)tmp 32 | lower;
with or without:
Reviewed-by: Ben Widawsky b...@bwidawsk.net
I've punted on this and pulled this (and the follow-up polish patch in).
#define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg)
#define POSTING_READ16(reg)(void)I915_READ16_NOTRACE(reg)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16..9519aa2 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6..9caae98 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
{
struct drm_i915_private *dev_priv = ring-dev-dev_private;
- u32 cmd, ipehr, acthd, acthd_min;
+ u64 acthd, acthd_min;
+ u32 cmd, ipehr;
With a little conflict here - this hunk seems spurious.
Thanks, Daniel
ipehr = I915_READ(RING_IPEHR(ring-mmio_base));
if ((ipehr ~(0x3 16)) !=
@@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct
drm_i915_private *dev_priv)
}
static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Wed, Mar 26, 2014 at 05:09:53PM -0700, Ben Widawsky wrote:
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
+#define I915_READ64_2x32(lower_reg, upper_reg) ({ \
+ u32 upper = I915_READ(upper_reg); \
+ u32 lower = I915_READ(lower_reg); \
+ u32 tmp = I915_READ(upper_reg); \
+ if (upper != tmp) { \
+ upper = tmp;\
+ lower = I915_READ(lower_reg); \
+ WARN_ON(I915_READ(upper_reg) != upper); \
+ } \
+ (u64)upper 32 | lower; })
+
May as well get the most recent value of upper:
WARN_ON((tmp = I915_READ(upper_reg)) != upper);
}
return (u64)tmp 32 | lower;
Bleh, I thought if the WARN ever fires, the result is so unstable that
it really doesn't matter what we return, or we go full-loop.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
v3: Use a WARN_ON rather than loop indefinitely on an unstable
register read.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 13 -
drivers/gpu/drm/i915/i915_gpu_error.c |2 +-
drivers/gpu/drm/i915/i915_irq.c |8 +---
drivers/gpu/drm/i915/i915_reg.h |1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 22 --
drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++---
6 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5418253..4c09fb2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
@@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private
*dev_priv, int fw_engine);
#define I915_WRITE64(reg, val)
dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
#define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv,
(reg), true)
+#define I915_READ64_2x32(lower_reg, upper_reg) ({\
+ u32 upper = I915_READ(upper_reg); \
+ u32 lower = I915_READ(lower_reg); \
+ u32 tmp = I915_READ(upper_reg); \
+ if (upper != tmp) { \
+ upper = tmp;\
+ lower = I915_READ(lower_reg); \
+ WARN_ON(I915_READ(upper_reg) != upper); \
+ } \
+ (u64)upper 32 | lower; })
+
May as well get the most recent value of upper:
WARN_ON((tmp = I915_READ(upper_reg)) != upper);
}
return (u64)tmp 32 | lower;
with or without:
Reviewed-by: Ben Widawsky b...@bwidawsk.net
#define POSTING_READ(reg)(void)I915_READ_NOTRACE(reg)
#define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16..9519aa2 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6..9caae98 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
{
struct drm_i915_private *dev_priv = ring-dev-dev_private;
- u32 cmd, ipehr, acthd, acthd_min;
+ u64 acthd, acthd_min;
+ u32 cmd, ipehr;
ipehr = I915_READ(RING_IPEHR(ring-mmio_base));
if ((ipehr ~(0x3 16)) !=
@@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct
drm_i915_private *dev_priv)
}
static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
+ring_stuck(struct intel_ring_buffer *ring, u64 acthd)
{
struct drm_device *dev = ring-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data)
return;
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Mon, Mar 24, 2014 at 07:43:48PM -0700, Ben Widawsky wrote:
On Mon, Mar 24, 2014 at 07:41:17PM -0700, Ben Widawsky wrote:
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
v3: Use a WARN_ON rather than loop indefinitely on an unstable
register read.
I truly feel bad for saying this at v3, but we can probably simplify
this. Unless I am missing something, we only actually care about the
value of acthd in:
if (ring-hangcheck.acthd != acthd)
return HANGCHECK_ACTIVE;
I think therefore it would be safe to make hangcheck.acthd a u64.
Compare the lower dword. If they're not equal, then we're done. If they
are equal, compare the UDW. If UDW doesn't match, we're done. If UDW
does match, do another read of the LDW and call it good:
ring_stuck(u32 acthd)
{
if (lower_32_bits(ring-hangcheck.acthd) != acthd)
return HANGCHECK_ACTIVE;
else if (upper_32_bits(ring-hangcheck.acthd) !=
I915_READ(ACTHD_UDW))
return HANGCHECK_ACTIVE
else if (acthd != I915_READ(RING_ACTHD))
return HANGCHECK_ACTIVE;
}
After writing that, I'm not really sure how much less complex it is, but I
think it gets the same job done. Potentially there is some MMIO savings,
but I'd guess it to be negligible.
Jesse, please request ACTHD is expanded to a proper 64b register for
gen1.
Right after I sent that, I realized that doesn't provide for potentially
corrupting ring-hangcheck.acthd. So I am back to thinking this method
is better.
Plus having read64_2x32 should make it easier to dtrt next time.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
v3: Use a WARN_ON rather than loop indefinitely on an unstable
register read.
I truly feel bad for saying this at v3, but we can probably simplify
this. Unless I am missing something, we only actually care about the
value of acthd in:
if (ring-hangcheck.acthd != acthd)
return HANGCHECK_ACTIVE;
I think therefore it would be safe to make hangcheck.acthd a u64.
Compare the lower dword. If they're not equal, then we're done. If they
are equal, compare the UDW. If UDW doesn't match, we're done. If UDW
does match, do another read of the LDW and call it good:
ring_stuck(u32 acthd)
{
if (lower_32_bits(ring-hangcheck.acthd) != acthd)
return HANGCHECK_ACTIVE;
else if (upper_32_bits(ring-hangcheck.acthd) !=
I915_READ(ACTHD_UDW))
return HANGCHECK_ACTIVE
else if (acthd != I915_READ(RING_ACTHD))
return HANGCHECK_ACTIVE;
}
After writing that, I'm not really sure how much less complex it is, but I
think it gets the same job done. Potentially there is some MMIO savings,
but I'd guess it to be negligible.
Jesse, please request ACTHD is expanded to a proper 64b register for
gen1.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 13 -
drivers/gpu/drm/i915/i915_gpu_error.c |2 +-
drivers/gpu/drm/i915/i915_irq.c |8 +---
drivers/gpu/drm/i915/i915_reg.h |1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 22 --
drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++---
6 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5418253..4c09fb2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
@@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private
*dev_priv, int fw_engine);
#define I915_WRITE64(reg, val)
dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
#define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv,
(reg), true)
+#define I915_READ64_2x32(lower_reg, upper_reg) ({\
+ u32 upper = I915_READ(upper_reg); \
+ u32 lower = I915_READ(lower_reg); \
+ u32 tmp = I915_READ(upper_reg); \
+ if (upper != tmp) { \
+ upper = tmp;\
+ lower = I915_READ(lower_reg); \
+ WARN_ON(I915_READ(upper_reg) != upper); \
+ } \
+ (u64)upper 32 | lower; })
+
#define POSTING_READ(reg)(void)I915_READ_NOTRACE(reg)
#define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16..9519aa2 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6..9caae98 100644
---
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Mon, Mar 24, 2014 at 07:41:17PM -0700, Ben Widawsky wrote:
On Fri, Mar 21, 2014 at 12:41:53PM +, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
v3: Use a WARN_ON rather than loop indefinitely on an unstable
register read.
I truly feel bad for saying this at v3, but we can probably simplify
this. Unless I am missing something, we only actually care about the
value of acthd in:
if (ring-hangcheck.acthd != acthd)
return HANGCHECK_ACTIVE;
I think therefore it would be safe to make hangcheck.acthd a u64.
Compare the lower dword. If they're not equal, then we're done. If they
are equal, compare the UDW. If UDW doesn't match, we're done. If UDW
does match, do another read of the LDW and call it good:
ring_stuck(u32 acthd)
{
if (lower_32_bits(ring-hangcheck.acthd) != acthd)
return HANGCHECK_ACTIVE;
else if (upper_32_bits(ring-hangcheck.acthd) !=
I915_READ(ACTHD_UDW))
return HANGCHECK_ACTIVE
else if (acthd != I915_READ(RING_ACTHD))
return HANGCHECK_ACTIVE;
}
After writing that, I'm not really sure how much less complex it is, but I
think it gets the same job done. Potentially there is some MMIO savings,
but I'd guess it to be negligible.
Jesse, please request ACTHD is expanded to a proper 64b register for
gen1.
Right after I sent that, I realized that doesn't provide for potentially
corrupting ring-hangcheck.acthd. So I am back to thinking this method
is better.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 13 -
drivers/gpu/drm/i915/i915_gpu_error.c |2 +-
drivers/gpu/drm/i915/i915_irq.c |8 +---
drivers/gpu/drm/i915/i915_reg.h |1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 22 --
drivers/gpu/drm/i915/intel_ringbuffer.h |6 +++---
6 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 5418253..4c09fb2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
@@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private
*dev_priv, int fw_engine);
#define I915_WRITE64(reg, val)
dev_priv-uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
#define I915_READ64(reg) dev_priv-uncore.funcs.mmio_readq(dev_priv,
(reg), true)
+#define I915_READ64_2x32(lower_reg, upper_reg) ({ \
+ u32 upper = I915_READ(upper_reg); \
+ u32 lower = I915_READ(lower_reg); \
+ u32 tmp = I915_READ(upper_reg); \
+ if (upper != tmp) { \
+ upper = tmp;\
+ lower = I915_READ(lower_reg); \
+ WARN_ON(I915_READ(upper_reg) != upper); \
+ } \
+ (u64)upper 32 | lower; })
+
#define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg)
#define POSTING_READ16(reg)(void)I915_READ16_NOTRACE(reg)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16..9519aa2 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
err_printf(m, IPEIR: 0x%08x\n,
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On 03/20/2014 09:48 PM, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 8 +---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 30 --
drivers/gpu/drm/i915/intel_ringbuffer.h | 6 +++---
6 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 54182536dc46..d5a4a14d6723 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16ead0a..9519aa240614 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
I thought conclusion elsewhere in the thread for this was to include all
64-bits in the output?
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6af185..9caae9840c78 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
{
struct drm_i915_private *dev_priv = ring-dev-dev_private;
- u32 cmd, ipehr, acthd, acthd_min;
+ u64 acthd, acthd_min;
+ u32 cmd, ipehr;
ipehr = I915_READ(RING_IPEHR(ring-mmio_base));
if ((ipehr ~(0x3 16)) !=
@@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct
drm_i915_private *dev_priv)
}
static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
+ring_stuck(struct intel_ring_buffer *ring, u64 acthd)
{
struct drm_device *dev = ring-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data)
return;
for_each_ring(ring, dev_priv, i) {
- u32 seqno, acthd;
+ u64 acthd;
+ u32 seqno;
bool busy = true;
semaphore_clear_deadlocks(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f010ff7e7e2a..3c464d307a2b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -708,6 +708,7 @@ enum punit_power_well {
#define BLT_HWS_PGA_GEN7 (0x04280)
#define VEBOX_HWS_PGA_GEN7(0x04380)
#define RING_ACTHD(base) ((base)+0x74)
+#define RING_ACTHD_UDW(base) ((base)+0x5c)
#define RING_NOPID(base) ((base)+0x94)
#define RING_IMR(base)((base)+0xa8)
#define RING_TIMESTAMP(base) ((base)+0x358)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7a01911c16f8..45d8011e5a6c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -417,13 +417,28 @@ static void ring_write_tail(struct intel_ring_buffer
*ring,
I915_WRITE_TAIL(ring, value);
}
-u32 intel_ring_get_active_head(struct intel_ring_buffer *ring)
+u64 intel_ring_get_active_head(struct intel_ring_buffer *ring)
{
drm_i915_private_t *dev_priv = ring-dev-dev_private;
- u32
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 10:03:38AM +, Tvrtko Ursulin wrote:
On 03/20/2014 09:48 PM, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 8 +---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 30 --
drivers/gpu/drm/i915/intel_ringbuffer.h | 6 +++---
6 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 54182536dc46..d5a4a14d6723 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
-u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16ead0a..9519aa240614 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
-err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
I thought conclusion elsewhere in the thread for this was to include
all 64-bits in the output?
Eh? They are... There's a second patch to change the way we print 64bit
values.
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6af185..9caae9840c78 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
{
struct drm_i915_private *dev_priv = ring-dev-dev_private;
-u32 cmd, ipehr, acthd, acthd_min;
+u64 acthd, acthd_min;
+u32 cmd, ipehr;
ipehr = I915_READ(RING_IPEHR(ring-mmio_base));
if ((ipehr ~(0x3 16)) !=
@@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct
drm_i915_private *dev_priv)
}
static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
+ring_stuck(struct intel_ring_buffer *ring, u64 acthd)
{
struct drm_device *dev = ring-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data)
return;
for_each_ring(ring, dev_priv, i) {
-u32 seqno, acthd;
+u64 acthd;
+u32 seqno;
bool busy = true;
semaphore_clear_deadlocks(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index f010ff7e7e2a..3c464d307a2b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -708,6 +708,7 @@ enum punit_power_well {
#define BLT_HWS_PGA_GEN7 (0x04280)
#define VEBOX_HWS_PGA_GEN7 (0x04380)
#define RING_ACTHD(base) ((base)+0x74)
+#define RING_ACTHD_UDW(base)((base)+0x5c)
#define RING_NOPID(base) ((base)+0x94)
#define RING_IMR(base) ((base)+0xa8)
#define RING_TIMESTAMP(base) ((base)+0x358)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7a01911c16f8..45d8011e5a6c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -417,13 +417,28 @@ static void ring_write_tail(struct intel_ring_buffer
*ring,
I915_WRITE_TAIL(ring, value);
}
-u32 intel_ring_get_active_head(struct intel_ring_buffer
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On 03/21/2014 10:14 AM, Chris Wilson wrote:
On Fri, Mar 21, 2014 at 10:03:38AM +, Tvrtko Ursulin wrote:
On 03/20/2014 09:48 PM, Chris Wilson wrote:
As Broadwell has an increased virtual address size, it requires more
than 32 bits to store offsets into its address space. This includes the
debug registers to track the current HEAD of the individual rings, which
may be anywhere within the per-process address spaces. In order to find
the full location, we need to read the high bits from a second register.
We then also need to expand our storage to keep track of the larger
address.
v2: Carefully read the two registers to catch wraparound between
the reads.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ben Widawsky benjamin.widaw...@intel.com
Cc: Timo Aaltonen tjaal...@ubuntu.com
Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 8 +---
drivers/gpu/drm/i915/i915_reg.h | 1 +
drivers/gpu/drm/i915/intel_ringbuffer.c | 30 --
drivers/gpu/drm/i915/intel_ringbuffer.h | 6 +++---
6 files changed, 35 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 54182536dc46..d5a4a14d6723 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -354,12 +354,12 @@ struct drm_i915_error_state {
u32 ipeir;
u32 ipehr;
u32 instdone;
- u32 acthd;
u32 bbstate;
u32 instpm;
u32 instps;
u32 seqno;
u64 bbaddr;
+ u64 acthd;
u32 fault_reg;
u32 faddr;
u32 rc_psmi; /* sleep state */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index b153a16ead0a..9519aa240614 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -248,7 +248,7 @@ static void i915_ring_error_state(struct
drm_i915_error_state_buf *m,
err_printf(m, TAIL: 0x%08x\n, ring-tail);
err_printf(m, CTL: 0x%08x\n, ring-ctl);
err_printf(m, HWS: 0x%08x\n, ring-hws);
- err_printf(m, ACTHD: 0x%08x\n, ring-acthd);
+ err_printf(m, ACTHD: 0x%08llx\n, ring-acthd);
I thought conclusion elsewhere in the thread for this was to include
all 64-bits in the output?
Eh? They are... There's a second patch to change the way we print 64bit
values.
Ok, missed that.
err_printf(m, IPEIR: 0x%08x\n, ring-ipeir);
err_printf(m, IPEHR: 0x%08x\n, ring-ipehr);
err_printf(m, INSTDONE: 0x%08x\n, ring-instdone);
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 77dbef6af185..9caae9840c78 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
{
struct drm_i915_private *dev_priv = ring-dev-dev_private;
- u32 cmd, ipehr, acthd, acthd_min;
+ u64 acthd, acthd_min;
+ u32 cmd, ipehr;
ipehr = I915_READ(RING_IPEHR(ring-mmio_base));
if ((ipehr ~(0x3 16)) !=
@@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct
drm_i915_private *dev_priv)
}
static enum intel_ring_hangcheck_action
-ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
+ring_stuck(struct intel_ring_buffer *ring, u64 acthd)
{
struct drm_device *dev = ring-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data)
return;
for_each_ring(ring, dev_priv, i) {
- u32 seqno, acthd;
+ u64 acthd;
+ u32 seqno;
bool busy = true;
semaphore_clear_deadlocks(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f010ff7e7e2a..3c464d307a2b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -708,6 +708,7 @@ enum punit_power_well {
#define BLT_HWS_PGA_GEN7 (0x04280)
#define VEBOX_HWS_PGA_GEN7(0x04380)
#define RING_ACTHD(base) ((base)+0x74)
+#define RING_ACTHD_UDW(base) ((base)+0x5c)
#define RING_NOPID(base) ((base)+0x94)
#define RING_IMR(base)((base)+0xa8)
#define RING_TIMESTAMP(base) ((base)+0x358)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7a01911c16f8..45d8011e5a6c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -417,13 +417,28 @@ static void ring_write_tail(struct intel_ring_buffer
*ring,
I915_WRITE_TAIL(ring, value);
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On Fri, Mar 21, 2014 at 10:50:05AM +, Tvrtko Ursulin wrote:
No, think you misunderstood me. I said slightly more defensive
just in the sense that in case of weird hardware failures you have a
potentially infinite loop now, where you don't really need a loop -
probabilities strongly suggest you cannot get two upper dword wraps
between the reads. So it is enough to read the upper dword twice,
without the loop. Same effect, slightly more defensive in reality.
Yup, misunderstood what you wanted. If in doubt, C is much more
concise ;-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 45d8011..8c82316 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -425,12 +425,14 @@ u64 intel_ring_get_active_head(struct intel_ring_buffer
*ring)
if (INTEL_INFO(ring-dev)-gen = 8) {
u32 upper, lower, tmp;
+ upper = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
+ lower = I915_READ(RING_ACTHD(ring-mmio_base));
tmp = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
- do {
+ if (upper != tmp) {
upper = tmp;
lower = I915_READ(RING_ACTHD(ring-mmio_base));
- tmp = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
- } while (upper != tmp);
+ WARN_ON(I915_READ(RING_ACTHD_UDW(ring-mmio_base) !=
upper);
+ }
acthd = (u64)upper 32 | lower;
} else if (INTEL_INFO(ring-dev)-gen = 4)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Broadwell expands ACTHD to 64bit
On 03/21/2014 12:00 PM, Chris Wilson wrote:
On Fri, Mar 21, 2014 at 10:50:05AM +, Tvrtko Ursulin wrote:
No, think you misunderstood me. I said slightly more defensive
just in the sense that in case of weird hardware failures you have a
potentially infinite loop now, where you don't really need a loop -
probabilities strongly suggest you cannot get two upper dword wraps
between the reads. So it is enough to read the upper dword twice,
without the loop. Same effect, slightly more defensive in reality.
Yup, misunderstood what you wanted. If in doubt, C is much more
concise ;-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 45d8011..8c82316 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -425,12 +425,14 @@ u64 intel_ring_get_active_head(struct intel_ring_buffer
*ring)
if (INTEL_INFO(ring-dev)-gen = 8) {
u32 upper, lower, tmp;
+ upper = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
+ lower = I915_READ(RING_ACTHD(ring-mmio_base));
tmp = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
- do {
+ if (upper != tmp) {
upper = tmp;
lower = I915_READ(RING_ACTHD(ring-mmio_base));
- tmp = I915_READ(RING_ACTHD_UDW(ring-mmio_base));
- } while (upper != tmp);
+ WARN_ON(I915_READ(RING_ACTHD_UDW(ring-mmio_base) !=
upper);
+ }
acthd = (u64)upper 32 | lower;
} else if (INTEL_INFO(ring-dev)-gen = 4)
Yes, I was just uneasy with the loop. Also Ben's suggestion in case of
wrap was I think:
WARN_ON(I915_READ(RING_ACTHD(ring-mmio_base) = lower);
Or in other words, if we have observed the upper wrap, check that the
lower matches with that observation. But I feel bad now that we are
over-engineering this. Perhaps these WARNs are just silly.
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
