Re: [PATCH] virtio-gpu: Fix HW cursor visibility

[Marc-André Lureau]

Hi

On Sat, Feb 24, 2024 at 4:49 AM Satyeshwar Singh
<satyeshwar.singh@intel.com> wrote:
>
> When show-cursor is on, most of the time Windows VM draws the cursor by
> itself and hands it over to Qemu as a separate resource. However,
> sometimes, Windows OS gives control of the cursor to applications like
> Notepad. In such cases a software cursor which is part of the overall
> framebuffer is drawn by the application. Windows intimates the indirect
> display driver (IDD) about this change through a flag and IDD is in turn
> responsible for communicating with the hypervisor (Qemu) to hide the HW
> cursor. This show/hide cursor can happen any time dynamically.
>
> Unfortunately, it seems like Qemu doesn't expect this change to happen
> dynamically. The code in virtio-gpu.c was written such that
> update_cursor would first call dpy_cursor_define if the cursor shape has
> changed and this is not a simple move operation (which indeed is the
> case when moving to a SW cursor) and then call dpy_mouse_set.
> dpy_cursor_define calls toolkits like GTK but in addition to creating
> the cursor content, it also calls gdk_window_set_cursor thereby setting
> the cursor. It is therefore, the right function to either show or hide
> the cursor but there was no code present to hide the cursor. Changing
> the cursor visibility in dpy_mouse_set has two issues. First,
> dpy_cursor_define already decided to display the cursor so showing the
> cursor in the previous call only to hide it in dpy_mouse_set doesn't
> sound right. Second, dpy_mouse_set skips the rest of the code if we
> are in absolute mode so adding this code there wouldn't work in that
> mode.
>
> Qemu makes the decision of whether to show or hide the cursor by
> observing the cursor->resource_id flag in virtio-gpu.c as is evident
> from the lines
>         dpy_mouse_set(s->con, cursor->pos.x, cursor->pos.y,
>                   cursor->resource_id ? 1 : 0);
> The last parameter is considered the "visible" parameter in gdk code.
> Therefore, in this patch we continue with the same model. Instead of
> changing the function prototype and changing a dozen plus files, we are
> adding the visible field in QEMUCursor data structure and passing
> it from virtio-gpu to the GTK code where we check if the cursor is

You will need to review all usages of QEMUCursor and set visible =
true by default.

Whether it's better or not than modifying a dozen files with a new
"visible" argument, I can't say.

Also we should have consistent behaviour across all display backends,
not just GTK.

> visible or not. If not, we simply call gdk_window_set_cursor with NULL

You should use GtkDisplayState.null_cursor, there is an option to
still show the cursor, even when the guest makes it invisible.
(show-cursor=on)

Which guest software do you test with?

Both gtk and firefox with cursor none test will actually set a
"transparent" cursor, which we don't detect. Maybe we should also have
some check for this case to implement that "show-cursor" behaviour in
that case too.

> parameter, thereby hiding the cursor. Once Windows VM switches back to
> the HW cursor, then IDD would again provide a new resource_id to Qemu
> and we can start displaying it once more.
>
> Signed-off-by: Satyeshwar Singh <satyeshwar.singh@intel.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Cc: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  hw/display/virtio-gpu.c | 1 +
>  include/ui/console.h    | 1 +
>  ui/gtk.c                | 5 +++++
>  3 files changed, 7 insertions(+)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 1c1ee230b3..1ae9be605b 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -98,6 +98,7 @@ static void update_cursor(VirtIOGPU *g, struct 
> virtio_gpu_update_cursor *cursor)
>
>          s->current_cursor->hot_x = cursor->hot_x;
>          s->current_cursor->hot_y = cursor->hot_y;
> +        s->current_cursor->visible = cursor->resource_id ? 1 : 0;
>
>          if (cursor->resource_id > 0) {
>              vgc->update_cursor_data(g, s, cursor->resource_id);
> diff --git a/include/ui/console.h b/include/ui/console.h
> index a4a49ffc64..47c39ed405 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -161,6 +161,7 @@ typedef struct QEMUCursor {
>      uint16_t            width, height;
>      int                 hot_x, hot_y;
>      int                 refcount;
> +    int                 visible;
>      uint32_t            data[];
>  } QEMUCursor;
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 810d7fc796..12a76de570 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -478,6 +478,11 @@ static void gd_cursor_define(DisplayChangeListener *dcl,
>          return;
>      }
>
> +    if(!c->visible) {
> +        gdk_window_set_cursor(gtk_widget_get_window(vc->gfx.drawing_area), 
> NULL);
> +        return;
> +    }
> +
>      pixbuf = gdk_pixbuf_new_from_data((guchar *)(c->data),
>                                        GDK_COLORSPACE_RGB, true, 8,
>                                        c->width, c->height, c->width * 4,
> --
> 2.33.1
>