| Message ID | 20230227140213.35084-83-philmd@linaro.org |
|---|---|
| State | Accepted |
| Commit | c09b5158e17f7112678a9d80e13419195e45dba8 |
| Headers | show |
| Series | None | expand |
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? > + for (x = 0; x < width; x++, i += bypp) { > + stn_he_p(&d[i], bypp, color); > + } > + } thanks -- PMM
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 > >
On Tue, 4 Apr 2023 at 20:26, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Tue, 4 Apr 2023, Peter Maydell wrote: > > 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? If you could send a patch that would be helpful -- I don't have a setup handy to test this beyond "did it compile". This isn't an urgent thing to fix. -- PMM
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index c4c567d977..17835159fc 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -692,7 +692,7 @@ static void sm501_2d_operation(SM501State *s) unsigned int dst_pitch = (s->twoD_pitch >> 16) & 0x1FFF; int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0; int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt); - bool overlap = false; + bool overlap = false, fallback = false; if ((s->twoD_stretch >> 16) & 0xF) { qemu_log_mask(LOG_UNIMP, "sm501: only XY addressing is supported.\n"); @@ -835,25 +835,48 @@ static void sm501_2d_operation(SM501State *s) if (tmp_stride * sizeof(uint32_t) * height > sizeof(tmp_buf)) { tmp = g_malloc(tmp_stride * sizeof(uint32_t) * height); } - pixman_blt((uint32_t *)&s->local_mem[src_base], tmp, - src_pitch * bypp / sizeof(uint32_t), - tmp_stride, 8 * bypp, 8 * bypp, - src_x, src_y, 0, 0, width, height); - pixman_blt(tmp, (uint32_t *)&s->local_mem[dst_base], - tmp_stride, - dst_pitch * bypp / sizeof(uint32_t), - 8 * bypp, 8 * bypp, - 0, 0, dst_x, dst_y, width, height); + fallback = !pixman_blt((uint32_t *)&s->local_mem[src_base], + tmp, + src_pitch * bypp / sizeof(uint32_t), + tmp_stride, + 8 * bypp, 8 * bypp, + src_x, src_y, 0, 0, width, height); + if (!fallback) { + fallback = !pixman_blt(tmp, + (uint32_t *)&s->local_mem[dst_base], + tmp_stride, + dst_pitch * bypp / sizeof(uint32_t), + 8 * bypp, 8 * bypp, + 0, 0, dst_x, dst_y, width, height); + } if (tmp != tmp_buf) { g_free(tmp); } } else { - pixman_blt((uint32_t *)&s->local_mem[src_base], - (uint32_t *)&s->local_mem[dst_base], - src_pitch * bypp / sizeof(uint32_t), - dst_pitch * bypp / sizeof(uint32_t), - 8 * bypp, 8 * bypp, - src_x, src_y, dst_x, dst_y, width, height); + fallback = !pixman_blt((uint32_t *)&s->local_mem[src_base], + (uint32_t *)&s->local_mem[dst_base], + src_pitch * bypp / sizeof(uint32_t), + dst_pitch * bypp / sizeof(uint32_t), + 8 * bypp, 8 * bypp, src_x, src_y, + dst_x, dst_y, width, height); + } + if (fallback) { + uint8_t *sp = s->local_mem + src_base; + uint8_t *d = s->local_mem + dst_base; + unsigned int y, i, j; + for (y = 0; y < height; y++) { + if (overlap) { /* overlap also means rtl */ + i = (dst_y + height - 1 - y) * dst_pitch; + i = (dst_x + i) * bypp; + j = (src_y + height - 1 - y) * src_pitch; + j = (src_x + j) * bypp; + memmove(&d[i], &sp[j], width * bypp); + } else { + i = (dst_x + (dst_y + y) * dst_pitch) * bypp; + j = (src_x + (src_y + y) * src_pitch) * bypp; + memcpy(&d[i], &sp[j], width * bypp); + } + } } } break; @@ -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) { + 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; }