[PATCH] hw/display/sm501: Add alternatives to pixman routines

[PATCH] hw/display/sm501: Add alternatives to pixman routines

[BALATON Zoltan]

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

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(),
 };
 
-- 
2.30.7

Re: [PATCH] hw/display/sm501: Add alternatives to pixman routines

[Peter Maydell]

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

Re: [PATCH] hw/display/sm501: Add alternatives to pixman routines

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

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

Re: [PATCH] hw/display/sm501: Add alternatives to pixman routines

[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