[Spice-devel] [RFC PATCH spice-common v2 02/20] A version 2 of the MousePosition message

[Marc-André Lureau]

Hi

On Thu, Aug 23, 2018 at 2:15 PM Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Thu, 2018-08-23 at 03:51 -0400, Frediano Ziglio wrote:
> > >
> > > Hi
> > >
> > > On Wed, Aug 22, 2018 at 11:11 PM Jonathon Jongsma <jjongsma at redhat.com>
> > > wrote:
> > > >
> > > > On Thu, 2018-08-16 at 18:26 +0200, Lukáš Hrázký wrote:
> > > > > The version 2 is using a (channel_id, monitor_id) pair to uniquely
> > > > > identify the display on which the event occured, instead of the
> > > > > ambiguous display_id.
> > > > >
> > > > > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > > > > ---
> > > > >  common/messages.h | 8 ++++++++
> > > > >  spice.proto       | 8 ++++++++
> > > > >  2 files changed, 16 insertions(+)
> > > > >
> > > > > diff --git a/common/messages.h b/common/messages.h
> > > > > index 942ba07..9b05cee 100644
> > > > > --- a/common/messages.h
> > > > > +++ b/common/messages.h
> > > > > @@ -460,6 +460,14 @@ typedef struct SpiceMsgcMousePosition {
> > > > >      uint8_t display_id;
> > > > >  } SpiceMsgcMousePosition;
> > > > >
> > > > > +typedef struct SpiceMsgcMousePositionV2 {
> > > > > +    uint32_t x;
> > > > > +    uint32_t y;
> > > > > +    uint32_t buttons_state;
> > > > > +    uint32_t channel_id;
> > > > > +    uint32_t monitor_id;
> > > > > +} SpiceMsgcMousePositionV2;
> > > > > +
> > > > >  typedef struct SpiceMsgcMousePress {
> > > > >      int32_t button;
> > > > >      int32_t buttons_state;
> > > > > diff --git a/spice.proto b/spice.proto
> > > > > index 80976d4..14475fc 100644
> > > > > --- a/spice.proto
> > > > > +++ b/spice.proto
> > > > > @@ -1092,6 +1092,14 @@ channel InputsChannel : BaseChannel {
> > > > >          uint8 display_id;
> > > > >      } @ctype(SpiceMsgcMousePosition) mouse_position;
> > > > >
> > > > > +    message {
> > > > > +        uint32 x;
> > > > > +        uint32 y;
> > > > > +        mouse_button_mask buttons_state;
> > > > > +        uint32 channel_id;
> > > > > +        uint32 monitor_id;
> >
> > uint8 for both channel_id and monitor_id.
> >
> > > > > +    } @ctype(SpiceMsgcMousePositionV2) mouse_position_v2;
> > > > > +
> > > > >      message {
> > > > >          mouse_button button;
> > > > >          mouse_button_mask buttons_state;
> > > >
> > > >
> > > > This protocol change is clearly necessary if we're going to support any
> > > > configurations other than those that we've supported in the past.
> > >
> > > It's not clear to me that it's necessary if we still have the same
> > > constrains, that allow us to use the "displayid = channeld_id ||
> > > monitor_id" formula.
> >
> > The formula is not "channel_id || monitor_id", this is just confusing, is
> > more "channel_id + monitor_id" or using GCC extension "channel_id ?: monitor_id".
> > Note that this formula is present in spice-gtk, virt-viewer (as Lukash mentioned),
> > and in SPICE protocol as index in agent MonitorsConfig (structure that is
> > used also by the server) and agent MouseState (as display_id field).
> > One of the main reason of these patches is that before (as use cases were limited)
> > the agent understand what the display_id was, now this is no more true.
> >
> > Note also that under Linux currently all these protocol limitations allows
> > to support only a single device with multiple output, which is a problem for
> > vGPU. Under Windows we support multiple devices but all devices are limited
> > to one monitor. Extending Linux using the same limitation requires currently
> > that all monitor output would be mapped to a single DisplayChannel surface
> > (as currently only surface 0 can be primary) with all monitor sub-area of
> > the single surface. The complexity of this just for the above formula is not
> > worth. Extending the Windows idea (also in Linux) would require to have single
> > monitor for each DisplayChannel (so QXL devices has to be limited to one output)
> > so potentially we need to map a single device (like vGPU) to multiple
> > DisplayChannels (each with one monitor). This I think would be the only option
> > to carry on the above formula (which I state again is part of the protocol).
> > Potentially is possible to allocate in advance enough DisplayChannels (on the
> > server) to make possible to bind/unbind the outputs dynamically. This last
> > however won't solve the agent not knowing what a given display_id is and
> > would require some protocol updates (but not between client and server).
> > I think Jonathon, whom is mainly working on multi monitor support, was
> > thinking on not using these mappings but instead using a single DisplayChannel
> > for a single vGPU device even with multiple outputs.
> > Personally I would remove the restriction (mainly implementation, not
> > protocol) of having the primary surface only with surface_id == 0, this
> > was discussed also with Gerd when atomic settings were implemented in Linux
> > driver.
>
> Yes, this is a possibility, let me try to sum up what it would look
> like:
>
> If we use multiple channels and only a single monitor in each of them,
> then along with a QXL device we could have e.g.:
> QXL:            channel_id 0, monitor_id 0
> vGPU monitor 1: channel_id 1, monitor_id 0
> vGPU monitor 2: channel_id 2, monitor_id 0
> vGPU monitor 3: channel_id 3, monitor_id 0
> ...
>
> This would not invalidate assumption ZERO (i.e. having only a single
> channel or multiple channels with only one monitor), therefore we could
> keep using the combined display_id = channel_id ? channel_id :
> monitor_id.
>
> But, since there would still be a QXL device not configured under X,
> the display_id would not match xrandr output IDs in the guest anyway.
> Therefore (unless we solve this in another way, and I haven't seen any
> sensible suggestion yet), we still need the guest_output_id hash table
> on the server, only instead of the (channel_id, monitor_id) pair, we
> could use the combined display_id as the key.
>
> However, I do _not_ want to do this. It is simply not the correct way
> to work with IDs. It is spreading the wrong concept more across the
> code base. It will make the code even more confusing and it's even more
> probable it's going to cause more trouble in the future. (do you need
> more? :))

The argument that the xrandr output ID does not match is flawed. It's
not a protocol issue, it's an implementation issue.

Changing the API/protocol/client is asking for trouble. Adding a
mapping, if necessary, between display_id and channel_id/monitor_id
in the new vdagent or server is less risky.

In your example, even if QXL device is disabled somehow, if you want
to move cursor on vGPU monitor 2, you'll have display_id==2 in current
protocol, or with your proposal channel_id==2 && monitor_id==0. What
ambiguity is removed? (since monitor_id is always 0 in the supported
setup, we know display_id==channel_id)?

>
> > > So do we have to support both multiple display channel & multiple
> > > monitors per display channels? Could you give some details on how this
> > > is going to operate?
>
> Marc-André, tbh. we are having a discussion precisely about this the
> past serveral days. These questions were already answered, why are you
> asking them again and going back to the start of the discussion?

It's not enough to give some answers. You have to proove that all
these risky changes are necessary. And so far, I am not convinced, so
I try to understand the real reasons.

-- 
Marc-André Lureau