| Message ID | 20230224001852.4B813746324@zero.eik.bme.hu |
|---|---|
| State | New |
| Headers | show |
| Series | hw/display/sm501: Add alternatives to pixman routines | expand |
On Fri, 24 Feb 2023 at 00:18, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > Pixman can sometimes return false so add fallbacks for such cases and > also add a property to disable pixman and always use the fallbacks > which can be useful on platforms where pixman is broken or for testing > different drawing methods. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > Also ping for the other sm501 patch I've sent a week ago: > https://patchew.org/QEMU/20230216144043.D632874634B@zero.eik.bme.hu/ > These two patches are needed to fix graphics issues with AmigaOS so > I'd like them to be merged for 8.0 > > @@ -2010,6 +2035,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) > static Property sm501_sysbus_properties[] = { > DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), > DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), > + DEFINE_PROP_BOOL("x-pixman", SM501SysBusState, state.use_pixman, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -2093,6 +2119,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp) > > static Property sm501_pci_properties[] = { > DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB), > + DEFINE_PROP_BOOL("x-pixman", SM501PCIState, state.use_pixman, true), > DEFINE_PROP_END_OF_LIST(), > }; I don't think this should be a user-facing property on a single graphics device. Either pixman works, or it doesn't (in which case we might need to do configure time checks and have a fallback), but we shouldn't make the user have to set an undocumented property on the device to get it to work. thanks -- PMM
On Fri, 24 Feb 2023, Peter Maydell wrote: > On Fri, 24 Feb 2023 at 00:18, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> Pixman can sometimes return false so add fallbacks for such cases and >> also add a property to disable pixman and always use the fallbacks >> which can be useful on platforms where pixman is broken or for testing >> different drawing methods. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> Also ping for the other sm501 patch I've sent a week ago: >> https://patchew.org/QEMU/20230216144043.D632874634B@zero.eik.bme.hu/ >> These two patches are needed to fix graphics issues with AmigaOS so >> I'd like them to be merged for 8.0 >> >> @@ -2010,6 +2035,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) >> static Property sm501_sysbus_properties[] = { >> DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), >> DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), >> + DEFINE_PROP_BOOL("x-pixman", SM501SysBusState, state.use_pixman, true), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -2093,6 +2119,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp) >> >> static Property sm501_pci_properties[] = { >> DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB), >> + DEFINE_PROP_BOOL("x-pixman", SM501PCIState, state.use_pixman, true), >> DEFINE_PROP_END_OF_LIST(), >> }; > > I don't think this should be a user-facing property on a single > graphics device. Either pixman works, or it doesn't (in which > case we might need to do configure time checks and have a > fallback), but we shouldn't make the user have to set an > undocumented property on the device to get it to work. This is mostly for testing now, that's why I've also called it with an x- prefix and not intended as a final solution or something that users would need to use. I'd decide later if we want to keep pixman or replace it with something else (as the problems we found with recently makes me think it might not be such a good idea after all) but for now I need a way to control it and do testing with or without pixman to get some data to help to decide what to do with it. This patch should handle tha cases where pixman returns false (like missing implementation on macOS aarch64) so no need to set this option normally. It's only when I want to easily test if a problem is pixman related, so I can easily set it to fallback or ask users who report a problem to try that. If you think this should be clarified in the commit message can you please suggest a consise sentence to describe this in a way that makes sense in English? Regards, BALATON Zoltan
On Fri, 24 Feb 2023, Peter Maydell wrote: > On Fri, 24 Feb 2023 at 00:18, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> Pixman can sometimes return false so add fallbacks for such cases and >> also add a property to disable pixman and always use the fallbacks >> which can be useful on platforms where pixman is broken or for testing >> different drawing methods. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> Also ping for the other sm501 patch I've sent a week ago: >> https://patchew.org/QEMU/20230216144043.D632874634B@zero.eik.bme.hu/ >> These two patches are needed to fix graphics issues with AmigaOS so >> I'd like them to be merged for 8.0 >> >> @@ -2010,6 +2035,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) >> static Property sm501_sysbus_properties[] = { >> DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), >> DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), >> + DEFINE_PROP_BOOL("x-pixman", SM501SysBusState, state.use_pixman, true), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -2093,6 +2119,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp) >> >> static Property sm501_pci_properties[] = { >> DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB), >> + DEFINE_PROP_BOOL("x-pixman", SM501PCIState, state.use_pixman, true), >> DEFINE_PROP_END_OF_LIST(), >> }; > > I don't think this should be a user-facing property on a single > graphics device. Either pixman works, or it doesn't (in which > case we might need to do configure time checks and have a > fallback), but we shouldn't make the user have to set an > undocumented property on the device to get it to work. Also got some reports now that pixman may have problems with 8 bit depths where it fails without reporting error (so couldn't even be tested by configure) so I'll do a v2 with activating fallback also for 8 bit depths and updated commit message if you can tell me how or any other comments I get today. Regards, BALATON Zoltan
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 58bc9701ee..2a0a9a37f8 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -464,6 +464,7 @@ typedef struct SM501State { uint32_t last_width; uint32_t last_height; bool do_full_update; /* perform a full update next time */ + bool use_pixman; I2CBus *i2c_bus; /* mmio registers */ @@ -691,7 +692,7 @@ static void sm501_2d_operation(SM501State *s) unsigned int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF; int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0; int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt); - bool overlap = false; + bool overlap = false, fallback = false; if ((s->twoD_stretch >> 16) & 0xF) { qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n"); @@ -826,7 +827,7 @@ static void sm501_2d_operation(SM501State *s) de = db + (width + (height - 1) * dst_pitch) * bypp; overlap = (db < se && sb < de); } - if (overlap) { + if (s->use_pixman && overlap) { /* pixman can't do reverse blit: copy via temporary */ int tmp_stride = DIV_ROUND_UP(width * bypp, sizeof(uint32_t)); uint32_t *tmp = tmp_buf; @@ -834,25 +835,43 @@ static void sm501_2d_operation(SM501State *s) if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) { tmp = g_malloc(tmp_stride * sizeof(uint32_t) * height); } - pixman_blt((uint32_t *)&s->local_mem[src_base], tmp, - src_pitch * bypp / sizeof(uint32_t), - tmp_stride, 8 * bypp, 8 * bypp, - src_x, src_y, 0, 0, width, height); - pixman_blt(tmp, (uint32_t *)&s->local_mem[dst_base], - tmp_stride, - dst_pitch * bypp / sizeof(uint32_t), - 8 * bypp, 8 * bypp, - 0, 0, dst_x, dst_y, width, height); + fallback = pixman_blt((uint32_t *)&s->local_mem[src_base], tmp, + src_pitch * bypp / sizeof(uint32_t), + tmp_stride, 8 * bypp, 8 * bypp, + src_x, src_y, 0, 0, width, height); + fallback |= pixman_blt(tmp, (uint32_t *)&s->local_mem[dst_base], + tmp_stride, + dst_pitch * bypp / sizeof(uint32_t), + 8 * bypp, 8 * bypp, 0, 0, dst_x, dst_y, + width, height); if (tmp != tmp_buf) { g_free(tmp); } - } else { - pixman_blt((uint32_t *)&s->local_mem[src_base], - (uint32_t *)&s->local_mem[dst_base], - src_pitch * bypp / sizeof(uint32_t), - dst_pitch * bypp / sizeof(uint32_t), - 8 * bypp, 8 * bypp, - src_x, src_y, dst_x, dst_y, width, height); + } else if (s->use_pixman) { + fallback = pixman_blt((uint32_t *)&s->local_mem[src_base], + (uint32_t *)&s->local_mem[dst_base], + src_pitch * bypp / sizeof(uint32_t), + dst_pitch * bypp / sizeof(uint32_t), + 8 * bypp, 8 * bypp, src_x, src_y, + dst_x, dst_y, width, height); + } + if (!s->use_pixman || fallback) { + uint8_t *sp = s->local_mem + src_base; + uint8_t *d = s->local_mem + dst_base; + unsigned int y, i, j; + for (y = 0; y < height; y++) { + if (overlap) { /* overlap also means rtl */ + i = (dst_y + height - 1 - y) * dst_pitch; + i = (dst_x + i) * bypp; + j = (src_y + height - 1 - y) * src_pitch; + j = (src_x + j) * bypp; + memmove(&d[i], &sp[j], width * bypp); + } else { + i = (dst_x + (dst_y + y) * dst_pitch) * bypp; + j = (src_x + (src_y + y) * src_pitch) * bypp; + memcpy(&d[i], &sp[j], width * bypp); + } + } } } break; @@ -867,13 +886,19 @@ static void sm501_2d_operation(SM501State *s) color = cpu_to_le16(color); } - if (width == 1 && height == 1) { - unsigned int i = (dst_x + dst_y * dst_pitch) * bypp; - stn_he_p(&s->local_mem[dst_base + i], bypp, color); - } else { - pixman_fill((uint32_t *)&s->local_mem[dst_base], - dst_pitch * bypp / sizeof(uint32_t), - 8 * bypp, dst_x, dst_y, width, height, color); + if (!s->use_pixman || (width == 1 && height == 1) || + !pixman_fill((uint32_t *)&s->local_mem[dst_base], + dst_pitch * bypp / sizeof(uint32_t), 8 * bypp, + dst_x, dst_y, width, height, color)) { + /* fallback when pixman failed or we don't want to call it */ + uint8_t *d = s->local_mem + dst_base; + unsigned int x, y, i; + for (y = 0; y < height; y++, i += dst_pitch * bypp) { + i = (dst_x + (dst_y + y) * dst_pitch) * bypp; + for (x = 0; x < width; x++, i += bypp) { + stn_he_p(&d[i], bypp, color); + } + } } break; } @@ -2010,6 +2035,7 @@ static void sm501_realize_sysbus(DeviceState *dev, Error **errp) static Property sm501_sysbus_properties[] = { DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0), DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0), + DEFINE_PROP_BOOL("x-pixman", SM501SysBusState, state.use_pixman, true), DEFINE_PROP_END_OF_LIST(), }; @@ -2093,6 +2119,7 @@ static void sm501_realize_pci(PCIDevice *dev, Error **errp) static Property sm501_pci_properties[] = { DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB), + DEFINE_PROP_BOOL("x-pixman", SM501PCIState, state.use_pixman, true), DEFINE_PROP_END_OF_LIST(), };
Pixman can sometimes return false so add fallbacks for such cases and also add a property to disable pixman and always use the fallbacks which can be useful on platforms where pixman is broken or for testing different drawing methods. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- Also ping for the other sm501 patch I've sent a week ago: https://patchew.org/QEMU/20230216144043.D632874634B@zero.eik.bme.hu/ These two patches are needed to fix graphics issues with AmigaOS so I'd like them to be merged for 8.0 hw/display/sm501.c | 77 +++++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 25 deletions(-)