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

[Zhang, Yunwei]

On 3/27/2018 4:13 PM, Chris Wilson wrote:
> Quoting Zhang, Yunwei (2018-03-27 23:49:27)
>>
>> 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.
> What mmio read? The only accessor should be this function.
>
> And still the two comments are in direct conflict with each other.
> -Chris
This function is only used in INST_DONE case which you need to iterate 
through each slice/subslice to check and makes sense to program MCR for 
each s/ss combination. But there could be inadvertent read into this 
range without using this function, the value would be wrong without this 
WA.

Anyway, the comment is not right given the latest WA required, I will 
submit a new patchset to update this comment.

Thanks,
Yunwei