[PATCH v2 0/7] *** Add virtio gpu userptr support ***

[PATCH v2 0/7] *** Add virtio gpu userptr support ***

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Hello,

This series add virtio gpu userptr support and add libhsakmt capset.
The userptr feature is used for let host access guest user space memory,
this feature is used for GPU compute use case, to enable ROCm/OpenCL native
context. It should be pointed out that we are not to implement SVM here, 
this is just a buffer based userptr implementation.
The libhsakmt capset is used for ROCm context, libhsakmt is like the role 
of libdrm in Mesa.

Patches 1-2 add libhsakmt capset and userptr blob resource flag.
Patches 3-5 implement basic userptr feature, in some popular bench marks,
it has an efficiency of about 70% compared to bare metal in OpenCL API.
Patch 6 adds interval tree to manage userptrs and prevent duplicate creation.

V2: - Split add HSAKMT context and blob userptr resource to two patches.
    - Remove MMU notifier related patches, cause use not moveable user space
      memory with MMU notifier is not a good idea.
    - Remove HSAKMT context check when create context, let all the context
      support the userptr feature.
    - Remove MMU notifier related content in cover letter.
    - Add more comments  for patch 6 in cover letter.

Honglei Huang (7):
  virtio-gpu api: add HSAKMT context
  virtio-gpu api: add blob userptr resource
  drm/virtgpu api: add blob userptr resource
  drm/virtio: implement userptr: probe for the feature
  drm/virtio: implement userptr: add userptr obj
  drm/virtio: advertise base userptr feature to userspace
  drm/virtio: implement userptr: add interval tree

 drivers/gpu/drm/virtio/Makefile          |   3 +-
 drivers/gpu/drm/virtio/virtgpu_debugfs.c |   1 +
 drivers/gpu/drm/virtio/virtgpu_drv.c     |   1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  48 ++++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c   |  20 +-
 drivers/gpu/drm/virtio/virtgpu_kms.c     |   8 +-
 drivers/gpu/drm/virtio/virtgpu_object.c  |   5 +
 drivers/gpu/drm/virtio/virtgpu_userptr.c | 351 +++++++++++++++++++++++
 include/uapi/drm/virtgpu_drm.h           |   5 +-
 include/uapi/linux/virtio_gpu.h          |   7 +
 10 files changed, 443 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/virtio/virtgpu_userptr.c

-- 
2.34.1

[PATCH v2 1/7] virtio-gpu api: add HSAKMT context

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Add a new context named HSAKMT for compute use case.

- The capset VIRTIO_GPU_CAPSET_HSAKMT used for context init,
in this series patches only HSAKMT context can use the userptr
feature. HSAKMT is a GPU compute library in HSA stack, like
the role libdrm in mesa stack.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 include/uapi/linux/virtio_gpu.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index bf2c9cabd207..331fae983193 100644
--- a/include/uapi/linux/virtio_gpu.h
+++ b/include/uapi/linux/virtio_gpu.h
@@ -312,6 +312,7 @@ struct virtio_gpu_cmd_submit {
 /* 3 is reserved for gfxstream */
 #define VIRTIO_GPU_CAPSET_VENUS 4
 #define VIRTIO_GPU_CAPSET_DRM 6
+#define VIRTIO_GPU_CAPSET_HSAKMT 8
 
 /* VIRTIO_GPU_CMD_GET_CAPSET_INFO */
 struct virtio_gpu_get_capset_info {
-- 
2.34.1

[PATCH v2 2/7] virtio-gpu api: add blob userptr resource

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Add a new resource for blob resource, called userptr, used for let
host access guest user space memory, to acquire buffer based userptr
feature in virtio GPU.

- New flag VIRTIO_GPU_BLOB_FLAG_USE_USERPTR used in blob create
to indicate the blob create ioctl is used for create a userptr
blob resource.
- New resource type VIRTIO_GPU_F_RESOURCE_USERPTR is for feature
check and probe.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 include/uapi/linux/virtio_gpu.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/virtio_gpu.h b/include/uapi/linux/virtio_gpu.h
index 331fae983193..4da36a1e62c4 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
 
+/*
+ * VIRTGPU_BLOB_FLAG_USE_USERPTR
+ */
+#define VIRTIO_GPU_F_RESOURCE_USERPTR    5
+
 enum virtio_gpu_ctrl_type {
 	VIRTIO_GPU_UNDEFINED = 0,
 
@@ -405,6 +410,7 @@ struct virtio_gpu_resource_create_blob {
 #define VIRTIO_GPU_BLOB_FLAG_USE_MAPPABLE     0x0001
 #define VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE    0x0002
 #define VIRTIO_GPU_BLOB_FLAG_USE_CROSS_DEVICE 0x0004
+#define VIRTIO_GPU_BLOB_FLAG_USE_USERPTR      0x0008
 	/* zero is invalid blob mem */
 	__le32 blob_mem;
 	__le32 blob_flags;
-- 
2.34.1

[PATCH v2 3/7] drm/virtgpu api: add blob userptr resource

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

This makes blob userptr resource available to guest userspace.

- Flag VIRTGPU_BLOB_FLAG_USE_USERPTR for guest userspace blob create,
enable this flag to indicate blob userptr resource create.
- Flag VIRTGPU_BLOB_FLAG_USERPTR_RDONLY used for read only userptr,
if not set then the userptr will be writeable.
- New parameter blob_userptr for bypass userspace memory address to
virtio GPU, like other userptr design, virtio GPU needs a userspace
memory for device access.

Used for userptr feature, in compute side, this feature is basic and
essential. Let device to access userspace memory directly instead of
copying.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 include/uapi/drm/virtgpu_drm.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index c2ce71987e9b..071f31752721 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -179,13 +179,14 @@ struct drm_virtgpu_resource_create_blob {
 #define VIRTGPU_BLOB_FLAG_USE_MAPPABLE     0x0001
 #define VIRTGPU_BLOB_FLAG_USE_SHAREABLE    0x0002
 #define VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE 0x0004
+#define VIRTGPU_BLOB_FLAG_USE_USERPTR      0x0008
+#define VIRTGPU_BLOB_FLAG_USERPTR_RDONLY   0x0010
 	/* zero is invalid blob_mem */
 	__u32 blob_mem;
 	__u32 blob_flags;
 	__u32 bo_handle;
 	__u32 res_handle;
 	__u64 size;
-
 	/*
 	 * for 3D contexts with VIRTGPU_BLOB_MEM_HOST3D_GUEST and
 	 * VIRTGPU_BLOB_MEM_HOST3D otherwise, must be zero.
@@ -194,6 +195,7 @@ struct drm_virtgpu_resource_create_blob {
 	__u32 cmd_size;
 	__u64 cmd;
 	__u64 blob_id;
+	__u64 userptr;
 };
 
 #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
-- 
2.34.1

[PATCH v2 4/7] drm/virtio: implement userptr: probe for the feature

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Add probe code path for virtio gpu userptr.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_debugfs.c | 1 +
 drivers/gpu/drm/virtio/virtgpu_drv.c     | 1 +
 drivers/gpu/drm/virtio/virtgpu_drv.h     | 1 +
 drivers/gpu/drm/virtio/virtgpu_kms.c     | 8 ++++++--
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index 853dd9aa397e..da9fa034db0e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -57,6 +57,7 @@ static int virtio_gpu_features(struct seq_file *m, void *data)
 	virtio_gpu_add_bool(m, "context init", vgdev->has_context_init);
 	virtio_gpu_add_int(m, "cap sets", vgdev->num_capsets);
 	virtio_gpu_add_int(m, "scanouts", vgdev->num_scanouts);
+	virtio_gpu_add_int(m, "blob userptr", vgdev->has_resource_userptr);
 	if (vgdev->host_visible_region.len) {
 		seq_printf(m, "%-16s : 0x%lx +0x%lx\n", "host visible region",
 			   (unsigned long)vgdev->host_visible_region.addr,
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
index ffca6e2e1c9a..d79558139084 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -151,6 +151,7 @@ static unsigned int features[] = {
 	VIRTIO_GPU_F_RESOURCE_UUID,
 	VIRTIO_GPU_F_RESOURCE_BLOB,
 	VIRTIO_GPU_F_CONTEXT_INIT,
+	VIRTIO_GPU_F_RESOURCE_USERPTR,
 };
 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 64c236169db8..7bdcbaa20ef1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -249,6 +249,7 @@ struct virtio_gpu_device {
 	bool has_resource_blob;
 	bool has_host_visible;
 	bool has_context_init;
+	bool has_resource_userptr;
 	struct virtio_shm_region host_visible_region;
 	struct drm_mm host_visible_mm;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 7dfb2006c561..3d5158caef46 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -174,6 +174,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_USERPTR)) {
+		vgdev->has_resource_userptr = 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,
@@ -197,11 +200,12 @@ int virtio_gpu_init(struct virtio_device *vdev, struct drm_device *dev)
 		vgdev->has_context_init = true;
 	}
 
-	DRM_INFO("features: %cvirgl %cedid %cresource_blob %chost_visible",
+	DRM_INFO("features: %cvirgl %cedid %cresource_blob %chost_visible %cresource_userptr",
 		 vgdev->has_virgl_3d    ? '+' : '-',
 		 vgdev->has_edid        ? '+' : '-',
 		 vgdev->has_resource_blob ? '+' : '-',
-		 vgdev->has_host_visible ? '+' : '-');
+		 vgdev->has_host_visible ? '+' : '-',
+		 vgdev->has_resource_userptr ? '+' : '-');
 
 	DRM_INFO("features: %ccontext_init\n",
 		 vgdev->has_context_init ? '+' : '-');
-- 
2.34.1

[PATCH v2 5/7] drm/virtio: implement userptr: add userptr obj

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Add implement for virtio gpu userptr. Current solution is pinning
all the user space memory. The UMD needs manage all the userptrs.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 drivers/gpu/drm/virtio/Makefile          |   3 +-
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  33 ++++
 drivers/gpu/drm/virtio/virtgpu_object.c  |   5 +
 drivers/gpu/drm/virtio/virtgpu_userptr.c | 230 +++++++++++++++++++++++
 4 files changed, 270 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/virtio/virtgpu_userptr.c

diff --git a/drivers/gpu/drm/virtio/Makefile b/drivers/gpu/drm/virtio/Makefile
index d2e1788a8227..fe7332a621aa 100644
--- a/drivers/gpu/drm/virtio/Makefile
+++ b/drivers/gpu/drm/virtio/Makefile
@@ -6,6 +6,7 @@
 virtio-gpu-y := virtgpu_drv.o virtgpu_kms.o virtgpu_gem.o virtgpu_vram.o \
 	virtgpu_display.o virtgpu_vq.o \
 	virtgpu_fence.o virtgpu_object.o virtgpu_debugfs.o virtgpu_plane.o \
-	virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o virtgpu_submit.o
+	virtgpu_ioctl.o virtgpu_prime.o virtgpu_trace_points.o virtgpu_submit.o \
+	virtgpu_userptr.o
 
 obj-$(CONFIG_DRM_VIRTIO_GPU) += virtio-gpu.o
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 7bdcbaa20ef1..f3dcbd241f5a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -85,6 +85,7 @@ struct virtio_gpu_object_params {
 	uint32_t blob_mem;
 	uint32_t blob_flags;
 	uint64_t blob_id;
+	uint64_t userptr;
 };
 
 struct virtio_gpu_object {
@@ -112,12 +113,38 @@ struct virtio_gpu_object_vram {
 	struct drm_mm_node vram_node;
 };
 
+struct virtio_gpu_object_userptr;
+
+struct virtio_gpu_object_userptr_ops {
+	int (*get_pages)(struct virtio_gpu_object_userptr *userptr);
+	void (*put_pages)(struct virtio_gpu_object_userptr *userptr);
+	void (*release)(struct virtio_gpu_object_userptr *userptr);
+};
+struct virtio_gpu_object_userptr {
+	struct virtio_gpu_object base;
+	const struct virtio_gpu_object_userptr_ops *ops;
+	struct mutex lock;
+
+	uint64_t start;
+	uint32_t npages;
+	uint32_t bo_handle;
+	uint32_t flags;
+
+	struct virtio_gpu_device *vgdev;
+	struct drm_file *file;
+	struct page **pages;
+	struct sg_table *sgt;
+};
+
 #define to_virtio_gpu_shmem(virtio_gpu_object) \
 	container_of((virtio_gpu_object), struct virtio_gpu_object_shmem, base)
 
 #define to_virtio_gpu_vram(virtio_gpu_object) \
 	container_of((virtio_gpu_object), struct virtio_gpu_object_vram, base)
 
+#define to_virtio_gpu_userptr(virtio_gpu_object) \
+	container_of((virtio_gpu_object), struct virtio_gpu_object_userptr, base)
+
 struct virtio_gpu_object_array {
 	struct ww_acquire_ctx ticket;
 	struct list_head next;
@@ -489,4 +516,10 @@ void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
 int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file);
 
+/* virtgpu_userptr.c */
+int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
+			      struct drm_file *file,
+			      struct virtio_gpu_object_params *params,
+			      struct virtio_gpu_object **bo_ptr);
+bool virtio_gpu_is_userptr(struct virtio_gpu_object *bo);
 #endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index c7e74cf13022..31659b0a028d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -80,6 +80,11 @@ void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo)
 		drm_gem_free_mmap_offset(&vram->base.base.base);
 		drm_gem_object_release(&vram->base.base.base);
 		kfree(vram);
+	} else if (virtio_gpu_is_userptr(bo)) {
+		struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
+
+		drm_gem_object_release(&userptr->base.base.base);
+		kfree(userptr);
 	}
 }
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c b/drivers/gpu/drm/virtio/virtgpu_userptr.c
new file mode 100644
index 000000000000..b4a08811d345
--- /dev/null
+++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c
@@ -0,0 +1,230 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/dma-mapping.h>
+#include <linux/mm.h>
+#include <linux/pid.h>
+#include <linux/vmalloc.h>
+
+#include "virtgpu_drv.h"
+#include "drm/drm_gem.h"
+
+static struct sg_table *
+virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj);
+
+static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
+{
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+	struct virtio_gpu_device *vgdev = obj->dev->dev_private;
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
+
+	if (bo->created) {
+		userptr->ops->release(userptr);
+
+		virtio_gpu_cmd_unref_resource(vgdev, bo);
+		virtio_gpu_notify(vgdev);
+	}
+}
+
+static void virtio_gpu_userptr_object_close(struct drm_gem_object *obj,
+					    struct drm_file *file)
+{
+	virtio_gpu_gem_object_close(obj, file);
+}
+
+static const struct drm_gem_object_funcs virtio_gpu_userptr_funcs = {
+	.open = virtio_gpu_gem_object_open,
+	.close = virtio_gpu_userptr_object_close,
+	.free = virtio_gpu_userptr_free,
+	.get_sg_table = virtio_gpu_userptr_get_sg_table,
+};
+
+bool virtio_gpu_is_userptr(struct virtio_gpu_object *bo)
+{
+	return bo->base.base.funcs == &virtio_gpu_userptr_funcs;
+}
+
+static int
+virtio_gpu_userptr_get_pages(struct virtio_gpu_object_userptr *userptr)
+{
+	unsigned int flag = FOLL_LONGTERM;
+	unsigned int num_pages, pinned = 0;
+	int ret = 0;
+
+	if (userptr->pages)
+		return 0;
+
+	userptr->pages = kvmalloc_array(userptr->npages, sizeof(struct page *),
+					GFP_KERNEL);
+	if (!userptr->pages)
+		return -ENOMEM;
+
+	if (!(userptr->flags & VIRTGPU_BLOB_FLAG_USERPTR_RDONLY))
+		flag |= FOLL_WRITE;
+
+	do {
+		num_pages = userptr->npages - pinned;
+
+		ret = pin_user_pages_fast(userptr->start + pinned * PAGE_SIZE,
+					  num_pages, flag,
+					  userptr->pages + pinned);
+
+		if (ret < 0) {
+			if (pinned)
+				unpin_user_pages(userptr->pages, pinned);
+			kvfree(userptr->pages);
+			return ret;
+		}
+
+		pinned += ret;
+
+	} while (pinned < userptr->npages);
+
+	return 0;
+}
+
+static void
+virtio_gpu_userptr_put_pages(struct virtio_gpu_object_userptr *userptr)
+{
+	if (userptr->pages) {
+		unpin_user_pages(userptr->pages, userptr->npages);
+		kvfree(userptr->pages);
+		userptr->pages = NULL;
+	}
+
+	if (userptr->sgt) {
+		sg_free_table(userptr->sgt);
+		kfree(userptr->sgt);
+		userptr->sgt = NULL;
+	}
+}
+
+static void
+virtio_gpu_userptr_release(struct virtio_gpu_object_userptr *userptr)
+{
+	mutex_lock(&userptr->lock);
+	userptr->ops->put_pages(userptr);
+	mutex_unlock(&userptr->lock);
+}
+
+static struct sg_table *
+virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj)
+{
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
+
+	mutex_lock(&userptr->lock);
+	if (!userptr->pages) {
+		if (userptr->ops->get_pages(userptr)) {
+			mutex_unlock(&userptr->lock);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	if (!userptr->sgt)
+		userptr->sgt = drm_prime_pages_to_sg(NULL, userptr->pages,
+						     userptr->npages);
+	mutex_unlock(&userptr->lock);
+
+	return userptr->sgt;
+}
+
+static int
+virtio_gpu_userptr_init(struct drm_device *dev, struct drm_file *file,
+			struct virtio_gpu_object_userptr *userptr,
+			struct virtio_gpu_object_params *params,
+			const struct virtio_gpu_object_userptr_ops *ops)
+{
+	uint32_t page_offset;
+	uint64_t aligned_size;
+	uint64_t aligned_addr;
+	int ret;
+	struct drm_gem_object *obj;
+
+	page_offset = params->userptr & (PAGE_SIZE - 1UL);
+	aligned_addr = params->userptr - page_offset;
+	aligned_size = roundup(page_offset + params->size, PAGE_SIZE);
+
+	userptr->start = aligned_addr;
+	userptr->npages = aligned_size >> PAGE_SHIFT;
+	userptr->flags = params->blob_flags;
+
+	mutex_init(&userptr->lock);
+	userptr->vgdev = dev->dev_private;
+	userptr->file = file;
+	userptr->ops = ops;
+
+	obj = &userptr->base.base.base;
+	obj->funcs = &virtio_gpu_userptr_funcs;
+
+	drm_gem_private_object_init(dev, obj, aligned_size);
+
+	ret = virtio_gpu_resource_id_get(userptr->vgdev,
+					 &userptr->base.hw_res_handle);
+
+	return ret;
+}
+
+static const struct virtio_gpu_object_userptr_ops virtio_gpu_userptr_ops = {
+	.get_pages = virtio_gpu_userptr_get_pages,
+	.put_pages = virtio_gpu_userptr_put_pages,
+	.release = virtio_gpu_userptr_release,
+};
+
+int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
+			      struct drm_file *file,
+			      struct virtio_gpu_object_params *params,
+			      struct virtio_gpu_object **bo_ptr)
+{
+	struct virtio_gpu_object_userptr *userptr;
+	int ret, si;
+	struct sg_table *sgt;
+	struct scatterlist *sg;
+	struct virtio_gpu_mem_entry *ents;
+
+	if (!params->size)
+		return -EINVAL;
+
+	if (!access_ok((char __user *)(unsigned long)params->userptr,
+		       params->size))
+		return -EFAULT;
+
+	userptr = kzalloc(sizeof(*userptr), GFP_KERNEL);
+	if (!userptr)
+		return -ENOMEM;
+
+	ret = virtio_gpu_userptr_init(vgdev->ddev, file, userptr, params,
+				      &virtio_gpu_userptr_ops);
+
+	if (ret)
+		goto failed_free;
+
+	sgt = virtio_gpu_userptr_get_sg_table(&userptr->base.base.base);
+
+	if (IS_ERR(sgt)) {
+		ret = PTR_ERR(sgt);
+		goto failed_free;
+	}
+
+	ents = kvmalloc_array(sgt->nents, sizeof(struct virtio_gpu_mem_entry),
+			      GFP_KERNEL);
+
+	if (!ents) {
+		ret = -ENOMEM;
+		goto failed_free;
+	}
+
+	for_each_sgtable_sg(sgt, sg, si) {
+		(ents)[si].addr = cpu_to_le64(sg_phys(sg));
+		(ents)[si].length = cpu_to_le32(sg->length);
+		(ents)[si].padding = 0;
+	}
+
+	virtio_gpu_cmd_resource_create_blob(vgdev, &userptr->base, params, ents,
+					    sgt->nents);
+
+	*bo_ptr = &userptr->base;
+	return 0;
+
+failed_free:
+	kfree(userptr);
+	return ret;
+}
-- 
2.34.1

[PATCH v2 6/7] drm/virtio: advertise base userptr feature to userspace

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Introduce the basic userptr feature to userspace.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index e4f76f315550..8a89774d0737 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -36,7 +36,9 @@
 
 #define VIRTGPU_BLOB_FLAG_USE_MASK (VIRTGPU_BLOB_FLAG_USE_MAPPABLE | \
 				    VIRTGPU_BLOB_FLAG_USE_SHAREABLE | \
-				    VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE)
+				    VIRTGPU_BLOB_FLAG_USE_CROSS_DEVICE | \
+				    VIRTGPU_BLOB_FLAG_USE_USERPTR | \
+				    VIRTGPU_BLOB_FLAG_USERPTR_RDONLY)
 
 /* Must be called with &virtio_gpu_fpriv.struct_mutex held. */
 static void virtio_gpu_create_context_locked(struct virtio_gpu_device *vgdev,
@@ -444,6 +446,8 @@ static int verify_blob(struct virtio_gpu_device *vgdev,
 {
 	if (!vgdev->has_resource_blob)
 		return -EINVAL;
+	if (!vgdev->has_resource_userptr && rc_blob->userptr)
+		return -EINVAL;
 
 	if (rc_blob->blob_flags & ~VIRTGPU_BLOB_FLAG_USE_MASK)
 		return -EINVAL;
@@ -489,6 +493,7 @@ static int verify_blob(struct virtio_gpu_device *vgdev,
 	params->size = rc_blob->size;
 	params->blob = true;
 	params->blob_flags = rc_blob->blob_flags;
+	params->userptr = rc_blob->userptr;
 	return 0;
 }
 
@@ -527,8 +532,10 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
 				      vfpriv->ctx_id, NULL, NULL);
 	}
 
-	if (guest_blob)
+	if (guest_blob && !params.userptr)
 		ret = virtio_gpu_object_create(vgdev, &params, &bo, NULL);
+	else if (guest_blob && params.userptr)
+		ret = virtio_gpu_userptr_create(vgdev, file, &params, &bo);
 	else if (!guest_blob && host3d_blob)
 		ret = virtio_gpu_vram_create(vgdev, &params, &bo);
 	else
-- 
2.34.1

[PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Honglei Huang]

From: Honglei Huang <Honglei1.Huang@amd.com>

Add interval tree to manage the userptrs to prevent repeat creation.
If the userptr exists, the ioctl will return the existing BO, and it's
offset with the create ioctl address.

Signed-off-by: Honglei Huang <Honglei1.Huang@amd.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  16 ++-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c   |  13 ++-
 drivers/gpu/drm/virtio/virtgpu_userptr.c | 129 ++++++++++++++++++++++-
 include/uapi/drm/virtgpu_drm.h           |   1 +
 4 files changed, 152 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index f3dcbd241f5a..fa5dd46e3732 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -54,6 +54,7 @@
 #define STATE_INITIALIZING 0
 #define STATE_OK 1
 #define STATE_ERR 2
+#define STATE_RES_EXISTS 3
 
 #define MAX_CAPSET_ID 63
 #define MAX_RINGS 64
@@ -114,18 +115,23 @@ struct virtio_gpu_object_vram {
 };
 
 struct virtio_gpu_object_userptr;
+struct virtio_gpu_fpriv;
 
 struct virtio_gpu_object_userptr_ops {
 	int (*get_pages)(struct virtio_gpu_object_userptr *userptr);
 	void (*put_pages)(struct virtio_gpu_object_userptr *userptr);
 	void (*release)(struct virtio_gpu_object_userptr *userptr);
+	int (*insert)(struct virtio_gpu_object_userptr *userptr, struct virtio_gpu_fpriv *fpriv);
+	int (*remove)(struct virtio_gpu_object_userptr *userptr, struct virtio_gpu_fpriv *fpriv);
 };
 struct virtio_gpu_object_userptr {
 	struct virtio_gpu_object base;
 	const struct virtio_gpu_object_userptr_ops *ops;
 	struct mutex lock;
 
+	uint64_t ptr;
 	uint64_t start;
+	uint64_t last;
 	uint32_t npages;
 	uint32_t bo_handle;
 	uint32_t flags;
@@ -134,6 +140,8 @@ struct virtio_gpu_object_userptr {
 	struct drm_file *file;
 	struct page **pages;
 	struct sg_table *sgt;
+
+	struct interval_tree_node it_node;
 };
 
 #define to_virtio_gpu_shmem(virtio_gpu_object) \
@@ -307,6 +315,8 @@ struct virtio_gpu_fpriv {
 	struct mutex context_lock;
 	char debug_name[DEBUG_NAME_MAX_LEN];
 	bool explicit_debug_name;
+	struct rb_root_cached userptrs_tree;
+	struct mutex userptrs_tree_lock;
 };
 
 /* virtgpu_ioctl.c */
@@ -520,6 +530,10 @@ int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 			      struct drm_file *file,
 			      struct virtio_gpu_object_params *params,
-			      struct virtio_gpu_object **bo_ptr);
+			      struct virtio_gpu_object **bo_ptr,
+			      struct drm_virtgpu_resource_create_blob *rc_blob);
 bool virtio_gpu_is_userptr(struct virtio_gpu_object *bo);
+void virtio_gpu_userptr_interval_tree_init(struct virtio_gpu_fpriv *vfpriv);
+void virtio_gpu_userptr_set_handle(struct virtio_gpu_object *qobj,
+				   uint32_t handle);
 #endif
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 8a89774d0737..b512d4b37981 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -534,8 +534,11 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
 
 	if (guest_blob && !params.userptr)
 		ret = virtio_gpu_object_create(vgdev, &params, &bo, NULL);
-	else if (guest_blob && params.userptr)
-		ret = virtio_gpu_userptr_create(vgdev, file, &params, &bo);
+	else if (guest_blob && params.userptr) {
+		ret = virtio_gpu_userptr_create(vgdev, file, &params, &bo, rc_blob);
+		if (ret > 0)
+			return ret;
+	}
 	else if (!guest_blob && host3d_blob)
 		ret = virtio_gpu_vram_create(vgdev, &params, &bo);
 	else
@@ -567,6 +570,9 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
 	rc_blob->res_handle = bo->hw_res_handle;
 	rc_blob->bo_handle = handle;
 
+	if (guest_blob && params.userptr)
+		virtio_gpu_userptr_set_handle(bo, handle);
+
 	/*
 	 * The handle owns the reference now.  But we must drop our
 	 * remaining reference *after* we no longer need to dereference
@@ -691,6 +697,9 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,
 		}
 	}
 
+	if (vgdev->has_resource_userptr)
+		virtio_gpu_userptr_interval_tree_init(vfpriv);
+
 	virtio_gpu_create_context_locked(vgdev, vfpriv);
 	virtio_gpu_notify(vgdev);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_userptr.c b/drivers/gpu/drm/virtio/virtgpu_userptr.c
index b4a08811d345..03398c3b9f30 100644
--- a/drivers/gpu/drm/virtio/virtgpu_userptr.c
+++ b/drivers/gpu/drm/virtio/virtgpu_userptr.c
@@ -10,6 +10,92 @@
 static struct sg_table *
 virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj);
 
+static int virtio_gpu_userptr_insert(struct virtio_gpu_object_userptr *userptr,
+				     struct virtio_gpu_fpriv *vfpriv)
+{
+	if (!userptr->ops->insert)
+		return -EINVAL;
+
+	return userptr->ops->insert(userptr, vfpriv);
+}
+
+static int virtio_gpu_userptr_remove(struct virtio_gpu_object_userptr *userptr,
+				     struct virtio_gpu_fpriv *vfpriv)
+{
+	if (!userptr->ops->remove)
+		return -EINVAL;
+
+	return userptr->ops->remove(userptr, vfpriv);
+}
+
+static uint64_t virtio_gpu_userptr_get_offset(struct virtio_gpu_object *qobj,
+					      uint64_t addr)
+{
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(qobj);
+
+	return PAGE_ALIGN_DOWN(addr) - PAGE_ALIGN_DOWN(userptr->ptr);
+}
+
+static struct virtio_gpu_object_userptr *
+virtio_gpu_userptr_from_addr_range(struct virtio_gpu_fpriv *vfpriv,
+				   u_int64_t start, u_int64_t last)
+{
+	struct interval_tree_node *node;
+	struct virtio_gpu_object_userptr *userptr = NULL;
+	struct virtio_gpu_object_userptr *ret = NULL;
+
+	node = interval_tree_iter_first(&vfpriv->userptrs_tree, start, last);
+
+	while (node) {
+		struct interval_tree_node *next;
+
+		userptr = container_of(node, struct virtio_gpu_object_userptr,
+				       it_node);
+
+		if (start >= userptr->start && last <= userptr->last) {
+			ret = userptr;
+			return ret;
+		}
+
+		next = interval_tree_iter_next(node, start, last);
+		node = next;
+	}
+
+	return ret;
+}
+
+static int virtio_gpu_userptr_insert_interval_tree(
+	struct virtio_gpu_object_userptr *userptr,
+	struct virtio_gpu_fpriv *vfpriv)
+{
+	if (userptr->it_node.start != 0 && userptr->it_node.last != 0) {
+		userptr->it_node.start = userptr->start;
+		userptr->it_node.last = userptr->last;
+		interval_tree_insert(&userptr->it_node, &vfpriv->userptrs_tree);
+		return 0;
+	} else
+		return -EINVAL;
+}
+
+static int virtio_gpu_userptr_remove_interval_tree(
+	struct virtio_gpu_object_userptr *userptr,
+	struct virtio_gpu_fpriv *vfpriv)
+{
+	if (userptr->it_node.start != 0 && userptr->it_node.last != 0) {
+		interval_tree_remove(&userptr->it_node, &vfpriv->userptrs_tree);
+		return 0;
+	} else
+		return -EINVAL;
+}
+
+void virtio_gpu_userptr_set_handle(struct virtio_gpu_object *qobj,
+				   uint32_t handle)
+{
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(qobj);
+
+	userptr->bo_handle = handle;
+}
+
 static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
 {
 	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
@@ -27,6 +113,11 @@ static void virtio_gpu_userptr_free(struct drm_gem_object *obj)
 static void virtio_gpu_userptr_object_close(struct drm_gem_object *obj,
 					    struct drm_file *file)
 {
+	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
+	struct virtio_gpu_object_userptr *userptr = to_virtio_gpu_userptr(bo);
+
+	virtio_gpu_userptr_remove(userptr, file->driver_priv);
+
 	virtio_gpu_gem_object_close(obj, file);
 }
 
@@ -63,9 +154,9 @@ virtio_gpu_userptr_get_pages(struct virtio_gpu_object_userptr *userptr)
 	do {
 		num_pages = userptr->npages - pinned;
 
-		ret = pin_user_pages_fast(userptr->start + pinned * PAGE_SIZE,
-					  num_pages, flag,
-					  userptr->pages + pinned);
+		ret = pin_user_pages_fast(
+			PAGE_ALIGN_DOWN(userptr->start) + pinned * PAGE_SIZE,
+			num_pages, flag, userptr->pages + pinned);
 
 		if (ret < 0) {
 			if (pinned)
@@ -127,6 +218,12 @@ virtio_gpu_userptr_get_sg_table(struct drm_gem_object *obj)
 	return userptr->sgt;
 }
 
+void virtio_gpu_userptr_interval_tree_init(struct virtio_gpu_fpriv *vfpriv)
+{
+	vfpriv->userptrs_tree = RB_ROOT_CACHED;
+	mutex_init(&vfpriv->userptrs_tree_lock);
+}
+
 static int
 virtio_gpu_userptr_init(struct drm_device *dev, struct drm_file *file,
 			struct virtio_gpu_object_userptr *userptr,
@@ -144,6 +241,8 @@ virtio_gpu_userptr_init(struct drm_device *dev, struct drm_file *file,
 	aligned_size = roundup(page_offset + params->size, PAGE_SIZE);
 
 	userptr->start = aligned_addr;
+	userptr->last = aligned_addr + aligned_size - 1UL;
+	userptr->ptr = params->userptr;
 	userptr->npages = aligned_size >> PAGE_SHIFT;
 	userptr->flags = params->blob_flags;
 
@@ -167,13 +266,17 @@ static const struct virtio_gpu_object_userptr_ops virtio_gpu_userptr_ops = {
 	.get_pages = virtio_gpu_userptr_get_pages,
 	.put_pages = virtio_gpu_userptr_put_pages,
 	.release = virtio_gpu_userptr_release,
+	.insert = virtio_gpu_userptr_insert_interval_tree,
+	.remove = virtio_gpu_userptr_remove_interval_tree,
 };
 
 int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 			      struct drm_file *file,
 			      struct virtio_gpu_object_params *params,
-			      struct virtio_gpu_object **bo_ptr)
+			      struct virtio_gpu_object **bo_ptr,
+			      struct drm_virtgpu_resource_create_blob *rc_blob)
 {
+	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_object_userptr *userptr;
 	int ret, si;
 	struct sg_table *sgt;
@@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 		       params->size))
 		return -EFAULT;
 
+	mutex_lock(&vfpriv->userptrs_tree_lock);
+
+	userptr = virtio_gpu_userptr_from_addr_range(
+		vfpriv, params->userptr, params->userptr + params->size - 1UL);
+	if (userptr) {
+		*bo_ptr = &userptr->base;
+		rc_blob->res_handle = userptr->base.hw_res_handle;
+		rc_blob->bo_handle = userptr->bo_handle;
+		rc_blob->offset = virtio_gpu_userptr_get_offset(
+			&userptr->base, rc_blob->userptr);
+		mutex_unlock(&vfpriv->userptrs_tree_lock);
+		return STATE_RES_EXISTS;
+	}
+
 	userptr = kzalloc(sizeof(*userptr), GFP_KERNEL);
 	if (!userptr)
 		return -ENOMEM;
@@ -218,6 +335,9 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 		(ents)[si].padding = 0;
 	}
 
+	virtio_gpu_userptr_insert(userptr, vfpriv);
+	mutex_unlock(&vfpriv->userptrs_tree_lock);
+
 	virtio_gpu_cmd_resource_create_blob(vgdev, &userptr->base, params, ents,
 					    sgt->nents);
 
@@ -225,6 +345,7 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
 	return 0;
 
 failed_free:
+	mutex_unlock(&vfpriv->userptrs_tree_lock);
 	kfree(userptr);
 	return ret;
 }
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index 071f31752721..07c22cf1a9e0 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -196,6 +196,7 @@ struct drm_virtgpu_resource_create_blob {
 	__u64 cmd;
 	__u64 blob_id;
 	__u64 userptr;
+	__u64 offset;
 };
 
 #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
-- 
2.34.1

Re: [PATCH v2 0/7] *** Add virtio gpu userptr support ***

[Demi Marie Obenour]

On 3/21/25 4:00 AM, Honglei Huang wrote:
> From: Honglei Huang <Honglei1.Huang@amd.com>
> 
> Hello,
> 
> This series add virtio gpu userptr support and add libhsakmt capset.
> The userptr feature is used for let host access guest user space memory,
> this feature is used for GPU compute use case, to enable ROCm/OpenCL native
> context. It should be pointed out that we are not to implement SVM here, 
> this is just a buffer based userptr implementation.
> The libhsakmt capset is used for ROCm context, libhsakmt is like the role 
> of libdrm in Mesa.
> 
> Patches 1-2 add libhsakmt capset and userptr blob resource flag.

libhsakmt and userptr are orthogonal from each other.
Should the libhsakmt context be a separate patch series?

> Patches 3-5 implement basic userptr feature, in some popular bench marks,
> it has an efficiency of about 70% compared to bare metal in OpenCL API.
> Patch 6 adds interval tree to manage userptrs and prevent duplicate creation.
> 
> V2: - Split add HSAKMT context and blob userptr resource to two patches.
>     - Remove MMU notifier related patches, cause use not moveable user space
>       memory with MMU notifier is not a good idea.
>     - Remove HSAKMT context check when create context, let all the context
>       support the userptr feature.
>     - Remove MMU notifier related content in cover letter.
>     - Add more comments  for patch 6 in cover letter.

I have not looked at the implementation, but thanks for removing the MMU
notifier support.  Should the interval tree be added before the feature
is exposed to userspace?  That would prevent users who are doing kernel
bisects from temporarily exposing a buggy feature to userspace.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Dmitry Osipenko]

If the purpose of this feature is to dedup usrptr BOs of a the single
process/application, can this can be done in userspace?

On 3/21/25 11:00, Honglei Huang wrote:
>  int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>  			      struct drm_file *file,
>  			      struct virtio_gpu_object_params *params,
> -			      struct virtio_gpu_object **bo_ptr)
> +			      struct virtio_gpu_object **bo_ptr,
> +			      struct drm_virtgpu_resource_create_blob *rc_blob)
>  {
> +	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>  	struct virtio_gpu_object_userptr *userptr;
>  	int ret, si;
>  	struct sg_table *sgt;
> @@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>  		       params->size))
>  		return -EFAULT;
>  
> +	mutex_lock(&vfpriv->userptrs_tree_lock);
> +
> +	userptr = virtio_gpu_userptr_from_addr_range(
> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);

Is it possible that userptr address will be same for two different
processes?

What if userptr->flags mismatch?

> +	if (userptr) {
> +		*bo_ptr = &userptr->base;
> +		rc_blob->res_handle = userptr->base.hw_res_handle;
> +		rc_blob->bo_handle = userptr->bo_handle;

Doesn't BO refcount need to be bumped?

> +		rc_blob->offset = virtio_gpu_userptr_get_offset(
> +			&userptr->base, rc_blob->userptr);
> +		mutex_unlock(&vfpriv->userptrs_tree_lock);
> +		return STATE_RES_EXISTS;

STATE_RES_EXISTS isn't a error code

-- 
Best regards,
Dmitry

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Dmitry Osipenko]

On 3/30/25 14:57, Dmitry Osipenko wrote:
>> +	userptr = virtio_gpu_userptr_from_addr_range(
>> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);
> Is it possible that userptr address will be same for two different
> processes?

See now the vfpriv->userptrs_tree, i.e. it's not a global tree.

-- 
Best regards,
Dmitry

Re: [PATCH v2 0/7] Add virtio gpu userptr support

[Huang, Honglei1]

On 2025/3/30 3:56, Demi Marie Obenour wrote:
> On 3/21/25 4:00 AM, Honglei Huang wrote:
>> From: Honglei Huang <Honglei1.Huang@amd.com>
>>
>> Hello,
>>
>> This series add virtio gpu userptr support and add libhsakmt capset.
>> The userptr feature is used for let host access guest user space memory,
>> this feature is used for GPU compute use case, to enable ROCm/OpenCL native
>> context. It should be pointed out that we are not to implement SVM here,
>> this is just a buffer based userptr implementation.
>> The libhsakmt capset is used for ROCm context, libhsakmt is like the role
>> of libdrm in Mesa.
>>
>> Patches 1-2 add libhsakmt capset and userptr blob resource flag.
> 
> libhsakmt and userptr are orthogonal from each other.
> Should the libhsakmt context be a separate patch series?

I will separate libhsakmt capset patch into another patch series.

> 
>> Patches 3-5 implement basic userptr feature, in some popular bench marks,
>> it has an efficiency of about 70% compared to bare metal in OpenCL API.
>> Patch 6 adds interval tree to manage userptrs and prevent duplicate creation.
>>
>> V2: - Split add HSAKMT context and blob userptr resource to two patches.
>>      - Remove MMU notifier related patches, cause use not moveable user space
>>        memory with MMU notifier is not a good idea.
>>      - Remove HSAKMT context check when create context, let all the context
>>        support the userptr feature.
>>      - Remove MMU notifier related content in cover letter.
>>      - Add more comments  for patch 6 in cover letter.
> 
> I have not looked at the implementation, but thanks for removing the MMU
> notifier support.  Should the interval tree be added before the feature
> is exposed to userspace?  That would prevent users who are doing kernel
> bisects from temporarily exposing a buggy feature to userspace.
Ok I will add interval tree patch before introduce the feature into user 
space in next version. Really thanks for the review.

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Huang, Honglei1]

On 2025/3/30 19:57, Dmitry Osipenko wrote:
> If the purpose of this feature is to dedup usrptr BOs of a the single
> process/application, can this can be done in userspace?
> 
> On 3/21/25 11:00, Honglei Huang wrote:
>>   int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>>   			      struct drm_file *file,
>>   			      struct virtio_gpu_object_params *params,
>> -			      struct virtio_gpu_object **bo_ptr)
>> +			      struct virtio_gpu_object **bo_ptr,
>> +			      struct drm_virtgpu_resource_create_blob *rc_blob)
>>   {
>> +	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
>>   	struct virtio_gpu_object_userptr *userptr;
>>   	int ret, si;
>>   	struct sg_table *sgt;
>> @@ -187,6 +290,20 @@ int virtio_gpu_userptr_create(struct virtio_gpu_device *vgdev,
>>   		       params->size))
>>   		return -EFAULT;
>>   
>> +	mutex_lock(&vfpriv->userptrs_tree_lock);
>> +
>> +	userptr = virtio_gpu_userptr_from_addr_range(
>> +		vfpriv, params->userptr, params->userptr + params->size - 1UL);
> 
> Is it possible that userptr address will be same for two different
> processes?
> 
> What if userptr->flags mismatch?

This situation need to be considered, I will add flag check, and add
handle the situation of increasing write permissions.

> 
>> +	if (userptr) {
>> +		*bo_ptr = &userptr->base;
>> +		rc_blob->res_handle = userptr->base.hw_res_handle;
>> +		rc_blob->bo_handle = userptr->bo_handle;
> 
> Doesn't BO refcount need to be bumped?

Will add Bo refcount in next version.

> 
>> +		rc_blob->offset = virtio_gpu_userptr_get_offset(
>> +			&userptr->base, rc_blob->userptr);
>> +		mutex_unlock(&vfpriv->userptrs_tree_lock);
>> +		return STATE_RES_EXISTS;
> 
> STATE_RES_EXISTS isn't a error code

Will use the system's built-in error code, really thanks for the review.

> 

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Dmitry Osipenko]

On 4/2/25 04:53, Huang, Honglei1 wrote:
> 
> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>> If the purpose of this feature is to dedup usrptr BOs of a the single
>> process/application, can this can be done in userspace? 

I assume it can be done in userspace, don't see why it needs to be in
kernel.

-- 
Best regards,
Dmitry

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Demi Marie Obenour]

On 4/2/25 8:34 AM, Dmitry Osipenko wrote:
> On 4/2/25 04:53, Huang, Honglei1 wrote:
>>
>> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>>> If the purpose of this feature is to dedup usrptr BOs of a the single
>>> process/application, can this can be done in userspace? 
> 
> I assume it can be done in userspace, don't see why it needs to be in
> kernel.

The kernel definitely does not need to be responsible for deduplication,
but is it safe to allow userspace to create overlapping BOs, especially
ones that are partially but not entirely overlapping?  If the userspace
libraries ~everyone will be using refuse to create such BOs, then
overlapping BOs will be tested by ~nobody, and untested kernel code is
a good place for security vulnerabilities to linger.

If there are no legitimate use-cases for overlapping BOs, I would treat
attempts to create them as an errors and return -EINVAL, indicating that
the userspace code attempting to create them is buggy.  Userspace can
deduplicate the BOs itself if necessary.  Of course, there need to be
tests for userspace attempting to create overlapping BOs, including
attempting to do so concurrently from multiple threads.

That said, probably the most important part is consistency with userptr
in other (non-virtio) drivers, such as Intel and AMD.  If they allow
overlapping userptr BOs, then virtio should too; if they do not, then
virtio should also forbid them.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Huang, Honglei1]

Hi Dmitry:

Really sorry for missed this comment. Yes it can be done in UMD, 
actually the interval tree is used with the MMU notifier normally,
it is for preventing create same MMU notifier for overlapped areas.
Cause this version patch set doesn't have MMU notifier, removing 
interval tree is reasonable.

Hi Demi:
Adding interval tree can make virtio userptr has robust check, it can be 
done in UMD. And for AMD userptr driver, it is a SVM type driver, it has
both interval tree and MMU notifier but userptr memory is moveable in 
it. No interval tree for Intel i386, not sure about the Intel XE driver.

Maybe I can remove the interval tree in next version.


On 2025/4/3 2:45, Demi Marie Obenour wrote:
> On 4/2/25 8:34 AM, Dmitry Osipenko wrote:
>> On 4/2/25 04:53, Huang, Honglei1 wrote:
>>>
>>> On 2025/3/30 19:57, Dmitry Osipenko wrote:
>>>> If the purpose of this feature is to dedup usrptr BOs of a the single
>>>> process/application, can this can be done in userspace?
>>
>> I assume it can be done in userspace, don't see why it needs to be in
>> kernel.
> 
> The kernel definitely does not need to be responsible for deduplication,
> but is it safe to allow userspace to create overlapping BOs, especially
> ones that are partially but not entirely overlapping?  If the userspace
> libraries ~everyone will be using refuse to create such BOs, then
> overlapping BOs will be tested by ~nobody, and untested kernel code is
> a good place for security vulnerabilities to linger.
> 
> If there are no legitimate use-cases for overlapping BOs, I would treat
> attempts to create them as an errors and return -EINVAL, indicating that
> the userspace code attempting to create them is buggy.  Userspace can
> deduplicate the BOs itself if necessary.  Of course, there need to be
> tests for userspace attempting to create overlapping BOs, including
> attempting to do so concurrently from multiple threads.
> 
> That said, probably the most important part is consistency with userptr
> in other (non-virtio) drivers, such as Intel and AMD.  If they allow
> overlapping userptr BOs, then virtio should too; if they do not, then
> virtio should also forbid them.

Re: [PATCH v2 7/7] drm/virtio: implement userptr: add interval tree

[Dmitry Osipenko]

On 4/3/25 06:28, Huang, Honglei1 wrote:
> 
> Hi Dmitry:
> 
> Really sorry for missed this comment. Yes it can be done in UMD,
> actually the interval tree is used with the MMU notifier normally,
> it is for preventing create same MMU notifier for overlapped areas.
> Cause this version patch set doesn't have MMU notifier, removing
> interval tree is reasonable.
> 
> Hi Demi:
> Adding interval tree can make virtio userptr has robust check, it can be
> done in UMD. And for AMD userptr driver, it is a SVM type driver, it has
> both interval tree and MMU notifier but userptr memory is moveable in
> it. No interval tree for Intel i386, not sure about the Intel XE driver.
> 
> Maybe I can remove the interval tree in next version.

Great, thanks for the clarification. And no need to be sorry, it happens
to everyone on the ML :D

-- 
Best regards,
Dmitry