Re: [PATCH v6 12/23] virtio-gpu: replace PIXMAN for region/rect test

[BALATON Zoltan]

On Wed, 25 Oct 2023, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Use a simpler implementation for rectangle geometry & intersect, drop
> the need for (more complex) PIXMAN functions.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/ui/rect.h       | 59 +++++++++++++++++++++++++++++++++++++++++
> hw/display/virtio-gpu.c | 30 ++++++++-------------
> 2 files changed, 70 insertions(+), 19 deletions(-)
> create mode 100644 include/ui/rect.h
>
> diff --git a/include/ui/rect.h b/include/ui/rect.h
> new file mode 100644
> index 0000000000..94898f92d0
> --- /dev/null
> +++ b/include/ui/rect.h
> @@ -0,0 +1,59 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +#ifndef QEMU_RECT_H
> +#define QEMU_RECT_H
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +typedef struct QemuRect {
> +    int16_t x;
> +    int16_t y;
> +    uint16_t width;
> +    uint16_t height;
> +} QemuRect;
> +
> +static inline void qemu_rect_init(QemuRect *rect,
> +                                  int16_t x, int16_t y,
> +                                  uint16_t width, uint16_t height)
> +{
> +    rect->x = x;
> +    rect->y = x;
> +    rect->width = width;
> +    rect->height = height;
> +}
> +
> +static inline void qemu_rect_translate(QemuRect *rect,
> +                                       int16_t dx, int16_t dy)
> +{
> +    rect->x += dx;
> +    rect->y += dy;
> +}
> +
> +static inline bool qemu_rect_intersect(const QemuRect *a, const QemuRect *b,
> +                                       QemuRect *res)
> +{
> +    int16_t x1, x2, y1, y2;
> +
> +    x1 = MAX(a->x, b->x);
> +    y1 = MAX(a->y, b->y);
> +    x2 = MIN(a->x + a->width, b->x + b->width);
> +    y2 = MIN(a->y + a->height, b->y + b->height);
> +
> +    if (x1 >= x2 || y1 >= y2) {
> +        if (res) {
> +            qemu_rect_init(res, 0, 0, 0, 0);
> +        }
> +
> +        return false;
> +    }
> +
> +    if (res) {
> +        qemu_rect_init(res, x1, y1, x2 - x1, y2 - y1);
> +    }
> +
> +    return true;
> +}
> +
> +#endif

Maybe you could move these to pixman-minimal.h as well implementing the 
missing pixman_region funcs so no need to change the callers? But I don't 
mind either way.

Regards,
BALATON Zoltan

> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 4265316cbb..a8e767245f 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -16,6 +16,7 @@
> #include "qemu/iov.h"
> #include "sysemu/cpus.h"
> #include "ui/console.h"
> +#include "ui/rect.h"
> #include "trace.h"
> #include "sysemu/dma.h"
> #include "sysemu/sysemu.h"
> @@ -503,7 +504,7 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>     struct virtio_gpu_simple_resource *res;
>     struct virtio_gpu_resource_flush rf;
>     struct virtio_gpu_scanout *scanout;
> -    pixman_region16_t flush_region;
> +    QemuRect flush_rect;
>     bool within_bounds = false;
>     bool update_submitted = false;
>     int i;
> @@ -565,34 +566,25 @@ static void virtio_gpu_resource_flush(VirtIOGPU *g,
>         return;
>     }
>
> -    pixman_region_init_rect(&flush_region,
> -                            rf.r.x, rf.r.y, rf.r.width, rf.r.height);
> +    qemu_rect_init(&flush_rect, rf.r.x, rf.r.y, rf.r.width, rf.r.height);
>     for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {
> -        pixman_region16_t region, finalregion;
> -        pixman_box16_t *extents;
> +        QemuRect rect;
>
>         if (!(res->scanout_bitmask & (1 << i))) {
>             continue;
>         }
>         scanout = &g->parent_obj.scanout[i];
>
> -        pixman_region_init(&finalregion);
> -        pixman_region_init_rect(&region, scanout->x, scanout->y,
> -                                scanout->width, scanout->height);
> +        qemu_rect_init(&rect, scanout->x, scanout->y,
> +                       scanout->width, scanout->height);
>
> -        pixman_region_intersect(&finalregion, &flush_region, &region);
> -        pixman_region_translate(&finalregion, -scanout->x, -scanout->y);
> -        extents = pixman_region_extents(&finalregion);
>         /* work out the area we need to update for each console */
> -        dpy_gfx_update(g->parent_obj.scanout[i].con,
> -                       extents->x1, extents->y1,
> -                       extents->x2 - extents->x1,
> -                       extents->y2 - extents->y1);
> -
> -        pixman_region_fini(&region);
> -        pixman_region_fini(&finalregion);
> +        if (qemu_rect_intersect(&flush_rect, &rect, &rect)) {
> +            qemu_rect_translate(&rect, -scanout->x, -scanout->y);
> +            dpy_gfx_update(g->parent_obj.scanout[i].con,
> +                           rect.x, rect.y, rect.width, rect.height);
> +        }
>     }
> -    pixman_region_fini(&flush_region);
> }
>
> static void virtio_unref_resource(pixman_image_t *image, void *data)