Re: [PULL 092/126] hw/display/sm501: Add fallbacks to pixman routines

[BALATON Zoltan]

On Tue, 4 Apr 2023, Peter Maydell wrote:
> On Mon, 27 Feb 2023 at 14:38, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > From: BALATON Zoltan <balaton@eik.bme.hu>
> >
> > Pixman may return false if it does not have a suitable implementation.
> > Add fallbacks to handle such cases.
> >
> > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > Reported-by: Rene Engel <ReneEngel80@emailn.de>
> > Tested-by: Rene Engel <ReneEngel80@emailn.de>
> > Message-Id: 
> > <20ed9442a0146238254ccc340c0d1efa226c6356.1677445307.git.balaton@eik.bme.hu>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Hi; Coverity points out what may be a bug in this code
> (CID 1508621):
>
> > @@ -868,13 +891,19 @@ static void sm501_2d_operation(SM501State *s)
> >              color = cpu_to_le16(color);
> >          }
> >
> > -        if (width == 1 && height == 1) {
> > -            unsigned int i = (dst_x + dst_y * dst_pitch) * bypp;
> > -            stn_he_p(&s->local_mem[dst_base + i], bypp, color);
> > -        } else {
> > -            pixman_fill((uint32_t *)&s->local_mem[dst_base],
> > -                        dst_pitch * bypp / sizeof(uint32_t),
> > -                        8 * bypp, dst_x, dst_y, width, height, color);
> > +        if ((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_pitch * bypp) {
>
> In this loop, the on-every-iteration expression updates i...
>
> > +                i = (dst_x + (dst_y + y) * dst_pitch) * bypp;
>
> ...but the first statement in the loop unconditionally sets i,
> so whatever value the loop iteration expression calculated is ignored.
>
> Should the iteration expression just be deleted, or should the
> statement in the loop be updating i rather than just setting it?

In a previous version I've tried to update i from the loop but that did 
not work too well so I've reverted to setting i at every iteration which 
is also how this is done elsewhere already. So the increment in the for 
line can be dropped, I've just forgot that there apparently. Should I send 
a patch or you can do it?

Regards,
BALATON Zoltan

> > +                for (x = 0; x < width; x++, i += bypp) {
> > +                    stn_he_p(&d[i], bypp, color);
> > +                }
> > +            }
>
> thanks
> -- PMM