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

[Qemu-devel] [PATCH v2 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.

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             | 159 ++++++++++++++++++++++++++++++++++++++++++
 hw/vfio/pci.c                 |   2 +
 hw/vfio/trace-events          |   7 ++
 5 files changed, 174 insertions(+)

-- 
2.9.3

[Qemu-devel] [PATCH v2 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 7624c9f511..5f7f709b95 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..ed2eb19ea3 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:
+    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 +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 f41ca96160..28d445caaf 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

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

[Alex Williamson]

On Wed, 20 Feb 2019 09:47:51 +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.

What are the requirements to enable this resizing feature?  I grabbed
the gvt-next-2019-02-01 branch and my ever expanding qemu:commandline
now looks like this:

  <qemu:commandline>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.ramfb=on'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.driver=vfio-pci-nohotplug'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.xres=1600'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.hostdev0.yres=900'/>
  </qemu:commandline>

Other relevant sections:

    <graphics type='spice'>
      <listen type='none'/>
      <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/>
    </graphics>
    <video>
      <model type='none'/>
      <alias name='video0'/>
    </video>
    <hostdev mode='subsystem' type='mdev' managed='no' model='vfio-pci' display='on'>
      <source>
        <address uuid='0b2d768f-95ac-4c7e-a20e-dc002b7381fd'/>
      </source>
      <alias name='hostdev0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/>
    </hostdev>

The 1600x900 is used, which is great, but neither virt-manager nor
virt-viewer are giving me any automatic resizing as this seems to
suggest it should.

One nit and one bug below.  Thanks,

Alex

> 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 7624c9f511..5f7f709b95 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..ed2eb19ea3 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;

nit, no unwind and only one call point, could probably do without the
goto.

> +}
> +
> +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:
> +    fprintf(stderr, "%s: Oops, pread error\n", __func__);
> +    g_free(dpy->edid_regs);
> +    dpy->edid_regs = NULL;
> +    return;

This code is unreachable.

> +}
> +
> +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 f41ca96160..28d445caaf 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) ""

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

[Gerd Hoffmann]

On Wed, Feb 20, 2019 at 02:54:35PM -0700, Alex Williamson wrote:
> On Wed, 20 Feb 2019 09:47:51 +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.
> 
> What are the requirements to enable this resizing feature?  I grabbed
> the gvt-next-2019-02-01 branch and my ever expanding qemu:commandline
> now looks like this:
> 
>   <qemu:commandline>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.ramfb=on'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.driver=vfio-pci-nohotplug'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.xres=1600'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.hostdev0.yres=900'/>
>   </qemu:commandline>
> 
> Other relevant sections:
> 
>     <graphics type='spice'>
>       <listen type='none'/>
>       <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/>
>     </graphics>

When using spice you also need the spicevmc channel and the spice agent
being installed and active in the guest.

> > +    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;
> 
> nit, no unwind and only one call point, could probably do without the
> goto.

Not that easily due to the goto being hidden in the pwrite_field()
macro.

> > +    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:
> > +    fprintf(stderr, "%s: Oops, pread error\n", __func__);
> > +    g_free(dpy->edid_regs);
> > +    dpy->edid_regs = NULL;
> > +    return;
> 
> This code is unreachable.

It's not.  Again, the goto is in pread_field.

But I just noticed I missed one fprintf which should be a
trace_vfio_display_edid_write_error() ...

cheers,
  Gerd

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

[Alex Williamson]

On Thu, 21 Feb 2019 08:38:50 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Wed, Feb 20, 2019 at 02:54:35PM -0700, Alex Williamson wrote:
> > On Wed, 20 Feb 2019 09:47:51 +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.  
> > 
> > What are the requirements to enable this resizing feature?  I grabbed
> > the gvt-next-2019-02-01 branch and my ever expanding qemu:commandline
> > now looks like this:
> > 
> >   <qemu:commandline>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.x-igd-opregion=on'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.ramfb=on'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.driver=vfio-pci-nohotplug'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.xres=1600'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.hostdev0.yres=900'/>
> >   </qemu:commandline>
> > 
> > Other relevant sections:
> > 
> >     <graphics type='spice'>
> >       <listen type='none'/>
> >       <gl enable='yes' rendernode='/dev/dri/by-path/pci-0000:00:02.0-render'/>
> >     </graphics>  
> 
> When using spice you also need the spicevmc channel and the spice agent
> being installed and active in the guest.

This is the vdagent using installation like this:

https://www.ovirt.org/develop/infra/testing/spice.html

ie. vdservice install, net start vdservice?

I'm not seeing anything magically change when I do that.  I do have the
default serial and redirection devices installed by virt-manager:

     <serial type='pty'>
      <source path='/dev/pts/4'/>
      <target type='isa-serial' port='0'>
        <model name='isa-serial'/>
      </target>
      <alias name='serial0'/>
    </serial>
    <console type='pty' tty='/dev/pts/4'>
      <source path='/dev/pts/4'/>
      <target type='serial' port='0'/>
      <alias name='serial0'/>
    </console>
    <redirdev bus='usb' type='spicevmc'>
      <alias name='redir0'/>
      <address type='usb' bus='0' port='2'/>
    </redirdev>
    <redirdev bus='usb' type='spicevmc'>
      <alias name='redir1'/>
      <address type='usb' bus='0' port='3'/>
    </redirdev>

I believe I have drivers installed, but it's a bit difficult to verify
given the Intel graphics glitches I copied you on.

> > > +    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;  
> > 
> > nit, no unwind and only one call point, could probably do without the
> > goto.  
> 
> Not that easily due to the goto being hidden in the pwrite_field()
> macro.

