[Spice-devel] [PATCH spice-gtk v2 9/8] main: don't update display timer for unchanged config

[Jonathon Jongsma]

On Wed, 2016-03-23 at 18:02 +0100, Marc-André Lureau wrote:
> With virgl, set_monitor_ready() may be called each time the scanout is
> updated to set the monitor area. This will call
> spice_main_update_display(), and keep the timer postponed even if the
> monitor configuration didn't change. Treat unchanged configuration has a

has -> as

> no-op and keep configuration timer unchanged. This fixes monitor
> autoconfig with virgl (when the display is regularly updated).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at gmail.com>
> ---
>  src/channel-main.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 1c19de1..b4875f6 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -121,6 +121,14 @@ typedef enum {
>      DISPLAY_ENABLED,
>  } SpiceDisplayState;
>  
> +typedef struct {
> +    int                     x;
> +    int                     y;
> +    int                     width;
> +    int                     height;
> +    SpiceDisplayState       display_state;
> +} SpiceDisplayConfig;
> +
>  struct _SpiceMainChannelPrivate  {
>      enum SpiceMouseMode         mouse_mode;
>      bool                        agent_connected;
> @@ -140,13 +148,7 @@ struct _SpiceMainChannelPrivate  {
>      guint                       agent_msg_pos;
>      uint8_t                     agent_msg_size;
>      uint32_t                    agent_caps[VD_AGENT_CAPS_SIZE];
> -    struct {
> -        int                     x;
> -        int                     y;
> -        int                     width;
> -        int                     height;
> -        SpiceDisplayState       display_state;
> -    } display[MAX_DISPLAY];
> +    SpiceDisplayConfig          display[MAX_DISPLAY];
>      gint                        timer_id;
>      GQueue                      *agent_msg_queue;
>      GHashTable                  *file_xfer_tasks;
> @@ -2686,10 +2688,15 @@ void spice_main_update_display(SpiceMainChannel
> *channel, int id,
>  
>      g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
>  
> -    c->display[id].x      = x;
> -    c->display[id].y      = y;
> -    c->display[id].width  = width;
> -    c->display[id].height = height;
> +    SpiceDisplayConfig display = {
> +        .x = x, .y = y, .width = width, .height = height,
> +        .display_state = c->display[id].display_state
> +    };
> +
> +    if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> +        return;
> +
> +    c->display[id] = display;
>  
>      if (update)
>          update_display_timer(channel, 1);


Looks basically OK. I do have the same minor concern I had when Pavel submitted
a similar patch in the past[1]. Your goal is slightly different than Pavel's was
in that you want to avoid delaying the monitor config message, and he wanted to
avoid sending it. But it would seem more correct to me if you only did this
comparison and early return if we already had a config message scheduled to be
sent.

For example consider the following contrived situation:
A. client updates display to AxB, which sends a monitor config message to the
guest
B. guest gets re-configured to AxB
(some time passes)
C. guest changes resolution and notifies client that it is now CxD
D. client updates monitor to AxB
E. since this matches the last monitor config message that we sent to the guest,
it returns without scheduling a monitor update, and guest does not get updated.

At the end of this scenario, you'd expect the guest to be configured to AxB, but
it would remain configured to CxD. 

In practice, this scenario probably cannot happen with current spice-gtk. This i
s because spice-gtk currently (and arguably, unnecessarily) sends a CxD monitor
config message back to the guest in response to step C. But if we ever "fixed"
that behavior to avoid sending that CxD config to the guest, this issue would
suddenly appear.


[1] https://lists.freedesktop.org/archives/spice-devel/2015-October/022785.html