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

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

On Fri, Sep 12, 2025 at 09:07:10AM -0500, Lucas De Marchi wrote:
> On Fri, Sep 12, 2025 at 10:05:21AM +0200, Raag Jadav wrote:
> > 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?
> 
> no, the "(yet)" means that it's only a struct to stash the bb. There
> isn't an exec queue associated to it like in the xe_bb case. The hw
> association will come later in the runtime flow, not in a future patch.
> 
> I may remove the "(yet)" in the next version to avoid confusion.

Thanks.

> > 
> > > +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?
> 
> Then maybe we'd need to change the input parsing as well :-/. Not sure
> it's worth it.

Fair.

> > 
> > > +			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)
> 
> it's actually not a zeroed page, but it is terminated:
> 
> 	if (!buffer->page)
> 		 buffer->page = (char *)__get_free_pages(GFP_KERNEL, 0);
> 	...
> 	copied = copy_from_iter(buffer->page, SIMPLE_ATTR_SIZE - 1, from);
> 	...
> 	/* if buf is assumed to contain a string, terminate it by \0,
> 	 * so e.g. sscanf() can scan the string easily */
> 	buffer->page[copied] = 0;

*facepalm* I didn't realize I was looking at read_iter().

> > 
> > > +		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.
> 
> It's basically "where info should point to in case parse_engine is
> succesfull". Pretty common pattern in parse/lookup code like this.
> 
> in this case we need to use the ret to know how many chars we parsed.
> 
> 	info = parse_engine(p, " \t\n", NULL, &len);
> 
> would avoid the double pointer, but also make it harder to follow as
> we'd need to encode the error in the pointer and use a different pattern
> than the other functions.

A NULL return for error cases would be one way, but at the cost of loosing
internal error codes which is probably controversial. In any case,

Reviewed-by: Raag Jadav <raag.jadav@intel.com>