Ah, I knew those macros were going to be tricky, but I fell for it
anyway.  Sorry.

> > > +    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:
> > > +    fprintf(stderr, "%s: Oops, pread error\n", __func__);
> > > +    g_free(dpy->edid_regs);
> > > +    dpy->edid_regs = NULL;
> > > +    return;  
> > 
> > This code is unreachable.  
> 
> It's not.  Again, the goto is in pread_field.
> 
> But I just noticed I missed one fprintf which should be a
> trace_vfio_display_edid_write_error() ...

Yep.

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

[Gerd Hoffmann]

  Hi,

> This is the vdagent using installation like this:
> 
> https://www.ovirt.org/develop/infra/testing/spice.html
> 
> ie. vdservice install, net start vdservice?

If the page says so, probably.

I've tested the agent setup with linux only, where everything happens
automatically, you only have to make sure spice-vdagent.rpm is installed.

> I'm not seeing anything magically change when I do that.  I do have the
> default serial and redirection devices installed by virt-manager:

This one too?

    <channel type='spicevmc'>
      <target type='virtio' name='com.redhat.spice.0'/>
      <address type='virtio-serial' controller='0' bus='0' port='2'/>
    </channel>

cheers,
  Gerd

[Qemu-devel] [PATCH v2 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 | 16 ++++++++++++++--
 hw/vfio/pci.c     |  2 ++
 3 files changed, 18 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 ed2eb19ea3..7b9b604a64 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;
@@ -117,6 +117,10 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
                                    VFIO_REGION_SUBTYPE_GFX_EDID,
                                    &dpy->edid_info);
     if (ret) {
+        if (vdev->display_xres || vdev->display_yres) {
+            warn_report("vfio: no edid support available, "
+                        "xres and yres properties have no effect.");
+        }
         return;
     }
 
@@ -128,6 +132,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 dd12f36391..edb8394038 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

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

[Alex Williamson]

On Wed, 20 Feb 2019 09:47:52 +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 | 16 ++++++++++++++--
>  hw/vfio/pci.c     |  2 ++
>  3 files changed, 18 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 ed2eb19ea3..7b9b604a64 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;
> @@ -117,6 +117,10 @@ static void vfio_display_edid_init(VFIOPCIDevice *vdev)
>                                     VFIO_REGION_SUBTYPE_GFX_EDID,
>                                     &dpy->edid_info);
>      if (ret) {
> +        if (vdev->display_xres || vdev->display_yres) {
> +            warn_report("vfio: no edid support available, "
> +                        "xres and yres properties have no effect.");
> +        }

In order to get here the device needs to have a display option set to
'on' or 'auto' and that display needs to be backed by a dmabuf graphics
plane.  That means that QEMU is happy to run without any warning if a
user sets a resolution on a region backed display, or a device without
a display.  I think that QEMU should probably fail, not just warn, for
all cases where an option is not appropriate for a device.  Perhaps
EDID setup should set a feature bit or flag that we can test similar to
how and where we test for a stray ramfb option.

>          return;
>      }
>  
> @@ -128,6 +132,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 dd12f36391..edb8394038 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),

This is actually quite fun, I started my VM with arbitrary numbers and
the Windows GUI honored it every time.  Probably very useful for
playing with odd screen sizes.  I also tried to break it using
1000000x1000000, but the display came up as 1920x1200, the maximum
resolution GVT-g supports for this type.  I don't see that QEMU is
bounding this though, do we depend on the mdev device to ignore it if
we pass values it cannot support?  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 v2 2/3] vfio/display: add xres + yres properties

[Gerd Hoffmann]

  Hi,

> > +    DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> > +    DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),
> 
> This is actually quite fun, I started my VM with arbitrary numbers and
> the Windows GUI honored it every time.  Probably very useful for
> playing with odd screen sizes.  I also tried to break it using
> 1000000x1000000, but the display came up as 1920x1200, the maximum
> resolution GVT-g supports for this type.  I don't see that QEMU is
> bounding this though, do we depend on the mdev device to ignore it if
> we pass values it cannot support?

There is a check in vfio_display_edid_update().

cheers,
  Gerd

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

[Alex Williamson]

On Thu, 21 Feb 2019 08:46:43 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
> 
> > > +    DEFINE_PROP_UINT32("xres", VFIOPCIDevice, display_xres, 0),
> > > +    DEFINE_PROP_UINT32("yres", VFIOPCIDevice, display_yres, 0),  
> > 
> > This is actually quite fun, I started my VM with arbitrary numbers and
> > the Windows GUI honored it every time.  Probably very useful for
> > playing with odd screen sizes.  I also tried to break it using
> > 1000000x1000000, but the display came up as 1920x1200, the maximum
> > resolution GVT-g supports for this type.  I don't see that QEMU is
> > bounding this though, do we depend on the mdev device to ignore it if
> > we pass values it cannot support?  
> 
> There is a check in vfio_display_edid_update().

Ok, so we're bounded within QEMU, that seems good.  Thanks,

Alex

[Qemu-devel] [PATCH v2 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 5f7f709b95..b65a2f0518 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 7b9b604a64..361823b23b 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:
@@ -140,6 +155,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;
 
@@ -158,6 +176,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 v2 3/3] vfio/display: delay link up event

[Alex Williamson]

On Wed, 20 Feb 2019 09:47:53 +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             | 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 5f7f709b95..b65a2f0518 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 7b9b604a64..361823b23b 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();

No jumps to here.  Thanks,

Alex

> +}
> +
>  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:
> @@ -140,6 +155,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;
>  
> @@ -158,6 +176,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,