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

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

[Gerd Hoffmann]

The 5.0-rc1 linux kernel header update -- which contains the vfio api
update for edid support -- is about to land in qemu master.  Intel just
posted patches implementing EDID support.  Time to undust the test
patches, polish them and post them for review & 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.

Gerd Hoffmann (5):
  vfio: update kernel headers.
  vfio/display: add edid support.
  vfio/display: add xres + yres properties
  vfio/display: delay link up event
  [debug] some logging

 hw/vfio/pci.h                 |   2 +
 include/hw/vfio/vfio-common.h |   4 ++
 linux-headers/linux/vfio.h    |  50 +++++++++++++
 hw/vfio/display.c             | 158 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 |   2 +
 5 files changed, 216 insertions(+)

-- 
2.9.3

[Qemu-devel] [PATCH 1/5] vfio: update kernel headers.

[Gerd Hoffmann]

Temporary, for testing convinience.

Once qemu master is synced with the linux 5.0-rc1 headers
this is not needed any more.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 linux-headers/linux/vfio.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
index ceb6453394..51662a7209 100644
--- a/linux-headers/linux/vfio.h
+++ b/linux-headers/linux/vfio.h
@@ -303,6 +303,56 @@ struct vfio_region_info_cap_type {
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG	(2)
 #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG	(3)
 
+#define VFIO_REGION_TYPE_GFX                    (1)
+#define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
+
+/**
+ * struct vfio_region_gfx_edid - EDID region layout.
+ *
+ * Set display link state and EDID blob.
+ *
+ * The EDID blob has monitor information such as brand, name, serial
+ * number, physical size, supported video modes and more.
+ *
+ * This special region allows userspace (typically qemu) set a virtual
+ * EDID for the virtual monitor, which allows a flexible display
+ * configuration.
+ *
+ * For the edid blob spec look here:
+ *    https://en.wikipedia.org/wiki/Extended_Display_Identification_Data
+ *
+ * On linux systems you can find the EDID blob in sysfs:
+ *    /sys/class/drm/${card}/${connector}/edid
+ *
+ * You can use the edid-decode ulility (comes with xorg-x11-utils) to
+ * decode the EDID blob.
+ *
+ * @edid_offset: location of the edid blob, relative to the
+ *               start of the region (readonly).
+ * @edid_max_size: max size of the edid blob (readonly).
+ * @edid_size: actual edid size (read/write).
+ * @link_state: display link state (read/write).
+ * VFIO_DEVICE_GFX_LINK_STATE_UP: Monitor is turned on.
+ * VFIO_DEVICE_GFX_LINK_STATE_DOWN: Monitor is turned off.
+ * @max_xres: max display width (0 == no limitation, readonly).
+ * @max_yres: max display height (0 == no limitation, readonly).
+ *
+ * EDID update protocol:
+ *   (1) set link-state to down.
+ *   (2) update edid blob and size.
+ *   (3) set link-state to up.
+ */
+struct vfio_region_gfx_edid {
+	__u32 edid_offset;
+	__u32 edid_max_size;
+	__u32 edid_size;
+	__u32 max_xres;
+	__u32 max_yres;
+	__u32 link_state;
+#define VFIO_DEVICE_GFX_LINK_STATE_UP    1
+#define VFIO_DEVICE_GFX_LINK_STATE_DOWN  2
+};
+
 /*
  * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
  * which allows direct access to non-MSIX registers which happened to be within
-- 
2.9.3

[Qemu-devel] [PATCH 2/5] 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             | 118 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 121 insertions(+)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 1b434d02f6..ff5c425048 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 dead30e626..0ef4d77e21 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -15,6 +15,7 @@
 #include <sys/ioctl.h>
 
 #include "sysemu/sysemu.h"
+#include "hw/display/edid.h"
 #include "ui/console.h"
 #include "qapi/error.h"
 #include "pci.h"
@@ -24,6 +25,120 @@
 # 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;
+    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(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
+
+    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);
+
+    dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
+    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_size);
+    if (pwrite(vdev->vbasedev.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(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
+    return;
+
+err:
+    fprintf(stderr, "%s: Oops, pwrite error\n", __func__);
+    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 ret;
+
+    ret = vfio_get_dev_region_info(&vdev->vbasedev,
+                                   VFIO_REGION_TYPE_GFX,
+                                   VFIO_REGION_SUBTYPE_GFX_EDID,
+                                   &dpy->edid_info);
+    if (ret) {
+        return;
+    }
+
+    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
+    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_offset);
+    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_max_size);
+    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, max_xres);
+    pread_field(vdev->vbasedev.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:
+    fprintf(stderr, "%s: Oops, pread error\n", __func__);
+    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 +286,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 +303,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 +483,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);
 }
-- 
2.9.3

[Qemu-devel] [PATCH 3/5] 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 | 10 ++++++++--
 hw/vfio/pci.c     |  2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index b1ae4c0754..c11c3f1670 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 0ef4d77e21..3a10072823 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -43,8 +43,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
     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;
@@ -120,6 +120,12 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
     pread_field(vdev->vbasedev.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 c0cb1ec289..6f9b6992fc 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3182,6 +3182,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 4/5] 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             | 22 ++++++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index ff5c425048..9e29d5810e 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 3a10072823..a3a710b3ee 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -37,6 +37,19 @@
         goto err;
 
 
+static void vfio_display_edid_link_up(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+    VFIODisplay *dpy = vdev->dpy;
+
+    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
+    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
+    return;
+
+err:
+    fprintf(stderr, "%s: Oops, pwrite error\n", __func__);
+}
+
 static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int prefx, int prefy)
 {
     VFIODisplay *dpy = vdev->dpy;
@@ -47,6 +60,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
         .prefy = prefy ?: vdev->display_yres,
     };
 
+    timer_del(dpy->edid_link_timer);
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
     pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
 
@@ -72,8 +86,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
         goto err;
     }
 
-    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
-    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
+    timer_mod(dpy->edid_link_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 100);
     return;
 
 err:
@@ -126,6 +139,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
     if (!vdev->display_yres)
         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;
 
@@ -143,6 +159,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

[Qemu-devel] [PATCH 5/5] [debug] some logging

[Gerd Hoffmann]

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

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index a3a710b3ee..96b5474243 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -42,6 +42,8 @@ static void vfio_display_edid_link_up(void *opaque)
     VFIOPCIDevice *vdev = opaque;
     VFIODisplay *dpy = vdev->dpy;
 
+    fprintf(stderr, "%s:\n", __func__);
+
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
     pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
     return;
@@ -60,6 +62,9 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
         .prefy = prefy ?: vdev->display_yres,
     };
 
+    fprintf(stderr, "%s: ui info: %dx%d, %s\n", __func__,
+            prefx, prefy, enabled ? "on" : "off");
+
     timer_del(dpy->edid_link_timer);
     dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
     pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
@@ -74,6 +79,9 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
     if (edid.maxy && edid.prefy > edid.maxy) {
         edid.prefy = edid.maxy;
     }
+    fprintf(stderr, "%s: edid: %dx%d\n", __func__,
+            edid.prefx, edid.prefy);
+
     qemu_edid_generate(dpy->edid_blob,
                        dpy->edid_regs->edid_max_size,
                        &edid);
@@ -123,6 +131,8 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                    VFIO_REGION_SUBTYPE_GFX_EDID,
                                    &dpy->edid_info);
     if (ret) {
+        fprintf(stderr, "%s: no edid region (%s)\n",
+                __func__, strerror(errno));
         return;
     }
 
@@ -132,6 +142,12 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
     pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, max_xres);
     pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, max_yres);
     dpy->edid_blob = g_malloc0(dpy->edid_regs->edid_max_size);
+    fprintf(stderr, "%s: edid region: offset %d, size %d, max-res %dx%d\n",
+            __func__,
+            dpy->edid_regs->edid_offset,
+            dpy->edid_regs->edid_max_size,
+            dpy->edid_regs->max_xres,
+            dpy->edid_regs->max_yres);
 
     /* if xres + yres properties are unset use the maximum resolution */
     if (!vdev->display_xres)
-- 
2.9.3

Re: [Qemu-devel] [PATCH 1/5] vfio: update kernel headers.

[Eric Blake]

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

On 1/11/19 3:31 AM, Gerd Hoffmann wrote:
> Temporary, for testing convinience.

convenience

> 
> Once qemu master is synced with the linux 5.0-rc1 headers
> this is not needed any more.

What's the timeline for when that patch will happen, compared to whether
we want this one now followed by a revert?

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  linux-headers/linux/vfio.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 5/5] [debug] some logging

