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 <jjong...@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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ > Spice-devel mailing list > Spice-devel@lists.freedesktop.org <mailto:Spice-devel@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel