[Qemu-devel] [PATCH v3 0/3] vfio/display: add edid support.

[Qemu-devel] [PATCH v3 0/3] vfio/display: add edid support.

[Gerd Hoffmann]

The 5.0 linux kernel header update finally landed in master.  So this
series has no unmerged dependencies any more.  Rebasing and re-sending
for merge.

This series adds EDID support to the qemu vfio display code.  Various
display-reladed information -- most importantly the display resolution
which should be used -- is passed to the guest that way.  The (initial)
display resolution can be set using the new xres and yres properties.
When supported by the UI it will also be updated on window resizes.

v3:
 - change xres+yres property error handling.
 - swap one leftover fprintf for a tracepoint.

Gerd Hoffmann (3):
  vfio/display: add edid support.
  vfio/display: add xres + yres properties
  vfio/display: delay link up event

 hw/vfio/pci.h                 |   2 +
 include/hw/vfio/vfio-common.h |   4 ++
 hw/vfio/display.c             | 155 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 |  12 ++++
 hw/vfio/trace-events          |   7 ++
 5 files changed, 180 insertions(+)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/3] vfio/display: add edid support.

[Gerd Hoffmann]

This patch adds EDID support to the vfio display (aka vgpu) code.
When supported by the mdev driver qemu will generate a EDID blob
and pass it on using the new vfio edid region.  The EDID blob will
be updated on UI changes (i.e. window resize), so the guest can
adapt.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/vfio/vfio-common.h |   3 +
 hw/vfio/display.c             | 127 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/trace-events          |   7 +++
 3 files changed, 137 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 7624c9f511c4..5f7f709b95f1 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -148,6 +148,9 @@ typedef struct VFIODMABuf {
 typedef struct VFIODisplay {
     QemuConsole *con;
     RAMFBState *ramfb;
+    struct vfio_region_info *edid_info;
+    struct vfio_region_gfx_edid *edid_regs;
+    uint8_t *edid_blob;
     struct {
         VFIORegion buffer;
         DisplaySurface *surface;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index dead30e626cb..f59fcc487128 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -15,15 +15,139 @@
 #include <sys/ioctl.h>
 
 #include "sysemu/sysemu.h"
+#include "hw/display/edid.h"
 #include "ui/console.h"
 #include "qapi/error.h"
 #include "pci.h"
+#include "trace.h"
 
 #ifndef DRM_PLANE_TYPE_PRIMARY
 # define DRM_PLANE_TYPE_PRIMARY 1
 # define DRM_PLANE_TYPE_CURSOR  2
 #endif
 
+#define pread_field(_fd, _reg, _ptr, _fld)                              \
+    if (sizeof(_ptr->_fld) !=                                           \
+        pread(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                   \
+              _reg->offset + offsetof(typeof(*_ptr), _fld)))            \
+        goto err;
+#define pwrite_field(_fd, _reg, _ptr, _fld)                             \
+    if (sizeof(_ptr->_fld) !=                                           \
+        pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                  \
+               _reg->offset + offsetof(typeof(*_ptr), _fld)))           \
+        goto err;
+
+
+static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
+                                     int prefx, int prefy)
+{
+    VFIODisplay *dpy = vdev->dpy;
+    int fd = vdev->vbasedev.fd;
+    qemu_edid_info edid = {
+        .maxx  = dpy->edid_regs->max_xres,
+        .maxy  = dpy->edid_regs->max_yres,
+        .prefx = prefx,
+        .prefy = prefy,
+    };
+
+    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
+    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    trace_vfio_display_edid_link_down();
+
+    if (!enabled) {
+        return;
+    }
+
+    if (edid.maxx && edid.prefx > edid.maxx) {
+        edid.prefx = edid.maxx;
+    }
+    if (edid.maxy && edid.prefy > edid.maxy) {
+        edid.prefy = edid.maxy;
+    }
+    qemu_edid_generate(dpy->edid_blob,
+                       dpy->edid_regs->edid_max_size,
+                       &edid);
+    trace_vfio_display_edid_update(edid.prefx, edid.prefy);
+
+    dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
+    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, edid_size);
+    if (pwrite(fd, dpy->edid_blob, dpy->edid_regs->edid_size,
+               dpy->edid_info->offset + dpy->edid_regs->edid_offset)
+        != dpy->edid_regs->edid_size) {
+        goto err;
+    }
+
+    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
+    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    trace_vfio_display_edid_link_up();
+    return;
+
+err:
+    trace_vfio_display_edid_write_error();
+    return;
+}
+
+static int vfio_display_edid_ui_info(void *opaque, uint32_t idx,
+                                     QemuUIInfo *info)
+{
+    VFIOPCIDevice *vdev = opaque;
+    VFIODisplay *dpy = vdev->dpy;
+
+    if (!dpy->edid_regs) {
+        return 0;
+    }
+
+    if (info->width && info->height) {
+        vfio_display_edid_update(vdev, true, info->width, info->height);
+    } else {
+        vfio_display_edid_update(vdev, false, 0, 0);
+    }
+
+    return 0;
+}
+
+static void vfio_display_edid_init(VFIOPCIDevice *vdev)
+{
+    VFIODisplay *dpy = vdev->dpy;
+    int fd = vdev->vbasedev.fd;
+    int ret;
+
+    ret = vfio_get_dev_region_info(&vdev->vbasedev,
+                                   VFIO_REGION_TYPE_GFX,
+                                   VFIO_REGION_SUBTYPE_GFX_EDID,
+                                   &dpy->edid_info);
+    if (ret) {
+        return;
+    }
+
+    trace_vfio_display_edid_available();
+    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
+    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset);
+    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
+    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres);
+    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
+    dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
+
+    vfio_display_edid_update(vdev, true, 0, 0);
+    return;
+
+err:
+    trace_vfio_display_edid_write_error();
+    g_free(dpy->edid_regs);
+    dpy->edid_regs = NULL;
+    return;
+}
+
+static void vfio_display_edid_exit(VFIODisplay *dpy)
+{
+    if (!dpy->edid_regs) {
+        return;
+    }
+
+    g_free(dpy->edid_regs);
+    g_free(dpy->edid_blob);
+}
+
 static void vfio_display_update_cursor(VFIODMABuf *dmabuf,
                                        struct vfio_device_gfx_plane_info *plane)
 {
@@ -171,6 +295,7 @@ static void vfio_display_dmabuf_update(void *opaque)
 
 static const GraphicHwOps vfio_display_dmabuf_ops = {
     .gfx_update = vfio_display_dmabuf_update,
+    .ui_info    = vfio_display_edid_ui_info,
 };
 
 static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
@@ -187,6 +312,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
     if (vdev->enable_ramfb) {
         vdev->dpy->ramfb = ramfb_setup(errp);
     }
+    vfio_display_edid_init(vdev);
     return 0;
 }
 
@@ -366,5 +492,6 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
     graphic_console_close(vdev->dpy->con);
     vfio_display_dmabuf_exit(vdev->dpy);
     vfio_display_region_exit(vdev->dpy);
+    vfio_display_edid_exit(vdev->dpy);
     g_free(vdev->dpy);
 }
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index f41ca96160bf..28d445caaf74 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -131,3 +131,10 @@ vfio_prereg_unregister(uint64_t va, uint64_t size, int ret) "va=0x%"PRIx64" size
 vfio_spapr_create_window(int ps, uint64_t ws, uint64_t off) "pageshift=0x%x winsize=0x%"PRIx64" offset=0x%"PRIx64
 vfio_spapr_remove_window(uint64_t off) "offset=0x%"PRIx64
 vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
+
+# hw/vfio/display.c
+vfio_display_edid_available(void) ""
+vfio_display_edid_link_up(void) ""
+vfio_display_edid_link_down(void) ""
+vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%dx%d"
+vfio_display_edid_write_error(void) ""
-- 
2.9.3

[Qemu-devel] [PATCH v3 2/3] vfio/display: add xres + yres properties

[Gerd Hoffmann]

This allows configure the display resolution which the vgpu should use.
The information will be passed to the guest using EDID, so the mdev
driver must support the vfio edid region for this to work.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/vfio/pci.h     |  2 ++
 hw/vfio/display.c | 12 ++++++++++--
 hw/vfio/pci.c     | 12 ++++++++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index b1ae4c07549a..c11c3f167070 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -149,6 +149,8 @@ typedef struct VFIOPCIDevice {
 #define VFIO_FEATURE_ENABLE_IGD_OPREGION \
                                 (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
     OnOffAuto display;
+    uint32_t display_xres;
+    uint32_t display_yres;
     int32_t bootindex;
     uint32_t igd_gms;
     OffAutoPCIBAR msix_relo;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index f59fcc487128..8bf7c890be96 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -46,8 +46,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
     qemu_edid_info edid = {
         .maxx  = dpy->edid_regs->max_xres,
         .maxy  = dpy->edid_regs->max_yres,
-        .prefx = prefx,
-        .prefy = prefy,
+        .prefx = prefx ?: vdev->display_xres,
+        .prefy = prefy ?: vdev->display_yres,
     };
 
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
@@ -128,6 +128,14 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
     pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
     dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
 
+    /* if xres + yres properties are unset use the maximum resolution */
+    if (!vdev->display_xres) {
+        vdev->display_xres = dpy->edid_regs->max_xres;
+    }
+    if (!vdev->display_yres) {
+        vdev->display_yres = dpy->edid_regs->max_yres;
+    }
+
     vfio_display_edid_update(vdev, true, 0, 0);
     return;
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index dd12f363915d..504019c4582b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3068,6 +3068,16 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
         error_setg(errp, "ramfb=on requires display=on");
         goto out_teardown;
     }
+    if (vdev->display_xres || vdev->display_yres) {
+        if (vdev->dpy == NULL) {
+            error_setg(errp, "xres and yres properties require display=on");
+            goto out_teardown;
+        }
+        if (vdev->dpy->edid_regs == NULL) {
+            error_setg(errp, "xres and yres properties need edid support");
+            goto out_teardown;
+        }
+    }
 
     vfio_register_err_notifier(vdev);
     vfio_register_req_notifier(vdev);
@@ -3182,6 +3192,8 @@ static Property vfio_pci_dev_properties[] = {
     DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
     DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
                             display, ON_OFF_AUTO_OFF),
+    DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
+    DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
     DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
                        intx.mmap_timeout, 1100),
     DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
-- 
2.9.3

[Qemu-devel] [PATCH v3 3/3] vfio/display: delay link up event

[Gerd Hoffmann]

Kick the display link up event with a 0.1 sec delay,
so the guest has a chance to notice the link down first.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/hw/vfio/vfio-common.h |  1 +
 hw/vfio/display.c             | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 5f7f709b95f1..b65a2f051886 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -151,6 +151,7 @@ typedef struct VFIODisplay {
     struct vfio_region_info *edid_info;
     struct vfio_region_gfx_edid *edid_regs;
     uint8_t *edid_blob;
+    QEMUTimer *edid_link_timer;
     struct {
         VFIORegion buffer;
         DisplaySurface *surface;
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 8bf7c890be96..971e801dc05c 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -38,6 +38,21 @@
         goto err;
 
 
+static void vfio_display_edid_link_up(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+    VFIODisplay *dpy = vdev->dpy;
+    int fd = vdev->vbasedev.fd;
+
+    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
+    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    trace_vfio_display_edid_link_up();
+    return;
+
+err:
+    trace_vfio_display_edid_write_error();
+}
+
 static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
                                      int prefx, int prefy)
 {
@@ -50,6 +65,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
         .prefy = prefy ?: vdev->display_yres,
     };
 
+    timer_del(dpy->edid_link_timer);
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
     pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
     trace_vfio_display_edid_link_down();
@@ -77,9 +93,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
         goto err;
     }
 
-    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
-    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
-    trace_vfio_display_edid_link_up();
+    timer_mod(dpy->edid_link_timer,
+              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 100);
     return;
 
 err:
@@ -136,6 +151,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
         vdev->display_yres = dpy->edid_regs->max_yres;
     }
 
+    dpy->edid_link_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
+                                        vfio_display_edid_link_up, vdev);
+
     vfio_display_edid_update(vdev, true, 0, 0);
     return;
 
@@ -154,6 +172,8 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
 
     g_free(dpy->edid_regs);
     g_free(dpy->edid_blob);
+    timer_del(dpy->edid_link_timer);
+    timer_free(dpy->edid_link_timer);
 }
 
 static void vfio_display_update_cursor(VFIODMABuf *dmabuf,
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 1/3] vfio/display: add edid support.

[Liam Merwick]

On 22/02/2019 05:49, Gerd Hoffmann wrote:
> This patch adds EDID support to the vfio display (aka vgpu) code.
> When supported by the mdev driver qemu will generate a EDID blob
> and pass it on using the new vfio edid region.  The EDID blob will
> be updated on UI changes (i.e. window resize), so the guest can
> adapt.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>   include/hw/vfio/vfio-common.h |   3 +
>   hw/vfio/display.c             | 127 ++++++++++++++++++++++++++++++++++++++++++
>   hw/vfio/trace-events          |   7 +++
>   3 files changed, 137 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 7624c9f511c4..5f7f709b95f1 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -148,6 +148,9 @@ typedef struct VFIODMABuf {
>   typedef struct VFIODisplay {
>       QemuConsole *con;
>       RAMFBState *ramfb;
> +    struct vfio_region_info *edid_info;
> +    struct vfio_region_gfx_edid *edid_regs;
> +    uint8_t *edid_blob;
>       struct {
>           VFIORegion buffer;
>           DisplaySurface *surface;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index dead30e626cb..f59fcc487128 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -15,15 +15,139 @@
>   #include <sys/ioctl.h>
>   
>   #include "sysemu/sysemu.h"
> +#include "hw/display/edid.h"
>   #include "ui/console.h"
>   #include "qapi/error.h"
>   #include "pci.h"
> +#include "trace.h"
>   
>   #ifndef DRM_PLANE_TYPE_PRIMARY
>   # define DRM_PLANE_TYPE_PRIMARY 1
>   # define DRM_PLANE_TYPE_CURSOR  2
>   #endif
>   
> +#define pread_field(_fd, _reg, _ptr, _fld)                              \
> +    if (sizeof(_ptr->_fld) !=                                           \
> +        pread(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                   \
> +              _reg->offset + offsetof(typeof(*_ptr), _fld)))            \
> +        goto err;
> +#define pwrite_field(_fd, _reg, _ptr, _fld)                             \
> +    if (sizeof(_ptr->_fld) !=                                           \
> +        pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                  \
> +               _reg->offset + offsetof(typeof(*_ptr), _fld)))           \
> +        goto err;
> +
> +
> +static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
> +                                     int prefx, int prefy)
> +{
> +    VFIODisplay *dpy = vdev->dpy;
> +    int fd = vdev->vbasedev.fd;
> +    qemu_edid_info edid = {
> +        .maxx  = dpy->edid_regs->max_xres,
> +        .maxy  = dpy->edid_regs->max_yres,
> +        .prefx = prefx,
> +        .prefy = prefy,
> +    };
> +
> +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
> +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    trace_vfio_display_edid_link_down();
> +
> +    if (!enabled) {
> +        return;
> +    }
> +
> +    if (edid.maxx && edid.prefx > edid.maxx) {
> +        edid.prefx = edid.maxx;
> +    }
> +    if (edid.maxy && edid.prefy > edid.maxy) {
> +        edid.prefy = edid.maxy;
> +    }
> +    qemu_edid_generate(dpy->edid_blob,
> +                       dpy->edid_regs->edid_max_size,
> +                       &edid);
> +    trace_vfio_display_edid_update(edid.prefx, edid.prefy);
> +
> +    dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
> +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, edid_size);
> +    if (pwrite(fd, dpy->edid_blob, dpy->edid_regs->edid_size,
> +               dpy->edid_info->offset + dpy->edid_regs->edid_offset)
> +        != dpy->edid_regs->edid_size) {
> +        goto err;
> +    }
> +
> +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    trace_vfio_display_edid_link_up();
> +    return;
> +
> +err:
> +    trace_vfio_display_edid_write_error();
> +    return;
> +}
> +
> +static int vfio_display_edid_ui_info(void *opaque, uint32_t idx,
> +                                     QemuUIInfo *info)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +    VFIODisplay *dpy = vdev->dpy;
> +
> +    if (!dpy->edid_regs) {
> +        return 0;
> +    }
> +
> +    if (info->width && info->height) {
> +        vfio_display_edid_update(vdev, true, info->width, info->height);
> +    } else {
> +        vfio_display_edid_update(vdev, false, 0, 0);
> +    }
> +
> +    return 0;
> +}
> +
> +static void vfio_display_edid_init(VFIOPCIDevice *vdev)
> +{
> +    VFIODisplay *dpy = vdev->dpy;
> +    int fd = vdev->vbasedev.fd;
> +    int ret;
> +
> +    ret = vfio_get_dev_region_info(&vdev->vbasedev,
> +                                   VFIO_REGION_TYPE_GFX,
> +                                   VFIO_REGION_SUBTYPE_GFX_EDID,
> +                                   &dpy->edid_info);
> +    if (ret) {
> +        return;
> +    }
> +
> +    trace_vfio_display_edid_available();
> +    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
> +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset);
> +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
> +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres);
> +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
> +    dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
> +
> +    vfio_display_edid_update(vdev, true, 0, 0);
> +    return;
> +
> +err:


Might it be worth a comment that the p{read|write}_field macros use this 
label since it's not immediately obvious when reading the code there's a 
use for this (compiler would have flagged it admittedly). Same comment 
for vfio_display_edid_update() to a lesser extent.


> +    trace_vfio_display_edid_write_error();
> +    g_free(dpy->edid_regs);
> +    dpy->edid_regs = NULL;
> +    return;
> +}
> +
> +static void vfio_display_edid_exit(VFIODisplay *dpy)
> +{
> +    if (!dpy->edid_regs) {
> +        return;
> +    }
> +
> +    g_free(dpy->edid_regs);
> +    g_free(dpy->edid_blob);
> +}
> +
>   static void vfio_display_update_cursor(VFIODMABuf *dmabuf,
>                                          struct vfio_device_gfx_plane_info *plane)
>   {
> @@ -171,6 +295,7 @@ static void vfio_display_dmabuf_update(void *opaque)
>   
>   static const GraphicHwOps vfio_display_dmabuf_ops = {
>       .gfx_update = vfio_display_dmabuf_update,
> +    .ui_info    = vfio_display_edid_ui_info,
>   };
>   
>   static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
> @@ -187,6 +312,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice *vdev, Error **errp)
>       if (vdev->enable_ramfb) {
>           vdev->dpy->ramfb = ramfb_setup(errp);
>       }
> +    vfio_display_edid_init(vdev);
>       return 0;
>   }
>   
> @@ -366,5 +492,6 @@ void vfio_display_finalize(VFIOPCIDevice *vdev)
>       graphic_console_close(vdev->dpy->con);
>       vfio_display_dmabuf_exit(vdev->dpy);
>       vfio_display_region_exit(vdev->dpy);
> +    vfio_display_edid_exit(vdev->dpy);
>       g_free(vdev->dpy);
>   }
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index f41ca96160bf..28d445caaf74 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -131,3 +131,10 @@ vfio_prereg_unregister(uint64_t va, uint64_t size, int ret) "va=0x%"PRIx64" size
>   vfio_spapr_create_window(int ps, uint64_t ws, uint64_t off) "pageshift=0x%x winsize=0x%"PRIx64" offset=0x%"PRIx64
>   vfio_spapr_remove_window(uint64_t off) "offset=0x%"PRIx64
>   vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn fd %d"
> +
> +# hw/vfio/display.c
> +vfio_display_edid_available(void) ""
> +vfio_display_edid_link_up(void) ""
> +vfio_display_edid_link_down(void) ""
> +vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%dx%d"

These should be %u since the variables are uint32_t

> +vfio_display_edid_write_error(void) ""
> 

Otherwise

Reviewed-by: Liam Merwick <liam.merwick@oracle.com>

Re: [Qemu-devel] [PATCH v3 2/3] vfio/display: add xres + yres properties

[Liam Merwick]

On 22/02/2019 05:49, Gerd Hoffmann wrote:
> This allows configure the display resolution which the vgpu should use.

"configure" -> "configuration of" or "us to configure" ?

> The information will be passed to the guest using EDID, so the mdev
> driver must support the vfio edid region for this to work.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>


Reviewed-by: Liam Merwick <liam.merwick@oracle.com>


> ---
>   hw/vfio/pci.h     |  2 ++
>   hw/vfio/display.c | 12 ++++++++++--
>   hw/vfio/pci.c     | 12 ++++++++++++
>   3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index b1ae4c07549a..c11c3f167070 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -149,6 +149,8 @@ typedef struct VFIOPCIDevice {
>   #define VFIO_FEATURE_ENABLE_IGD_OPREGION \
>                                   (1 << VFIO_FEATURE_ENABLE_IGD_OPREGION_BIT)
>       OnOffAuto display;
> +    uint32_t display_xres;
> +    uint32_t display_yres;
>       int32_t bootindex;
>       uint32_t igd_gms;
>       OffAutoPCIBAR msix_relo;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index f59fcc487128..8bf7c890be96 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -46,8 +46,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
>       qemu_edid_info edid = {
>           .maxx  = dpy->edid_regs->max_xres,
>           .maxy  = dpy->edid_regs->max_yres,
> -        .prefx = prefx,
> -        .prefy = prefy,
> +        .prefx = prefx ?: vdev->display_xres,
> +        .prefy = prefy ?: vdev->display_yres,
>       };
>   
>       dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
> @@ -128,6 +128,14 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>       pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
>       dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
>   
> +    /* if xres + yres properties are unset use the maximum resolution */
> +    if (!vdev->display_xres) {
> +        vdev->display_xres = dpy->edid_regs->max_xres;
> +    }
> +    if (!vdev->display_yres) {
> +        vdev->display_yres = dpy->edid_regs->max_yres;
> +    }
> +
>       vfio_display_edid_update(vdev, true, 0, 0);
>       return;
>   
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index dd12f363915d..504019c4582b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3068,6 +3068,16 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
>           error_setg(errp, "ramfb=on requires display=on");
>           goto out_teardown;
>       }
> +    if (vdev->display_xres || vdev->display_yres) {
> +        if (vdev->dpy == NULL) {
> +            error_setg(errp, "xres and yres properties require display=on");
> +            goto out_teardown;
> +        }
> +        if (vdev->dpy->edid_regs == NULL) {
> +            error_setg(errp, "xres and yres properties need edid support");
> +            goto out_teardown;
> +        }
> +    }
>   
>       vfio_register_err_notifier(vdev);
>       vfio_register_req_notifier(vdev);
> @@ -3182,6 +3192,8 @@ static Property vfio_pci_dev_properties[] = {
>       DEFINE_PROP_STRING("sysfsdev", VFIOPCIDevice, vbasedev.sysfsdev),
>       DEFINE_PROP_ON_OFF_AUTO("display", VFIOPCIDevice,
>                               display, ON_OFF_AUTO_OFF),
> +    DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> +    DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
>       DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                          intx.mmap_timeout, 1100),
>       DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,
> 

Re: [Qemu-devel] [PATCH v3 3/3] vfio/display: delay link up event

[Liam Merwick]

