Message-Id: <DF5NE5WRSCYT.4NV0451K0SRU@bootlin.com>
Date: Tue, 23 Dec 2025 14:58:41 +0100
From: "Luca Ceresoli" <luca.ceresoli@...tlin.com>
To: "Louis Chauvet" <louis.chauvet@...tlin.com>, "Haneen Mohammed"
 <hamohammed.sa@...il.com>, "Simona Vetter" <simona@...ll.ch>, "Melissa Wen"
 <melissa.srw@...il.com>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "David Airlie"
 <airlied@...il.com>, <jose.exposito89@...il.com>, "Jonathan Corbet"
 <corbet@....net>
Cc: <victoria@...tem76.com>, <sebastian.wick@...hat.com>,
 <thomas.petazzoni@...tlin.com>, <dri-devel@...ts.freedesktop.org>,
 <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v3 18/33] drm/vkms: Introduce configfs for plane format

On Mon Dec 22, 2025 at 11:11 AM CET, Louis Chauvet wrote:
> To allow the userspace to test many hardware configuration, introduce a
> new interface to configure the available formats per planes. VKMS supports
> multiple formats, so the userspace can choose any combination.
>
> The supported formats are configured by writing the fourcc code in
> supported_formats:
>  # enable AR24 format
>   echo '+AR24' > /config/vkms/DEVICE_1/planes/PLANE_1/supported_formats
>  # disable AR24 format
>   echo '-AR24' > /config/vkms/DEVICE_1/planes/PLANE_1/supported_formats
>  # enable all format supported by VKMS
>   echo '+*' > /config/vkms/DEVICE_1/planes/PLANE_1/supported_formats
>  # disable all formats
>   echo '-*' > /config/vkms/DEVICE_1/planes/PLANE_1/supported_formats
>
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>

> --- a/Documentation/ABI/testing/configfs-vkms
> +++ b/Documentation/ABI/testing/configfs-vkms
> @@ -153,6 +153,15 @@ Description:
>          Default color range presented to userspace, same
>          values as supported_color_ranges.
>
> +What:		/sys/kernel/config/vkms/<device>/planes/<plane>/supported_formats
> +Date:		Nov 2025

Jan 2026.

> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -87,7 +87,7 @@ Start by creating one or more planes::
>
>    sudo mkdir /config/vkms/my-vkms/planes/plane0
>
> -Planes have 8 configurable attributes:
> +Planes have 9 configurable attributes:
>
>  - type: Plane type: 0 overlay, 1 primary, 2 cursor (same values as those
>    exposed by the "type" property of a plane)
> @@ -109,6 +109,11 @@ Planes have 8 configurable attributes:
>    must be set too.
>  - default_color_range: Default color range presented to the userspace, same
>    values as supported_color_ranges
> +- supported_formats: List of supported formats for this plane. To add a new item in the
> +  list, write it using a plus and fourcc code: +XR24
> +  To remove a format, use a minus and its fourcc: -XR24

>From the docs examples it's not obvious that you can add/remove multiple
formats in one write operation ("+XR24 -RG24"), but the implementation
allows it. So either add a more complete example or forbid multiple
operations in one write. I would consider the latter option seriously
because it would simplify the string parsing code, which is very tricky to
get right and robust.

