[PATCH v2 0/4] vhost-user-gpu get_edid feature

[PATCH v2 0/4] vhost-user-gpu get_edid feature

[Erico Nunes]

This adds support to the virtio-gpu get_edid command in qemu as the
frontend as well as the vhost-user-gpu implementation in contrib/.
So far, qemu has been outputting the following message:
  EDID requested but the backend doesn't support it.
when using that implementation.

This is tested with vhost-user-gpu, the dbus ui backend and the
monitor-edid application, which now shows complete "QEMU Monitor" edid
data.

The VHOST_USER_GPU_PROTOCOL_F_EDID protocol feature is required if the
frontend sets the VIRTIO_GPU_F_EDID virtio-gpu feature. If the frontend
sets the virtio-gpu feature but does not support the protocol feature,
the backend will abort with an error.

Erico Nunes (4):
  virtio-gpu: refactor generate_edid function to virtio_gpu_base
  docs: vhost-user-gpu: add protocol changes for EDID
  contrib/vhost-user-gpu: implement get_edid feature
  vhost-user-gpu: implement get_edid frontend feature

 contrib/vhost-user-gpu/vhost-user-gpu.c | 67 +++++++++++++++++++++++--
 contrib/vhost-user-gpu/virgl.c          |  3 ++
 contrib/vhost-user-gpu/vugpu.h          | 11 ++++
 docs/interop/vhost-user-gpu.rst         | 18 +++++--
 hw/display/vhost-user-gpu.c             | 36 +++++++++++++
 hw/display/virtio-gpu-base.c            | 17 +++++++
 hw/display/virtio-gpu.c                 | 20 +-------
 include/hw/virtio/virtio-gpu.h          |  2 +
 8 files changed, 148 insertions(+), 26 deletions(-)

-- 
2.40.1

[PATCH v2 1/4] virtio-gpu: refactor generate_edid function to virtio_gpu_base

[Erico Nunes]

This functionality can be shared with upcoming use in vhost-user-gpu, so
move it to the shared file to avoid duplicating it.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 hw/display/virtio-gpu-base.c   | 17 +++++++++++++++++
 hw/display/virtio-gpu.c        | 20 +-------------------
 include/hw/virtio/virtio-gpu.h |  2 ++
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index a29f191aa8..7ab7d08d0a 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -17,6 +17,7 @@
 #include "migration/blocker.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
+#include "hw/display/edid.h"
 #include "trace.h"
 
 void
@@ -51,6 +52,22 @@ virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
     }
 }
 
+void
+virtio_gpu_base_generate_edid(VirtIOGPUBase *g, int scanout,
+                              struct virtio_gpu_resp_edid *edid)
+{
+    qemu_edid_info info = {
+        .width_mm = g->req_state[scanout].width_mm,
+        .height_mm = g->req_state[scanout].height_mm,
+        .prefx = g->req_state[scanout].width,
+        .prefy = g->req_state[scanout].height,
+        .refresh_rate = g->req_state[scanout].refresh_rate,
+    };
+
+    edid->size = cpu_to_le32(sizeof(edid->edid));
+    qemu_edid_generate(edid->edid, sizeof(edid->edid), &info);
+}
+
 static void virtio_gpu_invalidate_display(void *opaque)
 {
 }
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 66cddd94d9..9ff1a43353 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -24,7 +24,6 @@
 #include "hw/virtio/virtio-gpu-bswap.h"
 #include "hw/virtio/virtio-gpu-pixman.h"
 #include "hw/virtio/virtio-bus.h"
-#include "hw/display/edid.h"
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
@@ -207,23 +206,6 @@ void virtio_gpu_get_display_info(VirtIOGPU *g,
                              sizeof(display_info));
 }
 
-static void
-virtio_gpu_generate_edid(VirtIOGPU *g, int scanout,
-                         struct virtio_gpu_resp_edid *edid)
-{
-    VirtIOGPUBase *b = VIRTIO_GPU_BASE(g);
-    qemu_edid_info info = {
-        .width_mm = b->req_state[scanout].width_mm,
-        .height_mm = b->req_state[scanout].height_mm,
-        .prefx = b->req_state[scanout].width,
-        .prefy = b->req_state[scanout].height,
-        .refresh_rate = b->req_state[scanout].refresh_rate,
-    };
-
-    edid->size = cpu_to_le32(sizeof(edid->edid));
-    qemu_edid_generate(edid->edid, sizeof(edid->edid), &info);
-}
-
 void virtio_gpu_get_edid(VirtIOGPU *g,
                          struct virtio_gpu_ctrl_command *cmd)
 {
@@ -242,7 +224,7 @@ void virtio_gpu_get_edid(VirtIOGPU *g,
     trace_virtio_gpu_cmd_get_edid(get_edid.scanout);
     memset(&edid, 0, sizeof(edid));
     edid.hdr.type = VIRTIO_GPU_RESP_OK_EDID;
-    virtio_gpu_generate_edid(g, get_edid.scanout, &edid);
+    virtio_gpu_base_generate_edid(VIRTIO_GPU_BASE(g), get_edid.scanout, &edid);
     virtio_gpu_ctrl_response(g, cmd, &edid.hdr, sizeof(edid));
 }
 
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 2e28507efe..39c1a6a6ea 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -239,6 +239,8 @@ void virtio_gpu_base_reset(VirtIOGPUBase *g);
 void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
                         struct virtio_gpu_resp_display_info *dpy_info);
 
+void virtio_gpu_base_generate_edid(VirtIOGPUBase *g, int scanout,
+                                   struct virtio_gpu_resp_edid *edid);
 /* virtio-gpu.c */
 void virtio_gpu_ctrl_response(VirtIOGPU *g,
                               struct virtio_gpu_ctrl_command *cmd,
-- 
2.40.1

[PATCH v2 2/4] docs: vhost-user-gpu: add protocol changes for EDID

[Erico Nunes]

VHOST_USER_GPU_GET_EDID is defined as a message from the backend to the
frontend to retrieve the EDID data for a given scanout.

The VHOST_USER_GPU_PROTOCOL_F_EDID 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 | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/docs/interop/vhost-user-gpu.rst b/docs/interop/vhost-user-gpu.rst
index 1640553729..ab86391aee 100644
--- a/docs/interop/vhost-user-gpu.rst
+++ b/docs/interop/vhost-user-gpu.rst
@@ -141,6 +141,8 @@ In QEMU the vhost-user-gpu message is implemented with the following struct:
           VhostUserGpuScanout scanout;
           VhostUserGpuUpdate update;
           VhostUserGpuDMABUFScanout dmabuf_scanout;
+          VhostUserGpuEdidRequest edid_req;
+          struct virtio_gpu_resp_edid resp_edid;
           struct virtio_gpu_resp_display_info display_info;
           uint64_t u64;
       } payload;
@@ -149,10 +151,11 @@ In QEMU the vhost-user-gpu message is implemented with the following struct:
 Protocol features
 -----------------
 
-None yet.
+.. code:: c
+
+  #define VHOST_USER_GPU_PROTOCOL_F_EDID 0
 
