[Spice-devel] [PATCH] Don't close all but one display during reboot.
Christophe de Dinechin
christophe at dinechin.org
Mon Feb 20 17:00:22 UTC 2017
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 <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170220/03b9eedd/attachment-0001.html>
More information about the Spice-devel
mailing list