DisplayState interface change

PATCH In review Stefano Stabellini 2 participants 3 messages Nov 17, 2008

Share & tools

Stefano Stabellini

author

2008-11-17 12:41 UTC #0 lore raw

PATCH 1/1 DisplayState interface change

Hi all,
this patch changes the DisplayState interface adding support for multiple frontends at the same time (sdl and vnc) and implements most of the benefit of the shared_buf patch without the added complexity.

Currently DisplayState is managed by sdl (or vnc) and sdl (or vnc) is also responsible for allocating the data and setting the depth. Vga.c (or another backend) will do any necessary conversion.

The idea is to change it so that is vga.c (or another backend) that fully manages the DisplayState interface allocating data and setting the depth (either 16 or 32 bit, if the guest uses a different resolution or is in text mode, vga.c (or another backend) is in charge of doing the conversion seamlessly).

The other idea is that DisplayState supports *multiple* frontends like sdl and vnc; each of them can register some callbacks to be called when a display event occurs.

The shared_buf flag is still there to advertise that the buffer is currently not allocated but points directly to video ram.
It would be nice to get rid of it but it is still necessary due to multiple consoles that can share the same DisplayState.
It is probably possible to further simplify this patch making different assumptions in console.c, any suggestion is welcome.

I hope you think this patch is more elegant and functional than the previous shared_buf patch, enough to be merged soon :)

I am very open to any suggestion and (constructive) critics.

Cheers,

Stefano Stabellini

Signed-off-by Stefano Stabellini <[email protected]>

---

console.c +46 ~~-26~~

diff -r 5c78dd111aae console.c
@@ -126,6 +126,8 @@
     void *hw;
 
     int g_width, g_height;
+    int g_depth;
+    int g_linesize;
     int width;
     int height;
     int total_height;

@@ -1046,10 +1048,18 @@
         return;
     s = consoles[index];
     if (s) {
+        DisplayState *ds = s->ds;
         active_console = s;
-        if (s->console_type != TEXT_CONSOLE && s->g_width && s->g_height
-            && (s->g_width != s->ds->width || s->g_height != s->ds->height))
-            dpy_resize(s->ds, s->g_width, s->g_height);
+        ds->depth = s->g_depth;
+        ds->linesize = s->g_linesize;
+        ds->width = s->g_width;
+        ds->height = s->g_height;
+        if (ds->shared_buf) {
+            ds->shared_buf = 0;
+            ds->data = 0;
+        }
+        ds->data_resize(ds);
+        dpy_resize(s->ds);
         vga_hw_invalidate();
     }
 }

@@ -1158,15 +1168,6 @@
 {
     TextConsole *s = (TextConsole *) opaque;
 
-    if (s->g_width != s->ds->width || s->g_height != s->ds->height) {
-        if (s->console_type == TEXT_CONSOLE_FIXED_SIZE)
-            dpy_resize(s->ds, s->g_width, s->g_height);
-        else {
-            s->g_width = s->ds->width;
-            s->g_height = s->ds->height;
-            text_console_resize(s);
-        }
-    }
     console_refresh(s);
 }

@@ -1247,6 +1248,11 @@
     return s;
 }
 
+int is_active_console(QEMUConsole *s)
+{
+    return (active_console == s);
+}
+
 int is_graphic_console(void)
 {
     return active_console && active_console->console_type == GRAPHIC_CONSOLE;

@@ -1321,6 +1327,8 @@
     }
     s->g_width = width;
     s->g_height = height;
+    s->g_linesize = 4 * width;
+    s->g_depth = 32;
 
     s->hw_invalidate = text_console_invalidate;
     s->hw_text_update = text_console_update;

@@ -1344,15 +1352,33 @@
     return chr;
 }
 