-As the protocol may need to evolve, new messages and communication
-changes are negotiated thanks to preliminary
+New messages and communication changes are negotiated thanks to the
 ``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` and
 ``VHOST_USER_GPU_SET_PROTOCOL_FEATURES`` requests.
 
@@ -241,3 +244,12 @@ Message types
   Note: there is no data payload, since the scanout is shared thanks
   to DMABUF, that must have been set previously with
   ``VHOST_USER_GPU_DMABUF_SCANOUT``.
+
+``VHOST_USER_GPU_GET_EDID``
+  :id: 11
+  :request payload: ``struct VhostUserGpuEdidRequest``
+  :reply payload: ``struct virtio_gpu_resp_edid`` (from virtio specification)
+
+  Retrieve the EDID data for a given scanout.
+  This message requires the ``VHOST_USER_GPU_PROTOCOL_F_EDID`` protocol
+  feature to be supported.
-- 
2.40.1

[PATCH v2 3/4] contrib/vhost-user-gpu: implement get_edid feature

[Erico Nunes]

Implement the virtio-gpu feature in contrib/vhost-user-gpu, which was
unsupported until now.
In this implementation, the feature is enabled inconditionally to avoid
creating another optional config argument.
Similarly to get_display_info, vhost-user-gpu sends a message back to
the frontend to have access to all the display information. In the
case of get_edid, it also needs to pass which scanout we should
retrieve the edid for.

The VHOST_USER_GPU_PROTOCOL_F_EDID protocol feature is required if the
frontend sets the VIRTIO_GPU_F_EDID virtio-gpu feature. If the frontend
sets the virtio-gpu feature but does not support the protocol feature,
the backend will abort with an error.

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

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index bfb8d93cf8..8035dc71d7 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -303,6 +303,53 @@ vg_get_display_info(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd)
     cmd->state = VG_CMD_STATE_PENDING;
 }
 
+static gboolean
+get_edid_cb(gint fd, GIOCondition condition, gpointer user_data)
+{
+    struct virtio_gpu_resp_edid resp_edid;
+    VuGpu *vg = user_data;
+    struct virtio_gpu_ctrl_command *cmd = QTAILQ_LAST(&vg->fenceq);
+
+    g_debug("get edid cb");
+    assert(cmd->cmd_hdr.type == VIRTIO_GPU_CMD_GET_EDID);
+    if (!vg_recv_msg(vg, VHOST_USER_GPU_GET_EDID,
+                     sizeof(resp_edid), &resp_edid)) {
+        return G_SOURCE_CONTINUE;
+    }
+
+    QTAILQ_REMOVE(&vg->fenceq, cmd, next);
+    vg_ctrl_response(vg, cmd, &resp_edid.hdr, sizeof(resp_edid));
+
+    vg->wait_in = 0;
+    vg_handle_ctrl(&vg->dev.parent, 0);
+
+    return G_SOURCE_REMOVE;
+}
+
+void
+vg_get_edid(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd)
+{
+    struct virtio_gpu_cmd_get_edid get_edid;
+
+    VUGPU_FILL_CMD(get_edid);
+    virtio_gpu_bswap_32(&get_edid, sizeof(get_edid));
+
+    VhostUserGpuMsg msg = {
+        .request = VHOST_USER_GPU_GET_EDID,
+        .size = sizeof(VhostUserGpuEdidRequest),
+        .payload.edid_req = {
+                .scanout_id = get_edid.scanout,
+        },
+    };
+
+    assert(vg->wait_in == 0);
+
+    vg_send_msg(vg, &msg, -1);
+    vg->wait_in = g_unix_fd_add(vg->sock_fd, G_IO_IN | G_IO_HUP,
+                               get_edid_cb, vg);
+    cmd->state = VG_CMD_STATE_PENDING;
+}
+
 static void
 vg_resource_create_2d(VuGpu *g,
                       struct virtio_gpu_ctrl_command *cmd)
@@ -837,8 +884,9 @@ vg_process_cmd(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd)
     case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
         vg_resource_detach_backing(vg, cmd);
         break;
-    /* case VIRTIO_GPU_CMD_GET_EDID: */
-    /*     break */
+    case VIRTIO_GPU_CMD_GET_EDID:
+        vg_get_edid(vg, cmd);
+        break;
     default:
         g_warning("TODO handle ctrl %x\n", cmd->cmd_hdr.type);
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
@@ -1022,6 +1070,7 @@ vg_queue_set_started(VuDev *dev, int qidx, bool started)
 static gboolean
 protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
 {
+    const uint64_t protocol_edid = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID);
     VuGpu *g = user_data;
     uint64_t u64;
     VhostUserGpuMsg msg = {
@@ -1035,13 +1084,19 @@ protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
     msg = (VhostUserGpuMsg) {
         .request = VHOST_USER_GPU_SET_PROTOCOL_FEATURES,
         .size = sizeof(uint64_t),
-        .payload.u64 = 0
+        .payload.u64 = protocol_edid,
     };
     vg_send_msg(g, &msg, -1);
 
     g->wait_in = 0;
     vg_handle_ctrl(&g->dev.parent, 0);
 
+    if (g->edid_inited && !(u64 & protocol_edid)) {
+        g_printerr("EDID feature set by the frontend but it does not support "
+                   "the EDID vhost-user-gpu protocol.\n");
+        exit(EXIT_FAILURE);
+    }
+
     return G_SOURCE_REMOVE;
 }
 
@@ -1049,7 +1104,8 @@ static void
 set_gpu_protocol_features(VuGpu *g)
 {
     VhostUserGpuMsg msg = {
-        .request = VHOST_USER_GPU_GET_PROTOCOL_FEATURES
+        .request = VHOST_USER_GPU_GET_PROTOCOL_FEATURES,
+        .size = sizeof(uint64_t),
     };
 
     vg_send_msg(g, &msg, -1);
@@ -1086,6 +1142,7 @@ vg_get_features(VuDev *dev)
     if (opt_virgl) {
         features |= 1 << VIRTIO_GPU_F_VIRGL;
     }
+    features |= 1 << VIRTIO_GPU_F_EDID;
 
     return features;
 }
@@ -1103,6 +1160,8 @@ vg_set_features(VuDev *dev, uint64_t features)
         g->virgl_inited = true;
     }
 
+    g->edid_inited = !!(features & (1 << VIRTIO_GPU_F_EDID));
+
     g->virgl = virgl;
 }
 
diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 3e45e1bd33..211aa110a9 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -495,6 +495,9 @@ void vg_virgl_process_cmd(VuGpu *g, struct virtio_gpu_ctrl_command *cmd)
     case VIRTIO_GPU_CMD_GET_DISPLAY_INFO:
         vg_get_display_info(g, cmd);
         break;
+    case VIRTIO_GPU_CMD_GET_EDID:
+        vg_get_edid(g, cmd);
+        break;
     default:
         g_debug("TODO handle ctrl %x\n", cmd->cmd_hdr.type);
         cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
diff --git a/contrib/vhost-user-gpu/vugpu.h b/contrib/vhost-user-gpu/vugpu.h
index e2864bba68..f0f2069c47 100644
--- a/contrib/vhost-user-gpu/vugpu.h
+++ b/contrib/vhost-user-gpu/vugpu.h
@@ -36,6 +36,7 @@ typedef enum VhostUserGpuRequest {
     VHOST_USER_GPU_UPDATE,
     VHOST_USER_GPU_DMABUF_SCANOUT,
     VHOST_USER_GPU_DMABUF_UPDATE,
+    VHOST_USER_GPU_GET_EDID,
 } VhostUserGpuRequest;
 
 typedef struct VhostUserGpuDisplayInfoReply {
@@ -83,6 +84,10 @@ typedef struct VhostUserGpuDMABUFScanout {
     int fd_drm_fourcc;
 } QEMU_PACKED VhostUserGpuDMABUFScanout;
 
+typedef struct VhostUserGpuEdidRequest {
+    uint32_t scanout_id;
+} QEMU_PACKED VhostUserGpuEdidRequest;
+
 typedef struct VhostUserGpuMsg {
     uint32_t request; /* VhostUserGpuRequest */
     uint32_t flags;
@@ -93,6 +98,8 @@ typedef struct VhostUserGpuMsg {
         VhostUserGpuScanout scanout;
         VhostUserGpuUpdate update;
         VhostUserGpuDMABUFScanout dmabuf_scanout;
+        VhostUserGpuEdidRequest edid_req;
+        struct virtio_gpu_resp_edid resp_edid;
         struct virtio_gpu_resp_display_info display_info;
         uint64_t u64;
     } payload;
@@ -104,6 +111,8 @@ static VhostUserGpuMsg m __attribute__ ((unused));
 
 #define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
 
+#define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+
 struct virtio_gpu_scanout {
     uint32_t width, height;
     int x, y;
@@ -122,6 +131,7 @@ typedef struct VuGpu {
 
     bool virgl;
     bool virgl_inited;
+    bool edid_inited;
     uint32_t inflight;
 
     struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];
@@ -171,6 +181,7 @@ int     vg_create_mapping_iov(VuGpu *g,
                               struct iovec **iov);
 void    vg_cleanup_mapping_iov(VuGpu *g, struct iovec *iov, uint32_t count);
 void    vg_get_display_info(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd);
+void    vg_get_edid(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd);
 
 void    vg_wait_ok(VuGpu *g);
 
-- 
2.40.1

[PATCH v2 4/4] vhost-user-gpu: implement get_edid frontend feature

[Erico Nunes]

Implement the frontend side of the get_edid feature in the qemu
vhost-user-gpu frontend device.

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

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 71dfd956b8..184e57dfc7 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -31,6 +31,7 @@ typedef enum VhostUserGpuRequest {
     VHOST_USER_GPU_UPDATE,
     VHOST_USER_GPU_DMABUF_SCANOUT,
     VHOST_USER_GPU_DMABUF_UPDATE,
+    VHOST_USER_GPU_GET_EDID,
 } VhostUserGpuRequest;
 
 typedef struct VhostUserGpuDisplayInfoReply {
@@ -78,6 +79,10 @@ typedef struct VhostUserGpuDMABUFScanout {
     int fd_drm_fourcc;
 } QEMU_PACKED VhostUserGpuDMABUFScanout;
 
+typedef struct VhostUserGpuEdidRequest {
+    uint32_t scanout_id;
+} QEMU_PACKED VhostUserGpuEdidRequest;
+
 typedef struct VhostUserGpuMsg {
     uint32_t request; /* VhostUserGpuRequest */
     uint32_t flags;
@@ -88,6 +93,8 @@ typedef struct VhostUserGpuMsg {
         VhostUserGpuScanout scanout;
         VhostUserGpuUpdate update;
         VhostUserGpuDMABUFScanout dmabuf_scanout;
+        VhostUserGpuEdidRequest edid_req;
+        struct virtio_gpu_resp_edid resp_edid;
         struct virtio_gpu_resp_display_info display_info;
         uint64_t u64;
     } payload;
@@ -99,6 +106,8 @@ static VhostUserGpuMsg m __attribute__ ((unused));
 
 #define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
 
+#define VHOST_USER_GPU_PROTOCOL_F_EDID 0
+
 static void vhost_user_gpu_update_blocked(VhostUserGPU *g, bool blocked);
 
 static void
@@ -161,6 +170,9 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
             .request = msg->request,
             .flags = VHOST_USER_GPU_MSG_FLAG_REPLY,
             .size = sizeof(uint64_t),
+            .payload = {
+                .u64 = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID)
+            }
         };
 
         vhost_user_gpu_send_msg(g, &reply);
@@ -184,6 +196,30 @@ vhost_user_gpu_handle_display(VhostUserGPU *g, VhostUserGpuMsg *msg)
         vhost_user_gpu_send_msg(g, &reply);
         break;
     }
+    case VHOST_USER_GPU_GET_EDID: {
+        VhostUserGpuEdidRequest *m = &msg->payload.edid_req;
+        struct virtio_gpu_resp_edid resp = { {} };
+        int fd = qemu_chr_fe_get_msgfd(&g->vhost_chr);
+        VhostUserGpuMsg reply = {
+            .request = msg->request,
+            .flags = VHOST_USER_GPU_MSG_FLAG_REPLY,
+            .size = sizeof(reply.payload.resp_edid),
+        };
+
+        if (m->scanout_id >= g->parent_obj.conf.max_outputs) {
+            error_report("invalid scanout: %d", m->scanout_id);
+            if (fd >= 0) {
+                close(fd);
+            }
+            break;
+        }
+
+        resp.hdr.type = VIRTIO_GPU_RESP_OK_EDID;
+        virtio_gpu_base_generate_edid(VIRTIO_GPU_BASE(g), m->scanout_id, &resp);
+        memcpy(&reply.payload.resp_edid, &resp, sizeof(resp));
+        vhost_user_gpu_send_msg(g, &reply);
+        break;
+    }
     case VHOST_USER_GPU_SCANOUT: {
         VhostUserGpuScanout *m = &msg->payload.scanout;
 
-- 
2.40.1

Re: [PATCH v2 2/4] docs: vhost-user-gpu: add protocol changes for EDID

[Marc-André Lureau]

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

Hi

On Wed, May 31, 2023 at 8:08 PM Erico Nunes <ernunes@redhat.com> wrote:

> VHOST_USER_GPU_GET_EDID is defined as a message from the backend to the
> frontend to retrieve the EDID data for a given scanout.
>
> The VHOST_USER_GPU_PROTOCOL_F_EDID 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 | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/docs/interop/vhost-user-gpu.rst
> b/docs/interop/vhost-user-gpu.rst
> index 1640553729..ab86391aee 100644
> --- a/docs/interop/vhost-user-gpu.rst
> +++ b/docs/interop/vhost-user-gpu.rst
> @@ -141,6 +141,8 @@ In QEMU the vhost-user-gpu message is implemented with
> the following struct:
>            VhostUserGpuScanout scanout;
>            VhostUserGpuUpdate update;
>            VhostUserGpuDMABUFScanout dmabuf_scanout;
> +          VhostUserGpuEdidRequest edid_req;
>

Please add  VhostUserGpuEdidRequest payload description in "Payload types"
section

+          struct virtio_gpu_resp_edid resp_edid;
>            struct virtio_gpu_resp_display_info display_info;
>            uint64_t u64;
>        } payload;
> @@ -149,10 +151,11 @@ In QEMU the vhost-user-gpu message is implemented
> with the following struct:
>  Protocol features
>  -----------------
>
> -None yet.
> +.. code:: c
> +
> +  #define VHOST_USER_GPU_PROTOCOL_F_EDID 0
>
> -As the protocol may need to evolve, new messages and communication
> -changes are negotiated thanks to preliminary
> +New messages and communication changes are negotiated thanks to the
>  ``VHOST_USER_GPU_GET_PROTOCOL_FEATURES`` and
>  ``VHOST_USER_GPU_SET_PROTOCOL_FEATURES`` requests.
>
> @@ -241,3 +244,12 @@ Message types
>    Note: there is no data payload, since the scanout is shared thanks
>    to DMABUF, that must have been set previously with
>    ``VHOST_USER_GPU_DMABUF_SCANOUT``.
> +
> +``VHOST_USER_GPU_GET_EDID``
> +  :id: 11
> +  :request payload: ``struct VhostUserGpuEdidRequest``
> +  :reply payload: ``struct virtio_gpu_resp_edid`` (from virtio
> specification)
> +
> +  Retrieve the EDID data for a given scanout.
> +  This message requires the ``VHOST_USER_GPU_PROTOCOL_F_EDID`` protocol
> +  feature to be supported.
> --
> 2.40.1
>
>
>
thanks

-- 
Marc-André Lureau

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

Re: [PATCH v2 3/4] contrib/vhost-user-gpu: implement get_edid feature

[Marc-André Lureau]

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

Hi

On Wed, May 31, 2023 at 8:07 PM Erico Nunes <ernunes@redhat.com> wrote:

> Implement the virtio-gpu feature in contrib/vhost-user-gpu, which was
> unsupported until now.
> In this implementation, the feature is enabled inconditionally to avoid
> creating another optional config argument.
> Similarly to get_display_info, vhost-user-gpu sends a message back to
> the frontend to have access to all the display information. In the
> case of get_edid, it also needs to pass which scanout we should
> retrieve the edid for.
>
> The VHOST_USER_GPU_PROTOCOL_F_EDID protocol feature is required if the
> frontend sets the VIRTIO_GPU_F_EDID virtio-gpu feature. If the frontend
> sets the virtio-gpu feature but does not support the protocol feature,
> the backend will abort with an error.
>
> Signed-off-by: Erico Nunes <ernunes@redhat.com>
> ---
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 67 +++++++++++++++++++++++--
>  contrib/vhost-user-gpu/virgl.c          |  3 ++
>  contrib/vhost-user-gpu/vugpu.h          | 11 ++++
>  3 files changed, 77 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index bfb8d93cf8..8035dc71d7 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -303,6 +303,53 @@ vg_get_display_info(VuGpu *vg, struct
> virtio_gpu_ctrl_command *cmd)
>      cmd->state = VG_CMD_STATE_PENDING;
>  }
>
> +static gboolean
> +get_edid_cb(gint fd, GIOCondition condition, gpointer user_data)
> +{
> +    struct virtio_gpu_resp_edid resp_edid;
> +    VuGpu *vg = user_data;
> +    struct virtio_gpu_ctrl_command *cmd = QTAILQ_LAST(&vg->fenceq);
> +
> +    g_debug("get edid cb");
> +    assert(cmd->cmd_hdr.type == VIRTIO_GPU_CMD_GET_EDID);
> +    if (!vg_recv_msg(vg, VHOST_USER_GPU_GET_EDID,
> +                     sizeof(resp_edid), &resp_edid)) {
> +        return G_SOURCE_CONTINUE;
> +    }
> +
> +    QTAILQ_REMOVE(&vg->fenceq, cmd, next);
> +    vg_ctrl_response(vg, cmd, &resp_edid.hdr, sizeof(resp_edid));
> +
> +    vg->wait_in = 0;
> +    vg_handle_ctrl(&vg->dev.parent, 0);
> +
> +    return G_SOURCE_REMOVE;
> +}
> +
> +void
> +vg_get_edid(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_cmd_get_edid get_edid;
> +
> +    VUGPU_FILL_CMD(get_edid);
> +    virtio_gpu_bswap_32(&get_edid, sizeof(get_edid));
> +
> +    VhostUserGpuMsg msg = {
> +        .request = VHOST_USER_GPU_GET_EDID,
> +        .size = sizeof(VhostUserGpuEdidRequest),
> +        .payload.edid_req = {
> +                .scanout_id = get_edid.scanout,
> +        },
> +    };
> +
> +    assert(vg->wait_in == 0);
> +
> +    vg_send_msg(vg, &msg, -1);
> +    vg->wait_in = g_unix_fd_add(vg->sock_fd, G_IO_IN | G_IO_HUP,
> +                               get_edid_cb, vg);
> +    cmd->state = VG_CMD_STATE_PENDING;
> +}
> +
>  static void
>  vg_resource_create_2d(VuGpu *g,
>                        struct virtio_gpu_ctrl_command *cmd)
> @@ -837,8 +884,9 @@ vg_process_cmd(VuGpu *vg, struct
> virtio_gpu_ctrl_command *cmd)
>      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>          vg_resource_detach_backing(vg, cmd);
>          break;
> -    /* case VIRTIO_GPU_CMD_GET_EDID: */
> -    /*     break */
> +    case VIRTIO_GPU_CMD_GET_EDID:
> +        vg_get_edid(vg, cmd);
> +        break;
>      default:
>          g_warning("TODO handle ctrl %x\n", cmd->cmd_hdr.type);
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> @@ -1022,6 +1070,7 @@ vg_queue_set_started(VuDev *dev, int qidx, bool
> started)
>  static gboolean
>  protocol_features_cb(gint fd, GIOCondition condition, gpointer user_data)
>  {
> +    const uint64_t protocol_edid = (1 << VHOST_USER_GPU_PROTOCOL_F_EDID);
>      VuGpu *g = user_data;
>      uint64_t u64;
>      VhostUserGpuMsg msg = {
> @@ -1035,13 +1084,19 @@ protocol_features_cb(gint fd, GIOCondition
> condition, gpointer user_data)
>      msg = (VhostUserGpuMsg) {
>          .request = VHOST_USER_GPU_SET_PROTOCOL_FEATURES,
>          .size = sizeof(uint64_t),
> -        .payload.u64 = 0
> +        .payload.u64 = protocol_edid,
>


You should not set flags unsupported by the frontend (qemu). Use
"protocol_edid & u64". It might be worth renaming the "u64" variable too,
to "protocol_features" for ex. You could reuse that variable and udate with
"protocol_features &= (1 << VHOST_USER_GPU_PROTOCOL_F_EDID);"


>      };
>      vg_send_msg(g, &msg, -1);
>
>      g->wait_in = 0;
>      vg_handle_ctrl(&g->dev.parent, 0);
>
> +    if (g->edid_inited && !(u64 & protocol_edid)) {
> +        g_printerr("EDID feature set by the frontend but it does not
> support "
> +                   "the EDID vhost-user-gpu protocol.\n");
> +        exit(EXIT_FAILURE);
> +    }
> +
>      return G_SOURCE_REMOVE;
>  }
>
> @@ -1049,7 +1104,8 @@ static void
>  set_gpu_protocol_features(VuGpu *g)
>  {
>      VhostUserGpuMsg msg = {
> -        .request = VHOST_USER_GPU_GET_PROTOCOL_FEATURES
> +        .request = VHOST_USER_GPU_GET_PROTOCOL_FEATURES,
> +        .size = sizeof(uint64_t),
>

Why? there is no payload


>      };
>
>      vg_send_msg(g, &msg, -1);
> @@ -1086,6 +1142,7 @@ vg_get_features(VuDev *dev)
>      if (opt_virgl) {
>          features |= 1 << VIRTIO_GPU_F_VIRGL;
>      }
> +    features |= 1 << VIRTIO_GPU_F_EDID;
>

>      return features;
>  }
> @@ -1103,6 +1160,8 @@ vg_set_features(VuDev *dev, uint64_t features)
>          g->virgl_inited = true;
>      }
>
> +    g->edid_inited = !!(features & (1 << VIRTIO_GPU_F_EDID));
> +
>      g->virgl = virgl;
>  }
>
> diff --git a/contrib/vhost-user-gpu/virgl.c
> b/contrib/vhost-user-gpu/virgl.c
> index 3e45e1bd33..211aa110a9 100644
> --- a/contrib/vhost-user-gpu/virgl.c
> +++ b/contrib/vhost-user-gpu/virgl.c
> @@ -495,6 +495,9 @@ void vg_virgl_process_cmd(VuGpu *g, struct
> virtio_gpu_ctrl_command *cmd)
>      case VIRTIO_GPU_CMD_GET_DISPLAY_INFO:
>          vg_get_display_info(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_GET_EDID:
> +        vg_get_edid(g, cmd);
> +        break;
>      default:
>          g_debug("TODO handle ctrl %x\n", cmd->cmd_hdr.type);
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
> diff --git a/contrib/vhost-user-gpu/vugpu.h
> b/contrib/vhost-user-gpu/vugpu.h
> index e2864bba68..f0f2069c47 100644
> --- a/contrib/vhost-user-gpu/vugpu.h
> +++ b/contrib/vhost-user-gpu/vugpu.h
> @@ -36,6 +36,7 @@ typedef enum VhostUserGpuRequest {
>      VHOST_USER_GPU_UPDATE,
>      VHOST_USER_GPU_DMABUF_SCANOUT,
>      VHOST_USER_GPU_DMABUF_UPDATE,
> +    VHOST_USER_GPU_GET_EDID,
>  } VhostUserGpuRequest;
>
>  typedef struct VhostUserGpuDisplayInfoReply {
> @@ -83,6 +84,10 @@ typedef struct VhostUserGpuDMABUFScanout {
>      int fd_drm_fourcc;
>  } QEMU_PACKED VhostUserGpuDMABUFScanout;
>
> +typedef struct VhostUserGpuEdidRequest {
> +    uint32_t scanout_id;
> +} QEMU_PACKED VhostUserGpuEdidRequest;
> +
>  typedef struct VhostUserGpuMsg {
>      uint32_t request; /* VhostUserGpuRequest */
>      uint32_t flags;
> @@ -93,6 +98,8 @@ typedef struct VhostUserGpuMsg {
>          VhostUserGpuScanout scanout;
>          VhostUserGpuUpdate update;
>          VhostUserGpuDMABUFScanout dmabuf_scanout;
> +        VhostUserGpuEdidRequest edid_req;
> +        struct virtio_gpu_resp_edid resp_edid;
>          struct virtio_gpu_resp_display_info display_info;
>          uint64_t u64;
>      } payload;
> @@ -104,6 +111,8 @@ static VhostUserGpuMsg m __attribute__ ((unused));
>
>  #define VHOST_USER_GPU_MSG_FLAG_REPLY 0x4
>
> +#define VHOST_USER_GPU_PROTOCOL_F_EDID 0
> +
>  struct virtio_gpu_scanout {
>      uint32_t width, height;
>      int x, y;
> @@ -122,6 +131,7 @@ typedef struct VuGpu {
>
>      bool virgl;
>      bool virgl_inited;
> +    bool edid_inited;
>      uint32_t inflight;
>
>      struct virtio_gpu_scanout scanout[VIRTIO_GPU_MAX_SCANOUTS];
> @@ -171,6 +181,7 @@ int     vg_create_mapping_iov(VuGpu *g,
>                                struct iovec **iov);
>  void    vg_cleanup_mapping_iov(VuGpu *g, struct iovec *iov, uint32_t
> count);
>  void    vg_get_display_info(VuGpu *vg, struct virtio_gpu_ctrl_command
> *cmd);
> +void    vg_get_edid(VuGpu *vg, struct virtio_gpu_ctrl_command *cmd);
>
>  void    vg_wait_ok(VuGpu *g);
>
> --
> 2.40.1
>
>
>

-- 
Marc-André Lureau

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