On 01/12/2022 23:23, Matt Roper wrote:
On Thu, Dec 01, 2022 at 09:23:07AM -0800, Lucas De Marchi wrote:
On Thu, Dec 01, 2022 at 01:15:35PM +0000, Tvrtko Ursulin wrote:
On 30/11/2022 23:17, Matt Atwood wrote:
From: Matt Roper <matthew.d.roper@xxxxxxxxx>
This patch introduces initial workarounds for mtl platform
Bspec:66622
Signed-off-by: Matt Atwood <matthew.s.atwood@xxxxxxxxx>
Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
---
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 +-
.../drm/i915/gt/intel_execlists_submission.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 11 +-
drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 +
drivers/gpu/drm/i915/gt/intel_workarounds.c | 105 +++++++++++++-----
drivers/gpu/drm/i915/gt/uc/intel_guc.c | 9 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 10 +-
drivers/gpu/drm/i915/i915_drv.h | 4 +
drivers/gpu/drm/i915/intel_device_info.c | 6 +
9 files changed, 121 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c33e0d72d670..af88d8ab61c1 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1479,7 +1479,9 @@ static int __intel_engine_stop_cs(struct intel_engine_cs *engine,
* Wa_22011802037 : gen11, gen12, Prior to doing a reset, ensure CS is
* stopped, set ring stop bit and prefetch disable bit to halt CS
*/
- if (IS_GRAPHICS_VER(engine->i915, 11, 12))
+ if (IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_A0, STEP_B0) ||
+ (GRAPHICS_VER(engine->i915) >= 11 &&
+ GRAPHICS_VER_FULL(engine->i915) < IP_VER(12, 70)))
Does comment need updating to reflect the workaround applicability?
Elsewhere as well. Some are left as dg2 only. Some gen11,gen12 only.
Then there's a few of this same change logic throught the patch, so I
assume a general situation of workarounds applying to only early MTL.
if ((IS_GRAPHICS_VER(engine->i915, 11, 12)) &&
!IS_MTL_GRAPHICS_STEP(engine->i915, M, STEP_B1, STEP_FOREVER)
Would this be correct and simpler? Not sure about STEP_B1 for start of
should be STEP_B0 if doing this. The stepping check is inclusive on the
left, exclusive on the right, i.e: [STEP_A0, STEP_B0).
But even if the check is simpler, I'd avoid doing a negative check to
maintain consistency.
Agreed; if you have access to the internal workaround database, you can
query a list of which platforms/steppings a given workaround applies to
and get a list that basically lays things out something like
Wa_XXXXXXXX:
MTL: [a0..b0)
PVC: not needed
DG2-G10: [b1..c3)
DG2-G11: [a0..a2)
XEHPSDV: all steppings
ADL-P: not needed
...
Even if the code condition has a bunch of arms, it should translate
pretty clearly to what's in the workaround database, so it's easier to
audit and make sure we aren't missing anything. With all the platforms
we have these days, negative tests make it a lot harder to verify (and
in your example here would cause problems if we get something like a new
12.80 or 12.90 platform down the road...presumably those wouldn't want
this workaround either, but wouldn't be captured properly).
The corollary of that is that we should be really careful about using
range checks like IS_GRAPHICS_VER() that only compare the major version
number. If we aren't sure we've fully moved past the upper end of the
range, there's a possibility that new platforms may show up that
shouldn't be included in that range (as MTL did in this case, breaking
our "applies to all 11.x and 12.x" assumption).
Yeah okay, perhaps it is just a mix of major and full ver check which
looked a bit naff in the code to me, and the unfortunate fact the
workaround ends in the middle (stepping wise) of a part/major gen.
On the topic of comments ("/* Wa_16012604467:adlp,mtl[a0,b0] */" and
such) and code getting out of sync - lets tidy that up. Either it's
always correct (in sync), or it's completely removed. Having a mix is
just bad an pointless.
If the tool you guys use to query the database can output the
"Wa_NNNNNN:xxx,yyy,zzz" format, I'd say keep the comments (and fix them
up) because then that is the reference which *can* be useful when
reviewing and spotting things like (oh you've inverted the logic when
adding the new platform). But if the tool can no to that then just drop
the platform list from them? Although I am kind of hoping the tool can
output that info and it's useful to have them sprinkled around the code.
Regards,
Tvrtko
