Re: [Intel-gfx] [PATCH v4 1/2] drm/i915/dg2: Introduce Wa_18018764978

[Gustavo Sousa <gustavo.sousa@intel.com>]

On Thu, Jan 19, 2023 at 01:22:24PM -0800, Matt Roper wrote:
> On Wed, Jan 18, 2023 at 12:01:20PM -0500, Rodrigo Vivi wrote:
> > On Wed, Jan 18, 2023 at 09:54:56AM +0000, Tvrtko Ursulin wrote:
> > > 
> > > On 28/11/2022 18:26, Matt Roper wrote:
> > > > On Wed, Nov 23, 2022 at 04:45:25PM -0300, Gustavo Sousa wrote:
> > > > > On Wed, Nov 23, 2022 at 10:36:47AM -0800, Matt Atwood wrote:
> > > > > > Wa_18018764978 applies to specific steppings of DG2 (G10 C0+,
> > > > > > G11 and G12 A0+). Clean up style in function at the same time.
> > > > > > 
> > > > > > Bspec: 66622
> > > > > > 
> > > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > > > > 
> > > > > Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
> > > > 
> > > > Both patches applied to drm-intel-gt-next.  Thanks for the patches and
> > > > review.
> > > 
> > > Do these need to be sent to 6.2 fixes, given DG2 is out of force probe
> > > there?
> > 
> > Yeap, good point. I'd say in general we are not very good in cherry-picking
> > the hw w/a to the fixes. But this one was a good catch. Let's ensure the
> > best stability for DG2 on 6.2.
> > 
> > pushed to drm-intel-fixes now.
> 
> I don't have any concerns about putting this in drm-intel-fixes, and the
> patch will functionally behave as expected, but I did just notice one
> minor problem with the original patch that we should probably fix up on
> drm-intel-gt-next:  the PSS_MODE2 register is in a range that has
> multicast behavior (GSLICE replication) on Xe_HP platforms.  So the
> register should have been defined as MCR_REG() instead of _MMIO, and the
> workaround should use wa_mcr_masked_en().
> 
> As I mentioned, this oversight shouldn't cause any real problems, so no
> concerns about keeping this in -fixes.  On DG2 we don't have to worry
> about steering races with external agents, so even with the 'wrong'
> register type definition, the steering should already be implicitly set
> to multicast mode at the point we're applying workarounds.  The need to
> follow up with a correction on drm-intel-gt-next is really only
> important for consistency and in case this register gets used in other
> manners (not just workaround lists) at some point in the future.
> 
> MattA/Gustavo will one of you guys send an update for that, or should I
> take care of it?

I'll do it. Sorry about missing that in my review. I *think* I came to learn
about MCR registers only some time after that.

--
Gustavo Sousa

> 
> 
> Matt
> 
> > 
> > Thanks,
> > Rodrigo.
> > 
> > > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > > > > ---
> > > > > >   drivers/gpu/drm/i915/gt/intel_gt_regs.h     | 3 +++
> > > > > >   drivers/gpu/drm/i915/gt/intel_workarounds.c | 7 ++++++-
> > > > > >   2 files changed, 9 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > > index 80a979e6f6be..74379d3c5a4d 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > > > @@ -457,6 +457,9 @@
> > > > > >   #define GEN8_L3CNTLREG				_MMIO(0x7034)
> > > > > >   #define   GEN8_ERRDETBCTRL			(1 << 9)
> > > > > > +#define PSS_MODE2				_MMIO(0x703c)
> > > > > > +#define   SCOREBOARD_STALL_FLUSH_CONTROL	REG_BIT(5)
> > > > > > +
> > > > > >   #define GEN7_SC_INSTDONE			_MMIO(0x7100)
> > > > > >   #define GEN12_SC_INSTDONE_EXTRA			_MMIO(0x7104)
> > > > > >   #define GEN12_SC_INSTDONE_EXTRA2		_MMIO(0x7108)
> > > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > > index 2afb4f80a954..870db5a202dd 100644
> > > > > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > > > > @@ -771,9 +771,14 @@ static void dg2_ctx_workarounds_init(struct intel_engine_cs *engine,
> > > > > >   	/* Wa_14014947963:dg2 */
> > > > > >   	if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_B0, STEP_FOREVER) ||
> > > > > > -		IS_DG2_G11(engine->i915) || IS_DG2_G12(engine->i915))
> > > > > > +	    IS_DG2_G11(engine->i915) || IS_DG2_G12(engine->i915))
> > > > > >   		wa_masked_field_set(wal, VF_PREEMPTION, PREEMPTION_VERTEX_COUNT, 0x4000);
> > > > > > +	/* Wa_18018764978:dg2 */
> > > > > > +	if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_C0, STEP_FOREVER) ||
> > > > > > +	    IS_DG2_G11(engine->i915) || IS_DG2_G12(engine->i915))
> > > > > > +		wa_masked_en(wal, PSS_MODE2, SCOREBOARD_STALL_FLUSH_CONTROL);
> > > > > > +
> > > > > >   	/* Wa_15010599737:dg2 */
> > > > > >   	wa_masked_en(wal, CHICKEN_RASTER_1, DIS_SF_ROUND_NEAREST_EVEN);
> > > > > >   }
> > > > > > -- 
> > > > > > 2.38.1
> > > > > > 
> > > > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation