Hi
On Wed, Aug 22, 2018 at 10:41 AM Lukáš Hrázký <lhrazky@xxxxxxxxxx> 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.
It's a redundant information, not worth a protocol change.
>
> > 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?
It could happen. From a _user_ point of view, it didn't matter so much
how you use channel X or Y, as long as you get the requested
configuration. Now, if we have a channel associated with different
devices with different capabilities, it may make sense to specify the
channel. (not the xrandr output! - please be OS agnostic when making
protocol changes). However, the client will lack details about the
channels to make inform decision... Instead the server/guest may do a
best guess without changing the protocol/client.
>
> > 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... :)
Please get rid of all the unnecessary changes (it's not the first time
you say you have leftovers in your series): make sure to introduce the
minimum amount of API and protocol breakage, this should be a priority
before introducing something new.
>
> 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
--
Marc-André Lureau
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel