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 17:09 +0200, Gerd Hoffmann wrote:
> > > 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.

Ok.. and what's actually the problem using qemu_console_get_head()? It
just doesn't feel right to me using channel_id as an index into this
array. It is not the right index strictly speaking.

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

That's right. But if we don't actually break any currently supported
use cases, it might be fine? The only thing we would be breaking is the
virtio-gpu, I think? Is that already something we don't want to break?

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

Ok.

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

Sure, no problem.

> cheers,
>   Gerd
>