[Spice-devel] [RFC PATCH v2 00/20] Monitor ID rework

[Marc-André Lureau]

Hi

On Tue, Aug 21, 2018 at 9:32 AM Frediano Ziglio <fziglio at redhat.com> 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