[Qemu-devel] [PATCH] DisplayAllocator interface

[Stefano Stabellini]

Hi all,
this patch adds a DisplayAllocator interface that allows display
frontends (sdl in particular) to provide a preallocated display buffer
for the graphical backend to use.

Whenever a graphical backend cannot use
qemu_create_displaysurface_from because its own internal pixel format
cannot be exported directly (text mode or graphical mode with color
depth 8 or 24), it creates another display buffer in memory using
qemu_create_displaysurface and does the conversion.
This new buffer needs to be blitted into the sdl surface buffer every time
we need to update portions of the screen.
We can avoid this using the DisplayAllocator interace: sdl provides its
own implementation of qemu_create_displaysurface, giving back the sdl
surface buffer directly (as we used to do before the DisplayState
changes).
Since the buffer returned by sdl could be in bgr format we need to put
back in the handlers of that case.

This approach is good if the two following conditions are true:

1) the sdl surface is a software surface that resides in main memory;

2) the host display color depth is either 16 or 32 bpp.

If first condition is false we can have bad performances using sdl
and vnc together.
If the second condition is false performances are certainly not going to
improve but they shouldn't get worse either.

The first condition is always true, at least on linux/X11 systems; but I
believe is true also on other platforms.
The second condition is true in the vast majority of the cases.

This patch should also have the good side effect of solving the sdl
2D slowness malc was reporting on MacOS, because SDL_BlitSurface is not
going to be called anymore when the guest is in text mode or 24bpp.
However the root problem is still present so I suspect we may
still see some slowness on MacOS when the guest is in 32 or 16 bpp.

Signed-off-by: Stefano Stabellini <address@hidden>

---

diff --git a/console.c b/console.c
index 3f2794e..2c3a45d 100644
--- a/console.c
+++ b/console.c
@@ -1068,8 +1068,7 @@ void console_select(unsigned int index)
         DisplayState *ds = s->ds;
         active_console = s;
         if (ds_get_bits_per_pixel(s->ds)) {
-            ds->surface = qemu_resize_displaysurface(ds->surface, s->g_width,
-                    s->g_height, 32, 4 * s->g_width);
+            ds->surface = qemu_resize_displaysurface(ds, s->g_width, 
s->g_height);
         } else {
             s->ds->surface->width = s->width;
             s->ds->surface->height = s->height;
@@ -1277,11 +1276,12 @@ DisplayState *graphic_console_init(vga_hw_update_ptr 
update,
     DisplayState *ds;
 
     ds = (DisplayState *) qemu_mallocz(sizeof(DisplayState));
-    ds->surface = qemu_create_displaysurface(640, 480, 32, 640 * 4);
+    ds->allocator = &default_allocator; 
+    ds->surface = qemu_create_displaysurface(ds, 640, 480);
 
     s = new_console(ds, GRAPHIC_CONSOLE);
     if (s == NULL) {
-        qemu_free_displaysurface(ds->surface);
+        qemu_free_displaysurface(ds);
         qemu_free(ds);
         return NULL;
     }
@@ -1429,7 +1429,7 @@ void qemu_console_resize(DisplayState *ds, int width, int 
height)
     s->g_width = width;
     s->g_height = height;
     if (is_graphic_console()) {
-        ds->surface = qemu_resize_displaysurface(ds->surface, width, height, 
32, 4 * width);
+        ds->surface = qemu_resize_displaysurface(ds, width, height);
         dpy_resize(ds);
     }
 }
@@ -1552,14 +1552,14 @@ PixelFormat qemu_default_pixelformat(int bpp)
     return pf;
 }
 
-DisplaySurface* qemu_create_displaysurface(int width, int height, int bpp, int 
linesize)
+DisplaySurface* defaultallocator_create_displaysurface(int width, int height)
 {
     DisplaySurface *surface = (DisplaySurface*) 
qemu_mallocz(sizeof(DisplaySurface));
 
     surface->width = width;
     surface->height = height;
-    surface->linesize = linesize;
-    surface->pf = qemu_default_pixelformat(bpp);
+    surface->linesize = width * 4;
+    surface->pf = qemu_default_pixelformat(32);
 #ifdef WORDS_BIGENDIAN
     surface->flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
 #else
@@ -1570,13 +1570,13 @@ DisplaySurface* qemu_create_displaysurface(int width, 
int height, int bpp, int l
     return surface;
 }
 
-DisplaySurface* qemu_resize_displaysurface(DisplaySurface *surface,
-                                          int width, int height, int bpp, int 
linesize)
+DisplaySurface* defaultallocator_resize_displaysurface(DisplaySurface *surface,
+                                          int width, int height)
 {
     surface->width = width;
     surface->height = height;
-    surface->linesize = linesize;
-    surface->pf = qemu_default_pixelformat(bpp);
+    surface->linesize = width * 4;
+    surface->pf = qemu_default_pixelformat(32);
     if (surface->flags & QEMU_ALLOCATED_FLAG)
         surface->data = (uint8_t*) qemu_realloc(surface->data, 
surface->linesize * surface->height);
     else
@@ -1607,7 +1607,7 @@ DisplaySurface* qemu_create_displaysurface_from(int 
width, int height, int bpp,
     return surface;
 }
 
-void qemu_free_displaysurface(DisplaySurface *surface)
+void defaultallocator_free_displaysurface(DisplaySurface *surface)
 {
     if (surface == NULL)
         return;
diff --git a/console.h b/console.h
index 8f438e9..1501bb4 100644
--- a/console.h
+++ b/console.h
@@ -113,11 +113,18 @@ struct DisplayChangeListener {
     struct DisplayChangeListener *next;
 };
 
+struct DisplayAllocator {
+    DisplaySurface* (*create_displaysurface)(int width, int height);
+    DisplaySurface* (*resize_displaysurface)(DisplaySurface *surface, int 
width, int height);
+    void (*free_displaysurface)(DisplaySurface *surface);
+};
+
 struct DisplayState {
     struct DisplaySurface *surface;
     void *opaque;
     struct QEMUTimer *gui_timer;
 
+    struct DisplayAllocator* allocator;
     struct DisplayChangeListener* listeners;
 
     void (*mouse_set)(int x, int y, int on);
@@ -129,15 +136,40 @@ struct DisplayState {
 
 void register_displaystate(DisplayState *ds);
 DisplayState *get_displaystate(void);
-DisplaySurface* qemu_create_displaysurface(int width, int height, int bpp, int 
linesize);
-DisplaySurface* qemu_resize_displaysurface(DisplaySurface *surface,
-                                           int width, int height, int bpp, int 
linesize);
 DisplaySurface* qemu_create_displaysurface_from(int width, int height, int bpp,
                                                 int linesize, uint8_t *data);
-void qemu_free_displaysurface(DisplaySurface *surface);
 PixelFormat qemu_different_endianness_pixelformat(int bpp);
 PixelFormat qemu_default_pixelformat(int bpp);
 
+extern struct DisplayAllocator default_allocator;
+DisplayAllocator *register_displayallocator(DisplayState *ds, DisplayAllocator 
*da);
+DisplaySurface* defaultallocator_create_displaysurface(int width, int height);
+DisplaySurface* defaultallocator_resize_displaysurface(DisplaySurface 
*surface, int width, int height);
+void defaultallocator_free_displaysurface(DisplaySurface *surface);
+
+static inline DisplaySurface* qemu_create_displaysurface(DisplayState *ds, int 
width, int height)
+{
+    return ds->allocator->create_displaysurface(width, height);    
+}
+
+static inline DisplaySurface* qemu_resize_displaysurface(DisplayState *ds, int 
width, int height)
+{
+    return ds->allocator->resize_displaysurface(ds->surface, width, height);
+}
+
+static inline void qemu_free_displaysurface(DisplayState *ds)
+{
+    ds->allocator->free_displaysurface(ds->surface);
+}
+
+static inline int is_surface_bgr(DisplaySurface *surface)
+{
+    if (surface->pf.bits_per_pixel == 32 && surface->pf.rshift == 0)
+        return 1;
+    else
+        return 0;
+}
+
 static inline int is_buffer_shared(DisplaySurface *surface)
 {
     return (!(surface->flags & QEMU_ALLOCATED_FLAG));
diff --git a/curses.c b/curses.c
index d699b5e..541323a 100644
--- a/curses.c
+++ b/curses.c
@@ -366,7 +366,7 @@ void curses_display_init(DisplayState *ds, int full_screen)
     dcl->dpy_refresh = curses_refresh;
     dcl->dpy_text_cursor = curses_cursor_position;
     register_displaychangelistener(ds, dcl);
-    qemu_free_displaysurface(ds->surface);
+    qemu_free_displaysurface(ds);
     ds->surface = qemu_create_displaysurface_from(640, 400, 0, 0, (uint8_t*) 
screen);
 
     invalidate = 1;
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 75c8887..1612a62 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -822,7 +822,7 @@ static void lcd_refresh(void *opaque)
         break;
     LCD_REFRESH(8, rgb_to_pixel8)
     LCD_REFRESH(16, rgb_to_pixel16)
-    LCD_REFRESH(32, rgb_to_pixel32)
+    LCD_REFRESH(32, (is_surface_bgr(s->ds) ? rgb_to_pixel32bgr : 
rgb_to_pixel32))
     default:
         cpu_abort(cpu_single_env, "unsupported colour depth %i\n",
                   ds_get_bits_per_pixel(s->ds));
diff --git a/hw/nseries.c b/hw/nseries.c
index 32aaead..0c7da77 100644
--- a/hw/nseries.c
+++ b/hw/nseries.c
@@ -1362,7 +1362,7 @@ static void n8x0_init(ram_addr_t ram_size, const char 
*boot_device,
        will set the size once configured, so this just sets an initial
        size until the guest activates the display.  */
     ds = get_displaystate();
-    ds->surface = qemu_resize_displaysurface(ds->surface, 800, 480, 32, 4 * 
800);
+    ds->surface = qemu_resize_displaysurface(ds, 800, 480);
     dpy_resize(ds);
 }
 
diff --git a/hw/palm.c b/hw/palm.c
index 93d1236..10fcd02 100644
--- a/hw/palm.c
+++ b/hw/palm.c
@@ -278,7 +278,7 @@ static void palmte_init(ram_addr_t ram_size, int 
vga_ram_size,
     /* 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.  */
-    ds->surface = qemu_resize_displaysurface(ds->surface, 320, 320, 32, 4 * 
320);
+    ds->surface = qemu_resize_displaysurface(ds, 320, 320);
     dpy_resize(ds);
 }
 
diff --git a/hw/sm501.c b/hw/sm501.c
index ca9528b..f94fa0e 100644
--- a/hw/sm501.c
+++ b/hw/sm501.c
@@ -948,7 +948,10 @@ static inline int get_depth_index(DisplayState *s)
     case 16:
         return 2;
     case 32:
-        return 3;
+       if (is_surface_bgr(s->surface))
+           return 4;
+       else
+           return 3;
     }
 }
 
diff --git a/hw/tcx.c b/hw/tcx.c
index 20c0dbb..4854815 100644
--- a/hw/tcx.c
+++ b/hw/tcx.c
@@ -66,7 +66,10 @@ static void update_palette_entries(TCXState *s, int start, 
int end)
             s->palette[i] = rgb_to_pixel16(s->r[i], s->g[i], s->b[i]);
             break;
         case 32:
-            s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
+            if (is_surface_bgr(s->ds->surface))
+                s->palette[i] = rgb_to_pixel32bgr(s->r[i], s->g[i], s->b[i]);
+            else
+                s->palette[i] = rgb_to_pixel32(s->r[i], s->g[i], s->b[i]);
             break;
         }
     }
@@ -124,11 +127,12 @@ static inline void tcx24_draw_line32(TCXState *s1, 
uint8_t *d,
                                      const uint32_t *cplane,
                                      const uint32_t *s24)
 {
-    int x, r, g, b;
+    int x, bgr, r, g, b;
     uint8_t val, *p8;
     uint32_t *p = (uint32_t *)d;
     uint32_t dval;
 
+    bgr = is_surface_bgr(s1->ds->surface);
     for(x = 0; x < width; x++, s++, s24++) {
         if ((be32_to_cpu(*cplane++) & 0xff000000) == 0x03000000) {
             // 24-bit direct, BGR order
@@ -137,7 +141,10 @@ static inline void tcx24_draw_line32(TCXState *s1, uint8_t 
*d,
             b = *p8++;
             g = *p8++;
             r = *p8++;
-            dval = rgb_to_pixel32(r, g, b);
+            if (bgr)
+                dval = rgb_to_pixel32bgr(r, g, b);
+            else
+                dval = rgb_to_pixel32(r, g, b);
         } else {
             val = *s;
             dval = s1->palette[val];
diff --git a/hw/vga.c b/hw/vga.c
index 98c2234..9be0480 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1161,7 +1161,10 @@ static inline int get_depth_index(DisplayState *s)
     case 16:
         return 2;
     case 32:
-        return 3;
+        if (is_surface_bgr(s->surface))
+            return 4;
+        else
+            return 3;
     }
 }
 
@@ -1627,7 +1630,7 @@ static void vga_draw_graphic(VGAState *s, int full_update)
         if (depth == 32) {
 #endif
             if (is_graphic_console()) {
-                qemu_free_displaysurface(s->ds->surface);
+                qemu_free_displaysurface(s->ds);
                 s->ds->surface = qemu_create_displaysurface_from(disp_width, 
height, depth,
                                                                s->line_offset,
                                                                s->vram_ptr + 
(s->start_addr * 4));
@@ -2619,7 +2622,7 @@ static void vga_screen_dump_common(VGAState *s, const 
char *filename,
     dcl.dpy_resize = vga_save_dpy_resize;
     dcl.dpy_refresh = vga_save_dpy_refresh;
     register_displaychangelistener(ds, &dcl);
-    ds->surface = qemu_create_displaysurface(w, h, 32, 4 * w);
+    ds->surface = qemu_create_displaysurface(ds, w, h);
 
     s->ds = ds;
     s->graphic_mode = -1;
@@ -2627,7 +2630,7 @@ static void vga_screen_dump_common(VGAState *s, const 
char *filename,
 
     ppm_save(filename, ds->surface);
 
-    qemu_free_displaysurface(ds->surface);
+    qemu_free_displaysurface(ds);
     s->ds = saved_ds;
 }
 
diff --git a/qemu-common.h b/qemu-common.h
index db33493..fa969cb 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -168,6 +168,7 @@ typedef struct BlockDriverState BlockDriverState;
 typedef struct DisplayState DisplayState;
 typedef struct DisplayChangeListener DisplayChangeListener;
 typedef struct DisplaySurface DisplaySurface;
+typedef struct DisplayAllocator DisplayAllocator;
 typedef struct PixelFormat PixelFormat;
 typedef struct TextConsole TextConsole;
 typedef TextConsole QEMUConsole;
diff --git a/sdl.c b/sdl.c
index c685b81..7b3bb61 100644
--- a/sdl.c
+++ b/sdl.c
@@ -53,17 +53,20 @@ static int absolute_enabled = 0;
 static int guest_cursor = 0;
 static int guest_x, guest_y;
 static SDL_Cursor *guest_sprite = 0;
+static uint8_t allocator;
+static uint8_t hostbpp;
 
 static void sdl_update(DisplayState *ds, int x, int y, int w, int h)
 {
-    SDL_Rect rec;
-    rec.x = x;
-    rec.y = y;
-    rec.w = w;
-    rec.h = h;
     //    printf("updating x=%d y=%d w=%d h=%d\n", x, y, w, h);
-
-    SDL_BlitSurface(guest_screen, &rec, real_screen, &rec);
+    if (guest_screen) {
+        SDL_Rect rec;
+        rec.x = x;
+        rec.y = y;
+        rec.w = w;
+        rec.h = h;
+        SDL_BlitSurface(guest_screen, &rec, real_screen, &rec);
+    }
     SDL_UpdateRect(real_screen, x, y, w, h);
 }
 
@@ -83,7 +86,7 @@ static void sdl_setdata(DisplayState *ds)
                                             ds->surface->pf.bmask, 
ds->surface->pf.amask);
 }
 
-static void sdl_resize(DisplayState *ds)
+static void do_sdl_resize(int width, int height, int bpp)
 {
     int flags;
 
@@ -95,15 +98,101 @@ static void sdl_resize(DisplayState *ds)
     if (gui_noframe)
         flags |= SDL_NOFRAME;
 
-    width = ds_get_width(ds);
-    height = ds_get_height(ds);
-    real_screen = SDL_SetVideoMode(width, height, 0, flags);
+    real_screen = SDL_SetVideoMode(width, height, bpp, flags);
     if (!real_screen) {
         fprintf(stderr, "Could not open SDL display\n");
         exit(1);
     }
+}
+
+static void sdl_resize(DisplayState *ds)
+{
+    if  (!allocator) {
+        do_sdl_resize(ds_get_width(ds), ds_get_height(ds), 0);
+        sdl_setdata(ds);
+    } else {
+        if (guest_screen != NULL) {
+            SDL_FreeSurface(guest_screen);
+            guest_screen = NULL;
+        }
+    }
+}
+
+static PixelFormat sdl_to_qemu_pixelformat(SDL_PixelFormat *sdl_pf)
+{
+    PixelFormat qemu_pf;
+
+    memset(&qemu_pf, 0x00, sizeof(PixelFormat));
+
+    qemu_pf.bits_per_pixel = sdl_pf->BitsPerPixel;
+    qemu_pf.bytes_per_pixel = sdl_pf->BytesPerPixel;
+    qemu_pf.depth = (qemu_pf.bits_per_pixel) == 32 ? 24 : 
(qemu_pf.bits_per_pixel);
+
+    qemu_pf.rmask = sdl_pf->Rmask;
+    qemu_pf.gmask = sdl_pf->Gmask;
+    qemu_pf.bmask = sdl_pf->Bmask;
+    qemu_pf.amask = sdl_pf->Amask;
+
+    qemu_pf.rshift = sdl_pf->Rshift;
+    qemu_pf.gshift = sdl_pf->Gshift;
+    qemu_pf.bshift = sdl_pf->Bshift;
+    qemu_pf.ashift = sdl_pf->Ashift;
+
+    qemu_pf.rbits = 8 - sdl_pf->Rloss;
+    qemu_pf.gbits = 8 - sdl_pf->Gloss;
+    qemu_pf.bbits = 8 - sdl_pf->Bloss;
+    qemu_pf.abits = 8 - sdl_pf->Aloss;
+
+    qemu_pf.rmax = ((1 << qemu_pf.rbits) - 1);
+    qemu_pf.gmax = ((1 << qemu_pf.gbits) - 1);
+    qemu_pf.bmax = ((1 << qemu_pf.bbits) - 1);
+    qemu_pf.amax = ((1 << qemu_pf.abits) - 1);
+
+    return qemu_pf;
+}
+
+static DisplaySurface* sdl_create_displaysurface(int width, int height)
+{
+    DisplaySurface *surface = (DisplaySurface*) 
qemu_mallocz(sizeof(DisplaySurface));
+    if (surface == NULL) {
+        fprintf(stderr, "sdl_create_displaysurface: malloc failed\n");
+        exit(1);
+    }
+
+    surface->width = width;
+    surface->height = height;
 
-    sdl_setdata(ds);
+    if (hostbpp == 16)
+        do_sdl_resize(width, height, 16);
+    else
+        do_sdl_resize(width, height, 32);
+
+    surface->pf = sdl_to_qemu_pixelformat(real_screen->format);
+    surface->linesize = real_screen->pitch;
+    surface->data = real_screen->pixels;
+
+#ifdef WORDS_BIGENDIAN
+    surface->flags = QEMU_ALLOCATED_FLAG | QEMU_BIG_ENDIAN_FLAG;
+#else
+    surface->flags = QEMU_ALLOCATED_FLAG;
+#endif
+    allocator = 1;
+
+    return surface;
+}
+
+static void sdl_free_displaysurface(DisplaySurface *surface)
+{
+    allocator = 0;
+    if (surface == NULL)
+        return;
+    qemu_free(surface);
+}
+
+static DisplaySurface* sdl_resize_displaysurface(DisplaySurface *surface, int 
width, int height)
+{
+    sdl_free_displaysurface(surface);
+    return sdl_create_displaysurface(width, height);
 }
 
 /* generic keyboard conversion */
@@ -392,7 +481,7 @@ static void sdl_send_mouse_event(int dx, int dy, int dz, 
int x, int y, int state
 static void toggle_full_screen(DisplayState *ds)
 {
     gui_fullscreen = !gui_fullscreen;
-    sdl_resize(ds);
+    do_sdl_resize(real_screen->w, real_screen->h, 
real_screen->format->BitsPerPixel);
     if (gui_fullscreen) {
         gui_saved_grab = gui_grab;
         sdl_grab_start();
@@ -670,6 +759,8 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
 {
     int flags;
     uint8_t data = 0;
+    DisplayAllocator *da;
+    const SDL_VideoInfo *vi;
 
 #if defined(__APPLE__)
     /* always use generic keymaps */
@@ -690,6 +781,8 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
         fprintf(stderr, "Could not initialize SDL - exiting\n");
         exit(1);
     }
+    vi = SDL_GetVideoInfo();
+    hostbpp = vi->vfmt->BitsPerPixel;
 
     dcl = qemu_mallocz(sizeof(DisplayChangeListener));
     dcl->dpy_update = sdl_update;
@@ -701,6 +794,18 @@ void sdl_display_init(DisplayState *ds, int full_screen, 
int no_frame)
     ds->cursor_define = sdl_mouse_define;
     register_displaychangelistener(ds, dcl);
 
+    da = qemu_mallocz(sizeof(DisplayAllocator));
+    da->create_displaysurface = sdl_create_displaysurface;
+    da->resize_displaysurface = sdl_resize_displaysurface;
+    da->free_displaysurface = sdl_free_displaysurface;
+    if (register_displayallocator(ds, da) == da) {
+        DisplaySurface *surf;
+        surf = sdl_create_displaysurface(ds_get_width(ds), ds_get_height(ds));
+        defaultallocator_free_displaysurface(ds->surface);
+        ds->surface = surf;
+        dpy_resize(ds);
+    }
+
     sdl_update_caption();
     SDL_EnableKeyRepeat(250, 50);
     gui_grab = 0;
diff --git a/vl.c b/vl.c
index d3863c4..438fb15 100644
--- a/vl.c
+++ b/vl.c
@@ -2843,6 +2843,12 @@ void pcmcia_info(void)
 /***********************************************************/
 /* register display */
 
+struct DisplayAllocator default_allocator = {
+    defaultallocator_create_displaysurface,
+    defaultallocator_resize_displaysurface,
+    defaultallocator_free_displaysurface
+};
+
 void register_displaystate(DisplayState *ds)
 {
     DisplayState **s;
@@ -2858,12 +2864,19 @@ DisplayState *get_displaystate(void)
     return display_state;
 }
 
+DisplayAllocator *register_displayallocator(DisplayState *ds, DisplayAllocator 
*da)
+{
+    if(ds->allocator ==  &default_allocator) ds->allocator = da;
+    return ds->allocator;
+}
+
 /* dumb display */
 
 static void dumb_display_init(void)
 {
     DisplayState *ds = qemu_mallocz(sizeof(DisplayState));
-    ds->surface = qemu_create_displaysurface(640, 480, 32, 640 * 4);
+    ds->allocator = &default_allocator;
+    ds->surface = qemu_create_displaysurface(ds, 640, 480);
     register_displaystate(ds);
 }