[Intel-xe] [PATCH v4 12/21] drm/xe: Add support for OOB workarounds

Lucas De Marchi lucas.demarchi at intel.com
Fri May 26 04:59:39 UTC 2023


On Thu, May 25, 2023 at 04:13:27PM -0700, Matt Roper wrote:
>On Thu, May 25, 2023 at 02:33:58PM -0700, Lucas De Marchi wrote:
>> There are WAs that, due to their nature, cannot be applied from a
>> central place like xe_wa.c. Those are peppered around the rest of the
>> code, as needed. Now they have a new name:  "out-of-band workarounds".
>>
>> These workarounds have their names and rules still grouped in xe_wa.c,
>> inside the xe_wa_oob array, which is generated at compile time by
>> xe_wa_oob.rules and the hostprog xe_gen_wa_oob. The code generation
>> guarantees that the header xe_wa_oob.h contains the IDs for the
>> workarounds that match the index in the table. This way the runtime
>> checks that are spread throughout the code are simple tests against the
>> bitmap saved during initialization.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/Makefile        |  14 +++
>>  drivers/gpu/drm/xe/xe_gen_wa_oob.c | 164 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_gt.c         |   1 +
>>  drivers/gpu/drm/xe/xe_gt_types.h   |   2 +
>>  drivers/gpu/drm/xe/xe_wa.c         |  40 ++++++-
>>  drivers/gpu/drm/xe/xe_wa.h         |   9 ++
>>  drivers/gpu/drm/xe/xe_wa_oob.rules |   0
>>  7 files changed, 226 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/gpu/drm/xe/xe_gen_wa_oob.c
>>  create mode 100644 drivers/gpu/drm/xe/xe_wa_oob.rules
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index e058b1dda6e6..738975faaa05 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -26,6 +26,20 @@ subdir-ccflags-$(CONFIG_DRM_XE_WERROR) += -Werror
>>
>>  subdir-ccflags-y += -I$(obj) -I$(srctree)/$(src)
>>
>> +# generated sources
>> +hostprogs := xe_gen_wa_oob
>> +
>> +XE_WA_OOB := $(obj)/generated/xe_wa_oob.c $(obj)/generated/xe_wa_oob.h
>> +
>> +quiet_cmd_wa_oob = GEN     xe_wa_oob.[ch]
>
>I think we could use "$(notdir $(XE_WA_OOB))" here to avoid hardcoding
>the names?

yeah, sounds good.

>
>Anyway, the rest looks okay.
>
>Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>
>> +      cmd_wa_oob = $^ $(XE_WA_OOB)
>> +
>> +$(XE_WA_OOB) &: $(obj)/xe_gen_wa_oob $(srctree)/$(src)/xe_wa_oob.rules
>> +	@mkdir -p $(@D)
>> +	$(call cmd,wa_oob)
>> +
>> +%.o:  $(XE_WA_OOB)

I don't know why, but when building for kunit this is not working for
me.  I'm having to replace it with:

$(obj)/xe_wa.o $(obj)/xe_guc.o: $(XE_WA_OOB)

but that is ugly and with the workarounds spread throughout the codebase
it will be a pain to maintain this in the build system. Any alternative?

Lucas De Marchi

>> +
>>  # Please keep these build lists sorted!
>>
>>  # core driver code
>> diff --git a/drivers/gpu/drm/xe/xe_gen_wa_oob.c b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
>> new file mode 100644
>> index 000000000000..930cdb7ccf52
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_gen_wa_oob.c
>> @@ -0,0 +1,164 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#define _GNU_SOURCE
>> +#include <ctype.h>
>> +#include <errno.h>
>> +#include <stdbool.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +#define HEADER \
>> +	"// SPDX-License-Identifier: MIT\n" \
>> +	"\n" \
>> +	"/*\n" \
>> +	" * DO NOT MODIFY.\n" \
>> +	" *\n" \
>> +	" * This file was generated from rules: %s\n" \
>> +	" */\n" \
>> +	"#ifndef _GENERATED_XE_WA_OOB_\n" \
>> +	"#define _GENERATED_XE_WA_OOB_\n" \
>> +	"\n" \
>> +	"enum {\n"
>> +
>> +#define FOOTER \
>> +	"};\n" \
>> +	"\n" \
>> +	"#endif\n"
>> +
>> +static void print_usage(FILE *f)
>> +{
>> +	fprintf(f, "usage: %s <input-rule-file> <generated-c-source-file> <generated-c-header-file>\n",
>> +		program_invocation_short_name);
>> +}
>> +
>> +static void print_parse_error(const char *err_msg, const char *line,
>> +			      unsigned int lineno)
>> +{
>> +	fprintf(stderr, "ERROR: %s\nERROR: %u: %.60s\n",
>> +		err_msg, lineno, line);
>> +}
>> +
>> +static char *strip(char *line, size_t linelen)
>> +{
>> +	while (isspace(*(line + linelen)))
>> +		linelen--;
>> +
>> +	line[linelen - 1] = '\0';
>> +
>> +	return  line + strspn(line, " \f\n\r\t\v");
>> +}
>> +
>> +#define MAX_LINE_LEN 4096
>> +static int parse(FILE *input, FILE *csource, FILE *cheader)
>> +{
>> +	char line[MAX_LINE_LEN + 1];
>> +	char *name, *prev_name = NULL, *rules;
>> +	unsigned int lineno = 0, idx = 0;
>> +
>> +	while (fgets(line, sizeof(line), input)) {
>> +		size_t linelen;
>> +		bool is_continuation;
>> +
>> +		if (line[0] == '\0' || line[0] == '#' || line[0] == '\n') {
>> +			lineno++;
>> +			continue;
>> +		}
>> +
>> +		linelen = strlen(line);
>> +		if (linelen == MAX_LINE_LEN) {
>> +			print_parse_error("line too long", line, lineno);
>> +			return -EINVAL;
>> +		}
>> +
>> +		is_continuation = isspace(line[0]);
>> +		name = strip(line, linelen);
>> +
>> +		if (!is_continuation) {
>> +			name = strtok(name, " \t");
>> +			rules = strtok(NULL, "");
>> +		} else {
>> +			if (!prev_name) {
>> +				print_parse_error("invalid rule continuation",
>> +						  line, lineno);
>> +				return -EINVAL;
>> +			}
>> +
>> +			rules = name;
>> +			name = NULL;
>> +		}
>> +
>> +		if (rules[0] == '\0') {
>> +			print_parse_error("invalid empty rule\n", line, lineno);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (name) {
>> +			fprintf(cheader, "\tXE_WA_OOB_%s = %u,\n", name, idx);
>> +			fprintf(csource, "{ XE_RTP_NAME(\"%s\"), XE_RTP_RULES(%s) },\n",
>> +				name, rules);
>> +		} else {
>> +			fprintf(csource, "{ XE_RTP_NAME(NULL), XE_RTP_RULES(%s) },\n",
>> +				rules);
>> +		}
>> +
>> +		idx++;
>> +		lineno++;
>> +		prev_name = name;
>> +	}
>> +
>> +	fprintf(cheader, "\t_XE_WA_OOB_COUNT = %u\n", idx);
>> +
>> +	return 0;
>> +}
>> +
>> +int main(int argc, const char *argv[])
>> +{
>> +	enum {
>> +		ARGS_INPUT,
>> +		ARGS_CSOURCE,
>> +		ARGS_CHEADER,
>> +		_ARGS_COUNT
>> +	};
>> +	struct {
>> +		const char *fn;
>> +		const char *mode;
>> +		FILE *f;
>> +	} args[] = {
>> +		[ARGS_INPUT] = { .fn = argv[1], .mode = "r" },
>> +		[ARGS_CSOURCE] = { .fn = argv[2], .mode = "w" },
>> +		[ARGS_CHEADER] = { .fn = argv[3], .mode = "w" },
>> +	};
>> +	int ret = 1;
>> +
>> +	if (argc < 3) {
>> +		fprintf(stderr, "ERROR: wrong arguments\n");
>> +		print_usage(stderr);
>> +		return 1;
>> +	}
>> +
>> +	for (int i = 0; i < _ARGS_COUNT; i++) {
>> +		args[i].f = fopen(args[i].fn, args[i].mode);
>> +		if (!args[i].f) {
>> +			fprintf(stderr, "ERROR: Can't open %s: %m\n",
>> +				args[i].fn);
>> +			goto err;
>> +		}
>> +	}
>> +
>> +	fprintf(args[ARGS_CHEADER].f, HEADER, args[ARGS_INPUT].fn);
>> +	ret = parse(args[ARGS_INPUT].f, args[ARGS_CSOURCE].f,
>> +		    args[ARGS_CHEADER].f);
>> +	if (!ret)
>> +		fprintf(args[ARGS_CHEADER].f, FOOTER);
>> +
>> +err:
>> +	for (int i = 0; i < _ARGS_COUNT; i++) {
>> +		if (args[i].f)
>> +			fclose(args[i].f);
>> +	}
>> +
>> +	return ret;
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index d139554316d4..18eda5b1377f 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -321,6 +321,7 @@ int xe_gt_init_early(struct xe_gt *gt)
>>  		return err;
>>
>>  	xe_wa_process_gt(gt);
>> +	xe_wa_process_oob(gt);
>>  	xe_tuning_process_gt(gt);
>>
>>  	return 0;
>> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
>> index 017ab60f2498..b83c834e7ced 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_types.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
>> @@ -368,6 +368,8 @@ struct xe_gt {
>>  		unsigned long *engine;
>>  		/** @lrc: bitmap with active LRC workarounds */
>>  		unsigned long *lrc;
>> +		/** @oob: bitmap with active OOB workaroudns */
>> +		unsigned long *oob;
>>  	} wa_active;
>>  };
>>
>> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
>> index b717e0025bd9..ded100245479 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.c
>> +++ b/drivers/gpu/drm/xe/xe_wa.c
>> @@ -9,6 +9,7 @@
>>  #include <kunit/visibility.h>
>>  #include <linux/compiler_types.h>
>>
>> +#include "generated/xe_wa_oob.h"
>>  #include "regs/xe_engine_regs.h"
>>  #include "regs/xe_gt_regs.h"
>>  #include "regs/xe_regs.h"
>> @@ -73,8 +74,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
>> @@ -579,8 +580,31 @@ static const struct xe_rtp_entry_sr lrc_was[] = {
>>  	{}
>>  };
>>
>> +static __maybe_unused const struct xe_rtp_entry oob_was[] = {
>> +#include <generated/xe_wa_oob.c>
>> +	{}
>> +};
>> +
>> +static_assert(ARRAY_SIZE(oob_was) - 1 == _XE_WA_OOB_COUNT);
>> +
>>  __diag_pop();
>>
>> +/**
>> + * xe_wa_process_oob - process OOB workaround table
>> + * @gt: GT instance to process workarounds for
>> + *
>> + * Process OOB workaround table for this platform, marking in @gt the
>> + * workarounds that are active.
>> + */
>> +void xe_wa_process_oob(struct xe_gt *gt)
>> +{
>> +	struct xe_rtp_process_ctx ctx = XE_RTP_PROCESS_CTX_INITIALIZER(gt);
>> +
>> +	xe_rtp_process_ctx_enable_active_tracking(&ctx, gt->wa_active.oob,
>> +						  ARRAY_SIZE(oob_was));
>> +	xe_rtp_process(&ctx, oob_was);
>> +}
>> +
>>  /**
>>   * xe_wa_process_gt - process GT workaround table
>>   * @gt: GT instance to process workarounds for
>> @@ -641,13 +665,14 @@ void xe_wa_process_lrc(struct xe_hw_engine *hwe)
>>  int xe_wa_init(struct xe_gt *gt)
>>  {
>>  	struct xe_device *xe = gt_to_xe(gt);
>> -	size_t n_lrc, n_engine, n_gt, total;
>> +	size_t n_oob, n_lrc, n_engine, n_gt, total;
>>  	unsigned long *p;
>>
>>  	n_gt = BITS_TO_LONGS(ARRAY_SIZE(gt_was));
>>  	n_engine = BITS_TO_LONGS(ARRAY_SIZE(engine_was));
>>  	n_lrc = BITS_TO_LONGS(ARRAY_SIZE(lrc_was));
>> -	total = n_gt + n_engine + n_lrc;
>> +	n_oob = BITS_TO_LONGS(ARRAY_SIZE(oob_was));
>> +	total = n_gt + n_engine + n_lrc + n_oob;
>>
>>  	p = drmm_kzalloc(&xe->drm, sizeof(*p) * total, GFP_KERNEL);
>>  	if (!p)
>> @@ -658,6 +683,8 @@ int xe_wa_init(struct xe_gt *gt)
>>  	gt->wa_active.engine = p;
>>  	p += n_engine;
>>  	gt->wa_active.lrc = p;
>> +	p += n_lrc;
>> +	gt->wa_active.oob = p;
>>
>>  	return 0;
>>  }
>> @@ -677,4 +704,9 @@ void xe_wa_dump(struct xe_gt *gt, struct drm_printer *p)
>>  	drm_printf(p, "\nLRC Workarounds\n");
>>  	for_each_set_bit(idx, gt->wa_active.lrc, ARRAY_SIZE(lrc_was))
>>  		drm_printf_indent(p, 1, "%s\n", lrc_was[idx].name);
>> +
>> +	drm_printf(p, "\nOOB Workarounds\n");
>> +	for_each_set_bit(idx, gt->wa_active.oob, ARRAY_SIZE(oob_was))
>> +		if (oob_was[idx].name)
>> +			drm_printf_indent(p, 1, "%s\n", oob_was[idx].name);
>>  }
>> diff --git a/drivers/gpu/drm/xe/xe_wa.h b/drivers/gpu/drm/xe/xe_wa.h
>> index defefa5d9611..cfe685989524 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.h
>> +++ b/drivers/gpu/drm/xe/xe_wa.h
>> @@ -11,6 +11,7 @@ struct xe_gt;
>>  struct xe_hw_engine;
>>
>>  int xe_wa_init(struct xe_gt *gt);
>> +void xe_wa_process_oob(struct xe_gt *gt);
>>  void xe_wa_process_gt(struct xe_gt *gt);
>>  void xe_wa_process_engine(struct xe_hw_engine *hwe);
>>  void xe_wa_process_lrc(struct xe_hw_engine *hwe);
>> @@ -18,4 +19,12 @@ void xe_wa_process_lrc(struct xe_hw_engine *hwe);
>>  void xe_reg_whitelist_process_engine(struct xe_hw_engine *hwe);
>>  void xe_wa_dump(struct xe_gt *gt, struct drm_printer *p);
>>
>> +/**
>> + * XE_WA - Out-of-band workarounds, that don't fit the lifecycle any
>> + *         other more specific type
>> + * @gt__: gt instance
>> + * @id__: XE_OOB_<id__>, as generated by build system in generated/xe_wa_oob.h
>> + */
>> +#define XE_WA(gt__, id__) test_bit(XE_WA_OOB_ ## id__, (gt__)->wa_active.oob)
>> +
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> new file mode 100644
>> index 000000000000..e69de29bb2d1
>> --
>> 2.40.1
>>
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation


More information about the Intel-xe mailing list