Re: [PATCH v4 6/6] drm/xe/configfs: Add post context restore bb

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Sep 11, 2025 at 12:36:30PM -0700, Lucas De Marchi wrote:
> Allow the user to specify commands to execute during a context restore.
> Currently it's possible to parse 2 types of actions:
> 
> 	- cmd: the instructions are added as is to the bb
> 	- reg: just use the address and value, without worrying about
> 	  encoding the right LRI instruction. This is possibly the most
> 	  useful use case, so added a dedicated action for that.
> 
> This also prepares for future BBs: mid context restore and rc6 context
> restore that can re-use the same parsing functions.

...

> @@ -123,11 +156,18 @@
>   *	# rmdir /sys/kernel/config/xe/0000:03:00.0/
>   */
>  
> +/* Similar to struct xe_bb, but not tied to HW (yet) */

Should I assume we plan to do it at some point?

> +struct wa_bb {
> +	u32 *cs;
> +	u32 len; /* in dwords */
> +};

...

> +static ssize_t wa_bb_show(struct xe_config_group_device *dev,
> +			  struct wa_bb wa_bb[static XE_ENGINE_CLASS_MAX],
> +			  char *data, size_t sz)
> +{
> +	char *p = data;
> +
> +	guard(mutex)(&dev->lock);
> +
> +	for (size_t i = 0; i < ARRAY_SIZE(engine_info); i++) {
> +		enum xe_engine_class ec = engine_info[i].engine_class;
> +		size_t len;
> +
> +		if (!wa_bb[ec].len)
> +			continue;
> +
> +		len = snprintf(p, sz, "%s:", engine_info[i].cls);
> +		if (!wa_bb_read_advance(data, &p, NULL, len, &sz))
> +			return -ENOBUFS;
> +
> +		for (size_t j = 0; j < wa_bb[ec].len; j++) {
> +			len = snprintf(p, sz, " %08x", wa_bb[ec].cs[j]);

Should we use '0x' prefix?

> +			if (!wa_bb_read_advance(data, &p, NULL, len, &sz))
> +				return -ENOBUFS;
> +		}
> +
> +		if (!wa_bb_read_advance(data, &p, "\n", 1, &sz))
> +			return -ENOBUFS;
> +	}
> +
> +	if (!wa_bb_read_advance(data, &p, "", 1, &sz))
> +		return -ENOBUFS;
> +
> +	/* Reserve one more to match check for '\0' */
> +	if (!data)
> +		p++;
> +
> +	return p - data;
> +}

...

> +static ssize_t parse_wa_bb_lines(const char *lines,
> +				 struct wa_bb wa_bb[static XE_ENGINE_CLASS_MAX])
> +{
> +	ssize_t dwords = 0, ret;
> +	const char *p;
> +
> +	for (p = lines; *p; p++) {

I found it to be zeroed page but does it gurantee termination at page
boundary? (just thinking out loud about overrun cases)

> +		const struct engine_info *info = NULL;
> +		u32 val, val2;
> +
> +		/* Also allow empty lines */
> +		p += strspn(p, " \t\n");
> +		if (!*p)
> +			break;
> +
> +		ret = parse_engine(p, " \t\n", NULL, &info);

Nit: Can we work without a double pointer? I've always found it easier to read.

> +		if (ret < 0)
> +			return ret;
> +
> +		p += ret;
> +		p += strspn(p, " \t");
> +
> +		if (str_has_prefix(p, "cmd")) {
> +			for (p += strlen("cmd"); *p;) {
> +				ret = parse_hex(p, &val);
> +				if (ret < 0)
> +					return -EINVAL;
> +				if (!ret)
> +					break;
> +
> +				p += ret;
> +				dwords++;
> +				wa_bb_append(&wa_bb[info->engine_class], val);
> +			}
> +		} else if (str_has_prefix(p, "reg")) {
> +			p += strlen("reg");
> +			ret = parse_hex(p, &val);
> +			if (ret <= 0)
> +				return -EINVAL;
> +
> +			p += ret;
> +			ret = parse_hex(p, &val2);
> +			if (ret <= 0)
> +				return -EINVAL;
> +
> +			p += ret;
> +			dwords += 3;
> +			wa_bb_append(&wa_bb[info->engine_class],
> +				     MI_LOAD_REGISTER_IMM | MI_LRI_NUM_REGS(1));
> +			wa_bb_append(&wa_bb[info->engine_class], val);
> +			wa_bb_append(&wa_bb[info->engine_class], val2);
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return dwords;
> +}

...

> +CONFIGFS_ATTR(, ctx_restore_post_bb);

I know this is alphabetic order but let's be consistent, either at the top ...

>  CONFIGFS_ATTR(, enable_psmi);
>  CONFIGFS_ATTR(, engines_allowed);
>  CONFIGFS_ATTR(, survivability_mode);
> @@ -379,6 +640,7 @@ static struct configfs_attribute *xe_config_device_attrs[] = {
>  	&attr_enable_psmi,
>  	&attr_engines_allowed,
>  	&attr_survivability_mode,
> +	&attr_ctx_restore_post_bb,

... or bottom.

>  	NULL,
>  };

...

> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index f337954066f55..c706585611d55 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -8,6 +8,7 @@
>  #include <generated/xe_wa_oob.h>
>  
>  #include <linux/ascii85.h>
> +#include <linux/panic.h>
>  
>  #include "instructions/xe_mi_commands.h"
>  #include "instructions/xe_gfxpipe_commands.h"
> @@ -1120,6 +1121,12 @@ static ssize_t setup_configfs_post_ctx_restore_bb(struct xe_lrc *lrc,
>  	if (count > max_len)
>  		return -ENOSPC;
>  
> +	/*
> +	 * This should be used only for tests and validation. Taint the kernel
> +	 * as anything could be submitted directly in context switches
> +	 */
> +	add_taint(TAINT_TEST, LOCKDEP_STILL_OK);

I know we add wa_bb support in this patch but perhaps this belongs to
previous patch?

Raag

>  	memcpy(cmd, user_batch, count * sizeof(u32));
>  	cmd += count;
>  
> 
> -- 
> 2.50.1
>