* [PATCH] drm/i915: fix forcewake counts for gen8
@ 2014-02-18 15:48 Mika Kuoppala
2014-02-18 16:42 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Mika Kuoppala @ 2014-02-18 15:48 UTC (permalink / raw)
To: intel-gfx; +Cc: miku, Ben Widawsky
Generic driver code gets forcewake explicitly by gen6_gt_force_wake_get(),
which keeps force wake counts before accessing low level fw get.
However the underlying gen8 register write function access low level
accessors directly. This leads to nested fw get from hardware, causing
forcewake ack clear errors and/or hangs.
Fix this by checking the forcewake count in gen8 accessors.
Also implement read accessors for gen8 to gain symmetry for
shadowed register access.
References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 74 +++++++++++++++++++++++------------
1 file changed, 49 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c628414..089feaa 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -498,6 +498,45 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
REG_READ_FOOTER; \
}
+static const u32 gen8_shadowed_regs[] = {
+ FORCEWAKE_MT,
+ GEN6_RPNSWREQ,
+ GEN6_RC_VIDEO_FREQ,
+ RING_TAIL(RENDER_RING_BASE),
+ RING_TAIL(GEN6_BSD_RING_BASE),
+ RING_TAIL(VEBOX_RING_BASE),
+ RING_TAIL(BLT_RING_BASE),
+ /* TODO: Other registers are not yet used */
+};
+
+static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
+{
+ int i;
+ for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++)
+ if (reg == gen8_shadowed_regs[i])
+ return true;
+
+ return false;
+}
+
+#define __gen8_read(x) \
+static u##x \
+gen8_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
+ bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
+ REG_READ_HEADER(x); \
+ __needs_put &= dev_priv->uncore.forcewake_count == 0; \
+ if (__needs_put) { \
+ dev_priv->uncore.funcs.force_wake_get(dev_priv, \
+ FORCEWAKE_ALL); \
+ } \
+ val = __raw_i915_read##x(dev_priv, reg); \
+ if (__needs_put) { \
+ dev_priv->uncore.funcs.force_wake_put(dev_priv, \
+ FORCEWAKE_ALL); \
+ } \
+ REG_READ_FOOTER; \
+}
+
#define __vlv_read(x) \
static u##x \
vlv_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
@@ -531,6 +570,10 @@ __vlv_read(8)
__vlv_read(16)
__vlv_read(32)
__vlv_read(64)
+__gen8_read(8)
+__gen8_read(16)
+__gen8_read(32)
+__gen8_read(64)
__gen6_read(8)
__gen6_read(16)
__gen6_read(32)
@@ -545,6 +588,7 @@ __gen4_read(32)
__gen4_read(64)
#undef __vlv_read
+#undef __gen8_read
#undef __gen6_read
#undef __gen5_read
#undef __gen4_read
@@ -610,32 +654,12 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
REG_WRITE_FOOTER; \
}
-static const u32 gen8_shadowed_regs[] = {
- FORCEWAKE_MT,
- GEN6_RPNSWREQ,
- GEN6_RC_VIDEO_FREQ,
- RING_TAIL(RENDER_RING_BASE),
- RING_TAIL(GEN6_BSD_RING_BASE),
- RING_TAIL(VEBOX_RING_BASE),
- RING_TAIL(BLT_RING_BASE),
- /* TODO: Other registers are not yet used */
-};
-
-static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
-{
- int i;
- for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++)
- if (reg == gen8_shadowed_regs[i])
- return true;
-
- return false;
-}
-
#define __gen8_write(x) \
static void \
gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
REG_WRITE_HEADER; \
+ __needs_put &= dev_priv->uncore.forcewake_count == 0; \
if (__needs_put) { \
dev_priv->uncore.funcs.force_wake_get(dev_priv, \
FORCEWAKE_ALL); \
@@ -734,10 +758,10 @@ void intel_uncore_init(struct drm_device *dev)
dev_priv->uncore.funcs.mmio_writew = gen8_write16;
dev_priv->uncore.funcs.mmio_writel = gen8_write32;
dev_priv->uncore.funcs.mmio_writeq = gen8_write64;
- dev_priv->uncore.funcs.mmio_readb = gen6_read8;
- dev_priv->uncore.funcs.mmio_readw = gen6_read16;
- dev_priv->uncore.funcs.mmio_readl = gen6_read32;
- dev_priv->uncore.funcs.mmio_readq = gen6_read64;
+ dev_priv->uncore.funcs.mmio_readb = gen8_read8;
+ dev_priv->uncore.funcs.mmio_readw = gen8_read16;
+ dev_priv->uncore.funcs.mmio_readl = gen8_read32;
+ dev_priv->uncore.funcs.mmio_readq = gen8_read64;
break;
case 7:
case 6:
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: fix forcewake counts for gen8
2014-02-18 15:48 [PATCH] drm/i915: fix forcewake counts for gen8 Mika Kuoppala
@ 2014-02-18 16:42 ` Ville Syrjälä
2014-02-18 17:10 ` [PATCH] drm/i915: Fix " Mika Kuoppala
0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2014-02-18 16:42 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx, miku, Ben Widawsky
On Tue, Feb 18, 2014 at 05:48:54PM +0200, Mika Kuoppala wrote:
> Generic driver code gets forcewake explicitly by gen6_gt_force_wake_get(),
> which keeps force wake counts before accessing low level fw get.
> However the underlying gen8 register write function access low level
> accessors directly. This leads to nested fw get from hardware, causing
> forcewake ack clear errors and/or hangs.
>
> Fix this by checking the forcewake count in gen8 accessors.
> Also implement read accessors for gen8 to gain symmetry for
> shadowed register access.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 74 +++++++++++++++++++++++------------
> 1 file changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c628414..089feaa 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -498,6 +498,45 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> REG_READ_FOOTER; \
> }
>
> +static const u32 gen8_shadowed_regs[] = {
> + FORCEWAKE_MT,
> + GEN6_RPNSWREQ,
> + GEN6_RC_VIDEO_FREQ,
> + RING_TAIL(RENDER_RING_BASE),
> + RING_TAIL(GEN6_BSD_RING_BASE),
> + RING_TAIL(VEBOX_RING_BASE),
> + RING_TAIL(BLT_RING_BASE),
> + /* TODO: Other registers are not yet used */
> +};
> +
> +static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
> +{
> + int i;
> + for (i = 0; i < ARRAY_SIZE(gen8_shadowed_regs); i++)
> + if (reg == gen8_shadowed_regs[i])
> + return true;
> +
> + return false;
> +}
> +
> +#define __gen8_read(x) \
> +static u##x \
> +gen8_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
> + bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
This isn't right. Shadowed registers still need forcewake for reads.
> + REG_READ_HEADER(x); \
> + __needs_put &= dev_priv->uncore.forcewake_count == 0; \
This looks a bit funky. I'm not sure I understood the issue correctly,
but it looks like just a failure to obey forcewake_count in gen8_write.
So I'd just fix gen8_write. Something like this:
#define __gen8_write(x) \
static void \
gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
- bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
REG_WRITE_HEADER; \
- if (__needs_put) { \
- dev_priv->uncore.funcs.force_wake_get(dev_priv, \
- FORCEWAKE_ALL); \
- } \
- __raw_i915_write##x(dev_priv, reg, val); \
- if (__needs_put) { \
- dev_priv->uncore.funcs.force_wake_put(dev_priv, \
- FORCEWAKE_ALL); \
+ if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
+ if (dev_priv->uncore.forcewake_count == 0) \
+ dev_priv->uncore.funcs.force_wake_get(dev_priv, \
+ FORCEWAKE_ALL); \
+ __raw_i915_write##x(dev_priv, reg, val); \
+ if (dev_priv->uncore.forcewake_count == 0) \
+ dev_priv->uncore.funcs.force_wake_put(dev_priv, \
+ FORCEWAKE_ALL); \
+ } else { \
+ __raw_i915_write##x(dev_priv, reg, val); \
} \
REG_WRITE_FOOTER; \
}
I also noticed that gen6 doesn't increment/decrement the forcewake_count
here. We can follow that rule here too since we have the uncore lock
around the whole operation. I see the VLV code has the ++/--. I think we
should either add them ++/-- everywhere, or remove them from the VLV
code, just to make the code more uniform.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] drm/i915: Fix forcewake counts for gen8
2014-02-18 16:42 ` Ville Syrjälä
@ 2014-02-18 17:10 ` Mika Kuoppala
2014-02-18 18:21 ` Ben Widawsky
0 siblings, 1 reply; 5+ messages in thread
From: Mika Kuoppala @ 2014-02-18 17:10 UTC (permalink / raw)
To: intel-gfx; +Cc: miku, Ben Widawsky
Sometimes generic driver code gets forcewake explicitly by
gen6_gt_force_wake_get(), which check forcewake_count before accessing
hardware. However the register access with gen8_write function access
low level hw accessors directly, ignoring the forcewake_count. This
leads to nested forcewake get from hardware, in ring init and possibly
elsewhere, causing forcewake ack clear errors and/or hangs.
Fix this by checking the forcewake count also in gen8_write
v2: Read side doesn't care about shadowed registers,
Remove __needs_put funkiness from gen8_write. (Ville)
Improved commit message.
References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c628414..d1e9d63 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -634,16 +634,17 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
#define __gen8_write(x) \
static void \
gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
- bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
REG_WRITE_HEADER; \
- if (__needs_put) { \
- dev_priv->uncore.funcs.force_wake_get(dev_priv, \
- FORCEWAKE_ALL); \
- } \
- __raw_i915_write##x(dev_priv, reg, val); \
- if (__needs_put) { \
- dev_priv->uncore.funcs.force_wake_put(dev_priv, \
- FORCEWAKE_ALL); \
+ if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
+ if (dev_priv->uncore.forcewake_count == 0) \
+ dev_priv->uncore.funcs.force_wake_get(dev_priv, \
+ FORCEWAKE_ALL); \
+ __raw_i915_write##x(dev_priv, reg, val); \
+ if (dev_priv->uncore.forcewake_count == 0) \
+ dev_priv->uncore.funcs.force_wake_put(dev_priv, \
+ FORCEWAKE_ALL); \
+ } else { \
+ __raw_i915_write##x(dev_priv, reg, val); \
} \
REG_WRITE_FOOTER; \
}
--
1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Fix forcewake counts for gen8
2014-02-18 17:10 ` [PATCH] drm/i915: Fix " Mika Kuoppala
@ 2014-02-18 18:21 ` Ben Widawsky
2014-02-18 19:08 ` Mika Kuoppala
0 siblings, 1 reply; 5+ messages in thread
From: Ben Widawsky @ 2014-02-18 18:21 UTC (permalink / raw)
To: Mika Kuoppala; +Cc: intel-gfx, miku
On Tue, Feb 18, 2014 at 07:10:24PM +0200, Mika Kuoppala wrote:
> Sometimes generic driver code gets forcewake explicitly by
> gen6_gt_force_wake_get(), which check forcewake_count before accessing
> hardware. However the register access with gen8_write function access
> low level hw accessors directly, ignoring the forcewake_count. This
> leads to nested forcewake get from hardware, in ring init and possibly
> elsewhere, causing forcewake ack clear errors and/or hangs.
>
> Fix this by checking the forcewake count also in gen8_write
>
> v2: Read side doesn't care about shadowed registers,
> Remove __needs_put funkiness from gen8_write. (Ville)
> Improved commit message.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Thanks for finding this. I'd been meaning to track down the extra "Timed
out" messages. I do wonder how this actually fixes a hang. Do you have
any idea?
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c628414..d1e9d63 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -634,16 +634,17 @@ static bool is_gen8_shadowed(struct drm_i915_private *dev_priv, u32 reg)
> #define __gen8_write(x) \
> static void \
> gen8_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
> - bool __needs_put = reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg); \
> REG_WRITE_HEADER; \
> - if (__needs_put) { \
> - dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> - FORCEWAKE_ALL); \
> - } \
> - __raw_i915_write##x(dev_priv, reg, val); \
> - if (__needs_put) { \
> - dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> - FORCEWAKE_ALL); \
> + if (reg < 0x40000 && !is_gen8_shadowed(dev_priv, reg)) { \
> + if (dev_priv->uncore.forcewake_count == 0) \
> + dev_priv->uncore.funcs.force_wake_get(dev_priv, \
> + FORCEWAKE_ALL); \
> + __raw_i915_write##x(dev_priv, reg, val); \
> + if (dev_priv->uncore.forcewake_count == 0) \
> + dev_priv->uncore.funcs.force_wake_put(dev_priv, \
> + FORCEWAKE_ALL); \
> + } else { \
> + __raw_i915_write##x(dev_priv, reg, val); \
> } \
> REG_WRITE_FOOTER; \
> }
> --
> 1.7.9.5
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915: Fix forcewake counts for gen8
2014-02-18 18:21 ` Ben Widawsky
@ 2014-02-18 19:08 ` Mika Kuoppala
0 siblings, 0 replies; 5+ messages in thread
From: Mika Kuoppala @ 2014-02-18 19:08 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx, miku
Ben Widawsky <benjamin.widawsky@intel.com> writes:
> On Tue, Feb 18, 2014 at 07:10:24PM +0200, Mika Kuoppala wrote:
>> Sometimes generic driver code gets forcewake explicitly by
>> gen6_gt_force_wake_get(), which check forcewake_count before accessing
>> hardware. However the register access with gen8_write function access
>> low level hw accessors directly, ignoring the forcewake_count. This
>> leads to nested forcewake get from hardware, in ring init and possibly
>> elsewhere, causing forcewake ack clear errors and/or hangs.
>>
>> Fix this by checking the forcewake count also in gen8_write
>>
>> v2: Read side doesn't care about shadowed registers,
>> Remove __needs_put funkiness from gen8_write. (Ville)
>> Improved commit message.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=74007
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
Double signed-off slipped in it seems.
> Thanks for finding this. I'd been meaning to track down the extra "Timed
> out" messages. I do wonder how this actually fixes a hang. Do you have
> any idea?
No clear idea. I just suspect that the two writes into FORCEWAKE_MT
without proper put/ack dance in between upsets the gpu.
Interestingly with init it hangs like 1 out of 2 times,
but after a gpu reset, it hangs always. The spot is always
the same: I915_WRITE(mmio, (u32)ring->status_page.gfx_addr);
-Mika
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-18 19:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-18 15:48 [PATCH] drm/i915: fix forcewake counts for gen8 Mika Kuoppala
2014-02-18 16:42 ` Ville Syrjälä
2014-02-18 17:10 ` [PATCH] drm/i915: Fix " Mika Kuoppala
2014-02-18 18:21 ` Ben Widawsky
2014-02-18 19:08 ` Mika Kuoppala
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox