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

Submitted by Victor Toso on March 11, 2019, 10:02 a.m.

Details

Message ID 20190311100227.9477-1-victortoso@redhat.com
State New
Headers show
Series "display-channel: monitors_config: add 200ms delay" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso March 11, 2019, 10:02 a.m.
From: Victor Toso <me@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@redhat.com>
---

Changes v1->v2
* Using reds_core_timer_* api, which calls the timeout function from the
  right context (Frediano);
* Remove the SpiceTimer on finalize (Frediano)

 server/display-channel-private.h |  2 ++
 server/display-channel.c         | 27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

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);
     }
     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");
+        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)

Comments


> 
> From: Victor Toso <me@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@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