[PATCH 0/3] vhost-user-gpu: support dmabuf modifiers

[PATCH 0/3] vhost-user-gpu: support dmabuf modifiers

[Erico Nunes]

virglrenderer recently added virgl_renderer_resource_get_info_ext as a
new api, which gets resource information, including dmabuf modifiers.
https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024

We have to support dmabuf modifiers since the driver may choose to
allocate buffers with these modifiers to improve performance, and
importing buffers without modifiers information may result in completely
broken rendering.

Currently trying to use vhost-user-gpu for rendering backend and using
the qemu dbus ui as a ui backend results in a broken framebuffer with
Intel GPUs as the buffer is allocated with a modifier. With this
patchset, that is fixed.


It is tricky to support since it requires to keep compatibility at the
same time with:
(1) build against older virglrenderer which do not provide
virgl_renderer_resource_get_info_ext;
(2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to
increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT
message.

I tried to reach a compromise here by not defining a completely new
message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels
like a bit of a hack, so I appreciate feedback if there is a better way
(or naming) to handle it.


Erico Nunes (3):
  docs: vhost-user-gpu: add protocol changes for dmabuf modifiers
  contrib/vhost-user-gpu: add support for sending dmabuf modifiers
  vhost-user-gpu: support dmabuf modifiers

 contrib/vhost-user-gpu/vhost-user-gpu.c |  5 ++-
 contrib/vhost-user-gpu/virgl.c          | 51 +++++++++++++++++++++++--
 contrib/vhost-user-gpu/vugpu.h          |  9 +++++
 docs/interop/vhost-user-gpu.rst         | 26 ++++++++++++-
 hw/display/vhost-user-gpu.c             | 17 ++++++++-
 5 files changed, 102 insertions(+), 6 deletions(-)

-- 
2.40.1

[PATCH 1/3] docs: vhost-user-gpu: add protocol changes for dmabuf modifiers

[Erico Nunes]

VHOST_USER_GPU_DMABUF_SCANOUT2 is defined as a message with all the
contents of VHOST_USER_GPU_DMABUF_SCANOUT plus the dmabuf modifiers
which were ommitted.

The VHOST_USER_GPU_PROTOCOL_F_DMABUF2 protocol feature is defined as a
way to check whether this new message is supported or not.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 docs/interop/vhost-user-gpu.rst | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/docs/interop/vhost-user-gpu.rst b/docs/interop/vhost-user-gpu.rst
index b78806892d..3035822d05 100644
--- a/docs/interop/vhost-user-gpu.rst
+++ b/docs/interop/vhost-user-gpu.rst
@@ -134,6 +134,19 @@ VhostUserGpuEdidRequest
 :scanout-id: ``u32``, the scanout to get edid from
 
 
+VhostUserGpuDMABUFScanout2
+^^^^^^^^^^^^^^^^^^^^^^^^^^
+
++----------------+----------+
+| dmabuf_scanout | modifier |
++----------------+----------+
+
+:dmabuf_scanout: ``VhostUserGpuDMABUFScanout``, filled as described in the
+                 VhostUserGpuDMABUFScanout structure.
+
+:modifier: ``u64``, the DMABUF modifiers
+
+
 C structure
 -----------
 
@@ -163,7 +176,8 @@ Protocol features
 
 .. code:: c
 
-  #define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+  #define VHOST_USER_GPU_PROTOCOL_F_EDID    0
+  #define VHOST_USER_GPU_PROTOCOL_F_DMABUF2 1
 
 New messages and communication changes are negotiated thanks to the
 ``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` and
@@ -263,3 +277,13 @@ Message types
   Retrieve the EDID data for a given scanout.
   This message requires the ``VHOST_USER_GPU_PROTOCOL_F_EDID`` protocol
   feature to be supported.
+
+``VHOST_USER_GPU_DMABUF_SCANOUT2``
+  :id: 12
+  :request payload: ``VhostUserGpuDMABUFScanout2``
+  :reply payload: N/A
+
+  Same as VHOST_USER_GPU_DMABUF_SCANOUT, but also sends the dmabuf modifiers
+  appended to the message, which were not provided in the other message.
+  This message requires the ``VHOST_USER_GPU_PROTOCOL_F_DMABUF2`` protocol
+  feature to be supported.
-- 
2.40.1

[PATCH 2/3] contrib/vhost-user-gpu: add support for sending dmabuf modifiers

[Erico Nunes]

virglrenderer recently added virgl_renderer_resource_get_info_ext as a
new api, which gets resource information, including dmabuf modifiers.

We have to support dmabuf modifiers since the driver may choose to
allocate buffers with these modifiers for efficiency, and importing
buffers without modifiers information may result in completely broken
rendering.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c |  5 ++-
 contrib/vhost-user-gpu/virgl.c          | 51 +++++++++++++++++++++++--
 contrib/vhost-user-gpu/vugpu.h          |  9 +++++
 3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index 2e7815a7a3..aa304475a0 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1071,6 +1071,7 @@ static gboolean
 protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
 {
     const uint64_t protocol_edid = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID);
+    const uint64_t protocol_dmabuf2 = (1 << VHOST_USER_GPU_PROTOCOL_F_DMABUF2);
     VuGpu *g = user_data;
     uint64_t protocol_features;
     VhostUserGpuMsg msg = {
@@ -1082,7 +1083,7 @@ protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
         return G_SOURCE_CONTINUE;
     }
 
-    protocol_features &= protocol_edid;
+    protocol_features &= (protocol_edid | protocol_dmabuf2);
 
     msg = (VhostUserGpuMsg) {
         .request = VHOST_USER_GPU_SET_PROTOCOL_FEATURES,
@@ -1100,6 +1101,8 @@ protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
         exit(EXIT_FAILURE);
     }
 
+    g->use_modifiers = !!(protocol_features & protocol_dmabuf2);
+
     return G_SOURCE_REMOVE;
 }
 
diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 211aa110a9..1da6cc1588 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -318,6 +318,37 @@ virgl_resource_detach_backing(VuGpu *g,
     vg_cleanup_mapping_iov(g, res_iovs, num_iovs);
 }
 
+static int
+virgl_get_resource_info_modifiers(uint32_t resource_id,
+                                  struct virgl_renderer_resource_info *info,
+                                  uint64_t *modifiers)
+{
+    int ret;
+#ifdef VIRGL_RENDERER_RESOURCE_INFO_EXT_VERSION
+    struct virgl_renderer_resource_info_ext info_ext;
+    ret = virgl_renderer_resource_get_info_ext(resource_id, &info_ext);
+    if (ret < 0) {
+        return ret;
+    }
+
+    *info = info_ext.base;
+    *modifiers = info_ext.modifiers;
+#else
+    ret = virgl_renderer_resource_get_info(resource_id, info);
+    if (ret < 0) {
+        return ret;
+    }
+
+    /*
+     * Before virgl_renderer_resource_get_info_ext,
+     * getting the modifiers was not possible.
+     */
+    *modifiers = 0;
+#endif
+
+    return 0;
+}
+
 static void
 virgl_cmd_set_scanout(VuGpu *g,
                       struct virtio_gpu_ctrl_command *cmd)
@@ -338,7 +369,9 @@ virgl_cmd_set_scanout(VuGpu *g,
     memset(&info, 0, sizeof(info));
 
     if (ss.resource_id && ss.r.width && ss.r.height) {
-        ret = virgl_renderer_resource_get_info(ss.resource_id, &info);
+        uint64_t modifiers = 0;
+        ret = virgl_get_resource_info_modifiers(ss.resource_id, &info,
+                                                &modifiers);
         if (ret == -1) {
             g_critical("%s: illegal resource specified %d\n",
                        __func__, ss.resource_id);
@@ -354,8 +387,6 @@ virgl_cmd_set_scanout(VuGpu *g,
         }
         assert(fd >= 0);
         VhostUserGpuMsg msg = {
-            .request = VHOST_USER_GPU_DMABUF_SCANOUT,
-            .size = sizeof(VhostUserGpuDMABUFScanout),
             .payload.dmabuf_scanout.scanout_id = ss.scanout_id,
             .payload.dmabuf_scanout.x =  ss.r.x,
             .payload.dmabuf_scanout.y =  ss.r.y,
@@ -367,6 +398,20 @@ virgl_cmd_set_scanout(VuGpu *g,
             .payload.dmabuf_scanout.fd_flags = info.flags,
             .payload.dmabuf_scanout.fd_drm_fourcc = info.drm_fourcc
         };
+
+        if (g->use_modifiers) {
+            /*
+             * The mesage uses all the fields set in dmabuf_scanout plus
+             * modifiers which is appended after VhostUserGpuDMABUFScanout.
+             */
+            msg.request = VHOST_USER_GPU_DMABUF_SCANOUT2;
+            msg.size = sizeof(VhostUserGpuDMABUFScanout2);
+            msg.payload.dmabuf_scanout2.modifier = modifiers;
+        } else {
+            msg.request = VHOST_USER_GPU_DMABUF_SCANOUT;
+            msg.size = sizeof(VhostUserGpuDMABUFScanout);
+        }
+
         vg_send_msg(g, &msg, fd);
         close(fd);
     } else {
diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
index f0f2069c47..509b679f03 100644
--- a/contrib/vhost-user-gpu/vugpu.h
+++ b/contrib/vhost-user-gpu/vugpu.h
@@ -37,6 +37,7 @@ typedef enum VhostUserGpuRequest {
     VHOST_USER_GPU_DMABUF_SCANOUT,
     VHOST_USER_GPU_DMABUF_UPDATE,
     VHOST_USER_GPU_GET_EDID,
+    VHOST_USER_GPU_DMABUF_SCANOUT2,
 } VhostUserGpuRequest;
 
 typedef struct VhostUserGpuDisplayInfoReply {
@@ -84,6 +85,11 @@ typedef struct VhostUserGpuDMABUFScanout {
     int fd_drm_fourcc;
 } QEMU_PACKED VhostUserGpuDMABUFScanout;
 
+typedef struct VhostUserGpuDMABUFScanout2 {
+    struct VhostUserGpuDMABUFScanout dmabuf_scanout;
+    uint64_t modifier;
+} QEMU_PACKED VhostUserGpuDMABUFScanout2;
+
 typedef struct VhostUserGpuEdidRequest {
     uint32_t scanout_id;
 } QEMU_PACKED VhostUserGpuEdidRequest;
@@ -98,6 +104,7 @@ typedef struct VhostUserGpuMsg {
         VhostUserGpuScanout scanout;
         VhostUserGpuUpdate update;
         VhostUserGpuDMABUFScanout dmabuf_scanout;
+        VhostUserGpuDMABUFScanout2 dmabuf_scanout2;
         VhostUserGpuEdidRequest edid_req;
         struct virtio_gpu_resp_edid resp_edid;
         struct virtio_gpu_resp_display_info display_info;
@@ -112,6 +119,7 @@ static VhostUserGpuMsg m __attribute__ ((unused));
 #define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
 
 #define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+#define VHOST_USER_GPU_PROTOCOL_F_DMABUF2 1
 
 struct virtio_gpu_scanout {
     uint32_t width, height;
@@ -132,6 +140,7 @@ typedef struct VuGpu {
     bool virgl;
     bool virgl_inited;
     bool edid_inited;
+    bool use_modifiers;
     uint32_t inflight;
 
     struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];
-- 
2.40.1

[PATCH 3/3] vhost-user-gpu: support dmabuf modifiers

[Erico Nunes]

When the backend sends VHOST_USER_GPU_DMABUF_SCANOUT2, handle it
by getting the modifiers information which is now available.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 hw/display/vhost-user-gpu.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index e8ee03094e..1150521d9d 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -32,6 +32,7 @@ typedef enum VhostUserGpuRequest {
     VHOST_USER_GPU_DMABUF_SCANOUT,
     VHOST_USER_GPU_DMABUF_UPDATE,
     VHOST_USER_GPU_GET_EDID,
+    VHOST_USER_GPU_DMABUF_SCANOUT2,
 } VhostUserGpuRequest;
 
 typedef struct VhostUserGpuDisplayInfoReply {
@@ -79,6 +80,11 @@ typedef struct VhostUserGpuDMABUFScanout {
     int fd_drm_fourcc;
 } QEMU_PACKED VhostUserGpuDMABUFScanout;
 
+typedef struct VhostUserGpuDMABUFScanout2 {
+    struct VhostUserGpuDMABUFScanout dmabuf_scanout;
+    uint64_t modifier;
+} QEMU_PACKED VhostUserGpuDMABUFScanout2;
+
 typedef struct VhostUserGpuEdidRequest {
     uint32_t scanout_id;
 } QEMU_PACKED VhostUserGpuEdidRequest;
@@ -93,6 +99,7 @@ typedef struct VhostUserGpuMsg {
         VhostUserGpuScanout scanout;
         VhostUserGpuUpdate update;
         VhostUserGpuDMABUFScanout dmabuf_scanout;
+        VhostUserGpuDMABUFScanout2 dmabuf_scanout2;
         VhostUserGpuEdidRequest edid_req;
         struct virtio_gpu_resp_edid resp_edid;
         struct virtio_gpu_resp_display_info display_info;
@@ -107,6 +114,7 @@ static VhostUserGpuMsg m __attribute__ ((unused));
 #define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
 
 #define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+#define VHOST_USER_GPU_PROTOCOL_F_DMABUF2 1
 
 static void vhost_user_gpu_update_blocked(VhostUserGPU *g, bool blocked);
 
@@ -171,7 +179,8 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
             .flags = VHOST_USER_GPU_MSG_FLAG_REPLY,
             .size = sizeof(uint64_t),
             .payload = {
-                .u64 = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID)
+                .u64 = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID) |
+                       (1 << VHOST_USER_GPU_PROTOCOL_F_DMABUF2)
             }
         };
 
@@ -236,6 +245,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
 
         break;
     }
+    case VHOST_USER_GPU_DMABUF_SCANOUT2:
     case VHOST_USER_GPU_DMABUF_SCANOUT: {
         VhostUserGpuDMABUFScanout *m = &msg->payload.dmabuf_scanout;
         int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
@@ -269,6 +279,11 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
             .fourcc = m->fd_drm_fourcc,
             .y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
         };
+        if (msg->request == VHOST_USER_GPU_DMABUF_SCANOUT2) {
+            VhostUserGpuDMABUFScanout2 *m2 = &msg->payload.dmabuf_scanout2;
+            dmabuf->modifier = m2->modifier;
+        }
+
         dpy_gl_scanout_dmabuf(con, dmabuf);
         break;
     }
-- 
2.40.1

Re: [PATCH 0/3] vhost-user-gpu: support dmabuf modifiers

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

Hi

On Fri, Jul 14, 2023 at 7:42 PM Erico Nunes <ernunes@redhat.com> wrote:

> virglrenderer recently added virgl_renderer_resource_get_info_ext as a
> new api, which gets resource information, including dmabuf modifiers.
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024
>
> We have to support dmabuf modifiers since the driver may choose to
> allocate buffers with these modifiers to improve performance, and
> importing buffers without modifiers information may result in completely
> broken rendering.
>
> Currently trying to use vhost-user-gpu for rendering backend and using
> the qemu dbus ui as a ui backend results in a broken framebuffer with
> Intel GPUs as the buffer is allocated with a modifier. With this
> patchset, that is fixed.
>
>
> It is tricky to support since it requires to keep compatibility at the
> same time with:
> (1) build against older virglrenderer which do not provide
> virgl_renderer_resource_get_info_ext;
> (2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to
> increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT
> message.
>
> I tried to reach a compromise here by not defining a completely new
> message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels
> like a bit of a hack, so I appreciate feedback if there is a better way
> (or naming) to handle it.
>

looks fine to me, we may consider this as a fix for 8.1 imho
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


>
>
> Erico Nunes (3):
>   docs: vhost-user-gpu: add protocol changes for dmabuf modifiers
>   contrib/vhost-user-gpu: add support for sending dmabuf modifiers
>   vhost-user-gpu: support dmabuf modifiers
>
>  contrib/vhost-user-gpu/vhost-user-gpu.c |  5 ++-
>  contrib/vhost-user-gpu/virgl.c          | 51 +++++++++++++++++++++++--
>  contrib/vhost-user-gpu/vugpu.h          |  9 +++++
>  docs/interop/vhost-user-gpu.rst         | 26 ++++++++++++-
>  hw/display/vhost-user-gpu.c             | 17 ++++++++-
>  5 files changed, 102 insertions(+), 6 deletions(-)
>
> --
> 2.40.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 3062 bytes --]

Re: [PATCH 0/3] vhost-user-gpu: support dmabuf modifiers

[Sergio Lopez]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On Fri, Jul 14, 2023 at 05:38:57PM +0200, Erico Nunes wrote:
> virglrenderer recently added virgl_renderer_resource_get_info_ext as a
> new api, which gets resource information, including dmabuf modifiers.
> https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024
> 
> We have to support dmabuf modifiers since the driver may choose to
> allocate buffers with these modifiers to improve performance, and
> importing buffers without modifiers information may result in completely
> broken rendering.
> 
> Currently trying to use vhost-user-gpu for rendering backend and using
> the qemu dbus ui as a ui backend results in a broken framebuffer with
> Intel GPUs as the buffer is allocated with a modifier. With this
> patchset, that is fixed.
> 
> 
> It is tricky to support since it requires to keep compatibility at the
> same time with:
> (1) build against older virglrenderer which do not provide
> virgl_renderer_resource_get_info_ext;
> (2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to
> increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT
> message.
> 
> I tried to reach a compromise here by not defining a completely new
> message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels
> like a bit of a hack, so I appreciate feedback if there is a better way
> (or naming) to handle it.
> 
> 
> Erico Nunes (3):
>   docs: vhost-user-gpu: add protocol changes for dmabuf modifiers
>   contrib/vhost-user-gpu: add support for sending dmabuf modifiers
>   vhost-user-gpu: support dmabuf modifiers
> 
>  contrib/vhost-user-gpu/vhost-user-gpu.c |  5 ++-
>  contrib/vhost-user-gpu/virgl.c          | 51 +++++++++++++++++++++++--
>  contrib/vhost-user-gpu/vugpu.h          |  9 +++++
>  docs/interop/vhost-user-gpu.rst         | 26 ++++++++++++-
>  hw/display/vhost-user-gpu.c             | 17 ++++++++-
>  5 files changed, 102 insertions(+), 6 deletions(-)

Series:

Reviewed-by: Sergio Lopez <slp@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] vhost-user-gpu: support dmabuf modifiers

[Erico Nunes]

Hello,

On 14/07/2023 22:03, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Jul 14, 2023 at 7:42 PM Erico Nunes <ernunes@redhat.com
> <mailto:ernunes@redhat.com>> wrote:
> 
>     virglrenderer recently added virgl_renderer_resource_get_info_ext as a
>     new api, which gets resource information, including dmabuf modifiers.
>     https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024 <https://gitlab.freedesktop.org/virgl/virglrenderer/-/merge_requests/1024>
> 
>     We have to support dmabuf modifiers since the driver may choose to
>     allocate buffers with these modifiers to improve performance, and
>     importing buffers without modifiers information may result in completely
>     broken rendering.
> 
>     Currently trying to use vhost-user-gpu for rendering backend and using
>     the qemu dbus ui as a ui backend results in a broken framebuffer with
>     Intel GPUs as the buffer is allocated with a modifier. With this
>     patchset, that is fixed.
> 
> 
>     It is tricky to support since it requires to keep compatibility at the
>     same time with:
>     (1) build against older virglrenderer which do not provide
>     virgl_renderer_resource_get_info_ext;
>     (2) runtime between frontend (qemu) and backend (vhost-user-gpu) due to
>     increased size and a new field in the VHOST_USER_GPU_DMABUF_SCANOUT
>     message.
> 
>     I tried to reach a compromise here by not defining a completely new
>     message and duplicate VHOST_USER_GPU_DMABUF_SCANOUT but it still feels
>     like a bit of a hack, so I appreciate feedback if there is a better way
>     (or naming) to handle it.
> 
> 
> looks fine to me, we may consider this as a fix for 8.1 imho
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com
> <mailto:marcandre.lureau@redhat.com>>
Just making sure this one didn't fall through the cracks; should I do
something about this series or is it set to be in an upcoming merge?

Thanks

Erico