On Thu, 2018-08-23 at 11:27 +0200, Lukáš Hrázký wrote:
> 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?
Well, I can't be certain without testing it. But I suspect that after
the monitor reconfiguration, the surfaces will probably be destroyed
and will create new surfaces for the displays. So I suspect that the
contents would also be swapped. But I can't be sure, and as you say,
it's kind of a moot point.
>
> > 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