-void qemu_console_resize(QEMUConsole *console, int width, int height)
+void qemu_console_resize(QEMUConsole *console, int width, int height,
+                         int depth, int linesize, uint8_t *data)
 {
-    if (console->g_width != width || console->g_height != height
-        || !console->ds->data) {
-        console->g_width = width;
-        console->g_height = height;
-        if (active_console == console) {
-            dpy_resize(console->ds, width, height);
+    console->g_width = width;
+    console->g_height = height;
+    console->g_depth = depth;
+    console->g_linesize = linesize;
+    if (active_console == console) {
+        DisplayState *ds = console->ds;
+        ds->depth = depth;
+        ds->linesize = linesize;
+        ds->width = width;
+        ds->height = height;
+        if (data != NULL) {
+            if (!ds->shared_buf) {
+                qemu_free(ds->data);
+                ds->shared_buf = 1;
+            }
+            ds->data = data;
+        } else {
+            if (ds->shared_buf) {
+                ds->data = 0;
+                ds->shared_buf = 0;
+            }
+            ds->data_resize(ds);
         }
+        dpy_resize(console->ds);
     }
 }

@@ -1360,12 +1386,6 @@
                 int dst_x, int dst_y, int w, int h)
 {
     if (active_console == console) {
-        if (console->ds->dpy_copy)
-            console->ds->dpy_copy(console->ds,
-                            src_x, src_y, dst_x, dst_y, w, h);
-        else {
-            /* TODO */
-            console->ds->dpy_update(console->ds, dst_x, dst_y, w, h);
-        }
+            dpy_copy(console->ds, src_x, src_y, dst_x, dst_y, w, h);
     }
 }

console.h +86 ~~-18~~

142 lines not on this page · raw message

curses.c +21 ~~-14~~

diff -r 5c78dd111aae curses.c
@@ -97,13 +97,13 @@
     }
 }
 
-static void curses_resize(DisplayState *ds, int w, int h)
+static void curses_resize(DisplayState *ds)
 {
-    if (w == gwidth && h == gheight)
+    if (ds->width == gwidth && ds->height == gheight)
         return;
 
-    gwidth = w;
-    gheight = h;
+    gwidth = ds->width;
+    gheight = ds->height;
 
     curses_calc_pad();
 }

@@ -336,8 +336,14 @@
     }
 }
 
+static void data_resize(DisplayState *ds)
+{
+    ds->data = (void *) screen;
+}
+
 void curses_display_init(DisplayState *ds, int full_screen)
 {
+    DisplayChangeListener *dcl;
 #ifndef _WIN32
     if (!isatty(1)) {
         fprintf(stderr, "We need a terminal output\n");

@@ -357,18 +363,19 @@
 #endif
 #endif
 
-    ds->data = (void *) screen;
-    ds->linesize = 0;
-    ds->depth = 0;
-    ds->width = 640;
-    ds->height = 400;
-    ds->dpy_update = curses_update;
-    ds->dpy_resize = curses_resize;
-    ds->dpy_refresh = curses_refresh;
-    ds->dpy_text_cursor = curses_cursor_position;
+    dcl = (DisplayChangeListener *) qemu_mallocz(sizeof(DisplayChangeListener));
+    if (!dcl)
+        exit(1);
+    dcl->dpy_update = curses_update;
+    dcl->dpy_resize = curses_resize;
+    dcl->dpy_refresh = curses_refresh;
+    dcl->dpy_text_cursor = curses_cursor_position;
+    register_displaychangelistener(ds, dcl);
+    ds->data_resize = data_resize;
+    data_resize(ds);
 
     invalidate = 1;
 
     /* Standard VGA initial text mode dimensions */
-    curses_resize(ds, 80, 25);
+    curses_resize(ds);
 }

hw/blizzard.c +1 -1

diff -r 5c78dd111aae hw/blizzard.c
@@ -897,7 +897,7 @@
 
     if (s->x != s->state->width || s->y != s->state->height) {
         s->invalidate = 1;
-        qemu_console_resize(s->console, s->x, s->y);
+        qemu_console_resize(s->console, s->x, s->y, 32, 4 * s->x, NULL);
     }
 
     if (s->invalidate) {

hw/cirrus_vga.c +9 ~~-15~~

diff -r 5c78dd111aae hw/cirrus_vga.c
@@ -788,22 +788,16 @@
     if (BLTUNSAFE(s))
         return 0;
 
-    if (s->ds->dpy_copy) {
-	cirrus_do_copy(s, s->cirrus_blt_dstaddr - s->start_addr,
-		       s->cirrus_blt_srcaddr - s->start_addr,
-		       s->cirrus_blt_width, s->cirrus_blt_height);
-    } else {
-	(*s->cirrus_rop) (s, s->vram_ptr +
-                (s->cirrus_blt_dstaddr & s->cirrus_addr_mask),
-			  s->vram_ptr +
-                (s->cirrus_blt_srcaddr & s->cirrus_addr_mask),
-			  s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch,
-			  s->cirrus_blt_width, s->cirrus_blt_height);
+    (*s->cirrus_rop) (s, s->vram_ptr +
+            (s->cirrus_blt_dstaddr & s->cirrus_addr_mask),
+            s->vram_ptr +
+            (s->cirrus_blt_srcaddr & s->cirrus_addr_mask),
+            s->cirrus_blt_dstpitch, s->cirrus_blt_srcpitch,
+            s->cirrus_blt_width, s->cirrus_blt_height);
 
-	cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
-				 s->cirrus_blt_dstpitch, s->cirrus_blt_width,
-				 s->cirrus_blt_height);
-    }
+    cirrus_invalidate_region(s, s->cirrus_blt_dstaddr,
+            s->cirrus_blt_dstpitch, s->cirrus_blt_width,
+            s->cirrus_blt_height);
 
     return 1;
 }

