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

[Frediano Ziglio]

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

Good question, currently every spice_qxl_gl_scanout is followed by a
spice_qxl_gl_draw_async so there's no much difference but in theory
would make more sense after spice_qxl_gl_scanout.

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

It only is returning a success/failure that was not returned before.
The meaning of the function is the same.

I think allow_now could be converted to bool (follow up) as we discovered
that mouse_mode in this case is TRUE/FALSE.

> >  {
> > -    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.
> 

This is not a regression of this patch and as this patch is fixing a
regression on Virgl I would integrate it.

I tried with Windows. If you have more QXL cards and agent is disabled
client mouse is not working. If you have only a card client mouse is
working with tablet. Note that if you have only a display active and
multiple cards the client mouse does not work either. So there must
another part of code that test if you have only a QXL card.

Maybe the test could be: if you have only a QXL card active and a
tablet client mouse can work (so would cover more cases) but
currently works either with the agent active or single card and
tablet.

> Otherwise it seems to work.
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>
> 
> 

Frediano