[Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---
 console.c    |   22 ++++++++++----
 console.h    |   10 ++++++
 hw/vga.c     |   88 +++++++++++++++++++++++++++++++++++++++------------------
 hw/vga_int.h |    2 +
 4 files changed, 87 insertions(+), 35 deletions(-)

diff --git a/console.c b/console.c
index 1c94980..f6e6a10 100644
--- a/console.c
+++ b/console.c
@@ -1334,12 +1334,20 @@ CharDriverState *text_console_init(DisplayState *ds, const char *p)
 
 void qemu_console_resize(QEMUConsole *console, int width, int height)
 {
-    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;
+    if (active_console == console) {
+        dpy_resize(console->ds, width, height);
+    }
+}
+
+void qemu_console_resize_shared(QEMUConsole *console, int width, int height,
+                                int depth, int linesize, void *pixels)
+{
+    console->g_width = width;
+    console->g_height = height;
+    if (active_console == console) {
+        dpy_resize_shared(console->ds, width, height, depth, linesize, pixels);
     }
 }
+
diff --git a/console.h b/console.h
index fcac1eb..a054d97 100644
--- a/console.h
+++ b/console.h
@@ -84,9 +84,12 @@ struct DisplayState {
     struct QEMUTimer *gui_timer;
     uint64_t gui_timer_interval;
     int idle; /* there is nothing to update (window invisible), set by vnc/sdl */
+    int shared_buf;
 
     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_shared)(struct DisplayState *s, int w, int h, int depth, int linesize, void *pixels);
+    void (*dpy_setdata)(DisplayState *s, void *pixels);
     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);
@@ -108,6 +111,11 @@ static inline void dpy_resize(DisplayState *s, int w, int h)
     s->dpy_resize(s, w, h);
 }
 
