Re: [PATCH RESEND v2 19/32] drm/vkms: Introduce config for plane zpos property

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

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@bootlin.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