[Intel-xe] [RFC 2/2] drm/xe: Register OOB workarounds
Lucas De Marchi
lucas.demarchi at intel.com
Wed Apr 12 16:21:16 UTC 2023
On Wed, Apr 12, 2023 at 02:31:18PM +0300, Mika Kuoppala wrote:
>Lucas De Marchi <lucas.demarchi at intel.com> writes:
>
>> Some workarounds can't be implemented in the xe_wa.c as entries
>> on a table since they need to be spread throughout the driver.
>> Provide a mechanism to allow "registering" these workarounds so they can
>> later be dumped together with the others.
>>
>> This uses a trick to define the static const entry when checking if the
>> workarounds is "enabled". That entry is put in a specific section so
>> later we can access all the entries in that section. Normally the linker
>> provides the __start_<section>/__stop_<section> symbols. However since
>> the kernel itself uses a linker script, this doesn't work, hence provide
>> our own custom linker script to do what is missing.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 2 ++
>> drivers/gpu/drm/xe/xe.lds | 8 ++++++++
>> drivers/gpu/drm/xe/xe_guc.c | 6 +++---
>> drivers/gpu/drm/xe/xe_rtp_types.h | 8 ++++++++
>> drivers/gpu/drm/xe/xe_wa.c | 14 ++++++++++++--
>> drivers/gpu/drm/xe/xe_wa.h | 10 ++++++++++
>> 6 files changed, 43 insertions(+), 5 deletions(-)
>> create mode 100644 drivers/gpu/drm/xe/xe.lds
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index aceca651de57..9cbcc0ed06b2 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -216,6 +216,8 @@ ifeq ($(CONFIG_DRM_FBDEV_EMULATION),y)
>> xe-$(CONFIG_DRM_XE_DISPLAY) += display/intel_fbdev.o
>> endif
>>
>> +LDFLAGS_xe.o := -T $(srctree)/$(src)/xe.lds
>> +
>> obj-$(CONFIG_DRM_XE) += xe.o
>> obj-$(CONFIG_DRM_XE_KUNIT_TEST) += tests/
>>
>> diff --git a/drivers/gpu/drm/xe/xe.lds b/drivers/gpu/drm/xe/xe.lds
>> new file mode 100644
>> index 000000000000..a2187eab2c8b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe.lds
>> @@ -0,0 +1,8 @@
>> +SECTIONS
>> +{
>> + .rodata.__xe_wa_oob ALIGN(8) : {
>> + __start___xe_wa_oob = .;
>> + *(__xe_wa_oob)
>> + __stop___xe_wa_oob = .;
>> + }
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index ee71b969bcbf..608ac2a022e8 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -20,6 +20,7 @@
>> #include "xe_mmio.h"
>> #include "xe_platform_types.h"
>> #include "xe_uc_fw.h"
>> +#include "xe_wa.h"
>> #include "xe_wopcm.h"
>>
>> static struct xe_gt *
>> @@ -134,9 +135,8 @@ 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("22012773006",
>> + GRAPHICS_VER(xe) >= 11 && GRAPHICS_VERx100(xe) < 1250))
>> flags |= GUC_WA_POLLCS;
>
>Now the entry will appear regardless if it was activated.
you mean "appear in the debugfs"? Listing all of them was intentional
and marking some as "active" would be a future patch on top.
I didn't implement that since I'm really on the fence regard the 2 or 3
possible approaches and I think it'd be easier
My idea here is basically like was described in the coverletter. During
init we'd do:
idx = 0;
for each wa in oob section {
wa->idx = idx++;
}
Then on the WA side, instead of having XE_WA() just returning the
condition, it would evaluate the condition and write to
xe->oob_workarounds.active[wa->idx]. Downside is that WAs would change
state and only be "active" when executed, which depends on the code
paths.
But the alternatives detailed in the cover letter make it a little bit
different: I think most of the conditions could be executed on init
rather than where it's called. For that we'd need to either
a) convert the checks to "normal RTP rules", but without actions
since the actions are to be done on the caller side
it may not be to horrible to drop the `if` in th `if (XE_WA(...))` and
just write it like:
XE_WA(xe, name, rtp-rules, action);
and allow "action" to be any statement
b) migrate to the table approach I mentioned, so the callers would
have:
if (XE_WA(xe, name) {
<actions>
}
Lucas De Marchi
>Would it add value to have it only listed if it was activated, or even
>both: implemented and activated as separate entries.
>
>-Mika
>
>>
>> /* Wa_16011759253 */
>> diff --git a/drivers/gpu/drm/xe/xe_rtp_types.h b/drivers/gpu/drm/xe/xe_rtp_types.h
>> index e87f1b280d96..4f7832d074bd 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp_types.h
>> +++ b/drivers/gpu/drm/xe/xe_rtp_types.h
>> @@ -101,4 +101,12 @@ struct xe_rtp_entry {
>> u8 flags;
>> };
>>
>> +/**
>> + * struct xe_rtp_entry - Entry in a special section, used for "out-of-band"
>> + * definitions
>> + */
>> +struct xe_rtp_oob_entry {
>> + const char *name;
>> +};
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
>> index 100315c03014..26fb91aae014 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.c
>> +++ b/drivers/gpu/drm/xe/xe_wa.c
>> @@ -72,8 +72,8 @@
>> * engine registers are restored in a context restore sequence. This is
>> * currently not used in the driver.
>> *
>> - * - Other: There are WAs that, due to their nature, cannot be applied from a
>> - * central place. Those are peppered around the rest of the code, as needed.
>> + * - Other/OOB: There are WAs that, due to their nature, cannot be applied from
>> + * a central place. Those are peppered around the rest of the code, as needed.
>> * Workarounds related to the display IP are the main example.
>> *
>> * .. [1] Technically, some registers are powercontext saved & restored, so they
>> @@ -92,6 +92,9 @@
>> #define _MMIO(x) _XE_RTP_REG(x)
>> #define MCR_REG(x) _XE_RTP_MCR_REG(x)
>>
>> +extern const struct xe_rtp_oob_entry __start___xe_wa_oob[];
>> +extern const struct xe_rtp_oob_entry __stop___xe_wa_oob[];
>> +
>> __diag_push();
>> __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>>
>> @@ -665,6 +668,7 @@ void xe_wa_process_lrc(struct xe_hw_engine *hwe)
>> void xe_wa_dump(struct xe_device *xe, struct drm_printer *p)
>> {
>> const struct xe_rtp_entry *entry;
>> + const struct xe_rtp_oob_entry *oob_entry;
>>
>> drm_printf(p, "Workarounds\n");
>>
>> @@ -679,4 +683,10 @@ void xe_wa_dump(struct xe_device *xe, struct drm_printer *p)
>> drm_printf(p, "\tLRC\n");
>> for (entry = lrc_was; entry && entry->name; entry++)
>> drm_printf(p, "\t\t%s\n", entry->name);
>> +
>> + drm_printf(p, "\tOOB\n");
>> + for (oob_entry = __start___xe_wa_oob;
>> + oob_entry < __stop___xe_wa_oob;
>> + oob_entry++)
>> + drm_printf(p, "\t\t%s\n", oob_entry->name);
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_wa.h b/drivers/gpu/drm/xe/xe_wa.h
>> index 97d104b7e566..212fc9b4efe6 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.h
>> +++ b/drivers/gpu/drm/xe/xe_wa.h
>> @@ -6,6 +6,8 @@
>> #ifndef _XE_WA_
>> #define _XE_WA_
>>
>> +#include "xe_rtp_types.h"
>> +
>> struct drm_printer;
>> struct xe_device;
>> struct xe_gt;
>> @@ -19,4 +21,12 @@ void xe_wa_dump(struct xe_device *xe, struct drm_printer *p);
>>
>> void xe_reg_whitelist_process_engine(struct xe_hw_engine *hwe);
>>
>> +#define XE_WA(name__, cond__) ({ \
>> + static const struct xe_rtp_oob_entry entry__ \
>> + __used __aligned(8) __section("__xe_wa_oob") = { \
>> + .name = name__, \
>> + }; \
>> + cond__; \
>> +})
>> +
>> #endif
>> --
>> 2.39.0
More information about the Intel-xe
mailing list