[PATCH v1] drm/virtio: Extend blob UAPI with deferred-mapping hinting

Dmitry Osipenko posted 1 patch 1 year ago
drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
drivers/gpu/drm/virtio/virtgpu_gem.c   |  9 ++++++++
drivers/gpu/drm/virtio/virtgpu_ioctl.c |  1 +
drivers/gpu/drm/virtio/virtgpu_vram.c  | 30 +++++++++++++++++++++-----
include/uapi/drm/virtgpu_drm.h         |  4 ++++
5 files changed, 41 insertions(+), 5 deletions(-)
[PATCH v1] drm/virtio: Extend blob UAPI with deferred-mapping hinting
Posted by Dmitry Osipenko 1 year ago
If userspace never maps GEM object, then BO wastes hostmem space
because VirtIO-GPU driver maps VRAM BO at the BO's creating time.

Make mappings on-demand by adding new RESOURCE_CREATE_BLOB IOCTL/UAPI
hinting flag telling that host mapping should be deferred until first
mapping is made when the flag is set by userspace.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
 drivers/gpu/drm/virtio/virtgpu_gem.c   |  9 ++++++++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c |  1 +
 drivers/gpu/drm/virtio/virtgpu_vram.c  | 30 +++++++++++++++++++++-----
 include/uapi/drm/virtgpu_drm.h         |  4 ++++
 5 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 64c236169db8..e5db91a3e8ba 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;
+	uint32_t blob_hints;
 };
 
 struct virtio_gpu_object {
@@ -483,6 +484,7 @@ struct sg_table *virtio_gpu_vram_map_dma_buf(struct virtio_gpu_object *bo,
 void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
 				   struct sg_table *sgt,
 				   enum dma_data_direction dir);
+void virtio_gpu_vram_map_deferred(struct virtio_gpu_object_vram *vram);
 
 /* virtgpu_submit.c */
 int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 7db48d17ee3a..c902b68cc25a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -103,12 +103,21 @@ int virtio_gpu_mode_dumb_mmap(struct drm_file *file_priv,
 			      struct drm_device *dev,
 			      uint32_t handle, uint64_t *offset_p)
 {
+	struct virtio_gpu_object_vram *vram;
+	struct virtio_gpu_object *bo;
 	struct drm_gem_object *gobj;
 
 	BUG_ON(!offset_p);
 	gobj = drm_gem_object_lookup(file_priv, handle);
 	if (gobj == NULL)
 		return -ENOENT;
+
+	bo = gem_to_virtio_gpu_obj(gobj);
+	if (virtio_gpu_is_vram(bo)) {
+		vram = to_virtio_gpu_vram(bo);
+		virtio_gpu_vram_map_deferred(vram);
+	}
+
 	*offset_p = drm_vma_node_offset_addr(&gobj->vma_node);
 	drm_gem_object_put(gobj);
 	return 0;
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index e4f76f315550..51544ee8d3c3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -489,6 +489,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->blob_hints = rc_blob->blob_hints;
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
index 25df81c02783..d72c93e06a4a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vram.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
@@ -41,6 +41,11 @@ static int virtio_gpu_vram_mmap(struct drm_gem_object *obj,
 	if (!(bo->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE))
 		return -EINVAL;
 
+	virtio_gpu_vram_map_deferred(vram);
+
+	if (vram->map_state == STATE_INITIALIZING)
+		virtio_gpu_notify(vgdev);
+
 	wait_event(vgdev->resp_wq, vram->map_state != STATE_INITIALIZING);
 	if (vram->map_state != STATE_OK)
 		return -EINVAL;
@@ -215,14 +220,29 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
 
 	virtio_gpu_cmd_resource_create_blob(vgdev, &vram->base, params, NULL,
 					    0);
-	if (params->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE) {
-		ret = virtio_gpu_vram_map(&vram->base);
-		if (ret) {
-			virtio_gpu_vram_free(obj);
-			return ret;
+	if (!(params->blob_hints & DRM_VIRTGPU_BLOB_FLAG_HINT_DEFER_MAPPING)) {
+		if (params->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE) {
+			ret = virtio_gpu_vram_map(&vram->base);
+			if (ret) {
+				virtio_gpu_vram_free(obj);
+				return ret;
+			}
 		}
 	}
 
 	*bo_ptr = &vram->base;
 	return 0;
 }
+
+void virtio_gpu_vram_map_deferred(struct virtio_gpu_object_vram *vram)
+{
+	static DEFINE_MUTEX(map_lock);
+
+	if (!(vram->base.blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE))
+		return;
+
+	mutex_lock(&map_lock);
+	if (!drm_mm_node_allocated(&vram->vram_node))
+		virtio_gpu_vram_map(&vram->base);
+	mutex_unlock(&map_lock);
+}
diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
index c2ce71987e9b..3004a1d08570 100644
--- a/include/uapi/drm/virtgpu_drm.h
+++ b/include/uapi/drm/virtgpu_drm.h
@@ -194,6 +194,10 @@ struct drm_virtgpu_resource_create_blob {
 	__u32 cmd_size;
 	__u64 cmd;
 	__u64 blob_id;
+
+#define DRM_VIRTGPU_BLOB_FLAG_HINT_DEFER_MAPPING        0x0001
+	__u32 blob_hints;
+	__u32 pad2;
 };
 
 #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
-- 
2.47.1
Re: [PATCH v1] drm/virtio: Extend blob UAPI with deferred-mapping hinting
Posted by Rob Clark 11 months, 4 weeks ago
On Sun, Jan 26, 2025 at 12:43 PM Dmitry Osipenko
<dmitry.osipenko@collabora.com> wrote:
>
> If userspace never maps GEM object, then BO wastes hostmem space
> because VirtIO-GPU driver maps VRAM BO at the BO's creating time.
>
> Make mappings on-demand by adding new RESOURCE_CREATE_BLOB IOCTL/UAPI
> hinting flag telling that host mapping should be deferred until first
> mapping is made when the flag is set by userspace.
>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

I suppose we could have just added a new blob_flag, but then userspace
would need to probe the kernel to see whether the new flag was
supported.  (Which isn't a big deal, that sort of thing is done in
many places.)  But extending the struct does make it more transparent
to userspace.  Either way,

Reviewed-by: Rob Clark <robdclark@gmail.com


> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
>  drivers/gpu/drm/virtio/virtgpu_gem.c   |  9 ++++++++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c |  1 +
>  drivers/gpu/drm/virtio/virtgpu_vram.c  | 30 +++++++++++++++++++++-----
>  include/uapi/drm/virtgpu_drm.h         |  4 ++++
>  5 files changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 64c236169db8..e5db91a3e8ba 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;
> +       uint32_t blob_hints;
>  };
>
>  struct virtio_gpu_object {
> @@ -483,6 +484,7 @@ struct sg_table *virtio_gpu_vram_map_dma_buf(struct virtio_gpu_object *bo,
>  void virtio_gpu_vram_unmap_dma_buf(struct device *dev,
>                                    struct sg_table *sgt,
>                                    enum dma_data_direction dir);
> +void virtio_gpu_vram_map_deferred(struct virtio_gpu_object_vram *vram);
>
>  /* virtgpu_submit.c */
>  int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 7db48d17ee3a..c902b68cc25a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -103,12 +103,21 @@ int virtio_gpu_mode_dumb_mmap(struct drm_file *file_priv,
>                               struct drm_device *dev,
>                               uint32_t handle, uint64_t *offset_p)
>  {
> +       struct virtio_gpu_object_vram *vram;
> +       struct virtio_gpu_object *bo;
>         struct drm_gem_object *gobj;
>
>         BUG_ON(!offset_p);
>         gobj = drm_gem_object_lookup(file_priv, handle);
>         if (gobj == NULL)
>                 return -ENOENT;
> +
> +       bo = gem_to_virtio_gpu_obj(gobj);
> +       if (virtio_gpu_is_vram(bo)) {
> +               vram = to_virtio_gpu_vram(bo);
> +               virtio_gpu_vram_map_deferred(vram);
> +       }
> +
>         *offset_p = drm_vma_node_offset_addr(&gobj->vma_node);
>         drm_gem_object_put(gobj);
>         return 0;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index e4f76f315550..51544ee8d3c3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -489,6 +489,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->blob_hints = rc_blob->blob_hints;
>         return 0;
>  }
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vram.c b/drivers/gpu/drm/virtio/virtgpu_vram.c
> index 25df81c02783..d72c93e06a4a 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vram.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vram.c
> @@ -41,6 +41,11 @@ static int virtio_gpu_vram_mmap(struct drm_gem_object *obj,
>         if (!(bo->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE))
>                 return -EINVAL;
>
> +       virtio_gpu_vram_map_deferred(vram);
> +
> +       if (vram->map_state == STATE_INITIALIZING)
> +               virtio_gpu_notify(vgdev);
> +
>         wait_event(vgdev->resp_wq, vram->map_state != STATE_INITIALIZING);
>         if (vram->map_state != STATE_OK)
>                 return -EINVAL;
> @@ -215,14 +220,29 @@ int virtio_gpu_vram_create(struct virtio_gpu_device *vgdev,
>
>         virtio_gpu_cmd_resource_create_blob(vgdev, &vram->base, params, NULL,
>                                             0);
> -       if (params->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE) {
> -               ret = virtio_gpu_vram_map(&vram->base);
> -               if (ret) {
> -                       virtio_gpu_vram_free(obj);
> -                       return ret;
> +       if (!(params->blob_hints & DRM_VIRTGPU_BLOB_FLAG_HINT_DEFER_MAPPING)) {
> +               if (params->blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE) {
> +                       ret = virtio_gpu_vram_map(&vram->base);
> +                       if (ret) {
> +                               virtio_gpu_vram_free(obj);
> +                               return ret;
> +                       }
>                 }
>         }
>
>         *bo_ptr = &vram->base;
>         return 0;
>  }
> +
> +void virtio_gpu_vram_map_deferred(struct virtio_gpu_object_vram *vram)
> +{
> +       static DEFINE_MUTEX(map_lock);
> +
> +       if (!(vram->base.blob_flags & VIRTGPU_BLOB_FLAG_USE_MAPPABLE))
> +               return;
> +
> +       mutex_lock(&map_lock);
> +       if (!drm_mm_node_allocated(&vram->vram_node))
> +               virtio_gpu_vram_map(&vram->base);
> +       mutex_unlock(&map_lock);
> +}
> diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> index c2ce71987e9b..3004a1d08570 100644
> --- a/include/uapi/drm/virtgpu_drm.h
> +++ b/include/uapi/drm/virtgpu_drm.h
> @@ -194,6 +194,10 @@ struct drm_virtgpu_resource_create_blob {
>         __u32 cmd_size;
>         __u64 cmd;
>         __u64 blob_id;
> +
> +#define DRM_VIRTGPU_BLOB_FLAG_HINT_DEFER_MAPPING        0x0001
> +       __u32 blob_hints;
> +       __u32 pad2;
>  };
>
>  #define VIRTGPU_CONTEXT_PARAM_CAPSET_ID       0x0001
> --
> 2.47.1
>
Re: [PATCH v1] drm/virtio: Extend blob UAPI with deferred-mapping hinting
Posted by Dmitry Osipenko 11 months, 4 weeks ago
On 2/14/25 20:45, Rob Clark wrote:
> On Sun, Jan 26, 2025 at 12:43 PM Dmitry Osipenko
> <dmitry.osipenko@collabora.com> wrote:
>>
>> If userspace never maps GEM object, then BO wastes hostmem space
>> because VirtIO-GPU driver maps VRAM BO at the BO's creating time.
>>
>> Make mappings on-demand by adding new RESOURCE_CREATE_BLOB IOCTL/UAPI
>> hinting flag telling that host mapping should be deferred until first
>> mapping is made when the flag is set by userspace.
>>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> 
> I suppose we could have just added a new blob_flag, but then userspace
> would need to probe the kernel to see whether the new flag was
> supported.  (Which isn't a big deal, that sort of thing is done in
> many places.)  But extending the struct does make it more transparent
> to userspace.  Either way,
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com

Thanks for the review!

-- 
Best regards,
Dmitry