[Spice-devel] [PATCH] Don't close all but one display during reboot.
Jonathon Jongsma
jjongsma at redhat.com
Mon Feb 20 14:49:37 UTC 2017
On Thu, 2017-02-02 at 10:30 -0600, Jonathon Jongsma wrote:
> On Thu, 2017-02-02 at 05:29 -0500, Frediano Ziglio 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.
> > >
> >
> > After reading this paragraph and the bug I don't see the issue.
> > I mean, if on my laptop I reboot when I reboot I get a single
> > monitor
> > till the OS and other stuff kick in. After a reboot the firmware
> > use
> > one monitor or if capable do the mirroring but always the same way.
> >
> > I think the issue is that on first boot the guest activate the
> > additional monitors and the client do the same while on second
> > boot (reboot) not so to me looks like more an issue of the client
> > instead of the server.
> >
>
> Well, it could be considered a client issue to some extent, but not
> an
> easy one to fix.
>
> > I would try to get a network capture to look at DisplayChannel
> > messages if they are more or less the same for first boot and
> > reboot. If they are the same I would look at the client state
> > to check that while booting it's the same.
>
> The initial boot and the reboot are the same, and that's basically
> why
> the problem exists. At initial boot, it brings up a single display.
> And
> on reboot, it also brings up a single display. The issue is that we
> want the reboot to behave differently.
>
> Initial boot:
> -------------
> - In early boot, the server sends monitors config message when
> primary
> surface is created. monitors=1, max-monitors=1
> - client only shows a single window, disables menu items for
> enabling
> additional displays because the guest doesn't support more than 1
> - When QXL driver is loaded and takes over, it takes over sending
> monitors config messages. monitors=1, max-monitors=4
> - client still shows a single window, but now the menu items for
> enabling additional menus are enabled
> - client enables a second monitor. Client sends monitors-config
> request to server
> - QXL driver enables the second monitor and sends a new monitors-
> config response to the client. monitors = 2, max-monitors=4
>
> Reboot:
> -------
> - At some point in the reboot process while the QXL driver is still
> loaded, it disables all but the first monitor, but still claims to
> support more than one. monitors=1, max-monitors=4
> - client keeps both display windows open, but the second one just
> says
> "Waiting for display 2..."
> - eventually the QXL driver gets unloaded, and the
> RedWorker::driver_cap_monitors_config flag gets set to false
> - The next time a primary surface is created (during early boot?)
> the
> server sends out a new monitors config message to match the size of
> the
> primary surface. monitors=1, max-monitors=1
> - the client sees that the guest only supports a single monitor, so
> it
> closes that inactive second display window
> - when the qxl driver is loaded and takes over, it takes over
> sending
> monitors-config messages. monitors=1, max-monitors=4.
>
>
> Before qemu commit 0a2b5e3a7899b40d05d7c6c1c41eb4e64dd2ed4b, the QXL
> driver never called _unload() during reboot. This meant that during
> early boot, the server would never send out monitors-config messages
> with max-monitors=1. So the client would have never closed its extra
> inactive display windows. They would have simply remained open with a
> "Waiting for display n..." message. And as soon as the vdagent was
> reconnected, the client would have sent a new monitors-config request
> attempting to enable displays for all open windows. But now that
> doesn't happen because those windows get closed during reboot.
>
> So my fix attempts to retain that behavior (while still fixing the
> bug
> that was addressed by 0a2b5e3a7899b40d05d7c6c1c41eb4e64dd2ed4b) by
> keeping the max-monitors value the same as it had been before
> rebooting.
>
> You could possibly argue that you should instead fix this issue in
> the
> client by refusing to close/destroy displays when we recieve a
> monitors-config message with a max-monitors value that is less than
> the
> number of currently-open windows. But the design of the client is
> such
> that when we recieve a monitors-config message from the server, we
> automatically create N Display objects (where N is the value of max-
> monitors) even if these displays are not yet enabled. The existence
> of
> these Display objects determines whether or not the menu for enabling
> additional displays are enabled, etc. So if we change the client to
> not
> destroy extra displays when we receive max-monitors, I fear that we
> will introduce some additional bugs by violating some hidden
> assumptions. But we could try it if you think it's a better approach.
> It would be a more complicated fix, however.
Anybody have additional thoughts about this patch?
>
>
> >
> > > 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.
> > >
> > > 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)
> >
> > Frediano
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list