| Message ID | 20230918135206.2739222-17-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make Pixman an optional dependency | expand |
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. 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 = { >
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 :) > > 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
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 > >
Hi On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > 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. Sure, but we could have more explicitly different BIT properties ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit"). thanks
On Tue, 10 Oct 2023, Marc-André Lureau wrote: > On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >> 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. > > Sure, but we could have more explicitly different BIT properties > ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit"). That was also proposed when I've added it and concluded that we don't want that. This is a debug option for experts and adding a lot of experimental options for that that are also harder to type is not an improvement so having just a value is fine. Regards, BALATON Zoltan
Hi On Tue, Oct 10, 2023 at 2:09 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Tue, 10 Oct 2023, Marc-André Lureau wrote: > > On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > >> 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. > > > > Sure, but we could have more explicitly different BIT properties > > ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit"). > > That was also proposed when I've added it and concluded that we don't want > that. This is a debug option for experts and adding a lot of experimental > options for that that are also harder to type is not an improvement so > having just a value is fine. Ok, I'll change the comment to: /* this a debug option, prefer UINT over PROP_BIT for simplicity */ r-b with that? thanks
On Tue, 10 Oct 2023, Marc-André Lureau wrote: > Hi > > On Tue, Oct 10, 2023 at 2:09 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >> >> On Tue, 10 Oct 2023, Marc-André Lureau wrote: >>> On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: >>>> 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. >>> >>> Sure, but we could have more explicitly different BIT properties >>> ("x-pixman-fill", "x-pixman-blit", "x-pixman-overlap-blit"). >> >> That was also proposed when I've added it and concluded that we don't want >> that. This is a debug option for experts and adding a lot of experimental >> options for that that are also harder to type is not an improvement so >> having just a value is fine. > > Ok, I'll change the comment to: > /* this a debug option, prefer UINT over PROP_BIT for simplicity */ > > r-b with that? I don't think it needs a comment but I don't mind if you add one. I'll review the latest version later and if you add a comment that won't change it. Regards, BALATON Zoltan
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 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 = {