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

[Jonathon Jongsma]

On Tue, 2018-08-21 at 14:51 +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Aug 21, 2018 at 2:08 PM Lukáš Hrázký <lhrazky at redhat.com>
> wrote:
> > 
> > On Tue, 2018-08-21 at 13:09 +0200, Marc-André Lureau wrote:
> > > The API & protocol abstracted away the channel ID/monitor ID
> > > details
> > > for the client. You want to expose it now, but the reasons aren't
> > > well
> > > justified, and you are pushing the problems to the client side
> > > while
> > > breaking API/protocol etc - given our experience with this, this
> > > is
> > > quite risky.
> > 
> > I'm sorry, but this statement is 100% false and in fact, it is
> > actually
> > the opposite.
> > 
> > Please have a look at virt-viewer-display-spice.c:295 in virt-
> > viewer
> > (current master):
> > 
> >     self = g_object_new(VIRT_VIEWER_TYPE_DISPLAY_SPICE,
> >                         "session", session,
> >                         // either monitorid is always 0 or
> > channelid
> >                         // is, we can't have display (0, 2) and (2,
> > 0)
> >                         // for example
> >                         "nth-display", channelid + monitorid,
> >                         NULL);
> > 
> > Do you see the "channelid + monitorid"? Now where are the channel
> > ID/monitor ID details abstracted away from the client? Where are
> > the
> > problems now, if not on the client side, where you have to do this
> > ugly
> > nonsense?
> 
> In fact it should be written: channelid || monitorid imho. Is this
> really so ugly? How adding more details is going to make it look less
> "ugly"?

Well, I think it is fairly ugly because it relies on implicit shared
assumptions between the server and the client. It's never very
satisfying to have implicit assumptions between two different software
components. Things should be explicit as much as possible. We've been
able to make it work so far by saying "we only support this
configuration, and if you configure it any other way, you'll need to
deal with the undefined behavior".

But now we have a situation where we need to use a configuration that
hasn't been supported up to this point. So we're running into the
limitations of relying on these implicit assumptions.

Lukas's changes here are basically just an attempt to make those
details explicit. 


> This formula isn't used or necessary to set the monitor config at the
> lower level: spice-gtk, protocol etc.. The client requests a display
> configuration (without specifying channel/monitor id), the
> server/guest side does its best, and the client reflects the
> current/selected configuration. what is wrong?

You seem to be overlooking one of the primary problems here. The
situation we have is one where we have (at the moment) two devices that
each have a single dispay (so far, no assumptions violated). However,
it not all devices are configured to be used within X, the ID that we
use to refer to the display will not match the ID used by the vdagent
within the guest. So there's another implicit assumption that is
getting violated.

> 
> How is the client supposed to know that it should use monitor id or
> multiple channel id, or which display id is associated with which gpu
> etc? In general, does he need to care, can't we decide the best
> configuration for him?
> 
> > 
> > > > > 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).
> > > > 
> > > > Not sure how you mean to solve the issue with this, but I'm
> > > > quite
> > > > certain it wouldn't work.
> > > 
> > > The agent shouldn't just rely on XRandr, it could correctly
> > > associate
> > > a display_id with a device, knowing that some QXL devices are
> > > disabled. Imho, it's more an guest/agent issue than a client
> > > issue.
> > 
> > So you are suggesting that the vd_agent should look for a QXL
> > device,
> > verify that it is not configured under X, assume that this device's
> > monitors precede the xrandr output IDs in the sequence and use this
> > information to shift the IDs it received?
> 
> If he did configure it that way, it's his responsability, yes.

"he"? the vdagent did not configure it to use the vgpu and not the qxl
device for X. That was done by the person that set up the guest.
vdagent should not try to be smart and assume things. That will just
make things more unpredictable.

> 
> But then again, I am not sure this is all necessary depending on how
> the problem is solved (if we expose all devices with seperate display
> channel, or if we don't have to support mixing QXL & vgpu)

Well, yeah, if we didn't have to support this case, these changes
wouldn't be necessary. But if we had a more flexible protocol from the
start, we could have supported this use case without much work.

> 
> > If so, it's complicated, fragile, will not work for the mjpeg
> > streaming
> > of a QXL device case and in general just digs us a deeper hole...
> 
> Be sure I try to avoid that: breaking thing is digging deeper hole
> imho
> 
> > > > > What is setting up the X server config?
> > > > 
> > > > Nothing is atm., there's a fixed config for a single monitor.
> > > > We still
> > > > need to do this.
> > > 
> > > I would explore that kind of options before breaking things.
> > 
> > If you're still talking about generating Xorg config for Nvidia
> > vGPU, I
> > don't see how it can actually help with the monitor ID issues in
> > general. IMO it's mostly unrelated.
> 
> We'll have to disagree then.
> 
>