[Spice-devel] [PATCH spice-server v2] gl: fix client mouse mode

[Frediano Ziglio]

> > On 3 Oct 2017, at 15:56, Frediano Ziglio < fziglio at redhat.com > wrote:
> 

> > > > On 7 Sep 2017, at 16:49, Frediano Ziglio < fziglio at redhat.com > wrote:
> > > 
> > 
> 

> > > > Since 2.8, QEMU now longer creates QXL primary surfaces when using GL.
> > > 
> > 
> 

> > > Typo: “no longer"
> > 
> 

> > > > This change broke client-side mouse mode, because Spice server relies
> > > > on
> > > 
> > 
> 
> > > > primary surface conditions.
> > > 
> > 
> 

> > > I do not understand what you mean by “primary surface conditions”?
> > 
> 
> > > Do you mean the state of the primary surface, or specific conditions
> > 
> 
> > > testing the primary surface, or something else?
> > 
> 

> > In this case the existence of such surface.
> 

> Oh, then “Spice server relies on having a primary surface"

> > > > When GL is enabled, use GL scanout informations.
> > > 
> > 
> 
> > > > Mouse mode is always client when GL surfaces are used.
> > > 
> > 
> 

> > > > This patch and most of the message are based on a patch from
> > > 
> > 
> 
> > > > Marc-André Lureau, just moving responsibility from reds to RedQxl.
> > > 
> > 
> 

> > > > Signed-off-by: Frediano Ziglio < fziglio at redhat.com >
> > > 
> > 
> 
> > > > ---
> > > 
> > 
> 
> > > > server/red-qxl.c | 31 +++++++++++++++++++++----------
> > > 
> > 
> 
> > > > server/red-qxl.h | 3 +--
> > > 
> > 
> 
> > > > server/reds.c | 3 +--
> > > 
> > 
> 
> > > > 3 files changed, 23 insertions(+), 14 deletions(-)
> > > 
> > 
> 

> > > > Changes since v1:
> > > 
> > 
> 
> > > > - do not change qxl_state resolution.
> > > 
> > 
> 

> > > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > 
> > 
> 
> > > > index b556428d2..93e7eb7b8 100644
> > > 
> > 
> 
> > > > --- a/server/red-qxl.c
> > > 
> > 
> 
> > > > +++ b/server/red-qxl.c
> > > 
> > 
> 
> > > > @@ -903,6 +903,8 @@ void spice_qxl_gl_draw_async(QXLInstance *qxl,
> > > 
> > 
> 

> > > > qxl_state->gl_draw_async = async_command_alloc(qxl_state, message,
> > > 
> > 
> 
> > > > cookie);
> > > 
> > 
> 
> > > > dispatcher_send_message(qxl_state->dispatcher, message, &draw);
> > > 
> > 
> 
> > > > +
> > > 
> > 
> 
> > > > + reds_update_client_mouse_allowed(qxl_state->reds);
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

> > > > void red_qxl_async_complete(QXLInstance *qxl, AsyncCommand
> > > > *async_command)
> > > 
> > 
> 
> > > > @@ -1022,20 +1024,29 @@ void red_qxl_clear_pending(QXLState *qxl_state,
> > > > int
> > > 
> > 
> 
> > > > pending)
> > > 
> > 
> 
> > > > clear_bit(pending, &qxl_state->pending);
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

> > > > -gboolean red_qxl_get_primary_active(QXLInstance *qxl)
> > > 
> > 
> 
> > > > +bool red_qxl_get_allow_client_mouse(QXLInstance *qxl, int *x_res, int
> > > 
> > 
> 
> > > > *y_res, int *allow_now)
> > > 
> > 
> 

> > > While you are at it, why not make allow_now a bool?
> > 
> 

> > Yes and not. This strange fields came from a mouse_mode field.
> 
> > The confusion on this value is that in different cases mouse_mode
> 
> > means a bit field while in this specific instance the field is used
> 
> > as a boolean (0/1). For compatibility int was used.
> 

> I am confused. In the code I am looking at, “allow_now” is a
> local variable in the caller.

> Or are you talking about primary_active?

Yes, but is filled with use_hardware_cursor which is copied from 
surface->mouse_mode which is uint32_t and confusingly in this 
case a TRUE/FALSE value is used. 
Maybe now that we know that this value is basically a boolean we can 
use mouse_mode as a bool. 
On the other hand if somebody used this as a bit field the behaviour 
can change a bit. 

> > Had recent discussion with Jonathon on this value and turned
> 
> > out is currently always TRUE (1).
> 

> > > > {
> > > 
> > 
> 
> > > > - return qxl->st->primary_active;
> > > 
> > 
> 
> > > > -}
> > > 
> > 
> 
> > > > + // try to get resolution from 3D
> > > 
> > 
> 

> > > I would rephrase this comment to be closer to your commit message log,
> > 
> 
> > > i.e. that with 3D enabled, qemu does not create a QXL primary surface.
> > 
> 

> > Specific suggestions?
> 

> “try to get resolution when 3D enabled, since qemu did not create QXL primary
> surface”

> > > > + SpiceMsgDisplayGlScanoutUnix *gl;
> > > 
> > 
> 
> > > > + if ((gl = red_qxl_get_gl_scanout(qxl))) {
> > > 
> > 
> 
> > > > + *x_res = gl->width;
> > > 
> > 
> 
> > > > + *y_res = gl->height;
> > > 
> > 
> 
> > > > + *allow_now = TRUE;
> > > 
> > 
> 
> > > > + red_qxl_put_gl_scanout(qxl, gl);
> > > 
> > 
> 
> > > > + return true;
> > > 
> > 
> 
> > > > + }
> > > 
> > 
> 
> > > > +
> > > 
> > 
> 
> > > > + // check for 2D
> > > 
> > 
> 
> > > > + if (!qxl->st->primary_active) {
> > > 
> > 
> 
> > > > + return false;
> > > 
> > 
> 
> > > > + }
> > > 
> > 
> 

> > > > -gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res,
> > > 
> > 
> 
> > > > gint *y_res)
> > > 
> > 
> 
> > > > -{
> > > 
> > 
> 
> > > > if (qxl->st->use_hardware_cursor) {
> > > 
> > 
> 
> > > > - if (x_res)
> > > 
> > 
> 
> > > > - *x_res = qxl->st->x_res;
> > > 
> > 
> 
> > > > - if (y_res)
> > > 
> > 
> 
> > > > - *y_res = qxl->st->y_res;
> > > 
> > 
> 
> > > > + *x_res = qxl->st->x_res;
> > > 
> > 
> 
> > > > + *y_res = qxl->st->y_res;
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 
> > > > - return qxl->st->use_hardware_cursor;
> > > 
> > 
> 
> > > > + *allow_now = qxl->st->use_hardware_cursor;
> > > 
> > 
> 
> > > > + return true;
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

> > > Any reason to not set use_hardware_cursor in the 3D case?
> > 
> 

> > For a detailed answer see history in the ML.
> 
> > For a short on: is not valid.
> 

> > > Actually, looking at the rest of the code, I did not understand why we
> > > have
> > > a
> > 
> 
> > > field for that.
> > 
> 

> > I think old cards (QXL ones) do not support hardware mouse so you can't
> 
> > get cursor information (for instance position and shape) from the card.
> 

> I see. Thanks

> > > > void red_qxl_on_ic_change(QXLInstance *qxl, SpiceImageCompression ic)
> > > 
> > 
> 
> > > > diff --git a/server/red-qxl.h b/server/red-qxl.h
> > > 
> > 
> 
> > > > index f925f065b..503ba223d 100644
> > > 
> > 
> 
> > > > --- a/server/red-qxl.h
> > > 
> > 
> 
> > > > +++ b/server/red-qxl.h
> > > 
> > 
> 
> > > > @@ -39,8 +39,7 @@ void red_qxl_async_complete(QXLInstance *qxl,
> > > 
> > 
> 
> > > > AsyncCommand *async_command);
> > > 
> > 
> 
> > > > struct Dispatcher *red_qxl_get_dispatcher(QXLInstance *qxl);
> > > 
> > 
> 
> > > > gboolean red_qxl_use_client_monitors_config(QXLInstance *qxl);
> > > 
> > 
> 
> > > > gboolean red_qxl_client_monitors_config(QXLInstance *qxl,
> > > 
> > 
> 
> > > > VDAgentMonitorsConfig *monitors_config);
> > > 
> > 
> 
> > > > -gboolean red_qxl_get_primary_active(QXLInstance *qxl);
> > > 
> > 
> 
> > > > -gboolean red_qxl_get_allow_client_mouse(QXLInstance *qxl, gint *x_res,
> > > 
> > 
> 
> > > > gint *y_res);
> > > 
> > 
> 
> > > > +bool red_qxl_get_allow_client_mouse(QXLInstance *qxl, int *x_res, int
> > > 
> > 
> 
> > > > *y_res, int *allow_now);
> > > 
> > 
> 
> > > > SpiceMsgDisplayGlScanoutUnix *red_qxl_get_gl_scanout(QXLInstance *qxl);
> > > 
> > 
> 
> > > > void red_qxl_put_gl_scanout(QXLInstance *qxl,
> > > > SpiceMsgDisplayGlScanoutUnix
> > > 
> > 
> 
> > > > *scanout);
> > > 
> > 
> 
> > > > void red_qxl_gl_draw_async_complete(QXLInstance *qxl);
> > > 
> > 
> 
> > > > diff --git a/server/reds.c b/server/reds.c
> > > 
> > 
> 
> > > > index b6ecc6c69..2f4f12ab9 100644
> > > 
> > 
> 
> > > > --- a/server/reds.c
> > > 
> > 
> 
> > > > +++ b/server/reds.c
> > > 
> > 
> 
> > > > @@ -4374,8 +4374,7 @@ void reds_update_client_mouse_allowed(RedsState
> > > 
> > 
> 
> > > > *reds)
> > > 
> > 
> 

> > > > allow_now = TRUE;
> > > 
> > 
> 
> > > > FOREACH_QXL_INSTANCE(reds, qxl) {
> > > 
> > 
> 
> > > > - if (red_qxl_get_primary_active(qxl)) {
> > > 
> > 
> 
> > > > - allow_now = red_qxl_get_allow_client_mouse(qxl, &x_res,
> > > 
> > 
> 
> > > > &y_res);
> > > 
> > 
> 
> > > > + if (red_qxl_get_allow_client_mouse(qxl, &x_res, &y_res,
> > > 
> > 
> 
> > > > &allow_now)) {
> > > 
> > 
> 
> > > > break;
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 
> > > > }
> > > 
> > 
> 

Frediano 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171003/6e4b2716/attachment-0001.html>