> +++ b/drivers/gpu/drm/vkms/tests/vkms_configfs_test.c
> @@ -0,0 +1,102 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include "linux/printk.h"
> +#include <kunit/test.h>
> +
> +#include "../vkms_configfs.h"
> +
> +MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
> +
> +/**
> + * struct vkms_configfs_parse_format_case - Store test case for format parsing
> + * @str: Contains the string to parse
> + * @str_len: str len
> + * @expected_len: expected len of the matched format
> + * @expected_offset: expected offset in the string for the parsed format
> + */
> +struct vkms_configfs_parse_format_case {
> +	const char *str;
> +	int str_len;
> +	int expected_len;
> +	int expected_offset;
> +};
> +
> +struct vkms_configfs_parse_format_case vkms_configfs_parse_format_test_cases[] = {
> +	{
> +		.str = "+RG24",
> +		.str_len = 6,
> +		.expected_len = 5,
> +		.expected_offset = 0,

Thanks for having renamed 'data' to 'str'! However now I realize the
'str_len' name becomes misleading: the string length does not include the
training NUL character, while the value you need here does. I beg your
pardon... I guess 'str_len' should be renamed too, maybe to 'str_size' if
no better name comes to mind.

> +	}, {

Based on the question I asked after v3,resend and on your answer, I'd add a
clarifying comment here about the following test:

	   /* ensure the algorithm stops at data_len and not \0 */

> +		.str = "-R1111",
> +		.str_len = 3,
> +		.expected_len = 3,
> +		.expected_offset = 0
> +	}

Testing wrong and corner cases is more important than testing perfectly
clean cases. So it would be nice to add tests for not-obviously-wrong and
definitely-wrong cases, such as "+ RG24" (note the space), "fubar", "+**",
"*+", "++", "-+", "-A*42" (see below), ":-)" (dash after non-blank char)
and "(-o-)".

> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -3,6 +3,8 @@
>  #include <linux/configfs.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/string.h>
> +#include <kunit/visibility.h>
>
>  #include "vkms_drv.h"
>  #include "vkms_config.h"
> @@ -628,6 +630,120 @@ static ssize_t plane_default_color_encoding_store(struct config_item *item,
>  	return count;
>  }
>
> +static ssize_t plane_supported_formats_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs_plane *plane;
> +
> +	plane = plane_item_to_vkms_configfs_plane(item);
> +
> +	page[0] = '\0';
> +
> +	scoped_guard(mutex, &plane->dev->lock) {
> +		u32 *formats = vkms_config_plane_get_supported_formats(plane->config);
> +
> +		for (int i = 0;
> +		     i < vkms_config_plane_get_supported_formats_count(plane->config);
> +		     i++) {
> +			char tmp[6] = { 0 };
> +			const ssize_t ret = snprintf(tmp, ARRAY_SIZE(tmp), "%.*s\n",
> +					       (int)sizeof(*formats),
> +					       (char *)&formats[i]);
> +			if (ret < 0)
> +				return ret;
> +			/*
> +			 * Limitation of ConfigFS attributes, an attribute can't be bigger
> +			 * than PAGE_SIZE. This will crop the result if this plane support
> +			 * more than ≈1000 formats.

Every format takes 5 chars, so about 800 formats, no?

> +			 */
> +			if (ret + strlen(page) > PAGE_SIZE - 1)
> +				return -ENOMEM;
> +			strncat(page, tmp, ARRAY_SIZE(tmp));
> +		}
> +	}
> +
> +	return strlen(page);
> +}
> +
> +/**
> + * parse_next_format() - Parse the next format in page, skipping all non fourcc-related characters
> + * @page: page to search into
> + * @page_end: last character of the page
> + * @out: Output pointer, will point inside page
> + *
> + * Returns: size of the matched format, @out will point to the + or -
> + */
> +VISIBLE_IF_KUNIT
> +int vkms_configfs_parse_next_format(const char *page, const char *page_end, char **out)
> +{
> +	int count = page - page_end;
> +	char *tmp_plus = strnchr(page, count, '+');
> +	char *tmp_minus = strnchr(page, count, '-');
> +
> +	if (!tmp_plus && !tmp_minus)
> +		return 0;
> +	if (!tmp_plus)
> +		*out = tmp_minus;
> +	else if (!tmp_minus)
> +		*out = tmp_plus;
> +	else
> +		*out = min(tmp_plus, tmp_minus);
> +
> +	char *end = *out + 1;
> +
> +	while (end < page_end) {
> +		if (!isalnum(*end) && *end != '*')
> +			break;
> +		end++;
> +	}

I think this while loop will capture a string like "A*42", which is wrong.

Maybe you could change this function to be both stricter and simpler by not
trying to accept leading spaces, for example.

> +static ssize_t plane_supported_formats_store(struct config_item *item,
> +					     const char *page, size_t count)
> +{
> +	struct vkms_configfs_plane *plane;
> +
> +	plane = plane_item_to_vkms_configfs_plane(item);
> +	int ret = 0;
> +	const char *end_page = page + count;
> +
> +	scoped_guard(mutex, &plane->dev->lock) {
> +		while (1) {
> +			char *tmp;
> +			char fmt[4] = {' ', ' ', ' ', ' '};
> +			int len = vkms_configfs_parse_next_format(page, end_page, &tmp);
> +
> +			// No fourcc code found
> +			if (len <= 1 || len > 5)
> +				break;
> +
> +			page = tmp + len;
> +			memcpy(fmt, &tmp[1], min(len - 1, 4));
> +			if (tmp[0] == '+') {
> +				if (fmt[0] == '*') {
> +					ret = vkms_config_plane_add_all_formats(plane->config);
> +					if (ret)
> +						return ret;
> +				} else {
> +					ret = vkms_config_plane_add_format(plane->config,
> +									   *(int *)fmt);
> +					if (ret)
> +						return ret;
> +				}

Minor code simplification:

				if (fmt[0] == '*')
					ret = vkms_config_plane_add_all_formats(plane->config);
				else
					ret = vkms_config_plane_add_format(plane->config,
									   *(int *)fmt);
				if (ret)
					return ret;

Or, if you like the ternary operator:


				ret = (fmt[0] == '*') ?
					vkms_config_plane_add_all_formats(plane->config):
					vkms_config_plane_add_format(plane->config, *(int *)fmt);
				if (ret)
					return ret;

I'm sorry some of these comments could have been written asof v2, but this
patch is really intricate and they came to mind only while re-thinking
about the code.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists