subject:"\[PATCH\] ui\/dbus\: Implement damage regions for GL"

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

2023-08-14 Thread Bilal Elmoussaoui

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 
> 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 
> > Signed-off-by: Bilal Elmoussaoui 
> > ---
> >  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 
> > +#endif
> >  #ifdef G_OS_UNIX
> >  #include 
> >  #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(>gl_damage);
> > +
> > +for (int i = 0; i < n_rects; i++) {
> > +pixman_box32_t *box;
> > +box = pixman_region32_rectangles(>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(>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(_region, x, y, w, h);
> > +pixman_region32_union(>gl_damage, >gl_damage,
> _region);
> > +pixman_region32_fini(_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(>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

2023-08-14 Thread Marc-André Lureau

Hi

On Mon, Aug 14, 2023 at 4:10 PM Bilal Elmoussaoui  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 
> Signed-off-by: Bilal Elmoussaoui 
> ---
>  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 
> +#endif
>  #ifdef G_OS_UNIX
>  #include 
>  #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(>gl_damage);
> +
> +for (int i = 0; i < n_rects; i++) {
> +pixman_box32_t *box;
> +box = pixman_region32_rectangles(>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(>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(_region, x, y, w, h);
> +pixman_region32_union(>gl_damage, >gl_damage, _region);
> +pixman_region32_fini(_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(>gl_damage);
> +#endif
>  ddl->bus_name = g_strdup(bus_name);
>  ddl->conn = conn;
>  ddl->console = console;
> --
> 2.41.0
>
>

otherwise, lgtm

-- 
Marc-André Lureau

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

2023-08-14 Thread 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 
Signed-off-by: Bilal Elmoussaoui 
---
 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 
+#endif
 #ifdef G_OS_UNIX
 #include 
 #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(>gl_damage);
+
+for (int i = 0; i < n_rects; i++) {
+pixman_box32_t *box;
+box = pixman_region32_rectangles(>gl_damage, NULL) + i;
+
+dbus_call_update_gl(dcl, box->x1, box->y1,
+box->x2 - box->x1, box->y2 - box->y1);
 }
+pixman_region32_clear(>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(_region, x, y, w, h);
+pixman_region32_union(>gl_damage, >gl_damage, _region);
+pixman_region32_fini(_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(>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

2023-05-09 Thread Philippe Mathieu-Daudé

On 9/5/23 17:04, Bilal Elmoussaoui wrote:

cairo declared as optional dep, ...

I don't see where cairo is marked as an optional dependency, the `cairo 
= not_found` part could probably be dropped to make it clearer.

I was expecting:

.require(cairo.found(), error_message: '-display dbus requires cairo')

On Tue, May 9, 2023 at 4:34 PM Philippe Mathieu-Daudé > wrote:

Hi,

On 9/5/23 13:59, Bilal Elmoussaoui wrote:
 > From: Christian Hergert mailto:cherg...@redhat.com>>

 > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
 > index 911acdc529..047be5cb3a 100644
 > --- a/ui/dbus-listener.c
 > +++ b/ui/dbus-listener.c
 > @@ -25,6 +25,7 @@
 >   #include "qemu/error-report.h"
 >   #include "sysemu/sysemu.h"
 >   #include "dbus.h"
 > +#include 

cairo used unconditionally.

Shouldn't we now declared it as a strict dependency in meson?

 >   #include 
 >
 >   #ifdef CONFIG_OPENGL

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

2023-05-09 Thread Bilal Elmoussaoui

>
> cairo declared as optional dep, ...
>

I don't see where cairo is marked as an optional dependency, the `cairo =
not_found` part could probably be dropped to make it clearer.

On Tue, May 9, 2023 at 4:34 PM Philippe Mathieu-Daudé 
wrote:

> Hi,
>
> On 9/5/23 13:59, Bilal Elmoussaoui wrote:
> > From: Christian Hergert 
> >
> > 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.
> >
> > In the future, when additional methods are allowed on the D-Bus
> > interface,
> > this should likely be updated to send damage regions as a single RPC to
> > avoid additional message processing.
> >
> > Currently, Linux does not propagate damage rectangles when using the
> > virtio-gpu drm driver. That means compositors such as Mutter which
> > utilize
> > drmModePageFlip() will be sending full or near-full surface damages.
> >
> > https://lists.freedesktop.org/archives/dri-devel/2023-March/395164.html
> > contains a patch to fix that too.
> >
> > Signed-off-by: Bilal Elmoussaoui 
> > ---
> >   meson.build|  8 
> >   ui/dbus-listener.c | 25 +++--
> >   ui/meson.build |  2 +-
> >   3 files changed, 28 insertions(+), 7 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 229eb585f7..72678ef78e 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -1761,6 +1761,14 @@ dbus_display = get_option('dbus_display') \
> >  error_message: '-display dbus is not available on Windows')
> \
> > .allowed()
>
> ^ dbus strictly required deps, ...
>
> > +cairo = not_found
> > +if dbus_display
> > +  cairo = dependency('cairo',
> > + kwargs: static_kwargs,
> > + method: 'pkg-config',
> > +)
>
> cairo declared as optional dep, ...
>
> > +endif
> > +
> >   have_virtfs = get_option('virtfs') \
> >   .require(targetos == 'linux' or targetos == 'darwin',
> >error_message: 'virtio-9p (virtfs) requires Linux or
> macOS') \
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 911acdc529..047be5cb3a 100644
> > --- a/ui/dbus-listener.c
> > +++ b/ui/dbus-listener.c
> > @@ -25,6 +25,7 @@
> >   #include "qemu/error-report.h"
> >   #include "sysemu/sysemu.h"
> >   #include "dbus.h"
> > +#include 
>
> cairo used unconditionally.
>
> Shouldn't we now declared it as a strict dependency in meson?
>
> >   #include 
> >
> >   #ifdef CONFIG_OPENGL
>
>

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

2023-05-09 Thread Philippe Mathieu-Daudé


Hi,

On 9/5/23 13:59, Bilal Elmoussaoui wrote:

From: Christian Hergert 

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.

In the future, when additional methods are allowed on the D-Bus
interface,
this should likely be updated to send damage regions as a single RPC to
avoid additional message processing.

Currently, Linux does not propagate damage rectangles when using the
virtio-gpu drm driver. That means compositors such as Mutter which
utilize
drmModePageFlip() will be sending full or near-full surface damages.

https://lists.freedesktop.org/archives/dri-devel/2023-March/395164.html
contains a patch to fix that too.

Signed-off-by: Bilal Elmoussaoui 
---
  meson.build|  8 
  ui/dbus-listener.c | 25 +++--
  ui/meson.build |  2 +-
  3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index 229eb585f7..72678ef78e 100644
--- a/meson.build
+++ b/meson.build
@@ -1761,6 +1761,14 @@ dbus_display = get_option('dbus_display') \
 error_message: '-display dbus is not available on Windows') \
.allowed()


^ dbus strictly required deps, ...


+cairo = not_found
+if dbus_display
+  cairo = dependency('cairo',
+ kwargs: static_kwargs,
+ method: 'pkg-config',
+)


cairo declared as optional dep, ...


+endif
+
  have_virtfs = get_option('virtfs') \
  .require(targetos == 'linux' or targetos == 'darwin',
   error_message: 'virtio-9p (virtfs) requires Linux or macOS') \
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 911acdc529..047be5cb3a 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -25,6 +25,7 @@
  #include "qemu/error-report.h"
  #include "sysemu/sysemu.h"
  #include "dbus.h"
+#include 


cairo used unconditionally.

Shouldn't we now declared it as a strict dependency in meson?


  #include 
  
  #ifdef CONFIG_OPENGL

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

2023-05-09 Thread Bilal Elmoussaoui

From: Christian Hergert 

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.

In the future, when additional methods are allowed on the D-Bus
interface,
this should likely be updated to send damage regions as a single RPC to
avoid additional message processing.

Currently, Linux does not propagate damage rectangles when using the
virtio-gpu drm driver. That means compositors such as Mutter which
utilize
drmModePageFlip() will be sending full or near-full surface damages.

https://lists.freedesktop.org/archives/dri-devel/2023-March/395164.html
contains a patch to fix that too.

Signed-off-by: Bilal Elmoussaoui 
---
 meson.build|  8 
 ui/dbus-listener.c | 25 +++--
 ui/meson.build |  2 +-
 3 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/meson.build b/meson.build
index 229eb585f7..72678ef78e 100644
--- a/meson.build
+++ b/meson.build
@@ -1761,6 +1761,14 @@ dbus_display = get_option('dbus_display') \
error_message: '-display dbus is not available on Windows') \
   .allowed()
 
+cairo = not_found
+if dbus_display
+  cairo = dependency('cairo',
+ kwargs: static_kwargs,
+ method: 'pkg-config',
+)
+endif
+
 have_virtfs = get_option('virtfs') \
 .require(targetos == 'linux' or targetos == 'darwin',
  error_message: 'virtio-9p (virtfs) requires Linux or macOS') \
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 911acdc529..047be5cb3a 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -25,6 +25,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "dbus.h"
+#include 
 #include 
 
 #ifdef CONFIG_OPENGL
@@ -43,9 +44,10 @@ struct _DBusDisplayListener {
 
 QemuDBusDisplay1Listener *proxy;
 
+cairo_region_t *gl_damage;
+
 DisplayChangeListener dcl;
 DisplaySurface *ds;
-int gl_updates;
 };
 
 G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
@@ -226,10 +228,17 @@ static void dbus_gl_refresh(DisplayChangeListener *dcl)
 return;
 }
 
-if (ddl->gl_updates) {
-dbus_call_update_gl(ddl, 0, 0,
-surface_width(ddl->ds), surface_height(ddl->ds));
-ddl->gl_updates = 0;
+if (ddl->gl_damage) {
+int n_rects = cairo_region_num_rectangles(ddl->gl_damage);
+
+for (int i = 0; i < n_rects; i++) {
+cairo_rectangle_int_t rect;
+
+cairo_region_get_rectangle(ddl->gl_damage, i, );
+dbus_call_update_gl(ddl, rect.x, rect.y, rect.width, rect.height);
+}
+
+g_clear_pointer(>gl_damage, cairo_region_destroy);
 }
 }
 #endif
@@ -245,7 +254,11 @@ static void dbus_gl_gfx_update(DisplayChangeListener *dcl,
 {
 DBusDisplayListener *ddl = container_of(dcl, DBusDisplayListener, dcl);
 
-ddl->gl_updates++;
+if (ddl->gl_damage == NULL) {
+ddl->gl_damage = cairo_region_create();
+}
+cairo_region_union_rectangle(ddl->gl_damage,
+ &(cairo_rectangle_int_t) {x, y, w, h});
 }
 #endif
 
diff --git a/ui/meson.build b/ui/meson.build
index 330369707d..8b4004ff01 100644
--- a/ui/meson.build
+++ b/ui/meson.build
@@ -85,7 +85,7 @@ if dbus_display
   '--generate-c-code', '@BASENAME@'])
   dbus_display1_lib = static_library('dbus-display1', dbus_display1, 
dependencies: gio)
   dbus_display1_dep = declare_dependency(link_with: dbus_display1_lib, 
include_directories: include_directories('.'))
-  dbus_ss.add(when: [gio, pixman, dbus_display1_dep],
+  dbus_ss.add(when: [gio, pixman, dbus_display1_dep, cairo],
   if_true: [files(
 'dbus-chardev.c',
 'dbus-clipboard.c',
-- 
2.40.0

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

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

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

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

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

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

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

7 matches

Site Navigation

Mail list logo

Footer information