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

[Jonathon Jongsma]

On Thu, 2017-09-07 at 15:49 +0100, Frediano Ziglio wrote:
> Since 2.8, QEMU now longer creates QXL primary surfaces when using
> GL.
> This change broke client-side mouse mode, because Spice server relies
> on
> primary surface conditions.
> 
> 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);
>  }

Why do you update client mouse allowed here instead of in
spice_qxl_gl_scanout()? Since _allow_client_mouse() only really depends
on the scanout, it doesn't seem like the calls to
spice_qxl_gl_draw_async() should affect it? Or am I missing something?

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

So I feel that changing the signature of
red_qxl_get_allow_client_mouse() makes things a bit more complicated to
follow. I understand that you did it in order to maintain the exact
behavior as before, but it feels like there should be a better way. It
also made me examine the code more closely (see below)

>  {
> -    return qxl->st->primary_active;
> -}
> +    // try to get resolution from 3D
> +    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;
>  }
>  
>  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;
>              }

So, this is not a commentary on your patch, but I'm trying to
understand the objective here. Let's assume we have multiple 2D qxl
devices here (i.e. the windows case). What we seem to be doing is
finding the first qxl device that has an active primary surface (which
will be the last device added since devices are prepended to the list),
and then checking whether that device allows client mouse, and ignoring
all other devices. It also uses the resolution of that device to set
the resolution of the tablet device. That seems very odd to me.

Otherwise it seems to work.

Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>