Re: [PATCH v2 16/18] hw/sm501: allow compiling without PIXMAN

[BALATON Zoltan]

On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> Hi Zoltan
>
> On Mon, Sep 18, 2023 at 9:59 PM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> > On Mon, 18 Sep 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.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > > hw/display/sm501.c | 19 ++++++++++++++++---
> > > 1 file changed, 16 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> > > index 0eecd00701..a897c82f04 100644
> > > --- a/hw/display/sm501.c
> > > +++ b/hw/display/sm501.c
> > > @@ -730,7 +730,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 +827,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 +861,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,10 +898,13 @@ 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)) {
> > > +                         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;
> > > @@ -2038,7 +2045,9 @@ static void sm501_realize_sysbus(DeviceState *dev, Error 
> > > **errp)
> > >
> > > static Property sm501_sysbus_properties[] = {
> > >     DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
> > > +#ifdef CONFIG_PIXMAN
> > >     DEFINE_PROP_UINT8("x-pixman", SM501SysBusState, state.use_pixman, 7),
> > > +#endif
> >
> > Do we want to omit the property when compiled without pixman? I think we
> > could leave it there and it would just be ignored without pixman but the
> > same command line would still work and need less ifdefs in code.
>
> That's a reasonable idea to me. At least, it can handle x-pixman=0
> fine when !CONFIG_PIXMAN then.
>
> Btw, looking at it, it seems it should be DEFINE_PROP_BIT instead. I
> will add a TODO :)

Erm, a bit can be 1 or 0 but the default value of it is 7. It's not a 
single but but a bitmask the enable/disable pisman for different 
operations separately so I think it can't be _BIT.

Regards,
BALATON Zoltan

> > Otherwise:
> >
> > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > Regards,
> > BALATON Zoltan
> >
> > >     DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > @@ -2126,7 +2135,9 @@ static void sm501_realize_pci(PCIDevice *dev, Error 
> > > **errp)
> > >
> > > static Property sm501_pci_properties[] = {
> > >     DEFINE_PROP_UINT32("vram-size", SM501PCIState, vram_size, 64 * MiB),
> > > +#ifdef CONFIG_PIXMAN
> > >     DEFINE_PROP_UINT8("x-pixman", SM501PCIState, state.use_pixman, 7),
> > > +#endif
> > >     DEFINE_PROP_END_OF_LIST(),
> > > };
> > >
> > > @@ -2169,8 +2180,10 @@ static void sm501_pci_class_init(ObjectClass *klass, 
> > > void *data)
> > >
> > > static void sm501_pci_init(Object *o)
> > > {
> > > +#ifdef CONFIG_PIXMAN
> > >     object_property_set_description(o, "x-pixman", "Use pixman for: "
> > >                                     "1: fill, 2: blit, 4: overlap blit");
> > > +#endif
> > > }
> > >
> > > static const TypeInfo sm501_pci_info = {
>
>
>
> --
> Marc-André Lureau