| Message ID | 6003064214f099ab5ec9625e356543b22f01b9cf.1488665935.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 February 2017 at 18:46, BALATON Zoltan <balaton@eik.bme.hu> wrote: > We only emulate the sysbus device in its default LE mode and PCI is LE > as well so specify this for registers. Colors were also off on both > SH4 and PPC which is also fixed here. > diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h > index 832ee61..6e56baf 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 This says "the pixel in the framebuffer is little endian. If the guest CPU is bigendian then the pixel format is RGB565; otherwise it's BGR565". (This difference isn't a simple endianness thing because the G pixel crosses the boundary between two bytes.) In the Linux kernel driver source, these two formats correspond to "SWAP_FB_ENDIAN set" (BGR565) and not set (RGB565), and the swap flag is set for BE PCI and for (LE) embedded sh. I'm not convinced by that code though, because the comment claims it's for handling an IO layer swap that makes the PCI bus little endian, but that can't make RGB565 turn into BGR565. See also kernel commit fedbb3625b3c1644 which also says that code is wrong. So I would not trust the kernel driver as a guide to the right behaviour for 16 bit formats: it is clearly buggy. > *(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 says "the pixel in the framebuffer is 32-bits always little endian. If the guest CPU is bigendian then the pixel format is XBGR; otherwise it's RGBX". This is also saying the hardware behaves differently for big and little endian guest CPUs, but in a different way from 16 bit (both in whether big or little gets the RGB order and in whether the other order is a straight "all the fields in reverse order" or not). That seems even less likely. In the Linux kernel, "SWAP_FB_ENDIAN" flag set gets us BGRX; not set gets us XRGB. I think the correct behaviour here is for 16 bits: rgb565 = lduw_le_p(s); r = (rgb565 >> 8) & 0xf8; g = (rgb565 >> 3) & 0xfc; b = (rgb565 << 3) & 0xf8; ie "always LE framebuffer, RGB565" and for 32 bits: r = s[2]; g = s[1]; b = s[0]; ie "always LE framebuffer, XRGB8888". That makes sense, and it matches what the hardware claims it does. I would recommend implementing that, unless it doesn't work for some bit of guest software that you can 100% guarantee does display correctly on real hardware. If it doesn't work on a particular bit of guest software then that is quite likely guest software error because nobody really thoroughly tested the driver code a decade ago and nobody has the hardware now to test the driver against. The sm501 x.org driver appears to have been independently implemented, so might be an interesting thing to test. (Yes, trying to sort out a model of ancient hardware so it's accurate sucks. If you can find a real copy of the hardware so you can cross-check what it really does that's about the only way to be sure about things.) thanks -- PMM
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index c92a5fa..a628ef1 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -850,7 +850,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) @@ -1086,7 +1086,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, @@ -1174,7 +1174,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 */ @@ -1510,7 +1510,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) if (s->chr_state) { serial_mm_init(&s->state.mmio_region, SM501_UART0, 2, NULL, /* TODO : chain irq to IRL */ - 115200, s->chr_state, DEVICE_NATIVE_ENDIAN); + 115200, s->chr_state, DEVICE_LITTLE_ENDIAN); } } diff --git a/hw/display/sm501_template.h b/hw/display/sm501_template.h index 832ee61..6e56baf 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;
We only emulate the sysbus device in its default LE mode and PCI is LE as well so specify this for registers. Colors were also off on both SH4 and PPC which is also fixed here. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v2: Split off small clean up to other patch v4: Set serial part to little endian as well hw/display/sm501.c | 8 ++++---- hw/display/sm501_template.h | 23 ++++++++++++++--------- 2 files changed, 18 insertions(+), 13 deletions(-)