Re: [PATCH v5 17/19] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

On Mon, 23 Oct 2023, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Drop the "x-pixman" property and use fallback path in such case.

Patch diverged from commit message, the x-pixman property is not dropped 
just the default changed. Needs updated commit message to reflect that.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/display/sm501.c | 59 +++++++++++++++++++++++++++++++++-------------
> 1 file changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0eecd00701..282671d1b3 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -438,6 +438,12 @@
> #define SM501_HWC_WIDTH                 64
> #define SM501_HWC_HEIGHT                64
>
> +#ifdef CONFIG_PIXMAN
> +#define DEFAULT_X_PIXMAN 7
> +#else
> +#define DEFAULT_X_PIXMAN 0
> +#endif
> +
> /* SM501 local memory size taken from "linux/drivers/mfd/sm501.c" */
> static const uint32_t sm501_mem_local_size[] = {
>     [0] = 4 * MiB,
> @@ -730,7 +736,6 @@ static void sm501_2d_operation(SM501State *s)
>     switch (cmd) {
>     case 0: /* BitBlt */
>     {
> -        static uint32_t tmp_buf[16384];
>         unsigned int src_x = (s->twoD_source >> 16) & 0x01FFF;
>         unsigned int src_y = s->twoD_source & 0xFFFF;
>         uint32_t src_base = s->twoD_source_base & 0x03FFFFFF;
> @@ -828,9 +833,11 @@ static void sm501_2d_operation(SM501State *s)
>                 de = db + (width + (height - 1) * dst_pitch) * bypp;
>                 overlap = (db < se && sb < de);
>             }
> +#ifdef CONFIG_PIXMAN
>             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));
> +                static uint32_t tmp_buf[16384];
>                 uint32_t *tmp = tmp_buf;
>
>                 if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) {
> @@ -860,9 +867,12 @@ static void sm501_2d_operation(SM501State *s)
>                                        dst_pitch * bypp / sizeof(uint32_t),
>                                        8 * bypp, 8 * bypp, src_x, src_y,
>                                        dst_x, dst_y, width, height);
> -            } else {
> +            } else
> +#else
> +            {
>                 fallback = true;
>             }
> +#endif
>             if (fallback) {
>                 uint8_t *sp = s->local_mem + src_base;
>                 uint8_t *d = s->local_mem + dst_base;
> @@ -894,20 +904,23 @@ static void sm501_2d_operation(SM501State *s)
>             color = cpu_to_le16(color);
>         }
>
> +#ifdef CONFIG_PIXMAN
>         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)) {
> -            /* 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_x + (dst_y + y) * dst_pitch) * bypp;
> -                for (x = 0; x < width; x++, i += bypp) {
> -                    stn_he_p(&d[i], bypp, color);
> +                         dst_x, dst_y, width, height, color))
> +#endif
> +            {
> +                /* 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_x + (dst_y + y) * dst_pitch) * bypp;
> +                    for (x = 0; x < width; x++, i += bypp) {
> +                        stn_he_p(&d[i], bypp, color);
> +                    }
>                 }
>             }
> -        }
>         break;
>     }
>     default:
> @@ -1875,9 +1888,15 @@ static void sm501_reset(SM501State *s)
>     s->twoD_wrap = 0;
> }
>
> -static void sm501_init(SM501State *s, DeviceState *dev,
> -                       uint32_t local_mem_bytes)
> +static bool sm501_init(SM501State *s, DeviceState *dev,
> +                       uint32_t local_mem_bytes, Error **errp)
> {
> +#ifndef CONFIG_PIXMAN
> +    if (s->use_pixman != 0) {
> +        error_setg(&error_warn, "x-pixman != 0, not effective without PIXMAN");

I think this should just be warn_report not an error

> +    }
> +#endif
> +
>     s->local_mem_size_index = get_local_mem_size_index(local_mem_bytes);
>
>     /* local memory */
> @@ -1916,6 +1935,7 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>
>     /* create qemu graphic console */
>     s->con = graphic_console_init(dev, 0, &sm501_ops, s);
> +    return true;

and no need to add errp as we never return false so it won't be printed 
anyway.

> }
>
> static const VMStateDescription vmstate_sm501_state = {
> @@ -2014,7 +2034,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error 
> **errp)
>     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>     MemoryRegion *mr;
>
> -    sm501_init(&s->state, dev, s->vram_size);
> +    if (!sm501_init(&s->state, dev, s->vram_size, errp)) {
> +        return;
> +    }

This could be left alone too, just issuing a warning above should be 
enough, no other cahnges are needed.

Regards,
BALATON Zoltan

>     if (get_local_mem_size(&s->state) != s->vram_size) {
>         error_setg(errp, "Invalid VRAM size, nearest valid size is %" PRIu32,
>                    get_local_mem_size(&s->state));
> @@ -2038,7 +2060,8 @@ 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),
> +    /* this a debug option, prefer PROP_UINT over PROP_BIT for simplicity */
> +    DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 
> DEFAULT_X_PIXMAN),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2112,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error 
> **errp)
> {
>     SM501PCIState *s = PCI_SM501(dev);
>
> -    sm501_init(&s->state, DEVICE(dev), s->vram_size);
> +    if (!sm501_init(&s->state, DEVICE(dev), s->vram_size, errp)) {
> +        return;
> +    }
>     if (get_local_mem_size(&s->state) != s->vram_size) {
>         error_setg(errp, "Invalid VRAM size, nearest valid size is %" PRIu32,
>                    get_local_mem_size(&s->state));
> @@ -2126,7 +2151,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_UINT8("x-pixman", SM501PCIState, state.use_pixman, 
> DEFAULT_X_PIXMAN),
>     DEFINE_PROP_END_OF_LIST(),
> };