On 22/02/2019 05:49, Gerd Hoffmann wrote:
> Kick the display link up event with a 0.1 sec delay,
> so the guest has a chance to notice the link down first.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Depending on your thoughts on the suggestion in patch 1 regarding a 
comment at the 'err' label - another candidate in 
vfio_display_edid_link_up().

Either way:

Reviewed-by: Liam Merwick <liam.merwick@oracle.com>


> ---
>   include/hw/vfio/vfio-common.h |  1 +
>   hw/vfio/display.c             | 26 +++++++++++++++++++++++---
>   2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 5f7f709b95f1..b65a2f051886 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -151,6 +151,7 @@ typedef struct VFIODisplay {
>       struct vfio_region_info *edid_info;
>       struct vfio_region_gfx_edid *edid_regs;
>       uint8_t *edid_blob;
> +    QEMUTimer *edid_link_timer;
>       struct {
>           VFIORegion buffer;
>           DisplaySurface *surface;
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 8bf7c890be96..971e801dc05c 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -38,6 +38,21 @@
>           goto err;
>   
>   
> +static void vfio_display_edid_link_up(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +    VFIODisplay *dpy = vdev->dpy;
> +    int fd = vdev->vbasedev.fd;
> +
> +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    trace_vfio_display_edid_link_up();
> +    return;
> +
> +err:
> +    trace_vfio_display_edid_write_error();
> +}
> +
>   static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
>                                        int prefx, int prefy)
>   {
> @@ -50,6 +65,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
>           .prefy = prefy ?: vdev->display_yres,
>       };
>   
> +    timer_del(dpy->edid_link_timer);
>       dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
>       pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
>       trace_vfio_display_edid_link_down();
> @@ -77,9 +93,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
>           goto err;
>       }
>   
> -    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> -    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> -    trace_vfio_display_edid_link_up();
> +    timer_mod(dpy->edid_link_timer,
> +              qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 100);
>       return;
>   
>   err:
> @@ -136,6 +151,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>           vdev->display_yres = dpy->edid_regs->max_yres;
>       }
>   
> +    dpy->edid_link_timer = timer_new_ms(QEMU_CLOCK_REALTIME,
> +                                        vfio_display_edid_link_up, vdev);
> +
>       vfio_display_edid_update(vdev, true, 0, 0);
>       return;
>   
> @@ -154,6 +172,8 @@ static void vfio_display_edid_exit(VFIODisplay *dpy)
>   
>       g_free(dpy->edid_regs);
>       g_free(dpy->edid_blob);
> +    timer_del(dpy->edid_link_timer);
> +    timer_free(dpy->edid_link_timer);
>   }
>   
>   static void vfio_display_update_cursor(VFIODMABuf *dmabuf,
> 

Re: [Qemu-devel] [PATCH v3 1/3] vfio/display: add edid support.

[Alex Williamson]

On Fri, 22 Feb 2019 11:09:06 +0000
Liam Merwick <liam.merwick@oracle.com> wrote:

> On 22/02/2019 05:49, Gerd Hoffmann wrote:
> > This patch adds EDID support to the vfio display (aka vgpu) code.
> > When supported by the mdev driver qemu will generate a EDID blob
> > and pass it on using the new vfio edid region.  The EDID blob will
> > be updated on UI changes (i.e. window resize), so the guest can
> > adapt.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >   include/hw/vfio/vfio-common.h |   3 +
> >   hw/vfio/display.c             | 127 ++++++++++++++++++++++++++++++++++++++++++
> >   hw/vfio/trace-events          |   7 +++
> >   3 files changed, 137 insertions(+)
> > 
> > diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> > index 7624c9f511c4..5f7f709b95f1 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -148,6 +148,9 @@ typedef struct VFIODMABuf {
> >   typedef struct VFIODisplay {
> >       QemuConsole *con;
> >       RAMFBState *ramfb;
> > +    struct vfio_region_info *edid_info;
> > +    struct vfio_region_gfx_edid *edid_regs;
> > +    uint8_t *edid_blob;
> >       struct {
> >           VFIORegion buffer;
> >           DisplaySurface *surface;
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index dead30e626cb..f59fcc487128 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -15,15 +15,139 @@
> >   #include <sys/ioctl.h>
> >   
> >   #include "sysemu/sysemu.h"
> > +#include "hw/display/edid.h"
> >   #include "ui/console.h"
> >   #include "qapi/error.h"
> >   #include "pci.h"
> > +#include "trace.h"
> >   
> >   #ifndef DRM_PLANE_TYPE_PRIMARY
> >   # define DRM_PLANE_TYPE_PRIMARY 1
> >   # define DRM_PLANE_TYPE_CURSOR  2
> >   #endif
> >   
> > +#define pread_field(_fd, _reg, _ptr, _fld)                              \
> > +    if (sizeof(_ptr->_fld) !=                                           \
> > +        pread(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                   \
> > +              _reg->offset + offsetof(typeof(*_ptr), _fld)))            \
> > +        goto err;
> > +#define pwrite_field(_fd, _reg, _ptr, _fld)                             \
> > +    if (sizeof(_ptr->_fld) !=                                           \
> > +        pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                  \
> > +               _reg->offset + offsetof(typeof(*_ptr), _fld)))           \
> > +        goto err;
> > +
> > +
> > +static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
> > +                                     int prefx, int prefy)
> > +{
> > +    VFIODisplay *dpy = vdev->dpy;
> > +    int fd = vdev->vbasedev.fd;
> > +    qemu_edid_info edid = {
> > +        .maxx  = dpy->edid_regs->max_xres,
> > +        .maxy  = dpy->edid_regs->max_yres,
> > +        .prefx = prefx,
> > +        .prefy = prefy,
> > +    };
> > +
> > +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
> > +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> > +    trace_vfio_display_edid_link_down();
> > +
> > +    if (!enabled) {
> > +        return;
> > +    }
> > +
> > +    if (edid.maxx && edid.prefx > edid.maxx) {
> > +        edid.prefx = edid.maxx;
> > +    }
> > +    if (edid.maxy && edid.prefy > edid.maxy) {
> > +        edid.prefy = edid.maxy;
> > +    }
> > +    qemu_edid_generate(dpy->edid_blob,
> > +                       dpy->edid_regs->edid_max_size,
> > +                       &edid);
> > +    trace_vfio_display_edid_update(edid.prefx, edid.prefy);
> > +
> > +    dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
> > +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, edid_size);
> > +    if (pwrite(fd, dpy->edid_blob, dpy->edid_regs->edid_size,
> > +               dpy->edid_info->offset + dpy->edid_regs->edid_offset)
> > +        != dpy->edid_regs->edid_size) {
> > +        goto err;
> > +    }
> > +
> > +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> > +    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> > +    trace_vfio_display_edid_link_up();
> > +    return;
> > +
> > +err:
> > +    trace_vfio_display_edid_write_error();
> > +    return;
> > +}
> > +
> > +static int vfio_display_edid_ui_info(void *opaque, uint32_t idx,
> > +                                     QemuUIInfo *info)
> > +{
> > +    VFIOPCIDevice *vdev = opaque;
> > +    VFIODisplay *dpy = vdev->dpy;
> > +
> > +    if (!dpy->edid_regs) {
> > +        return 0;
> > +    }
> > +
> > +    if (info->width && info->height) {
> > +        vfio_display_edid_update(vdev, true, info->width, info->height);
> > +    } else {
> > +        vfio_display_edid_update(vdev, false, 0, 0);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void vfio_display_edid_init(VFIOPCIDevice *vdev)
> > +{
> > +    VFIODisplay *dpy = vdev->dpy;
> > +    int fd = vdev->vbasedev.fd;
> > +    int ret;
> > +
> > +    ret = vfio_get_dev_region_info(&vdev->vbasedev,
> > +                                   VFIO_REGION_TYPE_GFX,
> > +                                   VFIO_REGION_SUBTYPE_GFX_EDID,
> > +                                   &dpy->edid_info);
> > +    if (ret) {
> > +        return;
> > +    }
> > +
> > +    trace_vfio_display_edid_available();
> > +    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
> > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset);
> > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
> > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres);
> > +    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
> > +    dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
> > +
> > +    vfio_display_edid_update(vdev, true, 0, 0);
> > +    return;
> > +
> > +err:  
> 
> 
> Might it be worth a comment that the p{read|write}_field macros use this 
> label since it's not immediately obvious when reading the code there's a 
> use for this (compiler would have flagged it admittedly). Same comment 
> for vfio_display_edid_update() to a lesser extent.

Hi Gerd,

Liam and I both found some difficulty with the cleverness of the
macros, so for the sake of better maintainability, I'd like to propose
rolling in the following patch, including Liam's trace format fix.  It's
not as compact as your version, but I think it's equivalent, it's easier
to follow, and doesn't covertly introduce a non-curly braced block ;)
If you agree, I'll roll this into your v3. Thanks,

Alex

 hw/vfio/display.c    |   44 +++++++++++++++++++++++++++++---------------
 hw/vfio/trace-events |    2 +-
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index f59fcc487128..276fba090d8b 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -27,15 +27,14 @@
 #endif
 
 #define pread_field(_fd, _reg, _ptr, _fld)                              \
-    if (sizeof(_ptr->_fld) !=                                           \
-        pread(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                   \
-              _reg->offset + offsetof(typeof(*_ptr), _fld)))            \
-        goto err;
+    (sizeof(_ptr->_fld) !=                                              \
+     pread(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                      \
+           _reg->offset + offsetof(typeof(*_ptr), _fld)))
+
 #define pwrite_field(_fd, _reg, _ptr, _fld)                             \
-    if (sizeof(_ptr->_fld) !=                                           \
-        pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                  \
-               _reg->offset + offsetof(typeof(*_ptr), _fld)))           \
-        goto err;
+    (sizeof(_ptr->_fld) !=                                              \
+     pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                     \
+            _reg->offset + offsetof(typeof(*_ptr), _fld)))
 
 
 static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
