[Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Anthony Liguori]

Revision: 6989
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6989
Author:   aliguori
Date:     2009-04-05 18:41:18 +0000 (Sun, 05 Apr 2009)
Log Message:
-----------
Fix display breakage when resizing the screen (v2) (Avi Kivity)

When the vga resolution changes, a new display surface is not allocated
immediately; instead that is deferred until the next update.  However,
if we're running without a display client attached, that won't happen
and the next bitblt is likely to cause a segfault by overflowing the
display surface.

Fix by reallocating the display immediately when the resolution changes.

Tested with (Windows|Linux) x (cirrus|std) x (curses|sdl).

Changes from v1:
 - fix segfault when switching virtual consoles with curses

Signed-off-by: Avi Kivity <avi@redhat.com>
Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/hw/cirrus_vga.c
    trunk/hw/vga.c
    trunk/hw/vga_int.h

Modified: trunk/hw/cirrus_vga.c
===================================================================
--- trunk/hw/cirrus_vga.c	2009-04-05 18:16:10 UTC (rev 6988)
+++ trunk/hw/cirrus_vga.c	2009-04-05 18:41:18 UTC (rev 6989)
@@ -1392,6 +1392,8 @@
 	break;
     }
 
+    vga_update_resolution((VGAState *)s);
+
     return CIRRUS_HOOK_HANDLED;
 }
 
@@ -1419,6 +1421,7 @@
 #endif
     }
     s->cirrus_hidden_dac_lockindex = 0;
+    vga_update_resolution((VGAState *)s);
 }
 
 /***************************************
@@ -1705,6 +1708,8 @@
 	break;
     }
 
+    vga_update_resolution((VGAState *)s);
+
     return CIRRUS_HOOK_HANDLED;
 }
 
@@ -2830,6 +2835,7 @@
 	if (s->ar_flip_flop == 0) {
 	    val &= 0x3f;
 	    s->ar_index = val;
+            vga_update_resolution((VGAState *)s);
 	} else {
 	    index = s->ar_index & 0x1f;
 	    switch (index) {
@@ -2923,6 +2929,7 @@
 	    /* can always write bit 4 of CR7 */
 	    if (s->cr_index == 7)
 		s->cr[7] = (s->cr[7] & ~0x10) | (val & 0x10);
+            vga_update_resolution((VGAState *)s);
 	    return;
 	}
 	switch (s->cr_index) {
@@ -2951,6 +2958,7 @@
 	    s->update_retrace_info((VGAState *) s);
 	    break;
 	}
+        vga_update_resolution((VGAState *)s);
 	break;
     case 0x3ba:
     case 0x3da:
@@ -3157,7 +3165,8 @@
 
     cirrus_update_memory_access(s);
     /* force refresh */
-    s->graphic_mode = -1;
+    vga_update_resolution((VGAState *)s);
+    s->want_full_update = 1;
     cirrus_update_bank_ptr(s, 0);
     cirrus_update_bank_ptr(s, 1);
     return 0;

Modified: trunk/hw/vga.c
===================================================================
--- trunk/hw/vga.c	2009-04-05 18:16:10 UTC (rev 6988)
+++ trunk/hw/vga.c	2009-04-05 18:41:18 UTC (rev 6989)
@@ -36,6 +36,10 @@
 
 //#define DEBUG_BOCHS_VBE
 
+#define GMODE_TEXT     0
+#define GMODE_GRAPH    1
+#define GMODE_BLANK 2
+
 /* force some bits to zero */
 const uint8_t sr_mask[8] = {
     0x03,
@@ -393,6 +397,7 @@
         if (s->ar_flip_flop == 0) {
             val &= 0x3f;
             s->ar_index = val;
+            vga_update_resolution(s);
         } else {
             index = s->ar_index & 0x1f;
             switch(index) {
@@ -433,6 +438,7 @@
 #endif
         s->sr[s->sr_index] = val & sr_mask[s->sr_index];
         if (s->sr_index == 1) s->update_retrace_info(s);
+        vga_update_resolution(s);
         break;
     case 0x3c7:
         s->dac_read_index = val;
@@ -460,6 +466,7 @@
         printf("vga: write GR%x = 0x%02x\n", s->gr_index, val);
 #endif
         s->gr[s->gr_index] = val & gr_mask[s->gr_index];
+        vga_update_resolution(s);
         break;
     case 0x3b4:
     case 0x3d4:
@@ -475,6 +482,7 @@
             /* can always write bit 4 of CR7 */
             if (s->cr_index == 7)
                 s->cr[7] = (s->cr[7] & ~0x10) | (val & 0x10);
+            vga_update_resolution(s);
             return;
         }
         switch(s->cr_index) {
@@ -502,6 +510,7 @@
             s->update_retrace_info(s);
             break;
         }
+        vga_update_resolution(s);
         break;
     case 0x3ba:
     case 0x3da:
@@ -581,11 +590,13 @@
             if ((val <= VBE_DISPI_MAX_XRES) && ((val & 7) == 0)) {
                 s->vbe_regs[s->vbe_index] = val;
             }
+            vga_update_resolution(s);
             break;
         case VBE_DISPI_INDEX_YRES:
             if (val <= VBE_DISPI_MAX_YRES) {
                 s->vbe_regs[s->vbe_index] = val;
             }
+            vga_update_resolution(s);
             break;
         case VBE_DISPI_INDEX_BPP:
             if (val == 0)
@@ -594,6 +605,7 @@
                 val == 16 || val == 24 || val == 32) {
                 s->vbe_regs[s->vbe_index] = val;
             }
+            vga_update_resolution(s);
             break;
         case VBE_DISPI_INDEX_BANK:
             if (s->vbe_regs[VBE_DISPI_INDEX_BPP] == 4) {
@@ -662,6 +674,7 @@
             }
             s->dac_8bit = (val & VBE_DISPI_8BIT_DAC) > 0;
             s->vbe_regs[s->vbe_index] = val;
+            vga_update_resolution(s);
             break;
         case VBE_DISPI_INDEX_VIRT_WIDTH:
             {
@@ -682,6 +695,7 @@
                 s->vbe_regs[VBE_DISPI_INDEX_VIRT_HEIGHT] = h;
                 s->vbe_line_offset = line_offset;
             }
+            vga_update_resolution(s);
             break;
         case VBE_DISPI_INDEX_X_OFFSET:
         case VBE_DISPI_INDEX_Y_OFFSET:
@@ -696,6 +710,7 @@
                     s->vbe_start_addr += x * ((s->vbe_regs[VBE_DISPI_INDEX_BPP] + 7) >> 3);
                 s->vbe_start_addr >>= 2;
             }
+            vga_update_resolution(s);
             break;
         default:
             break;
@@ -1302,7 +1317,6 @@
         s->plane_updated = 0;
         full_update = 1;
     }
-    full_update |= update_basic_params(s);
 
     line_offset = s->line_offset;
     s1 = s->vram_ptr + (s->start_addr * 4);
@@ -1314,18 +1328,6 @@
         return;
     }
 
-    if (width != s->last_width || height != s->last_height ||
-        cw != s->last_cw || cheight != s->last_ch || s->last_depth) {
-        s->last_scr_width = width * cw;
-        s->last_scr_height = height * cheight;
-        qemu_console_resize(s->ds, s->last_scr_width, s->last_scr_height);
-        s->last_depth = 0;
-        s->last_width = width;
-        s->last_height = height;
-        s->last_ch = cheight;
-        s->last_cw = cw;
-        full_update = 1;
-    }
     s->rgb_to_pixel =
         rgb_to_pixel_dup_table[get_depth_index(s->ds)];
     full_update |= update_palette16(s);
@@ -1582,39 +1584,21 @@
     vga_dirty_log_start(s);
 }
 
-/*
- * graphic modes
- */
-static void vga_draw_graphic(VGAState *s, int full_update)
+static void vga_update_resolution_graphics(VGAState *s)
 {
-    int y1, y, update, page_min, page_max, linesize, y_start, double_scan, mask, depth;
-    int width, height, shift_control, line_offset, page0, page1, bwidth, bits;
+    int depth = s->get_bpp(s);
+    int width, height, shift_control, double_scan;
     int disp_width, multi_scan, multi_run;
-    uint8_t *d;
-    uint32_t v, addr1, addr;
-    vga_draw_line_func *vga_draw_line;
 
-    full_update |= update_basic_params(s);
-
-    if (!full_update)
-        vga_sync_dirty_bitmap(s);
-
     s->get_resolution(s, &width, &height);
     disp_width = width;
 
     shift_control = (s->gr[0x05] >> 5) & 3;
     double_scan = (s->cr[0x09] >> 7);
-    if (shift_control != 1) {
-        multi_scan = (((s->cr[0x09] & 0x1f) + 1) << double_scan) - 1;
-    } else {
-        /* in CGA modes, multi_scan is ignored */
-        /* XXX: is it correct ? */
-        multi_scan = double_scan;
-    }
-    multi_run = multi_scan;
+
     if (shift_control != s->shift_control ||
         double_scan != s->double_scan) {
-        full_update = 1;
+        s->want_full_update = 1;
         s->shift_control = shift_control;
         s->double_scan = double_scan;
     }
@@ -1628,12 +1612,28 @@
             disp_width <<= 1;
         }
     }
