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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

Quoting Tvrtko Ursulin (2021-11-01 12:15:19)
> 
> On 25/10/2021 05:26, Cooper Chiou wrote:
> > This implements WaProgramMgsrForCorrectSliceSpecificMmioReads which
> > was omitted by mistake from Gen9 documentation, while it is actually
> > applicable to fused off parts.
> > 
> > Workaround consists of making sure MCR packet control register is
> > programmed to point to enabled slice/subslice pair before doing any
> > MMIO reads from the affected registers.
> > 
> > Failure do to this can result in complete system hangs when running
> > certain workloads. Two known cases which can cause system hangs are:
> > 
> > 1. "test_basic progvar_prog_scope_uninit" test which is part of
> >      Khronos OpenCL conformance suite
> >      (https://github.com/KhronosGroup/OpenCL-CTS) with the Intel
> >      OpenCL driver (https://github.com/intel/compute-runtime).
> > 
> > 2. VP8 media hardware encoding using the full-feature build of the
> >      Intel media-driver (https://github.com/intel/media-driver) and
> >      ffmpeg.
> > 
> > For the former case patch was verified to fix the hard system hang
> > when executing the OCL test on Intel Pentium CPU 6405U which contains
> > fused off GT1 graphics.
> > 
> > Reference: HSD#1508045018,1405586840, BSID#0575
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Cc: William Tseng <william.tseng@intel.com>
> > Cc: Shawn C Lee <shawn.c.lee@intel.com>
> > Cc: Pawel Wilma <pawel.wilma@intel.com>
> > Signed-off-by: Cooper Chiou <cooper.chiou@intel.com>
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Thanks for following through with all the testing and validation for the
patch!

Acked-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

> 
> Regards,
> 
> Tvrtko
> 
> P.S. You could have preserved my r-b from an earlier version since it is 
> the same code, just different commit message.
> 
> > ---
> >   drivers/gpu/drm/i915/gt/intel_workarounds.c | 41 +++++++++++++++++++++
> >   1 file changed, 41 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index e1f362530889..8ae24da601b0 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -877,11 +877,52 @@ hsw_gt_workarounds_init(struct intel_gt *gt, 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(GRAPHICS_VER(i915) != 9);
> > +
> > +     /*
> > +      * WaProgramMgsrForCorrectSliceSpecificMmioReads:gen9,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 = ffs(sseu->slice_mask) - 1;
> > +     GEM_BUG_ON(slice >= ARRAY_SIZE(sseu->subslice_mask));
> > +     subslice = ffs(intel_sseu_get_subslices(sseu, slice));
> > +     GEM_BUG_ON(!subslice);
> > +     subslice--;
> > +
> > +     /*
> > +      * We use GEN8_MCR..() macros to calculate the |mcr| value for
> > +      * Gen9 to address WaProgramMgsrForCorrectSliceSpecificMmioReads
> > +      */
> > +     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);
> > +}
> > +
> >   static void
> >   gen9_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> >   {
> >       struct drm_i915_private *i915 = gt->i915;
> >   
> > +     /* WaProgramMgsrForCorrectSliceSpecificMmioReads:glk,kbl,cml,gen9 */
> > +     gen9_wa_init_mcr(i915, wal);
> > +
> >       /* WaDisableKillLogic:bxt,skl,kbl */
> >       if (!IS_COFFEELAKE(i915) && !IS_COMETLAKE(i915))
> >               wa_write_or(wal,
> >