| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <DF2CXBF8ZCSQ.28C9DWKRIFW8Y@bootlin.com>
Date: Fri, 19 Dec 2025 18:08:30 +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 RESEND v2 19/32] drm/vkms: Introduce config for plane
zpos property
On Wed Oct 29, 2025 at 3:36 PM CET, Louis Chauvet wrote:
> VKMS can render plane in any order. Introduce the appropriate
> configuration.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> --- a/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
> @@ -606,6 +609,94 @@ static void vkms_config_test_valid_plane_color_range(struct kunit *test)
> vkms_config_destroy(config);
> }
>
> +static void vkms_config_test_valid_plane_zpos(struct kunit *test)
> +{
> + struct vkms_config *config;
> + struct vkms_config_plane *plane_cfg;
> +
> + config = vkms_config_default_create(false, false, false);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, config);
> +
> + plane_cfg = get_first_plane(config);
> +
> + /* Valid, all color range supported */
> + plane_cfg = get_first_plane(config);
These 2 lines should be removed? Invalid comment and duplicated line,
apparently.
> + /* Valid, zpos disabled */
> + vkms_config_plane_set_zpos_enabled(plane_cfg, false);
> + vkms_config_plane_set_zpos_mutable(plane_cfg, false);
> + vkms_config_plane_set_zpos_initial(plane_cfg, 0);
> + vkms_config_plane_set_zpos_min(plane_cfg, 0);
> + vkms_config_plane_set_zpos_max(plane_cfg, 0);
> + KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> +
> + /* Valid, zpos disabled, min/max are ignored */
> + vkms_config_plane_set_zpos_enabled(plane_cfg, false);
> + vkms_config_plane_set_zpos_mutable(plane_cfg, false);
> + vkms_config_plane_set_zpos_initial(plane_cfg, 8);
> + vkms_config_plane_set_zpos_min(plane_cfg, 3);
> + vkms_config_plane_set_zpos_max(plane_cfg, 2);
> + KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
> +
> + /* Valid, zpos enabled but initial value is out of range */
> + vkms_config_plane_set_zpos_enabled(plane_cfg, true);
> + vkms_config_plane_set_zpos_mutable(plane_cfg, false);
> + vkms_config_plane_set_zpos_initial(plane_cfg, 1);
> + vkms_config_plane_set_zpos_min(plane_cfg, 0);
> + vkms_config_plane_set_zpos_max(plane_cfg, 0);
> + KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
vkms_config_valid_plane_zpos() returns OK if mutable is false. So the test
is correct but the comment is misleading. It should read:
/* Valid, zpos enabled but mutable disabled */
> + /* Valid, zpos enabled with valid initial value */
> + vkms_config_plane_set_zpos_enabled(plane_cfg, true);
> + vkms_config_plane_set_zpos_mutable(plane_cfg, false);
> + vkms_config_plane_set_zpos_initial(plane_cfg, 0);
> + vkms_config_plane_set_zpos_min(plane_cfg, 0);
> + vkms_config_plane_set_zpos_max(plane_cfg, 0);
> + KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
Here the comment is misleading as well, it should still be:
/* Valid, zpos enabled but mutable disabled */
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -249,6 +252,37 @@ bool vkms_config_valid_plane_color_range(const struct vkms_config *config,
> }
> EXPORT_SYMBOL_IF_KUNIT(vkms_config_valid_plane_color_range);
>
> +VISIBLE_IF_KUNIT
> +bool vkms_config_valid_plane_zpos(const struct vkms_config *config,
> + const struct vkms_config_plane *plane_cfg)
> +{
> + struct drm_device *dev = config->dev ? &config->dev->drm : NULL;
> +
> + if (!vkms_config_plane_get_zpos_enabled(plane_cfg) ||
> + !vkms_config_plane_get_zpos_mutable(plane_cfg))
> + return true;
> +
> + if (vkms_config_plane_get_zpos_initial(plane_cfg) >
> + vkms_config_plane_get_zpos_max(plane_cfg)) {
> + drm_info(dev, "Configured initial zpos value bigger than zpos max\n");
> + return false;
> + }
> +
> + if (vkms_config_plane_get_zpos_max(plane_cfg) <
> + vkms_config_plane_get_zpos_min(plane_cfg)) {
> + drm_info(dev, "Configured zpos max value smaller than zpos min\n");
> + return false;
> + }
> +
> + if (vkms_config_plane_get_zpos_initial(plane_cfg) <
> + vkms_config_plane_get_zpos_min(plane_cfg)) {
> + drm_info(dev, "Configured initial zpos value smaller than zpos min\n");
> + return false;
> + }
Maybe merge the if(initial < min) and the if(initial > max) into a single
if(), to avoid unnecessarily detailed error reporting. Not a strong request
however, up to you.
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -51,6 +51,11 @@ struct vkms_config {
> * @supported_color_ranges: Color range that this plane will support
> * @supported_formats: List of supported formats
> * @supported_formats_count: Length of @supported_formats
> + * @zpos_enabled: Enable or disable the zpos property
> + * @zpos_mutable: Make the zpos property mutable or not (ignored if @zpos_enabled is false)
> + * @zpos_initial: Initial value for zpos property (ignored if @zpos_enabled is false)
> + * @zpos_min: Minimal value for zpos property (ignored if @zpos_enabled is false)
> + * @zpos_max: Maximal value for zpos property (ignored if @zpos_enabled is false)
> */
> struct vkms_config_plane {
> struct list_head link;
[...]
> +/**
> + * vkms_config_plane_set_zpos_min() - Set the minimum zpos value
> + * @plane_cfg: Plane configuration to modify
> + * @zpos_min: Minimum zpos value
> + */
> +static inline
> +void vkms_config_plane_set_zpos_min(struct vkms_config_plane *plane_cfg,
> + unsigned int zpos_min)
> +{
> + plane_cfg->zpos_min = zpos_min;
> +}
> +
> +/**
> + * vkms_config_plane_set_zpos_max() - Set the maximum zpos value
> + * @plane_cfg: Plane configuration to modify
> + * @zpos_max: Maximum zpos value
> + *
> + * Sets the maximum allowed value for the zpos property. This setting is
> + * ignored if zpos is disabled.
These two lines are not present for the other functions. I suggest removing
them here, the kdoc in struct vkms_config is already saying so.
> +/**
> + * vkms_config_plane_get_zpos_mutable() - Check if zpos property is mutable
> + * @plane_cfg: Plane configuration to check
> + *
> + * Returns:
> + * True if the zpos property is mutable for this plane, false otherwise.
> + * Returns false if zpos is disabled.
This last line is correct? There is no check in the code. I think it's OK
to leave the code as is and remove this line.
> + */
> +static inline
> +bool vkms_config_plane_get_zpos_mutable(const struct vkms_config_plane *plane_cfg)
> +{
> + return plane_cfg->zpos_mutable;
> +}
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists