| Message ID | 20240513141917.74208-1-angus.chen@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/mtl: Update workaround 14018778641 | expand |
Hi Angus, 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 */ > + 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? Besides, in MTL we have the media GT where the MOD_CTRL family has address 0x38cf34. Should this be checked and included, as well? Thanks, Andi > /* Wa_22016670082 */ > wa_write_or(wal, GEN12_SQCNT1, GEN12_STRICT_RAR_ENABLE); > > -- > 2.34.1
On Fri, May 24, 2024 at 12:31:26AM +0200, Andi Shyti wrote: > Hi Angus, > > 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. > > + 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. > > 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. Matt > > Thanks, > Andi > > > /* Wa_22016670082 */ > > wa_write_or(wal, GEN12_SQCNT1, GEN12_STRICT_RAR_ENABLE); > > > > -- > > 2.34.1
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
On Fri, May 24, 2024 at 01:26:41PM +0200, Andi Shyti wrote: > 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? Yeah, it would definitely be good to update this since it's caused confusion multiple times in the past as well. > > > > > + 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. Documented in the workaround database you mean? The description there seems pretty clear to me --- they indicate that although the hardware issue theoretically existed on all engines, their belief was that only the CCS engines could generate the kind of memory traffic necessary to cause problems, so the initial suggestion was to implement it only on the CCS engine. However the workaround also provides the details for how to implement it on all the other engine types if necessary in case real-world usage uncovers corner cases that wind up needing it. Maybe we should try to get them to update the language a bit now that we have reports that at least RCS and VCS do indeed seem to need the workaround in real-world usage. Matt > > 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
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 */ + wa_write_or(wal, XELPMP_VDBX_MOD_CTRL, FORCE_MISS_FTLB); + /* Wa_22016670082 */ wa_write_or(wal, GEN12_SQCNT1, GEN12_STRICT_RAR_ENABLE);
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(+)