1. Patchew
  2. QEMU
  3. View series

[PATCH] ui/dbus: implement damage regions for GL

Bilal Elmoussaoui posted 1 patch 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230814120739.89213-1-belmouss@redhat.com
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>
There is a newer version of this series
ui/dbus-listener.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
[PATCH] ui/dbus: implement damage regions for GL
Posted by Bilal Elmoussaoui 9 months ago 
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 <chergert@redhat.com>
Signed-off-by: Bilal Elmoussaoui <belmouss@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
+
     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);
     }
+    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
Re: [PATCH] ui/dbus: implement damage regions for GL
Posted by Marc-André Lureau 9 months ago 
Hi

On Mon, Aug 14, 2023 at 4:10 PM Bilal Elmoussaoui <belmouss@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 <chergert@redhat.com>
> Signed-off-by: Bilal Elmoussaoui <belmouss@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
Re: [PATCH] ui/dbus: implement damage regions for GL
Posted by Bilal Elmoussaoui 9 months ago 
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.lureau@gmail.com> wrote:

> Hi
>
> On Mon, Aug 14, 2023 at 4:10 PM Bilal Elmoussaoui <belmouss@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 <chergert@redhat.com>
> > Signed-off-by: Bilal Elmoussaoui <belmouss@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
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.