DisplayState interface change

PATCH In review Stefano Stabellini 2 participants 8 messages Nov 21, 2008 – Nov 25, 2008 1 message not in the archive

Share & tools

Stefano Stabellini

author

2008-11-21 17:08 UTC #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;

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

---

console.c +149 ~~-27~~

diff -r d57f9f83d5eb 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);
 }

@@ -1245,6 +1238,11 @@
     s->hw_text_update = text_update;
     s->hw = opaque;
     return s;
+}
+
+int is_active_console(QEMUConsole *s)
+{
+    return (active_console == s);
 }
 
 int is_graphic_console(void)

@@ -1346,13 +1344,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 +1357,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_reallocz(surface->data, surface->linesize * surface->height);
+    else
+        surface->data = (uint8_t*) qemu_mallocz(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 +119 ~~-20~~

195 lines not on this page · raw message

curses.c +20 ~~-18~~

diff -r d57f9f83d5eb 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 d57f9f83d5eb 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/nseries.c +2 -1

diff -r d57f9f83d5eb 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 d57f9f83d5eb 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 d57f9f83d5eb 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,                        \

qemu-common.h +3 -0

diff -r d57f9f83d5eb qemu-common.h
@@ -129,6 +129,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 +33 ~~-72~~

214 lines not on this page · raw message

Stefano Stabellini

author

2008-11-21 17:41 UTC #2 lore raw

In reply to a message not in the archive

Quoted 7 lines · Paul Brook

Quoted 5 lines · Stefano Stabellini
+ Â Â Â Â surface->data = (uint8_t*) qemu_reallocz(surface->data, surface->linesize * surface->height);
+ Â Â else
+ Â Â Â Â surface->data = (uint8_t*) qemu_mallocz(surface->linesize * surface->height);

Quoted 6 lines

Do these really need to be zeroed?
reallocz seems like a bit of a funny concept. I doubt it's more efficient than free/mallocz. The point of normal realloc is that is preserves the contents of the block, even if it moves.

Actually I think they don't need to be, I have been probably overzelous. I'll do some tests to be sure, then I'll drop it.

Anthony Liguori

2008-11-22 20:00 UTC #3 lore raw

In reply to Stefano Stabellini

Quoted 37 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
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

Quoted 6 lines

- changes to vga_draw_graphic in hw/vga.c
this is the place where the DisplaySurface buffer is shared with the videoram, when possible;
Signed-off-by Stefano Stabellini <[email protected]>

I think we need another intermediate step here.

Right now, we have a pretty poor abstraction wrt QEMUConsole and DisplayState. QEMUConsole is known to much of the hardware emulation but DisplayState is too.

I think we need a step that replaces all notions of QEMUConsole in hw/*.c with DisplayState including an appropriate creation functions. There's really no point in passing a DisplayState to the hardware devices, instead they should be allocating it and returning it to be passed to the SDL/VNC front-ends.

Regards,

Anthony Liguori

Stefano Stabellini

author

2008-11-24 16:47 UTC #4 lore raw

Quoted 8 lines · Anthony Liguori

I think we need another intermediate step here.
Right now, we have a pretty poor abstraction wrt QEMUConsole and DisplayState. QEMUConsole is known to much of the hardware emulation but DisplayState is too.

Quoted 6 lines

I think we need a step that replaces all notions of QEMUConsole in hw/*.c with DisplayState including an appropriate creation functions. There's really no point in passing a DisplayState to the hardware devices, instead they should be allocating it and returning it to be passed to the SDL/VNC front-ends.

If am not mistaken this is exactly what Paul proposed.
As I said before, this model is certainly more elegant, but presents some hidden issues:

what do we do with the text consoles?
Right now a single DisplayState is shared between a graphic console and multiple text console.
Do we really want to allocate a buffer each?
Who is going to do the console multiplexing?

Even though the QEMUConsole abstraction is not very elegant and should probably be improved I believe it is a different and independent issue from the one addressed in this series.

Anthony Liguori

2008-11-24 19:59 UTC #5 lore raw

Quoted 8 lines · Stefano Stabellini

Anthony Liguori wrote:
If am not mistaken this is exactly what Paul proposed.
As I said before, this model is certainly more elegant, but presents some hidden issues:

Quoted 6 lines

what do we do with the text consoles?
Right now a single DisplayState is shared between a graphic console and multiple text console.
Do we really want to allocate a buffer each?
Who is going to do the console multiplexing?

We really should have a DisplayState that multiplexes multiple DisplayStates. TextConsole should just provide a DisplayState that can be multiplexed.

Quoted 4 lines

Even though the QEMUConsole abstraction is not very elegant and should probably be improved I believe it is a different and independent issue from the one addressed in this series.

I'm not asking you to do that refactoring. But since we all pretty much agree that this is how things should look, I want to make sure that the DisplayState API takes all of this into account.

So in that vein, I'd like to see all the things that call graphic_console_init changed to not take a DisplayState, but rather to have graphic_console_init() return an allocated DisplayState. These functions should then return a DisplayState which can be passed to VNC/SDL.

From what I can tell, this is not a huge change. You shouldn't have to really touch any of the console code. I think this is important to sure up what the DisplayState API looks like and how the rest of QEMU interacts with it.

If this seems like a major refactoring, let me know and either you're misunderstanding what I'm suggesting or I've miscalculated how much change is required here :-)

Regards,

Anthony Liguori

Stefano Stabellini

author

2008-11-25 12:19 UTC #6 lore raw

Quoted 9 lines · Anthony Liguori

I'm not asking you to do that refactoring. But since we all pretty much agree that this is how things should look, I want to make sure that the DisplayState API takes all of this into account.
So in that vein, I'd like to see all the things that call graphic_console_init changed to not take a DisplayState, but rather to have graphic_console_init() return an allocated DisplayState. These functions should then return a DisplayState which can be passed to VNC/SDL.

Quoted 8 lines

From what I can tell, this is not a huge change. You shouldn't have to really touch any of the console code. I think this is important to sure up what the DisplayState API looks like and how the rest of QEMU interacts with it.
If this seems like a major refactoring, let me know and either you're misunderstanding what I'm suggesting or I've miscalculated how much change is required here :-)

That's fine, it is a simple change, it was me that didn't understand correctly what you were suggesting.
Since at the moment we don't have a DisplayState multiplexing function, this means that we are going to keep the text_console_init as it is now so that we can still the serial and the monitor interfaces, right?

Stefano Stabellini

author

2008-11-25 12:34 UTC #7 lore raw

Quoted 6 lines · Stefano Stabellini

That's fine, it is a simple change, it was me that didn't understand correctly what you were suggesting.
Since at the moment we don't have a DisplayState multiplexing function, this means that we are going to keep the text_console_init as it is now so that we can still the serial and the monitor interfaces, right?

the missing word is "have"

Anthony Liguori

2008-11-25 14:35 UTC #8 lore raw

In reply to Stefano Stabellini

Quoted 8 lines · Stefano Stabellini

Quoted 4 lines · Anthony Liguori
If this seems like a major refactoring, let me know and either you're misunderstanding what I'm suggesting or I've miscalculated how much change is required here :-)

Quoted 6 lines

That's fine, it is a simple change, it was me that didn't understand correctly what you were suggesting.
Since at the moment we don't have a DisplayState multiplexing function, this means that we are going to keep the text_console_init as it is now so that we can still the serial and the monitor interfaces, right?

Yup.

Regards,

Anthony Liguori

Quoted 2 lines