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

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(-)

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