[Intel-xe] [PATCH 7/7] drm/xe/guc: Port workarounds to OOB infra

Thu May 4 19:30:28 UTC 2023

On Thu, May 04, 2023 at 03:30:48PM +0000, Jose Souza wrote:
>On Thu, 2023-05-04 at 00:32 -0700, Lucas De Marchi wrote:
>> Let xe_guc.c start using XE_WA() for workarounds. One workaround
>> couldn't be converted due to no support for function hook checks.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_guc.c | 40 +++++++++++++------------------------
>>  1 file changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index f7d32b744247..eea5ed015282 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -19,7 +19,9 @@
>>  #include "xe_guc_submit.h"
>>  #include "xe_mmio.h"
>>  #include "xe_platform_types.h"
>> +#include "xe_rtp.h"
>>  #include "xe_uc_fw.h"
>> +#include "xe_wa.h"
>>  #include "xe_wopcm.h"
>>
>>  #define MEDIA_GUC_HOST_INTERRUPT        XE_REG(0x190304)
>> @@ -136,29 +138,22 @@ static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>>  	struct xe_gt *gt = guc_to_gt(guc);
>>  	u32 flags = 0;
>>
>> -	/* Wa_22012773006:gen11,gen12 < XeHP */
>> -	if (GRAPHICS_VER(xe) >= 11 &&
>> -	    GRAPHICS_VERx100(xe) < 1250)
>> +	if (XE_WA(xe, "22012773006", GRAPHICS_VERSION_RANGE(1100, 1250)))
>>  		flags |= GUC_WA_POLLCS;
>
>Just a suggestion but would be better to have the workaround rule defined somewhere else and here only have 'if (XE_WA(xe, "22012773006"))'.
>
>
>Mesa now has something like that.
>We have a Python script that generates a JSON file with workaround data from the workaround database API.
>During build time, another Python script generates the intel_wa.c file using the information in JSON, setting a bit for each workaround that applies
>to each platform.
>In runtime it get the bit field with the workarounds that applies to running platform on given stepping.

that is one of the alternatives I shared in the RFC. There are
some issues with such approach though:

1) The condition is in a totally different place tha have to be checked
    to follow the code flow.

    The upside is that if the same condition needs to be checked again in
    multiple places for these OOB workarounds, the condition doesn't
    need to be repeated

2) Not possible to encode conditions that are not platform-centric or
that needs to be decided in runtime (e.g. if an engine is fused off or
not)

>In code Mesa we have 'if (intel_needs_workaround(devinfo, 14015055625))', Xe kmd would probably need additional checks like 'if (engine == BLIT &&
>XE_WA(xe, "xxxxxx"))'.

yeah... that maybe solves the issue (2) above. We'd need to leave the
non-platform checks to each callers. And maybe follow the same pattern
for the gt/engine/lrc workarounds.

>
>This solve the problem we have were workarounds are left enabled in platforms that already have a HW fix for it.

not sure how that solves this issue though. We mark the stepping in
which the workaround are supposedly fixed.

>
>Not sure if such think would be acceptable in Linux, just sharing what Mesa have done.

Generating code to be built is done in a few places in the kernel
I already suggested doing that for the register definitions, but
there are always push back.  I will take a look at this again.

Thanks
Lucas De Marchi

>
>>
>> -	/* Wa_16011759253 */
>> -	/* Wa_22011383443 */
>> -	if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_B0))
>> +	if (XE_WA(xe, "16011759253, 22011383443",
>> +		  SUBPLATFORM(DG2, G10), STEP(A0, B0)))
>>  		flags |= GUC_WA_GAM_CREDITS;
>>
>> -	/* Wa_14014475959 */
>> -	if (IS_PLATFORM_STEP(xe, XE_METEORLAKE, STEP_A0, STEP_B0) ||
>> -	    xe->info.platform == XE_DG2)
>> +	if (XE_WA(xe, "14014475959[mtl]", PLATFORM(METEORLAKE), STEP(A0, B0)) ||
>> +	    XE_WA(xe, "14014475959[dg2]", PLATFORM(DG2)))
>>  		flags |= GUC_WA_HOLD_CCS_SWITCHOUT;
>>
>>  	/*
>> -	 * Wa_14012197797
>> -	 * Wa_22011391025
>> -	 *
>>  	 * The same WA bit is used for both and 22011391025 is applicable to
>>  	 * all DG2.
>>  	 */
>> -	if (xe->info.platform == XE_DG2)
>> +	if (XE_WA(xe, "14012197797, 22011391025", PLATFORM(DG2)))
>>  		flags |= GUC_WA_DUAL_QUEUE;
>>
>>  	/*
>> @@ -169,27 +164,20 @@ static u32 guc_ctl_wa_flags(struct xe_guc *guc)
>>  	if (GRAPHICS_VERx100(xe) < 1270)
>>  		flags |= GUC_WA_PRE_PARSER;
>>
>> -	/* Wa_16011777198 */
>> -	if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_C0) ||
>> -	    IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G11, STEP_A0,
>> -				STEP_B0))
>> +	if (XE_WA(xe, "16011777198[dg2_g10]", SUBPLATFORM(DG2, G10), STEP(A0, C0)) ||
>> +	    XE_WA(xe, "16011777198[dg2_g11]", SUBPLATFORM(DG2, G11), STEP(A0, B0)))
>>  		flags |= GUC_WA_RCS_RESET_BEFORE_RC6;
>>
>> -	/*
>> -	 * Wa_22012727170
>> -	 * Wa_22012727685
>> -	 */
>> -	if (IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G10, STEP_A0, STEP_C0) ||
>> -	    IS_SUBPLATFORM_STEP(xe, XE_DG2, XE_SUBPLATFORM_DG2_G11, STEP_A0,
>> -				STEP_FOREVER))
>> +	if (XE_WA(xe, "22012727170, 22012727685 [dg2_g10]", SUBPLATFORM(DG2, G10), STEP(A0, C0)) ||
>> +	    XE_WA(xe, "22012727170, 22012727685 [dg2_g11]", SUBPLATFORM(DG2, G11), STEP(A0, FOREVER)))
>>  		flags |= GUC_WA_CONTEXT_ISOLATION;
>>
>>  	/* Wa_16015675438, Wa_18020744125 */
>> +	/* FIXME: currently there is no support for gt checks in XE_WA() */
>>  	if (!xe_hw_engine_mask_per_class(gt, XE_ENGINE_CLASS_RENDER))
>>  		flags |= GUC_WA_RCS_REGS_IN_CCS_REGS_LIST;
>>
>> -	/* Wa_1509372804 */
>> -	if (IS_PLATFORM_STEP(xe, XE_PVC, STEP_B0, STEP_C0))
>> +	if (XE_WA(xe, "1509372804", PLATFORM(PVC), STEP(B0, C0)))
>>  		flags |= GUC_WA_RENDER_RST_RC6_EXIT;
>>
>>  	return flags;
>