[Intel-gfx] [PATCH 2/2] drm/i915/mtl: Add initial gt workarounds

[Tvrtko Ursulin]

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 at intel.com>
>>>>
>>>> This patch introduces initial workarounds for mtl platform
>>>>
>>>> Bspec:66622
>>>>
>>>> Signed-off-by: Matt Atwood <matthew.s.atwood at intel.com>
>>>> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>>>> ---
>>>>   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