[PATCH] ui/console: Assert graphic console surface is not NULL

* [PATCH] ui/console: Assert graphic console surface is not NULL
@ 2021-02-19 10:17 Akihiko Odaki
  2021-02-19 14:48 ` Gerd Hoffmann
  0 siblings, 1 reply; 14+ messages in thread
From: Akihiko Odaki @ 2021-02-19 10:17 UTC (permalink / raw)
  Cc: Michael S. Tsirkin, qemu-devel, Akihiko Odaki, Gerd Hoffmann

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, there are only three cases where NULL is assigned as
graphic console surface, and we can study them to figure out the
desired behavior:
- virtio-gpu-base assigns NULL when the device is realized.
  We have nothing to do in the case because graphic consoles
  already have a surface with a message saying the content is
  not initializd yet.
- virtio-gpu-3d assigns NULL when the device is reset. The initial
  graphic console surfaces shows a message, so it would be
  appropriate to do similar.
- virtio-gpu-3d assigns NULL when scanout is disabled. That
  affects its operations later but itself do not mean any effects
  on displays. Removing the operation should be fine.

This change eliminates NULL as graphic console surface by
implementing those behaviors.

An assertion is added and NULL checks are removed in ui/console
to prevent NULL from being set again. However, this do not change
any references except those in ui/console because there are too
many of them to fix for a mere motal.

Signed-off-by: Akihiko Odaki <akihiko.odaki@gmail.com>
---
 hw/display/virtio-gpu-3d.c   | 11 ++++++-----
 hw/display/virtio-gpu-base.c |  3 ---
 ui/console.c                 | 31 +++++++++----------------------
 3 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 0b0c11474dd..4cf1901b47f 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -179,10 +179,6 @@ static void virgl_cmd_set_scanout(VirtIOGPU *g,
             info.width, info.height,
             ss.r.x, ss.r.y, ss.r.width, ss.r.height);
     } else {
-        if (ss.scanout_id != 0) {
-            dpy_gfx_replace_surface(
-                g->parent_obj.scanout[ss.scanout_id].con, NULL);
-        }
         dpy_gl_scanout_disable(g->parent_obj.scanout[ss.scanout_id].con);
     }
     g->parent_obj.scanout[ss.scanout_id].resource_id = ss.resource_id;
@@ -596,7 +592,12 @@ void virtio_gpu_virgl_reset(VirtIOGPU *g)
     virgl_renderer_reset();
     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
         if (i != 0) {
-            dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
+            DisplaySurface *surface =
+                qemu_create_message_surface(g->parent_obj.conf.xres,
+                                            g->parent_obj.conf.yres,
+                                            "Guest reset display.");
+
+            dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, surface);
         }
         dpy_gl_scanout_disable(g->parent_obj.scanout[i].con);
     }
diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
index 40ccd00f942..abc5fc89b9b 100644
--- a/hw/display/virtio-gpu-base.c
+++ b/hw/display/virtio-gpu-base.c
@@ -168,9 +168,6 @@ virtio_gpu_base_device_realize(DeviceState *qdev,
     for (i = 0; i < g->conf.max_outputs; i++) {
         g->scanout[i].con =
             graphic_console_init(DEVICE(g), i, &virtio_gpu_ops, g);
-        if (i > 0) {
-            dpy_gfx_replace_surface(g->scanout[i].con, NULL);
-        }
     }
 
     return true;
diff --git a/ui/console.c b/ui/console.c
index d80ce7037c3..bc300182c84 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1099,10 +1099,8 @@ void console_select(unsigned int index)
                     dcl->ops->dpy_gfx_switch(dcl, s->surface);
                 }
             }
-            if (s->surface) {
-                dpy_gfx_update(s, 0, 0, surface_width(s->surface),
-                               surface_height(s->surface));
-            }
+            dpy_gfx_update(s, 0, 0, surface_width(s->surface),
+                           surface_height(s->surface));
         }
         if (ds->have_text) {
             dpy_text_resize(s, s->width, s->height);
@@ -1587,13 +1585,9 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
 {
     DisplayState *s = con->ds;
     DisplayChangeListener *dcl;
-    int width = w;
-    int height = h;
+    int width = surface_width(con->surface);
+    int height = surface_height(con->surface);
 
-    if (con->surface) {
-        width = surface_width(con->surface);
-        height = surface_height(con->surface);
-    }
     x = MAX(x, 0);
     y = MAX(y, 0);
     x = MIN(x, width);
@@ -1616,9 +1610,6 @@ void dpy_gfx_update(QemuConsole *con, int x, int y, int w, int h)
 
 void dpy_gfx_update_full(QemuConsole *con)
 {
-    if (!con->surface) {
-        return;
-    }
     dpy_gfx_update(con, 0, 0,
                    surface_width(con->surface),
                    surface_height(con->surface));
@@ -1631,7 +1622,8 @@ void dpy_gfx_replace_surface(QemuConsole *con,
     DisplaySurface *old_surface = con->surface;
     DisplayChangeListener *dcl;
 
-    assert(old_surface != surface || surface == NULL);
+    assert(surface);
+    assert(old_surface != surface);
 
     con->surface = surface;
     QLIST_FOREACH(dcl, &s->listeners, next) {
@@ -1976,13 +1968,8 @@ void graphic_console_close(QemuConsole *con)
     static const char unplugged[] =
         "Guest display has been unplugged";
     DisplaySurface *surface;
-    int width = 640;
-    int height = 480;
-
-    if (con->surface) {
-        width = surface_width(con->surface);
-        height = surface_height(con->surface);
-    }
+    int width = surface_width(con->surface);
+    int height = surface_height(con->surface);
 
     trace_console_gfx_close(con->index);
     object_property_set_link(OBJECT(con), "device", NULL, &error_abort);
@@ -2293,7 +2280,7 @@ void qemu_console_resize(QemuConsole *s, int width, int height)
 
     assert(s->console_type == GRAPHIC_CONSOLE);
 
-    if (s->surface && (s->surface->flags & QEMU_ALLOCATED_FLAG) &&
+    if ((s->surface->flags & QEMU_ALLOCATED_FLAG) &&
         pixman_image_get_width(s->surface->image) == width &&
         pixman_image_get_height(s->surface->image) == height) {
         return;
-- 
2.24.3 (Apple Git-128)



^ permalink raw reply related	[flat|nested] 14+ messages in thread