[Spice-devel] [PATCH spice-server] stream-channel: Implements monitors_config
Christophe de Dinechin
dinechin at redhat.com
Wed Mar 21 07:54:17 UTC 2018
> 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)].
>
> 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 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.
If this causes problems, shouldn’t we start by clarifying what a monitor ID should be?
>>>
>>> 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
More information about the Spice-devel
mailing list