hw/g364fb.c +1 -1

diff -r 5c78dd111aae hw/g364fb.c
@@ -132,7 +132,7 @@
         full_update = 1;
     }
     if (s->scr_width != s->ds->width || s->scr_height != s->ds->height) {
-        qemu_console_resize(s->console, s->scr_width, s->scr_height);
+        qemu_console_resize(s->console, s->scr_width, s->scr_height, 32, 4 * s->scr_width, NULL);
         full_update = 1;
     }
     switch(graphic_mode) {

hw/jazz_led.c +2 -2

diff -r 5c78dd111aae hw/jazz_led.c
@@ -292,7 +292,7 @@
     char buf[2];
 
     dpy_cursor(s->ds, -1, -1);
-    qemu_console_resize(s->console, 2, 1);
+    qemu_console_resize(s->console, 2, 1, 32, 4 * 2, NULL);
 
     /* TODO: draw the segments */
     snprintf(buf, 2, "%02hhx\n", s->segments);

@@ -322,5 +322,5 @@
                                      jazz_led_invalidate_display,
                                      jazz_led_screen_dump,
                                      jazz_led_text_update, s);
-    qemu_console_resize(s->console, 60, 80);
+    qemu_console_resize(s->console, 60, 80, 32, 4 * 60, NULL);
 }

hw/musicpal.c +1 -1

diff -r 5c78dd111aae hw/musicpal.c
@@ -930,7 +930,7 @@
 
     s->console = graphic_console_init(ds, lcd_refresh, lcd_invalidate,
                                       NULL, NULL, s);
-    qemu_console_resize(s->console, 128*3, 64*3);
+    qemu_console_resize(s->console, 128*3, 64*3, 32, 4*128*3, NULL);
 }
 
 /* PIC register offsets */

hw/omap_lcdc.c +1 -1

diff -r 5c78dd111aae hw/omap_lcdc.c
@@ -177,7 +177,7 @@
     if (width != omap_lcd->state->width ||
             omap_lcd->height != omap_lcd->state->height) {
         qemu_console_resize(omap_lcd->console,
-                            omap_lcd->width, omap_lcd->height);
+                            omap_lcd->width, omap_lcd->height, 32, 4 * omap_lcd->width, NULL);
         omap_lcd->invalidate = 1;
     }

hw/pl110.c +2 -2

diff -r 5c78dd111aae hw/pl110.c
@@ -272,7 +272,7 @@
 {
     if (width != s->cols || height != s->rows) {
         if (pl110_enabled(s)) {
-            qemu_console_resize(s->console, width, height);
+            qemu_console_resize(s->console, width, height, 32, 4 * width, NULL);
         }
     }
     s->cols = width;

@@ -389,7 +389,7 @@
         s->cr = val;
         s->bpp = (val >> 1) & 7;
         if (pl110_enabled(s)) {
-            qemu_console_resize(s->console, s->cols, s->rows);
+            qemu_console_resize(s->console, s->cols, s->rows, 32, 4 * s->cols, NULL);
         }
         break;
     case 10: /* LCDICR */

hw/pxa2xx_lcd.c +2 -2

diff -r 5c78dd111aae hw/pxa2xx_lcd.c
@@ -795,9 +795,9 @@
 
     if (width != s->xres || height != s->yres) {
         if (s->orientation)
-            qemu_console_resize(s->console, height, width);
+            qemu_console_resize(s->console, height, width, 32, 4 * height, NULL);
         else
-            qemu_console_resize(s->console, width, height);
+            qemu_console_resize(s->console, width, height, 32, 4 * width, NULL);
         s->invalidated = 1;
         s->xres = width;
         s->yres = height;

hw/ssd0303.c +1 -1

diff -r 5c78dd111aae hw/ssd0303.c
@@ -318,6 +318,6 @@
     s->console = graphic_console_init(ds, ssd0303_update_display,
                                       ssd0303_invalidate_display,
                                       NULL, NULL, s);
-    qemu_console_resize(s->console, 96 * MAGNIFY, 16 * MAGNIFY);
+    qemu_console_resize(s->console, 96 * MAGNIFY, 16 * MAGNIFY, 32, 4 * 96 * MAGNIFY, NULL);
     register_savevm("ssd0303_oled", -1, 1, ssd0303_save, ssd0303_load, s);
 }

hw/ssd0323.c +1 -1

diff -r 5c78dd111aae hw/ssd0323.c
@@ -334,7 +334,7 @@
     s->console = graphic_console_init(ds, ssd0323_update_display,
                                       ssd0323_invalidate_display,
                                       NULL, NULL, s);
-    qemu_console_resize(s->console, 128 * MAGNIFY, 64 * MAGNIFY);
+    qemu_console_resize(s->console, 128 * MAGNIFY, 64 * MAGNIFY, 32, 4 * 128 * MAGNIFY, NULL);
 
     cmd = qemu_allocate_irqs(ssd0323_cd, s, 1);
     *cmd_p = *cmd;

hw/tc6393xb.c +1 -1

diff -r 5c78dd111aae hw/tc6393xb.c
@@ -486,7 +486,7 @@
         full_update = 1;
     }
     if (s->scr_width != s->ds->width || s->scr_height != s->ds->height) {
-        qemu_console_resize(s->console, s->scr_width, s->scr_height);
+        qemu_console_resize(s->console, s->scr_width, s->scr_height, 32, 4 * s->scr_width, NULL);
         full_update = 1;
     }
     if (s->blanked)

