[Spice-devel] [PATCH] Don't close all but one display during reboot.
Daimon Wang
daimon_swang at yahoo.com
Thu Feb 23 05:03:08 UTC 2017
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.
Regards,Daimon
On Tuesday, February 21, 2017 1:00 AM, Christophe de Dinechin <christophe 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170223/e4e0157c/attachment-0001.html>
More information about the Spice-devel
mailing list