Re: [PATCH i-g-t 09/39] lib/vkms: Test plane invalid values

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 18/02/2025 à 17:49, José Expósito a écrit :
> For a VKMS plane, it is only possible to set invalid values in the plane
> type.
> 
> Test that setting wrong values fails and that the plane is not
> accidentally changed.
> 
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   lib/igt_vkms.c             | 25 +++++++++++++++++++
>   lib/igt_vkms.h             |  1 +
>   tests/vkms/vkms_configfs.c | 51 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 77 insertions(+)
> 
> diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> index 5cfe5123c..ca6f37975 100644
> --- a/lib/igt_vkms.c
> +++ b/lib/igt_vkms.c
> @@ -340,3 +340,28 @@ int igt_vkms_plane_get_type(igt_vkms_t *dev, const char *name)
>   
>   	return read_int(path);
>   }
> +
> +/**
> + * igt_vkms_plane_set_type:
> + * @dev: Device the plane belongs to
> + * @name: Plane name
> + * @type: DRM_PLANE_TYPE_OVERLAY, DRM_PLANE_TYPE_PRIMARY or
> + * DRM_PLANE_TYPE_CURSOR
> + *
> + * Set a new type for the plane
> + */
> +void igt_vkms_plane_set_type(igt_vkms_t *dev, const char *name, int type)
> +{
> +	char path[PATH_MAX];
> +
> +	if (type != DRM_PLANE_TYPE_OVERLAY &&
> +	    type != DRM_PLANE_TYPE_PRIMARY &&
> +	    type != DRM_PLANE_TYPE_CURSOR)
> +		igt_assert(!"Cannot be reached: Unknown plane type");
> +
> +	snprintf(path, sizeof(path), "%s/%s/%s/%s", dev->path,
> +		 get_pipeline_item_dir_name(VKMS_PIPELINE_ITEM_PLANE), name,
> +		 VKMS_FILE_PLANE_TYPE);
> +
> +	write_int(path, type);

It can fail silently, see my comment on 05/39.

> +}
> diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> index dd06d8f5b..283be5699 100644
> --- a/lib/igt_vkms.h
> +++ b/lib/igt_vkms.h
> @@ -31,5 +31,6 @@ void igt_vkms_device_set_enabled(igt_vkms_t *dev, bool enabled);
>   
>   void igt_vkms_device_add_plane(igt_vkms_t *dev, const char *name);
>   int igt_vkms_plane_get_type(igt_vkms_t *dev, const char *name);
> +void igt_vkms_plane_set_type(igt_vkms_t *dev, const char *name, int type);
>   
>   #endif /* __IGT_VKMS_H__ */
> diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> index a3fa60762..d607b1a5a 100644
> --- a/tests/vkms/vkms_configfs.c
> +++ b/tests/vkms/vkms_configfs.c
> @@ -222,6 +222,56 @@ static void test_plane_default_values(void)
>   	igt_vkms_device_destroy(dev);
>   }
>   
> +/**
> + * SUBTEST: plane-wrong-values
> + * Description: Check that setting unexpected values doesn't work.
> + */
> +
> +static void test_plane_wrong_values(void)
> +{
> +	struct invalid_value invalid_type_values[] = {
> +		{ "", 0 },
> +		{ "\0", 1 },
> +		{ "-1", 2 },
> +		{ "4", 1 },
> +		{ "primary", 8 },
> +		{ "overlay", 8 },
> +	};
> +	igt_vkms_t *dev;
> +	char path[PATH_MAX];
> +	int fd;
> +	int ret;
> +
> +	/* Create a device with a primary plane */
> +	dev = igt_vkms_device_create(__func__);
> +	igt_assert(dev);
> +
> +	igt_vkms_device_add_plane(dev, "plane0");
> +	igt_vkms_plane_set_type(dev, "plane0", DRM_PLANE_TYPE_PRIMARY);
> +	igt_assert_eq(igt_vkms_plane_get_type(dev, "plane0"),
> +		      DRM_PLANE_TYPE_PRIMARY);
> +
> +	snprintf(path, sizeof(path), "%s/planes/plane0/type", dev->path);

I am not a big fan of duplicating the path generation here and in 
igt_vkms_plane_set_type. But OTOH, I don't see how to do better, so:

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

> +
> +	/* Test invalid values for "type" */
> +	for (int i = 0; i < ARRAY_SIZE(invalid_type_values); i++) {
> +		struct invalid_value v = invalid_type_values[i];
> +
> +		fd = open(path, O_WRONLY);
> +		igt_assert_f(fd >= 0, "Error opening '%s'\n", path);
> +
> +		ret = write(fd, v.value, v.size);
> +		igt_assert(ret <= 0);

As in a previous patch, can you add some information about the fail 
(value and path).

> +
> +		close(fd);
> +	}
> +
> +	igt_assert_eq(igt_vkms_plane_get_type(dev, "plane0"),
> +		      DRM_PLANE_TYPE_PRIMARY);
> +
> +	igt_vkms_device_destroy(dev);
> +}
> +
>   igt_main
>   {
>   	struct {
> @@ -233,6 +283,7 @@ igt_main
>   		{ "device-wrong-values", test_device_wrong_values },
>   		{ "plane-default-files", test_plane_default_files },
>   		{ "plane-default-values", test_plane_default_values },
> +		{ "plane-wrong-values", test_plane_wrong_values },
>   	};
>   
>   	igt_fixture {

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com