[Intel-gfx] [PATCH v3] drm/i915: Enable WaProgramMgsrForCorrectSliceSpecificMmioReads for Gen9

[Tvrtko Ursulin]

On 05/03/2021 00:53, Chris Wilson wrote:
> Quoting Chris Wilson (2021-03-04 11:56:16)
>> Quoting Chris Wilson (2021-03-04 09:19:24)
>>> Quoting Tvrtko Ursulin (2021-03-04 09:12:26)
>>>>
>>>> On 02/03/2021 06:27, Cooper Chiou wrote:
>>>>> WaProgramMgsrForCorrectSliceSpecificMmioReads applies for Gen9 to
>>>>> resolve VP8 hardware encoding system hang up on GT1 sku for
>>>>> ChromiumOS projects
>>>>>
>>>>> Slice specific MMIO read inaccurate so MGSR needs to be programmed
>>>>> appropriately to get correct reads from these slicet-related MMIOs.
>>>>>
>>>>> It dictates 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, especially GT1 fused sku
>>>>> since this issue can be reproduced on VP8 hardware encoding via ffmpeg
>>>>> on ChromiumOS devices.
>>>>> When exit PC7, MGSR will reset so that we have to skip fused subslice ID.
>>>>>
>>>>> Reference: HSD#1508045018,1405586840, BSID#0575
>>>>>
>>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
>>>>> Cc: William Tseng <william.tseng at intel.com>
>>>>> Cc: Lee Shawn C <shawn.c.lee at intel.com>
>>>>>
>>>>> Signed-off-by: Cooper Chiou <cooper.chiou at intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 +++++++++++++++++++++
>>>>>    1 file changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>> index 3b4a7da60f0b..4ad598a727a6 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
>>>>> @@ -878,9 +878,47 @@ hsw_gt_workarounds_init(struct drm_i915_private *i915, struct i915_wa_list *wal)
>>>>>        wa_write_clr(wal, GEN7_FF_THREAD_MODE, GEN7_FF_VS_REF_CNT_FFME);
>>>>>    }
>>>>>    
>>>>> +static void
>>>>> +gen9_wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
>>>>> +{
>>>>> +     const struct sseu_dev_info *sseu = &i915->gt.info.sseu;
>>>>> +     unsigned int slice, subslice;
>>>>> +     u32 mcr, mcr_mask;
>>>>> +
>>>>> +     GEM_BUG_ON(INTEL_GEN(i915) < 9);
>>>>> +
>>>>> +     /*
>>>>> +      * WaProgramMgsrForCorrectSliceSpecificMmioReads:glk,kbl,cml
>>>>> +      * Before any MMIO read into slice/subslice specific registers, MCR
>>>>> +      * packet control register needs to be programmed to point to any
>>>>> +      * enabled s/ss pair. Otherwise, incorrect values will be returned.
>>>>> +      * This means each subsequent MMIO read will be forwarded to an
>>>>> +      * specific s/ss combination, but this is OK since these registers
>>>>> +      * are consistent across s/ss in almost all cases. In the rare
>>>>> +      * occasions, such as INSTDONE, where this value is dependent
>>>>> +      * on s/ss combo, the read should be done with read_subslice_reg.
>>>>> +      */
>>>>> +     slice = fls(sseu->slice_mask) - 1;
>>>>> +     GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
>>>>> +     subslice = fls(intel_sseu_get_subslices(sseu, slice));
>>>>> +     GEM_BUG_ON(!subslice);
>>>>> +     subslice--;
>>>>> +
>>>>> +     mcr = GEN8_MCR_SLICE(slice) | GEN8_MCR_SUBSLICE(subslice);
>>>>> +     mcr_mask = GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK;
>>>>> +
>>>>> +     drm_dbg(&i915->drm, "MCR slice:%d/subslice:%d = %x\n", slice, subslice, mcr);
>>>>> +
>>>>> +     wa_write_clr_set(wal, GEN8_MCR_SELECTOR, mcr_mask, mcr);
>>>>> +}
>>>>
>>>> Have you considered reusing existing wa_init_mcr? Just needs the
>>>> top-level assert changed and otherwise it looks it would do the right
>>>> thing for Gen9. Advantage being a smaller patch and less code to carry.
>>>
>>> That was the first patch, and fails for the same reason. The problem
>>> would appear to be in the mcr_mask for gt3.
>>
>> For the record, it appears to be an issue with fls vs ffs. Switching to
>> ffs also removes the warnings for workaround failures on ehl/jsl.
> 
> Of course the icl in farm2 started spewing warnigns, but the other icl
> in farm1 were happy.

It figures yes, now I remember it was the shards which had a mix of ICL 
GT1/2 with different fusing which were exhibiting odd behaviour.

I have old patches around which a) try to program each WA in unicast 
mode (for all present ss), b) do verification for each present ss. Idea 
being to see if there are any patterns as to what writes land and which 
get lost. I don't think the results were conclusive back then but maybe 
I can try again.

I am not sure if PC8 and DMC could also be involved from what Cooper was 
saying in a different thread. Maybe another CI run without the DMC, both 
ffs and fls. Another for limiting cstates.

Regards,

Tvrtko