Re: [PATCH 2/7] drm/i915: Introduce per-engine workarounds

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Nov 30, 2018 at 05:44:07PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> We stopped re-applying the GT workarounds after engine reset since commit
> 59b449d5c82a ("drm/i915: Split out functions for different kinds of
> workarounds").
> 
> Issue with this is that some of the GT workarounds live in the MMIO space
> which gets lost during engine resets. So far the registers in 0x2xxx and
> 0xbxxx address range have been identified to be affected.
> 
> This losing of applied workarounds has obvious negative effects and can
> even lead to hard system hangs (see the linked Bugzilla).
> 
> Rather than just restoring this re-application, because we have also
> observed that it is not safe to just re-write all GT workarounds after
> engine resets (GPU might be live and weird hardware states can happen),
> we introduce a new class of per-engine workarounds and move only the
> affected GT workarounds over.
> 
> Using the framework introduced in the previous patch, we therefore after
> engine reset, re-apply only the workarounds living in the affected MMIO
> address ranges.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Bugzilla: https://bugzilla.freedesktop.org/show_bug.cgi?id=107945
> Fixes: 59b449d5c82a ("drm/i915: Split out functions for different kinds of workarounds")
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/i915/intel_engine_cs.c   |   2 +
>  drivers/gpu/drm/i915/intel_lrc.c         |   4 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +
>  drivers/gpu/drm/i915/intel_workarounds.c | 249 +++++++++++++----------
>  drivers/gpu/drm/i915/intel_workarounds.h |   3 +
>  5 files changed, 148 insertions(+), 112 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 759c0fd58f8c..ef5d202e9d45 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -723,6 +723,8 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  	__intel_context_unpin(i915->kernel_context, engine);
>  
>  	i915_timeline_fini(&engine->timeline);
> +
> +	intel_wa_list_free(&engine->wa_list);
>  }
>  
>  u64 intel_engine_get_active_head(const struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 11f4e6148557..dfafc3f710d6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1617,6 +1617,8 @@ static bool unexpected_starting_state(struct intel_engine_cs *engine)
>  
>  static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  {
> +	intel_engine_workarounds_apply(engine);
> +
>  	intel_mocs_init_engine(engine);
>  
>  	intel_engine_reset_breadcrumbs(engine);
> @@ -2314,6 +2316,8 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
>  			  ret);
>  	}
>  
> +	intel_engine_workarounds_init(engine);
> +
>  	return 0;
>  
>  err_cleanup_common:
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 8a2270b209b0..c5ff3d31cab7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -15,6 +15,7 @@
>  #include "i915_selftest.h"
>  #include "i915_timeline.h"
>  #include "intel_gpu_commands.h"
> +#include "intel_workarounds.h"
>  
>  struct drm_printer;
>  struct i915_sched_attr;
> @@ -451,6 +452,7 @@ struct intel_engine_cs {
>  
>  	struct intel_hw_status_page status_page;
>  	struct i915_ctx_workarounds wa_ctx;
> +	struct i915_wa_list wa_list;
>  	struct i915_vma *scratch;
>  
>  	u32             irq_keep_mask; /* always keep these interrupts */
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index ff20ebf9e040..be63a2af3481 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -653,17 +653,6 @@ static void gen9_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_wa_list *wal = &dev_priv->gt_wa_list;
>  
> -	/* WaContextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk,cfl */
> -	wa_masked_en(wal,
> -		     GEN9_CSFE_CHICKEN1_RCS,
> -		     GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE);
> -
> -
> -	/* WaEnableLbsSlaRetryTimerDecrement:skl,bxt,kbl,glk,cfl */
> -	wa_write_or(wal,
> -		    BDW_SCRATCH1,
> -		    GEN9_LBS_SLA_RETRY_TIMER_DECREMENT_ENABLE);
> -
>  	/* WaDisableKillLogic:bxt,skl,kbl */
>  	if (!IS_COFFEELAKE(dev_priv))
>  		wa_write_or(wal,
> @@ -685,24 +674,6 @@ static void gen9_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  	wa_write_or(wal,
>  		    GAM_ECOCHK,
>  		    BDW_DISABLE_HDC_INVALIDATION);
> -
> -	/* WaProgramL3SqcReg1DefaultForPerf:bxt,glk */
> -	if (IS_GEN9_LP(dev_priv))
> -		wa_write_masked_or(wal,
> -				   GEN8_L3SQCREG1,
> -				   L3_PRIO_CREDITS_MASK,
> -				   L3_GENERAL_PRIO_CREDITS(62) |
> -				   L3_HIGH_PRIO_CREDITS(2));
> -
> -	/* WaOCLCoherentLineFlush:skl,bxt,kbl,cfl */
> -	wa_write_or(wal,
> -		    GEN8_L3SQCREG4,
> -		    GEN8_LQSC_FLUSH_COHERENT_LINES);
> -
> -	/* WaEnablePreemptionGranularityControlByUMD:skl,bxt,kbl,cfl,[cnl] */
> -	wa_masked_en(wal,
> -		     GEN7_FF_SLICE_CS_CHICKEN1,
> -		     GEN9_FFSC_PERCTX_PREEMPT_CTRL);
>  }
>  
>  static void skl_gt_workarounds_init(struct drm_i915_private *dev_priv)
> @@ -711,11 +682,6 @@ static void skl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  
>  	gen9_gt_workarounds_init(dev_priv);
>  
> -	/* WaEnableGapsTsvCreditFix:skl */
> -	wa_write_or(wal,
> -		    GEN8_GARBCNTL,
> -		    GEN9_GAPS_TSV_CREDIT_DISABLE);
> -
>  	/* WaDisableGafsUnitClkGating:skl */
>  	wa_write_or(wal,
>  		    GEN7_UCGCTL4,
> @@ -734,11 +700,6 @@ static void bxt_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  
>  	gen9_gt_workarounds_init(dev_priv);
>  
> -	/* WaDisablePooledEuLoadBalancingFix:bxt */
> -	wa_masked_en(wal,
> -		     FF_SLICE_CS_CHICKEN2,
> -		     GEN9_POOLED_EU_LOAD_BALANCING_FIX_DISABLE);
> -
>  	/* WaInPlaceDecompressionHang:bxt */
>  	wa_write_or(wal,
>  		    GEN9_GAMT_ECO_REG_RW_IA,
> @@ -751,11 +712,6 @@ static void kbl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  
>  	gen9_gt_workarounds_init(dev_priv);
>  
> -	/* WaEnableGapsTsvCreditFix:kbl */
> -	wa_write_or(wal,
> -		    GEN8_GARBCNTL,
> -		    GEN9_GAPS_TSV_CREDIT_DISABLE);
> -
>  	/* WaDisableDynamicCreditSharing:kbl */
>  	if (IS_KBL_REVID(dev_priv, 0, KBL_REVID_B0))
>  		wa_write_or(wal,
> @@ -771,21 +727,6 @@ static void kbl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  	wa_write_or(wal,
>  		    GEN9_GAMT_ECO_REG_RW_IA,
>  		    GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> -
> -	/* WaKBLVECSSemaphoreWaitPoll:kbl */
> -	if (IS_KBL_REVID(dev_priv, KBL_REVID_A0, KBL_REVID_E0)) {
> -		struct intel_engine_cs *engine;
> -		unsigned int tmp;
> -
> -		for_each_engine(engine, dev_priv, tmp) {
> -			if (engine->id == RCS)
> -				continue;
> -
> -			wa_write(wal,
> -				 RING_SEMA_WAIT_POLL(engine->mmio_base),
> -				 1);
> -		}
> -	}
>  }
>  
>  static void glk_gt_workarounds_init(struct drm_i915_private *dev_priv)
> @@ -799,11 +740,6 @@ static void cfl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  
>  	gen9_gt_workarounds_init(dev_priv);
>  
> -	/* WaEnableGapsTsvCreditFix:cfl */
> -	wa_write_or(wal,
> -		    GEN8_GARBCNTL,
> -		    GEN9_GAPS_TSV_CREDIT_DISABLE);
> -
>  	/* WaDisableGafsUnitClkGating:cfl */
>  	wa_write_or(wal,
>  		    GEN7_UCGCTL4,
> @@ -894,11 +830,6 @@ static void cnl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  	wa_write_or(wal,
>  		    GEN9_GAMT_ECO_REG_RW_IA,
>  		    GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
> -
> -	/* WaEnablePreemptionGranularityControlByUMD:cnl */
> -	wa_masked_en(wal,
> -		     GEN7_FF_SLICE_CS_CHICKEN1,
> -		     GEN9_FFSC_PERCTX_PREEMPT_CTRL);
>  }
>  
>  static void icl_gt_workarounds_init(struct drm_i915_private *dev_priv)
> @@ -907,53 +838,17 @@ static void icl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  
>  	wa_init_mcr(dev_priv);
>  
> -	/* This is not an Wa. Enable for better image quality */
> -	wa_masked_en(wal,
> -		    _3D_CHICKEN3,
> -		    _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
> -
>  	/* WaInPlaceDecompressionHang:icl */
>  	wa_write_or(wal,
>  		    GEN9_GAMT_ECO_REG_RW_IA,
>  		    GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
>  
> -	/* WaPipelineFlushCoherentLines:icl */
> -	wa_write_or(wal,
> -		    GEN8_L3SQCREG4,
> -		    GEN8_LQSC_FLUSH_COHERENT_LINES);
> -
> -	/* Wa_1405543622:icl
> -	 * Formerly known as WaGAPZPriorityScheme
> -	 */
> -	wa_write_or(wal,
> -		    GEN8_GARBCNTL,
> -		    GEN11_ARBITRATION_PRIO_ORDER_MASK);
> -
> -	/* Wa_1604223664:icl
> -	 * Formerly known as WaL3BankAddressHashing
> -	 */
> -	wa_write_masked_or(wal,
> -			   GEN8_GARBCNTL,
> -			   GEN11_HASH_CTRL_EXCL_MASK,
> -			   GEN11_HASH_CTRL_EXCL_BIT0);
> -	wa_write_masked_or(wal,
> -			   GEN11_GLBLINVL,
> -			   GEN11_BANK_HASH_ADDR_EXCL_MASK,
> -			   GEN11_BANK_HASH_ADDR_EXCL_BIT0);
> -
>  	/* WaModifyGamTlbPartitioning:icl */
>  	wa_write_masked_or(wal,
>  			   GEN11_GACB_PERF_CTRL,
>  			   GEN11_HASH_CTRL_MASK,
>  			   GEN11_HASH_CTRL_BIT0 | GEN11_HASH_CTRL_BIT4);
>  
> -	/* Wa_1405733216:icl
> -	 * Formerly known as WaDisableCleanEvicts
> -	 */
> -	wa_write_or(wal,
> -		    GEN8_L3SQCREG4,
> -		    GEN11_LQSC_CLEAN_EVICT_DISABLE);
> -
>  	/* Wa_1405766107:icl
>  	 * Formerly known as WaCL2SFHalfMaxAlloc
>  	 */
> @@ -986,13 +881,6 @@ static void icl_gt_workarounds_init(struct drm_i915_private *dev_priv)
>  			    INF_UNIT_LEVEL_CLKGATE,
>  			    CGPSF_CLKGATE_DIS);
>  
> -	/* WaForwardProgressSoftReset:icl */
> -	wa_write_or(wal,
> -		    GEN10_SCRATCH_LNCF2,
> -		    PMFLUSHDONE_LNICRSDROP |
> -		    PMFLUSH_GAPL3UNBLOCK |
> -		    PMFLUSHDONE_LNEBLK);
> -
>  	/* Wa_1406463099:icl
>  	 * Formerly known as WaGamTlbPendError
>  	 */
> @@ -1242,6 +1130,143 @@ void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine)
>  	whitelist_apply(engine, whitelist_build(engine, &w));
>  }
>  
> +static void rcs_engine_wa_init(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *i915 = engine->i915;
> +	struct i915_wa_list *wal = &engine->wa_list;
> +
> +	if (IS_GEN9(i915)) {
> +		/* WaContextSwitchWithConcurrentTLBInvalidate:skl,bxt,kbl,glk,cfl */
> +		wa_masked_en(wal,
> +			     GEN9_CSFE_CHICKEN1_RCS,
> +			     GEN9_PREEMPT_GPGPU_SYNC_SWITCH_DISABLE);
> +
> +		/* WaEnableLbsSlaRetryTimerDecrement:skl,bxt,kbl,glk,cfl */
> +		wa_write_or(wal,
> +			    BDW_SCRATCH1,
> +			    GEN9_LBS_SLA_RETRY_TIMER_DECREMENT_ENABLE);
> +
> +		/* WaProgramL3SqcReg1DefaultForPerf:bxt,glk */
> +		if (IS_GEN9_LP(i915))
> +			wa_write_masked_or(wal,
> +					   GEN8_L3SQCREG1,
> +					   L3_PRIO_CREDITS_MASK,
> +					   L3_GENERAL_PRIO_CREDITS(62) |
> +					   L3_HIGH_PRIO_CREDITS(2));
> +
> +		/* WaOCLCoherentLineFlush:skl,bxt,kbl,cfl */
> +		wa_write_or(wal,
> +			    GEN8_L3SQCREG4,
> +			    GEN8_LQSC_FLUSH_COHERENT_LINES);
> +	}
> +
> +	if (IS_BROXTON(i915)) {
> +		/* WaDisablePooledEuLoadBalancingFix:bxt */
> +		wa_masked_en(wal,
> +			     FF_SLICE_CS_CHICKEN2,
> +			     GEN9_POOLED_EU_LOAD_BALANCING_FIX_DISABLE);
> +	}
> +
> +	if (IS_SKYLAKE(i915) || IS_KABYLAKE(i915) || IS_COFFEELAKE(i915)) {
> +		/* WaEnableGapsTsvCreditFix:skl,kbl,cfl */
> +		wa_write_or(wal,
> +			    GEN8_GARBCNTL,
> +			    GEN9_GAPS_TSV_CREDIT_DISABLE);
> +	}
> +
> +	if (IS_GEN9(i915) || IS_CANNONLAKE(i915)) {
> +		/* WaEnablePreemptionGranularityControlByUMD:skl,bxt,kbl,cfl,cnl */
> +		wa_masked_en(wal,
> +			     GEN7_FF_SLICE_CS_CHICKEN1,
> +			     GEN9_FFSC_PERCTX_PREEMPT_CTRL);
> +	}
> +
> +	if (IS_ICELAKE(i915)) {

Could we make newer platform coming first?

anyway:

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +		/* This is not an Wa. Enable for better image quality */
> +		wa_masked_en(wal,
> +			     _3D_CHICKEN3,
> +			     _3D_CHICKEN3_AA_LINE_QUALITY_FIX_ENABLE);
> +
> +		/* WaPipelineFlushCoherentLines:icl */
> +		wa_write_or(wal,
> +			    GEN8_L3SQCREG4,
> +			    GEN8_LQSC_FLUSH_COHERENT_LINES);
> +
> +		/*
> +		 * Wa_1405543622:icl
> +		 * Formerly known as WaGAPZPriorityScheme
> +		 */
> +		wa_write_or(wal,
> +			    GEN8_GARBCNTL,
> +			    GEN11_ARBITRATION_PRIO_ORDER_MASK);
> +
> +		/*
> +		 * Wa_1604223664:icl
> +		 * Formerly known as WaL3BankAddressHashing
> +		 */
> +		wa_write_masked_or(wal,
> +				   GEN8_GARBCNTL,
> +				   GEN11_HASH_CTRL_EXCL_MASK,
> +				   GEN11_HASH_CTRL_EXCL_BIT0);
> +		wa_write_masked_or(wal,
> +				   GEN11_GLBLINVL,
> +				   GEN11_BANK_HASH_ADDR_EXCL_MASK,
> +				   GEN11_BANK_HASH_ADDR_EXCL_BIT0);
> +
> +		/*
> +		 * Wa_1405733216:icl
> +		 * Formerly known as WaDisableCleanEvicts
> +		 */
> +		wa_write_or(wal,
> +			    GEN8_L3SQCREG4,
> +			    GEN11_LQSC_CLEAN_EVICT_DISABLE);
> +
> +		/* WaForwardProgressSoftReset:icl */
> +		wa_write_or(wal,
> +			    GEN10_SCRATCH_LNCF2,
> +			    PMFLUSHDONE_LNICRSDROP |
> +			    PMFLUSH_GAPL3UNBLOCK |
> +			    PMFLUSHDONE_LNEBLK);
> +	}
> +}
> +
> +static void xcs_engine_wa_init(struct intel_engine_cs *engine)
> +{
> +	struct drm_i915_private *i915 = engine->i915;
> +	struct i915_wa_list *wal = &engine->wa_list;
> +
> +	if (IS_KABYLAKE(i915)) {
> +		/* WaKBLVECSSemaphoreWaitPoll:kbl */
> +		if (IS_KBL_REVID(i915, KBL_REVID_A0, KBL_REVID_E0)) {
> +			wa_write(wal,
> +				 RING_SEMA_WAIT_POLL(engine->mmio_base),
> +				 1);
> +		}
> +	}
> +}
> +
> +void intel_engine_workarounds_init(struct intel_engine_cs *engine)
> +{
> +	struct i915_wa_list *wal = &engine->wa_list;
> +
> +	if (GEM_WARN_ON(INTEL_GEN(engine->i915) < 8))
> +		return;
> +
> +	wa_init_start(wal, engine->name);
> +
> +	if (engine->id == RCS)
> +		rcs_engine_wa_init(engine);
> +	else
> +		xcs_engine_wa_init(engine);
> +
> +	wa_init_finish(wal);
> +}
> +
> +void intel_engine_workarounds_apply(struct intel_engine_cs *engine)
> +{
> +	wa_list_apply(engine->i915, &engine->wa_list);
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/intel_workarounds.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.h b/drivers/gpu/drm/i915/intel_workarounds.h
> index 64aed9cf0200..2998767d51ca 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.h
> +++ b/drivers/gpu/drm/i915/intel_workarounds.h
> @@ -36,4 +36,7 @@ void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv);
>  
>  void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
>  
> +void intel_engine_workarounds_init(struct intel_engine_cs *engine);
> +void intel_engine_workarounds_apply(struct intel_engine_cs *engine);
> +
>  #endif
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx