Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Thu, 2018-10-11 at 15:43 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > How does all this monitors_config work currently in case multiple
> > > display channels are present?
> > > 
> > > There is the QXLInterface->client_monitors_config() callback.
> > > 
> > > There is the spice_qxl_monitors_config_async() function.
> > > 
> > > Both are linked to a display channel.  The server/client messages go
> > > through the main channel though ...
> > 
> > Not really, it actually is like this:
> > 
> >        display channel
> > server --------------> client
> > 
> > 
> >         main channel
> > client --------------> server
> > 
> > So the monitors_configs go each on its own display channel to the
> > client, the client puts it all together and sends back an aggregated
> > list on the main channel.
> 
> Ok.
> 
> > > So what happens if a message from the client arrives?  Is that
> > > broadcasted as-is to all display channels?  The current qemu code
> > > (interface_client_monitors_config in spice-display.c, which is used with
> > > virtio-gpu) simply uses the head as index into the array, so it looks
> > > like spice-server doesn't do any filtering here (like only filling in
> > > the monitors which belong to the display channel).
> > 
> > Correct, the spice server doesn't do any filtering and sends the whole
> > monitors_config to all the interfaces...
> 
> Ok.  We probably should fix interface_client_monitors_config() to use
> the channel_id instead of qemu_console_get_head() then.

It's not that simple. This would break the QXL with multiple monitors
per channel case.

I think we should fix spice server to actually do the filtering and
only send monitors_config that belongs to the device to the QXL
interface. As Frediano mentioned, it looks more like a bug.

> > > spice_qxl_monitors_config_async() is never called with virtio-gpu,
> > > except when opengl is enabled.  In that case qemu simply sets
> > > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
> > > (see qemu_spice_gl_monitor_config in spice-display.c).
> > 
> > As a side note, I have no idea about opengl. I got a slight impression
> > from some earlier conversation that this is actually not used, but I
> > don't really know.
> 
> Not by default, but when virgl is enabled (-spice gl=on -vga virtio).
> 
> > My wild guess at this moment is, that if you don't call
> > spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't
> > send any monitors config, a simple one containing a single monitor for
> > the surface is created somewhere along the way.
> 
> Probably.
> 
> > > Which would be
> > > ok in case spice-server merges the configs from all channels before
> > > sending it off to the client.  Not sure this actually happens ...
> > 
> > A bit differently, as I said, but the configs are merged on the client,
> > which should be an equivalent outcome.
> 
> Indeed.  So the qemu_spice_gl_monitor_config() function is correct then.

I don't follow you reasoning for this conclusion... Is it correct
because it sends a single monitor in the monitors_config? Why is this
function needed for the opengl case and not for regular virtio-gpu
case?

> > > > Interstingly enough, that seems to be the ID we want to have in the
> > > > device_display_id attribute.  I expect (still need to look that up, I'm
> > > > out of time right now) that for virtio-gpu the ID is a bit different,
> > > 
> > > Keep in mind that multiple display devices don't really work right now,
> > > and possibly we need to fix not only spice but qemu too.
> > 
> > What exactly do you mean by multiple devices not working?
> 
> qxl + vgpu (which is why we are discussing this).
> qxl + virtio-gpu will have simliar issues I think.

Ok, I see.

> > > I don't think we need the monitors_id in qemu then, qemu can simply use
> > > the channel_id (except for the legacy qxl case).
> > 
> > Ok. Given the fact that the monitor_ids actually come from the driver,
> > QEMU should actually know them already, right?
> 
> Yes.
> 
> > No need to pass them back anyway? (except for the virtio-gpu case,
> > which doesn't send monitors_config and so a single monitor_id = 0 is
> > deduced in spice server)
> 
> When virtio-gpu sends monitors_config monitor_id will be zero too,
> because we have one channel per monitor.

Right.

> Another story is linking display channels to device heads, i.e.
> virtio-gpu registers one display channel per head, each channel
> registers the same device path of course, and now you need to
> figure in the guest agent which xrandr output is which head.

This is actually the reason for the
spice_qxl_device_monitor_add_display_id(qxl, device_display_id)
function (using this opportunity to merge the other email thread
discussion into one).

So for example if you have 3 heads on a virtio-gpu device, and you call
it on qxl instance id (i.e. channel_id) = 2, the device_display_id
argument will be 2 (as it is the head ID) and in the current state
(which I agree with Jonathon is unintuitive and we should probably
change it) spice will associate the first call of this function with
monitor_id 0, so it will know that monitor_id 0 on this qxl instance
has the device_display_id 2.

> Simplest way would be to require display channels being registered in
> order, so the channel with the smallest channel_id is head 0, ...

I don't like this, you once again rely on implicit information derived
from the order of registration of the channels. We should make this
explicit, so that it doesn't cause trouble in the future...

Cheers,
Lukas

> cheers,
>   Gerd
>