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

Mon Aug 20 14:21:20 UTC 2018

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/monitors-
> > > config-v2
> > > https://gitlab.freedesktop.org/lukash/spice-common/tree/monitors-co
> > > nfig-v2
> > > https://gitlab.freedesktop.org/lukash/spice-streaming-agent/tree/mo
> > > nitors-config-v2
> > > https://gitlab.freedesktop.org/lukash/spice/tree/monitors-config-v2
> > > https://gitlab.freedesktop.org/lukash/spice-gtk/tree/monitors-confi
> > > g-v2
> > > https://gitlab.freedesktop.org/lukash/vd_agent/tree/monitors-config
> > > -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?

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

-- 
Marc-André Lureau