[Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config
Christophe de Dinechin
dinechin at redhat.com
Wed Mar 21 09:51:20 UTC 2018
> On 21 Mar 2018, at 10:22, Lukáš Hrázký <lhrazky at redhat.com> wrote:
>
> On Wed, 2018-03-21 at 08:54 +0100, Christophe de Dinechin wrote:
>>> On 20 Mar 2018, at 12:14, Frediano Ziglio <fziglio at redhat.com> wrote:
>>>
>>>>> On 19 Mar 2018, at 11:39, Frediano Ziglio <fziglio at redhat.com> 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 at redhat.com>
>>>>>>> ---
>>>>>>> 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.
I understand there is no such lookup. But it looks to me, based on your writeup, that we will need one.
>
>>>
>>> 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.
>
> 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list