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

[Gerd Hoffmann]

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.

cheers,
  Gerd