Hi On Tue, Aug 21, 2018 at 9:32 AM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > User experience or not even with the switch implemented this can't > work for the same reason that we want these patches. > How the server knows which displays to show to the client? I would say 1 vgpu associated with 1 optional qxl device, on 1 display channel. Switch when streaming start/stop. Is that not possible? > QXL? vGPU? Both? As stated already in this thread all 3 use cases can happen. > You'll still have to add some information exchange in order to share > this information. This is not a justification for the protocol & client side changes though. We support multiple devices already. > > Personally looking at how the hardware deal with similar conditions > the more close to Qxl not used is when the system disables the monitor > for power saving. Currently however the system don't do that, simply > Xorg uses (default case) only vGPU leaving Qxl as it is. Instead of > switching however this could be an idea (so the client instead of > showing text/blank/"waiting for display" will just hide the window). Disabling a monitor used to be reflected by the client, but I am not sure what is the current status and what you really want to develop here anyway. > > > > > > > > > > > > > > How is the client going to do that? Wouldn't that logic fit better in > > > > the server? > > > > > > > > > > > > > > > > Is there a new channel that the client will have to handle and > > > > > > somehow > > > > > > match with a display channel id (or channel_id=0 & monitor_id) ? > > > > > > > > > > There's not a new channel type, but there is a new channel, because > > > > > there are two devices. Both the QXL device and the vGPU have their > > > > > own > > > > > Display channels. > > > > > * channel #0 is the QXL device and only displays stuff at boot > > > > > time > > > > > (or when switching to a VT) > > > > > * channel #1 is the nvidia device, which displays the desktop > > > > > session > > > > > after X starts > > > > > > > > If they are meant to be the same display, then why the server can't > > > > handle the "switching" logic and use a single display channel? > > > > > > > > > Note that X only knows about the nvidia device, and from X's > > > > > perspective, that device had a xrandr id of 0. So when (for > > > > > example) we > > > > > send mouse events for channel #1 to the vdagent, the vdagent will > > > > > say: > > > > > I don't know any dislay with id #1, I only know about display #0, > > > > > and > > > > > it will ignore those events. > > > > > > > > > > Up to this point, we've used the formula "channel_id ? channel_id : > > > > > monitor_id" as the ID of the display. And we've assumed that this > > > > > ID > > > > > matches the guest output id (i.e. xrandr ID) of the display. But in > > > > > streaming mode we're now violating that assumption so things break. > > > > > > > > > > > > > > > > > > > > > > > > > In summary, this patch series addresses the issues as follows: > > > > > > > > > > > > > > 1. It replaces the current aggregate ID sent from the client to > > > > > > > the > > > > > > > server with the pair (channel_id, monitor_id). In particular, > > > > > > > it > > > > > > > replaces the VDAgentMonitorsConfig message with > > > > > > > SpiceMsgcMainMonitorsConfig and MousePosition with > > > > > > > MousePositionV2. > > > > > > > > > > > > As said above, I think both client and server can deal with the > > > > > > situation. I don't like either to expose that detail to the > > > > > > client > > > > > > API. > > > > > > > > > > *How* do you think the client and the server can deal with the > > > > > situation? Just saying that you think they can deal with it is not > > > > > very > > > > > helpful ;) > > > > > > > > Sorry, I am trying to understand what we are achieving here. I don't > > > > know why the client would know better either how to handle several > > > > display channels. > > > > > > > > To me it looks like there is an opportunity to solve the problem at > > > > the server level, without changing the protocol and modifying the > > > > client. I would like to understand why it is not possible. > > > > > > > > thanks > > > > > > > > > > > > > > > > > > > > > > > > 2. On the server, it translates the ID pair to the guest-side > > > > > > > ID in > > > > > > > one of the following ways: > > > > > > > > > > > > > > a) For the guest-side streaming case, it looks up the ID in the > > > > > > > monitors_guest_output_id hash table, into which the IDs are > > > > > > > stored > > > > > > > from > > > > > > > StreamInfo messages from the streaming agent (which runs in the > > > > > > > guest > > > > > > > context and therefore has the correct IDs). > > > > > > > > > > > > > > b) For the regular SPICE case, the calculation is still using > > > > > > > the > > > > > > > sub-optimal `channel_id ? channel_id : monitor_id` for lack of > > > > > > > better > > > > > > > options. This calculation is now confined in the server though > > > > > > > and > > > > > > > can > > > > > > > be more easily replaced with something better later. > > > > > > > > > > > > > > 3. The Messages from the server to the vd_agent now contain the > > > > > > > correct guest-side ID the vd_agent understands. (This involves > > > > > > > reworking > > > > > > > the MonitorsConfig message to contain the ID as an item in the > > > > > > > message > > > > > > > instead of relying on the array index. This is not strictly > > > > > > > necessary > > > > > > > but makes for a cleaner and more explicit code.) > > > > > > > > > > > > I can't comment on server/agent code, I haven't looked the > > > > > > streaming > > > > > > part. > > > > > > > > > > > > > > > > > > > > > > > > > > > The code is still lacking capabilities to ensure backwards > > > > > > > compatibility, marked with a TODO in a couple of places and > > > > > > > possibly > > > > > > > more is needed. It is meant to work only with all components > > > > > > > having > > > > > > > the > > > > > > > patches from this series. Once this is agreed upon, I'll add > > > > > > > the > > > > > > > capabilities in a later version. > > > > > > > > > > > > > > Some intrusive changes were needed on the client side, > > > > > > > impacting > > > > > > > also > > > > > > > the spice-gtk API. I'd especially like feedback on that, I'm > > > > > > > not > > > > > > > sure > > > > > > > it's good enough as it is. > > > > > > > > > > > > > > One known thing the spice-gtk changes break is the fullscreen > > > > > > > mode > > > > > > > in > > > > > > > virt-viewer. In fullscreen mode, virt-viewer tries to configure > > > > > > > the > > > > > > > monitors as soon as it starts, while now updating the monitors > > > > > > > only > > > > > > > work > > > > > > > after the monitors_config messages were received for them. My > > > > > > > plan > > > > > > > is to > > > > > > > attempt to fix this in virt-viewer, meaning building an older > > > > > > > virt-viewer with spice-gtk containing these changes will result > > > > > > > in > > > > > > > broken fullscreen mode. > > > > > > > > > > > > > > I'll also mention I had some redraw glitches and some gnome- > > > > > > > shell > > > > > > > crashes during testing, which most probably were bugs unrelated > > > > > > > to > > > > > > > SPICE. I decided to get the patches out there and investigate > > > > > > > later, if > > > > > > > you test this and get similar issues though, let me know. > > > > > > > > > > > > > > Cheers, > > > > > > > Lukas > > > > > > > > > Frediano -- Marc-André Lureau _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel