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 <[email protected]> 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/[email protected]?part=1
