[RfC,v3,5/5] vfio/display: adding region support

Message ID 20180213161846.26914-6-kraxel@redhat.com
State New
Headers show
Series
  • vfio: add display support
Related show

Commit Message

Gerd Hoffmann Feb. 13, 2018, 4:18 p.m.
Wire up region-based display.

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

Comments

Alex Williamson Feb. 13, 2018, 8:40 p.m. | #1
On Tue, 13 Feb 2018 17:18:46 +0100
Gerd Hoffmann <kraxel@redhat.com> wrote:

> Wire up region-based display.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/vfio/pci.h                 |   1 +
>  include/hw/vfio/vfio-common.h |   8 ++++
>  hw/vfio/display.c             | 102 +++++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 1d005d922d..9fe0f3f198 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -148,6 +148,7 @@ typedef struct VFIOPCIDevice {
>      bool no_kvm_msi;
>      bool no_kvm_msix;
>      bool no_geforce_quirks;
> +    VFIODisplay *dpy;
>  } VFIOPCIDevice;
>  
>  uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index f3a2ac9fee..fc8ae14fb7 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -142,6 +142,14 @@ typedef struct VFIOGroup {
>      QLIST_ENTRY(VFIOGroup) container_next;
>  } VFIOGroup;
>  
> +typedef struct VFIODisplay {
> +    QemuConsole *con;
> +    struct {
> +        VFIORegion buffer;
> +        DisplaySurface *surface;
> +    } region;
> +} VFIODisplay;
> +
>  void vfio_put_base_device(VFIODevice *vbasedev);
>  void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
>  void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index 4249be398d..819a0cb08c 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -19,6 +19,105 @@
>  #include "qapi/error.h"
>  #include "pci.h"
>  
> +/* ---------------------------------------------------------------------- */
> +
> +static void vfio_display_region_update(void *opaque)
> +{
> +    VFIOPCIDevice *vdev = opaque;
> +    VFIODisplay *dpy = vdev->dpy;
> +    struct vfio_device_gfx_plane_info plane = {
> +        .argsz = sizeof(plane),
> +        .flags = VFIO_GFX_PLANE_TYPE_REGION
> +    };
> +    pixman_format_code_t format = PIXMAN_x8r8g8b8;

nit, unused initialization

> +    int ret;
> +
> +    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &plane);
> +    if (ret < 0) {
> +        fprintf(stderr, "ioctl VFIO_DEVICE_QUERY_GFX_PLANE: %s\n",
> +                strerror(errno));

nit, should this be an error_report()?

> +        return;
> +    }
> +    if (!plane.drm_format || !plane.size) {
> +        return;
> +    }
> +    format = qemu_drm_format_to_pixman(plane.drm_format);
> +    if (!format) {
> +        return;
> +    }
> +
> +    if (dpy->region.buffer.size &&
> +        dpy->region.buffer.nr != plane.region_index) {
> +        /* region changed */
> +        vfio_region_exit(&dpy->region.buffer);

Don't we need to also call vfio_region_finalize()?  _exit only deletes
mmap MemoryRegions from the base MemoryRegion, _finalize does the
actual munmap, unparent calls, and frees dynamic memory.  As is, this
leaks memory afaict.

> +        memset(&dpy->region.buffer, 0, sizeof(dpy->region.buffer));

_finalize could cleanup the VFIORegion rather than requiring this
memset, we simply haven't needed it yet since regions are generally
tied to the life cycle of the device.

> +        dpy->region.surface = NULL;
> +    }
> +
> +    if (dpy->region.surface &&
> +        (surface_width(dpy->region.surface) != plane.width ||
> +         surface_height(dpy->region.surface) != plane.height ||
> +         surface_format(dpy->region.surface) != format)) {
> +        /* size changed */
> +        dpy->region.surface = NULL;
> +    }
> +
> +    if (!dpy->region.buffer.size) {
> +        /* mmap region */
> +        ret = vfio_region_setup(OBJECT(vdev), &vdev->vbasedev,
> +                                &dpy->region.buffer,
> +                                plane.region_index,
> +                                "display");
> +        if (ret != 0) {
> +            fprintf(stderr, "%s: vfio_region_setup(%d): %s\n",
> +                    __func__, plane.region_index, strerror(-ret));
> +            goto err1;
> +        }
> +        ret = vfio_region_mmap(&dpy->region.buffer);
> +        if (ret != 0) {
> +            fprintf(stderr, "%s: vfio_region_mmap(%d): %s\n", __func__,
> +                    plane.region_index, strerror(-ret));
> +            goto err2;
> +        }
> +        assert(dpy->region.buffer.mmaps[0].mmap != NULL);
> +    }
> +
> +    if (dpy->region.surface == NULL) {
> +        /* create surface */
> +        dpy->region.surface = qemu_create_displaysurface_from
> +            (plane.width, plane.height, format,
> +             plane.stride, dpy->region.buffer.mmaps[0].mmap);
> +        dpy_gfx_replace_surface(dpy->con, dpy->region.surface);
> +    }
> +
> +    /* full screen update */
> +    dpy_gfx_update(dpy->con, 0, 0,
> +                   surface_width(dpy->region.surface),
> +                   surface_height(dpy->region.surface));
> +    return;
> +
> +err2:
> +    vfio_region_exit(&dpy->region.buffer);