hw/tcx.c +1 -1

diff -r 5c78dd111aae hw/tcx.c
@@ -570,7 +570,7 @@
     register_savevm("tcx", addr, 4, tcx_save, tcx_load, s);
     qemu_register_reset(tcx_reset, s);
     tcx_reset(s);
-    qemu_console_resize(s->console, width, height);
+    qemu_console_resize(s->console, width, height, 32, 4 * width, NULL);
 }
 
 static void tcx_screen_dump(void *opaque, const char *filename)

hw/vga.c +53 ~~-38~~

212 lines not on this page · raw message

hw/vga_int.h +2 -0

diff -r 5c78dd111aae hw/vga_int.h
@@ -149,9 +149,11 @@
     uint32_t line_compare;                                              \
     uint32_t start_addr;                                                \
     uint32_t plane_updated;                                             \
+    uint32_t last_line_offset;                                          \
     uint8_t last_cw, last_ch;                                           \
     uint32_t last_width, last_height; /* in chars or pixels */          \
     uint32_t last_scr_width, last_scr_height; /* in pixels */           \
+    uint32_t last_depth; /* in bits */                                  \
     uint8_t cursor_start, cursor_end;                                   \
     uint32_t cursor_offset;                                             \
     unsigned int (*rgb_to_pixel)(unsigned int r,                        \

hw/vmware_vga.c +1 -1

diff -r 5c78dd111aae hw/vmware_vga.c
@@ -877,7 +877,7 @@
     if (s->new_width != s->width || s->new_height != s->height) {
         s->width = s->new_width;
         s->height = s->new_height;
-        qemu_console_resize(s->console, s->width, s->height);
+        qemu_console_resize(s->console, s->width, s->height, 32, 4 * s->width, NULL);
         s->invalidated = 1;
     }
 }

qemu-common.h +2 -0

diff -r 5c78dd111aae qemu-common.h
@@ -96,6 +96,7 @@
 
 void *qemu_malloc(size_t size);
 void *qemu_realloc(void *ptr, size_t size);
+void *qemu_reallocz(void *ptr, size_t size);
 void *qemu_mallocz(size_t size);
 void qemu_free(void *ptr);
 char *qemu_strdup(const char *str);

@@ -128,6 +129,7 @@
 typedef struct AudioState AudioState;
 typedef struct BlockDriverState BlockDriverState;
 typedef struct DisplayState DisplayState;
+typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct TextConsole TextConsole;
 typedef TextConsole QEMUConsole;
 typedef struct CharDriverState CharDriverState;

qemu-malloc.c +10 -0

diff -r 5c78dd111aae qemu-malloc.c
@@ -43,6 +43,16 @@
     return realloc(ptr, size);
 }
 
+void *qemu_reallocz(void *ptr, size_t size)
+{
+    void *res;
+    res = realloc(ptr, size);
+    if (!res)
+        return NULL;
+    memset(res, 0, size);
+    return res;
+}
+
 void *qemu_mallocz(size_t size)
 {
     void *ptr;

sdl.c +79 ~~-42~~

195 lines not on this page · raw message

vl.c +77 ~~-38~~

189 lines not on this page · raw message

vnc.c +31 ~~-62~~

206 lines not on this page · raw message

Anthony Liguori

2008-11-17 14:37 UTC #1 lore raw

Quoted 43 lines · Stefano Stabellini

diff -r 5c78dd111aae console.h

console.h +16 ~~-11~~

@@ -73,45 +73,111 @@
 
 /* consoles */
 
-struct DisplayState {
-    uint8_t *data;
-    int linesize;
-    int depth;
-    int bgr; /* BGR color order instead of RGB. Only valid for depth == 32 */
-    int width;
-    int height;
-    void *opaque;
-    struct QEMUTimer *gui_timer;
+struct DisplayChangeListener {
+    int idle;
     uint64_t gui_timer_interval;
-    int idle; /* there is nothing to update (window invisible), set by vnc/sdl */
 
     void (*dpy_update)(struct DisplayState *s, int x, int y, int w, int h);
-    void (*dpy_resize)(struct DisplayState *s, int w, int h);
+    void (*dpy_resize)(struct DisplayState *s);
+    void (*dpy_setdata)(struct DisplayState *s);
     void (*dpy_refresh)(struct DisplayState *s);
     void (*dpy_copy)(struct DisplayState *s, int src_x, int src_y,
                      int dst_x, int dst_y, int w, int h);
     void (*dpy_fill)(struct DisplayState *s, int x, int y,
                      int w, int h, uint32_t c);
     void (*dpy_text_cursor)(struct DisplayState *s, int x, int y);
+
+    struct DisplayChangeListener *next;
+};
+
+struct DisplayState {
+    uint8_t *data;
+    int shared_buf;
+    int linesize;
+    int depth;
+    int width;
+    int height;
+    void *opaque;

console.h line 110

Quoted 10 lines

console.h +6 -0

+    struct QEMUTimer *gui_timer;
+
+    struct DisplayChangeListener* listeners;
+
     void (*mouse_set)(int x, int y, int on);
     void (*cursor_define)(int width, int height, int bpp, int hot_x, int hot_y,
                           uint8_t *image, uint8_t *mask);
+
+    void (*data_resize)(struct DisplayState *s);
 }

If we're going to change DisplayState, and I think it's long over due that we do, then I think we should try to make sure we get it right.

What I would like to see is the buffer information extracted into a separate structure. That buffer should also describe the pixel format in a much more thorough way. There should be, in the very least, an endianness flag, information about the per-color shift and per-color mask, the bits per pixel, the bytes per pixel, the width and the height.

We should also have generic conversion functions to convert from one buffer to another. This would allow us to simplify a lot of things (like screen shot). It also ties in better to other display tool kits. Right now we have too many assumptions about what the pixel layout is from the depth.

The other thing to consider is that we do need some level of bidirectional communication. vmware VGA supports the ability to tell the guest what resolution/depth it ought to be using. You really want to get that information from SDL (at least the depth bits) so that we can avoid blitting. With KVM, we can pretty trivially directly map the SDL buffer directly into the guest's physical memory provided it uses the right format.

Regards,

Anthony Liguori

Stefano Stabellini

author

2008-11-17 16:04 UTC #2 lore raw

Quoted 8 lines · Anthony Liguori

If we're going to change DisplayState, and I think it's long over due that we do, then I think we should try to make sure we get it right.
What I would like to see is the buffer information extracted into a separate structure. That buffer should also describe the pixel format in a much more thorough way. There should be, in the very least, an endianness flag, information about the per-color shift and per-color mask, the bits per pixel, the bytes per pixel, the width and the height.

That should be pretty easy to do, since we already have all those informations.

Quoted 5 lines

We should also have generic conversion functions to convert from one buffer to another. This would allow us to simplify a lot of things (like screen shot). It also ties in better to other display tool kits. Right now we have too many assumptions about what the pixel layout is from the depth.

Those functions right now are in vga_template.h I tried not to change it in this patch.

Quoted 8 lines

The other thing to consider is that we do need some level of bidirectional communication. vmware VGA supports the ability to tell the guest what resolution/depth it ought to be using. You really want to get that information from SDL (at least the depth bits) so that we can avoid blitting. With KVM, we can pretty trivially directly map the SDL buffer directly into the guest's physical memory provided it uses the right format.

I agree that it be nice, we could probably do something like that on xen too.

Please don't ask me to make these big changes (generic functions, bidirectional communication) on top of this patch in order for it to get applied.
This patch is already too long, it would be difficult to accomplish all the things you suggested in a single shot without breaking anything.
I think it is best to do this incrementally and work together on the features suggested after this patch gets applied.