+    disp_width = width;
 
-    depth = s->get_bpp(s);
+    if (shift_control != 1) {
+        multi_scan = (((s->cr[0x09] & 0x1f) + 1) << double_scan) - 1;
+    } else {
+        /* in CGA modes, multi_scan is ignored */
+        /* XXX: is it correct ? */
+        multi_scan = double_scan;
+    }
+
+    multi_run = multi_scan;
+
     if (s->line_offset != s->last_line_offset ||
         disp_width != s->last_width ||
         height != s->last_height ||
-        s->last_depth != depth) {
+        s->last_depth != depth ||
+        s->multi_run != multi_run ||
+        s->multi_scan != multi_scan ||
+        s->want_full_update) {
+        if (s->ds->surface->pf.depth == 0) {
+            goto dont_touch_display_surface;
+        }
 #if defined(WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN)
         if (depth == 16 || depth == 32) {
 #else
@@ -1650,14 +1650,91 @@
         } else {
             qemu_console_resize(s->ds, disp_width, height);
         }
+    dont_touch_display_surface:
         s->last_scr_width = disp_width;
         s->last_scr_height = height;
         s->last_width = disp_width;
         s->last_height = height;
         s->last_line_offset = s->line_offset;
         s->last_depth = depth;
-        full_update = 1;
-    } else if (is_buffer_shared(s->ds->surface) &&
+        s->multi_run = multi_run;
+        s->multi_scan = multi_scan;
+        s->want_full_update = 1;
+    }
+}
+
+static void vga_update_resolution_text(VGAState *s)
+{
+    int width, height, cw, cheight;
+
+    vga_get_text_resolution(s, &width, &height, &cw, &cheight);
+    if (width != s->last_width || height != s->last_height ||
+        cw != s->last_cw || cheight != s->last_ch || s->last_depth) {
+        s->last_scr_width = width * cw;
+        s->last_scr_height = height * cheight;
+        if (s->ds->surface->pf.depth != 0) {
+            qemu_console_resize(s->ds, s->last_scr_width, s->last_scr_height);
+        } else {
+            /*
+             * curses expects width and height to be in character cell
+             * dimensions, not pixels.
+             */
+            s->ds->surface->width = width;
+            s->ds->surface->height = height;
+            dpy_resize(s->ds);
+        }
+        s->last_depth = 0;
+        s->last_width = width;
+        s->last_height = height;
+        s->last_ch = cheight;
+        s->last_cw = cw;
+        s->want_full_update = 1;
+    }
+}
+
+void vga_update_resolution(VGAState *s)
+{
+    int graphic_mode;
+
+    if (!(s->ar_index & 0x20)) {
+        graphic_mode = GMODE_BLANK;
+    } else {
+        graphic_mode = s->gr[6] & 1;
+    }
+    if (graphic_mode != s->graphic_mode) {
+        s->graphic_mode = graphic_mode;
+        s->want_full_update = 1;
+    }
+    s->want_full_update |= update_basic_params(s);
+    switch (graphic_mode) {
+    case GMODE_TEXT:
+        vga_update_resolution_text(s);
+        break;
+    case GMODE_GRAPH:
+        vga_update_resolution_graphics(s);
+        break;
+    }
+}
+
+/*
+ * graphic modes
+ */
+static void vga_draw_graphic(VGAState *s, int full_update)
+{
+    int y1, y, update, linesize, y_start, mask;
+    int width, height, line_offset, bwidth, bits;
+    int multi_run;
+    uint8_t *d;
+    uint32_t v, addr1, addr;
+    long page0, page1, page_min, page_max;
+    vga_draw_line_func *vga_draw_line;
+
+    if (!full_update)
+        vga_sync_dirty_bitmap(s);
+
+    s->get_resolution(s, &width, &height);
+    multi_run = s->multi_run;
+    if (is_buffer_shared(s->ds->surface) &&
                (full_update || s->ds->surface->data != s->vram_ptr + (s->start_addr * 4))) {
         s->ds->surface->data = s->vram_ptr + (s->start_addr * 4);
         dpy_setdata(s->ds);
@@ -1666,7 +1743,7 @@
     s->rgb_to_pixel =
         rgb_to_pixel_dup_table[get_depth_index(s->ds)];
 
-    if (shift_control == 0) {
+    if (s->shift_control == 0) {
         full_update |= update_palette16(s);
         if (s->sr[0x01] & 8) {
             v = VGA_DRAW_LINE4D2;
@@ -1674,7 +1751,7 @@
             v = VGA_DRAW_LINE4;
         }
         bits = 4;
-    } else if (shift_control == 1) {
+    } else if (s->shift_control == 1) {
         full_update |= update_palette16(s);
         if (s->sr[0x01] & 8) {
             v = VGA_DRAW_LINE2D2;
@@ -1770,7 +1847,7 @@
             if (y_start >= 0) {
                 /* flush to display */
                 dpy_update(s->ds, 0, y_start,
-                           disp_width, y - y_start);
+                           s->last_width, y - y_start);
                 y_start = -1;
             }
         }
@@ -1779,7 +1856,7 @@
             if ((y1 & mask) == mask)
                 addr1 += line_offset;
             y1++;
-            multi_run = multi_scan;
+            multi_run = s->multi_scan;
         } else {
             multi_run--;
         }
@@ -1791,7 +1868,7 @@
     if (y_start >= 0) {
         /* flush to display */
         dpy_update(s->ds, 0, y_start,
-                   disp_width, y - y_start);
+                   s->last_width, y - y_start);
     }
     /* reset modified pages */
     if (page_max != -1) {
@@ -1828,29 +1905,17 @@
                s->last_scr_width, s->last_scr_height);
 }
 
