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

Matt Roper matthew.d.roper at intel.com
Thu May 25 23:13:27 UTC 2023


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?

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)
> +
>  # 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