[PATCH v5 1/6] drm/xe: add xe_device_wa infrastructure
Matt Atwood
matthew.s.atwood at intel.com
Wed Jul 2 22:09:19 UTC 2025
On Wed, Jul 02, 2025 at 04:57:34PM -0500, Lucas De Marchi wrote:
> On Wed, Jul 02, 2025 at 12:30:31PM -0700, Matt Atwood wrote:
> > There are some workarounds that must be applied before gt init,
> > wa_150154044425 for example. Instead of sprinkling them conditionally
> > throughout the driver as we did for i915 generate an oob.rules file
> > reusing the RTP infrastructure to make these easier to track.
> >
> > v2: rename xe_soc_wa to xe_device_wa
> > v5: derive prefix from argument rather then hard coding the values.
> >
> > Signed-off-by: Matt Atwood <matthew.s.atwood at intel.com>
> > ---
> > drivers/gpu/drm/xe/Makefile | 11 ++++++-
> > drivers/gpu/drm/xe/xe_device_wa_oob.rules | 0
> > drivers/gpu/drm/xe/xe_gen_wa_oob.c | 38 ++++++++++++++++++-----
> > 3 files changed, 41 insertions(+), 8 deletions(-)
> > create mode 100644 drivers/gpu/drm/xe/xe_device_wa_oob.rules
> >
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index 1d97e5b63f4e..48d4aa9d4ecc 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -21,6 +21,15 @@ $(obj)/generated/%_wa_oob.c $(obj)/generated/%_wa_oob.h: $(obj)/xe_gen_wa_oob \
> > $(src)/xe_wa_oob.rules
> > $(call cmd,wa_oob)
> >
> > +generated_device_oob := $(obj)/generated/xe_device_wa_oob.c $(obj)/generated/xe_device_wa_oob.h
> > +quiet_cmd_device_wa_oob = GEN $(notdir $(generated_device_oob))
> > + cmd_device_wa_oob = mkdir -p $(@D); $^ $(generated_device_oob)
> > +$(obj)/generated/%_device_wa_oob.c $(obj)/generated/%_device_wa_oob.h: $(obj)/xe_gen_wa_oob \
> > + $(src)/xe_device_wa_oob.rules
> > + $(call cmd,device_wa_oob)
> > +
> > +
> > +
>
> drop these several newlines.
Ack
>
> > # Please keep these build lists sorted!
> >
> > # core driver code
> > @@ -340,4 +349,4 @@ $(obj)/%.hdrtest: $(src)/%.h FORCE
> > $(call if_changed_dep,hdrtest)
> >
> > uses_generated_oob := $(addprefix $(obj)/, $(xe-y))
> > -$(uses_generated_oob): $(obj)/generated/xe_wa_oob.h
> > +$(uses_generated_oob): $(obj)/generated/xe_wa_oob.h $(obj)/generated/xe_device_wa_oob.h
> > diff --git a/drivers/gpu/drm/xe/xe_device_wa_oob.rules b/drivers/gpu/drm/xe/xe_device_wa_oob.rules
> > new file mode 100644
> > index 000000000000..e69de29bb2d1
> > diff --git a/drivers/gpu/drm/xe/xe_gen_wa_oob.c b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
> > index ed9183599e31..341840926f60 100644
> > --- a/drivers/gpu/drm/xe/xe_gen_wa_oob.c
> > +++ b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
>
> All the changes in this files should be a prep commit: they can be done
> regardless of having device_wa_oob
So split out the make file change in addition? or move the makefile
changes to the following commit?
>
> > @@ -18,8 +18,8 @@
> > " *\n" \
> > " * This file was generated from rules: %s\n" \
> > " */\n" \
> > - "#ifndef _GENERATED_XE_WA_OOB_\n" \
> > - "#define _GENERATED_XE_WA_OOB_\n" \
> > + "#ifndef _GENERATED_%s_\n" \
> > + "#define _GENERATED_%s_\n" \
> > "\n" \
> > "enum {\n"
> >
> > @@ -52,7 +52,7 @@ static char *strip(char *line, size_t linelen)
> > }
> >
> > #define MAX_LINE_LEN 4096
> > -static int parse(FILE *input, FILE *csource, FILE *cheader)
> > +static int parse(FILE *input, FILE *csource, FILE *cheader, char *prefix)
> > {
> > char line[MAX_LINE_LEN + 1];
> > char *name, *prev_name = NULL, *rules;
> > @@ -96,7 +96,7 @@ static int parse(FILE *input, FILE *csource, FILE *cheader)
> > }
> >
> > if (name) {
> > - fprintf(cheader, "\tXE_WA_OOB_%s = %u,\n", name, idx);
> > + fprintf(cheader, "\t%s_%s = %u,\n", prefix, name, idx);
> >
> > /* Close previous entry before starting a new one */
> > if (idx)
> > @@ -118,7 +118,26 @@ static int parse(FILE *input, FILE *csource, FILE *cheader)
> > if (idx)
> > fprintf(csource, ") },\n");
> >
> > - fprintf(cheader, "\t_XE_WA_OOB_COUNT = %u\n", idx);
> > + fprintf(cheader, "\t_%s_COUNT = %u\n", prefix, idx);
> > +
> > + return 0;
> > +}
> > +
> > +static int fn_to_prefix(const char *fn, char *prefix, size_t size)
> > +{
> > + if (!strncpy(prefix, fn, size - 1))
> > + return -ENAMETOOLONG;
>
> I didn't suggest using strncpy(). If you do, then you need to adjust the
> error handling. There are 2 problems here:
Ack.
>
> 1) strncpy() doesn't return NULL on error. See CAVEATS section
> in strncpy(3) or strlcpy(3) that also documents strncpy and its
> return value:
>
>
> strcpy(3)
> strcat(3)
> strncpy(3)
> strncat(3)
> The dst pointer, which is useless.
>
> 2) prefix is on the stack in the caller, so it doesn't necessarily
> ends with '\0'. So even if you copied at most size - 1, the last
> char may not be 0 and you will have an infinity loop below.
>
> Those 2 could be fixed by doing this instead:
>
> diff --git a/drivers/gpu/drm/xe/xe_gen_wa_oob.c b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
> index 341840926f602..ed550dca5f439 100644
> --- a/drivers/gpu/drm/xe/xe_gen_wa_oob.c
> +++ b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
> @@ -125,7 +125,7 @@ static int parse(FILE *input, FILE *csource, FILE *cheader, char *prefix)
> static int fn_to_prefix(const char *fn, char *prefix, size_t size)
> {
> - if (!strncpy(prefix, fn, size - 1))
> + if (strlcpy(prefix, basename(fn), size) >= size)
> return -ENAMETOOLONG;
> for (char *p = prefix; *p; p++) {
> @@ -168,8 +168,10 @@ int main(int argc, const char *argv[])
> return 1;
> }
> - if (fn_to_prefix(basename(args[ARGS_CHEADER].fn), prefix, sizeof(prefix)) < 0)
> + if (fn_to_prefix(args[ARGS_CHEADER].fn, prefix, sizeof(prefix)) < 0) {
> + fprintf(stderr, "ERROR: invalid header name: %s\n", args[ARGS_CHEADER].fn);
> return 1;
> + }
> for (int i = 0; i < _ARGS_COUNT; i++) {
> args[i].f = fopen(args[i].fn, args[i].mode);
>
> did you find any issue with strlcpy? I think it's available on all
> toolchains, but maybe we need the workaround by including tools/include/linux/string.h
> I tried the diff above locally and it worked for me though.
strlcpy isnt currently supported in the distribution recommended for
simulation (ubuntu 22.04). Using this would cause team members that are
using recommended distro for simulations unable to build the kernel.
>
> Lucas De Marchi
MattA
>
> > +
> > + for (char *p = prefix; *p; p++) {
> > + switch (*p) {
> > + case '.':
> > + *p = '\0';
> > + return 0;
> > + default:
> > + *p = toupper(*p);
> > + break;
> > + }
> > + }
> >
> > return 0;
> > }
> > @@ -141,6 +160,7 @@ int main(int argc, const char *argv[])
> > [ARGS_CHEADER] = { .fn = argv[3], .mode = "w" },
> > };
> > int ret = 1;
> > + char prefix[128];
> >
> > if (argc < 3) {
> > fprintf(stderr, "ERROR: wrong arguments\n");
> > @@ -148,6 +168,9 @@ int main(int argc, const char *argv[])
> > return 1;
> > }
> >
> > + if (fn_to_prefix(basename(args[ARGS_CHEADER].fn), prefix, sizeof(prefix)) < 0)
> > + return 1;
> > +
> > for (int i = 0; i < _ARGS_COUNT; i++) {
> > args[i].f = fopen(args[i].fn, args[i].mode);
> > if (!args[i].f) {
> > @@ -157,9 +180,10 @@ int main(int argc, const char *argv[])
> > }
> > }
> >
> > - fprintf(args[ARGS_CHEADER].f, HEADER, args[ARGS_INPUT].fn);
> > + fprintf(args[ARGS_CHEADER].f, HEADER, args[ARGS_INPUT].fn, prefix, prefix);
> > +
> > ret = parse(args[ARGS_INPUT].f, args[ARGS_CSOURCE].f,
> > - args[ARGS_CHEADER].f);
> > + args[ARGS_CHEADER].f, prefix);
> > if (!ret)
> > fprintf(args[ARGS_CHEADER].f, FOOTER);
> >
> > --
> > 2.49.0
> >
More information about the Intel-xe
mailing list