Skip to contentSkip to conversation

DisplayState interface change

PATCH In review Stefano Stabellini 2 participants 3 messages Nov 26, 2008 – Dec 11, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 DisplayState interface change

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) together with console.c that fully manage 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 interesting changes are:

- the new structures and related functions in console.h and console.c

in particular the following functions are very helpful to manage a DisplaySurface:

qemu_createDisplaySurface
qemu_resizeDisplaySurface
qemu_createDisplaySurfaceFrom
qemu_freeDisplaySurface

- console_select and qemu_console_resize in console.c this two functions manage multiple consoles on a single host display

- moving code around in hw/vga.c
as for the shared_buf patch this is necessary to be able to handle a dynamic DisplaySurface bpp

- changes to vga_draw_graphic in hw/vga.c
this is the place where the DisplaySurface buffer is shared with the videoram, when possible;

Compared to the last version the only changes are:

- do not use qemu_reallocz, use qemu_realloc instead
- do not use is_active_console, use is_graphic_console instead. is_graphic_console is an already existing function in console.c, there is no need to implement a new function that does the same thing and adds another dependency upon QEMUConsole.

---

console.c +144 -27
diff -r f1fd23dcd333 console.c
@@ -1044,12 +1044,15 @@
 
     if (index >= MAX_CONSOLES)
         return;
+    active_console->g_width = ds_get_width(active_console->ds);
+    active_console->g_height = ds_get_height(active_console->ds);
     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 != ds_get_width(s->ds) || s->g_height != ds_get_height(s->ds)))
-            dpy_resize(s->ds, s->g_width, s->g_height);
+        ds->surface = qemu_resizeDisplaySurface(ds->surface, s->g_width,
+                                                s->g_height, 32, 4 * s->g_width);
+        dpy_resize(s->ds);
         vga_hw_invalidate();
     }
 }
@@ -1157,16 +1160,6 @@
 static void text_console_invalidate(void *opaque)
 {
     TextConsole *s = (TextConsole *) opaque;
-
-    if (s->g_width != ds_get_width(s->ds) || s->g_height != ds_get_height(s->ds)) {
-        if (s->console_type == TEXT_CONSOLE_FIXED_SIZE)
-            dpy_resize(s->ds, s->g_width, s->g_height);
-        else {
-            s->g_width = ds_get_width(s->ds);
-            s->g_height = ds_get_height(s->ds);
-            text_console_resize(s);
-        }
-    }
     console_refresh(s);
 }
 
@@ -1346,13 +1339,12 @@
 
 void qemu_console_resize(QEMUConsole *console, int width, int height)
 {
-    if (console->g_width != width || console->g_height != height
-        || !ds_get_data(console->ds)) {
-        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;
+    if (active_console == console) {
+        DisplayState *ds = console->ds;
+        ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width);
+        dpy_resize(console->ds);
     }
 }
 
@@ -1360,12 +1352,137 @@
                 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);
     }
 }
+
+static PixelFormat qemu_default_pixelformat(int bpp)
+{
+    PixelFormat pf;
+
+    memset(&pf, 0x00, sizeof(PixelFormat));
+
+    pf.bits_per_pixel = bpp;
+    pf.bytes_per_pixel = bpp / 8;
+    pf.depth = bpp == 32 ? 24 : bpp;
+
+    switch (bpp) {
+        case 8:
+            pf.rmask = 0x000000E0;
+            pf.gmask = 0x0000001C;
+            pf.bmask = 0x00000003;
+            pf.rmax = 7;
+            pf.gmax = 7;
+            pf.bmax = 3;
+            pf.rshift = 5;
+            pf.gshift = 2;
+            pf.bshift = 0;
+            break;
+        case 16:
+            pf.rmask = 0x0000F800;
+            pf.gmask = 0x000007E0;
+            pf.bmask = 0x0000001F;
+            pf.rmax = 31;
+            pf.gmax = 63;
+            pf.bmax = 31;
+            pf.rshift = 11;
+            pf.gshift = 5;
+            pf.bshift = 0;
+            break;
+        case 24:
+        case 32:
+            pf.rmask = 0x00FF0000;
+            pf.gmask = 0x0000FF00;
+            pf.bmask = 0x000000FF;
+            pf.rmax = 255;
+            pf.gmax = 255;
+            pf.bmax = 255;
+            pf.rshift = 16;
+            pf.gshift = 8;
+            pf.bshift = 0;
+            break;
+        default:
+            break;
+    }
+    return pf;
+}
+
+DisplaySurface* qemu_createDisplaySurface(int width, int height, int bpp, int linesize)
+{
+    DisplaySurface *surface = (DisplaySurface*) qemu_mallocz(sizeof(DisplaySurface));
+    if (surface == NULL) {
+        fprintf(stderr, "qemu_createDisplaySurface: malloc failed\n");
+        exit(1);
+    }
+
+    surface->width = width;
+    surface->height = height;
+    surface->linesize = linesize;
+    surface->pf = qemu_default_pixelformat(bpp);
+#ifdef WORDS_BIGENDIAN
+    surface->flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
+#else
+    surface->flags = QEMU_ALLOCATED_FLAG;
+#endif
+    surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height);
+    if (surface->data == NULL) {
+        fprintf(stderr, "qemu_createDisplaySurface: malloc failed\n");
+        exit(1);
+    }
+
+    return surface;
+}
+
+DisplaySurface* qemu_resizeDisplaySurface(DisplaySurface *surface,
+                                          int width, int height, int bpp, int linesize)
+{
+    surface->width = width;
+    surface->height = height;
+    surface->linesize = linesize;
+    surface->pf = qemu_default_pixelformat(bpp);
+    if (surface->flags & QEMU_ALLOCATED_FLAG)
+        surface->data = (uint8_t*) qemu_realloc(surface->data, surface->linesize * surface->height);
+    else
+        surface->data = (uint8_t*) qemu_malloc(surface->linesize * surface->height);
+    if (surface->data == NULL) {
+        fprintf(stderr, "qemu_createDisplaySurface: malloc failed\n");
+        exit(1);
+    }
+#ifdef WORDS_BIGENDIAN
+    surface->flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
+#else
+    surface->flags = QEMU_ALLOCATED_FLAG;
+#endif
+
+    return surface;
+}
+
+DisplaySurface* qemu_createDisplaySurfaceFrom(int width, int height, int bpp,
+                                              int linesize, uint8_t *data)
+{
+    DisplaySurface *surface = (DisplaySurface*) qemu_mallocz(sizeof(DisplaySurface));
+    if (surface == NULL) {
+        fprintf(stderr, "qemu_createDisplaySurface: malloc failed\n");
+        exit(1);
+    }
+
+    surface->width = width;
+    surface->height = height;
+    surface->linesize = linesize;
+    surface->pf = qemu_default_pixelformat(bpp);
+#ifdef WORDS_BIGENDIAN
+    surface->flags = QEMU_BIG_ENDIAN_FLAG;
+#endif
+    surface->data = data;
+
+    return surface;
+}
+
+void qemu_freeDisplaySurface(DisplaySurface *surface)
+{
+    if (surface == NULL)
+        return;
+    if (surface->flags & QEMU_ALLOCATED_FLAG)
+        qemu_free(surface->data);
+    qemu_free(surface);
+}
console.h +118 -20

188 lines not on this page · raw message

curses.c +20 -18
diff -r f1fd23dcd333 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_get_width(ds) == gwidth && ds_get_height(ds) == gheight)
         return;
 
-    gwidth = w;
-    gheight = h;
+    gwidth = ds_get_width(ds);
+    gheight = ds_get_height(ds);
 
     curses_calc_pad();
 }
@@ -169,8 +169,8 @@
         clear();
         refresh();
         curses_calc_pad();
-        ds->width = FONT_WIDTH * width;
-        ds->height = FONT_HEIGHT * height;
+        ds->surface->width = FONT_WIDTH * width;
+        ds->surface->height = FONT_HEIGHT * height;
         vga_hw_invalidate();
         invalidate = 0;
     }
@@ -197,8 +197,8 @@
             refresh();
             curses_calc_pad();
             curses_update(ds, 0, 0, width, height);
-            ds->width = FONT_WIDTH * width;
-            ds->height = FONT_HEIGHT * height;
+            ds->surface->width = FONT_WIDTH * width;
+            ds->surface->height = FONT_HEIGHT * height;
             continue;
         }
 #endif
@@ -338,6 +338,7 @@
 
 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 +358,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);
+    qemu_freeDisplaySurface(ds->surface);
+    ds->surface = qemu_createDisplaySurfaceFrom(80, 25, 0, 0, (uint8_t*) screen);
 
     invalidate = 1;
 
     /* Standard VGA initial text mode dimensions */
-    curses_resize(ds, 80, 25);
+    curses_resize(ds);
 }
hw/cirrus_vga.c +9 -15
diff -r f1fd23dcd333 hw/cirrus_vga.c
@@ -789,22 +789,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/nseries.c +2 -1
diff -r f1fd23dcd333 hw/nseries.c
@@ -1361,7 +1361,8 @@
     /* FIXME: We shouldn't really be doing this here.  The LCD controller
        will set the size once configured, so this just sets an initial
        size until the guest activates the display.  */
-    dpy_resize(ds, 800, 480);
+    ds->surface = qemu_resizeDisplaySurface(ds->surface, 800, 480, 32, 4 * 800);
+    dpy_resize(ds);
 }
 
 static struct arm_boot_info n800_binfo = {
hw/palm.c +2 -1
diff -r f1fd23dcd333 hw/palm.c
@@ -278,7 +278,8 @@
     /* FIXME: We shouldn't really be doing this here.  The LCD controller
        will set the size once configured, so this just sets an initial
        size until the guest activates the display.  */
-    dpy_resize(ds, 320, 320);
+    ds->surface = qemu_resizeDisplaySurface(ds->surface, 320, 320, 32, 4 * 320);
+    dpy_resize(ds);
 }
 
 QEMUMachine palmte_machine = {
hw/vga.c +68 -34

212 lines not on this page · raw message

hw/vga_int.h +2 -0
diff -r f1fd23dcd333 hw/vga_int.h
@@ -155,9 +155,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,                        \
qemu-common.h +3 -0
diff -r f1fd23dcd333 qemu-common.h
@@ -146,6 +146,9 @@
 typedef struct AudioState AudioState;
 typedef struct BlockDriverState BlockDriverState;
 typedef struct DisplayState DisplayState;
+typedef struct DisplayChangeListener DisplayChangeListener;
+typedef struct DisplaySurface DisplaySurface;
+typedef struct PixelFormat PixelFormat;
 typedef struct TextConsole TextConsole;
 typedef TextConsole QEMUConsole;
 typedef struct CharDriverState CharDriverState;
sdl.c +49 -37

154 lines not on this page · raw message

vl.c +60 -37

172 lines not on this page · raw message

vnc.c +34 -73

222 lines not on this page · raw message

#1 lore raw
Quoted 22 lines · Stefano Stabellini

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) together with console.c that fully manage 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 interesting changes are:

- the new structures and related functions in console.h and console.c

Quoted 8 lines

in particular the following functions are very helpful to manage a DisplaySurface:

qemu_createDisplaySurface
qemu_resizeDisplaySurface
qemu_createDisplaySurfaceFrom
qemu_freeDisplaySurface

These names suck. Please change them to something more in line with the rest of the code.

Quoted 18 lines

---
diff -r f1fd23dcd333 hw/cirrus_vga.c

hw/cirrus_vga.c +0 -8
@@ -789,22 +789,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 +

hw/cirrus_vga.c line 798

Quoted 10 lines
hw/cirrus_vga.c +6 -3
-                (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);
 

You've eliminated the dpy_copy optimization? This is extremely important for VNC.

Regards,

Anthony Liguori

#2 lore raw
Quoted 4 lines · Anthony Liguori
Quoted 3 lines · Stefano Stabellini

in particular the following functions are very helpful to manage a DisplaySurface:

Quoted 8 lines
Quoted 5 lines

qemu_createDisplaySurface
qemu_resizeDisplaySurface
qemu_createDisplaySurfaceFrom
qemu_freeDisplaySurface

These names suck. Please change them to something more in line with the rest of the code.

Are the capital letters that sucks, or the names?

qemu_create_displaysurface
qemu_resize_displaysurface
qemu_create_displaysurface_from
qemu_free_displaysurface

Are these any better?

Quoted 20 lines
Quoted 20 lines

diff -r f1fd23dcd333 hw/cirrus_vga.c

hw/cirrus_vga.c Wed Nov 26 11:43:17 2008 +0000 renamed from hw/cirrus_vga.c Tue Nov 25 12:28:31 2008 +0000 +2 -11
@@ -789,22 +789,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),
Quoted 9 lines

hw/cirrus_vga.c Wed Nov 26 11:43:17 2008 +0000 line 798

Quoted 5 lines
hw/cirrus_vga.c Wed Nov 26 11:43:17 2008 +0000 renamed from hw/cirrus_vga.c Tue Nov 25 12:28:31 2008 +0000 +4 -0
+            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);
 

You've eliminated the dpy_copy optimization? This is extremely important for VNC.

My approach has been too radical.
I need to remove the memmove from vnc.c, but I should be able to keep the dpy_copy optimization.