diff mbox series

ui/console: Assert graphic console surface is not NULL

Message ID 20210219101702.91002-1-akihiko.odaki@gmail.com
State New
Headers show
Series ui/console: Assert graphic console surface is not NULL | expand

Commit Message

Akihiko Odaki Feb. 19, 2021, 10:17 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, 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(-)

Comments

Gerd Hoffmann Feb. 19, 2021, 2:48 p.m. UTC | #1 
On Fri, Feb 19, 2021 at 07:17:02PM +0900, Akihiko Odaki wrote:
> 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.

No.

Background: Some display devices (qxl, virtio) support multiple outputs.
For secondary displays it totally makes sense to allow them being
disabled and the ui hiding the window then.  For the primary display it
usually is problematic though.

So in case the guest disabled the display virtio-gpu will create a
message surface for head 0 and pass NULL otherwise.

We certainly can make the whole thing more consistent.  One option I see
is deal with the surface == NULL case in dpy_gfx_replace_surface().  The
function can check whenever the display is primary (con->index == 0) and
show a message in that case and pass on the NULL otherwise.  We could
maybe also add a flag to the DisplayChangeListener struct to indicate
whenever surface == NULL can be handled (sdl for example) or not (vnc)
so dpy_gfx_replace_surface() could also create a message surface for
secondary displays if needed.

With that in plase virtio-gpu can just pass NULL for disabled displays
no matter what.  DisplayChangeListeners can ask for non-NULL surfaces if
they want.  All logic is in one place (dpy_gfx_replace_surface).

take care,
  Gerd
diff mbox series

Patch

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;