[Spice-devel] [PATCH spice-server v2] gl: fix client mouse mode
Christophe de Dinechin
cdupontd at redhat.com
Wed Oct 4 08:07:12 UTC 2017
> On 29 Sep 2017, at 09:52, Frediano Ziglio <fziglio at redhat.com> wrote:
>
>>
>> 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 <mailto:jjongsma at redhat.com>>
Based on your testing and Jonathon’s, as well as earlier comments
Acked-by: Christophe de Dinechin <dinechin at redhat.com>
But still no success with F26 for me (did not expect it either, it’s a crash in GL)
https://bugzilla.redhat.com/show_bug.cgi?id=1496766
Do you have a successful F26 host with 3D guests? If so, is it with the M2000
or another card?
Christophe
>>
>>
>
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20171004/53e2e1f8/attachment-0001.html>
More information about the Spice-devel
mailing list