diff mbox

[06/10] sm501: Fix device endianness

Message ID 9c4f503401fa3defa5dff8b938a0bee3aa8d89d5.1487522123.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show

Commit Message

BALATON Zoltan Feb. 19, 2017, 4:35 p.m. UTC
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c          |  6 +++---
 hw/display/sm501_template.h | 31 ++++++++++++++++++-------------
 2 files changed, 21 insertions(+), 16 deletions(-)

Comments

Peter Maydell Feb. 24, 2017, 2:50 p.m. UTC | #1
On 19 February 2017 at 16:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/sm501.c          |  6 +++---
>  hw/display/sm501_template.h | 31 ++++++++++++++++++-------------
>  2 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 9091bb5..3d32a3c 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -846,7 +846,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)
> @@ -1082,7 +1082,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,
> @@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops = {
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
> -    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };

Does this still all work for the sh4eb (big-endian) sysbus device case?

>  /* draw line functions for all console modes */
> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
> index 832ee61..5b516d6 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

This looks really suspicious. Previously we had
 TARGET_WORDS_BIGENDIAN -> bytes are XRGB
 otherwise                 bytes are BGRX

Now we have
 TARGET_WORDS_BIGENDIAN -> bytes are RGBX
 otherwise              -> bytes are XRGB

That doesn't seem very plausible at all.

>          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>          s += 4;


> @@ -103,7 +108,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
>                   uint8_t *palette, int c_y, uint8_t *d, int width)
>  {
>      int x, i;
> -    uint8_t *pixval, bitset = 0;
> +    uint8_t *pixval, r, g, b, bitset = 0;
>
>      /* get hardware cursor pattern */
>      uint32_t cursor_addr = get_hwc_address(s, crt);
> @@ -129,9 +134,9 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
>          /* write pixel */
>          if (v) {
>              v--;
> -            uint8_t r = palette[v * 3 + 0];
> -            uint8_t g = palette[v * 3 + 1];
> -            uint8_t b = palette[v * 3 + 2];
> +            r = palette[v * 3 + 0];
> +            g = palette[v * 3 + 1];
> +            b = palette[v * 3 + 2];
>              *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>          }
>          d += BPP;

This doesn't seem to be related to the rest of this patch?

thanks
-- PMM
BALATON Zoltan Feb. 24, 2017, 8:35 p.m. UTC | #2
On Fri, 24 Feb 2017, Peter Maydell wrote:
> On 19 February 2017 at 16:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/sm501.c          |  6 +++---
>>  hw/display/sm501_template.h | 31 ++++++++++++++++++-------------
>>  2 files changed, 21 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 9091bb5..3d32a3c 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -846,7 +846,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)
>> @@ -1082,7 +1082,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,
>> @@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops = {
>>          .min_access_size = 4,
>>          .max_access_size = 4,
>>      },
>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>
> Does this still all work for the sh4eb (big-endian) sysbus device case?

Not sure. Is there some image somewhere I can test with?

>>  /* draw line functions for all console modes */
>> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
>> index 832ee61..5b516d6 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
>
> This looks really suspicious. Previously we had
> TARGET_WORDS_BIGENDIAN -> bytes are XRGB
> otherwise                 bytes are BGRX
>
> Now we have
> TARGET_WORDS_BIGENDIAN -> bytes are RGBX
> otherwise              -> bytes are XRGB
>
> That doesn't seem very plausible at all.

I've tested it with sh4 and ppc guests running on x86_64 host and these 
work now while previous code resulted in mixed up colors. Maybe the host 
endianness could also be a factor and the previous code assumed big endian 
host or the previous code was already broken and only worked with the 
default 8 bit depth. I'm not completely sure I understand endian handling 
in QEMU to know if this is correct besides on what I've tested but at 
least little endian and big endian guests should work on little endian 
hosts now with my patch. I can't test on big endian host.

I've used this image: https://people.debian.org/~aurel32/qemu/sh4/
with video=800x600-16 kernel parameter where changing -16 to different bit 
depths reproduces the problem.

