Re: [PATCH RESEND v2 17/32] drm/vkms: Introduce configfs for plane format

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

On Wed Oct 29, 2025 at 3:36 PM 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@bootlin.com>

> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst

> @@ -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_range
> +- supported_formats: List of supported formats for this plane. To add a new item in the
> +  list, write it using a plus its fourcc code: +XR24
                                ^ and

> +++ 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
> + * @data: Contains the string to parse
> + * @data_len: data 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 *data;

This is a string, why not calling it 'string', or 'str'?

> +struct vkms_configfs_parse_format_case vkms_configfs_parse_format_test_cases[] = {
> +	{
> +		.data = "+RG24",
> +		.data_len = 6,
> +		.expected_len = 5,
> +		.expected_offset = 0,
> +	}, {
> +		.data = "-RG24",
> +		.data_len = 6,
> +		.expected_len = 5,
> +		.expected_offset = 0
> +	}, {
> +		.data = "  -RG24",
> +		.data_len = 8,
> +		.expected_len = 5,
> +		.expected_offset = 2
> +	}, {
> +		.data = "+*",
> +		.data_len = 3,
> +		.expected_len = 2,
> +		.expected_offset = 0
> +	}, {
> +		.data = "-RG24+RG24",
> +		.data_len = 11,
> +		.expected_len = 5,
> +		.expected_offset = 0
> +	}, {
> +		.data = "-R1+RG24",
> +		.data_len = 9,
> +		.expected_len = 3,
> +		.expected_offset = 0
> +	}, {
> +		.data = "\n-R1",
> +		.data_len = 5,
> +		.expected_len = 3,
> +		.expected_offset = 1
> +	}, {
> +		.data = "-R1111",
> +		.data_len = 3,

The string is longer than 3 chars, is this intended?

> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c

> +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)
> +				break;
> +
> +			page = tmp + len;
> +			memcpy(tmp, &fmt[1], min(len - 1, 4));
Should this be instead:        fmt   tmp
?

Also I think it would be good to reject strings longer than 4 chars (len >
5), because they cannot br fourccs.

Otherwise looks good.

BTW, I feel your pain in implementing the parsing!

Luca

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