diff mbox

[PULL,v4,05/12] milkymist-vgafb: swap pixel data in source buffer

Message ID 1390246471-25167-6-git-send-email-michael@walle.cc
State New
Headers show

Commit Message

Michael Walle Jan. 20, 2014, 7:34 p.m. UTC
In commit fc97bb5ba3e7239c0b6d24095df6784868dfebbf the lduw_raw() call was
eliminated. But we are reading from the target buffer a 16-bit value, which
is in big-endian format. Therefore, swap the bytes if we are building for a
little-endian host.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael Walle <michael@walle.cc>
---
 hw/display/milkymist-vgafb_template.h |    1 +
 1 file changed, 1 insertion(+)

Comments

Peter Maydell Feb. 1, 2014, 5:57 p.m. UTC | #1
On 20 January 2014 19:34, Michael Walle <michael@walle.cc> wrote:
> In commit fc97bb5ba3e7239c0b6d24095df6784868dfebbf the lduw_raw() call was
> eliminated. But we are reading from the target buffer a 16-bit value, which
> is in big-endian format. Therefore, swap the bytes if we are building for a
> little-endian host.

Paolo, can you remember why you included this change in that commit?
It purports to just be moving the display devices around but it seems to
have included the introduction of this bug, and also a removal of a lduw_raw()
call from (what is now) hw/display/blizzard_template.h which I suspect is
also wrong...

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  hw/display/milkymist-vgafb_template.h |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/display/milkymist-vgafb_template.h b/hw/display/milkymist-vgafb_template.h
> index e0036e1..3f25484 100644
> --- a/hw/display/milkymist-vgafb_template.h
> +++ b/hw/display/milkymist-vgafb_template.h
> @@ -62,6 +62,7 @@ static void glue(draw_line_, BITS)(void *opaque, uint8_t *d, const uint8_t *s,
>
>      while (width--) {
>          memcpy(&rgb565, s, sizeof(rgb565));
> +        rgb565 = be16_to_cpu(rgb565);

If we know the framebuffer is always bigendian (regardless of the
target CPU endianness) then rather than memcpy and then
byteswap we might as well just
    rgb565 = lduw_be_p(s);

I think.

>          r = ((rgb565 >> 11) & 0x1f) << 3;
>          g = ((rgb565 >>  5) & 0x3f) << 2;
>          b = ((rgb565 >>  0) & 0x1f) << 3;

thanks
-- PMM
Paolo Bonzini Feb. 3, 2014, 8:12 a.m. UTC | #2
Il 01/02/2014 18:57, Peter Maydell ha scritto:
>> > In commit fc97bb5ba3e7239c0b6d24095df6784868dfebbf the lduw_raw() call was
>> > eliminated. But we are reading from the target buffer a 16-bit value, which
>> > is in big-endian format. Therefore, swap the bytes if we are building for a
>> > little-endian host.
> Paolo, can you remember why you included this change in that commit?
> It purports to just be moving the display devices around but it seems to
> have included the introduction of this bug, and also a removal of a lduw_raw()
> call from (what is now) hw/display/blizzard_template.h which I suspect is
> also wrong...
>

Most likely it was an incorrectly squashed patch.

Paolo
diff mbox

Patch

diff --git a/hw/display/milkymist-vgafb_template.h b/hw/display/milkymist-vgafb_template.h
index e0036e1..3f25484 100644
--- a/hw/display/milkymist-vgafb_template.h
+++ b/hw/display/milkymist-vgafb_template.h
@@ -62,6 +62,7 @@  static void glue(draw_line_, BITS)(void *opaque, uint8_t *d, const uint8_t *s,
 
     while (width--) {
         memcpy(&rgb565, s, sizeof(rgb565));
+        rgb565 = be16_to_cpu(rgb565);
         r = ((rgb565 >> 11) & 0x1f) << 3;
         g = ((rgb565 >>  5) & 0x3f) << 2;
         b = ((rgb565 >>  0) & 0x1f) << 3;