This one probably needs to be replaced with _finalize, if _mmap fails,
_exit doesn't do anything useful.

> +err1:
> +    memset(&dpy->region.buffer, 0, sizeof(dpy->region.buffer));

vfio_region_setup has no failure path that modifies the provided
VFIORegion, so this is unnecessary if _finalize is called and cleans up.

> +}
> +
> +static const GraphicHwOps vfio_display_region_ops = {
> +    .gfx_update = vfio_display_region_update,
> +};
> +
> +static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> +{
> +    vdev->dpy = g_new0(VFIODisplay, 1);
> +    vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
> +                                          &vfio_display_region_ops,
> +                                          vdev);
> +    /* TODO: disable hotplug (there is no graphic_console_close) */

via DeviceClass.hotpluggable?  I know that QE will immediately test
this, so it needs to be sorted out sooner than later.

> +    return 0;
> +}
> +
> +/* ---------------------------------------------------------------------- */
> +
>  int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
>  {
>      struct vfio_device_gfx_plane_info probe;
> @@ -38,8 +137,7 @@ int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
>      probe.flags = VFIO_GFX_PLANE_TYPE_PROBE | VFIO_GFX_PLANE_TYPE_REGION;
>      ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &probe);
>      if (ret == 0) {
> -        error_setg(errp, "vfio-display: region support not implemented yet");
> -        return -1;
> +        return vfio_display_region_init(vdev, errp);
>      }
>  
>      if (vdev->display == ON_OFF_AUTO_AUTO) {
Gerd Hoffmann Feb. 16, 2018, 2:01 p.m. | #2
> > +static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
> > +{
> > +    vdev->dpy = g_new0(VFIODisplay, 1);
> > +    vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
> > +                                          &vfio_display_region_ops,
> > +                                          vdev);
> > +    /* TODO: disable hotplug (there is no graphic_console_close) */
> 
> via DeviceClass.hotpluggable?  I know that QE will immediately test
> this, so it needs to be sorted out sooner than later.

Hmm, I think setting DeviceClass.hotpluggable would tag all vfio devices
as not hotpluggable, not only the one we just created a display for.

cheers,
  Gerd

Patch

diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 1d005d922d..9fe0f3f198 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -148,6 +148,7 @@  typedef struct VFIOPCIDevice {
     bool no_kvm_msi;
     bool no_kvm_msix;
     bool no_geforce_quirks;
+    VFIODisplay *dpy;
 } VFIOPCIDevice;
 
 uint32_t vfio_pci_read_config(PCIDevice *pdev, uint32_t addr, int len);
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index f3a2ac9fee..fc8ae14fb7 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -142,6 +142,14 @@  typedef struct VFIOGroup {
     QLIST_ENTRY(VFIOGroup) container_next;
 } VFIOGroup;
 
+typedef struct VFIODisplay {
+    QemuConsole *con;
+    struct {
+        VFIORegion buffer;
+        DisplaySurface *surface;
+    } region;
+} VFIODisplay;
+
 void vfio_put_base_device(VFIODevice *vbasedev);
 void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
 void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 4249be398d..819a0cb08c 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -19,6 +19,105 @@ 
 #include "qapi/error.h"
 #include "pci.h"
 
+/* ---------------------------------------------------------------------- */
+
+static void vfio_display_region_update(void *opaque)
+{
+    VFIOPCIDevice *vdev = opaque;
+    VFIODisplay *dpy = vdev->dpy;
+    struct vfio_device_gfx_plane_info plane = {
+        .argsz = sizeof(plane),
+        .flags = VFIO_GFX_PLANE_TYPE_REGION
+    };
+    pixman_format_code_t format = PIXMAN_x8r8g8b8;
+    int ret;
+
+    ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &plane);
+    if (ret < 0) {
+        fprintf(stderr, "ioctl VFIO_DEVICE_QUERY_GFX_PLANE: %s\n",
+                strerror(errno));
+        return;
+    }
+    if (!plane.drm_format || !plane.size) {
+        return;
+    }
+    format = qemu_drm_format_to_pixman(plane.drm_format);
+    if (!format) {
+        return;
+    }
+
+    if (dpy->region.buffer.size &&
+        dpy->region.buffer.nr != plane.region_index) {
+        /* region changed */
+        vfio_region_exit(&dpy->region.buffer);
+        memset(&dpy->region.buffer, 0, sizeof(dpy->region.buffer));
+        dpy->region.surface = NULL;
+    }
+
+    if (dpy->region.surface &&
+        (surface_width(dpy->region.surface) != plane.width ||
+         surface_height(dpy->region.surface) != plane.height ||
+         surface_format(dpy->region.surface) != format)) {
+        /* size changed */
+        dpy->region.surface = NULL;
+    }
+
+    if (!dpy->region.buffer.size) {
+        /* mmap region */
+        ret = vfio_region_setup(OBJECT(vdev), &vdev->vbasedev,
+                                &dpy->region.buffer,
+                                plane.region_index,
+                                "display");
+        if (ret != 0) {
+            fprintf(stderr, "%s: vfio_region_setup(%d): %s\n",
+                    __func__, plane.region_index, strerror(-ret));
+            goto err1;
+        }
+        ret = vfio_region_mmap(&dpy->region.buffer);
+        if (ret != 0) {
+            fprintf(stderr, "%s: vfio_region_mmap(%d): %s\n", __func__,
+                    plane.region_index, strerror(-ret));
+            goto err2;
+        }
+        assert(dpy->region.buffer.mmaps[0].mmap != NULL);
+    }
+
+    if (dpy->region.surface == NULL) {
+        /* create surface */
+        dpy->region.surface = qemu_create_displaysurface_from
+            (plane.width, plane.height, format,
+             plane.stride, dpy->region.buffer.mmaps[0].mmap);
+        dpy_gfx_replace_surface(dpy->con, dpy->region.surface);
+    }
+
+    /* full screen update */
+    dpy_gfx_update(dpy->con, 0, 0,
+                   surface_width(dpy->region.surface),
+                   surface_height(dpy->region.surface));
+    return;
+
+err2:
+    vfio_region_exit(&dpy->region.buffer);
+err1:
+    memset(&dpy->region.buffer, 0, sizeof(dpy->region.buffer));
+}
+
+static const GraphicHwOps vfio_display_region_ops = {
+    .gfx_update = vfio_display_region_update,
+};
+
+static int vfio_display_region_init(VFIOPCIDevice *vdev, Error **errp)
+{
+    vdev->dpy = g_new0(VFIODisplay, 1);
+    vdev->dpy->con = graphic_console_init(DEVICE(vdev), 0,
+                                          &vfio_display_region_ops,
+                                          vdev);
+    /* TODO: disable hotplug (there is no graphic_console_close) */
+    return 0;
+}
+
+/* ---------------------------------------------------------------------- */
+
 int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
 {
     struct vfio_device_gfx_plane_info probe;
@@ -38,8 +137,7 @@  int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp)
     probe.flags = VFIO_GFX_PLANE_TYPE_PROBE | VFIO_GFX_PLANE_TYPE_REGION;
     ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_QUERY_GFX_PLANE, &probe);
     if (ret == 0) {
-        error_setg(errp, "vfio-display: region support not implemented yet");
-        return -1;
+        return vfio_display_region_init(vdev, errp);
     }
 
     if (vdev->display == ON_OFF_AUTO_AUTO) {