| Message ID | 20240224015123.1575580-1-satyeshwar.singh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-gpu: Fix HW cursor visibility | expand |
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 >
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,
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 visible or not. If not, we simply call gdk_window_set_cursor with NULL 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(+)