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@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