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

Tue Aug 21 07:32:06 UTC 2018

> Hi
> 
> On Mon, Aug 20, 2018 at 9:00 PM Jonathon Jongsma <jjongsma at redhat.com> wrote:
> >
> > On Mon, 2018-08-20 at 16:21 +0200, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Fri, Aug 17, 2018 at 9:48 PM Jonathon Jongsma <jjongsma at redhat.com
> > > > wrote:
> > > >
> > > > On Fri, 2018-08-17 at 16:53 +0200, Marc-André Lureau wrote:
> > > > > On Thu, Aug 16, 2018 at 6:26 PM Lukáš Hrázký <lhrazky at redhat.com>
> > > > > wrote:
> > > > > >
> > > > > > Hello list,
> > > > > >
> > > > > > this is the reworked second version of the Monitor ID series.
> > > > > > The
> > > > > > goal
> > > > > > of this series is to make the identification of monitors in the
> > > > > > monitors_config exchange and in the MousePosition messages more
> > > > > > robust,
> > > > > > as well as fix the case of guest-side streaming via the
> > > > > > spice-streaming-agent, where the current identification
> > > > > > mechanism
> > > > > > is no
> > > > > > longer sufficient.
> > > > > >
> > > > > > You can also find the patches in the following branches:
> > > > > > https://gitlab.freedesktop.org/lukash/spice-protocol/tree/monit
> > > > > > ors-
> > > > > > config-v2
> > > > > > https://gitlab.freedesktop.org/lukash/spice-common/tree/monitor
> > > > > > s-co
> > > > > > nfig-v2
> > > > > > https://gitlab.freedesktop.org/lukash/spice-streaming-agent/tre
> > > > > > e/mo
> > > > > > nitors-config-v2
> > > > > > https://gitlab.freedesktop.org/lukash/spice/tree/monitors-confi
> > > > > > g-v2
> > > > > > https://gitlab.freedesktop.org/lukash/spice-gtk/tree/monitors-c
> > > > > > onfi
> > > > > > g-v2
> > > > > > https://gitlab.freedesktop.org/lukash/vd_agent/tree/monitors-co
> > > > > > nfig
> > > > > > -v2
> > > > > >
> > > > > >
> > > > > > There are two issues with the IDs used currently to identify
> > > > > > guest
> > > > > > monitors:
> > > > > >
> > > > > > 1. The IDs sent from the client in VDAgentMonitorsConfig and
> > > > > > MousePosition messages are equal to either `channel_id +
> > > > > > monitor_id` or
> > > > > > `channel_id ? channel_id : monitor_id`. This is under the
> > > > > > assumption
> > > > > > that there is either only one display_channel or more display
> > > > > > channels
> > > > > > each with only a single monitor. Under this assumption the
> > > > > > aforementioned expressions are equivalent. It is not a reliable
> > > > > > way
> > > > > > to
> > > > > > identify monitors though.
> > > > > >
> > > > >
> > > > > It's not that mixed. We only support:
> > > > >
> > > > > 1 display channel, with id=0 & SPICE_DISPLAY_CAP_MONITORS_CONFIG:
> > > > > display ID = monitor ID
> > > > >
> > > > > > 1 display channel: display ID = channel ID.
> > > > >
> > > > > Whenever the server receives VD_AGENT_MONITORS_CONFIG, it can
> > > > > also
> > > > > easily handle the config accordingly.
> > > > > Similarly on the client side.
> > > >
> > > > I don't understand your comment here.
> > >
> > > I meant to say that both client & server can understand wether we use
> > > the channel_id == display_id mapping, or channel 0 + monitor_id ==
> > > display_id, based on the number of channel and the MONITORS_CONFIG
> > > capability.
> > >
> > > >
> > > > >
> > > > > > 2. In the guest-side streaming case, the ID scheme mentioned in
> > > > > > 1.
> > > > > > no
> > > > > > longer works, because these IDs are sent directly to the
> > > > > > vd_agent
> > > > > > and
> > > > > > interpreted as xrandr output IDs in the guest context. We have
> > > > > > two
> > > > > > main
> > > > > > use-cases in both of which the IDs sent to the guest do not
> > > > > > match
> > > > > > the
> > > > > > guest-side xrandr output IDs:
> > > > > > a) QXL device whose content is streamed with the mjpeg plugin.
> > > > > > So
> > > > > > there
> > > > > > are two displays on the client, the second one a streamed
> > > > > > mirror of
> > > > > > the
> > > > > > first. And there are two channels, each of which with one
> > > > > > monitor,
> > > > > > the IDs
> > > > > > are 0 and 1.  However, in the guest there is only a single
> > > > > > output,
> > > > > > ID 0.
> > > > > >
> > > > > > b) The main use case, vGPU-accelerated streaming from the
> > > > > > guest.
> > > > > > There
> > > > > > usually is a QXL device to see the boot etc., the streaming
> > > > > > only
> > > > > > starts
> > > > > > with X. Therefore, the QXL monitor has ID 0 and the streamed
> > > > > > monitor has
> > > > > > an ID of 1. However, the QXL monitor is not used in X, only the
> > > > > > vGPU is
> > > > > > configured in X and it's first monitor under X has the ID of 0.
> > > > >
> > > > > Sorry I can't follow as I am not familiar enough with the
> > > > > streaming
> > > > > mode.
> > > >
> > > > In streaming mode, the guest has 2 devices. The guest is configured
> > > > to
> > > > use the vGPU device for X. But the early boot is displayed on a QXL
> > > > device.
> > >
> > > And I suppose we want to map those 2 channels to the same display
> > > seamlessly. right?
> >
> > Well, no, not really. They are two separate displays. In the past we
> > did discuss trying to transparently switch between them, but we don't
> > have plans to do that at the moment. I personally think that it would
> > add a huge amount of complexity that would make things very difficult
> > to maintain.
> 
> Well it's a switching point, you need to define it carefully. It may
> be simple or not, but it is just a condition, And the code to switch
> from one to the other shouldn't be so terrible.
> 
> >
> > It's true that the user experience of having two separate displays is
> > not good, and we need to address that at some point. It's a bit of an
> > open question how this should be handled in a user-friendly way. But
> > I'm not sure tricky switching in the server is the answer. Feel free to
> > convince me, though.
> 
> From a user point of view, it seems pretty bad if you have half of
> your displays that are disabled. Is it really what we want to provide,
> even as the first step?
> 
> And it doesn't seem justified to change all the client side, api and
> protocol just because the guest X server is configured in an
> "uncommon" or "unsupported" configuration. Have you tried to teach
> vdagent to handle such X server setup instead (to handle the monitor
> config and avoid the cursor issue you described). What is setting up
> the X server config?
> 

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?
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.

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).

> >
> >
> > >
> > > 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