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

[Zhang, Yunwei]

On 3/27/2018 3:27 PM, Chris Wilson wrote:
> Quoting Yunwei Zhang (2018-03-27 23:14:16)
>> 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)
>>
>> 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>
>> ---
>>   drivers/gpu/drm/i915/intel_engine_cs.c | 39 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
>> index de09fa4..4c78d1e 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(struct drm_i915_private *dev_priv, u32 mcr)
>> +{
>> +       const struct sseu_dev_info *sseu = &(INTEL_INFO(dev_priv)->sseu);
>> +       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(dev_priv, mcr);
>> +       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);
>> +       WARN_ON_ONCE(INTEL_GEN(dev_priv) < 10 &&
>> +                    (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
>> +        */
> So the read was above, where we did set the subslice_select
> appropriately. Here we are resetting back to 0 *after* the read, as the
> comment before indicates.
>
> So what are you trying to accomplish with this patch? Other than leaving
> the code in conflict with itself.
> -Chris
Hi Chris,

The comment mentioned 0xFDC needs to be reset to 0 was before this WA 
was introduced, in later HW, this WA requires 0xFDC to be programmed to 
a enabled slice/subslice.

What this patch does it to initialize 0xFDC once at the initialization 
(also it will be called after engine reset/TDR/coming out of c6) and 
make sure every time it is changed, it will be reprogrammed to a enabled 
slice/subslice so that a MMIO
read will get the correct value. read_subslice_reg changes the 0xFDC 
value and if it is set to 0, it will cause MMIO read to return invalid 
value for s/ss specific registers.