[PATCH 0/2] virtio-gpu: reset gfx resources in main thread

[PATCH 0/2] virtio-gpu: reset gfx resources in main thread

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

See the second patch for details.
thanks

Marc-André Lureau (2):
  virtio-gpu: free BHs, by implementing unrealize
  virtio-gpu: reset gfx resources in main thread

 include/hw/virtio/virtio-gpu.h |  4 +++
 hw/display/virtio-gpu-base.c   |  2 +-
 hw/display/virtio-gpu.c        | 48 +++++++++++++++++++++++++++++-----
 3 files changed, 46 insertions(+), 8 deletions(-)

-- 
2.41.0

[PATCH 1/2] virtio-gpu: free BHs, by implementing unrealize

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/virtio/virtio-gpu.h |  1 +
 hw/display/virtio-gpu-base.c   |  2 +-
 hw/display/virtio-gpu.c        | 10 ++++++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 7ea8ae2bee..05bee09e1a 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -238,6 +238,7 @@ bool virtio_gpu_base_device_realize(DeviceState *qdev,
                                     VirtIOHandleOutput ctrl_cb,
                                     VirtIOHandleOutput cursor_cb,
                                     Error **errp);
+void virtio_gpu_base_device_unrealize(DeviceState *qdev);
 void virtio_gpu_base_reset(VirtIOGPUBase *g);
 void virtio_gpu_base_fill_display_info(VirtIOGPUBase *g,
                         struct virtio_gpu_resp_display_info *dpy_info);
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 7ab7d08d0a..ca1fb7b16f 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -244,7 +244,7 @@ virtio_gpu_base_set_features(VirtIODevice *vdev, uint64_t features)
     trace_virtio_gpu_features(((features & virgl) == virgl));
 }
 
-static void
+void
 virtio_gpu_base_device_unrealize(DeviceState *qdev)
 {
     VirtIOGPUBase *g = VIRTIO_GPU_BASE(qdev);
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index e8603d78ca..b1f5d392bb 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -1392,6 +1392,15 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
     QTAILQ_INIT(&g->fenceq);
 }
 
+static void virtio_gpu_device_unrealize(DeviceState *qdev)
+{
+    VirtIOGPU *g = VIRTIO_GPU(qdev);
+
+    g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
+    g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
+    virtio_gpu_base_device_unrealize(qdev);
+}
+
 void virtio_gpu_reset(VirtIODevice *vdev)
 {
     VirtIOGPU *g = VIRTIO_GPU(vdev);
@@ -1492,6 +1501,7 @@ static void virtio_gpu_class_init(ObjectClass *klass, void *data)
     vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;
 
     vdc->realize = virtio_gpu_device_realize;
+    vdc->unrealize = virtio_gpu_device_unrealize;
     vdc->reset = virtio_gpu_reset;
     vdc->get_config = virtio_gpu_get_config;
     vdc->set_config = virtio_gpu_set_config;
-- 
2.41.0

[PATCH 2/2] virtio-gpu: reset gfx resources in main thread

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling OpenGL from different threads can have bad consequences if not
carefully reviewed. It's not generally supported. In my case, I was
debugging a crash in glDeleteTextures from OPENGL32.DLL, where I asked
qemu for gl=es, and thus ANGLE implementation was expected. libepoxy did
resolution of the global pointer for glGenTexture to the GLES version
from the main thread. But it resolved glDeleteTextures to the GL
version, because it was done from a different thread without correct
context. Oops.

Let's stick to the main thread for GL calls by using a BH.

Note: I didn't use atomics for reset_finished check, assuming the BQL
will provide enough of sync, but I might be wrong.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/virtio/virtio-gpu.h |  3 +++
 hw/display/virtio-gpu.c        | 38 +++++++++++++++++++++++++++-------
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 05bee09e1a..390c4642b8 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -169,6 +169,9 @@ struct VirtIOGPU {
 
     QEMUBH *ctrl_bh;
     QEMUBH *cursor_bh;
+    QEMUBH *reset_bh;
+    QemuCond reset_cond;
+    bool reset_finished;
 
     QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist;
     QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index b1f5d392bb..bbd5c6561a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/iov.h"
+#include "sysemu/cpus.h"
 #include "ui/console.h"
 #include "trace.h"
 #include "sysemu/dma.h"
@@ -41,6 +42,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t resource_id,
 
 static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
                                        struct virtio_gpu_simple_resource *res);
+static void virtio_gpu_reset_bh(void *opaque);
 
 void virtio_gpu_update_cursor_data(VirtIOGPU *g,
                                    struct virtio_gpu_scanout *s,
@@ -1387,6 +1389,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error **errp)
                                      &qdev->mem_reentrancy_guard);
     g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
                                        &qdev->mem_reentrancy_guard);
+    g->reset_bh = qemu_bh_new(virtio_gpu_reset_bh, g);
+    qemu_cond_init(&g->reset_cond);
     QTAILQ_INIT(&g->reslist);
     QTAILQ_INIT(&g->cmdq);
     QTAILQ_INIT(&g->fenceq);
@@ -1398,20 +1402,44 @@ static void virtio_gpu_device_unrealize(DeviceState *qdev)
 
     g_clear_pointer(&g->ctrl_bh, qemu_bh_delete);
     g_clear_pointer(&g->cursor_bh, qemu_bh_delete);
+    g_clear_pointer(&g->reset_bh, qemu_bh_delete);
+    qemu_cond_destroy(&g->reset_cond);
     virtio_gpu_base_device_unrealize(qdev);
 }
 
