[Spice-devel] [RFC PATCH spice-protocol v2 04/20] Create a version 2 of the VDAgentMonitorsConfig message

[Lukáš Hrázký]

On Tue, 2018-08-21 at 15:41 -0500, Jonathon Jongsma wrote:
> I'm wondering if this new message is strictly necessary. We *could*
> still support the vgpu scenario with the old message, couldn't we?
> 
> (By the way, I'm not asking whether the old message is *better* or not,
> just whether this change is absolutely necessary to support the
> scenario we're trying to support.)
> 
> As far as I can tell, in the old scenario, the client would send a
> request to the server saying basically:
>  * I want 3 displays. 
>     * The first one should be W1xH1 and located at (X1,Y1)
>     * The second one should be W2xH2 and located at (X2,Y2)
>     * The third one should be W3xH3 and located at (X3,Y3)
> 
> This message is passed on to the vdagent (let's just consider that case
> for now), and the vdagent reconfigures X so that we have 3 displays
> that (mostly) satisfy those requirements. There are a few reasons that
> the new configuration may not match the request exactly. For example:
>  * The exact resolution is not supported by the driver
>  * There is not enough memory to allocate a display that large large
>  * The given coordinates would cause the displays to overlap, so the
>    agent may adjust them to be non-overlapping
> 
> So it's not unreasonable to expect the guest to make some decisions
> about how to achieve the requested configuration.
> 
> With your patch, this changes slightly to:
>  * I want 3 displays
>     * The first one must be guest output ID1 and should be W1xH1 @
>       (X1,Y1)
>     * etc.
> 
> Adding this guest_output_id to the message makes things a bit more
> explicit and perhaps more predictable, but it doesn't seem like it is
> absolutely necessary. In the end, both of these scenarios will result
> in the guest reconfiguring the displays to match the request from the
> client.

You're right, this is not strictly necessary. It is just a more
explicit way of conveying the same information, instead of using the
index of the array, there is an explicit ID member.

> There is a chance that they'll use different guest output IDs
> to accomplish this configuration, but that is easily handled by the
> client, I think.

I don't think this should even happen, and if it did, it would actually
be a problem? Can you think of a scenario when this could happen?

> If my analysis is correct, I wonder if this particular part of the
> series is worth keeping, since the benefit we get from it may not be
> worth the protocol compatibility issues that it introduces. Thoughts?

Yes, I think it can be left out. I kept it partly because it was there
from v1 and also because I think it's a cleaner solution (disregarding
having to maintain compatibility). Except for maybe a bit trickier
crafting of the old VDAgentMonitorsConfig message on the server, there
shouldn't be other issues.

What are people's opinions? I suppose the incentive not to change the
protocol is big... :)

Besides that, we'll probably need to finish the discussion on 01/20
first.

> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
> 
> 
> On Thu, 2018-08-16 at 18:26 +0200, Lukáš Hrázký wrote:
> > To keep compatibility with old endpoints (any of client, server,
> > vd_agent), we need to copy the message to add the guest_output_id
> > field.
> > 
> > The guest_output_id is the guest-side id of the xrandr output (to be
> > precise, it is the index in the list of xrandr outputs) that is set
> > in
> > the monitors config messages by the streaming agent. It is later used
> > in
> > the guest by vd_agent for mouse input and possibly monitors_config
> > (enabling/disabling monitors and setting the resolution/position of
> > monitors).
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  spice/vd_agent.h | 27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > index dda7044..43ec1a0 100644
> > --- a/spice/vd_agent.h
> > +++ b/spice/vd_agent.h
> > @@ -154,6 +154,33 @@ typedef struct SPICE_ATTR_PACKED
> > VDAgentMonitorsConfig {
> >      VDAgentMonConfig monitors[0];
> >  } VDAgentMonitorsConfig;
> >  
> > +typedef struct SPICE_ATTR_PACKED VDAgentMonConfigV2 {
> > +    /* The guest_output_id is the guest-side id of the xrandr output
> > (to be
> > +     * precise, it is the index in the list of xrandr outputs) that
> > is set in
> > +     * the monitors config messages by the streaming agent. It is
> > later used in
> > +     * the guest by vd_agent for mouse input and possibly
> > monitors_config
> > +     * (enabling/disabling monitors and setting the
> > resolution/position of
> > +     * monitors).
> > +     */
> > +    uint32_t guest_output_id;
> > +    /*
> > +     * Note a width and height of 0 can be used to indicate a
> > disabled
> > +     * monitor, this may only be used with agents with the
> > +     * VD_AGENT_CAP_SPARSE_MONITORS_CONFIG capability.
> > +     */
> > +    uint32_t height;
> > +    uint32_t width;
> > +    uint32_t depth;
> > +    int32_t x;
> > +    int32_t y;
> > +} VDAgentMonConfigV2;
> > +
> > +typedef struct SPICE_ATTR_PACKED VDAgentMonitorsConfigV2 {
> > +    uint32_t num_of_monitors;
> > +    uint32_t flags;
> > +    VDAgentMonConfigV2 monitors[0];
> > +} VDAgentMonitorsConfigV2;
> > +
> >  enum {
> >      VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_WALLPAPER = (1 << 0),
> >      VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_FONT_SMOOTH = (1 << 1),