On 12/21/23 13:00, Julia Zhang wrote:
> From: Daniel Stone <daniels@xxxxxxxxxxxxx>
>
> Add a new ioctl to allow the guest VM to discover how the guest
> actually allocated the underlying buffer, which allows buffers to
> be used for GL<->Vulkan interop and through standard window systems.
> It's also a step towards properly supporting modifiers in the guest.
>
> Signed-off-by: Daniel Stone <daniels@xxxxxxxxxxxxx>
> Co-developed-by: Julia Zhang <julia.zhang@xxxxxxx> # support query
> stride before it's created
> Signed-off-by: Julia Zhang <julia.zhang@xxxxxxx>
> ---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 1 +
> drivers/gpu/drm/virtio/virtgpu_drv.h | 22 ++++++++-
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 66 ++++++++++++++++++++++++++
> drivers/gpu/drm/virtio/virtgpu_kms.c | 8 +++-
> drivers/gpu/drm/virtio/virtgpu_vq.c | 63 ++++++++++++++++++++++++
> include/uapi/drm/virtgpu_drm.h | 21 ++++++++
> include/uapi/linux/virtio_gpu.h | 30 ++++++++++++
> 7 files changed, 208 insertions(+), 3 deletions(-)
...
> +static int virtio_gpu_resource_query_layout_ioctl(struct drm_device *dev,
> + void *data,
> + struct drm_file *file)
> +{
> + struct drm_virtgpu_resource_query_layout *args = data;
> + struct virtio_gpu_device *vgdev = dev->dev_private;
> + struct drm_gem_object *obj = NULL;
> + struct virtio_gpu_object *bo = NULL;
> + struct virtio_gpu_query_info bo_info = {0};
> + int ret = 0;
> + int i;
> +
> + if (!vgdev->has_resource_query_layout) {
> + DRM_ERROR("failing: no RQL on host\n");
Please remove this message
> + return -EINVAL;
return -ENOSYS
> + }
> +
> + if (args->handle > 0) {
> + obj = drm_gem_object_lookup(file, args->handle);
> + if (obj == NULL) {
> + DRM_ERROR("invalid handle 0x%x\n", args->handle);
> + return -ENOENT;
> + }
> + bo = gem_to_virtio_gpu_obj(obj);
> + }
> +
> + ret = virtio_gpu_cmd_get_resource_layout(vgdev, &bo_info, args->width,
> + args->height, args->format,
> + args->bind, bo ? bo->hw_res_handle : 0);
What this special hw_res_handle=0 is doing? Why is it needed?
> + if (ret)
> + goto out;
> +
> + ret = wait_event_timeout(vgdev->resp_wq,
> + atomic_read(&bo_info.is_valid),
> + 5 * HZ);
> + if (!ret)
> + goto out;
> +
> +valid:
> + smp_rmb();
> + WARN_ON(atomic_read(&bo_info.is_valid));
Please remove this WARN_ON and fix the kernelbot report
> + args->num_planes = bo_info.num_planes;
> + args->modifier = bo_info.modifier;
> + for (i = 0; i < args->num_planes; i++) {
> + args->planes[i].offset = bo_info.planes[i].offset;
> + args->planes[i].stride = bo_info.planes[i].stride;
> + }
> + for (; i < VIRTIO_GPU_MAX_RESOURCE_PLANES; i++) {
> + args->planes[i].offset = 0;
> + args->planes[i].stride = 0;
> + }
> + ret = 0;
ret is already 0 here
> +out:
> + if (obj)
> + drm_gem_object_put(obj);
> + return ret;
> +}
...
> diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
> index f556fde07b76..547575232376 100644
> --- a/include/uapi/linux/virtio_gpu.h
> +++ b/include/uapi/linux/virtio_gpu.h
> @@ -65,6 +65,11 @@
> */
> #define VIRTIO_GPU_F_CONTEXT_INIT 4
>
> +/*
> + * VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT
> + */
> +#define VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT 5
> +
> enum virtio_gpu_ctrl_type {
> VIRTIO_GPU_UNDEFINED = 0,
>
> @@ -95,6 +100,7 @@ enum virtio_gpu_ctrl_type {
> VIRTIO_GPU_CMD_SUBMIT_3D,
> VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB,
> VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB,
> + VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT,
>
> /* cursor commands */
> VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
> @@ -108,6 +114,7 @@ enum virtio_gpu_ctrl_type {
> VIRTIO_GPU_RESP_OK_EDID,
> VIRTIO_GPU_RESP_OK_RESOURCE_UUID,
> VIRTIO_GPU_RESP_OK_MAP_INFO,
> + VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT,
>
> /* error responses */
> VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
> @@ -453,4 +460,27 @@ struct virtio_gpu_resource_unmap_blob {
> __le32 padding;
> };
>
> +/* VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT */
> +struct virtio_gpu_resource_query_layout {
> + struct virtio_gpu_ctrl_hdr hdr;
> + __le32 resource_id;
> + __le32 width;
> + __le32 height;
> + __le32 format;
> + __le32 bind;
64b pad missing
> +};
> +
> +
> +/* VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT */
> +#define VIRTIO_GPU_RES_MAX_PLANES 4
> +struct virtio_gpu_resp_resource_layout {
> + struct virtio_gpu_ctrl_hdr hdr;
> + __le64 modifier;
> + __le32 num_planes;
> + struct virtio_gpu_resource_plane {
> + __le64 offset;
> + __le32 stride;
> + } planes[VIRTIO_GPU_RES_MAX_PLANES];
> +};
Virto-spec changes should have a corresponding doc update in [1].
[1]
https://github.com/oasis-tcs/virtio-spec/blob/master/device-types/gpu/description.tex
--
Best regards,
Dmitry
