Re: [PATCH v5 25/38] drm/vkms: Introduce config for plane zpos property

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The return values of `drm_plane_create_zpos_property()` and `drm_plane_create_zpos_immutable_property()` are ignored.
- [Low] Missing newline character `\n` at the end of the `drm_info` format string.
- [Low] Using signed format specifiers (`%d`) for unsigned integer variables in debugfs output.
--

commit 332270a595e6e9b34ad7075f21dc26e1c7579078
Author: Louis Chauvet <louis.chauvet@bootlin.com>

drm/vkms: Introduce config for plane zpos property

VKMS can render plane in any order. Introduce the appropriate
configuration.

> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index c7de3e3073674..5bc8248f99353 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
[ ... ]
> @@ -429,6 +462,19 @@ bool vkms_config_is_valid(const struct vkms_config *config)
>  				 "Both supported color encodings and color ranges must be set, or none\n");
>  			return false;
>  		}
> +
> +		if (!vkms_config_valid_plane_zpos(config, plane_cfg))
> +			return false;
> +
> +		if (vkms_config_plane_get_zpos_enabled(plane_cfg))
> +			has_zpos_enabled = true;
> +		else
> +			has_zpos_disabled = true;
> +	}
> +
> +	if (has_zpos_enabled && has_zpos_disabled) {
> +		drm_info(dev, "In the same device, all planes must have zpos enabled or none of them");

[Severity: Low]
Is this missing a newline character? Log messages without a trailing \n
can cause formatting issues by buffering and attaching to the next message.

> +		return false;
>  	}
>  
>  	if (!valid_plane_number(config))
[ ... ]
> @@ -527,6 +573,16 @@ static int vkms_config_show(struct seq_file *m, void *data)
>  		show_formats(m, vkms_config_plane_get_supported_formats(plane_cfg),
>  			     vkms_config_plane_get_supported_formats_count(plane_cfg));
>  		seq_puts(m, "\n");
> +		seq_printf(m, "\tzpos_enabled=%s\n",
> +			   str_true_false(vkms_config_plane_get_zpos_enabled(plane_cfg)));
> +		seq_printf(m, "\tzpos_mutable=%s\n",
> +			   str_true_false(vkms_config_plane_get_zpos_mutable(plane_cfg)));
> +		seq_printf(m, "\tzpos_min=%d\n",
> +			   vkms_config_plane_get_zpos_min(plane_cfg));
> +		seq_printf(m, "\tzpos_initial=%d\n",
> +			   vkms_config_plane_get_zpos_initial(plane_cfg));
> +		seq_printf(m, "\tzpos_max=%d\n",
> +			   vkms_config_plane_get_zpos_max(plane_cfg));

[Severity: Low]
Should these use the %u format specifier instead of %d? The getter 
functions (like vkms_config_plane_get_zpos_min) return unsigned int, 
so %d might display large values as negative numbers.

>  	}
>  
>  	vkms_config_for_each_crtc(vkmsdev->config, crtc_cfg) {
[ ... ]
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 6e767cdf81942..d600a6167042b 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
[ ... ]
> @@ -214,6 +214,17 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  
>  	if (vkms_config_plane_get_default_pipeline(plane_cfg))
>  		vkms_initialize_colorops(&plane->base);
> +	if (vkms_config_plane_get_zpos_enabled(plane_cfg)) {
> +		if (vkms_config_plane_get_zpos_mutable(plane_cfg)) {
> +			drm_plane_create_zpos_property(&plane->base,
> +						       vkms_config_plane_get_zpos_initial(plane_cfg),
> +						       vkms_config_plane_get_zpos_min(plane_cfg),
> +						       vkms_config_plane_get_zpos_max(plane_cfg));
> +		} else {
> +			drm_plane_create_zpos_immutable_property(&plane->base,
> +								 vkms_config_plane_get_zpos_initial(plane_cfg));
> +		}
> +	}

[Severity: Medium]
Is it safe to ignore the return values of drm_plane_create_zpos_property()
and drm_plane_create_zpos_immutable_property() here? 

These functions can return -ENOMEM on allocation failure. If that happens, 
the plane is partially initialized without the expected zpos property, 
which might confuse userspace applications. Should we propagate the error 
up through vkms_plane_init()?

>  
>  	return plane;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627-vkms-all-config-v5-0-854aa0840926@bootlin.com?part=25