-#define GMODE_TEXT     0
-#define GMODE_GRAPH    1
-#define GMODE_BLANK 2
-
 static void vga_update_display(void *opaque)
 {
     VGAState *s = (VGAState *)opaque;
-    int full_update, graphic_mode;
+    int full_update;
 
     if (ds_get_bits_per_pixel(s->ds) == 0) {
         /* nothing to do */
     } else {
-        full_update = 0;
-        if (!(s->ar_index & 0x20)) {
-            graphic_mode = GMODE_BLANK;
-        } else {
-            graphic_mode = s->gr[6] & 1;
-        }
-        if (graphic_mode != s->graphic_mode) {
-            s->graphic_mode = graphic_mode;
-            full_update = 1;
-        }
-        switch(graphic_mode) {
+        full_update = s->want_full_update;
+        s->want_full_update = 0;
+        switch(s->graphic_mode) {
         case GMODE_TEXT:
             vga_draw_text(s, full_update);
             break;
@@ -1870,8 +1935,8 @@
 {
     VGAState *s = (VGAState *)opaque;
 
-    s->last_width = -1;
-    s->last_height = -1;
+    vga_update_resolution(s);
+    s->want_full_update = 1;
 }
 
 void vga_reset(void *opaque)
@@ -1915,7 +1980,6 @@
     s->vbe_bank_mask = (s->vram_size >> 16) - 1;
 #endif
     memset(s->font_offsets, '\0', sizeof(s->font_offsets));
-    s->graphic_mode = -1; /* force full update */
     s->shift_control = 0;
     s->double_scan = 0;
     s->line_offset = 0;
@@ -1941,6 +2005,7 @@
         memset(&s->retrace_info, 0, sizeof (s->retrace_info));
         break;
     }
+    vga_update_resolution(s);
 }
 
 #define TEXTMODE_X(x)	((x) % width)
@@ -1952,50 +2017,28 @@
 static void vga_update_text(void *opaque, console_ch_t *chardata)
 {
     VGAState *s = (VGAState *) opaque;
-    int graphic_mode, i, cursor_offset, cursor_visible;
+    int i, cursor_offset, cursor_visible;
     int cw, cheight, width, height, size, c_min, c_max;
     uint32_t *src;
     console_ch_t *dst, val;
     char msg_buffer[80];
-    int full_update = 0;
+    int full_update = s->want_full_update;
 
-    if (!(s->ar_index & 0x20)) {
-        graphic_mode = GMODE_BLANK;
-    } else {
-        graphic_mode = s->gr[6] & 1;
-    }
-    if (graphic_mode != s->graphic_mode) {
-        s->graphic_mode = graphic_mode;
-        full_update = 1;
-    }
-    if (s->last_width == -1) {
-        s->last_width = 0;
-        full_update = 1;
-    }
-
-    switch (graphic_mode) {
+    s->want_full_update = 0;
+    switch (s->graphic_mode) {
     case GMODE_TEXT:
         /* TODO: update palette */
-        full_update |= update_basic_params(s);
 
-        /* total width & height */
-        cheight = (s->cr[9] & 0x1f) + 1;
-        cw = 8;
-        if (!(s->sr[1] & 0x01))
-            cw = 9;
-        if (s->sr[1] & 0x08)
-            cw = 16; /* NOTE: no 18 pixel wide */
-        width = (s->cr[0x01] + 1);
-        if (s->cr[0x06] == 100) {
-            /* ugly hack for CGA 160x100x16 - explain me the logic */
-            height = 100;
-        } else {
-            height = s->cr[0x12] | 
-                ((s->cr[0x07] & 0x02) << 7) | 
-                ((s->cr[0x07] & 0x40) << 3);
-            height = (height + 1) / cheight;
+        vga_get_text_resolution(s, &width, &height, &cw, &cheight);
+
+        if (s->ds->surface->width != width
+            || s->ds->surface->height != height) {
+            s->ds->surface->width = width;
+            s->ds->surface->height = height;
+            dpy_resize(s->ds);
         }
 
+        /* total width & height */
         size = (height * width);
         if (size > CH_ATTR_SIZE) {
             if (!full_update)
@@ -2006,20 +2049,6 @@
             break;
         }
 
-        if (width != s->last_width || height != s->last_height ||
-            cw != s->last_cw || cheight != s->last_ch) {
-            s->last_scr_width = width * cw;
-            s->last_scr_height = height * cheight;
-            s->ds->surface->width = width;
-            s->ds->surface->height = height;
-            dpy_resize(s->ds);
-            s->last_width = width;
-            s->last_height = height;
-            s->last_ch = cheight;
-            s->last_cw = cw;
-            full_update = 1;
-        }
-
         /* Update "hardware" cursor */
         cursor_offset = ((s->cr[0x0e] << 8) | s->cr[0x0f]) - s->start_addr;
         if (cursor_offset != s->cursor_offset ||
@@ -2218,7 +2247,8 @@
 #endif
 
     /* force refresh */
-    s->graphic_mode = -1;
+    vga_update_resolution(s);
+    s->want_full_update = 1;
     return 0;
 }
 
@@ -2641,7 +2671,8 @@
     ds->surface = qemu_create_displaysurface(ds, w, h);
 
     s->ds = ds;
-    s->graphic_mode = -1;
+    vga_update_resolution(s);
+    s->want_full_update = 1;
     vga_update_display(s);
 
     ppm_save(filename, ds->surface);
@@ -2672,10 +2703,16 @@
 {
     VGAState *s = (VGAState *)opaque;
 
-    if (!(s->ar_index & 0x20))
+    switch (s->graphic_mode) {
+    case GMODE_TEXT:
+        vga_screen_dump_text(s, filename);
+        break;
+    case GMODE_GRAPH:
+        vga_screen_dump_graphic(s, filename);
+        break;
+    case GMODE_BLANK:
+    default:
         vga_screen_dump_blank(s, filename);
-    else if (s->gr[6] & 1)
-        vga_screen_dump_graphic(s, filename);
-    else
-        vga_screen_dump_text(s, filename);
+        break;
+    }
 }

Modified: trunk/hw/vga_int.h
===================================================================
--- trunk/hw/vga_int.h	2009-04-05 18:16:10 UTC (rev 6988)
+++ trunk/hw/vga_int.h	2009-04-05 18:41:18 UTC (rev 6989)
@@ -147,8 +147,11 @@
     DisplayState *ds;                                                   \
     uint32_t font_offsets[2];                                           \
     int graphic_mode;                                                   \
+    int want_full_update;                                               \
     uint8_t shift_control;                                              \
     uint8_t double_scan;                                                \
+    uint8_t multi_run;                                                  \
+    uint8_t multi_scan;                                                 \
     uint32_t line_offset;                                               \
     uint32_t line_compare;                                              \
     uint32_t start_addr;                                                \
@@ -195,6 +198,7 @@
                      ram_addr_t vga_ram_offset, int vga_ram_size);
 void vga_init(VGAState *s);
 void vga_reset(void *s);
+void vga_update_resolution(VGAState *s);
 
 void vga_dirty_log_start(VGAState *s);
 void vga_dirty_log_stop(VGAState *s);

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Stefano Stabellini]

Anthony Liguori wrote:

> Revision: 6989
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6989
> Author:   aliguori
> Date:     2009-04-05 18:41:18 +0000 (Sun, 05 Apr 2009)
> Log Message:
> -----------
> Fix display breakage when resizing the screen (v2) (Avi Kivity)
> 
> When the vga resolution changes, a new display surface is not allocated
> immediately; instead that is deferred until the next update.  However,
> if we're running without a display client attached, that won't happen
> and the next bitblt is likely to cause a segfault by overflowing the
> display surface.
> 
> Fix by reallocating the display immediately when the resolution changes.
> 
> Tested with (Windows|Linux) x (cirrus|std) x (curses|sdl).
> 
> Changes from v1:
>  - fix segfault when switching virtual consoles with curses
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 

While I understand the problem this patch tries to fix I think the way
it fixes it is wrong.

First of all this patch breaks the VC switching mechanism in qemu: just
execute sleep 2; startx; in the first VC, then switch to the monitor and
wait.
Secondly it doesn't follow the basic idea behind the DisplayState
surface: it is supposed to be a pixel surface provided by the vga
emulator to the frontends, cirrus shouldn't have to care what size it is.
In fact cirrus emulates bitblit operations on the emulated framebuffer,
not on the DisplayState surface; if it does so is a bug and should be fixed.

For these reasons I think this patch should be reverted.

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Anthony Liguori]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>
> While I understand the problem this patch tries to fix I think the way
> it fixes it is wrong.
>
> First of all this patch breaks the VC switching mechanism in qemu: just
> execute sleep 2; startx; in the first VC, then switch to the monitor and
> wait.
> Secondly it doesn't follow the basic idea behind the DisplayState
> surface: it is supposed to be a pixel surface provided by the vga
> emulator to the frontends, cirrus shouldn't have to care what size it is.
> In fact cirrus emulates bitblit operations on the emulated framebuffer,
> not on the DisplayState surface; if it does so is a bug and should be fixed.
>
> For these reasons I think this patch should be reverted.
>   

Avi/Stefano, can one of you guys put together a proper patch?

Right now, it's better not to revert this patch because without it, our 
test suite cannot run.  However, I agree this is the wrong fix so we 
need to get a proper fix as soon as we can.

Regards,

Anthony Liguori

>
>   

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Anthony Liguori wrote:
>>
>> While I understand the problem this patch tries to fix I think the way
>> it fixes it is wrong.
>>
>> First of all this patch breaks the VC switching mechanism in qemu: just
>> execute sleep 2; startx; in the first VC, then switch to the monitor and
>> wait.
>> Secondly it doesn't follow the basic idea behind the DisplayState
>> surface: it is supposed to be a pixel surface provided by the vga
>> emulator to the frontends, cirrus shouldn't have to care what size it is.
>> In fact cirrus emulates bitblit operations on the emulated framebuffer,
>> not on the DisplayState surface; if it does so is a bug and should be fixed.
>>
>> For these reasons I think this patch should be reverted.
>>   
> 
> Avi/Stefano, can one of you guys put together a proper patch?
> 
> Right now, it's better not to revert this patch because without it, our 
> test suite cannot run.  However, I agree this is the wrong fix so we 
> need to get a proper fix as soon as we can.
> 

Can you please let me know a simple way to reproduce the breakage Avi
was trying to fix?

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Stefano Stabellini wrote:
> Anthony Liguori wrote:
>
>   
>> Revision: 6989
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=6989
>> Author:   aliguori
>> Date:     2009-04-05 18:41:18 +0000 (Sun, 05 Apr 2009)
>> Log Message:
>> -----------
>> Fix display breakage when resizing the screen (v2) (Avi Kivity)
>>
>> When the vga resolution changes, a new display surface is not allocated
>> immediately; instead that is deferred until the next update.  However,
>> if we're running without a display client attached, that won't happen
>> and the next bitblt is likely to cause a segfault by overflowing the
>> display surface.
>>
>> Fix by reallocating the display immediately when the resolution changes.
>>
>> Tested with (Windows|Linux) x (cirrus|std) x (curses|sdl).
>>
>> Changes from v1:
>>  - fix segfault when switching virtual consoles with curses
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>>     
>
> While I understand the problem this patch tries to fix I think the way
> it fixes it is wrong.
>
> First of all this patch breaks the VC switching mechanism in qemu: just
> execute sleep 2; startx; in the first VC, then switch to the monitor and
> wait.
>   

I guess, this can be fixed by not reallocating the display surface if 
vga is not the active console?

> Secondly it doesn't follow the basic idea behind the DisplayState
> surface: it is supposed to be a pixel surface provided by the vga
> emulator to the frontends, cirrus shouldn't have to care what size it is.
> In fact cirrus emulates bitblit operations on the emulated framebuffer,
> not on the DisplayState surface; if it does so is a bug and should be fixed.
>   

This happens through cirrus_do_copy(), which implies that the display 
surface is involved.  Is that wrong?


> For these reasons I think this patch should be reverted

Can you suggest a better fix?  The failure (without the patch) is pretty 
disastrous from my point of view.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Stefano Stabellini wrote:
> Can you please let me know a simple way to reproduce the breakage Avi
> was trying to fix?
>   

The trigger is kvm-autotest; I think you can easily reproduce it by 
booting a guest with vnc (unconnected), and issueing a screendump 
through the monitor (use -monitor stdio, not on the console...) every 
second.

-vnc may be unnecessary, haven't tried.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Avi Kivity wrote:
>> Secondly it doesn't follow the basic idea behind the DisplayState
>> surface: it is supposed to be a pixel surface provided by the vga
>> emulator to the frontends, cirrus shouldn't have to care what size it 
>> is.
>> In fact cirrus emulates bitblit operations on the emulated framebuffer,
>> not on the DisplayState surface; if it does so is a bug and should be 
>> fixed.
>>   
>
> This happens through cirrus_do_copy(), which implies that the display 
> surface is involved.  Is that wrong?

I think what you're saying is, we shouldn't call qemu_console_copy() in 
cirrus_do_copy() if vga is not displayed on the console?

(and conversely, if it is displayed, we shouldn't call 
cirrus_invalidate_region).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Stefano Stabellini]

Avi Kivity wrote:

> Avi Kivity wrote:
>>> Secondly it doesn't follow the basic idea behind the DisplayState
>>> surface: it is supposed to be a pixel surface provided by the vga
>>> emulator to the frontends, cirrus shouldn't have to care what size it 
>>> is.
>>> In fact cirrus emulates bitblit operations on the emulated framebuffer,
>>> not on the DisplayState surface; if it does so is a bug and should be 
>>> fixed.
>>>   
>> This happens through cirrus_do_copy(), which implies that the display 
>> surface is involved.  Is that wrong?
> 
> I think what you're saying is, we shouldn't call qemu_console_copy() in 
> cirrus_do_copy() if vga is not displayed on the console?
> 
> (and conversely, if it is displayed, we shouldn't call 
> cirrus_invalidate_region).
> 


I tried several times to manually reproduce the issue with no luck: if
no vnc clients are supposed to be connected, how do you make the cirrus
emulated hw do a bitblit?

In any case it makes sense that the problem is in qemu_console_copy()
rather than in the rop function itself that only operates on the
emulated framebuffer.
But calling qemu_console_copy() in cirrus_do_copy() shouldn't be risky
because before that, we call vga_hw_update() that should do the resize.

Without a better understanding of the bug I will refrain from making any
suggestion.

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Stefano Stabellini wrote:
> I tried several times to manually reproduce the issue with no luck: if
> no vnc clients are supposed to be connected, how do you make the cirrus
> emulated hw do a bitblit?
>   

The guest will boot and do it by itself.

> In any case it makes sense that the problem is in qemu_console_copy()
> rather than in the rop function itself that only operates on the
> emulated framebuffer.
> But calling qemu_console_copy() in cirrus_do_copy() shouldn't be risky
> because before that, we call vga_hw_update() that should do the resize.
>   

vga_hw_update() may not actually do anything if the conditions are 
right, and maybe that's the case here.

> Without a better understanding of the bug I will refrain from making any
> suggestion.
>
>   

There's a backtrace in 
https://bugzilla.redhat.com/show_bug.cgi?id=494002.  I'll also try to 
generate a good scenario for reproducing this.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Stefano Stabellini]

Avi Kivity wrote:

> Stefano Stabellini wrote:
>> I tried several times to manually reproduce the issue with no luck: if
>> no vnc clients are supposed to be connected, how do you make the cirrus
>> emulated hw do a bitblit?
>>   
> 
> The guest will boot and do it by itself.


what is the OS?

 
>> In any case it makes sense that the problem is in qemu_console_copy()
>> rather than in the rop function itself that only operates on the
>> emulated framebuffer.
>> But calling qemu_console_copy() in cirrus_do_copy() shouldn't be risky
>> because before that, we call vga_hw_update() that should do the resize.
>>   
> 
> vga_hw_update() may not actually do anything if the conditions are 
> right, and maybe that's the case here.

I thought about it, but if vga_hw_update does nothing, then
qemu_console_copy does nothing too.

>> Without a better understanding of the bug I will refrain from making any
>> suggestion.
>>
>>   
> 
> There's a backtrace in 
> https://bugzilla.redhat.com/show_bug.cgi?id=494002.  I'll also try to 
> generate a good scenario for reproducing this.
> 


thanks, that would be very useful (if it doesn't require me to compile
kvm it would be even better :)).

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Stefano Stabellini wrote:
> Avi Kivity wrote:
>
>   
>> Stefano Stabellini wrote:
>>     
>>> I tried several times to manually reproduce the issue with no luck: if
>>> no vnc clients are supposed to be connected, how do you make the cirrus
>>> emulated hw do a bitblit?
>>>   
>>>       
>> The guest will boot and do it by itself.
>>     
>
>
> what is the OS?
>   

