Re: [PATCH v2 1/3] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

[Oscar Mateo Lozano <oscar.mateo@xxxxxxxxx>]

On 5/18/2018 11:12 AM, Yunwei Zhang wrote:
> WaProgramMgsrForCorrectSliceSpecificMmioReads dictate that before any MMIO
> read into Slice/Subslice specific registers, MCR packet control
> register(0xFDC) needs to be programmed to point to any enabled
> slice/subslice pair. Otherwise, incorrect value will be returned.
>
> However, that means each subsequent MMIO read will be forwarded to a
> specific slice/subslice combination as read is unicast. This is OK since
> slice/subslice specific register values are consistent in almost all cases
> across slice/subslice. There are rare occasions such as INSTDONE that this
> value will be dependent on slice/subslice combo, in such cases, we need to
> program 0xFDC and recover this after. This is already covered by
> read_subslice_reg.
>
> Also, 0xFDC will lose its information after TDR/engine reset/power state
> change.
>
> References: HSD#1405586840, BSID#0575
>
> v2:
>   - use fls() instead of find_last_bit() (Chris)
>   - added INTEL_SSEU to extract sseu from device info. (Chris)
> v3:
>   - rebase on latest tip
> v5:
>   - Added references (Mika)
>   - Change the ordered of passing arguments and etc. (Ursulin)
> v7:
>   - Moved WA explanation Comments(Oscar)
>   - Rebased.
> v8:
>   - Renamed sanitize_mcr to calculate_s_ss_select. (Oscar)
>   - calculate s/ss selector instead of whole mcr. (Oscar)
>
> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Signed-off-by: Yunwei Zhang <yunwei.zhang@xxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>   drivers/gpu/drm/i915/intel_engine_cs.c   | 33 +++++++++++++++++++++++++++-----
>   drivers/gpu/drm/i915/intel_workarounds.c | 32 +++++++++++++++++++++++++++++++
>   3 files changed, 62 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e33c380..e697785 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2744,6 +2744,8 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
>   int intel_engines_init_mmio(struct drm_i915_private *dev_priv);
>   int intel_engines_init(struct drm_i915_private *dev_priv);
>   
> +u32 intel_calculate_s_ss_select(struct drm_i915_private *dev_priv);
> +

Maybe intel_calculate_mcr_s_ss_select is beter?



>   /* intel_hotplug.c */
>   void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
>   			   u32 pin_mask, u32 long_mask);
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 26f9f8a..511975f 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -819,12 +819,32 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
>   	}
>   }
>   
> +u32 intel_calculate_s_ss_select(struct drm_i915_private *dev_priv)
> +{
> +	const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
> +	u32 mcr_s_ss_mask;
> +	u32 mcr_s_ss_select;
> +	u32 slice = fls(sseu->slice_mask);
> +	u32 subslice = fls(sseu->subslice_mask[slice]);
> +
> +	mcr_s_ss_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;

Do we need this for anything?

> +
> +	if (INTEL_GEN(dev_priv) == 10)
> +		mcr_s_ss_select = GEN8_MCR_SLICE(slice) |
> +				  GEN8_MCR_SUBSLICE(subslice);
> +	else
> +		mcr_s_ss_select = 0;
> +
> +	return mcr_s_ss_select;
> +}
> +
>   static inline uint32_t
>   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   		  int subslice, i915_reg_t reg)
>   {
>   	uint32_t mcr_slice_subslice_mask;
>   	uint32_t mcr_slice_subslice_select;
> +	uint32_t default_mcr_s_ss_select;
>   	uint32_t mcr;
>   	uint32_t ret;
>   	enum forcewake_domains fw_domains;
> @@ -841,6 +861,8 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   					    GEN8_MCR_SUBSLICE(subslice);
>   	}
>   
> +	default_mcr_s_ss_select = intel_calculate_s_ss_select(dev_priv);
> +
>   	fw_domains = intel_uncore_forcewake_for_reg(dev_priv, reg,
>   						    FW_REG_READ);
>   	fw_domains |= intel_uncore_forcewake_for_reg(dev_priv,
> @@ -851,11 +873,10 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   	intel_uncore_forcewake_get__locked(dev_priv, fw_domains);
>   
>   	mcr = I915_READ_FW(GEN8_MCR_SELECTOR);
> -	/*
> -	 * The HW expects the slice and sublice selectors to be reset to 0
> -	 * after reading out the registers.
> -	 */
> -	WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
> +
> +	WARN_ON_ONCE((mcr & mcr_slice_subslice_mask) !=
> +		     default_mcr_s_ss_select);
> +
>   	mcr &= ~mcr_slice_subslice_mask;
>   	mcr |= mcr_slice_subslice_select;
>   	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
> @@ -863,6 +884,8 @@ read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   	ret = I915_READ_FW(reg);
>   
>   	mcr &= ~mcr_slice_subslice_mask;
> +	mcr |= default_mcr_s_ss_select;
> +
>   	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>   
>   	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
> index 2df3538..d814368 100644
> --- a/drivers/gpu/drm/i915/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
> @@ -672,8 +672,40 @@ static void cfl_gt_workarounds_apply(struct drm_i915_private *dev_priv)
>   		   GAMT_ECO_ENABLE_IN_PLACE_DECOMPRESS);
>   }
>   
> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
> +{
> +	u32 mcr;
> +	u32 mcr_slice_subslice_mask;
> +
> +	mcr = I915_READ(GEN8_MCR_SELECTOR);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> +					  GEN11_MCR_SUBSLICE_MASK;
> +	else
> +		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> +					  GEN8_MCR_SUBSLICE_MASK;
> +	/*
> +	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl
> +	 * Before any MMIO read into slice/subslice specific registers, MCR
> +	 * packet control register needs to be programmed to point to any
> +	 * enabled s/ss pair. Otherwise, incorrect values will be returned.
> +	 * This means each subsequent MMIO read will be forwarded to an
> +	 * specific s/ss combination, but this is OK since these registers
> +	 * are consistent across s/ss in almost all cases. In the rare
> +	 * occasions, such as INSTDONE, where this value is dependent
> +	 * on s/ss combo, the read should be done with read_subslice_reg.
> +	 */
> +	mcr &= ~mcr_slice_subslice_mask;
> +	mcr |= intel_calculate_s_ss_select(dev_priv);
> +	I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> +}
> +
>   static void cnl_gt_workarounds_apply(struct drm_i915_private *dev_priv)
>   {
> +	/* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */

I don't like duplicating the WA name. It's probably better to choose one 
place and stick with it (same thing for the L3Bank WA)

> +	wa_init_mcr(dev_priv);
> +
>   	/* WaDisableI2mCycleOnWRPort:cnl (pre-prod) */
>   	if (IS_CNL_REVID(dev_priv, CNL_REVID_B0, CNL_REVID_B0))
>   		I915_WRITE(GAMT_CHKN_BIT_REG,

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx