Hi
On Tue, Oct 10, 2023 at 2:09 PM BALATON Zoltan <bala...@eik.bme.hu> wrote:
>
> On Tue, 10 Oct 2023, Marc-André Lureau wrote:
> > On Tue, Oct 10, 2023 at 1:53 PM BALATON Zoltan <bala...@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 <bala...@eik.bme.hu> wrote:
> >>>>
> >>>> On Mon, 18 Sep 2023, marcandre.lur...@redhat.com wrote:
> >>>>> From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >>>>>
> >>>>> Drop the "x-pixman" property and use fallback path in such case.
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@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
--
Marc-André Lureau
