Hi
On Thu, Aug 23, 2018 at 11:53 AM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
>
> >
> > Hi
> > On Thu, Aug 23, 2018 at 9:51 AM Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > >
> > > >
> > > > Hi
> > > >
> > > > On Wed, Aug 22, 2018 at 11:11 PM Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > > > ---
> > > > > > 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
That "the agent understand what the display_id was, now this is no
more true" is not an argument to change the protocol.
The protocol supports one display channel with multiple monitor:
display_id is unambigous. Or multiple display channels: the protocol
is also unambigous. The agent implementation issues do not need
protocol changes.
> 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.
>
Then I agree, read my first reply. (but I still don't understand how
is the client going to chose a configuration - if it's done by the
guest/server, then the mapping between display_id and
channel_id/monitor_id can be done there without changing the
protocol).
> > 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
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel