Re: [PATCH v5 1/2] drm/i915/cnl: Implement WaProgramMgsrForCorrectSliceSpecificMmioReads

["Zhang, Yunwei" <yunwei.zhang@intel.com>]

On 3/28/2018 9:03 AM, Chris Wilson wrote:
> Quoting Zhang, Yunwei (2018-03-28 16:54:26)
>>
>> 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@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>
>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>>>> Signed-off-by: Yunwei Zhang <yunwei.zhang@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.
> Sure, but garbage in, garbage out. If we write an accessor for a
> register because it requires a workaround, anyone who wants to access
> the register should use the accessor. Not just leave HW in a random
> state so that one particular selector works.
> -Chris
But keep in mind this is not only *a* register, it is a range of 
registers, if we don't use this WA, developer needs to reminded if there 
is a MMIO read in slice/subslice/l3bank range, they will need to use 
read_subslice_reg instead of i915_READ.

To me, it is just safe to program MCR in the beginning and reprogram it 
every time we have to change like in INST_DONE case.

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