[Spice-devel] [PATCH] Don't close all but one display during reboot.
Jonathon Jongsma
jjongsma at redhat.com
Thu Feb 23 17:48:13 UTC 2017
Hi Daimon, thanks for the input.
On Thu, 2017-02-23 at 05:03 +0000, Daimon Wang wrote:
> Hi,
> I'll go against the fix as I don't see any reason for the
> "assumption".
>
> Let's split the question into 2 things, max monitor number and
> which monitor is enabled.
> The max monitor number seems correct now, controlled by qemu
> together with qxl. And it's used to control the client menu for
> monitors.
> The monitor enable should be "remembered" only by the guest
> OS.while can be controlled by both client and qxl.
> They won't affect each other, and the init-boot/reboot process
> should be similar:
> 1. (Re)Boot with primary VGA -- qemu send maxim one monitor with
> it enabled -- client have 1 window (or close extra ones)
> 2. Qxl loaded (early in guest boot) -- qxl send maxim n monitor
> with one enabled -- client can have 4 window, but only one enabled
> 3. Guest enable some monitors -- qxl send maxim n monitor with x
> enabled -- client have corresponding windows enabled (and placed
> correctly, e.g. onto different monitors)
>
> Everything looks fine above, but why do we have the bug? The
> symptom looks as if the guest "forget" to enable the monitor or
> someone change it. Let's dig it out.
You're basically correct. Technically the server is reporting the
correct values, and my patch is kind of a workaround rather than a
proper fix. But why is a workaround necessary?
Imagine that this is a physical machine rather than a virtual machine.
And further, imagine that we have two physical monitors connected and
enabled. When the machine reboots, at some point the driver reports
that it only supports a single monitor. When this happens, we do not
immediately unplug the second monitor; it stays connected. Now, when
the machine finishes reboot, it brings the driver up and again supports
4 monitors. It notices that 2 monitors are connected and re-enables
both of those monitors. If we had unplugged one of the monitors during
the reboot, then of course we would only have a single monitor enabled
after reboot, even though the driver still supports more than one
monitor.
Now let's go back to the virtual machine case. When we open a second
display window in virt-viewer, it's analogous to plugging in a monitor
in a physical machine. Conversely, closing a display window is
analogous to unplugging that monitor. (It's not a perfect analogy, but
it's very close, and it's good enough for our purposes here). When the
server informs the client that it now only supports a single monitor,
the client doesn't know that this is a transient event (e.g. a reboot).
So the client makes a decision that it no longer makes sense to to have
this second display window open since it is no longer supported, and it
closes this window. As I said above, this is basically equivalent to
unplugging the monitor. So after the guest fully reboots, it now
supports 4 monitors again, but it sees that only one display is
connected, so it only uses a single display. So it's not really a
matter of the guest "forgetting" anything. It's just that the display
is no longer connected, so it can't do anything. From the guest's point
of view, it's identical to the situation of a physical machine with 4
display ports but only a single monitor plugged in.
As I said in a previous email, we could try to fix the issue by
changing the client to not "unplug" the extra monitor when the server
reports that it no longer supports more than one monitor. But that has
some significant implications to the design of the client, and is not
entirely straightforward. And it may not be worth fixing at all in that
case.
Jonathon
>
> Regards,
> Daimon
>
>
> On Tuesday, February 21, 2017 1:00 AM, Christophe de Dinechin <christ
> ophe at dinechin.org> wrote:
>
>
> Is it possible to make the max number of monitors something
> persistent (e.g. set / get that using libvirt), so that the behavior
> would be consistent between a first boot and a reboot?
>
> Christophe
>
> > On 20 Feb 2017, at 15:49, Jonathon Jongsma <jjongsma at redhat.com>
> > wrote:
> >
> > 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
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
> _______________________________________________
> 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