Re: [PATCH 1/3] drm/i915: Implement Selective Write workaround for Haswell GT1

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> From: Ben Widawsky <ben@bwidawsk.net>
>
> The docs specify this needs to be set on HSW GT1 parts. I've implemented it as
> such since it should only be needed when using RC6, but it can probably go
> anywhere.
>
> This patch fixes extremely reproducible hangs on our Jenkins setup.
>
> The interesting failure signature is:
>   IPEHR: 0x780c0000 (3DSTATE_VF)
>   INSTDONE_0: 0xffdfbffa (SVG + VS)
>
> This replaces the homebrew workaround we implemented in
>
> commit 2c550183476dfa25641309ae9a28d30feed14379
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Dec 16 10:02:27 2014 +0000
>
>     drm/i915: Disable PSMI sleep messages on all rings around context switches
>
> as that is coupled into HW semaphore support which is scheduled for
> removal in the next patch.
>
> References: 2c550183476d ("drm/i915: Disable PSMI sleep messages on all rings around context switches")
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +
>  drivers/gpu/drm/i915/i915_reg.h         |  7 ++++
>  drivers/gpu/drm/i915/intel_pm.c         | 16 +++++++-
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 49 ++-----------------------
>  4 files changed, 27 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d44255a8655e..24a5e63cc443 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2276,6 +2276,8 @@ intel_info(const struct drm_i915_private *dev_priv)
>  				 (dev_priv)->info.gt == 3)
>  #define IS_HSW_ULT(dev_priv)	(IS_HASWELL(dev_priv) && \
>  				 (INTEL_DEVID(dev_priv) & 0xFF00) == 0x0A00)
> +#define IS_HSW_GT1(dev_priv)	(IS_HASWELL(dev_priv) && \
> +				 (dev_priv)->info.gt == 1)
>  #define IS_HSW_GT3(dev_priv)	(IS_HASWELL(dev_priv) && \
>  				 (dev_priv)->info.gt == 3)
>  /* ULX machines are also considered ULT. */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 02af9b5add34..ca6a2e925194 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2464,6 +2464,13 @@ enum i915_power_well_id {
>  #define RING_DMA_FADD_UDW(base)	_MMIO((base) + 0x60) /* gen8+ */
>  #define RING_INSTPM(base)	_MMIO((base) + 0xc0)
>  #define RING_MI_MODE(base)	_MMIO((base) + 0x9c)
> +#define RING_WAIT_FOR_RC6_EXIT(base)	_MMIO((base) + 0xcc)
> +#define   RING_RC6_SEL_WRITE_ADDR_MASK		(0x7 << 4)
> +#define   RING_RC6_SEL_WRITE_ADDR_MULTICAST	(0x0 << 4)
> +#define   RING_RC6_SEL_WRITE_ADDR_UPPER_LEFT	(0x4 << 4)
> +#define   RING_RC6_SEL_WRITE_ADDR_UPPER_RIGHT	(0x5 << 4)
> +#define   RING_RC6_SEL_WRITE_ADDR_LOWER_LEFT	(0x6 << 4)
> +#define   RING_RC6_SEL_WRITE_ADDR_LOWER_RIGHT	(0x7 << 4)
>  #define INSTPS		_MMIO(0x2070) /* 965+ only */
>  #define GEN4_INSTDONE1	_MMIO(0x207c) /* 965+ only, aka INSTDONE_2 on SNB */
>  #define ACTHD_I965	_MMIO(0x2074)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2a6ffb8b975a..c2e3502090c6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7122,9 +7122,23 @@ static void gen6_enable_rc6(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
>  	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
>  
> -	for_each_engine(engine, dev_priv, id)
> +	for_each_engine(engine, dev_priv, id) {
>  		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
>  
> +		/*
> +		 * HSW GT1: "This field must be always [be] programmed to “100”,
> +		 * this is required to address know [sic] HW issue."
> +		 *
> +		 * Failure to do so leads to a gpu hang on context load from
> +		 * under rc6, with a characteristic IPEHR of 0x780c0000 (the
> +		 * last command from the context image).
> +		 */
> +		if (IS_HSW_GT1(dev_priv))
> +			I915_WRITE(RING_WAIT_FOR_RC6_EXIT(engine->mmio_base),
> +				   _MASKED_FIELD(RING_RC6_SEL_WRITE_ADDR_MASK,
> +						 RING_RC6_SEL_WRITE_ADDR_UPPER_LEFT));
> +	}
> +
>  	I915_WRITE(GEN6_RC_SLEEP, 0);
>  	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
>  	if (IS_IVYBRIDGE(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 65fd92eb071d..57f20033b19d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1602,12 +1602,6 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
>  {
>  	struct drm_i915_private *i915 = rq->i915;
>  	struct intel_engine_cs *engine = rq->engine;
> -	enum intel_engine_id id;
> -	const int num_rings =
> -		/* Use an extended w/a on gen7 if signalling from other rings */
> -		(HAS_LEGACY_SEMAPHORES(i915) && IS_GEN(i915, 7)) ?
> -		INTEL_INFO(i915)->num_rings - 1 :
> -		0;
>  	bool force_restore = false;
>  	int len;
>  	u32 *cs;
> @@ -1621,7 +1615,7 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
>  
>  	len = 4;
>  	if (IS_GEN(i915, 7))
> -		len += 2 + (num_rings ? 4*num_rings + 6 : 0);
> +		len += 2;
>  	if (flags & MI_FORCE_RESTORE) {
>  		GEM_BUG_ON(flags & MI_RESTORE_INHIBIT);
>  		flags &= ~MI_FORCE_RESTORE;
> @@ -1634,23 +1628,8 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
>  		return PTR_ERR(cs);
>  
>  	/* WaProgramMiArbOnOffAroundMiSetContext:ivb,vlv,hsw,bdw,chv */
> -	if (IS_GEN(i915, 7)) {
> +	if (IS_GEN(i915, 7))
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_DISABLE;
> -		if (num_rings) {
> -			struct intel_engine_cs *signaller;
> -
> -			*cs++ = MI_LOAD_REGISTER_IMM(num_rings);
> -			for_each_engine(signaller, i915, id) {
> -				if (signaller == engine)
> -					continue;
> -
> -				*cs++ = i915_mmio_reg_offset(
> -					   RING_PSMI_CTL(signaller->mmio_base));
> -				*cs++ = _MASKED_BIT_ENABLE(
> -						GEN6_PSMI_SLEEP_MSG_DISABLE);
> -			}
> -		}
> -	}
>  
>  	if (force_restore) {
>  		/*
> @@ -1681,30 +1660,8 @@ static inline int mi_set_context(struct i915_request *rq, u32 flags)
>  	 */
>  	*cs++ = MI_NOOP;
>  
> -	if (IS_GEN(i915, 7)) {
> -		if (num_rings) {
> -			struct intel_engine_cs *signaller;
> -			i915_reg_t last_reg = {}; /* keep gcc quiet */
> -
> -			*cs++ = MI_LOAD_REGISTER_IMM(num_rings);
> -			for_each_engine(signaller, i915, id) {
> -				if (signaller == engine)
> -					continue;
> -
> -				last_reg = RING_PSMI_CTL(signaller->mmio_base);
> -				*cs++ = i915_mmio_reg_offset(last_reg);
> -				*cs++ = _MASKED_BIT_DISABLE(
> -						GEN6_PSMI_SLEEP_MSG_DISABLE);
> -			}
> -
> -			/* Insert a delay before the next switch! */
> -			*cs++ = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
> -			*cs++ = i915_mmio_reg_offset(last_reg);
> -			*cs++ = i915_scratch_offset(rq->i915);
> -			*cs++ = MI_NOOP;
> -		}
> +	if (IS_GEN(i915, 7))
>  		*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
> -	}
>  
>  	intel_ring_advance(rq, cs);
>  
> -- 
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx