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

["Lukáš Hrázký" <lhrazky@redhat.com>]

On Fri, 2018-10-12 at 11:27 +0200, Gerd Hoffmann wrote:
> On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote:
> > 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.
> 
> Assume you have one qxl and one virtio-gpu device (one head each), for
> example:
> 
>    00:02.0   qxl          channel 0
>    00:03.0   virtio-gpu   channel 1
> 
> So the client will assign display 0 to qxl and display 1 to virtio-gpu.
> In interface_client_monitors_config() we have to pick the correct array
> entry.
> 
> When using the channel_id it works correctly.
> 
> When using qemu_console_get_head() it doesn't work correctly, it would
> use the qxl card's data.  It would work if spice-server would filter the
> list to only include the entries for the given display channel before
> calling the ->client_monitors_config() callback.  But it doesn't, we get
> the complete set.
> 
> > > > 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?
> 
> It would break any multihead configuration which uses multiple display
> channels.  Yes, virtio-gpu is one case.  Breaking that would not be very
> nice, but maybe acceptable.  The other case is multiple qxl devices
> (i.e.  windows guest style multihead).  Breaking that is out of
> question.

The windows QXL driver doesn't support the monitors_config callback, at
least that's what Frediano said here:

https://lists.freedesktop.org/archives/spice-devel/2018-October/045965.html

So I don't think this is a problem and if breaking virtio-gpu would be
acceptable, we should probably consider fixing the interface...

Cheers,
Lukas

> cheers,
>   Gerd
>