[Eric Blake]

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

On 1/11/19 3:31 AM, Gerd Hoffmann wrote:
> ---
>  hw/vfio/display.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index a3a710b3ee..96b5474243 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -42,6 +42,8 @@ static void vfio_display_edid_link_up(void *opaque)
>      VFIOPCIDevice *vdev = opaque;
>      VFIODisplay *dpy = vdev->dpy;
>  
> +    fprintf(stderr, "%s:\n", __func__);

Why fprintf() instead of trace points?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 0/5] vfio/display: add edid support.

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190111093116.17188-1-kraxel@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 0/5] vfio/display: add edid support.
Type: series
Message-id: 20190111093116.17188-1-kraxel@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20190113143641.38936-1-lifei1214@126.com -> patchew/20190113143641.38936-1-lifei1214@126.com
Switched to a new branch 'test'
d3b83e9 some logging
46b3123 vfio/display: delay link up event
6ae7726 vfio/display: add xres + yres properties
d8234fb vfio/display: add edid support.
45e1bc1 vfio: update kernel headers.

=== OUTPUT BEGIN ===
1/5 Checking commit 45e1bc16552c (vfio: update kernel headers.)
2/5 Checking commit d8234fb5723c (vfio/display: add edid support.)
ERROR: line over 90 characters
#44: FILE: hw/vfio/display.c:40:
+static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int prefx, int prefy)

WARNING: line over 80 characters
#122: FILE: hw/vfio/display.c:118:
+    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_max_size);

ERROR: braces {} are necessary for all arms of this statement
#139: FILE: hw/vfio/display.c:135:
+    if (!dpy->edid_regs)
[...]

total: 2 errors, 1 warnings, 156 lines checked

Patch 2/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/5 Checking commit 6ae77261d5f3 (vfio/display: add xres + yres properties)
ERROR: braces {} are necessary for all arms of this statement
#34: FILE: hw/vfio/display.c:124:
+    if (!vdev->display_xres)
[...]

ERROR: braces {} are necessary for all arms of this statement
#36: FILE: hw/vfio/display.c:126:
+    if (!vdev->display_yres)
[...]

total: 2 errors, 0 warnings, 38 lines checked

Patch 3/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

4/5 Checking commit 46b312391ae4 (vfio/display: delay link up event)
WARNING: line over 80 characters
#51: FILE: hw/vfio/display.c:89:
+    timer_mod(dpy->edid_link_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 100);

total: 0 errors, 1 warnings, 59 lines checked

Patch 4/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
5/5 Checking commit d3b83e9d78a1 (some logging)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 46 lines checked

Patch 5/5 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190111093116.17188-1-kraxel@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 1/5] vfio: update kernel headers.

[Gerd Hoffmann]

On Fri, Jan 11, 2019 at 09:32:43AM -0600, Eric Blake wrote:
> On 1/11/19 3:31 AM, Gerd Hoffmann wrote:
> > Temporary, for testing convinience.
> 
> convenience
> 
> > 
> > Once qemu master is synced with the linux 5.0-rc1 headers
> > this is not needed any more.
> 
> What's the timeline for when that patch will happen, compared to whether
> we want this one now followed by a revert?

Paolo already posted patches to the list, I expect it to be merged within
days.  So, easiest would be to just wait for that to happen, then drop
the patch.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 5/5] [debug] some logging

[Gerd Hoffmann]

On Fri, Jan 11, 2019 at 09:33:40AM -0600, Eric Blake wrote:
> On 1/11/19 3:31 AM, Gerd Hoffmann wrote:
> > ---
> >  hw/vfio/display.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index a3a710b3ee..96b5474243 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -42,6 +42,8 @@ static void vfio_display_edid_link_up(void *opaque)
> >      VFIOPCIDevice *vdev = opaque;
> >      VFIODisplay *dpy = vdev->dpy;
> >  
> > +    fprintf(stderr, "%s:\n", __func__);
> 
> Why fprintf() instead of trace points?

Easier for temporary stuff.  I don't plan to actually merge this one.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/5] vfio/display: add edid support.

[Alex Williamson]

On Fri, 11 Jan 2019 10:31:13 +0100
Gerd Hoffmann <kraxel@redhat.com> 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             | 118 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 121 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 1b434d02f6..ff5c425048 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 dead30e626..0ef4d77e21 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -15,6 +15,7 @@
>  #include <sys/ioctl.h>
>  
>  #include "sysemu/sysemu.h"
> +#include "hw/display/edid.h"
>  #include "ui/console.h"
>  #include "qapi/error.h"
>  #include "pci.h"
> @@ -24,6 +25,120 @@
>  # 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)

Patchew noted this line too long, please wrap.