@@ -51,7 +50,9 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
     };
 
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
-    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    if (pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state)) {
+        goto err;
+    }
     trace_vfio_display_edid_link_down();
 
     if (!enabled) {
@@ -70,7 +71,9 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
     trace_vfio_display_edid_update(edid.prefx, edid.prefy);
 
     dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
-    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, edid_size);
+    if (pwrite_field(fd, dpy->edid_info, dpy->edid_regs, edid_size)) {
+        goto err;
+    }
     if (pwrite(fd, dpy->edid_blob, dpy->edid_regs->edid_size,
                dpy->edid_info->offset + dpy->edid_regs->edid_offset)
         != dpy->edid_regs->edid_size) {
@@ -78,7 +81,9 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled,
     }
 
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
-    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    if (pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state)) {
+        goto err;
+    }
     trace_vfio_display_edid_link_up();
     return;
 
@@ -122,10 +127,19 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
 
     trace_vfio_display_edid_available();
     dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
-    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset);
-    pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
-    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres);
-    pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres);
+    if (pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_offset)) {
+        goto err;
+    }
+    if (pread_field(fd, dpy->edid_info, dpy->edid_regs, edid_max_size)) {
+        goto err;
+    }
+    if (pread_field(fd, dpy->edid_info, dpy->edid_regs, max_xres)) {
+        goto err;
+    }
+    if (pread_field(fd, dpy->edid_info, dpy->edid_regs, max_yres)) {
+        goto err;
+    }
+
     dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
 
     vfio_display_edid_update(vdev, true, 0, 0);
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 0882f7b59644..decbde495ac2 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -137,5 +137,5 @@ vfio_spapr_group_attach(int groupfd, int tablefd) "Attached groupfd %d to liobn
 vfio_display_edid_available(void) ""
 vfio_display_edid_link_up(void) ""
 vfio_display_edid_link_down(void) ""
-vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%dx%d"
+vfio_display_edid_update(uint32_t prefx, uint32_t prefy) "%ux%u"
 vfio_display_edid_write_error(void) ""

Re: [Qemu-devel] [PATCH v3 3/3] vfio/display: delay link up event

[Alex Williamson]

On Fri, 22 Feb 2019 11:19:11 +0000
Liam Merwick <liam.merwick@oracle.com> wrote:

> On 22/02/2019 05:49, Gerd Hoffmann wrote:
> > Kick the display link up event with a 0.1 sec delay,
> > so the guest has a chance to notice the link down first.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>  
> 
> Depending on your thoughts on the suggestion in patch 1 regarding a 
> comment at the 'err' label - another candidate in 
> vfio_display_edid_link_up().

This would also get the following fixup rolled in.  Thanks,

Alex

 hw/vfio/display.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index e8f312dc3308..a3d9c8f5beac 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -44,7 +44,9 @@ static void vfio_display_edid_link_up(void *opaque)
     int fd = vdev->vbasedev.fd;
 
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
-    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
+    if (pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state)) {
+        goto err;
+    }
     trace_vfio_display_edid_link_up();
     return;
 

Re: [Qemu-devel] [PATCH v3 1/3] vfio/display: add edid support.

[Gerd Hoffmann]

  Hi,

> Liam and I both found some difficulty with the cleverness of the
> macros, so for the sake of better maintainability, I'd like to propose
> rolling in the following patch, including Liam's trace format fix.  It's
> not as compact as your version, but I think it's equivalent, it's easier
> to follow, and doesn't covertly introduce a non-curly braced block ;)
> If you agree, I'll roll this into your v3. Thanks,

>  #define pwrite_field(_fd, _reg, _ptr, _fld)                             \
> -    if (sizeof(_ptr->_fld) !=                                           \
> -        pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                  \
> -               _reg->offset + offsetof(typeof(*_ptr), _fld)))           \
> -        goto err;
> +    (sizeof(_ptr->_fld) !=                                              \
> +     pwrite(_fd, &(_ptr->_fld), sizeof(_ptr->_fld),                     \
> +            _reg->offset + offsetof(typeof(*_ptr), _fld)))

>      dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
> -    pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    if (pwrite_field(fd, dpy->edid_info, dpy->edid_regs, link_state)) {
> +        goto err;
> +    }

Fine with me.

thanks,
  Gerd