Re: [PATCH] drm/i915/vlv: Take forcewake on media engine writes

From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Deepak S <deepak.s@intel.com>
Subject: Re: [PATCH] drm/i915/vlv: Take forcewake on media engine writes
Date: Thu, 17 Dec 2015 07:26:15 -0800	[thread overview]
Message-ID: <5672D417.1050307@virtuousgeek.org> (raw)
In-Reply-To: <1450365294-4804-1-git-send-email-mika.kuoppala@intel.com>

On 12/17/2015 07:14 AM, Mika Kuoppala wrote:
> Since commit 940aece471bd ("drm/i915/vlv: Valleyview support
> for forcewake Individual power wells.") we have only taken
> media engine forcewake correctly on reads, but only taken render
> engine forcewake on media engine writes and omitted the media
> domain.
> 
> This asymmetry might have caused unstable behaviour on
> media ring access.
> 
> Fix is to take media engine forcewake symmetrically to writes.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=88012
> Cc: Deepak S <deepak.s@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 277e60ae0e47..a2e204088aa5 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -902,6 +902,23 @@ hsw_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool t
>  	GEN6_WRITE_FOOTER; \
>  }
>  
> +#define __vlv_write(x) \
> +static void \
> +vlv_write##x(struct drm_i915_private *dev_priv, i915_reg_t reg, u##x val, bool trace) { \
> +	enum forcewake_domains fw_engine = 0; \
> +	GEN6_WRITE_HEADER; \
> +	if (!NEEDS_FORCE_WAKE(offset)) \
> +		fw_engine = 0; \
> +	else if (FORCEWAKE_VLV_RENDER_RANGE_OFFSET(offset)) \
> +		fw_engine = FORCEWAKE_RENDER; \
> +	else if (FORCEWAKE_VLV_MEDIA_RANGE_OFFSET(offset)) \
> +		fw_engine = FORCEWAKE_MEDIA; \
> +	if (fw_engine) \
> +		__force_wake_get(dev_priv, fw_engine); \
> +	__raw_i915_write##x(dev_priv, reg, val); \
> +	GEN6_WRITE_FOOTER; \
> +}
> +
>  static const i915_reg_t gen8_shadowed_regs[] = {
>  	FORCEWAKE_MT,
>  	GEN6_RPNSWREQ,
> @@ -1019,6 +1036,10 @@ __gen8_write(8)
>  __gen8_write(16)
>  __gen8_write(32)
>  __gen8_write(64)
> +__vlv_write(8)
> +__vlv_write(16)
> +__vlv_write(32)
> +__vlv_write(64)
>  __hsw_write(8)
>  __hsw_write(16)
>  __hsw_write(32)
> @@ -1031,6 +1052,7 @@ __gen6_write(64)
>  #undef __gen9_write
>  #undef __chv_write
>  #undef __gen8_write
> +#undef __vlv_write
>  #undef __hsw_write
>  #undef __gen6_write
>  #undef GEN6_WRITE_FOOTER
> @@ -1243,6 +1265,8 @@ void intel_uncore_init(struct drm_device *dev)
>  	case 6:
>  		if (IS_HASWELL(dev)) {
>  			ASSIGN_WRITE_MMIO_VFUNCS(hsw);
> +		} else if (IS_VALLEYVIEW(dev)) {
> +			ASSIGN_WRITE_MMIO_VFUNCS(vlv);
>  		} else {
>  			ASSIGN_WRITE_MMIO_VFUNCS(gen6);
>  		}
> 

Looks good.  Looks like we also have it on chv, so I guess it was just
an oversight.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx