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

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

[Bilal Elmoussaoui]

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, 25 insertions(+), 7 deletions(-)

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 30917271ab..36548a7f52 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;
+        /* TODO: Add a UpdateList call to send multiple updates at once */
+        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
 
@@ -738,6 +752,7 @@ dbus_display_listener_dispose(GObject *object)
     g_clear_object(&ddl->d3d11_proxy);
     g_clear_pointer(&ddl->peer_process, CloseHandle);
 #ifdef CONFIG_OPENGL
+    pixman_region32_fini(&ddl->gl_damage);
     egl_fb_destroy(&ddl->fb);
 #endif
 #endif
@@ -772,6 +787,9 @@ dbus_display_listener_class_init(DBusDisplayListenerClass *klass)
 static void
 dbus_display_listener_init(DBusDisplayListener *ddl)
 {
+#ifdef CONFIG_OPENGL
+    pixman_region32_init(&ddl->gl_damage);
+#endif
 }
 
 const char *
-- 
2.41.0

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

[Marc-André Lureau]

Hi

On Mon, Aug 14, 2023 at 4:58 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>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

It could be considered as a fix, but I think we can delay it for the
next release. Fine with you?

> ---
>  ui/dbus-listener.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 30917271ab..36548a7f52 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;
> +        /* TODO: Add a UpdateList call to send multiple updates at once */
> +        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
>
> @@ -738,6 +752,7 @@ dbus_display_listener_dispose(GObject *object)
>      g_clear_object(&ddl->d3d11_proxy);
>      g_clear_pointer(&ddl->peer_process, CloseHandle);
>  #ifdef CONFIG_OPENGL
> +    pixman_region32_fini(&ddl->gl_damage);
>      egl_fb_destroy(&ddl->fb);
>  #endif
>  #endif
> @@ -772,6 +787,9 @@ dbus_display_listener_class_init(DBusDisplayListenerClass *klass)
>  static void
>  dbus_display_listener_init(DBusDisplayListener *ddl)
>  {
> +#ifdef CONFIG_OPENGL
> +    pixman_region32_init(&ddl->gl_damage);
> +#endif
>  }
>
>  const char *
> --
> 2.41.0
>

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

[Bilal Elmoussaoui]

[-- Attachment #1: Type: text/plain, Size: 4249 bytes --]

That is fine, it is not something we need urgently as we still need a
kernel patch to make virtio gpu use the KMS atomic API in Mutter. Am I
supposed to send a new patch with the "Reviewed-by" part in the commit
message or?

On Mon, Aug 14, 2023 at 3:02 PM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> Hi
>
> On Mon, Aug 14, 2023 at 4:58 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>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> It could be considered as a fix, but I think we can delay it for the
> next release. Fine with you?
>
> > ---
> >  ui/dbus-listener.c | 32 +++++++++++++++++++++++++-------
> >  1 file changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 30917271ab..36548a7f52 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;
> > +        /* TODO: Add a UpdateList call to send multiple updates at once
> */
> > +        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
> >
> > @@ -738,6 +752,7 @@ dbus_display_listener_dispose(GObject *object)
> >      g_clear_object(&ddl->d3d11_proxy);
> >      g_clear_pointer(&ddl->peer_process, CloseHandle);
> >  #ifdef CONFIG_OPENGL
> > +    pixman_region32_fini(&ddl->gl_damage);
> >      egl_fb_destroy(&ddl->fb);
> >  #endif
> >  #endif
> > @@ -772,6 +787,9 @@
> dbus_display_listener_class_init(DBusDisplayListenerClass *klass)
> >  static void
> >  dbus_display_listener_init(DBusDisplayListener *ddl)
> >  {
> > +#ifdef CONFIG_OPENGL
> > +    pixman_region32_init(&ddl->gl_damage);
> > +#endif
> >  }
> >
> >  const char *
> > --
> > 2.41.0
> >
>
>

[-- Attachment #2: Type: text/html, Size: 5650 bytes --]

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

[Marc-André Lureau]

Hi

On Mon, Aug 14, 2023 at 5:08 PM Bilal Elmoussaoui <belmouss@redhat.com> wrote:
>
> That is fine, it is not something we need urgently as we still need a kernel patch to make virtio gpu use the KMS atomic API in Mutter. Am I supposed to send a new patch with the "Reviewed-by" part in the commit message or?
>

That's not necessary. Either the maintainer will add it, or can pick
it from patchew who does that for us (ex:
https://patchew.org/QEMU/20230814125802.102160-1-belmouss@redhat.com/mbox)

thanks

-- 
Marc-André Lureau