>>          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>          s += 4;
>
>
>> @@ -103,7 +108,7 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
>>                   uint8_t *palette, int c_y, uint8_t *d, int width)
>>  {
>>      int x, i;
>> -    uint8_t *pixval, bitset = 0;
>> +    uint8_t *pixval, r, g, b, bitset = 0;
>>
>>      /* get hardware cursor pattern */
>>      uint32_t cursor_addr = get_hwc_address(s, crt);
>> @@ -129,9 +134,9 @@ static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
>>          /* write pixel */
>>          if (v) {
>>              v--;
>> -            uint8_t r = palette[v * 3 + 0];
>> -            uint8_t g = palette[v * 3 + 1];
>> -            uint8_t b = palette[v * 3 + 2];
>> +            r = palette[v * 3 + 0];
>> +            g = palette[v * 3 + 1];
>> +            b = palette[v * 3 + 2];
>>              *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>          }
>>          d += BPP;
>
> This doesn't seem to be related to the rest of this patch?

Should I split it off? Making every simple change another patch may double 
the size of the series but if that's preferred I can make this a separate 
patch.
Peter Maydell Feb. 25, 2017, 4:21 p.m. UTC | #3
On 24 February 2017 at 20:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>
>> On 19 February 2017 at 16:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/display/sm501.c          |  6 +++---
>>>  hw/display/sm501_template.h | 31 ++++++++++++++++++-------------
>>>  2 files changed, 21 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index 9091bb5..3d32a3c 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -846,7 +846,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)
>>> @@ -1082,7 +1082,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,
>>> @@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops =
>>> {
>>>          .min_access_size = 4,
>>>          .max_access_size = 4,
>>>      },
>>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>  };
>>
>>
>> Does this still all work for the sh4eb (big-endian) sysbus device case?
>
>
> Not sure. Is there some image somewhere I can test with?

I don't know, I'm afraid; I don't know anything about sh4.
I'm just going by looking at the code changes and flagging
up things which are potentially problems.

>>>  /* draw line functions for all console modes */
>>> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
>>> index 832ee61..5b516d6 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
>>
>>
>> This looks really suspicious. Previously we had
>> TARGET_WORDS_BIGENDIAN -> bytes are XRGB
>> otherwise                 bytes are BGRX
>>
>> Now we have
>> TARGET_WORDS_BIGENDIAN -> bytes are RGBX
>> otherwise              -> bytes are XRGB
>>
>> That doesn't seem very plausible at all.
>
>
> I've tested it with sh4 and ppc guests running on x86_64 host and these work
> now while previous code resulted in mixed up colors. Maybe the host
> endianness could also be a factor and the previous code assumed big endian
> host or the previous code was already broken and only worked with the
> default 8 bit depth. I'm not completely sure I understand endian handling in
> QEMU to know if this is correct besides on what I've tested but at least
> little endian and big endian guests should work on little endian hosts now
> with my patch. I can't test on big endian host.
>
> I've used this image: https://people.debian.org/~aurel32/qemu/sh4/
> with video=800x600-16 kernel parameter where changing -16 to different bit
> depths reproduces the problem.

Again, I don't know sh, and I don't know this particular device.
I'm just pointing out that the code change here looks really
implausible. Maybe the data sheet says that this is what the
pixel format looks like; but it's very odd.

>>>          *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>>          s += 4;
>>
>>
>>
>>> @@ -103,7 +108,7 @@ static void glue(draw_hwc_line_,
>>> PIXEL_NAME)(SM501State *s, int crt,
>>>                   uint8_t *palette, int c_y, uint8_t *d, int width)
>>>  {
>>>      int x, i;
>>> -    uint8_t *pixval, bitset = 0;
>>> +    uint8_t *pixval, r, g, b, bitset = 0;
>>>
>>>      /* get hardware cursor pattern */
>>>      uint32_t cursor_addr = get_hwc_address(s, crt);
>>> @@ -129,9 +134,9 @@ static void glue(draw_hwc_line_,
>>> PIXEL_NAME)(SM501State *s, int crt,
>>>          /* write pixel */
>>>          if (v) {
>>>              v--;
>>> -            uint8_t r = palette[v * 3 + 0];
>>> -            uint8_t g = palette[v * 3 + 1];
>>> -            uint8_t b = palette[v * 3 + 2];
>>> +            r = palette[v * 3 + 0];
>>> +            g = palette[v * 3 + 1];
>>> +            b = palette[v * 3 + 2];
>>>              *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
>>>          }
>>>          d += BPP;
>>
>>
>> This doesn't seem to be related to the rest of this patch?
>
>
> Should I split it off? Making every simple change another patch may double
> the size of the series but if that's preferred I can make this a separate
> patch.

The aim is to make it easier to review. Patches which are just
simple cleanup of code are easy to review because it's
possible to say "no change of behaviour". Patches which are
making functional changes require more thought, and it gets
worse if the same patch is also making no-change-of-behaviour
code changes.

thanks
-- PMM
BALATON Zoltan Feb. 26, 2017, 12:49 a.m. UTC | #4
On Sat, 25 Feb 2017, Peter Maydell wrote:
> On 24 February 2017 at 20:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> On Fri, 24 Feb 2017, Peter Maydell wrote:
>>> On 19 February 2017 at 16:35, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/display/sm501.c          |  6 +++---
>>>>  hw/display/sm501_template.h | 31 ++++++++++++++++++-------------
>>>>  2 files changed, 21 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>>> index 9091bb5..3d32a3c 100644
>>>> --- a/hw/display/sm501.c
>>>> +++ b/hw/display/sm501.c
>>>> @@ -846,7 +846,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)
>>>> @@ -1082,7 +1082,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,
>>>> @@ -1170,7 +1170,7 @@ static const MemoryRegionOps sm501_2d_engine_ops =
>>>> {
>>>>          .min_access_size = 4,
>>>>          .max_access_size = 4,
>>>>      },
>>>> -    .endianness = DEVICE_NATIVE_ENDIAN,
>>>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>>>>  };
>>>
>>>
>>> Does this still all work for the sh4eb (big-endian) sysbus device case?
>>
>>
>> Not sure. Is there some image somewhere I can test with?
>
> I don't know, I'm afraid; I don't know anything about sh4.
> I'm just going by looking at the code changes and flagging
> up things which are potentially problems.
>
>>>>  /* draw line functions for all console modes */
>>>> diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h
>>>> index 832ee61..5b516d6 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
>>>
>>>
>>> This looks really suspicious. Previously we had
>>> TARGET_WORDS_BIGENDIAN -> bytes are XRGB
>>> otherwise                 bytes are BGRX
>>>
>>> Now we have
>>> TARGET_WORDS_BIGENDIAN -> bytes are RGBX
>>> otherwise              -> bytes are XRGB
>>>
>>> That doesn't seem very plausible at all.
>>
>>
>> I've tested it with sh4 and ppc guests running on x86_64 host and these work
>> now while previous code resulted in mixed up colors. Maybe the host
>> endianness could also be a factor and the previous code assumed big endian
>> host or the previous code was already broken and only worked with the
>> default 8 bit depth. I'm not completely sure I understand endian handling in
>> QEMU to know if this is correct besides on what I've tested but at least
>> little endian and big endian guests should work on little endian hosts now
>> with my patch. I can't test on big endian host.
>>
>> I've used this image: https://people.debian.org/~aurel32/qemu/sh4/
>> with video=800x600-16 kernel parameter where changing -16 to different bit
>> depths reproduces the problem.
>
> Again, I don't know sh, and I don't know this particular device.
> I'm just pointing out that the code change here looks really
> implausible. Maybe the data sheet says that this is what the
> pixel format looks like; but it's very odd.

I don't know much about it either. I could not find docs about framebuffer 
endianness of this device but all clients I've seen treat it as little 
endian, both on little endian SH4 and big endian PPC. Maybe the previous 
code was already broken, because on a black & white text console (like the 
SH4 test image listed on qemu.org) the problem is not obvious, it still 
looks like white text on black with mixed up color components. It can only 
be seen when some graphics is displayed like the Tux logo on framebuffer 
in the above Debian image. If it ever worked it could have been easily 
broken by some qemu console changes. Or maybe it worked on big endian host 
only but I could not verify that. All I can say it works for me now on LE 
host with both LE and BE guests that I could test.
diff mbox

Patch

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 9091bb5..3d32a3c 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -846,7 +846,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)
@@ -1082,7 +1082,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,
@@ -1170,7 +1170,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..5b516d6 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;
@@ -103,7 +108,7 @@  static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
                  uint8_t *palette, int c_y, uint8_t *d, int width)
 {
     int x, i;
-    uint8_t *pixval, bitset = 0;
+    uint8_t *pixval, r, g, b, bitset = 0;
 
     /* get hardware cursor pattern */
     uint32_t cursor_addr = get_hwc_address(s, crt);
@@ -129,9 +134,9 @@  static void glue(draw_hwc_line_, PIXEL_NAME)(SM501State *s, int crt,
         /* write pixel */
         if (v) {
             v--;
-            uint8_t r = palette[v * 3 + 0];
-            uint8_t g = palette[v * 3 + 1];
-            uint8_t b = palette[v * 3 + 2];
+            r = palette[v * 3 + 0];
+            g = palette[v * 3 + 1];
+            b = palette[v * 3 + 2];
             *(PIXEL_TYPE *)d = glue(rgb_to_pixel, PIXEL_NAME)(r, g, b);
         }
         d += BPP;