[Spice-devel] [RFC PATCH spice-common v2 02/20] A version 2 of the MousePosition message
Frediano Ziglio
fziglio at redhat.com
Thu Aug 23 09:53:08 UTC 2018
>
> Hi
> On Thu, Aug 23, 2018 at 9:51 AM Frediano Ziglio <fziglio at redhat.com> 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".
>
> "channel_id + monitor_id" is wrong: we don't support the combination
> with channel_id != 0.
>
The formula is correct, not wrong, channel_id != 0 is a limitation of the
values that does not make the formula wrong. The || is C is a boolean operator
that would allow to support only two monitors in total. I agree that the +
formula is not enough to describe everything but from the mathematical/C point
of view is more correct than the || version. I would say that the COALESCE
function of some SQL dialect express better what the formula is trying to
do and why.
> > 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.
>
> This is not a good argument. We can reason and agree about the
> protocol, and fix the agent/guest implementation.
>
"This is not a good argument" why? That sentence without any reason does
not make sense to me. In previous e-mail you said we support multiple
devices and I explained why this is not true on Linux. You also said that
that formula is not part of the protocol and I explained why this is not
true and why the formula is part of the protocol. These are good argument
for me. Why are not good for you?
Agree on second sentence, I don't know if "fix" is used correctly here, fix
usually mean that something is broken, but here we are trying to make some
new use cases work in a sensible way, they are fix basically for new use
cases, not strongly regressions (this maybe are not good argument).
> >
> > 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
>
> On linux, a single device with multiple monitors (which is
> recommended), or multiple devices (the QXL & Windows way).
>
Sorry, what are you trying to say here? Are you adding something? Trying
to sum up what I'm saying? Suggesting an implementation?
> > 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.
>
> That doesn't change the argument about MonitorConfig display_id vs
> channel_id & monitor_id.
>
Yes, it does. Can you explain why it doesn't change the argument?
If we need to support multiple monitor for every DisplayChannel the
protocol is currently not able to do it.
> The only reason I have so far is that the agent is not able to map a
> display_id to the correct channel/monitor. Yet, I don't understand why
> and why the protocol would need to change.
>
The above formula is not injective so is not invertible. So display_id 1
can be channel_id == 0 and monitor_id == 1 or channel_id == 1 and monitor_id == 0,
the agent cannot know. This is one problem.
The other problem is, correctly, the mapping (assuming the above formula is now
injective), the new use cases make the mapping display_id -> output not reliable,
the agent needs some additional information to make more reliable and this
requires to extend some part of the protocol in order to get these information.
Maybe we should specify which part of the protocol we are talking about here, by
simply "protocol" I assume any part so SPICE (client <-> server), agent or QXL.
Frediano
More information about the Spice-devel
mailing list