On Wed, 2018-03-21 at 10:51 +0100, Christophe de Dinechin wrote:
> > On 21 Mar 2018, at 10:22, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > On Wed, 2018-03-21 at 08:54 +0100, Christophe de Dinechin wrote:
> > > > On 20 Mar 2018, at 12:14, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > > >
> > > > > > On 19 Mar 2018, at 11:39, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > >
> > > > > > > On 03/13/2018 08:21 AM, Frediano Ziglio wrote:
> > > > > > > > Although not necessary for a single monitor DisplayChannel implementation
> > > > > > > > this make the DiisplayChannels more coherent from the client
> > > > > > > > point of view.
> > > > > > > >
> > > > > > > > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > server/stream-channel.c | 33 ++++++++++++++++++++++++++++++---
> > > > > > > > 1 file changed, 30 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/server/stream-channel.c b/server/stream-channel.c
> > > > > > > > index 3a3b733f..a9882d9c 100644
> > > > > > > > --- a/server/stream-channel.c
> > > > > > > > +++ b/server/stream-channel.c
> > > > > > > > @@ -102,6 +102,7 @@ enum {
> > > > > > > > RED_PIPE_ITEM_TYPE_STREAM_DATA,
> > > > > > > > RED_PIPE_ITEM_TYPE_STREAM_DESTROY,
> > > > > > > > RED_PIPE_ITEM_TYPE_STREAM_ACTIVATE_REPORT,
> > > > > > > > + RED_PIPE_ITEM_TYPE_MONITORS_CONFIG,
> > > > > > > > };
> > > > > > > >
> > > > > > > > typedef struct StreamCreateItem {
> > > > > > > > @@ -204,6 +205,26 @@ fill_base(SpiceMarshaller *m, const StreamChannel
> > > > > > > > *channel)
> > > > > > > > spice_marshall_DisplayBase(m, &base);
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void
> > > > > > > > +marshall_monitors_config(RedChannelClient *rcc, StreamChannel *channel,
> > > > > > > > SpiceMarshaller *m)
> > > > > > > > +{
> > > > > > > > + struct {
> > > > > > > > + SpiceMsgDisplayMonitorsConfig config;
> > > > > > > > + SpiceHead head;
> > > > > > > > + } msg = {
> > > > > > > > + { 1, 1, },
> > > > > > > > + {
> > > > > > > > + 0, PRIMARY_SURFACE_ID,
> > > > > > >
> > > > > > > Hi Frediano,
> > > > > > >
> > > > > > > Why do you send id=0 ?
> > > > > > >
> > > > > > > Uri.
> > > > > > >
> > > > > >
> > > > > > Too much IDs :-)
> > > > > > These IDs are allocated starting from 0 and are per channel so as there
> > > > > > is only a monitor (currently) the ID should be 0.
> > > > > > So this id should not be confused with QXL ID or client/guest ID.
> > > > >
> > > > > If Uri is confused, a comment may be in order.
> > > > >
> > > > > Why so many IDs? Is it just to avoid an ID lookup during initialization?
> > > > >
> > > >
> > > > Not getting the lookup.
> > >
> > > I assume we selected different IDs because they access different arrays, so you can do array[id] instead of array[lookup(id)].
> >
> > If I understand what you mean, that is not the reason and there is no
> > such lookup() function AFAIK…
>
> So what is the reason? Initially, it looks to me like we just took the simple approach that a monitor ID was nothing but an array index.
Yes, that is what it is atm. Note it is also very hard to come up with
something better.
> I understand there is no such lookup. But it looks to me, based on your writeup, that we will need one.
Can you please clarify what a lookup would mean in this case? I thought
I understood how you mean it in general, but I'm not sure what it would
be here.
> >
> > > >
> > > > We have a global idea of the "monitor ID" which is quite confusing and
> > > > prone to different issues. The DisplayChannel (protocol) use channel IDs
> > > > (allocated per channel type starting from 0) and head IDs (allocated per
> > > > channel starting from 0). The client and vdagent use the global monitor ID.
> > > > The client craft these monitor IDs using basically the formula
> > > > "channel ID ? channel ID : head ID" and assuming the vdagent uses the same
> > > > IDs. Currently there are different problems of these assumption/schema.
> > >
> > > Yes, I can imagine the problems. This is confusing.
> > >
> > > But you answered the question of what we have, not the “why”.
> >
> > It may already be clear, but just to make sure, the 0 is there simply
> > because the monitor IDs are a sequence starting from 0 for each display
> > channels. So 0 is simply the ID of the first monitor.
> >
> > If you're asking about the greater purpose "why", I can only guess
> > nobody cared enough to come up with a robust solution, which is what
> > I'm trying to do now, because as the situation complicates, this is
> > starting to fail.
> >
> > > It may be a lookup optimization as indicated above.
> > >
> > > Or it may be that different IDs really refer to different
> > > entities, but then I’m surprised by a computation such as
> > > “channelID ? channelID : headID”
> > > which mixes two kinds of IDs.
> >
> > The IDs indeed refer to different entities. The “channelID ? channelID
> > : headID” computation is on the client, it is clearly wrong and was
> > working so far only under the assumptions that:
> > 1) the monitors on the guest match 1:1 to monitors on the client
> > 2) there is either one display channel with one or more monitors or
> > more display channels with exactly one monitor
> >
> > > If this causes problems, shouldn’t we start by clarifying what a monitor ID should be?
> >
> > Good idea! :)
>
> I knew you would pick up on that one :-)
>
> So my weak understanding at this stage is that we used to have some kind of identity between “monitor as a client window” and “monitor as a guest virtual device head”, and that we need to break this identity and replace it with a 1 to N mapping.
TL;DR: Correct. :)
Long version:
First, let me define some terminology that I'll use in the following
text and anyone please tell me if we should pick a better one:
head: guest virtual device head
monitor: an item in the monitors_config message exchanged between the
server and the client
display: a window on the client displaying the content of some head
from the guest
(note the "display" and "monitor" terminology is used this way in the
client code and "monitor" is used on the server so hopefully it's more
or less consistent)
Yes, what we (still currently) have, can be called an identity between
displays and heads, although it is for the most part in the form of an
assumption :)
Now let me try to break down what we have:
On the server, the display channel IDs are a sequence starting from 0,
in each display channel, there is a monitors_config message sent from
the server to the client, which contains a list of monitors likewise
ID'd in a sequence starting from 0.
There is one display channel per a graphics (QXL) device on the guest
and for the streaming, one display channel with one streamed monitor (I
suppose it's still TBD how we do it for multimonitor streaming).
What does the client do? (I hope I don't write something slightly wrong
here) It will gather the monitors from the display channels and put
them in a single flat array allocated under the main channel. They also
have IDs and the mapping function is the unfortunate
"display_channel_ID ? display_channel_ID : monitor_ID".
There is some interaction around what display is visible/enabled on the
client which I'll skip. On every update to visibility, resize, etc. the
client sends this list to the server on the main channel. The server
Will take this list and update the heads in the guest (either directly
through QXL hotplug events or through the vd_agent).
So this is relying on the fact that no virtual graphics device head is
used in more that one monitor, which we break when streaming with the
Mjpeg fallback and the relation is indeed changed from 1:1 to 1:N for
heads:monitors.
Another use for the unfortunate display ID on the client is when
sending mouse movement events for the client side mouse mode, which is
relative to the display. In this case, the relation of heads:monitors
can be 1:1, but if the assumption that all graphics devices are used by
X is not true, it still breaks, because the vd_agent is interpreting
the IDs once again as an array index for xrandr array of outputs, which
is off.
So as a solution, we need a unique head ID, that can be carried through
the roundtrip of monitors and displays, to identify the head across the
1:N relationship and also in the vd_agent, which may only see a subset
of the heads.
I really hope it makes sense :)
Cheers,
Lukas
> >
> > Cheers,
> > Lukas
> >
> > > > > >
> > > > > > Frediano
> > > > > >
> > > > > > > > + channel->width, channel->height,
> > > > > > > > + 0, 0,
> > > > > > > > + 0 // flags
> > > > > > > > + }
> > > > > > > > + };
> > > > > > > > +
> > > > > > > > + red_channel_client_init_send_data(rcc,
> > > > > > > > SPICE_MSG_DISPLAY_MONITORS_CONFIG);
> > > > > > > > + spice_marshall_msg_display_monitors_config(m, &msg.config);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static void
> > > > > > > > stream_channel_send_item(RedChannelClient *rcc, RedPipeItem *pipe_item)
> > > > > > > > {
> > > > > > > > @@ -229,6 +250,12 @@ stream_channel_send_item(RedChannelClient *rcc,
> > > > > > > > RedPipeItem *pipe_item)
> > > > > > > > spice_marshall_msg_display_surface_create(m, &surface_create);
> > > > > > > > break;
> > > > > > > > }
> > > > > > > > + case RED_PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > > > > > > > + if (!red_channel_client_test_remote_cap(rcc,
> > > > > > > > SPICE_DISPLAY_CAP_MONITORS_CONFIG)) {
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + marshall_monitors_config(rcc, channel, m);
> > > > > > > > + break;
> > > > > > > > case RED_PIPE_ITEM_TYPE_SURFACE_DESTROY: {
> > > > > > > > red_channel_client_init_send_data(rcc,
> > > > > > > > SPICE_MSG_DISPLAY_SURFACE_DESTROY);
> > > > > > > > SpiceMsgSurfaceDestroy surface_destroy = { PRIMARY_SURFACE_ID };
> > > > > > > > @@ -397,7 +424,6 @@ stream_channel_connect(RedChannel *red_channel,
> > > > > > > > RedClient *red_client, RedStream
> > > > > > > > request_new_stream(channel, start);
> > > > > > > >
> > > > > > > >
> > > > > > > > - // TODO set capabilities like SPICE_DISPLAY_CAP_MONITORS_CONFIG
> > > > > > > > // see guest_set_client_capabilities
> > > > > > > > RedChannelClient *rcc = RED_CHANNEL_CLIENT(client);
> > > > > > > > red_channel_client_push_set_ack(rcc);
> > > > > > > > @@ -415,6 +441,7 @@ stream_channel_connect(RedChannel *red_channel,
> > > > > > > > RedClient *red_client, RedStream
> > > > > > > >
> > > > > > > > // pass proper data
> > > > > > > > red_channel_client_pipe_add_type(rcc,
> > > > > > > > RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> > > > > > > > + red_channel_client_pipe_add_type(rcc,
> > > > > > > > RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
> > > > > > > > // surface data
> > > > > > > > red_channel_client_pipe_add_type(rcc,
> > > > > > > > RED_PIPE_ITEM_TYPE_FILL_SURFACE);
> > > > > > > > // TODO monitor configs ??
> > > > > > > > @@ -433,8 +460,7 @@ stream_channel_constructed(GObject *object)
> > > > > > > > client_cbs.connect = stream_channel_connect;
> > > > > > > > red_channel_register_client_cbs(red_channel, &client_cbs, NULL);
> > > > > > > >
> > > > > > > > - // TODO, send monitor to support multiple monitors in the future
> > > > > > > > -// red_channel_set_cap(red_channel,
> > > > > > > > SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > > > > > > > + red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_MONITORS_CONFIG);
> > > > > > > > red_channel_set_cap(red_channel, SPICE_DISPLAY_CAP_STREAM_REPORT);
> > > > > > > >
> > > > > > > > reds_register_channel(reds, red_channel);
> > > > > > > > @@ -489,6 +515,7 @@ stream_channel_change_format(StreamChannel *channel,
> > > > > > > > const StreamMsgFormat *fmt)
> > > > > > > > channel->width = fmt->width;
> > > > > > > > channel->height = fmt->height;
> > > > > > > > red_channel_pipes_add_type(red_channel,
> > > > > > > > RED_PIPE_ITEM_TYPE_SURFACE_CREATE);
> > > > > > > > + red_channel_pipes_add_type(red_channel,
> > > > > > > > RED_PIPE_ITEM_TYPE_MONITORS_CONFIG);
> > > > > > > > // TODO monitors config ??
> > > > > > > > red_channel_pipes_add_empty_msg(red_channel,
> > > > > > > > SPICE_MSG_DISPLAY_MARK);
> > > > > > > > }
> > > > > > > >
> > > >
> > > > Frediano
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> > >
> > > _______________________________________________
> > > Spice-devel mailing list
> > > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel