diff mbox series

[v2] ui/console: Pass placeholder surface to displays

Message ID 20210220113810.78371-1-akihiko.odaki@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] ui/console: Pass placeholder surface to displays | expand

Commit Message

Akihiko Odaki Feb. 20, 2021, 11:38 a.m. UTC 
ui/console used to accept NULL as graphic console surface, but its
semantics was inconsistent among displays:
- cocoa and gtk-egl perform NULL dereference.
- egl-headless, spice and spice-egl do nothing.
- gtk releases underlying resources.
- sdl2-2d and sdl2-gl destroys the window.
- vnc shows a message, "Display output is not active."

Fortunately, only virtio-gpu and virtio-gpu-3d assign NULL so
we can study them to figure out the desired behavior. They assign
NULL *except* for the primary display when the device is realized,
reset, or its scanout is disabled. This effectively destroys
windows for the (uninitialized) secondary displays.

To implement the consistent behavior of display device
realization/reset, this change embeds it to the operation
switching the surface. When NULL was given as a new surface when
switching, ui/console will instead passes a placeholder down
to each display listeners.

sdl calls is_placeholder when switching surfaces, and
destroys the window if the given surface was a placeholder. The
other displays simply shows the placeholder when the console is
inactive.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 include/ui/console.h |  6 ++++++
 ui/console.c         | 46 ++++++++++++++++++++++++++++++--------------
 ui/gtk.c             |  4 ----
 ui/sdl2-2d.c         |  7 ++-----
 ui/sdl2-gl.c         |  4 ++--
 ui/spice-display.c   |  6 +++---
 ui/vnc.c             | 10 ----------
 7 files changed, 45 insertions(+), 38 deletions(-)

Comments

Gerd Hoffmann Feb. 22, 2021, 10:51 a.m. UTC | #1 
Hi,

>  #define QEMU_ALLOCATED_FLAG     0x01
> +#define QEMU_PLACEHOLDER_FLAG   0x02

> +static inline int is_placeholder(DisplaySurface *surface)
> +{
> +    return surface->flags & QEMU_PLACEHOLDER_FLAG;
> +}

Interesting idea.  That approach makes sense too.

> +        if (!placeholder) {
> +            placeholder = qemu_create_message_surface(640, 480, placeholder_msg);
> +            placeholder->flags |= QEMU_PLACEHOLDER_FLAG;

I think we should set the placeholder flag in
qemu_create_message_surface() because every surface created with that
function is some kind if placeholder.

Also when replacing an existing surface we should make the placeholder
the same size, to avoid pointless ui window resizes.

> -    if (!new_surface) {
> +    if (is_placeholder(new_surface)) {

We should check whenever this is the primary or a secondary window here
and only destroy secondary windows.  qemu hiding all windows but
continuing to run has great potential for user confusion ...

> -    if (!new_surface) {
> +    if (is_placeholder(new_surface)) {

Same here.

take care,
  Gerd
Akihiko Odaki Feb. 23, 2021, 4:43 a.m. UTC | #2 
2021年2月22日(月) 19:51 Gerd Hoffmann <kraxel@redhat.com>:
>
>   Hi,
>
> >  #define QEMU_ALLOCATED_FLAG     0x01
> > +#define QEMU_PLACEHOLDER_FLAG   0x02
>
> > +static inline int is_placeholder(DisplaySurface *surface)
> > +{
> > +    return surface->flags & QEMU_PLACEHOLDER_FLAG;
> > +}
>
> Interesting idea.  That approach makes sense too.
>
> > +        if (!placeholder) {
> > +            placeholder = qemu_create_message_surface(640, 480, placeholder_msg);
> > +            placeholder->flags |= QEMU_PLACEHOLDER_FLAG;
>
> I think we should set the placeholder flag in
> qemu_create_message_surface() because every surface created with that
> function is some kind if placeholder.
>
> Also when replacing an existing surface we should make the placeholder
> the same size, to avoid pointless ui window resizes.
>
> > -    if (!new_surface) {
> > +    if (is_placeholder(new_surface)) {
>
> We should check whenever this is the primary or a secondary window here
> and only destroy secondary windows.  qemu hiding all windows but
> continuing to run has great potential for user confusion ...
>
> > -    if (!new_surface) {
> > +    if (is_placeholder(new_surface)) {
>
> Same here.

The other surfaces created by qemu_create_message_surface() are not
considered as "placeholder" here, and have contents to be displayed.
Since no emulated devices give NULL to dpy_gfx_replace_surface for the
primary connection, it will never get the "placeholder", and its
window will be always shown.

Regards,
Akihiko Odaki

>
> take care,
>   Gerd
>
Gerd Hoffmann Feb. 24, 2021, 11:06 a.m. UTC | #3 
Hi,

> > > -    if (!new_surface) {
> > > +    if (is_placeholder(new_surface)) {
> >
> > Same here.
> 
> The other surfaces created by qemu_create_message_surface() are not
> considered as "placeholder" here, and have contents to be displayed.

Which ones?  Pretty much any qemu_create_message_surface() use is
basically "we can't show the guest display for $reason".  Which is
the definition of a placeholder surface, isn't it?

> Since no emulated devices give NULL to dpy_gfx_replace_surface for the
> primary connection, it will never get the "placeholder", and its
> window will be always shown.

Well, this could change.  The special handling for scanout #0 in
virtio-gpu can go away when we have this in place.

Also:  The patch os growing, and it probably makes sense to split it
into a series, with one patch adding the placeholder flag and the core
code in console.c, next one patch for each UI, and finally one for
virtio-gpu ...

take care,
  Gerd
diff mbox series

Patch

diff --git a/include/ui/console.h b/include/ui/console.h
index d30e972d0b5..e0ac3850858 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -106,6 +106,7 @@  struct QemuConsoleClass {
 };
 
 #define QEMU_ALLOCATED_FLAG     0x01
+#define QEMU_PLACEHOLDER_FLAG   0x02
 
 typedef struct DisplaySurface {
     pixman_format_code_t format;
@@ -281,6 +282,11 @@  static inline int is_buffer_shared(DisplaySurface *surface)
     return !(surface->flags & QEMU_ALLOCATED_FLAG);
 }
 
+static inline int is_placeholder(DisplaySurface *surface)
+{
+    return surface->flags & QEMU_PLACEHOLDER_FLAG;
+}
+
 void register_displaychangelistener(DisplayChangeListener *dcl);
 void update_displaychangelistener(DisplayChangeListener *dcl,
                                   uint64_t interval);
diff --git a/ui/console.c b/ui/console.c
index c5d11bc7017..b786c36d5b1 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1088,6 +1088,30 @@  static void console_putchar(QemuConsole *s, int ch)
     }
 }
 
+static void dpy_gfx_switch(DisplayChangeListener *dcl, DisplaySurface *surface)
+{
+    static DisplaySurface *placeholder;
+    static const char placeholder_msg[] = "Display output is not active.";
+    DisplaySurface *broadcast;
+
+    if (!dcl->ops->dpy_gfx_switch) {
+        return;
+    }
+
+    if (surface) {
+        broadcast = surface;
+    } else {
+        if (!placeholder) {
+            placeholder = qemu_create_message_surface(640, 480, placeholder_msg);
+            placeholder->flags |= QEMU_PLACEHOLDER_FLAG;
+        }
+
+        broadcast = placeholder;
+    }
+
+    dcl->ops->dpy_gfx_switch(dcl, broadcast);
+}
+
 void console_select(unsigned int index)
 {
     DisplayChangeListener *dcl;
@@ -1104,9 +1128,7 @@  void console_select(unsigned int index)
                 if (dcl->con != NULL) {
                     continue;
                 }
-                if (dcl->ops->dpy_gfx_switch) {
-                    dcl->ops->dpy_gfx_switch(dcl, s->surface);
-                }
+                dpy_gfx_switch(dcl, s->surface);
             }
             if (s->surface) {
                 dpy_gfx_update(s, 0, 0, surface_width(s->surface),
@@ -1545,15 +1567,13 @@  void register_displaychangelistener(DisplayChangeListener *dcl)
     } else {
         con = active_console;
     }
-    if (dcl->ops->dpy_gfx_switch) {
-        if (con) {
-            dcl->ops->dpy_gfx_switch(dcl, con->surface);
-        } else {
-            if (!dummy) {
-                dummy = qemu_create_message_surface(640, 480, nodev);
-            }
-            dcl->ops->dpy_gfx_switch(dcl, dummy);
+    if (con) {
+        dpy_gfx_switch(dcl, con->surface);
+    } else {
+        if (!dummy) {
+            dummy = qemu_create_message_surface(640, 480, nodev);
         }
+        dpy_gfx_switch(dcl, dummy);
     }
     text_console_update_cursor(NULL);
 }
@@ -1685,9 +1705,7 @@  void dpy_gfx_replace_surface(QemuConsole *con,
         if (con != (dcl->con ? dcl->con : active_console)) {
             continue;
         }
-        if (dcl->ops->dpy_gfx_switch) {
-            dcl->ops->dpy_gfx_switch(dcl, surface);
-        }
+        dpy_gfx_switch(dcl, surface);
     }
     qemu_free_displaysurface(old_surface);
 }
diff --git a/ui/gtk.c b/ui/gtk.c
index 79dc2401203..a4a5f981e2a 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -567,10 +567,6 @@  static void gd_switch(DisplayChangeListener *dcl,
     }
     vc->gfx.ds = surface;
 
-    if (!surface) {
-        return;
-    }
-
     if (surface->format == PIXMAN_x8r8g8b8) {
         /*
          * PIXMAN_x8r8g8b8 == CAIRO_FORMAT_RGB24
diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c
index a2ea85127d5..ddd2e05734e 100644
--- a/ui/sdl2-2d.c
+++ b/ui/sdl2-2d.c
@@ -32,14 +32,11 @@  void sdl2_2d_update(DisplayChangeListener *dcl,
                     int x, int y, int w, int h)
 {
     struct sdl2_console *scon = container_of(dcl, struct sdl2_console, dcl);
-    DisplaySurface *surf = qemu_console_surface(dcl->con);
+    DisplaySurface *surf = scon->surface;
     SDL_Rect rect;
     size_t surface_data_offset;
     assert(!scon->opengl);
 
-    if (!surf) {
-        return;
-    }
     if (!scon->texture) {
         return;
     }
@@ -75,7 +72,7 @@  void sdl2_2d_switch(DisplayChangeListener *dcl,
         scon->texture = NULL;
     }
 
-    if (!new_surface) {
+    if (is_placeholder(new_surface)) {
         sdl2_window_destroy(scon);
         return;
     }
diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index fd594d74611..a68697714f0 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -86,7 +86,7 @@  void sdl2_gl_switch(DisplayChangeListener *dcl,
 
     scon->surface = new_surface;
 
-    if (!new_surface) {
+    if (is_placeholder(new_surface)) {
         qemu_gl_fini_shader(scon->gls);
         scon->gls = NULL;
         sdl2_window_destroy(scon);
@@ -112,7 +112,7 @@  void sdl2_gl_refresh(DisplayChangeListener *dcl)
     assert(scon->opengl);
 
     graphic_hw_update(dcl->con);
-    if (scon->updates && scon->surface) {
+    if (scon->updates && scon->real_window) {
         scon->updates = 0;
         sdl2_gl_render_surface(scon);
     }
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 6f32b66a6e7..222c7c20a2a 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -388,7 +388,7 @@  void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
     SimpleSpiceUpdate *update;
     bool need_destroy;
 
-    if (surface && ssd->surface &&
+    if (ssd->surface &&
         surface_width(surface) == pixman_image_get_width(ssd->surface) &&
         surface_height(surface) == pixman_image_get_height(ssd->surface) &&
         surface_format(surface) == pixman_image_get_format(ssd->surface)) {
@@ -410,8 +410,8 @@  void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
 
     /* full mode switch */
     trace_qemu_spice_display_surface(ssd->qxl.id,
-                                     surface ? surface_width(surface)  : 0,
-                                     surface ? surface_height(surface) : 0,
+                                     surface_width(surface),
+                                     surface_height(surface),
                                      false);
 
     memset(&ssd->dirty, 0, sizeof(ssd->dirty));
diff --git a/ui/vnc.c b/ui/vnc.c
index 16bb3be770b..310abc93781 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -790,20 +790,10 @@  static bool vnc_check_pageflip(DisplaySurface *s1,
 static void vnc_dpy_switch(DisplayChangeListener *dcl,
                            DisplaySurface *surface)
 {
-    static const char placeholder_msg[] =
-        "Display output is not active.";
-    static DisplaySurface *placeholder;
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     bool pageflip = vnc_check_pageflip(vd->ds, surface);
     VncState *vs;
 
-    if (surface == NULL) {
-        if (placeholder == NULL) {
-            placeholder = qemu_create_message_surface(640, 480, placeholder_msg);
-        }
-        surface = placeholder;
-    }
-
     vnc_abort_display_jobs(vd);
     vd->ds = surface;