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@xxxxxxxxx> > --- > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel