On Wed, 2018-08-22 at 11:53 -0500, Jonathon Jongsma wrote:
> On Wed, 2018-08-22 at 10:41 +0200, Lukáš Hrázký wrote:
> > 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?
>
> You're right, I don't think it can currently happen, and if it did it
> would be a bit of a problem. But it's a theoretical possibility since
> the protocol doesn't put any restrictions on how the configuration
> should happen. The request just says: I want these N displays at this
> location. The vdagent *could* choose to use whichever xrandr outputs it
> wants to achieve this.
>
> Just for fun, Let's see what would happen. Let's say you have two
> displays currently:
>
> display 0:
> ----------
> size: 800x600
> position: (0,0)
> guest output id: 0
>
> display 1:
> ----------
> size: 1024x768
> position: (800,0)
> guest output id: 1
>
> Now the user wants to change the second display to 1440x900. So we send
> a new MonitorsConfig message to the server:
> {
> num_of_monitors = 2,
> flags = $flags,
> monitors = [
> { 800, 600, $depth, 0, 0 },
> { 1440, 900, $depth, 800, 0 }
> ]
> }
>
> Let's say that the vdagent decided to reconfigure things so that guest
> output id 1 was 800x600 and output id 0 was 1440x900. After
> reconfiguring, the server would send a new MonitorsConfig message to
> the client:
> {
> count = 2,
> max_allowed = 16,
> heads = [
> { id = 0, $surface_id, 1440, 900, 800, 0, $flags },
> { id = 1, $surface_id, 800, 600, 0, 0, $flags }
> ]
> }
>
> When the client gets that message, it will adjust its windows to match
> the new sizes and we'll end up with the following scenario:
>
> display 0:
> ----------
> size: 1440x900
> position: (800,0)
> guest output id: 0
>
> display 1:
> ----------
> size: 800x600
> position: (0,0)
> guest output id: 1
>
> Essentially the guest outputs would swap windows on the client. It's
> really bad from a user experience perspective, but things would still
> technically "work", I believe. So my statement above that it "is easily
> handled by the client" is not really true. It would be problematic,
> though not necessarily catastrophic if the vdagent did this.
It's probably moot as this won't and shouldn't happen :) but unless I
misunderstand you the outputs would not swap windows on the client, it
would still be the old outputs with their old content, just their sizes
would be swapped, which would simply be a glitch?
> Of course, the vdagent does not actually do this, and I wouldn't expect
> it to.
Yeah, it doesn't do this. It's essentially using the array index as the
xrandr output ID and nothing too clever (luckily :)).
> > > 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@xxxxxxxxxx>
> > >
> > >
> > >
> > > 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@xxxxxxxxxx>
> > > > ---
> > > > 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),
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel