[Spice-devel] [PATCH] Don't close all but one display during reboot.

[Jonathon Jongsma]

On Thu, 2017-02-23 at 16:25 +0200, Snir Sheriber wrote:
> Hi,
> 
> On 01/31/2017 10:24 PM, Jonathon Jongsma wrote:
> > When a guest is rebooted, the QXL driver gets unloaded at some
> > point in
> > the reboot process. When the driver is unloaded, the spice server
> > sets a
> > single flag to FALSE: RedWorker::driver_cap_monitors_config. This
> > flag
> > indicates whether the driver is capable of sending its own monitors
> > config
> > messages to the client.
> > 
> > The only place this flag is used is when a new primary surface is
> > created. If
> > this flag is true, the server assumes that the driver will send its
> > own
> > monitors config very soon after the surface is created. If it's
> > false, the
> > server directly sends its own temporary monitors config message to
> > the client
> > since the driver cannot.  This temporary monitors config message is
> > based on
> > the size of the just-created primary surface and always has a
> > maximum monitor
> > count of 1.
> > 
> > This flag is initialized to false at startup so that in early boot
> > (e.g. vga
> > text mode), the server will send out these 'temporary' monitor
> > config messages
> > to the client whenever the primary surface is destroyed and re-
> > created. This
> > causes the client to resize its window to match the size of the
> > guest. When the
> > QXL driver is eventually loaded and starts taking over the monitors
> > config
> > responsibilities, we set this flag to true and the server stops
> > sending
> > monitors config messages out on its own.
> > 
> > If we reboot and set this flag to false, it will result in the
> > server sending a
> > monitors config message to the client indicating that the guest now
> > supports a
> > maximum of 1 monitor. If the guest happens to have more than one
> > display window
> > open, it will destroy those extra windows because they exceed the
> > maximum
> > allowed number of monitors. This means that if you reboot a guest
> > with 4
> > monitors, after reboot it will only have 1 monitor.
> > 
> > To avoid this, we assume that if we had the ability to support
> > multiple
> > monitors at some point, that ability will return at some point. So
> > when the
> > server constructs its own temporary monitors config message to send
> > to the
> > client (when the driver_cap_monitors_config flag is false), we
> > continue to send
> > the previous maximum monitor count instead of always sending a
> > maximum of 1.
> > 
> > Resolves: rhbz#1274447
> > ---
> > 
> > NOTE: this fix is a workaround that assumes some things that may
> > not be valid
> > in all cases.  The main assumption is that if the QXL driver was
> > used before
> > the reboot, it will continue to be used after the reboot. If this
> > assumption is
> > violated, the server will continue to report that the guest
> > supports e.g. 4
> > displays even though the driver may only support 1. The client
> > would then be
> > able to attempt to enable additional displays, which would
> > inevitably fail
> > without explanation.  This is not a huge problem, but maybe it's
> > not
> > acceptable. If it's not acceptable, I think that fixing this bug
> > would require
> > significantly more work and may not be worth the effort.
> 
> When i apply your patch, enable 2 displays , reboot & disable qxl
> (using 
> kernel cmd- before booting)
> 2 displays are still functioning after booting although there is no
> qxl. 
> (i know it's because of vdagent but
> can someone shed some light on what exactly is happening? )

Hi Snir,

With my patch, the second display should not get closed during reboot.
The way that spice-gtk/virt-viewer works is that whenever the client
sends a monitors config update, it sends a configuration based on the
current size and position of the client display windows. Since that
second display window was never closed, the client will attempt to
enable a monitor for that display window the next time it sends a
monitors config update. And the act of connecting to the vdagent
triggers the client to send a monitors config update. 

Note: Although both displays will be re-enabled even without the QXL
driver, arbitrary resolution will no longer work. So the displays may
end up being a slightly different size after reboot -- they'll probably
change to the nearest available standard resolution.

Hope that helps,
Jonathon


> 
> Snir.
> >   server/display-channel.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index a554bfd..88ee194 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -2459,15 +2459,18 @@ void
> > display_channel_set_monitors_config_to_primary(DisplayChannel
> > *display)
> >   {
> >       DrawContext *context = &display->priv->surfaces[0].context;
> >       QXLHead head = { 0, };
> > +    uint16_t old_max = 1;
> >   
> >       spice_return_if_fail(display->priv-
> > >surfaces[0].context.canvas);
> >   
> > -    if (display->priv->monitors_config)
> > +    if (display->priv->monitors_config) {
> > +        old_max = display->priv->monitors_config->max_allowed;
> >           monitors_config_unref(display->priv->monitors_config);
> > +    }
> >   
> >       head.width = context->width;
> >       head.height = context->height;
> > -    display->priv->monitors_config = monitors_config_new(&head, 1,
> > 1);
> > +    display->priv->monitors_config = monitors_config_new(&head, 1,
> > old_max);
> >   }
> >   
> >   void display_channel_reset_image_cache(DisplayChannel *self)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel