Re: [Qemu-devel] [RFC] qemu VGA endian swap low level drawing changes

[Gerd Hoffmann]

  Hi,

> The core issue stems from the fact that graphic stacks including X and a
> number of other components along the way are terminally broken if the
> framebuffer endianness doesn't match the guest endianness. We can
> discuss the historical reasons for that fact if you wish and we can
> discuss why it is almost unfixable separately.

"framebuffer" as in "linux framebuffer device" or as in "whatever memory
area X11 renders into" ?

> This is an obvious problem for qemu on ppc64 since we now support having
> either BE or LE guests.
> 
> At the moment, we have a broken patch in offb that makes the guest
> sort-of limp along with the reversed framebuffer but that patch is
> broken in several ways so I've reverted it upstream and X wouldn't work
> anyway.

Side note: tried "CONFIG_DRM_BOCHS=m"?

In theory it should just work.  I've never actually tested it in
practice on bigendian.

Not sure it helps in any way with the byteorder issue though.  Probably
not.  Could help with the "how do we flip the framebuffer byteorder"
though, as we can simply define a new register and let the driver set
it.

> This is purely about discussing the changes to vga.c and vga-templates.h
> to be able to handle the swapping in a runtime-decided way rather than
> compile time.

> What do you prefer ?

Let pixman handle it?  Well, except that pixman can't handle byteswapped
16bpp formats.  Too bad :(

In any case cleaning up the whole mess is overdue and doing that
beforehand should simplify the job alot.  For quite some time qemu uses
32bpp internally no matter what, so the functions for converting the
guest's framebuffer into anything but 32bpp should be dead code.

And as long as the guest uses 32bpp too there is nothing converted
anyway, we just setup pixman image in the correct format, backed by the
guests vga memory.  So this ...

> +#ifdef TARGET_WORDS_BIGENDIAN
> +static bool vga_is_be = true;
> +#else
> +static bool vga_is_be = false;
> +#endif
> +
> +void vga_set_big_endian(bool be)
> +{
> +    vga_is_be = be;
> +}
> +
> +static inline bool vga_is_big_endian(VGACommonState *s)
> +{
> +    return vga_is_be;
> +}
> +
> +static inline bool vga_need_swap(VGACommonState *s, bool target_be)
> +{
> +#ifdef HOST_WORDS_BIGENDIAN
> +    return !target_be;
> +#else
> +    return target_be;
> +#endif
> +}

> @@ -1645,11 +1690,9 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>      uint8_t *d;
>      uint32_t v, addr1, addr;
>      vga_draw_line_func *vga_draw_line;
> -#if defined(HOST_WORDS_BIGENDIAN) == defined(TARGET_WORDS_BIGENDIAN)
> -    static const bool byteswap = false;
> -#else
> -    static const bool byteswap = true;
> -#endif
> +    bool byteswap;
> +
> +    byteswap = vga_need_swap(s, vga_is_big_endian(s));
>  
>      full_update |= update_basic_params(s);
>  
> @@ -1691,7 +1734,8 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>      if (s->line_offset != s->last_line_offset ||
>          disp_width != s->last_width ||
>          height != s->last_height ||
> -        s->last_depth != depth) {
> +        s->last_depth != depth ||
> +        s->last_bswap != byteswap) {
>          if (depth == 32 || (depth == 16 && !byteswap)) {
>              surface = qemu_create_displaysurface_from(disp_width,
>                      height, depth, s->line_offset,
> @@ -1707,6 +1751,7 @@ static void vga_draw_graphic(VGACommonState *s, int 
> full_update)
>          s->last_height = height;
>          s->last_line_offset = s->line_offset;
>          s->last_depth = depth;
> +     s->last_bswap = byteswap;
>          full_update = 1;
>      } else if (is_buffer_shared(surface) &&
>                 (full_update || surface_data(surface) != s->vram_ptr

... is all you need to make things fly for the 32bpp case.

cheers,
  Gerd