+static inline void dpy_resize_shared(DisplayState *s, int w, int h, int depth, int linesize, void *pixels)
+{
+    s->dpy_resize_shared(s, w, h, depth, linesize, pixels);
+}
+
 static inline void dpy_cursor(DisplayState *s, int x, int y)
 {
     if (s->dpy_text_cursor)
@@ -140,6 +148,8 @@ CharDriverState *text_console_init(DisplayState *ds, const char *p);
 void console_select(unsigned int index);
 void console_color_init(DisplayState *ds);
 void qemu_console_resize(QEMUConsole *console, int width, int height);
+void qemu_console_resize_shared(QEMUConsole *console, int width, int height,
+                                int depth, int linesize, void *pixels);
 
 /* sdl.c */
 void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
diff --git a/hw/vga.c b/hw/vga.c
index eb0bae8..6e40d3d 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1078,6 +1078,10 @@ static const uint8_t cursor_glyph[32 * 4] = {
     0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
 };
 
+typedef unsigned int rgb_to_pixel_dup_func(unsigned int r, unsigned int g, unsigned b);
+
+static rgb_to_pixel_dup_func *rgb_to_pixel_dup_table[NB_DEPTHS];
+
 /*
  * Text mode update
  * Missing:
@@ -1099,9 +1103,6 @@ static void vga_draw_text(VGAState *s, int full_update)
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
-    full_update |= update_palette16(s);
-    palette = s->last_palette;
-
     /* compute font data address (in plane 2) */
     v = s->sr[3];
     offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2;
@@ -1135,7 +1136,6 @@ static void vga_draw_text(VGAState *s, int full_update)
         cw = 9;
     if (s->sr[1] & 0x08)
         cw = 16; /* NOTE: no 18 pixel wide */
-    x_incr = cw * ((s->ds->depth + 7) >> 3);
     width = (s->cr[0x01] + 1);
     if (s->cr[0x06] == 100) {
         /* ugly hack for CGA 160x100x16 - explain me the logic */
@@ -1152,16 +1152,23 @@ static void vga_draw_text(VGAState *s, int full_update)
     }
 
     if (width != s->last_width || height != s->last_height ||
-        cw != s->last_cw || cheight != s->last_ch) {
+        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->console, 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);
+    palette = s->last_palette;
+    x_incr = cw * ((s->ds->depth + 7) >> 3);
+
     cursor_offset = ((s->cr[0x0e] << 8) | s->cr[0x0f]) - s->start_addr;
     if (cursor_offset != s->cursor_offset ||
         s->cr[0xa] != s->cursor_start ||
@@ -1353,8 +1360,6 @@ static vga_draw_line_func *vga_draw_line_table[NB_DEPTHS * VGA_DRAW_LINE_NB] = {
     vga_draw_line32_16bgr,
 };
 
-typedef unsigned int rgb_to_pixel_dup_func(unsigned int r, unsigned int g, unsigned b);
-
 static rgb_to_pixel_dup_func *rgb_to_pixel_dup_table[NB_DEPTHS] = {
     rgb_to_pixel8_dup,
     rgb_to_pixel15_dup,
@@ -1419,6 +1424,7 @@ static void vga_draw_graphic(VGAState *s, int full_update)
 {
     int y1, y, update, page_min, page_max, linesize, y_start, double_scan, mask;
     int width, height, shift_control, line_offset, page0, page1, bwidth, bits;
+    int ds_depth, depth, palette_update;
     int disp_width, multi_scan, multi_run;
     uint8_t *d;
     uint32_t v, addr1, addr;
@@ -1446,8 +1452,39 @@ static void vga_draw_graphic(VGAState *s, int full_update)
         s->double_scan = double_scan;
     }
 
+    ds_depth = s->ds->depth;
+    depth = s->get_bpp(s);
+    if (s->ds->dpy_resize_shared) {
+        if (s->line_offset != s->last_line_offset || 
+            disp_width != s->last_width ||
+            height != s->last_height ||
+            s->last_depth != depth) {
+            qemu_console_resize_shared(s->console, disp_width, height,
+                                       depth, s->line_offset, s->vram_ptr + (s->start_addr * 4));
+            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 (disp_width != s->last_width ||
+               height != s->last_height) {
+        qemu_console_resize(s->console, disp_width, height);
+        s->last_scr_width = disp_width;
+        s->last_scr_height = height;
+        s->last_width = disp_width;
+        s->last_height = height;
+        full_update = 1;
+    }
+
+    s->rgb_to_pixel =
+        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
+
+    palette_update = 0;
     if (shift_control == 0) {
-        full_update |= update_palette16(s);
+        palette_update |= update_palette16(s);
         if (s->sr[0x01] & 8) {
             v = VGA_DRAW_LINE4D2;
             disp_width <<= 1;
@@ -1456,7 +1493,7 @@ static void vga_draw_graphic(VGAState *s, int full_update)
         }
         bits = 4;
     } else if (shift_control == 1) {
-        full_update |= update_palette16(s);
+        palette_update |= update_palette16(s);
         if (s->sr[0x01] & 8) {
             v = VGA_DRAW_LINE2D2;
             disp_width <<= 1;
@@ -1468,12 +1505,12 @@ static void vga_draw_graphic(VGAState *s, int full_update)
         switch(s->get_bpp(s)) {
         default:
         case 0:
-            full_update |= update_palette256(s);
+            palette_update |= update_palette256(s);
             v = VGA_DRAW_LINE8D2;
             bits = 4;
             break;
         case 8:
-            full_update |= update_palette256(s);
+            palette_update |= update_palette256(s);
             v = VGA_DRAW_LINE8;
             bits = 8;
             break;
@@ -1495,18 +1532,12 @@ static void vga_draw_graphic(VGAState *s, int full_update)
             break;
         }
     }
+    if (s->ds->shared_buf &&
+        (palette_update || s->ds->data != s->vram_ptr + (s->start_addr * 4)))
+        s->ds->dpy_setdata(s->ds, s->vram_ptr + (s->start_addr * 4));
+    full_update |= palette_update;
     vga_draw_line = vga_draw_line_table[v * NB_DEPTHS + get_depth_index(s->ds)];
-
-    if (disp_width != s->last_width ||
-        height != s->last_height) {
-        qemu_console_resize(s->console, disp_width, height);
-        s->last_scr_width = disp_width;
-        s->last_scr_height = height;
-        s->last_width = disp_width;
-        s->last_height = height;
-        full_update = 1;
-    }
-    if (s->cursor_invalidate)
+    if (!s->ds->shared_buf && s->cursor_invalidate)
         s->cursor_invalidate(s);
 
     line_offset = s->line_offset;
@@ -1552,9 +1583,11 @@ static void vga_draw_graphic(VGAState *s, int full_update)
                 page_min = page0;
             if (page1 > page_max)
                 page_max = page1;
-            vga_draw_line(s, d, s->vram_ptr + addr, width);
-            if (s->cursor_draw_line)
-                s->cursor_draw_line(s, d, y);
+            if (!s->ds->shared_buf) {
+                vga_draw_line(s, d, s->vram_ptr + addr, width);
+                if (s->cursor_draw_line)
+                    s->cursor_draw_line(s, d, y);
+            }
         } else {
             if (y_start >= 0) {
                 /* flush to display */
@@ -1599,6 +1632,8 @@ static void vga_draw_blank(VGAState *s, int full_update)
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
+    s->rgb_to_pixel =
+        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
     if (s->ds->depth == 8)
         val = s->rgb_to_pixel(0, 0, 0);
     else
@@ -1625,9 +1660,6 @@ static void vga_update_display(void *opaque)
     if (s->ds->depth == 0) {
         /* nothing to do */
     } else {
-        s->rgb_to_pixel =
-            rgb_to_pixel_dup_table[get_depth_index(s->ds)];
-
         full_update = 0;
         if (!(s->ar_index & 0x20)) {
             graphic_mode = GMODE_BLANK;
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 343da34..d187f1b 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -130,9 +130,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,                        \
-- 
1.5.4.3

Re: [Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Anthony Liguori]

Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>   

I need to carve out some time to test this series and think about 
whether it can be done without adding a new DisplayState hook.  I should 
be able to respond or apply within a few days.

I'm concerned about the complexity this adds to back-ends.  It's not 
clear to me whether the performance justifies the added complexity.

Regards,

Anthony Liguori

> ---
>  console.c    |   22 ++++++++++----
>  console.h    |   10 ++++++
>  hw/vga.c     |   88 +++++++++++++++++++++++++++++++++++++++------------------
>  hw/vga_int.h |    2 +
>  4 files changed, 87 insertions(+), 35 deletions(-)
>
> diff --git a/console.c b/console.c
> index 1c94980..f6e6a10 100644
> --- a/console.c
> +++ b/console.c
> @@ -1334,12 +1334,20 @@ CharDriverState *text_console_init(DisplayState *ds, const char *p)
>  
>  void qemu_console_resize(QEMUConsole *console, int width, int height)
>  {
> -    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;
> +    if (active_console == console) {
> +        dpy_resize(console->ds, width, height);
> +    }
> +}
> +
> +void qemu_console_resize_shared(QEMUConsole *console, int width, int height,
> +                                int depth, int linesize, void *pixels)
> +{
> +    console->g_width = width;
> +    console->g_height = height;
> +    if (active_console == console) {
> +        dpy_resize_shared(console->ds, width, height, depth, linesize, pixels);
>      }
>  }
> +
> diff --git a/console.h b/console.h
> index fcac1eb..a054d97 100644
> --- a/console.h
> +++ b/console.h
> @@ -84,9 +84,12 @@ struct DisplayState {
>      struct QEMUTimer *gui_timer;
>      uint64_t gui_timer_interval;
>      int idle; /* there is nothing to update (window invisible), set by vnc/sdl */
> +    int shared_buf;
>  
>      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_shared)(struct DisplayState *s, int w, int h, int depth, int linesize, void *pixels);
> +    void (*dpy_setdata)(DisplayState *s, void *pixels);
>      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);
> @@ -108,6 +111,11 @@ static inline void dpy_resize(DisplayState *s, int w, int h)
>      s->dpy_resize(s, w, h);
>  }
>  
> +static inline void dpy_resize_shared(DisplayState *s, int w, int h, int depth, int linesize, void *pixels)
> +{
> +    s->dpy_resize_shared(s, w, h, depth, linesize, pixels);
> +}
> +
>  static inline void dpy_cursor(DisplayState *s, int x, int y)
>  {
>      if (s->dpy_text_cursor)
> @@ -140,6 +148,8 @@ CharDriverState *text_console_init(DisplayState *ds, const char *p);
>  void console_select(unsigned int index);
>  void console_color_init(DisplayState *ds);
>  void qemu_console_resize(QEMUConsole *console, int width, int height);
> +void qemu_console_resize_shared(QEMUConsole *console, int width, int height,
> +                                int depth, int linesize, void *pixels);
>  
>  /* sdl.c */
>  void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
> diff --git a/hw/vga.c b/hw/vga.c
> index eb0bae8..6e40d3d 100644
> --- a/hw/vga.c
> +++ b/hw/vga.c
> @@ -1078,6 +1078,10 @@ static const uint8_t cursor_glyph[32 * 4] = {
>      0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>  };
>  
> +typedef unsigned int rgb_to_pixel_dup_func(unsigned int r, unsigned int g, unsigned b);
> +
> +static rgb_to_pixel_dup_func *rgb_to_pixel_dup_table[NB_DEPTHS];
> +
>  /*
>   * Text mode update
>   * Missing:
> @@ -1099,9 +1103,6 @@ static void vga_draw_text(VGAState *s, int full_update)
>      vga_draw_glyph8_func *vga_draw_glyph8;
>      vga_draw_glyph9_func *vga_draw_glyph9;
>  
> -    full_update |= update_palette16(s);
> -    palette = s->last_palette;
> -
>      /* compute font data address (in plane 2) */
>      v = s->sr[3];
>      offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2;
> @@ -1135,7 +1136,6 @@ static void vga_draw_text(VGAState *s, int full_update)
>          cw = 9;
>      if (s->sr[1] & 0x08)
>          cw = 16; /* NOTE: no 18 pixel wide */
> -    x_incr = cw * ((s->ds->depth + 7) >> 3);
>      width = (s->cr[0x01] + 1);
>      if (s->cr[0x06] == 100) {
>          /* ugly hack for CGA 160x100x16 - explain me the logic */
> @@ -1152,16 +1152,23 @@ static void vga_draw_text(VGAState *s, int full_update)
>      }
>  
>      if (width != s->last_width || height != s->last_height ||
> -        cw != s->last_cw || cheight != s->last_ch) {
> +        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->console, 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);
> +    palette = s->last_palette;
> +    x_incr = cw * ((s->ds->depth + 7) >> 3);
> +
>      cursor_offset = ((s->cr[0x0e] << 8) | s->cr[0x0f]) - s->start_addr;
>      if (cursor_offset != s->cursor_offset ||
>          s->cr[0xa] != s->cursor_start ||
> @@ -1353,8 +1360,6 @@ static vga_draw_line_func *vga_draw_line_table[NB_DEPTHS * VGA_DRAW_LINE_NB] = {
>      vga_draw_line32_16bgr,
>  };
>  
> -typedef unsigned int rgb_to_pixel_dup_func(unsigned int r, unsigned int g, unsigned b);
> -
>  static rgb_to_pixel_dup_func *rgb_to_pixel_dup_table[NB_DEPTHS] = {
>      rgb_to_pixel8_dup,
>      rgb_to_pixel15_dup,
> @@ -1419,6 +1424,7 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>  {
>      int y1, y, update, page_min, page_max, linesize, y_start, double_scan, mask;
>      int width, height, shift_control, line_offset, page0, page1, bwidth, bits;
> +    int ds_depth, depth, palette_update;
>      int disp_width, multi_scan, multi_run;
>      uint8_t *d;
>      uint32_t v, addr1, addr;
> @@ -1446,8 +1452,39 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>          s->double_scan = double_scan;
>      }
>  
> +    ds_depth = s->ds->depth;
> +    depth = s->get_bpp(s);
> +    if (s->ds->dpy_resize_shared) {
> +        if (s->line_offset != s->last_line_offset || 
> +            disp_width != s->last_width ||
> +            height != s->last_height ||
> +            s->last_depth != depth) {
> +            qemu_console_resize_shared(s->console, disp_width, height,
> +                                       depth, s->line_offset, s->vram_ptr + (s->start_addr * 4));
> +            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 (disp_width != s->last_width ||
> +               height != s->last_height) {
> +        qemu_console_resize(s->console, disp_width, height);
> +        s->last_scr_width = disp_width;
> +        s->last_scr_height = height;
> +        s->last_width = disp_width;
> +        s->last_height = height;
> +        full_update = 1;
> +    }
> +
> +    s->rgb_to_pixel =
> +        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
> +
> +    palette_update = 0;
>      if (shift_control == 0) {
> -        full_update |= update_palette16(s);
> +        palette_update |= update_palette16(s);
>          if (s->sr[0x01] & 8) {
>              v = VGA_DRAW_LINE4D2;
>              disp_width <<= 1;
> @@ -1456,7 +1493,7 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>          }
>          bits = 4;
>      } else if (shift_control == 1) {
> -        full_update |= update_palette16(s);
> +        palette_update |= update_palette16(s);
>          if (s->sr[0x01] & 8) {
>              v = VGA_DRAW_LINE2D2;
>              disp_width <<= 1;
> @@ -1468,12 +1505,12 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>          switch(s->get_bpp(s)) {
>          default:
>          case 0:
> -            full_update |= update_palette256(s);
> +            palette_update |= update_palette256(s);
>              v = VGA_DRAW_LINE8D2;
>              bits = 4;
>              break;
>          case 8:
> -            full_update |= update_palette256(s);
> +            palette_update |= update_palette256(s);
>              v = VGA_DRAW_LINE8;
>              bits = 8;
>              break;
> @@ -1495,18 +1532,12 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>              break;
>          }
>      }
> +    if (s->ds->shared_buf &&
> +        (palette_update || s->ds->data != s->vram_ptr + (s->start_addr * 4)))
> +        s->ds->dpy_setdata(s->ds, s->vram_ptr + (s->start_addr * 4));
> +    full_update |= palette_update;
>      vga_draw_line = vga_draw_line_table[v * NB_DEPTHS + get_depth_index(s->ds)];
> -
> -    if (disp_width != s->last_width ||
> -        height != s->last_height) {
> -        qemu_console_resize(s->console, disp_width, height);
> -        s->last_scr_width = disp_width;
> -        s->last_scr_height = height;
> -        s->last_width = disp_width;
> -        s->last_height = height;
> -        full_update = 1;
> -    }
> -    if (s->cursor_invalidate)
> +    if (!s->ds->shared_buf && s->cursor_invalidate)
>          s->cursor_invalidate(s);
>  
>      line_offset = s->line_offset;
> @@ -1552,9 +1583,11 @@ static void vga_draw_graphic(VGAState *s, int full_update)
>                  page_min = page0;
>              if (page1 > page_max)
>                  page_max = page1;
> -            vga_draw_line(s, d, s->vram_ptr + addr, width);
> -            if (s->cursor_draw_line)
> -                s->cursor_draw_line(s, d, y);
> +            if (!s->ds->shared_buf) {
> +                vga_draw_line(s, d, s->vram_ptr + addr, width);
> +                if (s->cursor_draw_line)
> +                    s->cursor_draw_line(s, d, y);
> +            }
>          } else {
>              if (y_start >= 0) {
>                  /* flush to display */
> @@ -1599,6 +1632,8 @@ static void vga_draw_blank(VGAState *s, int full_update)
>          return;
>      if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
>          return;
> +    s->rgb_to_pixel =
> +        rgb_to_pixel_dup_table[get_depth_index(s->ds)];
>      if (s->ds->depth == 8)
>          val = s->rgb_to_pixel(0, 0, 0);
>      else
> @@ -1625,9 +1660,6 @@ static void vga_update_display(void *opaque)
>      if (s->ds->depth == 0) {
>          /* nothing to do */
>      } else {
> -        s->rgb_to_pixel =
> -            rgb_to_pixel_dup_table[get_depth_index(s->ds)];
> -
>          full_update = 0;
>          if (!(s->ar_index & 0x20)) {
>              graphic_mode = GMODE_BLANK;
> diff --git a/hw/vga_int.h b/hw/vga_int.h
> index 343da34..d187f1b 100644
> --- a/hw/vga_int.h
> +++ b/hw/vga_int.h
> @@ -130,9 +130,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,                        \
>   

Re: [Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>   
> 
> I need to carve out some time to test this series and think about
> whether it can be done without adding a new DisplayState hook.  I should
> be able to respond or apply within a few days.
> 
> I'm concerned about the complexity this adds to back-ends.  It's not
> clear to me whether the performance justifies the added complexity.
> 

The shared buffer support is completely optional, so not all the
back-ends have to support it.

Regarding the performances, keep also in mind the memory savings :)

Re: [Qemu-devel] [PATCH 3 of 6] vga shared buffer

[andrzej zaborowski]

2008/9/15 Anthony Liguori <anthony@codemonkey.ws>:
> Stefano Stabellini wrote:
>>
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>
>
> I need to carve out some time to test this series and think about whether it
> can be done without adding a new DisplayState hook.  I should be able to
> respond or apply within a few days.

Any news? I think this change makes sense and it can help remove the
DIRECT_VRAM hack from vmware_vga.c.

>
> I'm concerned about the complexity this adds to back-ends.  It's not clear
> to me whether the performance justifies the added complexity.

One way to simplify this and everything else is use Stefano's code to
stop the backends (SDL, VNC) from managing the framebuffer altogether
and only support what these patches call shared buffer.  Then if the
given backed doesn't support given colorspace conversion in hardware,
it's its call to implement this, it could unify the color conversion
in qemu more generally.  The drawback is it's hard to support weird
colorspaces like those supported by OMAP2 display susbsystem, and even
harder to do features like overlaid framebuffers with different
colorspaces (but it's not currently implemented anyway).

Regards

Re: [Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Anthony Liguori]

andrzej zaborowski wrote:
> 2008/9/15 Anthony Liguori <anthony@codemonkey.ws>:
>   
>> Stefano Stabellini wrote:
>>     
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>>>       
>> I need to carve out some time to test this series and think about whether it
>> can be done without adding a new DisplayState hook.  I should be able to
>> respond or apply within a few days.
>>     
>
> Any news? I think this change makes sense and it can help remove the
> DIRECT_VRAM hack from vmware_vga.c.
>
>   
>> I'm concerned about the complexity this adds to back-ends.  It's not clear
>> to me whether the performance justifies the added complexity.
>>     
>
> One way to simplify this and everything else is use Stefano's code to
> stop the backends (SDL, VNC) from managing the framebuffer altogether
> and only support what these patches call shared buffer.

This is sort of what I've been thinking about.  If we're going to do 
this at all, we should move to a model where the backends stop managing 
the framebuffers completely.

>   Then if the
> given backed doesn't support given colorspace conversion in hardware,
> it's its call to implement this, it could unify the color conversion
> in qemu more generally.  The drawback is it's hard to support weird
> colorspaces like those supported by OMAP2 display susbsystem, and even
> harder to do features like overlaid framebuffers with different
> colorspaces (but it's not currently implemented anyway).
>   

It's slightly less optimal when you aren't going from a real 
framebuffer.  For instance, right now when in VGA planar mode, we're 
able to basically write pixels in the SDL buffer directly from the mmio 
callbacks.  If we switched to having the VGA devices manage their 
buffers, we would have to write to a buffer and then rely on SDL 
conversion functions to flip the buffer.  I don't know that it's really 
worth optimizing for things like VGA planar mode but I think that's 
really the trade off that we're making.

Regards,

Anthony Liguori

> Regards
>
>
>   

Re: [Qemu-devel] [PATCH 3 of 6] vga shared buffer

[Stefano Stabellini]

andrzej zaborowski wrote:

> 2008/9/15 Anthony Liguori <anthony@codemonkey.ws>:
>> Stefano Stabellini wrote:
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>
>> I need to carve out some time to test this series and think about whether it
>> can be done without adding a new DisplayState hook.  I should be able to
>> respond or apply within a few days.
> 
> Any news? I think this change makes sense and it can help remove the
> DIRECT_VRAM hack from vmware_vga.c.


Thanks for keeping this thread alive.
I was fearing it was slowing slipping into oblivion.

 
>> I'm concerned about the complexity this adds to back-ends.  It's not clear
>> to me whether the performance justifies the added complexity.
> 
> One way to simplify this and everything else is use Stefano's code to
> stop the backends (SDL, VNC) from managing the framebuffer altogether
> and only support what these patches call shared buffer.  Then if the
> given backed doesn't support given colorspace conversion in hardware,
> it's its call to implement this, it could unify the color conversion
> in qemu more generally.  The drawback is it's hard to support weird
> colorspaces like those supported by OMAP2 display susbsystem, and even
> harder to do features like overlaid framebuffers with different
> colorspaces (but it's not currently implemented anyway).
> 

I wouldn't be so concerned about complexity: backends can always drop
completely any shared buffer related code and still work fine.
The vga code instead is not changed much (sadly I had to move some code
around but the real changes are really few, about 20 lines of code, I
would say).

I am not sure to understand correctly what you are suggesting, are you
proposing to always share the buffer and leave completely any color
conversion to the backends?

If so, that would simplify the code changes in vga.c, but not by much
(keep in mind that we would still need to move the code I am moving
right now, so we would only affect the remaing ~20 lines added).
It would leave sdl mostly unaffected, because with my patches it already
does all the color conversions itself.
Finally it would add complexity to vnc.c, that at the moment is only
able to handle 16 bpp.