[Spice-devel] [spice v2] display-channel: monitors_config: add 200ms delay

[Frediano Ziglio]

> 
> From: Victor Toso <me at victortoso.com>
> 
> 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.
> 
> 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 at redhat.com>
> ---
> 
> Changes v1->v2
> * Using reds_core_timer_* api, which calls the timeout function from the
>   right context (Frediano);

Sorry, these functions suffers the same problem, you have to use the core
interface of the DisplayChannel.

OT: I was thinking to add some extra checks on these thread conditions.

> * Remove the SpiceTimer on finalize (Frediano)
> 
>  server/display-channel-private.h |  2 ++
>  server/display-channel.c         | 27 +++++++++++++++++++++++++--
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/server/display-channel-private.h
> b/server/display-channel-private.h
> index 58179531..848abe81 100644
> --- a/server/display-channel-private.h
> +++ b/server/display-channel-private.h
> @@ -118,6 +118,8 @@ struct DisplayChannelPrivate
>  
>      int gl_draw_async_count;
>  
> +    SpiceTimer *monitors_config_update_timer;
> +
>  /* 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 9bb7fa44..a2f95956 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -89,6 +89,12 @@ display_channel_finalize(GObject *object)
>      display_channel_destroy_surfaces(self);
>      image_cache_reset(&self->priv->image_cache);
>  
> +    if (self->priv->monitors_config_update_timer) {
> +        RedsState *reds = red_channel_get_server(RED_CHANNEL(self));
> +        reds_core_timer_remove(reds,
> self->priv->monitors_config_update_timer);
> +        self->priv->monitors_config_update_timer = NULL;
> +    }
> +
>      if (spice_extra_checks) {
>          unsigned int count;
>          _Drawable *drawable;
> @@ -2249,6 +2255,8 @@ DisplayChannel* display_channel_new(RedsState *reds,
>                             NULL);
>      if (display) {
>          display_channel_set_stream_video(display, stream_video);
> +        display->priv->monitors_config_update_timer =
> reds_core_timer_add(reds,
> +                (SpiceTimerFunc) display_channel_push_monitors_config,
> display);

Why not putting this in _init or constructed ?

>      }
>      return display;
>  }
> @@ -2435,6 +2443,11 @@ void
> display_channel_push_monitors_config(DisplayChannel *display)
>  {
>      DisplayChannelClient *dcc;
>  
> +    if (display->priv->monitors_config_update_timer) {
> +        RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
> +        reds_core_timer_cancel(reds,
> display->priv->monitors_config_update_timer);
> +    }
> +
>      FOREACH_DCC(display, dcc) {
>          dcc_push_monitors_config(dcc);
>      }
> @@ -2444,14 +2457,24 @@ void
> display_channel_update_monitors_config(DisplayChannel *display,
>                                              QXLMonitorsConfig *config,
>                                              uint16_t count, uint16_t
>                                              max_allowed)
>  {
> +    RedsState *reds = red_channel_get_server(RED_CHANNEL(display));
>  
> -    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_timer) {
> +        spice_warning("No timer for monitor_config updates");

Why a warning?
I think the code you wrote should initilise the timer once
and should never be NULL.

> +        display_channel_push_monitors_config(display);
> +        return;
> +    }
> +
> +    /* Cancel previous timer, if it was set */
> +    reds_core_timer_cancel(reds,
> display->priv->monitors_config_update_timer);
> +    reds_core_timer_start(reds, display->priv->monitors_config_update_timer,
> 200);
>  }
>  
>  void display_channel_set_monitors_config_to_primary(DisplayChannel *display)

I had a second though on this patch. Let's assume I have only a monitor and
this sequence:
- monitor_config
- update
Your code will change potentially to:
- update
- monitor_config
Now, the client will receive an update before the monitor configuration,
potentially will ignore the update as updating an invalid area.
If there are no other updates the client window could even remain black
waiting for data to arrive.

Frediano