Re: [PATCH] drm/virtio: add overlay plane format support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] drm/virtio: missing format translation for new overlay pixel formats
- [Medium] drm/virtio: overlay plane update overwrites scanout instead of compositing
- [Low] drm/virtio: overlay plane initialization is dead code
--

commit 6902c3c1a9efc5fd5b84d6539b7a7f5b7707c29f
Author: Sophia Gong <lili.gong@amd.com>

drm/virtio: add overlay plane format support

Advertise additional pixel formats for virtio-gpu overlay planes so a
userspace compositor can use KMS overlay composition. Handle
DRM_PLANE_TYPE_OVERLAY in virtio_gpu_plane_init() and reuse the primary
plane update path for scanout.

> diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
> index 10718bc736939..4277676aee4e1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_plane.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
> @@ -44,6 +44,21 @@ static const uint32_t virtio_gpu_cursor_formats[] = {
>  	DRM_FORMAT_HOST_ARGB8888,
>  };
>  
> +static const uint32_t virtio_gpu_overlay_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_BGRX1010102,
> +	DRM_FORMAT_NV12,
> +	DRM_FORMAT_P010,
> +};
> +
>  uint32_t virtio_gpu_translate_format(uint32_t drm_fourcc)

[Severity: Medium]
Does the addition of virtio_gpu_overlay_formats also require adding these
formats to the switch statement in virtio_gpu_translate_format()?

If an overlay plane is updated with one of the new formats (like
DRM_FORMAT_NV12), it looks like it will hit the default case in
virtio_gpu_translate_format().

This would trigger a WARN_ON and cause virtio_gpu_cmd_set_scanout_blob()
to send an invalid format (0) in the command packet to the host.

[ ... ]

> @@ -586,6 +601,13 @@ static const struct drm_plane_helper_funcs virtio_gpu_cursor_helper_funcs = {
>  	.atomic_update		= virtio_gpu_cursor_plane_update,
>  };
>  
> +static const struct drm_plane_helper_funcs virtio_gpu_overlay_helper_funcs = {
> +	.prepare_fb		= virtio_gpu_plane_prepare_fb,
> +	.cleanup_fb		= virtio_gpu_plane_cleanup_fb,
> +	.atomic_check		= virtio_gpu_plane_atomic_check,
> +	.atomic_update		= virtio_gpu_primary_plane_update,
> +};

[Severity: Medium]
Will reusing virtio_gpu_primary_plane_update() for the overlay plane break
display composition?

The primary plane update path assumes exclusive ownership of the display's
output->index.

If both a primary and an overlay plane update on the same CRTC, won't they
both send VIRTIO_GPU_CMD_SET_SCANOUT to the same scanout ID, replacing each
other's content rather than compositing it?

[ ... ]

> @@ -601,6 +623,10 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
>  		formats = virtio_gpu_cursor_formats;
>  		nformats = ARRAY_SIZE(virtio_gpu_cursor_formats);
>  		funcs = &virtio_gpu_cursor_helper_funcs;
> +	} else if (type == DRM_PLANE_TYPE_OVERLAY) {
> +		formats = virtio_gpu_overlay_formats;
> +		nformats = ARRAY_SIZE(virtio_gpu_overlay_formats);
> +		funcs = &virtio_gpu_overlay_helper_funcs;

[Severity: Low]
Is this overlay plane initialization currently reachable?

It appears this branch might be dead code, as there doesn't seem to be any
caller in the driver that requests the creation of an overlay plane by
passing DRM_PLANE_TYPE_OVERLAY to virtio_gpu_plane_init().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924142242.676-1-baorliu@amd.com?part=1