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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2014-06-17 at 12:45 +0200, Gerd Hoffmann wrote:
>   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" ?

Linux fbdev has some ability to deal with foreign endian but at a
performance cost.

Xorg has much more serious issues, unless we start using something like
shadowfb and byteswap everything on the way out, but again, at a
significant performance cost.

> > 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.

It should provided we have the right byte order in qemu for the guest
endian :-) But if it's the KMS driver I think it is yes, I did try an
earlier version, and it should work with the same problem that qemu
needs to change it's layout with the guest endian.

> 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.

Right.

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

Right :) As I said, it's a trainwreck along the whole stack. I think my
second patch is reasonably non-invasive and shouldn't affect performance
of the existing path though.

> 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.

I'm happy for that to happen upstream though I think I might go with my
current hack for powerkvm so we have a more immediate solution for
ubuntu and possibly sles12.

> 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 ...

pixman byteswaps 32bpp ? Good, I'd rather leave the work to it since it
has vector accelerations and other similar niceties which would make it
a better place than qemu.

However we still need to deal with 15/16bpp guest side fb

> > @@ -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.

Provider we also give pixman the right pixel format for "reverse endian"
which we don't do today and I yet have to investigate it :-) The pixmap
stuff in qemu to be honest is news to me, last time I looked it was SDL
and hand made vnc. Is the vnc server going though pixman as well ?

Cheers,
Ben.