Re: [Qemu-devel] [RFC] qemu VGA endian swap low level drawing changes

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2014-06-17 at 06:40 +0200, Paolo Bonzini wrote:
> Il 17/06/2014 05:07, Benjamin Herrenschmidt ha scritto:
> > I've come up with the proof of concept below which changes the various
> > line drawing functions in vga-template to select the right ld/st
> > function. However that adds a per-line test (overhead I can hear some
> > people shouting ! :-).
> 
> Sounds perfectly reasonable.

Thanks. I've tried the other approach of adding new functions which
means no overhead (hopefully) for the non-swap case and less invasive
changes to vga_template.c.

Patch below. What do you think ? This or the previous approach ? Then we
can discuss how we actually trigger the endian change and where we store
the state :-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 8cd6afe..eaa6791 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -983,27 +983,89 @@ typedef void vga_draw_glyph9_func(uint8_t *d, int linesize,
 typedef void vga_draw_line_func(VGACommonState *s1, uint8_t *d,
                                 const uint8_t *s, int width);
 
+#ifdef TARGET_WORDS_BIGENDIAN
+static bool vga_is_be = true;
+#else
+static bool vga_is_be = false;
+#endif
+
+void vga_set_big_endian(bool be)
+{
+    vga_is_be = be;
+}
+
+static inline bool vga_is_big_endian(VGACommonState *s)
+{
+    return vga_is_be;
+}
+
+#define BGR_FORMAT 0
+#define PIX_BE 0
 #define DEPTH 8
 #include "vga_template.h"
 
+#define BGR_FORMAT 0
+#define PIX_BE 0
 #define DEPTH 15
 #include "vga_template.h"
 
-#define BGR_FORMAT
+#define BGR_FORMAT 1
+#define PIX_BE 0
 #define DEPTH 15
 #include "vga_template.h"
 
+#define BGR_FORMAT 0
+#define PIX_BE 0
 #define DEPTH 16
 #include "vga_template.h"
 
-#define BGR_FORMAT
+#define BGR_FORMAT 1
+#define PIX_BE 0
 #define DEPTH 16
 #include "vga_template.h"
 
+#define BGR_FORMAT 0
+#define PIX_BE 0
 #define DEPTH 32
 #include "vga_template.h"
 
-#define BGR_FORMAT
+#define BGR_FORMAT 1
+#define PIX_BE 0
+#define DEPTH 32
+#include "vga_template.h"
+
+#define BGR_FORMAT 0
+#define PIX_BE 1
+#define DEPTH 8
+#include "vga_template.h"
+
+#define BGR_FORMAT 0
+#define PIX_BE 1
+#define DEPTH 15
+#include "vga_template.h"
+
+#define BGR_FORMAT 1
+#define PIX_BE 1
+#define DEPTH 15
+#include "vga_template.h"
+
+#define BGR_FORMAT 0
+#define PIX_BE 1
+#define DEPTH 16
+#include "vga_template.h"
+
+#define BGR_FORMAT 1
+#define PIX_BE 1
+#define DEPTH 16
+#include "vga_template.h"
+
+#define BGR_FORMAT 0
+#define PIX_BE 1
+#define DEPTH 32
+#include "vga_template.h"
+
+#define BGR_FORMAT 1
+#define PIX_BE 1
 #define DEPTH 32
 #include "vga_template.h"
 
@@ -1486,10 +1548,14 @@ enum {
     VGA_DRAW_LINE4D2,
     VGA_DRAW_LINE8D2,
     VGA_DRAW_LINE8,
-    VGA_DRAW_LINE15,
-    VGA_DRAW_LINE16,
-    VGA_DRAW_LINE24,
-    VGA_DRAW_LINE32,
+    VGA_DRAW_LINE15_LE,
+    VGA_DRAW_LINE16_LE,
+    VGA_DRAW_LINE24_LE,
+    VGA_DRAW_LINE32_LE,
+    VGA_DRAW_LINE15_BE,
+    VGA_DRAW_LINE16_BE,
+    VGA_DRAW_LINE24_BE,
+    VGA_DRAW_LINE32_BE,
     VGA_DRAW_LINE_NB,
 };
 
@@ -1542,37 +1608,69 @@ static vga_draw_line_func * const vga_draw_line_table[NB_DEPTHS * VGA_DRAW_LINE_
     vga_draw_line8_16,
     vga_draw_line8_16,
 
-    vga_draw_line15_8,
-    vga_draw_line15_15,
-    vga_draw_line15_16,
-    vga_draw_line15_32,
-    vga_draw_line15_32bgr,
-    vga_draw_line15_15bgr,
-    vga_draw_line15_16bgr,
-
-    vga_draw_line16_8,
-    vga_draw_line16_15,
-    vga_draw_line16_16,
-    vga_draw_line16_32,
-    vga_draw_line16_32bgr,
-    vga_draw_line16_15bgr,
-    vga_draw_line16_16bgr,
-
-    vga_draw_line24_8,
-    vga_draw_line24_15,
-    vga_draw_line24_16,
-    vga_draw_line24_32,
-    vga_draw_line24_32bgr,
-    vga_draw_line24_15bgr,
-    vga_draw_line24_16bgr,
-
-    vga_draw_line32_8,
-    vga_draw_line32_15,
-    vga_draw_line32_16,
-    vga_draw_line32_32,
-    vga_draw_line32_32bgr,
-    vga_draw_line32_15bgr,
-    vga_draw_line32_16bgr,
+    vga_draw_line15_8_le,
+    vga_draw_line15_15_le,
+    vga_draw_line15_16_le,
+    vga_draw_line15_32_le,
+    vga_draw_line15_32bgr_le,
+    vga_draw_line15_15bgr_le,
+    vga_draw_line15_16bgr_le,
+
+    vga_draw_line16_8_le,
+    vga_draw_line16_15_le,
+    vga_draw_line16_16_le,
+    vga_draw_line16_32_le,
+    vga_draw_line16_32bgr_le,
+    vga_draw_line16_15bgr_le,
+    vga_draw_line16_16bgr_le,
+
+    vga_draw_line24_8_le,
+    vga_draw_line24_15_le,
+    vga_draw_line24_16_le,
+    vga_draw_line24_32_le,
+    vga_draw_line24_32bgr_le,
+    vga_draw_line24_15bgr_le,
+    vga_draw_line24_16bgr_le,
+
+    vga_draw_line32_8_le,
+    vga_draw_line32_15_le,
+    vga_draw_line32_16_le,
+    vga_draw_line32_32_le,
+    vga_draw_line32_32bgr_le,
+    vga_draw_line32_15bgr_le,
+    vga_draw_line32_16bgr_le,
+
+    vga_draw_line15_8_be,
+    vga_draw_line15_15_be,
+    vga_draw_line15_16_be,
+    vga_draw_line15_32_be,
+    vga_draw_line15_32bgr_be,
+    vga_draw_line15_15bgr_be,
+    vga_draw_line15_16bgr_be,
+
+    vga_draw_line16_8_be,
+    vga_draw_line16_15_be,
+    vga_draw_line16_16_be,
+    vga_draw_line16_32_be,
+    vga_draw_line16_32bgr_be,
+    vga_draw_line16_15bgr_be,
+    vga_draw_line16_16bgr_be,
+
+    vga_draw_line24_8_be,
+    vga_draw_line24_15_be,
+    vga_draw_line24_16_be,
+    vga_draw_line24_32_be,
+    vga_draw_line24_32bgr_be,
+    vga_draw_line24_15bgr_be,
+    vga_draw_line24_16bgr_be,
+
+    vga_draw_line32_8_be,
+    vga_draw_line32_15_be,
+    vga_draw_line32_16_be,
+    vga_draw_line32_32_be,
+    vga_draw_line32_32bgr_be,
+    vga_draw_line32_15bgr_be,
+    vga_draw_line32_16bgr_be,
 };
 
 static int vga_get_bpp(VGACommonState *s)
@@ -1645,10 +1743,14 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     uint8_t *d;
     uint32_t v, addr1, addr;
     vga_draw_line_func *vga_draw_line;
-#if defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN)
-    static const bool byteswap = false;
+    bool byteswap, is_be;
+
+    is_be = vga_is_big_endian(s);
+
+#ifdef HOST_WORDS_BIGENDIAN
+    byteswap = !is_be;
 #else
-    static const bool byteswap = true;
+    byteswap = is_be;
 #endif
 
     full_update |= update_basic_params(s);
@@ -1691,7 +1793,8 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
     if (s->line_offset != s->last_line_offset ||
         disp_width != s->last_width ||
         height != s->last_height ||
-        s->last_depth != depth) {
+        s->last_depth != depth ||
+        s->last_bswap != byteswap) {
         if (depth == 32 || (depth == 16 && !byteswap)) {
             surface = qemu_create_displaysurface_from(disp_width,
                     height, depth, s->line_offset,
@@ -1707,6 +1810,7 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         s->last_height = height;
         s->last_line_offset = s->line_offset;
         s->last_depth = depth;
+	s->last_bswap = byteswap;
         full_update = 1;
     } else if (is_buffer_shared(surface) &&
                (full_update || surface_data(surface) != s->vram_ptr
@@ -1750,19 +1854,19 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
             bits = 8;
             break;
         case 15:
-            v = VGA_DRAW_LINE15;
+            v = is_be ? VGA_DRAW_LINE15_BE : VGA_DRAW_LINE15_LE;
             bits = 16;
             break;
         case 16:
-            v = VGA_DRAW_LINE16;
+            v = is_be ? VGA_DRAW_LINE16_BE : VGA_DRAW_LINE16_LE;
             bits = 16;
             break;
         case 24:
-            v = VGA_DRAW_LINE24;
+            v = is_be ? VGA_DRAW_LINE24_BE : VGA_DRAW_LINE24_LE;
             bits = 24;
             break;
         case 32:
-            v = VGA_DRAW_LINE32;
+            v = is_be ? VGA_DRAW_LINE32_BE : VGA_DRAW_LINE32_LE;
             bits = 32;
             break;
         }
diff --git a/hw/display/vga_int.h b/hw/display/vga_int.h
index 5320abd..129360f 100644
--- a/hw/display/vga_int.h
+++ b/hw/display/vga_int.h
@@ -148,6 +148,7 @@ typedef struct VGACommonState {
     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 */
+    bool last_bswap;
     uint8_t cursor_start, cursor_end;
     bool cursor_visible_phase;
     int64_t cursor_blink_time;
@@ -205,6 +206,8 @@ uint32_t vbe_ioport_read_data(void *opaque, uint32_t addr);
 void vbe_ioport_write_index(void *opaque, uint32_t addr, uint32_t val);
 void vbe_ioport_write_data(void *opaque, uint32_t addr, uint32_t val);
 
+void vga_set_big_endian(bool be);
+
 extern const uint8_t sr_mask[8];
 extern const uint8_t gr_mask[16];
 
diff --git a/hw/display/vga_template.h b/hw/display/vga_template.h
index 90ec9c2..f8ea15f 100644
--- a/hw/display/vga_template.h
+++ b/hw/display/vga_template.h
@@ -35,13 +35,24 @@
 #error unsupport depth
 #endif
 
-#ifdef BGR_FORMAT
-#define PIXEL_NAME glue(DEPTH, bgr)
-#else
+#if BGR_FORMAT
+#if PIX_BE
+#define PIXEL_FNAME glue(DEPTH,bgr_be)
+#else /* PIX_BE */
+#define PIXEL_FNAME glue(DEPTH,bgr_le)
+#endif /* PIX_BE */ 
+#define PIXEL_NAME glue(DEPTH,bgr)
+#else /* BGR_FORMAT */
+#if PIX_BE
+#define PIXEL_FNAME glue(DEPTH,_be)
+#else /* PIX_BE */
+#define PIXEL_FNAME glue(DEPTH,_le)
+#endif /* PIX_BE */
 #define PIXEL_NAME DEPTH
 #endif /* BGR_FORMAT */
 
-#if DEPTH != 15 && !defined(BGR_FORMAT)
+
+#if DEPTH != 15 && !BGR_FORMAT && !PIX_BE
 
 static inline void glue(vga_draw_glyph_line_, DEPTH)(uint8_t *d,
                                                      uint32_t font_data,
@@ -350,10 +361,10 @@ static void glue(vga_draw_line8_, DEPTH)(VGACommonState *s1, uint8_t *d,
 /*
  * 15 bit color
  */
-static void glue(vga_draw_line15_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
+static void glue(vga_draw_line15_, PIXEL_FNAME)(VGACommonState *s1, uint8_t *d,
                                           const uint8_t *s, int width)
 {
-#if DEPTH == 15 && defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN)
+#if DEPTH == 15 && PIX_BE == defined(HOST_WORDS_BIGENDIAN)
     memcpy(d, s, width * 2);
 #else
     int w;
@@ -361,7 +372,11 @@ static void glue(vga_draw_line15_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 
     w = width;
     do {
-        v = lduw_p((void *)s);
+#if PIX_BE
+        v = lduw_be_p((void *)s);
+#else
+        v = lduw_le_p((void *)s);
+#endif
         r = (v >> 7) & 0xf8;
         g = (v >> 2) & 0xf8;
         b = (v << 3) & 0xf8;
@@ -375,10 +390,10 @@ static void glue(vga_draw_line15_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 /*
  * 16 bit color
  */
-static void glue(vga_draw_line16_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
+static void glue(vga_draw_line16_, PIXEL_FNAME)(VGACommonState *s1, uint8_t *d,
                                           const uint8_t *s, int width)
 {
-#if DEPTH == 16 && defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN)
+#if DEPTH == 16 && PIX_BE == defined(HOST_WORDS_BIGENDIAN)
     memcpy(d, s, width * 2);
 #else
     int w;
@@ -386,7 +401,11 @@ static void glue(vga_draw_line16_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 
     w = width;
     do {
-        v = lduw_p((void *)s);
+#if PIX_BE
+        v = lduw_be_p((void *)s);
+#else
+        v = lduw_le_p((void *)s);
+#endif
         r = (v >> 8) & 0xf8;
         g = (v >> 3) & 0xfc;
         b = (v << 3) & 0xf8;
@@ -400,7 +419,7 @@ static void glue(vga_draw_line16_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 /*
  * 24 bit color
  */
-static void glue(vga_draw_line24_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
+static void glue(vga_draw_line24_, PIXEL_FNAME)(VGACommonState *s1, uint8_t *d,
                                           const uint8_t *s, int width)
 {
     int w;
@@ -408,7 +427,7 @@ static void glue(vga_draw_line24_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 
     w = width;
     do {
-#if defined(TARGET_WORDS_BIGENDIAN)
+#if PIX_BE
         r = s[0];
         g = s[1];
         b = s[2];
@@ -426,10 +445,10 @@ static void glue(vga_draw_line24_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 /*
  * 32 bit color
  */
-static void glue(vga_draw_line32_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
+static void glue(vga_draw_line32_, PIXEL_FNAME)(VGACommonState *s1, uint8_t *d,
                                           const uint8_t *s, int width)
 {
-#if DEPTH == 32 && defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN) && !defined(BGR_FORMAT)
+#if DEPTH == 32 && !BGR_FORMAT && PIX_BE == defined(HOST_WORDS_BIGENDIAN)
     memcpy(d, s, width * 4);
 #else
     int w;
@@ -437,7 +456,7 @@ static void glue(vga_draw_line32_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 
     w = width;
     do {
-#if defined(TARGET_WORDS_BIGENDIAN)
+#if PIX_BE
         r = s[1];
         g = s[2];
         b = s[3];
@@ -458,4 +477,7 @@ static void glue(vga_draw_line32_, PIXEL_NAME)(VGACommonState *s1, uint8_t *d,
 #undef BPP
 #undef PIXEL_TYPE
 #undef PIXEL_NAME
+#undef PIXEL_FNAME
 #undef BGR_FORMAT
+#undef PIX_BE
+
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0bae053..c8aaf3b 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -709,6 +709,8 @@ static target_ulong h_logical_dcbf(PowerPCCPU *cpu, sPAPREnvironment *spapr,
     return H_SUCCESS;
 }
 
+extern void vga_set_big_endian(bool trueis_be);
+
 static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                target_ulong opcode, target_ulong *args)
 {
@@ -733,6 +735,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPREnvironment *spapr,
             CPU_FOREACH(cs) {
                 set_spr(cs, SPR_LPCR, 0, LPCR_ILE);
             }
+            vga_set_big_endian(true);
             ret = H_SUCCESS;
             break;
 
@@ -740,6 +743,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, sPAPREnvironment *spapr,
             CPU_FOREACH(cs) {
                 set_spr(cs, SPR_LPCR, LPCR_ILE, LPCR_ILE);
             }
+            vga_set_big_endian(false);
             ret = H_SUCCESS;
             break;