[Qemu-devel] [PATCH 0/2] Add vfio display reset handling

[Qemu-devel] [PATCH 0/2] Add vfio display reset handling

[Tina Zhang]

vfio display needs to release the invalid display resource and disable
the scanout mode during guest OS reboot, otherwise bugs come out.

Thanks hang.yuan@intel.com for helping root cause the issue.

Tina Zhang (2):
  console: introduce dpy_gfx_switch_surface
  ui: introduce vfio_display_reset

 hw/vfio/display.c    | 20 ++++++++++++++++++++
 hw/vfio/pci.c        |  4 ++++
 hw/vfio/pci.h        |  1 +
 include/ui/console.h |  2 ++
 ui/console.c         | 16 ++++++++++++++++
 5 files changed, 43 insertions(+)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] console: introduce dpy_gfx_switch_surface

[Tina Zhang]

dpy_gfx_switch_surface is used to ask each valid DisplayChangeListener
of a QemuConsole to switch to another DisplaySurface.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 include/ui/console.h |  2 ++
 ui/console.c         | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/ui/console.h b/include/ui/console.h
index 37a8d68..434033d 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -293,6 +293,8 @@ int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info);
 void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h);
 void dpy_gfx_replace_surface(QemuConsole *con,
                              DisplaySurface *surface);
+void dpy_gfx_switch_surface(QemuConsole *con,
+                             DisplaySurface *surface);
 void dpy_text_cursor(QemuConsole *con, int x, int y);
 void dpy_text_update(QemuConsole *con, int x, int y, int w, int h);
 void dpy_text_resize(QemuConsole *con, int w, int h);
diff --git a/ui/console.c b/ui/console.c
index 3fb2f4e..2deb38b 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1595,6 +1595,22 @@ void dpy_gfx_replace_surface(QemuConsole *con,
     qemu_free_displaysurface(old_surface);
 }
 
+void dpy_gfx_switch_surface(QemuConsole *con,
+                             DisplaySurface *surface)
+{
+    DisplayState *s = con->ds;
+    DisplayChangeListener *dcl;
+
+    QLIST_FOREACH(dcl, &s->listeners, next) {
+        if (con != (dcl->con ? dcl->con : active_console)) {
+            continue;
+        }
+        if (dcl->ops->dpy_gfx_switch) {
+            dcl->ops->dpy_gfx_switch(dcl, surface);
+        }
+    }
+}
+
 bool dpy_gfx_check_format(QemuConsole *con,
                           pixman_format_code_t format)
 {
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] ui: introduce vfio_display_reset

[Tina Zhang]

During guest OS reboot, guest framebuffer is invalid. It will cause
bugs, if the invalid guest framebuffer is still used by host.

This patch is to introduce vfio_display_reset which is invoked
during vfio display reset. This vfio_display_reset function is used
to release the invalid display resource, disable scanout mode and
replace the invalid surface with QemuConsole's DisplaySurafce.

This patch can fix the GPU hang issue caused by gd_egl_draw during
guest OS reboot.

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 hw/vfio/display.c | 20 ++++++++++++++++++++
 hw/vfio/pci.c     |  4 ++++
 hw/vfio/pci.h     |  1 +
 3 files changed, 25 insertions(+)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index 7d727ce..65185c7 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -198,6 +198,26 @@ static void vfio_display_dmabuf_exit(VFIODisplay *dpy)
 }
 
 /* ---------------------------------------------------------------------- */
+void vfio_display_reset(VFIOPCIDevice *vdev)
+{
+    DisplaySurface *surface;
+
+    if (!vdev || !vdev->dpy || !vdev->dpy->con) {
+        return;
+    }
+
+    surface = qemu_console_surface(vdev->dpy->con);
+    if (!surface) {
+        return;
+    }
+
+    /* During reset, disable scanout mode and */
+    /* use QemuConsole DisplaySurface */
+    dpy_gl_scanout_disable(vdev->dpy->con);
+    dpy_gfx_switch_surface(vdev->dpy->con, surface);
+
+    vfio_display_dmabuf_exit(vdev->dpy);
+}
 
 static void vfio_display_region_update(void *opaque)
 {
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index b9bc6cd..4947fe3 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -3103,6 +3103,10 @@ static void vfio_pci_reset(DeviceState *dev)
 
     vfio_pci_pre_reset(vdev);
 
+    if (vdev->display != ON_OFF_AUTO_OFF) {
+        vfio_display_reset(vdev);
+    }
+
     if (vdev->resetfn && !vdev->resetfn(vdev)) {
         goto post_reset;
     }
diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
index 629c875..59ab775 100644
--- a/hw/vfio/pci.h
+++ b/hw/vfio/pci.h
@@ -176,6 +176,7 @@ int vfio_pci_igd_opregion_init(VFIOPCIDevice *vdev,
                                struct vfio_region_info *info,
                                Error **errp);
 
+void vfio_display_reset(VFIOPCIDevice *vdev);
 int vfio_display_probe(VFIOPCIDevice *vdev, Error **errp);
 void vfio_display_finalize(VFIOPCIDevice *vdev);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/2] console: introduce dpy_gfx_switch_surface

[Gerd Hoffmann]

 Hi,

>  void dpy_gfx_replace_surface(QemuConsole *con,
>                               DisplaySurface *surface);
> +void dpy_gfx_switch_surface(QemuConsole *con,
> +                             DisplaySurface *surface);

Why?  Any problems with dpy_gfx_replace_surface?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] ui: introduce vfio_display_reset

[Gerd Hoffmann]

> +    surface = qemu_console_surface(vdev->dpy->con);

> +    dpy_gl_scanout_disable(vdev->dpy->con);
> +    dpy_gfx_switch_surface(vdev->dpy->con, surface);

Hmm, so you ask which surface is active, then switch to it.
Why?

Maybe you just need a full display update after disabling the gl
scanout?

cheers,
  Gerd

PS: maybe adding a dpy_gfx_update_full() helper is useful:

diff --git a/include/ui/console.h b/include/ui/console.h
index 37a8d68d29..981b519dde 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -291,6 +291,7 @@ bool dpy_ui_info_supported(QemuConsole *con);
 int dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info);
 
 void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h);
+void dpy_gfx_update_full(QemuConsole *con);
 void dpy_gfx_replace_surface(QemuConsole *con,
                              DisplaySurface *surface);
 void dpy_text_cursor(QemuConsole *con, int x, int y);
diff --git a/ui/console.c b/ui/console.c
index 3fb2f4e09f..b02510cdca 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1574,6 +1574,16 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
     }
 }
 
+void dpy_gfx_update_full(QemuConsole *con)
+{
+    if (!con->surface) {
+        return;
+    }
+    dpy_gfx_update(con, 0, 0,
+                   surface_width(con->surface),
+                   surface_height(con->surface));
+}
+
 void dpy_gfx_replace_surface(QemuConsole *con,
                              DisplaySurface *surface)
 {

Re: [Qemu-devel] [PATCH 2/2] ui: introduce vfio_display_reset

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Monday, April 23, 2018 9:12 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: alex.williamson@redhat.com; intel-gvt-dev@lists.freedesktop.org; qemu-
> devel@nongnu.org; zhenyuw@linux.intel.com; Yuan, Hang
> <hang.yuan@intel.com>
> Subject: Re: [PATCH 2/2] ui: introduce vfio_display_reset
> 
> > +    surface = qemu_console_surface(vdev->dpy->con);
> 
> > +    dpy_gl_scanout_disable(vdev->dpy->con);
> > +    dpy_gfx_switch_surface(vdev->dpy->con, surface);
> 
> Hmm, so you ask which surface is active, then switch to it.
> Why?

What this patch-set is going to do is to disable scanout mode during guest OS reset, so that Qemu UI (e.g. gtk) can directly use QemuConsole's DisplaySurface instead of guest dma-buf framebuffer during the guest OS reset process. 
gd_egl_draw is invodked when the Qemu UI window gets the focus. If this process is happened during guest OS reboot, gd_egl_scanout_flush() will be invoked (as vc->gfx.scanout_mode==true), which uses guest dma-buf framebuffer. See the following logic in gtk-egl.c:
void gd_egl_draw(VirtualConsole *vc)
{
    GdkWindow *window;
    int ww, wh;

    if (!vc->gfx.gls) {
        return;
    }

    if (vc->gfx.scanout_mode) {
        gd_egl_scanout_flush(&vc->gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
    } else {
        if (!vc->gfx.ds) {
            return;
        }
        eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
                       vc->gfx.esurface, vc->gfx.ectx);

        window = gtk_widget_get_window(vc->gfx.drawing_area);
        gdk_drawable_get_size(window, &ww, &wh);
        surface_gl_setup_viewport(vc->gfx.gls, vc->gfx.ds, ww, wh);
        surface_gl_render_texture(vc->gfx.gls, vc->gfx.ds);

        eglSwapBuffers(qemu_egl_display, vc->gfx.esurface);
    }
}

Using guest dma-buf framebuffer during guest OS reboot will lead to host GPU hang, as the GGTT is invalid during guest OS reboot.
That's why we want to disable the scanout mode and release all the dma-buf resources during guest OS reset.

After reviewing this patch-set again, I think we might not need the proposed dpy_gfx_switch_surface() any more.
The reason I proposed it was because I thought gfx.ds is changed to the surface related to guest dma-buf framebuffer and we need to switch it with QemuConsole's DisplaySurgface during reset.
But after reviewing the code, vc->gfx.ds is always pointing to the valid QemuConsoles' DisplaySurface. So we don’t need dpy_gfx_switch_surface() to take care of it.
Thanks.

> 
> Maybe you just need a full display update after disabling the gl scanout?
Do you still think updating full display is needed? It seems the existing vfio_display_dmabuf_update can be helpful enough to create a new texture if needed.
Thanks.


BR,
Tina
> 
> cheers,
>   Gerd
> 
> PS: maybe adding a dpy_gfx_update_full() helper is useful:
> 
> diff --git a/include/ui/console.h b/include/ui/console.h index
> 37a8d68d29..981b519dde 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -291,6 +291,7 @@ bool dpy_ui_info_supported(QemuConsole *con);  int
> dpy_set_ui_info(QemuConsole *con, QemuUIInfo *info);
> 
>  void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h);
> +void dpy_gfx_update_full(QemuConsole *con);
>  void dpy_gfx_replace_surface(QemuConsole *con,
>                               DisplaySurface *surface);  void dpy_text_cursor(QemuConsole
> *con, int x, int y); diff --git a/ui/console.c b/ui/console.c index
> 3fb2f4e09f..b02510cdca 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1574,6 +1574,16 @@ void dpy_gfx_update(QemuConsole *con, int x, int
> y, int w, int h)
>      }
>  }
> 
> +void dpy_gfx_update_full(QemuConsole *con) {
> +    if (!con->surface) {
> +        return;
> +    }
> +    dpy_gfx_update(con, 0, 0,
> +                   surface_width(con->surface),
> +                   surface_height(con->surface)); }
> +
>  void dpy_gfx_replace_surface(QemuConsole *con,
>                               DisplaySurface *surface)  {
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev

Re: [Qemu-devel] [PATCH 2/2] ui: introduce vfio_display_reset

[Gerd Hoffmann]

  Hi,

> After reviewing this patch-set again, I think we might not need the
> proposed dpy_gfx_switch_surface() any more.  The reason I proposed it
> was because I thought gfx.ds is changed to the surface related to
> guest dma-buf framebuffer and we need to switch it with QemuConsole's
> DisplaySurgface during reset.  But after reviewing the code,
> vc->gfx.ds is always pointing to the valid QemuConsoles'
> DisplaySurface. So we don’t need dpy_gfx_switch_surface() to take care
> of it.

Good, this is what I expected.

> > Maybe you just need a full display update after disabling the gl
> > scanout?
> Do you still think updating full display is needed?

It certainly isn't required, gtk wouldn't access the dma-buf any more
after scanout_disable().

Without display update the last guest display may remain visible until
the guest reloads the driver though.  With display update the gtk ui
should show the (blank) DisplaySurface instead.  So it is more
cosmetical.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/2] ui: introduce vfio_display_reset

[Zhang, Tina]

> -----Original Message-----
> From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@lists.freedesktop.org] On
> Behalf Of Gerd Hoffmann
> Sent: Tuesday, April 24, 2018 4:16 PM
> To: Zhang, Tina <tina.zhang@intel.com>
> Cc: alex.williamson@redhat.com; intel-gvt-dev@lists.freedesktop.org; qemu-
> devel@nongnu.org; zhenyuw@linux.intel.com; Yuan, Hang
> <hang.yuan@intel.com>
> Subject: Re: [PATCH 2/2] ui: introduce vfio_display_reset
> 
>   Hi,
> 
> > After reviewing this patch-set again, I think we might not need the
> > proposed dpy_gfx_switch_surface() any more.  The reason I proposed it
> > was because I thought gfx.ds is changed to the surface related to
> > guest dma-buf framebuffer and we need to switch it with QemuConsole's
> > DisplaySurgface during reset.  But after reviewing the code,
> > vc->gfx.ds is always pointing to the valid QemuConsoles'
> > DisplaySurface. So we don’t need dpy_gfx_switch_surface() to take care
> > of it.
> 
> Good, this is what I expected.
> 
> > > Maybe you just need a full display update after disabling the gl
> > > scanout?
> > Do you still think updating full display is needed?
> 
> It certainly isn't required, gtk wouldn't access the dma-buf any more after
> scanout_disable().
> 
> Without display update the last guest display may remain visible until the guest
> reloads the driver though.  With display update the gtk ui should show the
> (blank) DisplaySurface instead.  So it is more cosmetical.
Indeed. Thanks for the comments. I will send out the next version of this patch-set.
Thanks.

BR,
Tina
> 
> cheers,
>   Gerd
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev