Re: [PATCH v2 2/6] drm/xe/configfs: Allow to select by class only

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

On Wed, Aug 27, 2025 at 03:35:27PM -0700, Lucas De Marchi wrote:
> For a future configfs attribute, it's desirable to select by engine mask
> only as the instance doesn't make sense.
> 
> If the caller is only interested in class, allow lookup_engine_mask() to
> return the matched index if mask is NULL. This allows parse_engine() to
> still return an item if the caller wants to allow parsing a class-only
> string like "rcs", "bcs", "ccs", etc.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_configfs.c | 53 ++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> index 6cccab5456811..a1d230007cc05 100644
> --- a/drivers/gpu/drm/xe/xe_configfs.c
> +++ b/drivers/gpu/drm/xe/xe_configfs.c
> @@ -150,6 +150,7 @@ static void set_device_defaults(struct xe_config_device *config)
>  struct engine_info {
>  	const char *cls;
>  	u64 mask;
> +	enum xe_engine_class engine_class;
>  };
>  
>  /* Some helpful macros to aid on the sizing of buffer allocation when parsing */
> @@ -157,12 +158,12 @@ struct engine_info {
>  #define MAX_ENGINE_INSTANCE_CHARS 2
>  
>  static const struct engine_info engine_info[] = {
> -	{ .cls = "rcs", .mask = XE_HW_ENGINE_RCS_MASK },
> -	{ .cls = "bcs", .mask = XE_HW_ENGINE_BCS_MASK },
> -	{ .cls = "vcs", .mask = XE_HW_ENGINE_VCS_MASK },
> -	{ .cls = "vecs", .mask = XE_HW_ENGINE_VECS_MASK },
> -	{ .cls = "ccs", .mask = XE_HW_ENGINE_CCS_MASK },
> -	{ .cls = "gsccs", .mask = XE_HW_ENGINE_GSCCS_MASK },
> +	{ .cls = "rcs", .mask = XE_HW_ENGINE_RCS_MASK, XE_ENGINE_CLASS_RENDER },
> +	{ .cls = "bcs", .mask = XE_HW_ENGINE_BCS_MASK, XE_ENGINE_CLASS_COPY },
> +	{ .cls = "vcs", .mask = XE_HW_ENGINE_VCS_MASK, XE_ENGINE_CLASS_VIDEO_DECODE },
> +	{ .cls = "vecs", .mask = XE_HW_ENGINE_VECS_MASK, XE_ENGINE_CLASS_VIDEO_ENHANCE },
> +	{ .cls = "ccs", .mask = XE_HW_ENGINE_CCS_MASK, XE_ENGINE_CLASS_COMPUTE },
> +	{ .cls = "gsccs", .mask = XE_HW_ENGINE_GSCCS_MASK, XE_ENGINE_CLASS_OTHER },

Should this have ".engine_class =" for consistency?

>  static struct xe_config_group_device *to_xe_config_group_device(struct config_item *item)
> @@ -251,7 +252,19 @@ static ssize_t engines_allowed_show(struct config_item *item, char *page)
>  	return p - page;
>  }
>  
> -static bool lookup_engine_mask(const char *pattern, u64 *mask)
> +/*
> + * Lookup engine index from engine_info. If @mask is not NULL, reduce the mask
> + * according to the instance in @pattern.
> + *
> + * Examples of inputs:
> + *
> + * - lookup_engine_mask("rcs0", &mask): return "rcs" index from @engine_info and
> + *   mask == BIT_ULL(XE_HW_ENGINE_RCS0)
> + * - lookup_engine_mask("rcs*", &mask): return "rcs" index from @engine_info and
> + *   mask == XE_HW_ENGINE_RCS_MASK
> + * - lookup_engine_mask("rcs", NULL): return "rcs" index from @engine_info
> + */
> +static int lookup_engine_mask(const char *pattern, u64 *mask)

Should this now be lookup_engine_index()?

>  	for (size_t i = 0; i < ARRAY_SIZE(engine_info); i++) {
>  		u8 instance;
> @@ -261,30 +274,34 @@ static bool lookup_engine_mask(const char *pattern, u64 *mask)
>  			continue;
>  
>  		pattern += strlen(engine_info[i].cls);
> +		if (!mask && !*pattern)
> +			return i;
>  
>  		if (!strcmp(pattern, "*")) {
>  			*mask = engine_info[i].mask;
> -			return true;
> +			return i;
>  		}
>  
>  		if (kstrtou8(pattern, 10, &instance))
> -			return false;
> +			return -ENOENT;
>  
>  		bit = __ffs64(engine_info[i].mask) + instance;
>  		if (bit >= fls64(engine_info[i].mask))
> -			return false;
> +			return -ENOENT;
>  
>  		*mask = BIT_ULL(bit);
> -		return true;
> +		return i;
>  	}
>  
> -	return false;
> +	return -ENOENT;
>  }
>  
> -static int parse_engine(const char *s, const char *end_chars, u64 *mask)
> +static int parse_engine(const char *s, const char *end_chars, u64 *mask,
> +			const struct engine_info **info)

Considering we're parsing a single entry, do we really need a double pointer?

Raag

>  {
>  	char buf[MAX_ENGINE_CLASS_CHARS + MAX_ENGINE_INSTANCE_CHARS + 1];
>  	size_t len;
> +	int idx;
>  
>  	len = strcspn(s, end_chars);
>  	if (len >= sizeof(buf))
> @@ -293,8 +310,12 @@ static int parse_engine(const char *s, const char *end_chars, u64 *mask)
>  	memcpy(buf, s, len);
>  	buf[len] = '\0';
>  
> -	if (!lookup_engine_mask(buf, mask))
> -		return -ENOENT;
> +	idx = lookup_engine_mask(buf, mask);
> +	if (idx < 0)
> +		return idx;
> +
> +	if (info)
> +		*info = &engine_info[idx];
>  
>  	return len;
>  }
> @@ -307,7 +328,7 @@ static ssize_t engines_allowed_store(struct config_item *item, const char *page,
>  	u64 mask, val = 0;
>  
>  	for (p = 0; p < len; p += patternlen + 1) {
> -		patternlen = parse_engine(page + p, ",\n", &mask);
> +		patternlen = parse_engine(page + p, ",\n", &mask, NULL);
>  		if (patternlen < 0)
>  			return -EINVAL;
>  
> 
> -- 
> 2.50.1
>