[PATCH 0/4] WIP: add physical display dimensions to spice/virtio-gpu

[PATCH 0/4] WIP: add physical display dimensions to spice/virtio-gpu

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

In order to improve support for HiDPI, I proposed some new Spice messages to
inform the guest of the display physical dimensions.

See spice-protocol proposal and related server & spice-gtk changes:
https://gitlab.freedesktop.org/spice/spice-protocol/-/merge_requests/24
https://gitlab.freedesktop.org/spice/spice/-/merge_requests/139
https://gitlab.freedesktop.org/spice/spice-gtk/-/merge_requests/64

Marc-André Lureau (4):
  edid: use physical dimensions if available
  ui: add getter for UIInfo
  spice: get monitors physical dimension
  virtio-gpu: set physical dimensions for EDID

 hw/display/edid-generate.c     | 21 ++++++++-----
 hw/display/virtio-gpu-base.c   |  2 ++
 hw/display/virtio-gpu.c        |  2 ++
 include/hw/display/edid.h      |  2 ++
 include/hw/virtio/virtio-gpu.h |  1 +
 include/ui/console.h           |  4 +++
 ui/console.c                   |  7 +++++
 ui/spice-display.c             | 54 +++++++++++++++++++++++++++++++++-
 8 files changed, 84 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH 1/4] edid: use physical dimensions if available

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add width_mm/height_mm to qemu_edid_info, and use it if it is
set (non-zero) to generate the EDID.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/edid-generate.c | 21 +++++++++++++--------
 include/hw/display/edid.h  |  2 ++
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/display/edid-generate.c b/hw/display/edid-generate.c
index e58472fde5..14cfb94447 100644
--- a/hw/display/edid-generate.c
+++ b/hw/display/edid-generate.c
@@ -205,12 +205,8 @@ static void edid_desc_dummy(uint8_t *desc)
 
 static void edid_desc_timing(uint8_t *desc,
                              uint32_t xres, uint32_t yres,
-                             uint32_t dpi)
+                             uint32_t xmm, uint32_t ymm)
 {
-    /* physical display size */
-    uint32_t xmm = xres * dpi / 254;
-    uint32_t ymm = yres * dpi / 254;
-
     /* pull some realistic looking timings out of thin air */
     uint32_t xfront = xres * 25 / 100;
     uint32_t xsync  = xres *  3 / 100;
@@ -296,6 +292,7 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
     uint32_t desc = 54;
     uint8_t *xtra3 = NULL;
     uint8_t *dta = NULL;
+    uint32_t width_mm, height_mm;
 
     /* =============== set defaults  =============== */
 
@@ -314,6 +311,13 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
     if (!info->prefy) {
         info->prefy = 768;
     }
+    if (info->width_mm && info->height_mm) {
+        width_mm = info->width_mm;
+        height_mm = info->height_mm;
+    } else {
+        width_mm = info->prefx * info->dpi / 254;
+        height_mm = info->prefy * info->dpi / 254;
+    }
 
     /* =============== extensions  =============== */
 
@@ -360,8 +364,8 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
     edid[20] = 0xa5;
 
     /* screen size: undefined */
-    edid[21] = info->prefx * 254 / 100 / info->dpi;
-    edid[22] = info->prefy * 254 / 100 / info->dpi;
+    edid[21] = width_mm / 10;
+    edid[22] = height_mm / 10;
 
     /* display gamma: 2.2 */
     edid[23] = 220 - 100;
@@ -387,7 +391,8 @@ void qemu_edid_generate(uint8_t *edid, size_t size,
 
     /* =============== descriptor blocks =============== */
 
-    edid_desc_timing(edid + desc, info->prefx, info->prefy, info->dpi);
+    edid_desc_timing(edid + desc, info->prefx, info->prefy,
+                     width_mm, height_mm);
     desc += 18;
 
     edid_desc_ranges(edid + desc);
diff --git a/include/hw/display/edid.h b/include/hw/display/edid.h
index 5b1de57f24..f98b49a944 100644
--- a/include/hw/display/edid.h
+++ b/include/hw/display/edid.h
@@ -6,6 +6,8 @@ typedef struct qemu_edid_info {
     const char *name;
     const char *serial;
     uint32_t    dpi;
+    uint16_t    width_mm;
+    uint16_t    height_mm;
     uint32_t    prefx;
     uint32_t    prefy;
     uint32_t    maxx;
-- 
2.26.2

[PATCH 2/4] ui: add getter for UIInfo

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The following patch is going to introduce extra fields / details to
UIInfo. Add a getter and keep the current values, instead of memset(0)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/console.c       | 7 +++++++
 ui/spice-display.c | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/ui/console.c b/ui/console.c
index 0579be792f..2a0d191d4e 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1513,6 +1513,13 @@ bool dpy_ui_info_supported(QemuConsole *con)
     return con->hw_ops->ui_info != NULL;
 }
 
+const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con)
+{
+    assert(con != NULL);
+
+    return &con->ui_info;
+}
+
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info)
 {
     assert(con != NULL);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 19632fdf6c..625d9232b9 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -672,7 +672,7 @@ static int interface_client_monitors_config(QXLInstance *sin,
         return 1;
     }
 
-    memset(&info, 0, sizeof(info));
+    info = *dpy_get_ui_info(ssd->dcl.con);
 
     if (mc->num_of_monitors == 1) {
         /*
-- 
2.26.2

[PATCH 3/4] spice: get monitors physical dimension

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Note that for consistency, we use the same logic as MonitorsConfig to
figure out the associated monitor. However, I can't find traces of the
discussion/patches about the "new spice-server" behaviour: it still uses
the multiple-configurations path in git master.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/ui/console.h |  4 ++++
 ui/spice-display.c   | 52 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index f35b4fc082..c334b92e70 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -133,6 +133,9 @@ typedef struct DisplaySurface {
 } DisplaySurface;
 
 typedef struct QemuUIInfo {
+    /* physical dimension */
+    uint16_t width_mm;
+    uint16_t height_mm;
     /* geometry */
     int       xoff;
     int       yoff;
@@ -278,6 +281,7 @@ void update_displaychangelistener(DisplayChangeListener *dcl,
 void unregister_displaychangelistener(DisplayChangeListener *dcl);
 
 bool dpy_ui_info_supported(QemuConsole *con);
+const QemuUIInfo *dpy_get_ui_info(const QemuConsole *con);
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info);
 
 void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h);
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 625d9232b9..93d4e1c0a4 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -657,6 +657,53 @@ static void interface_set_client_capabilities(QXLInstance *sin,
     /* nothing to do */
 }
 
+#if SPICE_INTERFACE_QXL_MAJOR >= 3 && SPICE_INTERFACE_QXL_MINOR >= 4
+static int interface_client_monitors_mm(QXLInstance *sin, VDAgentMonitorsMM *mm)
+{
+    SimpleSpiceDisplay *ssd = container_of(sin, SimpleSpiceDisplay, qxl);
+    QemuUIInfo info;
+    int head;
+
+    if (!dpy_ui_info_supported(ssd->dcl.con)) {
+        return 0; /* == not supported by guest */
+    }
+
+    if (!mm) {
+        return 1;
+    }
+
+    info = *dpy_get_ui_info(ssd->dcl.con);
+    /* Note: this code doesn't handle Spice multi-head support, where multiple
+     * monitor configuration for a single QXL device means multiple head. */
+    if (mm->num_of_monitors == 1) {
+        /*
+         * New spice-server version which filters the list of monitors
+         * to only include those that belong to our display channel.
+         *
+         * single-head configuration (where filtering doesn't matter)
+         * takes this code path too.
+         */
+        info.width_mm  = mm->monitors[0].width;
+        info.height_mm = mm->monitors[0].height;
+    } else {
+        /*
+         * Old spice-server which gives us all monitors, so we have to
+         * figure ourself which entry we need.  Array index is the
+         * channel_id, which is the qemu console index, see
+         * qemu_spice_add_display_interface().
+         */
+        head = qemu_console_get_index(ssd->dcl.con);
+        if (mm->num_of_monitors > head) {
+            info.width_mm  = mm->monitors[head].width;
+            info.height_mm = mm->monitors[head].height;
+        }
+    }
+
+    dpy_set_ui_info(ssd->dcl.con, &info);
+    return 1;
+}
+#endif
+
 static int interface_client_monitors_config(QXLInstance *sin,
                                             VDAgentMonitorsConfig *mc)
 {
@@ -674,6 +721,8 @@ static int interface_client_monitors_config(QXLInstance *sin,
 
     info = *dpy_get_ui_info(ssd->dcl.con);
 
+    /* Note: this code doesn't handle Spice multi-head support, where multiple
+     * monitor configuration for a single QXL device means multiple head. */
     if (mc->num_of_monitors == 1) {
         /*
          * New spice-server version which filters the list of monitors
@@ -728,6 +777,9 @@ static const QXLInterface dpy_interface = {
     .update_area_complete    = interface_update_area_complete,
     .set_client_capabilities = interface_set_client_capabilities,
     .client_monitors_config  = interface_client_monitors_config,
+#if SPICE_INTERFACE_QXL_MAJOR >= 3 && SPICE_INTERFACE_QXL_MINOR >= 4
+    .client_monitors_mm      = interface_client_monitors_mm,
+#endif
 };
 
 static void display_update(DisplayChangeListener *dcl,
-- 
2.26.2

[PATCH 4/4] virtio-gpu: set physical dimensions for EDID

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-base.c   | 2 ++
 hw/display/virtio-gpu.c        | 2 ++
 include/hw/virtio/virtio-gpu.h | 1 +
 3 files changed, 5 insertions(+)

diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 7961308606..2fb8beb7da 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -82,6 +82,8 @@ static int virtio_gpu_ui_info(void *opaque, uint32_t idx, QemuUIInfo *info)
     g->req_state[idx].y = info->yoff;
     g->req_state[idx].width = info->width;
     g->req_state[idx].height = info->height;
+    g->req_state[idx].width_mm = info->width_mm;
+    g->req_state[idx].height_mm = info->height_mm;
 
     if (info->width && info->height) {
         g->enabled_output_bitmask |= (1 << idx);
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5f0dd7c150..28c390078a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -212,6 +212,8 @@ virtio_gpu_generate_edid(VirtIOGPU *g, int scanout,
 {
     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,
     };
diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 6dd57f2025..3d1adc563c 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -65,6 +65,7 @@ struct virtio_gpu_scanout {
 };
 
 struct virtio_gpu_requested_state {
+    uint16_t width_mm, height_mm;
     uint32_t width, height;
     int x, y;
 };
-- 
2.26.2

Re: [PATCH 1/4] edid: use physical dimensions if available

[Gerd Hoffmann]

On Mon, Aug 17, 2020 at 04:00:53PM +0400, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Add width_mm/height_mm to qemu_edid_info, and use it if it is
> set (non-zero) to generate the EDID.

Any specific reason why you switch from dpi to xmm/ymm?

take care,
  Gerd

Re: [PATCH 1/4] edid: use physical dimensions if available

[Marc-André Lureau]

Hi

On Mon, Aug 17, 2020 at 4:21 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Mon, Aug 17, 2020 at 04:00:53PM +0400, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Add width_mm/height_mm to qemu_edid_info, and use it if it is
> > set (non-zero) to generate the EDID.
>
> Any specific reason why you switch from dpi to xmm/ymm?

Not really, but there is no DPI information from Gtk. I also find it
difficult to reason with DPI, dimensions are simpler to check about
correctness imho (I take the ruler from my desk for example ;). And
also DPI is a space density, without horizontal and vertical
distinction.

So by giving width/height in mm we actually have something more
correct and easier to debug imho. No?

Re: [PATCH 1/4] edid: use physical dimensions if available

[Gerd Hoffmann]

On Mon, Aug 17, 2020 at 04:57:55PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Aug 17, 2020 at 4:21 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Mon, Aug 17, 2020 at 04:00:53PM +0400, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > > Add width_mm/height_mm to qemu_edid_info, and use it if it is
> > > set (non-zero) to generate the EDID.
> >
> > Any specific reason why you switch from dpi to xmm/ymm?
> 
> Not really, but there is no DPI information from Gtk. I also find it
> difficult to reason with DPI, dimensions are simpler to check about
> correctness imho (I take the ruler from my desk for example ;). And
> also DPI is a space density, without horizontal and vertical
> distinction.

Typically computer displays have square pixels, so that shouldn't be a
problem.  For manually configuration it is easier if you have to deal
with one value only not two.

> So by giving width/height in mm we actually have something more
> correct and easier to debug imho. No?

I dislike having both with/height and dpi in struct qemu_edid_info.

Suggestion:  Drop dpi struct member (should be easy, I think it isn't
wired anywhere yet).  Add two little qemu_edid_* helpers to convert
from/to dpi.  If only one of xmm/ymm is given go calculate the other
automatically (assuming square pixels).  If none is given use 100 dpi
(like the current code does).

take care,
  Gerd