On Wed, 2018-08-22 at 11:34 +0200, Marc-André Lureau wrote:
> 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.
How could it happen?
I'm sorry, but I don't really understand this paragraph. If you think
you've got an important point here, please be more specific and
precise.
If you think I'm dragging in some unnecessary ties to specific OSes or
implementations or whatever, please point them out directly...
Cheers,
Lukas
> > > 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
>
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel