Hi Akihiko
On Sat, Oct 5, 2024 at 12:44 PM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
>
> On 2024/10/03 20:22, marcandre.lur...@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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;
>