In the cases I saw, I think it was Windows.  Sorry, I can't be sure and 
I didn't keep any records.

>>> In any case it makes sense that the problem is in qemu_console_copy()
>>> rather than in the rop function itself that only operates on the
>>> emulated framebuffer.
>>> But calling qemu_console_copy() in cirrus_do_copy() shouldn't be risky
>>> because before that, we call vga_hw_update() that should do the resize.
>>>   
>>>       
>> vga_hw_update() may not actually do anything if the conditions are 
>> right, and maybe that's the case here.
>>     
>
> I thought about it, but if vga_hw_update does nothing, then
> qemu_console_copy does nothing too.
>   

True.

>>> Without a better understanding of the bug I will refrain from making any
>>> suggestion.
>>>
>>>   
>>>       
>> There's a backtrace in 
>> https://bugzilla.redhat.com/show_bug.cgi?id=494002.  I'll also try to 
>> generate a good scenario for reproducing this.
>>
>>     
>
>
> thanks, that would be very useful (if it doesn't require me to compile
> kvm it would be even better :)).
>   

If you're running a recent distro it's already built in :)

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Ryan Harper]

* Avi Kivity <avi@redhat.com> [2009-04-06 10:21]:
> Stefano Stabellini wrote:
> >Can you please let me know a simple way to reproduce the breakage Avi
> >was trying to fix?
> >  
> 
> The trigger is kvm-autotest; I think you can easily reproduce it by 
> booting a guest with vnc (unconnected), and issueing a screendump 
> through the monitor (use -monitor stdio, not on the console...) every 
> second.

