Thanks for the quick review! I have sent a v2 with the requested changes.
On Mon, Aug 14, 2023 at 2:41 PM Marc-André Lureau <
marcandre.lur...@gmail.com> wrote:
> Hi
>
> On Mon, Aug 14, 2023 at 4:10 PM Bilal Elmoussaoui <belmo...@redhat.com>
> wrote:
> >
> > Currently, when using `-display dbus,gl=on` all updates to the client
> > become "full scanout" updates, meaning there is no way for the client to
> > limit damage regions to the display server.
> >
> > Instead of using an "update count", this patch tracks the damage region
> > and propagates it to the client.
> >
> > This was less of an issue when clients were using GtkGLArea for
> > rendering,
> > as you'd be doing full-surface redraw. To be efficient, the client needs
> > both a DMA-BUF and the damage region to be updated.
> >
> > Co-authored-by: Christian Hergert <cherg...@redhat.com>
> > Signed-off-by: Bilal Elmoussaoui <belmo...@redhat.com>
> > ---
> > ui/dbus-listener.c | 32 ++++++++++++++++++++++++--------
> > 1 file changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 30917271ab..d015e8d759 100644
> > --- a/ui/dbus-listener.c
> > +++ b/ui/dbus-listener.c
> > @@ -26,6 +26,9 @@
> > #include "qapi/error.h"
> > #include "sysemu/sysemu.h"
> > #include "dbus.h"
> > +#ifdef CONFIG_OPENGL
> > +#include <pixman.h>
> > +#endif
> > #ifdef G_OS_UNIX
> > #include <gio/gunixfdlist.h>
> > #endif
> > @@ -59,12 +62,15 @@ struct _DBusDisplayListener {
> >
> > QemuDBusDisplay1Listener *proxy;
> >
> > +#ifdef CONFIG_OPENGL
> > + /* Keep track of the damage region */
> > + pixman_region32_t gl_damage;
> > +#endif
>
> I think it should call pixman_region32_init() in
> dbus_display_listener_new(), & _fini() in _dispose()
>
> > +
> > DisplayChangeListener dcl;
> > DisplaySurface *ds;
> > enum share_kind ds_share;
> >
> > - int gl_updates;
> > -
> > bool ds_mapped;
> > bool can_share_map;
> >
> > @@ -539,11 +545,16 @@ static void dbus_gl_refresh(DisplayChangeListener
> *dcl)
> > return;
> > }
> >
> > - if (ddl->gl_updates) {
> > - dbus_call_update_gl(dcl, 0, 0,
> > - surface_width(ddl->ds),
> surface_height(ddl->ds));
> > - ddl->gl_updates = 0;
> > + int n_rects = pixman_region32_n_rects(&ddl->gl_damage);
> > +
> > + for (int i = 0; i < n_rects; i++) {
> > + pixman_box32_t *box;
> > + box = pixman_region32_rectangles(&ddl->gl_damage, NULL) + i;
> > +
> > + dbus_call_update_gl(dcl, box->x1, box->y1,
> > + box->x2 - box->x1, box->y2 - box->y1);
>
> May be worth to add a "TODO: add Update*List methods" ?
>
> > }
> > + pixman_region32_clear(&ddl->gl_damage);
> > }
> > #endif /* OPENGL */
> >
> > @@ -558,7 +569,10 @@ static void
> dbus_gl_gfx_update(DisplayChangeListener *dcl,
> > {
> > DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener,
> dcl);
> >
> > - ddl->gl_updates++;
> > + pixman_region32_t rect_region;
> > + pixman_region32_init_rect(&rect_region, x, y, w, h);
> > + pixman_region32_union(&ddl->gl_damage, &ddl->gl_damage,
> &rect_region);
> > + pixman_region32_fini(&rect_region);
> > }
> > #endif
> >
> > @@ -933,7 +947,9 @@ dbus_display_listener_new(const char *bus_name,
> > g_object_unref(ddl);
> > return NULL;
> > }
> > -
> > +#ifdef CONFIG_OPENGL
> > + pixman_region32_init(&ddl->gl_damage);
> > +#endif
> > ddl->bus_name = g_strdup(bus_name);
> > ddl->conn = conn;
> > ddl->console = console;
> > --
> > 2.41.0
> >
> >
>
> otherwise, lgtm
>
> --
> Marc-André Lureau
>
>
