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

[Marc-André Lureau]

Hi

On Wed, Feb 1, 2017 at 4:16 PM Marc-André Lureau <mlureau at redhat.com> wrote:

>
>
> ----- Original Message -----
> > >
> > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > >
> > > 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, similar to what
> > > red_qxl_get_allow_client_mouse() is doing. The main difference is that
> > > the GL surface doesn't have mouse_mode attached (what for?).
> > >
> > > NB: this code looks like it handles only first QXL instance only, and
> > > uses the whole scanout/surface size for the input table size, perhaps
> > > monitor-configs would be a better choice.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > ---
> > >  server/red-qxl.c | 2 ++
> > >  server/reds.c    | 8 +++++++-
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > > index b6b3770b..ba7b641a 100644
> > > --- a/server/red-qxl.c
> > > +++ b/server/red-qxl.c
> > > @@ -897,6 +897,8 @@ void spice_qxl_gl_scanout(QXLInstance *qxl,
> > >      /* FIXME: find a way to coallesce all pending SCANOUTs */
> > >      dispatcher_send_message(qxl_state->dispatcher,
> > >                              RED_WORKER_MESSAGE_GL_SCANOUT, &payload);
> > > +
> > > +    reds_update_client_mouse_allowed(qxl_state->reds);
> > >  }
> > >
> > >  SPICE_GNUC_VISIBLE
> > > diff --git a/server/reds.c b/server/reds.c
> > > index 40c94851..a89c5e90 100644
> > > --- a/server/reds.c
> > > +++ b/server/reds.c
> > > @@ -4256,10 +4256,16 @@ void reds_update_client_mouse_allowed(RedsState
> > > *reds)
> > >      if (num_active_workers > 0) {
> > >          GListIter it;
> > >          QXLInstance *qxl;
> > > +        SpiceMsgDisplayGlScanoutUnix *gl;
> > >
> > >          allow_now = TRUE;
> > >          FOREACH_QXL_INSTANCE(reds, it, qxl) {
> > > -            if (red_qxl_get_primary_active(qxl)) {
> > > +            if ((gl = red_qxl_get_gl_scanout(qxl))) {
> > > +                x_res = gl->width;
> > > +                y_res = gl->height;
> > > +                red_qxl_put_gl_scanout(qxl, gl);
> > > +                break;
> > > +            } else if (red_qxl_get_primary_active(qxl)) {
> > >                  allow_now = red_qxl_get_allow_client_mouse(qxl,
> &x_res,
> > >                  &y_res);
> > >                  break;
> > >              }
> >
> > First though when I first read this patch was:
> >
> > A: Is your cat red?
> > R: Yes, it's red. I don't have a cat.
> >
> > Why Qemu don't allocate the primary surface in the first place?
>
> Because it no longer uses QXL 2d drawing commands.
>
> > The issue is that multiple code (for instance monitor configs)
> > rely on having a primary surface.
>
> We would need to fix that then, I haven't checked multiple monitors with
> 2.8, I know it used to work not long ago.
>
> > Maybe a lazy allocation of the canvas could help?
>
> What for?
>
> > This patch looks like a workaround for a bug introduced by Qemu.
>
> It depends on spice-server behaviour, which is not enforced at this point.
> Imho, there is no need to create a 2d primary surface if all you do is GL
> scanouts.
>
> > I'm sure it solves the client mouse but I'd like to know what
> > other people knowing better client and multi monitor code/protocol
> > think about not having the primary surface.
> >
> > Reviewed-by: Frediano Ziglio <fziglio at redhat.com>
>

I still have this patch in my queue, and another user reported it fixed the
bug. You proposed an alternative? Could you submit a former patch if you
decline this one in favour of your approach?

Thanks
-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170611/ab2f1d91/attachment-0001.html>