Re: [PATCH v5 1/7] hw/display/sm501: Add debug property to control pixman usage

[BALATON Zoltan]

On Wed, 1 Mar 2023, BALATON Zoltan wrote:
> Add a property to allow disabling pixman and always use the fallbacks
> for different operations which is useful for testing different drawing
> methods or debugging pixman related issues.

Peter,

It was verified that the already merged patches fixed the problems with 
pixman on macOS by adding the fallback so that does not need this property 
but it is still useful for debugging or testing. Are you OK with adding it 
as a debug property as described in this version or still have a concern 
about this?

Regards,
BALATON Zoltan

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 17835159fc..dbabbc4339 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -465,6 +465,7 @@ typedef struct SM501State {
>     uint32_t last_width;
>     uint32_t last_height;
>     bool do_full_update; /* perform a full update next time */
> +    uint8_t use_pixman;
>     I2CBus *i2c_bus;
>
>     /* mmio registers */
> @@ -827,7 +828,7 @@ static void sm501_2d_operation(SM501State *s)
>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>                 overlap = (db < se && sb < de);
>             }
> -            if (overlap) {
> +            if (overlap && (s->use_pixman & BIT(2))) {
>                 /* 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;
> @@ -852,13 +853,15 @@ static void sm501_2d_operation(SM501State *s)
>                 if (tmp != tmp_buf) {
>                     g_free(tmp);
>                 }
> -            } else {
> +            } else if (!overlap && (s->use_pixman & BIT(1))) {
>                 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);
> +            } else {
> +                fallback = true;
>             }
>             if (fallback) {
>                 uint8_t *sp = s->local_mem + src_base;
> @@ -891,7 +894,7 @@ static void sm501_2d_operation(SM501State *s)
>             color = cpu_to_le16(color);
>         }
>
> -        if ((width == 1 && height == 1) ||
> +        if (!(s->use_pixman & BIT(0)) || (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)) {
> @@ -2035,6 +2038,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_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2122,6 +2126,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_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2162,11 +2167,18 @@ static void sm501_pci_class_init(ObjectClass *klass, 
> void *data)
>     dc->vmsd = &vmstate_sm501_pci;
> }
>
> +static void sm501_pci_init(Object *o)
> +{
> +    object_property_set_description(o, "x-pixman", "Use pixman for: "
> +                                    "1: fill, 2: blit, 4: overlap blit");
> +}
> +
> static const TypeInfo sm501_pci_info = {
>     .name          = TYPE_PCI_SM501,
>     .parent        = TYPE_PCI_DEVICE,
>     .instance_size = sizeof(SM501PCIState),
>     .class_init    = sm501_pci_class_init,
> +    .instance_init = sm501_pci_init,
>     .interfaces = (InterfaceInfo[]) {
>         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>         { },