Re: [spice v1 1/2] display-channel: monitors_config: add 200ms delay

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Mar 08, 2019 at 04:32:04PM -0500, Frediano Ziglio wrote:
hi,

> > 
> > From: Victor Toso <me@xxxxxxxxxxxxxx>
> > 
> > The device driver might take a few interactions to reconfigure all
> > displays but in each change it can send it down to spice-server. The
> > client is not interested in temporary states, only the final monitor
> > config can be helpful.
> > 
> > This patch adds a 200ms delay to wait for more changes from guest
> > device. Tested only with QXL in Fedora 29.
> > 
> 
> Which kind of guest? Windows or Linux?

Fedora guest

> > Simple example below, with 4 displays enabled but removing the display 1
> > (starting from 0), spice server receives:
> > 
> >  | 16:50:14.964: display-channel.c:173:monitors_config_unref: freeing
> >  | monitors config
> >  | 16:50:14.964: display-channel.c:181:monitors_config_debug: monitors config
> >  | count:3 max:4
> >  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +0+0 960x997
> >  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> >  | 16:50:14.964: display-channel.c:185:monitors_config_debug: +1984+0
> >  | 1024x740
> > 
> > Sends to the client #1
> > 
> >  | 16:50:14.965: display-channel.c:173:monitors_config_unref: freeing
> >  | monitors config
> >  | 16:50:14.965: display-channel.c:181:monitors_config_debug: monitors config
> >  | count:3 max:4
> >  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 960x997
> >  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +0+0 0x0
> >  | 16:50:14.965: display-channel.c:185:monitors_config_debug: +1984+0
> >  | 1024x740
> > 
> > Sends to the client #2
> > 
> >  | 16:50:14.966: red-worker.c:475:dev_create_primary_surface: trace
> >  | 16:50:14.967: display-channel.c:173:monitors_config_unref: freeing
> >  | monitors config
> >  | 16:50:14.967: display-channel.c:181:monitors_config_debug: monitors config
> >  | count:1 max:4
> >  | 16:50:14.967: display-channel.c:185:monitors_config_debug: +0+0 3008x997
> >  | 16:50:14.972: display-channel.c:173:monitors_config_unref: freeing
> >  | monitors config
> >  | 16:50:14.972: display-channel.c:181:monitors_config_debug: monitors config
> >  | count:3 max:4
> >  | 16:50:14.972: display-channel.c:185:monitors_config_debug: +0+0 960x997
> >  | 16:50:14.973: display-channel.c:185:monitors_config_debug: +0+0 0x0
> >  | 16:50:14.973: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> > 
> > Sends to the client #3
> > 
> >  | 16:50:14.973:
> >  | display-channel.c:2462:display_channel_update_monitors_config: Will
> >  | update monitors in 200ms ~~
> >  | 16:50:14.975: display-channel.c:173:monitors_config_unref: freeing
> >  | monitors config
> >  | 16:50:14.975: display-channel.c:181:monitors_config_debug: monitors config
> >  | count:4 max:4
> >  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 960x997
> >  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +0+0 0x0
> >  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +960+0 1024x740
> >  | 16:50:14.975: display-channel.c:185:monitors_config_debug: +1984+0
> >  | 1024x740
> > 
> > Sends to the client #4 and final
> > 
> > With this patch, only the last one is sent.
> > Resolves: rhbz#1673072
> > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > ---
> >  server/display-channel-private.h |  2 ++
> >  server/display-channel.c         | 16 +++++++++++++---
> >  server/display-channel.h         |  2 +-
> >  3 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/server/display-channel-private.h
> > b/server/display-channel-private.h
> > index 58179531..f1a4314c 100644
> > --- a/server/display-channel-private.h
> > +++ b/server/display-channel-private.h
> > @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
> >  
> >      int gl_draw_async_count;
> >  
> > +    guint monitors_config_update_delay_id;
> > +
> >  /* TODO: some day unify this, make it more runtime.. */
> >      stat_info_t add_stat;
> >      stat_info_t exclude_stat;
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index e68ed10f..7e2584ec 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2430,13 +2430,17 @@ gboolean
> > display_channel_validate_surface(DisplayChannel *display, uint32_t surf
> >      return TRUE;
> >  }
> >  
> > -void display_channel_push_monitors_config(DisplayChannel *display)
> > +gboolean display_channel_push_monitors_config(gpointer userdata)
> >  {
> >      DisplayChannelClient *dcc;
> > +    DisplayChannel *display = userdata;
> > +
> > +    display->priv->monitors_config_update_delay_id = 0;
> >  
> >      FOREACH_DCC(display, dcc) {
> >          dcc_push_monitors_config(dcc);
> >      }
> > +    return G_SOURCE_REMOVE;
> >  }
> >  
> >  void display_channel_update_monitors_config(DisplayChannel *display,
> > @@ -2444,13 +2448,19 @@ void
> > display_channel_update_monitors_config(DisplayChannel *display,
> >                                              uint16_t count, uint16_t
> >                                              max_allowed)
> >  {
> >  
> > -    if (display->priv->monitors_config)
> > +    if (display->priv->monitors_config) {
> >          monitors_config_unref(display->priv->monitors_config);
> > +    }
> >  
> >      display->priv->monitors_config =
> >          monitors_config_new(config->heads, count, max_allowed);
> >  
> > -    display_channel_push_monitors_config(display);
> > +    if (display->priv->monitors_config_update_delay_id != 0) {
> > +        g_source_remove(display->priv->monitors_config_update_delay_id);
> > +    }
> > +
> > +    display->priv->monitors_config_update_delay_id =
> > +        g_timeout_add (200, display_channel_push_monitors_config, display);
> 
> Don't use g_timeout_add, it adds the source to the main context
> so the function won't be executed in DisplayChannel thread
> causing thread race conditions.

Aha, I overlooked that. Many thanks.
It seems fine to use reds_core_timer_* instead, I'll be testing
and sending that later this morning.

> I don't see the part of code that remove the source in case
> DisplayChannel is destroyed.

I'll add as well.

> 
> >  }
> >  
> >  void display_channel_set_monitors_config_to_primary(DisplayChannel *display)
> > diff --git a/server/display-channel.h b/server/display-channel.h
> > index 948018cf..1d64afce 100644
> > --- a/server/display-channel.h
> > +++ b/server/display-channel.h
> > @@ -149,7 +149,7 @@ void                       display_channel_gl_draw_done
> > (DisplayCha
> >  void display_channel_update_monitors_config(DisplayChannel *display,
> >  QXLMonitorsConfig *config,
> >                                              uint16_t count, uint16_t
> >                                              max_allowed);
> >  void display_channel_set_monitors_config_to_primary(DisplayChannel
> >  *display);
> > -void display_channel_push_monitors_config(DisplayChannel *display);
> > +gboolean display_channel_push_monitors_config(gpointer display);
> >  
> >  gboolean display_channel_validate_surface(DisplayChannel *display, uint32_t
> >  surface_id);
> >  gboolean display_channel_surface_has_canvas(DisplayChannel *display,
> >  uint32_t surface_id);
> 
> Otherwise the patch seems good.
> 
> Frediano

Thanks,
Victor

Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]