diff mbox

[v4,07/13] sm501: Fix device endianness

Message ID 6003064214f099ab5ec9625e356543b22f01b9cf.1488665935.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show

Commit Message

BALATON Zoltan Feb. 25, 2017, 6:46 p.m. UTC
We only emulate the sysbus device in its default LE mode and PCI is LE
as well so specify this for registers. Colors were also off on both
SH4 and PPC which is also fixed here.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---

v2: Split off small clean up to other patch
v4: Set serial part to little endian as well

 hw/display/sm501.c          |  8 ++++----
 hw/display/sm501_template.h | 23 ++++++++++++++---------
 2 files changed, 18 insertions(+), 13 deletions(-)

Comments

Peter Maydell March 6, 2017, 11:46 a.m. UTC | #1
On 25 February 2017 at 18:46, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> We only emulate the sysbus device in its default LE mode and PCI is LE
> as well so specify this for registers. Colors were also off on both
> SH4 and PPC which is also fixed here.

> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
> index 832ee61..6e56baf 100644
> --- a/hw/display/sm501_template.h
> +++ b/hw/display/sm501_template.h
> @@ -64,10 +64,16 @@ static void glue(draw_line16_, PIXEL_NAME)(
>      uint8_t r, g, b;
>
>      do {
> -        rgb565 = lduw_p(s);
> -        r = ((rgb565 >> 11) & 0x1f) << 3;
> -        g = ((rgb565 >>  5) & 0x3f) << 2;
> -        b = ((rgb565 >>  0) & 0x1f) << 3;
> +        rgb565 = lduw_le_p(s);
> +#if defined(TARGET_WORDS_BIGENDIAN)
> +        r = (rgb565 >> 8) & 0xf8;
> +        g = (rgb565 >> 3) & 0xfc;
> +        b = (rgb565 << 3) & 0xf8;
> +#else
> +        b = (rgb565 >> 8) & 0xf8;
> +        g = (rgb565 >> 3) & 0xfc;
> +        r = (rgb565 << 3) & 0xf8;
> +#endif

This says "the pixel in the framebuffer is little endian. If
the guest CPU is bigendian then the pixel format is RGB565;
otherwise it's BGR565". (This difference isn't a simple
endianness thing because the G pixel crosses the boundary
between two bytes.)

In the Linux kernel driver source, these two formats
correspond to "SWAP_FB_ENDIAN set" (BGR565) and not set
(RGB565), and the swap flag is set for BE PCI and for
(LE) embedded sh. I'm not convinced by that code though,
because the comment claims it's for handling an IO layer
swap that makes the PCI bus little endian, but that can't
make RGB565 turn into BGR565. See also kernel commit
fedbb3625b3c1644 which also says that code is wrong.
So I would not trust the kernel driver as a guide to the
right behaviour for 16 bit formats: it is clearly buggy.

>          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>          s += 2;
>          d += BPP;
> @@ -80,15 +86,14 @@ static void glue(draw_line32_, PIXEL_NAME)(
>      uint8_t r, g, b;
>
>      do {
> -        ldub_p(s);
>  #if defined(TARGET_WORDS_BIGENDIAN)
> +        r = s[0];
> +        g = s[1];
> +        b = s[2];
> +#else
>          r = s[1];
>          g = s[2];
>          b = s[3];
> -#else
> -        b = s[0];
> -        g = s[1];
> -        r = s[2];
>  #endif

This says "the pixel in the framebuffer is 32-bits always
little endian. If the guest CPU is bigendian then the
pixel format is XBGR; otherwise it's RGBX".

This is also saying the hardware behaves differently
for big and little endian guest CPUs, but in a different
way from 16 bit (both in whether big or little gets
the RGB order and in whether the other order is a
straight "all the fields in reverse order" or not).
That seems even less likely.

In the Linux kernel, "SWAP_FB_ENDIAN" flag set gets
us BGRX; not set gets us XRGB.

I think the correct behaviour here is for 16 bits:
        rgb565 = lduw_le_p(s);
        r = (rgb565 >> 8) & 0xf8;
        g = (rgb565 >> 3) & 0xfc;
        b = (rgb565 << 3) & 0xf8;

ie "always LE framebuffer, RGB565"
and for 32 bits:
        r = s[2];
        g = s[1];
        b = s[0];
ie "always LE framebuffer, XRGB8888".

That makes sense, and it matches what the hardware
claims it does. I would recommend implementing that, unless
it doesn't work for some bit of guest software that you can
100% guarantee does display correctly on real hardware.

If it doesn't work on a particular bit of guest software
then that is quite likely guest software error
because nobody really thoroughly tested the driver
code a decade ago and nobody has the hardware now
to test the driver against.

The sm501 x.org driver appears to have been independently
implemented, so might be an interesting thing to test.

(Yes, trying to sort out a model of ancient hardware so it's
accurate sucks. If you can find a real copy of the hardware
so you can cross-check what it really does that's about
the only way to be sure about things.)

thanks
-- PMM
diff mbox

Patch

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index c92a5fa..a628ef1 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -850,7 +850,7 @@  static const MemoryRegionOps sm501_system_config_ops = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
@@ -1086,7 +1086,7 @@  static const MemoryRegionOps sm501_disp_ctrl_ops = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 static uint64_t sm501_2d_engine_read(void *opaque, hwaddr addr,
@@ -1174,7 +1174,7 @@  static const MemoryRegionOps sm501_2d_engine_ops = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
-    .endianness = DEVICE_NATIVE_ENDIAN,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 /* draw line functions for all console modes */
@@ -1510,7 +1510,7 @@  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
     if (s->chr_state) {
         serial_mm_init(&s->state.mmio_region, SM501_UART0, 2,
                        NULL, /* TODO : chain irq to IRL */
-                       115200, s->chr_state, DEVICE_NATIVE_ENDIAN);
+                       115200, s->chr_state, DEVICE_LITTLE_ENDIAN);
     }
 }
 
diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
index 832ee61..6e56baf 100644
--- a/hw/display/sm501_template.h
+++ b/hw/display/sm501_template.h
@@ -64,10 +64,16 @@  static void glue(draw_line16_, PIXEL_NAME)(
     uint8_t r, g, b;
 
     do {
-        rgb565 = lduw_p(s);
-        r = ((rgb565 >> 11) & 0x1f) << 3;
-        g = ((rgb565 >>  5) & 0x3f) << 2;
-        b = ((rgb565 >>  0) & 0x1f) << 3;
+        rgb565 = lduw_le_p(s);
+#if defined(TARGET_WORDS_BIGENDIAN)
+        r = (rgb565 >> 8) & 0xf8;
+        g = (rgb565 >> 3) & 0xfc;
+        b = (rgb565 << 3) & 0xf8;
+#else
+        b = (rgb565 >> 8) & 0xf8;
+        g = (rgb565 >> 3) & 0xfc;
+        r = (rgb565 << 3) & 0xf8;
+#endif
         *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
         s += 2;
         d += BPP;
@@ -80,15 +86,14 @@  static void glue(draw_line32_, PIXEL_NAME)(
     uint8_t r, g, b;
 
     do {
-        ldub_p(s);
 #if defined(TARGET_WORDS_BIGENDIAN)
+        r = s[0];
+        g = s[1];
+        b = s[2];
+#else
         r = s[1];
         g = s[2];
         b = s[3];
-#else
-        b = s[0];
-        g = s[1];
-        r = s[2];
 #endif
         *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
         s += 4;