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.
