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

[Marc-André Lureau]

Hi

On Thu, Oct 26, 2023 at 12:55 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> 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.
>

The pixman region functions are more complex than what we actually
need in QEMU (simple rectangle geometry).

> 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)
> >



-- 
Marc-André Lureau