> +{
> +    VFIODisplay *dpy = vdev->dpy;
> +    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(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
> +
> +    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);
> +
> +    dpy->edid_regs->edid_size = qemu_edid_size(dpy->edid_blob);
> +    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_size);
> +    if (pwrite(vdev->vbasedev.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(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    return;
> +
> +err:
> +    fprintf(stderr, "%s: Oops, pwrite error\n", __func__);

These are not the most helpful error messages and ought to be at least
using error_report() rather than dumping directly to stderr.  How about
some tracing support too?  Tracing feature init and link/resolution
state changes could be useful.

> +    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 ret;
> +
> +    ret = vfio_get_dev_region_info(&vdev->vbasedev,
> +                                   VFIO_REGION_TYPE_GFX,
> +                                   VFIO_REGION_SUBTYPE_GFX_EDID,
> +                                   &dpy->edid_info);
> +    if (ret) {
> +        return;
> +    }
> +
> +    dpy->edid_regs = g_new0(struct vfio_region_gfx_edid, 1);
> +    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_offset);
> +    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, edid_max_size);

Patchew didn't like this long line either.

> +    pread_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, max_xres);
> +    pread_field(vdev->vbasedev.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:
> +    fprintf(stderr, "%s: Oops, pread error\n", __func__);
> +    g_free(dpy->edid_regs);
> +    dpy->edid_regs = NULL;
> +    return;
> +}
> +
> +static void vfio_display_edid_exit(VFIODisplay *dpy)
> +{
> +    if (!dpy->edid_regs)
> +        return;

Excessive curly braces as demanded by our style guide please ;)

Thanks,
Alex

> +
> +    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 +286,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 +303,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 +483,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);
>  }

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

[Alex Williamson]

On Fri, 11 Jan 2019 10:31:14 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> 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 | 10 ++++++++--
>  hw/vfio/pci.c     |  2 ++
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index b1ae4c0754..c11c3f1670 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 0ef4d77e21..3a10072823 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -43,8 +43,8 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
>      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;
> @@ -120,6 +120,12 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>      pread_field(vdev->vbasedev.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;

Excessive curly braces here as well please.

> +
>      vfio_display_edid_update(vdev, true, 0, 0);
>      return;
>  
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index c0cb1ec289..6f9b6992fc 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3182,6 +3182,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),

We're committing to supporting this then vs the x- prefix?  Shouldn't
these options generate an error if there's no display device or there's
no EDID region support to back the option?  Thanks,

Alex

>      DEFINE_PROP_UINT32("x-intx-mmap-timeout-ms", VFIOPCIDevice,
>                         intx.mmap_timeout, 1100),
>      DEFINE_PROP_BIT("x-vga", VFIOPCIDevice, features,

Re: [Qemu-devel] [PATCH 4/5] vfio/display: delay link up event

[Alex Williamson]

On Fri, 11 Jan 2019 10:31:15 +0100
Gerd Hoffmann <kraxel@redhat.com> 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>
> ---
>  include/hw/vfio/vfio-common.h |  1 +
>  hw/vfio/display.c             | 22 ++++++++++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index ff5c425048..9e29d5810e 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 3a10072823..a3a710b3ee 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -37,6 +37,19 @@
>          goto err;
>  
>  
> +static void vfio_display_edid_link_up(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +    VFIODisplay *dpy = vdev->dpy;
> +
> +    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> +    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    return;
> +
> +err:
> +    fprintf(stderr, "%s: Oops, pwrite error\n", __func__);

Tracing an error_report again, and patchew found a long line somewhere
below.  Thanks,

Alex

> +}
> +
>  static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int prefx, int prefy)
>  {
>      VFIODisplay *dpy = vdev->dpy;
> @@ -47,6 +60,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
>          .prefy = prefy ?: vdev->display_yres,
>      };
>  
> +    timer_del(dpy->edid_link_timer);
>      dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_DOWN;
>      pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
>  
> @@ -72,8 +86,7 @@ static void vfio_display_edid_update(VFIOPCIDevice *vdev, bool enabled, int pref
>          goto err;
>      }
>  
> -    dpy->edid_regs->link_state = VFIO_DEVICE_GFX_LINK_STATE_UP;
> -    pwrite_field(vdev->vbasedev.fd, dpy->edid_info, dpy->edid_regs, link_state);
> +    timer_mod(dpy->edid_link_timer, qemu_clock_get_ms(QEMU_CLOCK_REALTIME) + 100);
>      return;
>  
>  err:
> @@ -126,6 +139,9 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>      if (!vdev->display_yres)
>          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;
>  
> @@ -143,6 +159,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,