-void virtio_gpu_reset(VirtIODevice *vdev)
+static void virtio_gpu_reset_bh(void *opaque)
 {
-    VirtIOGPU *g = VIRTIO_GPU(vdev);
+    VirtIOGPU *g = VIRTIO_GPU(opaque);
     struct virtio_gpu_simple_resource *res, *tmp;
-    struct virtio_gpu_ctrl_command *cmd;
     int i = 0;
 
     QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
         virtio_gpu_resource_destroy(g, res);
     }
 
+    for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
+        dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
+    }
+
+    g->reset_finished = true;
+    qemu_cond_signal(&g->reset_cond);
+}
+
+void virtio_gpu_reset(VirtIODevice *vdev)
+{
+    VirtIOGPU *g = VIRTIO_GPU(vdev);
+    struct virtio_gpu_ctrl_command *cmd;
+
+    if (qemu_in_vcpu_thread()) {
+        g->reset_finished = false;
+        qemu_bh_schedule(g->reset_bh);
+        while (!g->reset_finished) {
+            qemu_cond_wait_iothread(&g->reset_cond);
+        }
+    } else {
+        virtio_gpu_reset_bh(g);
+    }
+
     while (!QTAILQ_EMPTY(&g->cmdq)) {
         cmd = QTAILQ_FIRST(&g->cmdq);
         QTAILQ_REMOVE(&g->cmdq, cmd, next);
@@ -1425,10 +1453,6 @@ void virtio_gpu_reset(VirtIODevice *vdev)
         g_free(cmd);
     }
 
-    for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
-        dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
-    }
-
     virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
 }
 
-- 
2.41.0

Re: [PATCH 0/2] virtio-gpu: reset gfx resources in main thread

[Marc-André Lureau]

Hi Kim

Could you review those patches? I would like them for 8.1 and somewhat
fix your commit 0d0be87659b ("virtio-gpu: replace the surface with
null surface when resetting").

thanks


On Wed, Jul 26, 2023 at 10:53 PM <marcandre.lureau@redhat.com> wrote:
>
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Hi,
>
> See the second patch for details.
> thanks
>
> Marc-André Lureau (2):
>   virtio-gpu: free BHs, by implementing unrealize
>   virtio-gpu: reset gfx resources in main thread
>
>  include/hw/virtio/virtio-gpu.h |  4 +++
>  hw/display/virtio-gpu-base.c   |  2 +-
>  hw/display/virtio-gpu.c        | 48 +++++++++++++++++++++++++++++-----
>  3 files changed, 46 insertions(+), 8 deletions(-)
>
> --
> 2.41.0
>
>


--
Marc-André Lureau

Re: [PATCH 1/2] virtio-gpu: free BHs, by implementing unrealize

[Kim, Dongwon]

Acked-by: Dongwon Kim <dongwon.kim@intel.com> From: Marc-André Lureau 
<marcandre.lureau@redhat.com> Signed-off-by: Marc-André Lureau 
<marcandre.lureau@redhat.com> ---  include/hw/virtio/virtio-gpu.h |  1 + 
  hw/display/virtio-gpu-base.c   |  2 +-  hw/display/virtio-gpu.c        
| 10 ++++++++++  3 files changed, 12 insertions(+), 1 deletion(-) diff 
--git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h 
index 7ea8ae2bee..05bee09e1a 100644 --- a/include/hw/virtio/virtio-gpu.h 
+++ b/include/hw/virtio/virtio-gpu.h @@ -238,6 +238,7 @@ bool 
virtio_gpu_base_device_realize(DeviceState *qdev, 
                                      VirtIOHandleOutput ctrl_cb, 
                                      VirtIOHandleOutput cursor_cb, 
                                      Error **errp); +void 
virtio_gpu_base_device_unrealize(DeviceState *qdev);  void 
virtio_gpu_base_reset(VirtIOGPUBase *g);  void 
virtio_gpu_base_fill_display_info(VirtIOGPUBase *g, 
                          struct virtio_gpu_resp_display_info 
*dpy_info); diff --git a/hw/display/virtio-gpu-base.c 
b/hw/display/virtio-gpu-base.c index 7ab7d08d0a..ca1fb7b16f 100644 --- 
a/hw/display/virtio-gpu-base.c +++ b/hw/display/virtio-gpu-base.c @@ 
-244,7 +244,7 @@ virtio_gpu_base_set_features(VirtIODevice *vdev, 
uint64_t features)      trace_virtio_gpu_features(((features & virgl) == 
virgl));  } -static void +void 
  virtio_gpu_base_device_unrealize(DeviceState *qdev)  {      
VirtIOGPUBase *g = VIRTIO_GPU_BASE(qdev); diff --git 
a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 
e8603d78ca..b1f5d392bb 100644 --- a/hw/display/virtio-gpu.c +++ 
b/hw/display/virtio-gpu.c @@ -1392,6 +1392,15 @@ void 
virtio_gpu_device_realize(DeviceState *qdev, Error **errp)      
QTAILQ_INIT(&g->fenceq);  } +static void 
virtio_gpu_device_unrealize(DeviceState *qdev) +{ +    VirtIOGPU *g = 
VIRTIO_GPU(qdev); + +    g_clear_pointer(&g->ctrl_bh, qemu_bh_delete); 
+    g_clear_pointer(&g->cursor_bh, qemu_bh_delete); +    
virtio_gpu_base_device_unrealize(qdev); +} +  void 
virtio_gpu_reset(VirtIODevice *vdev)  {      VirtIOGPU *g = 
VIRTIO_GPU(vdev); @@ -1492,6 +1501,7 @@ static void 
virtio_gpu_class_init(ObjectClass *klass, void *data)      
vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;      vdc->realize = 
virtio_gpu_device_realize; +    vdc->unrealize = 
virtio_gpu_device_unrealize;      vdc->reset = virtio_gpu_reset;      
vdc->get_config = virtio_gpu_get_config;      vdc->set_config = 
virtio_gpu_set_config; -- 2.41.0