diff mbox series

[RFC,1/1] virtio-gpu: Use dmabuf for display updates if possible instead of pixman

Message ID 20210302080358.3095748-2-vivek.kasireddy@intel.com (mailing list archive)
State New, archived
Headers show
Series Use dmabufs for display updates instead of pixman | expand

Commit Message

Kasireddy, Vivek March 2, 2021, 8:03 a.m. UTC 
If a dmabuf can be created using Udmabuf driver for non-Virgil rendered
resources, then this should be preferred over pixman. If a dmabuf cannot
be created then we can fallback to pixman.

The dmabuf create and release functions are inspired by similar
functions in vfio/display.c.

Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
---
 hw/display/virtio-gpu.c        | 133 +++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-gpu.h |  12 +++
 2 files changed, 145 insertions(+)

Comments

Marc-André Lureau March 2, 2021, 8:29 a.m. UTC | #1 
Hi

(adding the maintainer, Gerd, in CC)

On Tue, Mar 2, 2021 at 12:17 PM Vivek Kasireddy <vivek.kasireddy@intel.com>
wrote:

> If a dmabuf can be created using Udmabuf driver for non-Virgil rendered
> resources, then this should be preferred over pixman. If a dmabuf cannot
> be created then we can fallback to pixman.
>
> The dmabuf create and release functions are inspired by similar
> functions in vfio/display.c.
>
> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
> ---
>  hw/display/virtio-gpu.c        | 133 +++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-gpu.h |  12 +++
>  2 files changed, 145 insertions(+)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 2e4a9822b6..399d46eac3 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -18,6 +18,7 @@
>  #include "trace.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/sysemu.h"
> +#include "exec/ramblock.h"
>  #include "hw/virtio/virtio.h"
>  #include "migration/qemu-file-types.h"
>  #include "hw/virtio/virtio-gpu.h"
> @@ -30,9 +31,14 @@
>  #include "qemu/module.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> +#include "standard-headers/drm/drm_fourcc.h"
> +#include <sys/ioctl.h>
> +
> +#include <linux/udmabuf.h>
>
>  #define VIRTIO_GPU_VM_VERSION 1
>
> +static int udmabuf_fd;
>  static struct virtio_gpu_simple_resource*
>  virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
>
> @@ -519,6 +525,119 @@ static void virtio_unref_resource(pixman_image_t
> *image, void *data)
>      pixman_image_unref(data);
>  }
>
> +static VGPUDMABuf *virtio_gpu_get_dmabuf(VirtIOGPU *g,
> +                                         struct
> virtio_gpu_simple_resource *res)
> +{
> +    VGPUDMABuf *dmabuf;
> +    RAMBlock *rb;
> +    ram_addr_t offset;
> +    struct udmabuf_create_list *create;
> +    uint32_t modifier_hi, modifier_lo;
> +    uint64_t modifier;
> +    static uint64_t ids = 1;
> +    int i, dmabuf_fd;
> +
> +    create = g_malloc0(sizeof(*create) +
> +                       res->iov_cnt * sizeof (struct
> udmabuf_create_item));
> +    if (!create)
> +        return NULL;
>

Pointless allocation check (g_malloc will abort if it failed to allocate)

+
> +    create->count = res->iov_cnt;
> +    create->flags = UDMABUF_FLAGS_CLOEXEC;
> +    for (i = 0; i < res->iov_cnt; i++) {
> +        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false,
> &offset);
> +        if (!rb || rb->fd < 0) {
> +                g_free(create);
> +                return NULL;
> +        }
> +
> +        create->list[i].memfd = rb->fd;
> +        create->list[i].__pad = 0;
> +        create->list[i].offset = offset;
> +        create->list[i].size = res->iov[i].iov_len;
> +    }
> +
> +    dmabuf_fd = ioctl(udmabuf_fd, UDMABUF_CREATE_LIST, create);
> +    if (dmabuf_fd < 0) {
>

It would be useful to print the error with errno.


> +        g_free(create);
>

(we now like new code to use g_auto)

+        return NULL;
> +    }
> +
> +    /* FIXME: We should get the modifier and format info with blob
> resources */
> +    modifier_hi = fourcc_mod_code(INTEL, I915_FORMAT_MOD_X_TILED) >> 32;
> +    modifier_lo = fourcc_mod_code(INTEL,I915_FORMAT_MOD_X_TILED) &
> 0xFFFFFFFF;
>

I have no idea if this format is going to work with various drivers and
matches the underlying memory representation.


> +    modifier = ((uint64_t)modifier_hi << 32) | modifier_lo;
> +
> +    dmabuf = g_new0(VGPUDMABuf, 1);
> +    dmabuf->dmabuf_id = ids++;
> +    dmabuf->buf.width = res->width;
> +    dmabuf->buf.height = res->height;
> +    dmabuf->buf.stride = pixman_image_get_stride(res->image);
> +    dmabuf->buf.fourcc = DRM_FORMAT_XRGB8888;
> +    dmabuf->buf.modifier = modifier;
> +    dmabuf->buf.fd = dmabuf_fd;
> +
> +    QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
> +    g_free(create);
> +
> +    return dmabuf;
> +}
> +
> +static void virtio_gpu_free_one_dmabuf(VirtIOGPU *g, VGPUDMABuf *dmabuf,
> +                                       struct virtio_gpu_scanout *scanout)
> +{
> +    QTAILQ_REMOVE(&g->dmabuf.bufs, dmabuf, next);
> +    dpy_gl_release_dmabuf(scanout->con, &dmabuf->buf);
> +
> +    close(dmabuf->buf.fd);
> +    g_free(dmabuf);
> +}
> +
> +static void virtio_gpu_free_dmabufs(VirtIOGPU *g,
> +                                    struct virtio_gpu_scanout *scanout)
> +{
> +    VGPUDMABuf *dmabuf, *tmp;
> +    uint32_t keep = 1;
> +
> +    QTAILQ_FOREACH_SAFE(dmabuf, &g->dmabuf.bufs, next, tmp) {
> +        if (keep > 0) {
> +            keep--;
> +            continue;
> +        }
> +        assert(dmabuf != g->dmabuf.primary);
> +        virtio_gpu_free_one_dmabuf(g, dmabuf, scanout);
> +    }
> +}
> +
> +static int virtio_gpu_dmabuf_update(VirtIOGPU *g,
> +                                    struct virtio_gpu_simple_resource
> *res,
> +                                    struct virtio_gpu_scanout *scanout)
> +{
> +    VGPUDMABuf *primary;
> +    bool free_bufs = false;
> +
> +    primary = virtio_gpu_get_dmabuf(g, res);
> +    if (!primary) {
> +        return -EINVAL;
> +    }
> +
> +    if (g->dmabuf.primary != primary) {
> +        g->dmabuf.primary = primary;
> +        qemu_console_resize(scanout->con,
> +                            primary->buf.width, primary->buf.height);
> +        dpy_gl_scanout_dmabuf(scanout->con, &primary->buf);
> +        free_bufs = true;
> +    }
> +
> +    dpy_gl_update(scanout->con, 0, 0, primary->buf.width,
> primary->buf.height);
> +
> +    if (free_bufs) {
> +        virtio_gpu_free_dmabufs(g, scanout);
> +    }
> +
> +    return 0;
> +}
> +
>  static void virtio_gpu_set_scanout(VirtIOGPU *g,
>                                     struct virtio_gpu_ctrl_command *cmd)
>  {
> @@ -528,6 +647,7 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>      uint32_t offset;
>      int bpp;
>      struct virtio_gpu_set_scanout ss;
> +    int ret;
>
>      VIRTIO_GPU_FILL_CMD(ss);
>      virtio_gpu_bswap_32(&ss, sizeof(ss));
> @@ -574,6 +694,12 @@ static void virtio_gpu_set_scanout(VirtIOGPU *g,
>
>      scanout = &g->parent_obj.scanout[ss.scanout_id];
>
> +    if (udmabuf_fd > 0) {
> +        ret = virtio_gpu_dmabuf_update(g, res, scanout);
> +        if (!ret)
> +            return;
> +    }
> +
>      format = pixman_image_get_format(res->image);
>      bpp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
>      offset = (ss.r.x * bpp) + ss.r.y *
> pixman_image_get_stride(res->image);
> @@ -1139,6 +1265,13 @@ static void virtio_gpu_device_realize(DeviceState
> *qdev, Error **errp)
>          return;
>      }
>
> +    udmabuf_fd = open("/dev/udmabuf", O_RDWR);
>

If udmabuf_fd is already opened, this will leak fds.


> +    if (udmabuf_fd < 0) {
> +        error_setg_errno(errp, errno,
> +                         "udmabuf: failed to open vhost device");
> +        return;
>

The fallback path should keep working

+    }
> +
>      g->ctrl_vq = virtio_get_queue(vdev, 0);
>      g->cursor_vq = virtio_get_queue(vdev, 1);
>      g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
> diff --git a/include/hw/virtio/virtio-gpu.h
> b/include/hw/virtio/virtio-gpu.h
> index fae149235c..153f3364a9 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -131,6 +131,13 @@ struct VirtIOGPUBaseClass {
>      DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1024), \
>      DEFINE_PROP_UINT32("yres", _state, _conf.yres, 768)
>
> +typedef struct VGPUDMABuf {
> +    QemuDmaBuf buf;
> +    uint32_t x, y;
> +    uint64_t dmabuf_id;
> +    QTAILQ_ENTRY(VGPUDMABuf) next;
> +} VGPUDMABuf;
> +
>  struct VirtIOGPU {
>      VirtIOGPUBase parent_obj;
>
> @@ -161,6 +168,11 @@ struct VirtIOGPU {
>          uint32_t req_3d;
>          uint32_t bytes_3d;
>      } stats;
> +
> +    struct {
> +        QTAILQ_HEAD(, VGPUDMABuf) bufs;
> +        VGPUDMABuf *primary;
> +    } dmabuf;
>  };
>
>  struct VhostUserGPU {
> --
> 2.26.2
>
>
>
It would be worth doing a similar change in vhost-user-gpu.
Kasireddy, Vivek March 9, 2021, 8:26 a.m. UTC | #2 
Hi Marc-Andre,

<snip>
+    create = g_malloc0(sizeof(*create) +
+                       res->iov_cnt * sizeof (struct udmabuf_create_item));
+    if (!create)
+        return NULL;

Pointless allocation check (g_malloc will abort if it failed to allocate)
[Kasireddy, Vivek] Ok, will remove it in v2.

+
+    create->count = res->iov_cnt;
+    create->flags = UDMABUF_FLAGS_CLOEXEC;
+    for (i = 0; i < res->iov_cnt; i++) {
+        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
+        if (!rb || rb->fd < 0) {
+                g_free(create);
+                return NULL;
+        }
+
+        create->list[i].memfd = rb->fd;
+        create->list[i].__pad = 0;
+        create->list[i].offset = offset;
+        create->list[i].size = res->iov[i].iov_len;
+    }
+
+    dmabuf_fd = ioctl(udmabuf_fd, UDMABUF_CREATE_LIST, create);
+    if (dmabuf_fd < 0) {

It would be useful to print the error with errno.
[Kasireddy, Vivek] Sure.

+        g_free(create);

(we now like new code to use g_auto)

+        return NULL;
+    }
+
+    /* FIXME: We should get the modifier and format info with blob resources */
+    modifier_hi = fourcc_mod_code(INTEL, I915_FORMAT_MOD_X_TILED) >> 32;
+    modifier_lo = fourcc_mod_code(INTEL,I915_FORMAT_MOD_X_TILED) & 0xFFFFFFFF;

I have no idea if this format is going to work with various drivers and matches the underlying memory representation.
[Kasireddy, Vivek] No, it won’t work with other drivers. The modifier information should
be obtained from the Guest that allocates the buffers. I just added it as a temporary hack
for testing with a Intel GPU.

+    modifier = ((uint64_t)modifier_hi << 32) | modifier_lo;
+
+    dmabuf = g_new0(VGPUDMABuf, 1);
+    dmabuf->dmabuf_id = ids++;
+    dmabuf->buf.width = res->width;
+    dmabuf->buf.height = res->height;
+    dmabuf->buf.stride = pixman_image_get_stride(res->image);
+    dmabuf->buf.fourcc = DRM_FORMAT_XRGB8888;
+    dmabuf->buf.modifier = modifier;
+    dmabuf->buf.fd = dmabuf_fd;
+
+    QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
+    g_free(create);
+
+    return dmabuf;
+}


+    udmabuf_fd = open("/dev/udmabuf", O_RDWR);

If udmabuf_fd is already opened, this will leak fds.
[Kasireddy, Vivek] Yup, will take care of it in v2.

+    if (udmabuf_fd < 0) {
+        error_setg_errno(errp, errno,
+                         "udmabuf: failed to open vhost device");
+        return;

The fallback path should keep working
[Kasireddy, Vivek] Makes sense; will fix it in v2.


It would be worth doing a similar change in vhost-user-gpu.
[Kasireddy, Vivek] Sure, will look into it after understanding the deltas between
vhost-user and the in-qemu implementations.

Thanks,
Vivek

--
Marc-André Lureau
diff mbox series

Patch

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 2e4a9822b6..399d46eac3 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -18,6 +18,7 @@ 
 #include "trace.h"
 #include "sysemu/dma.h"
 #include "sysemu/sysemu.h"
+#include "exec/ramblock.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
 #include "hw/virtio/virtio-gpu.h"
@@ -30,9 +31,14 @@ 
 #include "qemu/module.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "standard-headers/drm/drm_fourcc.h"
+#include <sys/ioctl.h>
+
+#include <linux/udmabuf.h>
 
 #define VIRTIO_GPU_VM_VERSION 1
 
+static int udmabuf_fd;
 static struct virtio_gpu_simple_resource*
 virtio_gpu_find_resource(VirtIOGPU *g, uint32_t resource_id);
 
@@ -519,6 +525,119 @@  static void virtio_unref_resource(pixman_image_t *image, void *data)
     pixman_image_unref(data);
 }
 
+static VGPUDMABuf *virtio_gpu_get_dmabuf(VirtIOGPU *g,
+                                         struct virtio_gpu_simple_resource *res)
+{
+    VGPUDMABuf *dmabuf;
+    RAMBlock *rb;
+    ram_addr_t offset;
+    struct udmabuf_create_list *create;
+    uint32_t modifier_hi, modifier_lo;
+    uint64_t modifier;
+    static uint64_t ids = 1;
+    int i, dmabuf_fd;
+
+    create = g_malloc0(sizeof(*create) +
+                       res->iov_cnt * sizeof (struct udmabuf_create_item));
+    if (!create)
+        return NULL;
+
+    create->count = res->iov_cnt;
+    create->flags = UDMABUF_FLAGS_CLOEXEC;
+    for (i = 0; i < res->iov_cnt; i++) {
+        rb = qemu_ram_block_from_host(res->iov[i].iov_base, false, &offset);
+        if (!rb || rb->fd < 0) {
+                g_free(create);
+                return NULL;
+        }
+
+        create->list[i].memfd = rb->fd;
+        create->list[i].__pad = 0;
+        create->list[i].offset = offset;
+        create->list[i].size = res->iov[i].iov_len;
+    }
+
+    dmabuf_fd = ioctl(udmabuf_fd, UDMABUF_CREATE_LIST, create);
+    if (dmabuf_fd < 0) {
+        g_free(create);
+        return NULL;
+    }
+
+    /* FIXME: We should get the modifier and format info with blob resources */
+    modifier_hi = fourcc_mod_code(INTEL, I915_FORMAT_MOD_X_TILED) >> 32;
+    modifier_lo = fourcc_mod_code(INTEL,I915_FORMAT_MOD_X_TILED) & 0xFFFFFFFF;
+    modifier = ((uint64_t)modifier_hi << 32) | modifier_lo;
+
+    dmabuf = g_new0(VGPUDMABuf, 1);
+    dmabuf->dmabuf_id = ids++;
+    dmabuf->buf.width = res->width;
+    dmabuf->buf.height = res->height;
+    dmabuf->buf.stride = pixman_image_get_stride(res->image);
+    dmabuf->buf.fourcc = DRM_FORMAT_XRGB8888;
+    dmabuf->buf.modifier = modifier;
+    dmabuf->buf.fd = dmabuf_fd;
+
+    QTAILQ_INSERT_HEAD(&g->dmabuf.bufs, dmabuf, next);
+    g_free(create);
+
+    return dmabuf;
+}
+
+static void virtio_gpu_free_one_dmabuf(VirtIOGPU *g, VGPUDMABuf *dmabuf,
+                                       struct virtio_gpu_scanout *scanout)
+{
+    QTAILQ_REMOVE(&g->dmabuf.bufs, dmabuf, next);
+    dpy_gl_release_dmabuf(scanout->con, &dmabuf->buf);
+
+    close(dmabuf->buf.fd);
+    g_free(dmabuf);
+}
+
+static void virtio_gpu_free_dmabufs(VirtIOGPU *g,
+                                    struct virtio_gpu_scanout *scanout)
+{
+    VGPUDMABuf *dmabuf, *tmp;
+    uint32_t keep = 1;
+
+    QTAILQ_FOREACH_SAFE(dmabuf, &g->dmabuf.bufs, next, tmp) {
+        if (keep > 0) {
+            keep--;
+            continue;
+        }
+        assert(dmabuf != g->dmabuf.primary);
+        virtio_gpu_free_one_dmabuf(g, dmabuf, scanout);
+    }
+}
+
+static int virtio_gpu_dmabuf_update(VirtIOGPU *g,
+                                    struct virtio_gpu_simple_resource *res,
+                                    struct virtio_gpu_scanout *scanout)
+{
+    VGPUDMABuf *primary;
+    bool free_bufs = false;
+
+    primary = virtio_gpu_get_dmabuf(g, res);
+    if (!primary) {
+        return -EINVAL;
+    }
+
+    if (g->dmabuf.primary != primary) {
+        g->dmabuf.primary = primary;
+        qemu_console_resize(scanout->con,
+                            primary->buf.width, primary->buf.height);
+        dpy_gl_scanout_dmabuf(scanout->con, &primary->buf);
+        free_bufs = true;
+    }
+
+    dpy_gl_update(scanout->con, 0, 0, primary->buf.width, primary->buf.height);
+
+    if (free_bufs) {
+        virtio_gpu_free_dmabufs(g, scanout);
+    }
+
+    return 0;
+}
+
 static void virtio_gpu_set_scanout(VirtIOGPU *g,
                                    struct virtio_gpu_ctrl_command *cmd)
 {
@@ -528,6 +647,7 @@  static void virtio_gpu_set_scanout(VirtIOGPU *g,
     uint32_t offset;
     int bpp;
     struct virtio_gpu_set_scanout ss;
+    int ret;
 
     VIRTIO_GPU_FILL_CMD(ss);
     virtio_gpu_bswap_32(&ss, sizeof(ss));
@@ -574,6 +694,12 @@  static void virtio_gpu_set_scanout(VirtIOGPU *g,
 
     scanout = &g->parent_obj.scanout[ss.scanout_id];
 
+    if (udmabuf_fd > 0) {
+        ret = virtio_gpu_dmabuf_update(g, res, scanout);
+        if (!ret)
+            return;
+    }
+
     format = pixman_image_get_format(res->image);
     bpp = DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
     offset = (ss.r.x * bpp) + ss.r.y * pixman_image_get_stride(res->image);
@@ -1139,6 +1265,13 @@  static void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
         return;
     }
 
+    udmabuf_fd = open("/dev/udmabuf", O_RDWR);
+    if (udmabuf_fd < 0) {
+        error_setg_errno(errp, errno,
+                         "udmabuf: failed to open vhost device");
+        return;
+    }
+
     g->ctrl_vq = virtio_get_queue(vdev, 0);
     g->cursor_vq = virtio_get_queue(vdev, 1);
     g->ctrl_bh = qemu_bh_new(virtio_gpu_ctrl_bh, g);
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index fae149235c..153f3364a9 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -131,6 +131,13 @@  struct VirtIOGPUBaseClass {
     DEFINE_PROP_UINT32("xres", _state, _conf.xres, 1024), \
     DEFINE_PROP_UINT32("yres", _state, _conf.yres, 768)
 
+typedef struct VGPUDMABuf {
+    QemuDmaBuf buf;
+    uint32_t x, y;
+    uint64_t dmabuf_id;
+    QTAILQ_ENTRY(VGPUDMABuf) next;
+} VGPUDMABuf;
+
 struct VirtIOGPU {
     VirtIOGPUBase parent_obj;
 
@@ -161,6 +168,11 @@  struct VirtIOGPU {
         uint32_t req_3d;
         uint32_t bytes_3d;
     } stats;
+
+    struct {
+        QTAILQ_HEAD(, VGPUDMABuf) bufs;
+        VGPUDMABuf *primary;
+    } dmabuf;
 };
 
 struct VhostUserGPU {