[PATCH v2 0/3] stdvga: fix screen blanking

[PATCH v2 0/3] stdvga: fix screen blanking

[Gerd Hoffmann]

Gerd Hoffmann (3):
  stdvga: fix screen blanking
  ui+display: rename is_buffer_shared() -> surface_is_borrowed()
  ui+display: rename is_placeholder -> surface_is_placeholder

 include/ui/surface.h    |  4 ++--
 hw/display/qxl-render.c |  2 +-
 hw/display/vga.c        | 14 ++++++++++----
 hw/display/xenfb.c      |  4 ++--
 ui/console.c            |  2 +-
 ui/sdl2-2d.c            |  2 +-
 ui/sdl2-gl.c            |  2 +-
 7 files changed, 18 insertions(+), 12 deletions(-)

-- 
2.45.1

[PATCH v2 1/3] stdvga: fix screen blanking

[Gerd Hoffmann]

In case the display surface uses a shared buffer (i.e. uses vga vram
directly instead of a shadow) go unshare the buffer before clearing it.

This avoids vga memory corruption, which in turn fixes unblanking not
working properly with X11.

Cc: qemu-stable@nongnu.org
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 30facc6c8e33..474b6b14c327 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, int full_update)
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
 
+    if (is_buffer_shared(surface)) {
+        /* unshare buffer, otherwise the blanking corrupts vga vram */
+        surface = qemu_create_displaysurface(s->last_scr_width, s->last_scr_height);
+        dpy_gfx_replace_surface(s->con, surface);
+    }
+
     w = s->last_scr_width * surface_bytes_per_pixel(surface);
     d = surface_data(surface);
     for(i = 0; i < s->last_scr_height; i++) {
-- 
2.45.1

[PATCH v2 2/3] ui+display: rename is_buffer_shared() -> surface_is_borrowed()

[Gerd Hoffmann]

No functional change.

Suggested-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/surface.h    |  2 +-
 hw/display/qxl-render.c |  2 +-
 hw/display/vga.c        | 10 +++++-----
 hw/display/xenfb.c      |  4 ++--
 ui/console.c            |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/ui/surface.h b/include/ui/surface.h
index 4244e0ca4a32..96f9b1611c1c 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -45,7 +45,7 @@ void qemu_displaysurface_win32_set_handle(DisplaySurface *surface,
 DisplaySurface *qemu_create_displaysurface(int width, int height);
 void qemu_free_displaysurface(DisplaySurface *surface);
 
-static inline int is_buffer_shared(DisplaySurface *surface)
+static inline int surface_is_borrowed(DisplaySurface *surface)
 {
     return !(surface->flags & QEMU_ALLOCATED_FLAG);
 }
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index ec99ec887a6e..bfdf2c59bdbe 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -31,7 +31,7 @@ static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
     uint8_t *src;
     int len, i;
 
-    if (is_buffer_shared(surface)) {
+    if (surface_is_borrowed(surface)) {
         return;
     }
     trace_qxl_render_blit(qxl->guest_primary.qxl_stride,
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 474b6b14c327..bd800a683e45 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1620,7 +1620,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         height != s->last_height ||
         s->last_depth != depth ||
         s->last_byteswap != byteswap ||
-        share_surface != is_buffer_shared(surface)) {
+        share_surface != surface_is_borrowed(surface)) {
         /* display parameters changed -> need new display surface */
         s->last_scr_width = disp_width;
         s->last_scr_height = height;
@@ -1635,7 +1635,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         full_update = 1;
     }
     if (surface_data(surface) != s->vram_ptr + (s->params.start_addr * 4)
-        && is_buffer_shared(surface)) {
+        && surface_is_borrowed(surface)) {
         /* base address changed (page flip) -> shared display surfaces
          * must be updated with the new base address */
         full_update = 1;
@@ -1655,7 +1655,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
 
     vga_draw_line = vga_draw_line_table[v];
 
-    if (!is_buffer_shared(surface) && s->cursor_invalidate) {
+    if (!surface_is_borrowed(surface) && s->cursor_invalidate) {
         s->cursor_invalidate(s);
     }
 
@@ -1707,7 +1707,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         if (update) {
             if (y_start < 0)
                 y_start = y;
-            if (!(is_buffer_shared(surface))) {
+            if (!(surface_is_borrowed(surface))) {
                 uint8_t *p;
                 p = vga_draw_line(s, d, addr, width, hpel);
                 if (p) {
@@ -1762,7 +1762,7 @@ static void vga_draw_blank(VGACommonState *s, int full_update)
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
 
-    if (is_buffer_shared(surface)) {
+    if (surface_is_borrowed(surface)) {
         /* unshare buffer, otherwise the blanking corrupts vga vram */
         surface = qemu_create_displaysurface(s->last_scr_width, s->last_scr_height);
         dpy_gfx_replace_surface(s->con, surface);
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c
index 27536bfce0cb..a9bc5a08cdc2 100644
--- a/hw/display/xenfb.c
+++ b/hw/display/xenfb.c
@@ -637,7 +637,7 @@ static void xenfb_guest_copy(struct XenFB *xenfb, int x, int y, int w, int h)
     int linesize = surface_stride(surface);
     uint8_t *data = surface_data(surface);
 
-    if (!is_buffer_shared(surface)) {
+    if (!surface_is_borrowed(surface)) {
         switch (xenfb->depth) {
         case 8:
             if (bpp == 16) {
@@ -755,7 +755,7 @@ static void xenfb_update(void *opaque)
         xen_pv_printf(&xenfb->c.xendev, 1,
                       "update: resizing: %dx%d @ %d bpp%s\n",
                       xenfb->width, xenfb->height, xenfb->depth,
-                      is_buffer_shared(surface) ? " (shared)" : "");
+                      surface_is_borrowed(surface) ? " (borrowed)" : "");
         xenfb->up_fullscreen = 1;
     }
 
diff --git a/ui/console.c b/ui/console.c
index 1b2cd0c7365d..d7967ddb0d1a 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1510,7 +1510,7 @@ void qemu_console_resize(QemuConsole *s, int width, int height)
     assert(QEMU_IS_GRAPHIC_CONSOLE(s));
 
     if ((s->scanout.kind != SCANOUT_SURFACE ||
-         (surface && !is_buffer_shared(surface) && !is_placeholder(surface))) &&
+         (surface && !surface_is_borrowed(surface) && !is_placeholder(surface))) &&
         qemu_console_get_width(s, -1) == width &&
         qemu_console_get_height(s, -1) == height) {
         return;
-- 
2.45.1

[PATCH v2 3/3] ui+display: rename is_placeholder -> surface_is_placeholder

[Gerd Hoffmann]

No functional change.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 include/ui/surface.h | 2 +-
 ui/console.c         | 2 +-
 ui/sdl2-2d.c         | 2 +-
 ui/sdl2-gl.c         | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/ui/surface.h b/include/ui/surface.h
index 96f9b1611c1c..60416a451901 100644
--- a/include/ui/surface.h
+++ b/include/ui/surface.h
@@ -50,7 +50,7 @@ static inline int surface_is_borrowed(DisplaySurface *surface)
     return !(surface->flags & QEMU_ALLOCATED_FLAG);
 }
 
-static inline int is_placeholder(DisplaySurface *surface)
+static inline int surface_is_placeholder(DisplaySurface *surface)
 {
     return surface->flags & QEMU_PLACEHOLDER_FLAG;
 }
diff --git a/ui/console.c b/ui/console.c
index d7967ddb0d1a..3bd2adcc33c3 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1510,7 +1510,7 @@ void qemu_console_resize(QemuConsole *s, int width, int height)
     assert(QEMU_IS_GRAPHIC_CONSOLE(s));
 
     if ((s->scanout.kind != SCANOUT_SURFACE ||
-         (surface && !surface_is_borrowed(surface) && !is_placeholder(surface))) &&
+         (surface && !surface_is_borrowed(surface) && !surface_is_placeholder(surface))) &&
         qemu_console_get_width(s, -1) == width &&
         qemu_console_get_height(s, -1) == height) {
         return;
diff --git a/ui/sdl2-2d.c b/ui/sdl2-2d.c
index 06468cd493ea..73052383c2e0 100644
--- a/ui/sdl2-2d.c
+++ b/ui/sdl2-2d.c
@@ -72,7 +72,7 @@ void sdl2_2d_switch(DisplayChangeListener *dcl,
         scon->texture = NULL;
     }
 
-    if (is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
+    if (surface_is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
         sdl2_window_destroy(scon);
         return;
     }
diff --git a/ui/sdl2-gl.c b/ui/sdl2-gl.c
index 28d796607c08..91b7ee2419b7 100644
--- a/ui/sdl2-gl.c
+++ b/ui/sdl2-gl.c
@@ -89,7 +89,7 @@ void sdl2_gl_switch(DisplayChangeListener *dcl,
 
     scon->surface = new_surface;
 
-    if (is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
+    if (surface_is_placeholder(new_surface) && qemu_console_get_index(dcl->con)) {
         qemu_gl_fini_shader(scon->gls);
         scon->gls = NULL;
         sdl2_window_destroy(scon);
-- 
2.45.1

Re: [PATCH v2 1/3] stdvga: fix screen blanking

[Marc-André Lureau]

Hi

On Mon, Jun 3, 2024 at 7:18 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> In case the display surface uses a shared buffer (i.e. uses vga vram
> directly instead of a shadow) go unshare the buffer before clearing it.
>
> This avoids vga memory corruption, which in turn fixes unblanking not
> working properly with X11.
>
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2067
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/vga.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 30facc6c8e33..474b6b14c327 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -1762,6 +1762,12 @@ static void vga_draw_blank(VGACommonState *s, int full_update)
>      if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
>          return;
>
> +    if (is_buffer_shared(surface)) {

Perhaps the suggestion to rename the function (in the following patch)
should instead be surface_is_allocated() ? that would match the actual
flag check. But callers would have to ! the result. Wdyt?

> +        /* unshare buffer, otherwise the blanking corrupts vga vram */
> +        surface = qemu_create_displaysurface(s->last_scr_width, s->last_scr_height);
> +        dpy_gfx_replace_surface(s->con, surface);

Ok, this looks safer than calling "resize".

thanks

> +    }
> +
>      w = s->last_scr_width * surface_bytes_per_pixel(surface);
>      d = surface_data(surface);
>      for(i = 0; i < s->last_scr_height; i++) {
> --
> 2.45.1
>

Re: [PATCH v2 1/3] stdvga: fix screen blanking

[Gerd Hoffmann]

On Tue, Jun 04, 2024 at 10:27:18AM GMT, Marc-André Lureau wrote:
> Hi
> 
> > +    if (is_buffer_shared(surface)) {
> 
> Perhaps the suggestion to rename the function (in the following patch)
> should instead be surface_is_allocated() ? that would match the actual
> flag check. But callers would have to ! the result. Wdyt?

surface_is_shadow() ?  Comes closer to the typical naming in computer
graphics.

take care,
  Gerd

Re: [PATCH v2 1/3] stdvga: fix screen blanking

[Marc-André Lureau]

Hi

On Wed, Jun 5, 2024 at 11:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Tue, Jun 04, 2024 at 10:27:18AM GMT, Marc-André Lureau wrote:
> > Hi
> >
> > > +    if (is_buffer_shared(surface)) {
> >
> > Perhaps the suggestion to rename the function (in the following patch)
> > should instead be surface_is_allocated() ? that would match the actual
> > flag check. But callers would have to ! the result. Wdyt?
>
> surface_is_shadow() ?  Comes closer to the typical naming in computer
> graphics.

If the underlying flag is renamed too, that's ok to me.