Re: [PATCH v2] drm/i915/mtl: Update workaround 14018778641

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi,

> > On Mon, May 13, 2024 at 02:19:17PM +0000, Chen, Angus wrote:
> > > The WA should be extended to cover VDBOX engine. We found that
> > > 28-channels 1080p VP9 encoding may hit this issue.
> > > 
> > > Signed-off-by: Chen, Angus <angus.chen@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_workarounds.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > index d1ab560fcdfc..da0a481a375e 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > > @@ -1586,6 +1586,9 @@ xelpmp_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> > >  	 */
> > >  	wa_write_or(wal, XELPMP_GSC_MOD_CTRL, FORCE_MISS_FTLB);
> > >  
> > > +	/* Wa_14018778641 */
> 
> I realize that the comment farther up in the code is wrong, but there's
> no such workaround as "Wa_14018778641."  14018778641 is just an internal
> database ID that isn't meaningful for tracking workarounds in code.
> Workarounds are always identified by their "lineage" number, which is
> the number that will identify the workaround in a consistent manner
> across multiple platforms.  In this case it sounds like the expected
> workaround number was actually Wa_14018575942.

Indeed... should this be updated accordingly?

> > > +	wa_write_or(wal, XELPMP_VDBX_MOD_CTRL, FORCE_MISS_FTLB);
> > 
> > Wa_14018778641 says that we need to disable the FTLB for Compute,
> > Render, GSC, VDBox and VEBox engines, but here we are doing it
> > only for GSC and VDBox, why?
> 
> Wa_14018575942 (which is a follow-up to an older Wa_18018781329), was
> originally supposed to apply to all engines.  But after some
> investigation, the hardware teams decided that it was _probably_ only
> needed on the CCS engines so they suggested dropping the workaround from
> other engine types to reclaim performance unless we started seeing
> functional issues when doing so.  At some point someone did report some
> functional issues with the RCS engine, so the workaround got restored
> there.  Based on this patch, it sounds like the media team is now
> reporting that they also see functional failures on the VD engines
> without the workaround, so it also needs to be restored there now.

But I don't see it documented. Wa_14018575942 only speaks about
the GSC_MOD_CTL. We need it documented, otherwise we need a note
in the comment explaining why this workaround is needed here, as
well.

Otherwise, as it happened in other cases, someone might open this
workaround, think it's wrong and revert this patch.

Angus, can you please explain the reason why this workaround is
needed here and put it in a comment?

> > Besides, in MTL we have the media GT where the MOD_CTRL family
> > has address 0x38cf34. Should this be checked and included, as
> > well?
> 
> The gt pointer passed into xelpmp_gt_workarounds_init() is always the
> media GT.  And the GSI offset of 0x380000 gets added into the register
> offset automatically so you don't need to worry about doing so manually.

Correct, thanks!

Andi