yeah, and btw, with this patch merged into kvm-userspace as of today,
I'm still tripping over it[1] on some guests (RHEL5.3 x86_64 install).


1. http://pastebin.ca/1384339


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Avi Kivity]

Ryan Harper wrote:
> * Avi Kivity <avi@redhat.com> [2009-04-06 10:21]:
>   
>> Stefano Stabellini wrote:
>>     
>>> Can you please let me know a simple way to reproduce the breakage Avi
>>> was trying to fix?
>>>  
>>>       
>> The trigger is kvm-autotest; I think you can easily reproduce it by 
>> booting a guest with vnc (unconnected), and issueing a screendump 
>> through the monitor (use -monitor stdio, not on the console...) every 
>> second.
>>     
>
> yeah, and btw, with this patch merged into kvm-userspace as of today,
> I'm still tripping over it[1] on some guests (RHEL5.3 x86_64 install).
>
>
> 1. http://pastebin.ca/1384339
>
>   

I found the real cause - vga_screen_dump() left values for the current 
display resolution in s->last_width and s->last_height.  This caused the 
next vga_draw_graphic() to render to the 640x400 display surface it had 
without resizing it.

The bug is triggered if you manage to screendump after a screen 
resolution change, but before the next bitblit or console refresh.  If 
no vnc or sdl client is connected, that's just the next bitblt so the 
window is considerably longer.

I'll follow up with a patch.  Anthony, please revert 6989 before 
applying it.  Thanks to Stefano for pointing out the problems with the 
patch.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [6989] Fix display breakage when resizing the screen (v2) (Avi Kivity)

[Stefano Stabellini]

Avi Kivity wrote:

> I found the real cause - vga_screen_dump() left values for the current 
> display resolution in s->last_width and s->last_height.  This caused the 
> next vga_draw_graphic() to render to the 640x400 display surface it had 
> without resizing it.
> 
> The bug is triggered if you manage to screendump after a screen 
> resolution change, but before the next bitblit or console refresh.  If 
> no vnc or sdl client is connected, that's just the next bitblt so the 
> window is considerably longer.


This explains why it is so hard to reproduce.

 
> I'll follow up with a patch.  Anthony, please revert 6989 before 
> applying it.  Thanks to Stefano for pointing out the problems with the 
> patch.
> 


No problem :)