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

Fri Aug 17 19:48:16 UTC 2018

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.

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

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

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

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