[Intel-gfx] [PATCH v8 1/2] drm/i915: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

[Oscar Mateo]

On 4/17/2018 2:05 PM, 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:
>   - Rebased.
> v8:
>   - Reviewed by Oscar
>   - Store default MCR value instead of calculate on the run. (Oscar)
>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Signed-off-by: Yunwei Zhang <yunwei.zhang at intel.com>
> Reviewed-by: Oscar Mateo <oscar.mateo at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_device_info.c | 33 ++++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/intel_device_info.h |  3 +++
>   drivers/gpu/drm/i915/intel_engine_cs.c   | 14 +++++++++++---
>   3 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index a32ba72..2243a23 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -719,6 +719,36 @@ static u32 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>   	return 0;
>   }
>   
> +static void wa_init_mcr(struct intel_device_info *info)

mcr_sanitize?

> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(info, struct drm_i915_private, info);
> +	u32 mcr;
> +	u32 mcr_slice_subslice_mask;
> +	u32 mcr_slice_subslice_select;
> +	u32 slice = fls(info->sseu.slice_mask);
> +	u32 subslice = fls(info->sseu.subslice_mask[slice]);
> +
> +	if (INTEL_GEN(dev_priv) >= 11) {
> +		mcr_slice_subslice_mask = GEN11_MCR_SLICE_MASK |
> +					  GEN11_MCR_SUBSLICE_MASK;
> +		mcr_slice_subslice_select = GEN11_MCR_SLICE(slice) |
> +						GEN11_MCR_SUBSLICE(subslice);
> +	} else {
> +		mcr_slice_subslice_mask = GEN8_MCR_SLICE_MASK |
> +					  GEN8_MCR_SUBSLICE_MASK;
> +		mcr_slice_subslice_select = GEN8_MCR_SLICE(slice) |
> +						GEN8_MCR_SUBSLICE(subslice);
> +	}
> +
> +	mcr = I915_READ(GEN8_MCR_SELECTOR);
> +	mcr &= ~mcr_slice_subslice_mask;

Until here you are not applying any WA, only sanitizing what the MCR 
contains. The real WA is in the two following lines. That's where the 
WaProgramMgsrForCorrectSliceSpecificMmioReads label should be (and maybe 
a small comment noting that we are selecting a kind of random 
slice/subslice combination to make sure MMIO reads in a certaing range 
are valid).

> +	if (INTEL_GEN(dev_priv) >= 10)
> +		mcr |= mcr_slice_subslice_select;
> +
> +	info->mcr = mcr;

And now you also want to write the HW register back, otherwise you are 
not applying the WA!

> +}
> +
>   /**
>    * intel_device_info_runtime_init - initialize runtime info
>    * @info: intel device info struct
> @@ -851,6 +881,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
>   	else if (INTEL_INFO(dev_priv)->gen >= 11)
>   		gen11_sseu_info_init(dev_priv);
>   
> +	/* WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl */
> +	wa_init_mcr(info);
> +
>   	/* Initialize command stream timestamp frequency */
>   	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 933e316..5449a15 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -176,6 +176,9 @@ struct intel_device_info {
>   	/* Slice/subslice/EU info */
>   	struct sseu_dev_info sseu;
>   
> +	/* MCR packet control */
> +	u32 mcr;

Use a better name for this, like default_mcr. I'll explain why in a 
second...

> +
>   	u32 cs_timestamp_frequency_khz;
>   
>   	struct color_luts {
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 1a83707..08798f2 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -831,18 +831,26 @@ 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.
> +	 * before GEN10 or to a enabled s/ss post GEN10 after reading out the
> +	 * registers.
>   	 */
> -	WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
> +	WARN_ON_ONCE(mcr != dev_priv->info.mcr);

... if you call it default_mcr, you can skip the comment completely. 
It's very clear you are just making sure the MCR has the expected value.

>   	mcr &= ~mcr_slice_subslice_mask;
>   	mcr |= mcr_slice_subslice_select;
>   	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>   
>   	ret = I915_READ_FW(reg);
>   
> -	mcr &= ~mcr_slice_subslice_mask;
> +	/*
> +	 * WaProgramMgsrForCorrectSliceSpecificMmioReads:cnl,icl

No need for this label here, because this is not were the WA should be 
applied!! Hopefully, you already applied it before now.

> +	 * expects mcr to be programed to a enabled slice/subslice pair
> +	 * before any MMIO read into slice/subslice register
> +	 */
> +	mcr = dev_priv->info.mcr;

Same here: with a better naming, it's very clear you are just setting 
the MCR to the default value, so there is no need for any extra comment 
(and remove the following blank line, to show this goes together with 
the I915_WRITE_FW).

> +
>   	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>   
>   	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);