diff mbox series

[05/16] ui/dbus: fix filtering all update messages

Message ID 20241003112244.3340697-6-marcandre.lureau@redhat.com (mailing list archive)
State New, archived
Headers show
Series UI-related fixes & shareable 2d memory with -display dbus | expand

Commit Message

Marc-André Lureau Oct. 3, 2024, 11:22 a.m. UTC 
From: Marc-André Lureau <marcandre.lureau@redhat.com>

Filtering pending messages when a new scanout is given shouldn't discard
pending cursor changes, for example.

Since filtering happens in a different thread, use atomic set/get.

Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

Comments

Akihiko Odaki Oct. 5, 2024, 8:44 a.m. UTC | #1 
On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Filtering pending messages when a new scanout is given shouldn't discard
> pending cursor changes, for example.
> 
> Since filtering happens in a different thread, use atomic set/get.
> 
> Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
>   1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> index 434bd608f2..a70cad3a90 100644
> --- a/ui/dbus-listener.c
> +++ b/ui/dbus-listener.c
> @@ -26,6 +26,7 @@
>   #include "qapi/error.h"
>   #include "sysemu/sysemu.h"
>   #include "dbus.h"
> +#include "glib.h"
>   #ifdef G_OS_UNIX
>   #include <gio/gunixfdlist.h>
>   #endif
> @@ -85,7 +86,7 @@ struct _DBusDisplayListener {
>   #endif
>   
>       guint dbus_filter;
> -    guint32 out_serial_to_discard;
> +    guint32 display_serial_to_discard;
>   };
>   
>   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> @@ -93,10 +94,12 @@ G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
>   static void dbus_gfx_update(DisplayChangeListener *dcl,
>                               int x, int y, int w, int h);
>   
> -static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
> +static void ddl_discard_display_messages(DBusDisplayListener *ddl)
>   {
> -    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
> +    guint32 serial = g_dbus_connection_get_last_serial(
>           g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> +
> +    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
>   }
>   
>   #ifdef CONFIG_OPENGL
> @@ -290,7 +293,7 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
>           return;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       width = qemu_dmabuf_get_width(dmabuf);
>       height = qemu_dmabuf_get_height(dmabuf);
> @@ -338,7 +341,7 @@ static bool dbus_scanout_map(DBusDisplayListener *ddl)
>           return false;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
>               ddl->map_proxy,
> @@ -401,7 +404,7 @@ dbus_scanout_share_d3d_texture(
>           return false;
>       }
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
>           ddl->d3d11_proxy,
> @@ -659,7 +662,7 @@ static void ddl_scanout(DBusDisplayListener *ddl)
>           surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
>           (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
>   
> -    ddl_discard_pending_messages(ddl);
> +    ddl_discard_display_messages(ddl);
>   
>       qemu_dbus_display1_listener_call_scanout(
>           ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
> @@ -992,17 +995,34 @@ dbus_filter(GDBusConnection *connection,
>               gpointer         user_data)
>   {
>       DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
> -    guint32 serial;
> +    const gchar *member = NULL;

I suggest removing the initialization will NULL as it may suppress 
uninitialized variable warning.

> +    guint32 serial, discard_serial;
>   
>       if (incoming) {
>           return message;
>       }
>   
>       serial = g_dbus_message_get_serial(message);
> -    if (serial <= ddl->out_serial_to_discard) {
> -        trace_dbus_filter(serial, ddl->out_serial_to_discard);
> -        g_object_unref(message);
> -        return NULL;
> +
> +    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
> +    if (serial <= discard_serial) {
> +        member = g_dbus_message_get_member(message);
> +        if (g_strv_contains((const gchar *[]) {
> +                "Scanout",
> +                "Update",
> +#ifdef CONFIG_GBM
> +                "ScanoutDMABUF",
> +                "UpdateDMABUF",
> +#endif
> +                "ScanoutMap",
> +                "UpdateMap",
> +                "Disable",
> +                NULL,
> +            }, member)) {

I prefer to have a static variable for the array. It makes the object 
code simpler and also avoids to have a multi-line condition in the if 
statement.

> +            trace_dbus_filter(serial, discard_serial);
> +            g_object_unref(message);
> +            return NULL;
> +        }
>       }
>   
>       return message;
Marc-André Lureau Oct. 7, 2024, 8:59 a.m. UTC | #2 
Hi Akihiko

On Sat, Oct 5, 2024 at 12:44 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Filtering pending messages when a new scanout is given shouldn't discard
> > pending cursor changes, for example.
> >
> > Since filtering happens in a different thread, use atomic set/get.
> >
> > Fixes: fa88b85dea ("ui/dbus: filter out pending messages when scanout")
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >   ui/dbus-listener.c | 44 ++++++++++++++++++++++++++++++++------------
> >   1 file changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
> > index 434bd608f2..a70cad3a90 100644
> > --- a/ui/dbus-listener.c
> > +++ b/ui/dbus-listener.c
> > @@ -26,6 +26,7 @@
> >   #include "qapi/error.h"
> >   #include "sysemu/sysemu.h"
> >   #include "dbus.h"
> > +#include "glib.h"
> >   #ifdef G_OS_UNIX
> >   #include <gio/gunixfdlist.h>
> >   #endif
> > @@ -85,7 +86,7 @@ struct _DBusDisplayListener {
> >   #endif
> >
> >       guint dbus_filter;
> > -    guint32 out_serial_to_discard;
> > +    guint32 display_serial_to_discard;
> >   };
> >
> >   G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> > @@ -93,10 +94,12 @@ G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
> >   static void dbus_gfx_update(DisplayChangeListener *dcl,
> >                               int x, int y, int w, int h);
> >
> > -static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
> > +static void ddl_discard_display_messages(DBusDisplayListener *ddl)
> >   {
> > -    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
> > +    guint32 serial = g_dbus_connection_get_last_serial(
> >           g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
> > +
> > +    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
> >   }
> >
> >   #ifdef CONFIG_OPENGL
> > @@ -290,7 +293,7 @@ static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
> >           return;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       width = qemu_dmabuf_get_width(dmabuf);
> >       height = qemu_dmabuf_get_height(dmabuf);
> > @@ -338,7 +341,7 @@ static bool dbus_scanout_map(DBusDisplayListener *ddl)
> >           return false;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
> >               ddl->map_proxy,
> > @@ -401,7 +404,7 @@ dbus_scanout_share_d3d_texture(
> >           return false;
> >       }
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
> >           ddl->d3d11_proxy,
> > @@ -659,7 +662,7 @@ static void ddl_scanout(DBusDisplayListener *ddl)
> >           surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
> >           (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
> >
> > -    ddl_discard_pending_messages(ddl);
> > +    ddl_discard_display_messages(ddl);
> >
> >       qemu_dbus_display1_listener_call_scanout(
> >           ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
> > @@ -992,17 +995,34 @@ dbus_filter(GDBusConnection *connection,
> >               gpointer         user_data)
> >   {
> >       DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
> > -    guint32 serial;
> > +    const gchar *member = NULL;
>
> I suggest removing the initialization will NULL as it may suppress
> uninitialized variable warning.

ok

>
> > +    guint32 serial, discard_serial;
> >
> >       if (incoming) {
> >           return message;
> >       }
> >
> >       serial = g_dbus_message_get_serial(message);
> > -    if (serial <= ddl->out_serial_to_discard) {
> > -        trace_dbus_filter(serial, ddl->out_serial_to_discard);
> > -        g_object_unref(message);
> > -        return NULL;
> > +
> > +    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
> > +    if (serial <= discard_serial) {
> > +        member = g_dbus_message_get_member(message);
> > +        if (g_strv_contains((const gchar *[]) {
> > +                "Scanout",
> > +                "Update",
> > +#ifdef CONFIG_GBM
> > +                "ScanoutDMABUF",
> > +                "UpdateDMABUF",
> > +#endif
> > +                "ScanoutMap",
> > +                "UpdateMap",
> > +                "Disable",
> > +                NULL,
> > +            }, member)) {
>
> I prefer to have a static variable for the array. It makes the object
> code simpler and also avoids to have a multi-line condition in the if
> statement.
>

done, thanks

> > +            trace_dbus_filter(serial, discard_serial);
> > +            g_object_unref(message);
> > +            return NULL;
> > +        }
> >       }
> >
> >       return message;
>
diff mbox series

Patch

diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 434bd608f2..a70cad3a90 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -26,6 +26,7 @@ 
 #include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "dbus.h"
+#include "glib.h"
 #ifdef G_OS_UNIX
 #include <gio/gunixfdlist.h>
 #endif
@@ -85,7 +86,7 @@  struct _DBusDisplayListener {
 #endif
 
     guint dbus_filter;
-    guint32 out_serial_to_discard;
+    guint32 display_serial_to_discard;
 };
 
 G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
@@ -93,10 +94,12 @@  G_DEFINE_TYPE(DBusDisplayListener, dbus_display_listener, G_TYPE_OBJECT)
 static void dbus_gfx_update(DisplayChangeListener *dcl,
                             int x, int y, int w, int h);
 
-static void ddl_discard_pending_messages(DBusDisplayListener *ddl)
+static void ddl_discard_display_messages(DBusDisplayListener *ddl)
 {
-    ddl->out_serial_to_discard = g_dbus_connection_get_last_serial(
+    guint32 serial = g_dbus_connection_get_last_serial(
         g_dbus_proxy_get_connection(G_DBUS_PROXY(ddl->proxy)));
+
+    g_atomic_int_set(&ddl->display_serial_to_discard, serial);
 }
 
 #ifdef CONFIG_OPENGL
@@ -290,7 +293,7 @@  static void dbus_scanout_dmabuf(DisplayChangeListener *dcl,
         return;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     width = qemu_dmabuf_get_width(dmabuf);
     height = qemu_dmabuf_get_height(dmabuf);
@@ -338,7 +341,7 @@  static bool dbus_scanout_map(DBusDisplayListener *ddl)
         return false;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     if (!qemu_dbus_display1_listener_win32_map_call_scanout_map_sync(
             ddl->map_proxy,
@@ -401,7 +404,7 @@  dbus_scanout_share_d3d_texture(
         return false;
     }
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     qemu_dbus_display1_listener_win32_d3d11_call_scanout_texture2d(
         ddl->d3d11_proxy,
@@ -659,7 +662,7 @@  static void ddl_scanout(DBusDisplayListener *ddl)
         surface_stride(ddl->ds) * surface_height(ddl->ds), TRUE,
         (GDestroyNotify)pixman_image_unref, pixman_image_ref(ddl->ds->image));
 
-    ddl_discard_pending_messages(ddl);
+    ddl_discard_display_messages(ddl);
 
     qemu_dbus_display1_listener_call_scanout(
         ddl->proxy, surface_width(ddl->ds), surface_height(ddl->ds),
@@ -992,17 +995,34 @@  dbus_filter(GDBusConnection *connection,
             gpointer         user_data)
 {
     DBusDisplayListener *ddl = DBUS_DISPLAY_LISTENER(user_data);
-    guint32 serial;
+    const gchar *member = NULL;
+    guint32 serial, discard_serial;
 
     if (incoming) {
         return message;
     }
 
     serial = g_dbus_message_get_serial(message);
-    if (serial <= ddl->out_serial_to_discard) {
-        trace_dbus_filter(serial, ddl->out_serial_to_discard);
-        g_object_unref(message);
-        return NULL;
+
+    discard_serial = g_atomic_int_get(&ddl->display_serial_to_discard);
+    if (serial <= discard_serial) {
+        member = g_dbus_message_get_member(message);
+        if (g_strv_contains((const gchar *[]) {
+                "Scanout",
+                "Update",
+#ifdef CONFIG_GBM
+                "ScanoutDMABUF",
+                "UpdateDMABUF",
+#endif
+                "ScanoutMap",
+                "UpdateMap",
+                "Disable",
+                NULL,
+            }, member)) {
+            trace_dbus_filter(serial, discard_serial);
+            g_object_unref(message);
+            return NULL;
+        }
     }
 
     return message;