Hi all,
Sorry to late reply. This is v2 of the implementation of
resource_query_layout. This adds a new ioctl to let guest query information
of host resource, which is originally from Daniel Stone. We add some
changes to support query the correct stride of host resource before it's
created, which is to support to blit data from dGPU to virtio iGPU for dGPU
prime feature.
Changes from v1 to v2:
-Squash two patches to a single patch.
-A small modification of VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT
Below is description of v1:
This add implementation of resource_query_layout to get the information of
how the host has actually allocated the buffer. This function is now used
to query the stride for guest linear resource for dGPU prime on guest VMs.
v1 of kernel side:
https:
//lore.kernel.org/xen-devel/[email protected]/T/#t
v1 of qemu side:
https:
//lore.kernel.org/qemu-devel/[email protected]/T/#t
Daniel Stone (1):
drm/virtio: Implement RESOURCE_GET_LAYOUT ioctl
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(-)
--
2.34.1
From: Daniel Stone <[email protected]>
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 <[email protected]>
Co-developed-by: Julia Zhang <[email protected]> # support query
stride before it's created
Signed-off-by: Julia Zhang <[email protected]>
---
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(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 4334c7608408..98061b714b98 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -148,6 +148,7 @@ static unsigned int features[] = {
VIRTIO_GPU_F_RESOURCE_UUID,
VIRTIO_GPU_F_RESOURCE_BLOB,
VIRTIO_GPU_F_CONTEXT_INIT,
+ VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT,
};
static struct virtio_driver virtio_gpu_driver = {
.feature_table = features,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 96365a772f77..bb5edcfeda54 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -214,6 +214,16 @@ struct virtio_gpu_drv_cap_cache {
atomic_t is_valid;
};
+struct virtio_gpu_query_info {
+ uint32_t num_planes;
+ uint64_t modifier;
+ struct {
+ uint64_t offset;
+ uint32_t stride;
+ } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
+ atomic_t is_valid;
+};
+
struct virtio_gpu_device {
struct drm_device *ddev;
@@ -246,6 +256,7 @@ struct virtio_gpu_device {
bool has_resource_blob;
bool has_host_visible;
bool has_context_init;
+ bool has_resource_query_layout;
struct virtio_shm_region host_visible_region;
struct drm_mm host_visible_mm;
@@ -277,7 +288,7 @@ struct virtio_gpu_fpriv {
};
/* virtgpu_ioctl.c */
-#define DRM_VIRTIO_NUM_IOCTLS 12
+#define DRM_VIRTIO_NUM_IOCTLS 13
extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
@@ -420,6 +431,15 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
uint32_t width, uint32_t height,
uint32_t x, uint32_t y);
+int
+virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_query_info *bo_info,
+ uint32_t width,
+ uint32_t height,
+ uint32_t format,
+ uint32_t bind,
+ uint32_t hw_res_handle);
+
/* virtgpu_display.c */
int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b24b11f25197..216c04314177 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -107,6 +107,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
value = vgdev->capset_id_mask;
break;
+ case VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT:
+ value = vgdev->has_resource_query_layout ? 1 : 0;
+ break;
default:
return -EINVAL;
}
@@ -668,6 +671,65 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
return ret;
}
+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");
+ return -EINVAL;
+ }
+
+ 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);
+ 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));
+ 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;
+
+out:
+ if (obj)
+ drm_gem_object_put(obj);
+ return ret;
+}
+
struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
DRM_RENDER_ALLOW),
@@ -707,4 +769,8 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
DRM_IOCTL_DEF_DRV(VIRTGPU_CONTEXT_INIT, virtio_gpu_context_init_ioctl,
DRM_RENDER_ALLOW),
+
+ DRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_QUERY_LAYOUT,
+ virtio_gpu_resource_query_layout_ioctl,
+ DRM_RENDER_ALLOW),
};
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 5a3b5aaed1f3..4f34f4145910 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -175,6 +175,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_BLOB)) {
vgdev->has_resource_blob = true;
}
+ if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT)) {
+ vgdev->has_resource_query_layout = true;
+ }
if (virtio_get_shm_region(vgdev->vdev, &vgdev->host_visible_region,
VIRTIO_GPU_SHM_ID_HOST_VISIBLE)) {
if (!devm_request_mem_region(&vgdev->vdev->dev,
@@ -204,8 +207,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
vgdev->has_resource_blob ? '+' : '-',
vgdev->has_host_visible ? '+' : '-');
- DRM_INFO("features: %ccontext_init\n",
- vgdev->has_context_init ? '+' : '-');
+ DRM_INFO("features: %ccontext_init %cresource_query_layout\n",
+ vgdev->has_context_init ? '+' : '-',
+ vgdev->has_resource_query_layout ? '+' : '-');
ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
if (ret) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index b1a00c0c25a7..26998a3ac4c2 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -1302,3 +1302,66 @@ void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
}
+
+static void virtio_gpu_cmd_get_resource_layout_cb(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_vbuffer *vbuf)
+{
+ struct virtio_gpu_resp_resource_layout *resp =
+ (struct virtio_gpu_resp_resource_layout *)vbuf->resp_buf;
+ struct virtio_gpu_query_info *bo_info = vbuf->resp_cb_data;
+ int i;
+
+ vbuf->resp_cb_data = NULL;
+
+ if (resp->hdr.type != VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT) {
+ atomic_set(&bo_info->is_valid, 0);
+ goto out;
+ }
+
+ bo_info->modifier = le64_to_cpu(resp->modifier);
+ bo_info->num_planes = le32_to_cpu(resp->num_planes);
+ for (i = 0; i < bo_info->num_planes; i++) {
+ bo_info->planes[i].stride = le32_to_cpu(resp->planes[i].stride);
+ bo_info->planes[i].offset = le32_to_cpu(resp->planes[i].offset);
+ }
+ smp_wmb();
+ atomic_set(&bo_info->is_valid, 1);
+
+out:
+ wake_up_all(&vgdev->resp_wq);
+}
+
+int virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
+ struct virtio_gpu_query_info *bo_info,
+ uint32_t width,
+ uint32_t height,
+ uint32_t format,
+ uint32_t bind,
+ uint32_t hw_res_handle)
+{
+ struct virtio_gpu_resource_query_layout *cmd_p;
+ struct virtio_gpu_vbuffer *vbuf;
+ void *resp_buf;
+
+ resp_buf = kzalloc(sizeof(struct virtio_gpu_resp_resource_layout),
+ GFP_KERNEL);
+ if (!resp_buf)
+ return -ENOMEM;
+
+ cmd_p = virtio_gpu_alloc_cmd_resp
+ (vgdev, &virtio_gpu_cmd_get_resource_layout_cb, &vbuf,
+ sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_resource_layout),
+ resp_buf);
+ memset(cmd_p, 0, sizeof(*cmd_p));
+
+ cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT);
+ cmd_p->resource_id = cpu_to_le32(hw_res_handle);
+ cmd_p->width = cpu_to_le32(width);
+ cmd_p->height = cpu_to_le32(height);
+ cmd_p->format = cpu_to_le32(format);
+ cmd_p->bind = cpu_to_le32(bind);
+ vbuf->resp_cb_data = bo_info;
+
+ virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+ return 0;
+}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index b1d0e56565bc..66f7c0fa1d4d 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -48,6 +48,7 @@ extern "C" {
#define DRM_VIRTGPU_GET_CAPS 0x09
#define DRM_VIRTGPU_RESOURCE_CREATE_BLOB 0x0a
#define DRM_VIRTGPU_CONTEXT_INIT 0x0b
+#define DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT 0x0c
#define VIRTGPU_EXECBUF_FENCE_FD_IN 0x01
#define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
@@ -97,6 +98,7 @@ struct drm_virtgpu_execbuffer {
#define VIRTGPU_PARAM_CROSS_DEVICE 5 /* Cross virtio-device resource sharing */
#define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
#define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
+#define VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT 8 /* DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT (also needs cap) */
struct drm_virtgpu_getparam {
__u64 param;
@@ -211,6 +213,21 @@ struct drm_virtgpu_context_init {
__u64 ctx_set_params;
};
+#define VIRTIO_GPU_MAX_RESOURCE_PLANES 4
+struct drm_virtgpu_resource_query_layout {
+ __u32 handle;
+ __u32 width;
+ __u32 height;
+ __u32 format;
+ __u32 bind;
+ __u32 num_planes;
+ __u64 modifier;
+ struct {
+ __u64 offset;
+ __u32 stride;
+ } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
+};
+
/*
* Event code that's given when VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is in
* effect. The event size is sizeof(drm_event), since there is no additional
@@ -261,6 +278,10 @@ struct drm_virtgpu_context_init {
DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_CONTEXT_INIT, \
struct drm_virtgpu_context_init)
+#define DRM_IOCTL_VIRTGPU_RESOURCE_QUERY_LAYOUT \
+ DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT, \
+ struct drm_virtgpu_resource_query_layout)
+
#if defined(__cplusplus)
}
#endif
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;
+};
+
+
+/* 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];
+};
+
#endif
--
2.34.1
Hi Julia,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.7-rc6]
[also build test WARNING on linus/master]
[cannot apply to drm-misc/drm-misc-next drm/drm-next next-20231222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Julia-Zhang/drm-virtio-Implement-RESOURCE_GET_LAYOUT-ioctl/20231222-182142
base: v6.7-rc6
patch link: https://lore.kernel.org/r/20231221100016.4022353-2-julia.zhang%40amd.com
patch subject: [PATCH 1/1] drm/virtio: Implement RESOURCE_GET_LAYOUT ioctl
config: i386-randconfig-004-20231225 (https://download.01.org/0day-ci/archive/20231225/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231225/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/virtio/virtgpu_ioctl.c:712:1: warning: unused label 'valid' [-Wunused-label]
valid:
^~~~~~
1 warning generated.
vim +/valid +712 drivers/gpu/drm/virtio/virtgpu_ioctl.c
673
674 static int virtio_gpu_resource_query_layout_ioctl(struct drm_device *dev,
675 void *data,
676 struct drm_file *file)
677 {
678 struct drm_virtgpu_resource_query_layout *args = data;
679 struct virtio_gpu_device *vgdev = dev->dev_private;
680 struct drm_gem_object *obj = NULL;
681 struct virtio_gpu_object *bo = NULL;
682 struct virtio_gpu_query_info bo_info = {0};
683 int ret = 0;
684 int i;
685
686 if (!vgdev->has_resource_query_layout) {
687 DRM_ERROR("failing: no RQL on host\n");
688 return -EINVAL;
689 }
690
691 if (args->handle > 0) {
692 obj = drm_gem_object_lookup(file, args->handle);
693 if (obj == NULL) {
694 DRM_ERROR("invalid handle 0x%x\n", args->handle);
695 return -ENOENT;
696 }
697 bo = gem_to_virtio_gpu_obj(obj);
698 }
699
700 ret = virtio_gpu_cmd_get_resource_layout(vgdev, &bo_info, args->width,
701 args->height, args->format,
702 args->bind, bo ? bo->hw_res_handle : 0);
703 if (ret)
704 goto out;
705
706 ret = wait_event_timeout(vgdev->resp_wq,
707 atomic_read(&bo_info.is_valid),
708 5 * HZ);
709 if (!ret)
710 goto out;
711
> 712 valid:
713 smp_rmb();
714 WARN_ON(atomic_read(&bo_info.is_valid));
715 args->num_planes = bo_info.num_planes;
716 args->modifier = bo_info.modifier;
717 for (i = 0; i < args->num_planes; i++) {
718 args->planes[i].offset = bo_info.planes[i].offset;
719 args->planes[i].stride = bo_info.planes[i].stride;
720 }
721 for (; i < VIRTIO_GPU_MAX_RESOURCE_PLANES; i++) {
722 args->planes[i].offset = 0;
723 args->planes[i].stride = 0;
724 }
725 ret = 0;
726
727 out:
728 if (obj)
729 drm_gem_object_put(obj);
730 return ret;
731 }
732
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 12/21/23 13:00, Julia Zhang wrote:
> From: Daniel Stone <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Julia Zhang <[email protected]> # support query
> stride before it's created
> Signed-off-by: Julia Zhang <[email protected]>
> ---
> 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
On Thu, Dec 21, 2023 at 06:00:16PM +0800, Julia Zhang wrote:
> From: Daniel Stone <[email protected]>
>
> 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 <[email protected]>
> Co-developed-by: Julia Zhang <[email protected]> # support query
> stride before it's created
> Signed-off-by: Julia Zhang <[email protected]>
> ---
> 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(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 4334c7608408..98061b714b98 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -148,6 +148,7 @@ static unsigned int features[] = {
> VIRTIO_GPU_F_RESOURCE_UUID,
> VIRTIO_GPU_F_RESOURCE_BLOB,
> VIRTIO_GPU_F_CONTEXT_INIT,
> + VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT,
> };
> static struct virtio_driver virtio_gpu_driver = {
> .feature_table = features,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 96365a772f77..bb5edcfeda54 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -214,6 +214,16 @@ struct virtio_gpu_drv_cap_cache {
> atomic_t is_valid;
> };
>
> +struct virtio_gpu_query_info {
> + uint32_t num_planes;
> + uint64_t modifier;
> + struct {
> + uint64_t offset;
> + uint32_t stride;
> + } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
> + atomic_t is_valid;
> +};
> +
> struct virtio_gpu_device {
> struct drm_device *ddev;
>
> @@ -246,6 +256,7 @@ struct virtio_gpu_device {
> bool has_resource_blob;
> bool has_host_visible;
> bool has_context_init;
> + bool has_resource_query_layout;
> struct virtio_shm_region host_visible_region;
> struct drm_mm host_visible_mm;
>
> @@ -277,7 +288,7 @@ struct virtio_gpu_fpriv {
> };
>
> /* virtgpu_ioctl.c */
> -#define DRM_VIRTIO_NUM_IOCTLS 12
> +#define DRM_VIRTIO_NUM_IOCTLS 13
> extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
> void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
>
> @@ -420,6 +431,15 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
> uint32_t width, uint32_t height,
> uint32_t x, uint32_t y);
>
> +int
> +virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_query_info *bo_info,
> + uint32_t width,
> + uint32_t height,
> + uint32_t format,
> + uint32_t bind,
> + uint32_t hw_res_handle);
> +
> /* virtgpu_display.c */
> int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
> void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index b24b11f25197..216c04314177 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -107,6 +107,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
> case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
> value = vgdev->capset_id_mask;
> break;
> + case VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT:
> + value = vgdev->has_resource_query_layout ? 1 : 0;
> + break;
> default:
> return -EINVAL;
> }
> @@ -668,6 +671,65 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
> return ret;
> }
>
> +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");
> + return -EINVAL;
> + }
> +
> + 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);
> + 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();
Please, please no hand-rolling of coherency/synchronization primitives
without writing an entire paper about why this is correct.
I've done a full-length talk about this:
https://blog.ffwll.ch/2023/07/eoss-prague-locking-engineering.html
See the "Level 3: Lockless Tricks" section here:
https://blog.ffwll.ch/2022/08/locking-hierarchy.html
To fix this please just use a struct completion, which is practically what
you hand-roll here.
Since I looked, on the patch itself: It would be good to add a lot more
context to this, like the userspace work and why exactly the kernel has to
be in the business of knowing all this stuff. Because generally it really
should not, ever: Userspace allocates buffers, userspace better knows how
it allocated its buffers and should share that through userspace protocol
(like wayland linux-dmabuf or x11 dri2/3). Why virtio breaks this needs a
big explainer imo.
Thanks, Sima
> + WARN_ON(atomic_read(&bo_info.is_valid));
> + 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;
> +
> +out:
> + if (obj)
> + drm_gem_object_put(obj);
> + return ret;
> +}
> +
> struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
> DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
> DRM_RENDER_ALLOW),
> @@ -707,4 +769,8 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
>
> DRM_IOCTL_DEF_DRV(VIRTGPU_CONTEXT_INIT, virtio_gpu_context_init_ioctl,
> DRM_RENDER_ALLOW),
> +
> + DRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_QUERY_LAYOUT,
> + virtio_gpu_resource_query_layout_ioctl,
> + DRM_RENDER_ALLOW),
> };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 5a3b5aaed1f3..4f34f4145910 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -175,6 +175,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_BLOB)) {
> vgdev->has_resource_blob = true;
> }
> + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT)) {
> + vgdev->has_resource_query_layout = true;
> + }
> if (virtio_get_shm_region(vgdev->vdev, &vgdev->host_visible_region,
> VIRTIO_GPU_SHM_ID_HOST_VISIBLE)) {
> if (!devm_request_mem_region(&vgdev->vdev->dev,
> @@ -204,8 +207,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
> vgdev->has_resource_blob ? '+' : '-',
> vgdev->has_host_visible ? '+' : '-');
>
> - DRM_INFO("features: %ccontext_init\n",
> - vgdev->has_context_init ? '+' : '-');
> + DRM_INFO("features: %ccontext_init %cresource_query_layout\n",
> + vgdev->has_context_init ? '+' : '-',
> + vgdev->has_resource_query_layout ? '+' : '-');
>
> ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
> if (ret) {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index b1a00c0c25a7..26998a3ac4c2 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -1302,3 +1302,66 @@ void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
>
> virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
> }
> +
> +static void virtio_gpu_cmd_get_resource_layout_cb(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_vbuffer *vbuf)
> +{
> + struct virtio_gpu_resp_resource_layout *resp =
> + (struct virtio_gpu_resp_resource_layout *)vbuf->resp_buf;
> + struct virtio_gpu_query_info *bo_info = vbuf->resp_cb_data;
> + int i;
> +
> + vbuf->resp_cb_data = NULL;
> +
> + if (resp->hdr.type != VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT) {
> + atomic_set(&bo_info->is_valid, 0);
> + goto out;
> + }
> +
> + bo_info->modifier = le64_to_cpu(resp->modifier);
> + bo_info->num_planes = le32_to_cpu(resp->num_planes);
> + for (i = 0; i < bo_info->num_planes; i++) {
> + bo_info->planes[i].stride = le32_to_cpu(resp->planes[i].stride);
> + bo_info->planes[i].offset = le32_to_cpu(resp->planes[i].offset);
> + }
> + smp_wmb();
> + atomic_set(&bo_info->is_valid, 1);
> +
> +out:
> + wake_up_all(&vgdev->resp_wq);
> +}
> +
> +int virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
> + struct virtio_gpu_query_info *bo_info,
> + uint32_t width,
> + uint32_t height,
> + uint32_t format,
> + uint32_t bind,
> + uint32_t hw_res_handle)
> +{
> + struct virtio_gpu_resource_query_layout *cmd_p;
> + struct virtio_gpu_vbuffer *vbuf;
> + void *resp_buf;
> +
> + resp_buf = kzalloc(sizeof(struct virtio_gpu_resp_resource_layout),
> + GFP_KERNEL);
> + if (!resp_buf)
> + return -ENOMEM;
> +
> + cmd_p = virtio_gpu_alloc_cmd_resp
> + (vgdev, &virtio_gpu_cmd_get_resource_layout_cb, &vbuf,
> + sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_resource_layout),
> + resp_buf);
> + memset(cmd_p, 0, sizeof(*cmd_p));
> +
> + cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT);
> + cmd_p->resource_id = cpu_to_le32(hw_res_handle);
> + cmd_p->width = cpu_to_le32(width);
> + cmd_p->height = cpu_to_le32(height);
> + cmd_p->format = cpu_to_le32(format);
> + cmd_p->bind = cpu_to_le32(bind);
> + vbuf->resp_cb_data = bo_info;
> +
> + virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
> + return 0;
> +}
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index b1d0e56565bc..66f7c0fa1d4d 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -48,6 +48,7 @@ extern "C" {
> #define DRM_VIRTGPU_GET_CAPS 0x09
> #define DRM_VIRTGPU_RESOURCE_CREATE_BLOB 0x0a
> #define DRM_VIRTGPU_CONTEXT_INIT 0x0b
> +#define DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT 0x0c
>
> #define VIRTGPU_EXECBUF_FENCE_FD_IN 0x01
> #define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
> @@ -97,6 +98,7 @@ struct drm_virtgpu_execbuffer {
> #define VIRTGPU_PARAM_CROSS_DEVICE 5 /* Cross virtio-device resource sharing */
> #define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
> #define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
> +#define VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT 8 /* DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT (also needs cap) */
>
> struct drm_virtgpu_getparam {
> __u64 param;
> @@ -211,6 +213,21 @@ struct drm_virtgpu_context_init {
> __u64 ctx_set_params;
> };
>
> +#define VIRTIO_GPU_MAX_RESOURCE_PLANES 4
> +struct drm_virtgpu_resource_query_layout {
> + __u32 handle;
> + __u32 width;
> + __u32 height;
> + __u32 format;
> + __u32 bind;
> + __u32 num_planes;
> + __u64 modifier;
> + struct {
> + __u64 offset;
> + __u32 stride;
> + } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
> +};
> +
> /*
> * Event code that's given when VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is in
> * effect. The event size is sizeof(drm_event), since there is no additional
> @@ -261,6 +278,10 @@ struct drm_virtgpu_context_init {
> DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_CONTEXT_INIT, \
> struct drm_virtgpu_context_init)
>
> +#define DRM_IOCTL_VIRTGPU_RESOURCE_QUERY_LAYOUT \
> + DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT, \
> + struct drm_virtgpu_resource_query_layout)
> +
> #if defined(__cplusplus)
> }
> #endif
> 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;
> +};
> +
> +
> +/* 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];
> +};
> +
> #endif
> --
> 2.34.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On 2024/1/10 18:47, Daniel Vetter wrote:
> On Thu, Dec 21, 2023 at 06:00:16PM +0800, Julia Zhang wrote:
>> From: Daniel Stone <[email protected]>
>>
>> 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 <[email protected]>
>> Co-developed-by: Julia Zhang <[email protected]> # support query
>> stride before it's created
>> Signed-off-by: Julia Zhang <[email protected]>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> index 4334c7608408..98061b714b98 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
>> @@ -148,6 +148,7 @@ static unsigned int features[] = {
>> VIRTIO_GPU_F_RESOURCE_UUID,
>> VIRTIO_GPU_F_RESOURCE_BLOB,
>> VIRTIO_GPU_F_CONTEXT_INIT,
>> + VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT,
>> };
>> static struct virtio_driver virtio_gpu_driver = {
>> .feature_table = features,
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> index 96365a772f77..bb5edcfeda54 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>> @@ -214,6 +214,16 @@ struct virtio_gpu_drv_cap_cache {
>> atomic_t is_valid;
>> };
>>
>> +struct virtio_gpu_query_info {
>> + uint32_t num_planes;
>> + uint64_t modifier;
>> + struct {
>> + uint64_t offset;
>> + uint32_t stride;
>> + } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
>> + atomic_t is_valid;
>> +};
>> +
>> struct virtio_gpu_device {
>> struct drm_device *ddev;
>>
>> @@ -246,6 +256,7 @@ struct virtio_gpu_device {
>> bool has_resource_blob;
>> bool has_host_visible;
>> bool has_context_init;
>> + bool has_resource_query_layout;
>> struct virtio_shm_region host_visible_region;
>> struct drm_mm host_visible_mm;
>>
>> @@ -277,7 +288,7 @@ struct virtio_gpu_fpriv {
>> };
>>
>> /* virtgpu_ioctl.c */
>> -#define DRM_VIRTIO_NUM_IOCTLS 12
>> +#define DRM_VIRTIO_NUM_IOCTLS 13
>> extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
>> void virtio_gpu_create_context(struct drm_device *dev, struct drm_file *file);
>>
>> @@ -420,6 +431,15 @@ virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
>> uint32_t width, uint32_t height,
>> uint32_t x, uint32_t y);
>>
>> +int
>> +virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
>> + struct virtio_gpu_query_info *bo_info,
>> + uint32_t width,
>> + uint32_t height,
>> + uint32_t format,
>> + uint32_t bind,
>> + uint32_t hw_res_handle);
>> +
>> /* virtgpu_display.c */
>> int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev);
>> void virtio_gpu_modeset_fini(struct virtio_gpu_device *vgdev);
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> index b24b11f25197..216c04314177 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
>> @@ -107,6 +107,9 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
>> case VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs:
>> value = vgdev->capset_id_mask;
>> break;
>> + case VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT:
>> + value = vgdev->has_resource_query_layout ? 1 : 0;
>> + break;
>> default:
>> return -EINVAL;
>> }
>> @@ -668,6 +671,65 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
>> return ret;
>> }
>>
>> +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");
>> + return -EINVAL;
>> + }
>> +
>> + 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);
>> + 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();
>
> Please, please no hand-rolling of coherency/synchronization primitives
> without writing an entire paper about why this is correct.
>
> I've done a full-length talk about this:
>
> https://blog.ffwll.ch/2023/07/eoss-prague-locking-engineering.html
>
> See the "Level 3: Lockless Tricks" section here:
>
> https://blog.ffwll.ch/2022/08/locking-hierarchy.html
>
> To fix this please just use a struct completion, which is practically what
> you hand-roll here.
>
> Since I looked, on the patch itself: It would be good to add a lot more
> context to this, like the userspace work and why exactly the kernel has to
> be in the business of knowing all this stuff. Because generally it really
> should not, ever: Userspace allocates buffers, userspace better knows how
> it allocated its buffers and should share that through userspace protocol
> (like wayland linux-dmabuf or x11 dri2/3). Why virtio breaks this needs a
> big explainer imo.
Yes I see your point, thank you very much.
Actually we are going to implement another way to get those information by
using existing submit_cmd ioctl of virtio gpu driver. We will create a buffer
in guest mesa to pass and get information between host and guest. So these
patches can be dropped.
Regards,
Julia
>
> Thanks, Sima
>
>> + WARN_ON(atomic_read(&bo_info.is_valid));
>> + 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;
>> +
>> +out:
>> + if (obj)
>> + drm_gem_object_put(obj);
>> + return ret;
>> +}
>> +
>> struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
>> DRM_IOCTL_DEF_DRV(VIRTGPU_MAP, virtio_gpu_map_ioctl,
>> DRM_RENDER_ALLOW),
>> @@ -707,4 +769,8 @@ struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS] = {
>>
>> DRM_IOCTL_DEF_DRV(VIRTGPU_CONTEXT_INIT, virtio_gpu_context_init_ioctl,
>> DRM_RENDER_ALLOW),
>> +
>> + DRM_IOCTL_DEF_DRV(VIRTGPU_RESOURCE_QUERY_LAYOUT,
>> + virtio_gpu_resource_query_layout_ioctl,
>> + DRM_RENDER_ALLOW),
>> };
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
>> index 5a3b5aaed1f3..4f34f4145910 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
>> @@ -175,6 +175,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
>> if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_BLOB)) {
>> vgdev->has_resource_blob = true;
>> }
>> + if (virtio_has_feature(vgdev->vdev, VIRTIO_GPU_F_RESOURCE_QUERY_LAYOUT)) {
>> + vgdev->has_resource_query_layout = true;
>> + }
>> if (virtio_get_shm_region(vgdev->vdev, &vgdev->host_visible_region,
>> VIRTIO_GPU_SHM_ID_HOST_VISIBLE)) {
>> if (!devm_request_mem_region(&vgdev->vdev->dev,
>> @@ -204,8 +207,9 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
>> vgdev->has_resource_blob ? '+' : '-',
>> vgdev->has_host_visible ? '+' : '-');
>>
>> - DRM_INFO("features: %ccontext_init\n",
>> - vgdev->has_context_init ? '+' : '-');
>> + DRM_INFO("features: %ccontext_init %cresource_query_layout\n",
>> + vgdev->has_context_init ? '+' : '-',
>> + vgdev->has_resource_query_layout ? '+' : '-');
>>
>> ret = virtio_find_vqs(vgdev->vdev, 2, vqs, callbacks, names, NULL);
>> if (ret) {
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
>> index b1a00c0c25a7..26998a3ac4c2 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
>> @@ -1302,3 +1302,66 @@ void virtio_gpu_cmd_set_scanout_blob(struct virtio_gpu_device *vgdev,
>>
>> virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
>> }
>> +
>> +static void virtio_gpu_cmd_get_resource_layout_cb(struct virtio_gpu_device *vgdev,
>> + struct virtio_gpu_vbuffer *vbuf)
>> +{
>> + struct virtio_gpu_resp_resource_layout *resp =
>> + (struct virtio_gpu_resp_resource_layout *)vbuf->resp_buf;
>> + struct virtio_gpu_query_info *bo_info = vbuf->resp_cb_data;
>> + int i;
>> +
>> + vbuf->resp_cb_data = NULL;
>> +
>> + if (resp->hdr.type != VIRTIO_GPU_RESP_OK_RESOURCE_LAYOUT) {
>> + atomic_set(&bo_info->is_valid, 0);
>> + goto out;
>> + }
>> +
>> + bo_info->modifier = le64_to_cpu(resp->modifier);
>> + bo_info->num_planes = le32_to_cpu(resp->num_planes);
>> + for (i = 0; i < bo_info->num_planes; i++) {
>> + bo_info->planes[i].stride = le32_to_cpu(resp->planes[i].stride);
>> + bo_info->planes[i].offset = le32_to_cpu(resp->planes[i].offset);
>> + }
>> + smp_wmb();
>> + atomic_set(&bo_info->is_valid, 1);
>> +
>> +out:
>> + wake_up_all(&vgdev->resp_wq);
>> +}
>> +
>> +int virtio_gpu_cmd_get_resource_layout(struct virtio_gpu_device *vgdev,
>> + struct virtio_gpu_query_info *bo_info,
>> + uint32_t width,
>> + uint32_t height,
>> + uint32_t format,
>> + uint32_t bind,
>> + uint32_t hw_res_handle)
>> +{
>> + struct virtio_gpu_resource_query_layout *cmd_p;
>> + struct virtio_gpu_vbuffer *vbuf;
>> + void *resp_buf;
>> +
>> + resp_buf = kzalloc(sizeof(struct virtio_gpu_resp_resource_layout),
>> + GFP_KERNEL);
>> + if (!resp_buf)
>> + return -ENOMEM;
>> +
>> + cmd_p = virtio_gpu_alloc_cmd_resp
>> + (vgdev, &virtio_gpu_cmd_get_resource_layout_cb, &vbuf,
>> + sizeof(*cmd_p), sizeof(struct virtio_gpu_resp_resource_layout),
>> + resp_buf);
>> + memset(cmd_p, 0, sizeof(*cmd_p));
>> +
>> + cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_QUERY_LAYOUT);
>> + cmd_p->resource_id = cpu_to_le32(hw_res_handle);
>> + cmd_p->width = cpu_to_le32(width);
>> + cmd_p->height = cpu_to_le32(height);
>> + cmd_p->format = cpu_to_le32(format);
>> + cmd_p->bind = cpu_to_le32(bind);
>> + vbuf->resp_cb_data = bo_info;
>> +
>> + virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
>> + return 0;
>> +}
>> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
>> index b1d0e56565bc..66f7c0fa1d4d 100644
>> --- a/include/uapi/drm/virtgpu_drm.h
>> +++ b/include/uapi/drm/virtgpu_drm.h
>> @@ -48,6 +48,7 @@ extern "C" {
>> #define DRM_VIRTGPU_GET_CAPS 0x09
>> #define DRM_VIRTGPU_RESOURCE_CREATE_BLOB 0x0a
>> #define DRM_VIRTGPU_CONTEXT_INIT 0x0b
>> +#define DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT 0x0c
>>
>> #define VIRTGPU_EXECBUF_FENCE_FD_IN 0x01
>> #define VIRTGPU_EXECBUF_FENCE_FD_OUT 0x02
>> @@ -97,6 +98,7 @@ struct drm_virtgpu_execbuffer {
>> #define VIRTGPU_PARAM_CROSS_DEVICE 5 /* Cross virtio-device resource sharing */
>> #define VIRTGPU_PARAM_CONTEXT_INIT 6 /* DRM_VIRTGPU_CONTEXT_INIT */
>> #define VIRTGPU_PARAM_SUPPORTED_CAPSET_IDs 7 /* Bitmask of supported capability set ids */
>> +#define VIRTGPU_PARAM_RESOURCE_QUERY_LAYOUT 8 /* DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT (also needs cap) */
>>
>> struct drm_virtgpu_getparam {
>> __u64 param;
>> @@ -211,6 +213,21 @@ struct drm_virtgpu_context_init {
>> __u64 ctx_set_params;
>> };
>>
>> +#define VIRTIO_GPU_MAX_RESOURCE_PLANES 4
>> +struct drm_virtgpu_resource_query_layout {
>> + __u32 handle;
>> + __u32 width;
>> + __u32 height;
>> + __u32 format;
>> + __u32 bind;
>> + __u32 num_planes;
>> + __u64 modifier;
>> + struct {
>> + __u64 offset;
>> + __u32 stride;
>> + } planes[VIRTIO_GPU_MAX_RESOURCE_PLANES];
>> +};
>> +
>> /*
>> * Event code that's given when VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK is in
>> * effect. The event size is sizeof(drm_event), since there is no additional
>> @@ -261,6 +278,10 @@ struct drm_virtgpu_context_init {
>> DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_CONTEXT_INIT, \
>> struct drm_virtgpu_context_init)
>>
>> +#define DRM_IOCTL_VIRTGPU_RESOURCE_QUERY_LAYOUT \
>> + DRM_IOWR(DRM_COMMAND_BASE + DRM_VIRTGPU_RESOURCE_QUERY_LAYOUT, \
>> + struct drm_virtgpu_resource_query_layout)
>> +
>> #if defined(__cplusplus)
>> }
>> #endif
>> 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;
>> +};
>> +
>> +
>> +/* 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];
>> +};
>> +
>> #endif
>> --
>> 2.34.1
>>
>