Hi
On Thu, Aug 3, 2023 at 10:23 PM Kim, Dongwon <dongwon....@intel.com> wrote:
>
> Looking good. By the way, what does 'BH' stand for?
>
BH: bottom-half, it's a kind of delayed callback.
> Acked-by: Dongwon Kim <dongwon....@intel.com>
thanks
>
> From: Marc-André Lureau <marcandre.lur...@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.lur...@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
>
>
