Re: [PATCH spice-server] stream-channel: Implements monitors_config

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...

> > 
> > 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! :)

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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]