diff mbox

[v4,1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

Message ID 1522080721-7029-1-git-send-email-yunwei.zhang@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Yunwei Zhang March 26, 2018, 4:12 p.m. UTC
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.

Reference: 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
v4:
 - Added references (Mika)

Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  1 +
 drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 2 deletions(-)

Comments

Tvrtko Ursulin March 26, 2018, 4:57 p.m. UTC | #1
On 26/03/2018 17:12, 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.
> 
> Reference: 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
> v4:
>   - Added references (Mika)
> 
> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h        |  1 +
>   drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++--
>   2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 800230b..2db2a04 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2297,6 +2297,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>   
>   #define INTEL_GEN(dev_priv)	((dev_priv)->info.gen)
>   #define INTEL_DEVID(dev_priv)	((dev_priv)->info.device_id)
> +#define INTEL_SSEU(dev_priv)	((dev_priv)->info.sseu)

If we add this someone gets the job of converting existing users?

>   
>   #define REVID_FOREVER		0xff
>   #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index de09fa4..cc19e0a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
>   	}
>   }
>   
> +static u32 calculate_mcr(u32 mcr, struct drm_i915_private *dev_priv)

dev_priv first would be more typical function argument order.

> +{
> +	const struct sseu_dev_info *sseu = &(INTEL_SSEU(dev_priv));
> +	u32 slice = fls(sseu->slice_mask);
> +	u32 subslice = fls(sseu->subslice_mask[slice]);
> +
> +	mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
> +	mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> +
> +	return mcr;
> +}
> +
> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
> +{
> +	u32 mcr;
> +
> +	mcr = I915_READ(GEN8_MCR_SELECTOR);
> +	mcr = calculate_mcr(mcr, dev_priv);
> +	I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> +}
> +
>   static inline uint32_t
>   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>   		  int subslice, i915_reg_t reg)
> @@ -828,18 +849,29 @@ 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);
> +	if (INTEL_GEN(dev_priv) < 10)
> +		WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);

Can squash to single line WARN_ON_ONCE(INTEL_GEN() < 10 && (mcr & ...)), 
if it fits.

>   	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
> +	 * expects mcr to be programed to a enabled slice/subslice pair
> +	 * before any MMIO read into slice/subslice register
> +	 */
> +	if (INTEL_GEN(dev_priv) < 10)
> +		mcr &= ~mcr_slice_subslice_mask;
> +	else
> +		mcr = calculate_mcr(mcr, dev_priv);

Does it make sense to move the conditional and comment to calculate_mcr 
- so here only a single call to it remains?

> +
>   	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>   
>   	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
> @@ -1307,6 +1339,9 @@ static int cnl_init_workarounds(struct intel_engine_cs *engine)
>   	struct drm_i915_private *dev_priv = engine->i915;
>   	int ret;
>   
> +	/* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */
> +	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,
> 

Above are suggestions and questions only.

Regards,

Tvrtko
Mika Kuoppala March 27, 2018, 2:22 p.m. UTC | #2
Yunwei Zhang <yunwei.zhang@intel.com> writes:

> 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.
>
> Reference: HSD#1405586840 BSID#0575

Use plural, and add comma in between.

Move 'References' right before Cc:s after the version log...

>
> v2:
>  - use fls() instead of find_last_bit() (Chris)
>  - added INTEL_SSEU to extract sseu from device info. (Chris)
> v3:
>  - rebase on latest tip
> v4:
>  - Added references (Mika)
>

.. in here.

And we usually do put Cc:s before s-o-b.

For example, see commit 465418c6064c88d4af555abe0480c417eb47eae3

Thanks,
-Mika

> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  1 +
>  drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 800230b..2db2a04 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2297,6 +2297,7 @@ intel_info(const struct drm_i915_private *dev_priv)
>  
>  #define INTEL_GEN(dev_priv)	((dev_priv)->info.gen)
>  #define INTEL_DEVID(dev_priv)	((dev_priv)->info.device_id)
> +#define INTEL_SSEU(dev_priv)	((dev_priv)->info.sseu)
>  
>  #define REVID_FOREVER		0xff
>  #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index de09fa4..cc19e0a 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
>  	}
>  }
>  
> +static u32 calculate_mcr(u32 mcr, struct drm_i915_private *dev_priv)
> +{
> +	const struct sseu_dev_info *sseu = &(INTEL_SSEU(dev_priv));
> +	u32 slice = fls(sseu->slice_mask);
> +	u32 subslice = fls(sseu->subslice_mask[slice]);
> +
> +	mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
> +	mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
> +
> +	return mcr;
> +}
> +
> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
> +{
> +	u32 mcr;
> +
> +	mcr = I915_READ(GEN8_MCR_SELECTOR);
> +	mcr = calculate_mcr(mcr, dev_priv);
> +	I915_WRITE(GEN8_MCR_SELECTOR, mcr);
> +}
> +
>  static inline uint32_t
>  read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>  		  int subslice, i915_reg_t reg)
> @@ -828,18 +849,29 @@ 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);
> +	if (INTEL_GEN(dev_priv) < 10)
> +		WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
>  	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
> +	 * expects mcr to be programed to a enabled slice/subslice pair
> +	 * before any MMIO read into slice/subslice register
> +	 */
> +	if (INTEL_GEN(dev_priv) < 10)
> +		mcr &= ~mcr_slice_subslice_mask;
> +	else
> +		mcr = calculate_mcr(mcr, dev_priv);
> +
>  	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>  
>  	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
> @@ -1307,6 +1339,9 @@ static int cnl_init_workarounds(struct intel_engine_cs *engine)
>  	struct drm_i915_private *dev_priv = engine->i915;
>  	int ret;
>  
> +	/* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */
> +	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,
> -- 
> 2.7.4
Chris Wilson March 27, 2018, 2:29 p.m. UTC | #3
Quoting Tvrtko Ursulin (2018-03-26 17:57:38)
> 
> On 26/03/2018 17:12, Yunwei Zhang wrote:
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h        |  1 +
> >   drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++--
> >   2 files changed, 38 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 800230b..2db2a04 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2297,6 +2297,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> >   
> >   #define INTEL_GEN(dev_priv) ((dev_priv)->info.gen)
> >   #define INTEL_DEVID(dev_priv)       ((dev_priv)->info.device_id)
> > +#define INTEL_SSEU(dev_priv) ((dev_priv)->info.sseu)
> 
> If we add this someone gets the job of converting existing users?

My bad, I hadn't realised that the INTEL_SSEU conversion was local to my
tree. It must have fallen out as part of the static device_info reform.
-Chris
Yunwei Zhang March 27, 2018, 4:17 p.m. UTC | #4
On 3/26/2018 9:57 AM, Tvrtko Ursulin wrote:
>
> On 26/03/2018 17:12, 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.
>>
>> Reference: 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
>> v4:
>>   - Added references (Mika)
>>
>> Signed-off-by: Yunwei Zhang <yunwei.zhang@intel.com>
>> Cc: Oscar Mateo <oscar.mateo@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h        |  1 +
>>   drivers/gpu/drm/i915/intel_engine_cs.c | 39 
>> ++++++++++++++++++++++++++++++++--
>>   2 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 800230b..2db2a04 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2297,6 +2297,7 @@ intel_info(const struct drm_i915_private 
>> *dev_priv)
>>     #define INTEL_GEN(dev_priv)    ((dev_priv)->info.gen)
>>   #define INTEL_DEVID(dev_priv) ((dev_priv)->info.device_id)
>> +#define INTEL_SSEU(dev_priv)    ((dev_priv)->info.sseu)
>
> If we add this someone gets the job of converting existing users?
This is suggestion from Chris Wilson, I am new here, but I guess if I am 
going to do that, it is better in a separate patch. I will remove this 
in next patchset and submit a new patch later.
>>     #define REVID_FOREVER        0xff
>>   #define INTEL_REVID(dev_priv) ((dev_priv)->drm.pdev->revision)
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index de09fa4..cc19e0a 100644
>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
>> @@ -796,6 +796,27 @@ const char *i915_cache_level_str(struct 
>> drm_i915_private *i915, int type)
>>       }
>>   }
>>   +static u32 calculate_mcr(u32 mcr, struct drm_i915_private *dev_priv)
>
> dev_priv first would be more typical function argument order.
>
>> +{
>> +    const struct sseu_dev_info *sseu = &(INTEL_SSEU(dev_priv));
>> +    u32 slice = fls(sseu->slice_mask);
>> +    u32 subslice = fls(sseu->subslice_mask[slice]);
>> +
>> +    mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
>> +    mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
>> +
>> +    return mcr;
>> +}
>> +
>> +static void wa_init_mcr(struct drm_i915_private *dev_priv)
>> +{
>> +    u32 mcr;
>> +
>> +    mcr = I915_READ(GEN8_MCR_SELECTOR);
>> +    mcr = calculate_mcr(mcr, dev_priv);
>> +    I915_WRITE(GEN8_MCR_SELECTOR, mcr);
>> +}
>> +
>>   static inline uint32_t
>>   read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
>>             int subslice, i915_reg_t reg)
>> @@ -828,18 +849,29 @@ 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);
>> +    if (INTEL_GEN(dev_priv) < 10)
>> +        WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
>
> Can squash to single line WARN_ON_ONCE(INTEL_GEN() < 10 && (mcr & 
> ...)), if it fits.
>
>>       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
>> +     * expects mcr to be programed to a enabled slice/subslice pair
>> +     * before any MMIO read into slice/subslice register
>> +     */
>> +    if (INTEL_GEN(dev_priv) < 10)
>> +        mcr &= ~mcr_slice_subslice_mask;
>> +    else
>> +        mcr = calculate_mcr(mcr, dev_priv);
>
> Does it make sense to move the conditional and comment to 
> calculate_mcr - so here only a single call to it remains?
I am thinking maybe it is better to save jump/return for GENs that don't 
have WA..
>> +
>>       I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
>>         intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
>> @@ -1307,6 +1339,9 @@ static int cnl_init_workarounds(struct 
>> intel_engine_cs *engine)
>>       struct drm_i915_private *dev_priv = engine->i915;
>>       int ret;
>>   +    /* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */
>> +    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,
>>
>
> Above are suggestions and questions only.
>
> Regards,
>
> Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 800230b..2db2a04 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2297,6 +2297,7 @@  intel_info(const struct drm_i915_private *dev_priv)
 
 #define INTEL_GEN(dev_priv)	((dev_priv)->info.gen)
 #define INTEL_DEVID(dev_priv)	((dev_priv)->info.device_id)
+#define INTEL_SSEU(dev_priv)	((dev_priv)->info.sseu)
 
 #define REVID_FOREVER		0xff
 #define INTEL_REVID(dev_priv)	((dev_priv)->drm.pdev->revision)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index de09fa4..cc19e0a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -796,6 +796,27 @@  const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
 	}
 }
 
+static u32 calculate_mcr(u32 mcr, struct drm_i915_private *dev_priv)
+{
+	const struct sseu_dev_info *sseu = &(INTEL_SSEU(dev_priv));
+	u32 slice = fls(sseu->slice_mask);
+	u32 subslice = fls(sseu->subslice_mask[slice]);
+
+	mcr &= ~(GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK);
+	mcr |= GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
+
+	return mcr;
+}
+
+static void wa_init_mcr(struct drm_i915_private *dev_priv)
+{
+	u32 mcr;
+
+	mcr = I915_READ(GEN8_MCR_SELECTOR);
+	mcr = calculate_mcr(mcr, dev_priv);
+	I915_WRITE(GEN8_MCR_SELECTOR, mcr);
+}
+
 static inline uint32_t
 read_subslice_reg(struct drm_i915_private *dev_priv, int slice,
 		  int subslice, i915_reg_t reg)
@@ -828,18 +849,29 @@  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);
+	if (INTEL_GEN(dev_priv) < 10)
+		WARN_ON_ONCE(mcr & mcr_slice_subslice_mask);
 	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
+	 * expects mcr to be programed to a enabled slice/subslice pair
+	 * before any MMIO read into slice/subslice register
+	 */
+	if (INTEL_GEN(dev_priv) < 10)
+		mcr &= ~mcr_slice_subslice_mask;
+	else
+		mcr = calculate_mcr(mcr, dev_priv);
+
 	I915_WRITE_FW(GEN8_MCR_SELECTOR, mcr);
 
 	intel_uncore_forcewake_put__locked(dev_priv, fw_domains);
@@ -1307,6 +1339,9 @@  static int cnl_init_workarounds(struct intel_engine_cs *engine)
 	struct drm_i915_private *dev_priv = engine->i915;
 	int ret;
 
+	/* WaProgramMgsrForCorrectSliceSpecificMmioReads: cnl */
+	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,