Re: [PATCH] drm/i915: fix forcewake counts for gen8

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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