[Spice-devel] [PATCH] Don't close all but one display during reboot.
Christophe de Dinechin
christophe at dinechin.org
Thu Feb 23 10:58:53 UTC 2017
Hi Daimon,
In a real system, the connected monitors and their EDID provide information to the OS about how many monitors are available, the resolution they support, etc. That information is persistent for the OS, i.e. it is still there even when the OS is down. Where do you plan to get the “available number of monitors” information from in step 2?
My understanding is that we can’t simply use the number of QXL devices, since one device can be multihead. Today, there is no persistent configuration of the number of monitors that I know of, only a hint regarding the number of channels based on the number of QXL instances.
What is the rationale behind the different recommendation for Windows and Linux (Windows: 1 QXL device per head; Linux: 1 multi-head QXL device) ?
I think the one case where this approach fails, which Jonathan’s patch tries to address, is remote-viewer with multiple monitors configured from remote-viewer. Is that correct, or is there another scenario I did not think of?
Thanks
Christophe
> On 23 Feb 2017, at 06:03, Daimon Wang <daimon_swang at yahoo.com> 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.
>
> 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 <mailto: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 <mailto: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>
>
> _______________________________________________
> 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>
>
>
> _______________________________________________
> 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/22724b00/attachment-0001.html>
More information about the Spice-devel
mailing list