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

[Gerd Hoffmann <kraxel@redhat.com>]

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

It is that simple.

qxl doesn't use that code path and has its own version of the callback
(in qxl.c).  Fixing it there is a bit more tricky.

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

Only problem is changing the callback semantics changes the library api.
We could add a second version of the callback which gets called with a
filtered list (if present).  Not sure this is worth the effort though.

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

Yes (again, qxl doesn't run this code path so it'll only see the
one-monitor-per-channel cases).

> Why is this
> function needed for the opengl case and not for regular virtio-gpu
> case?

Not fully sure why opengl needs this.  Maybe because the texture can be
larger than the actual scanout (for padding reasons) so we need to
communicate the scanout rectangle.

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

Ah, ok.  We should *not* call this thing display_id then, that'll be
confusing.  Or at very least rename the function to something like ...

spice_qxl_monitor_add_device_display_id()
                      ^^^^^^^^^^^^^^^^^   don't split this

... to make more clear this is something else than the display_id.

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

Fine with me too.

I'd suggest to split the patches, one for the path and one for the
device_display_id thing (or whatever else we call this to make it less
likely being confused with display_id, even though I don't have a good
suggestion offhand).

cheers,
  Gerd