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@xxxxxxxxxx>
---
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 aspice_qxl_gl_draw_async so there's no much difference but in theorywould 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 discoveredthat 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 aregression on Virgl I would integrate it.I tried with Windows. If you have more QXL cards and agent is disabledclient mouse is not working. If you have only a card client mouse isworking with tablet. Note that if you have only a display active andmultiple cards the client mouse does not work either. So there mustanother 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 atablet client mouse can work (so would cover more cases) butcurrently works either with the agent active or single card andtablet.
Based on your testing and Jonathon’s, as well as earlier comments
But still no success with F26 for me (did not expect it either, it’s a crash in GL) Do you have a successful F26 host with 3D guests? If so, is it with the M2000 or another card?
Christophe
|