Re: [Qemu-devel] [PATCH v2 07/14] sm501: Fix device endianness

[Peter Maydell <peter.maydell@linaro.org>]

On 25 February 2017 at 18:46, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>
> v2: Split off small clean up to other patch
>
>  hw/display/sm501.c          |  6 +++---
>  hw/display/sm501_template.h | 23 ++++++++++++++---------
>  2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index b977094..2694081 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -849,7 +849,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)
> @@ -1085,7 +1085,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,
> @@ -1173,7 +1173,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 */
> 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;

I still suspect this is not correct. In particular, if you
look at the Linux driver:

http://lxr.free-electrons.com/source/drivers/video/fbdev/sm501fb.c#L341

we have for 32 bit two layouts within a 32-bit word:
 BGRX (if "swap fb endian" is set)
 XRGB (the usual)
(plus the cross-product with "32-bit load is BE or LE",
since we've coded this as byte accesses rather than a ldl_*_p()).

and for 16 bit we have either RGB or BGR ordering.
I'm not completely sure this lines up with the code you have.

Looking at how the SWAP_FB_ENDIAN flag gets set:
 * for the r2d board it is set always
 * for PCI devices, it is set only if big-endian

I suspect that what we actually have here is something
like:
 * for the PCI device it's always little endian (and the
   code ought not to need to depend on TARGET_WORDS_BIGENDIAN)
 * sysbus device may be more complicated

Also I note there's an SM501_ENDIAN_CONTROL register on the
device, which presumably if written changes the behaviour.

Plus for the sysbus device if we change the settings to
DEVICE_LITTLE_ENDIAN for the various control register
regions that suggests we should be consistent about the
serial_mm_init() endian order and also the USB controller.

It's late evening here though so I can't investigate any
further right now.

thanks
-- PMM