Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
On Mon, Oct 10, 2016 at 08:37:04AM -0400, Marc-André Lureau wrote:
> Imho, it should be fine for the client to send the same monitor
> config, the server should however not notify of changes if none
> happened.
Yup, after quite a lot of digging, this seems to be what is happening,
nothing in the spice-server->QEMU->kernel qxl->mutter is checking
whether the configuration we get is the same as the one which we had.
(the kernel has some checks for that, except that the way the resolution
changes is done in mutter disable these checks)
The way resolution changes happen in mutter are going to trigger a
surface destroy/create. Which gets us into this flicker loop:
- the client sends a MonitorsConfig message
- spice-server gets it, calls a QEMU callback
- QEMU raises an interrupt to inform the guest
- the kernel catches it and sends a udev event to userspace
- mutter catches this, and this triggers a resolution change
- the kernel does the resolution change, which involves a
surface destroy/create
- the client receives the surface destroy/create, which will cause
a MonitorsConfig message to be sent, so we are back to step 1
Checking whether we are getting an unchanged MonitorsConfig message
anywehere in qemu/kernel/mutter would solve (avoid ?) this bug, still
not fully sure where the best place is. Maybe best to do it once
host-side, and once guest-side.
Kernel-side, the check would go in qxl_display.c in
qxl_display_read_client_monitors_config()
mutter-side, the check would belong in the call chain starting at on_uevent() in
backends/native/meta-monitor-manager-kms.c
In QEMU, the patch below fixes the flickering for me:
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 0e2682d..3530aeb 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1049,6 +1049,7 @@ static int interface_client_monitors_config(QXLInstance
*sin,
rect->right = monitor->x + monitor->width;
rect->bottom = monitor->y + monitor->height;
}
+uint32_t old_crc = rom->client_monitors_config_crc;
rom->client_monitors_config_crc = qxl_crc32(
(const uint8_t *)>client_monitors_config,
sizeof(rom->client_monitors_config));
@@ -1059,7 +1060,9 @@ static int interface_client_monitors_config(QXLInstance
*sin,
trace_qxl_interrupt_client_monitors_config(qxl->id,
rom->client_monitors_config.count,
rom->client_monitors_config.heads);
-qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+if (rom->client_monitors_config_crc != old_crc) {
+qxl_send_events(qxl, QXL_INTERRUPT_CLIENT_MONITORS_CONFIG);
+}
return 1;
}
Christophe
signature.asc
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
On Tue, Oct 11, 2016 at 04:56:42PM +0200, Pavel Grunt wrote: > On Tue, 2016-10-11 at 10:36 -0400, Marc-André Lureau wrote: > > > > > > Flickering is PRIMARY_DESTROY & PRIMARY_CREATE > > > > Is there a way to prevent DESTROY & CREATE of same size on > > server/guest side? why not? > > The problem is that it is not the same size, since client requests 401 > and driver creates 400. > > Maybe it would be possible to delay the DESTROY to the moment the new > surface is created and send it, as you said, only if there is a > change. For what it's worth, this PRIMARY_DESTROY/PRIMARY_CREATE seems to be more of a side-effect of how mutter/clutter do the resizing (using drmModeRmFB/drmModeAddFB) than being related to that size difference. Destroying the framebuffer used kernel-side causes the subsequent drmModeSetCrtc call to trigger a full modeset without checking if it's the same as the current mode. This also causes qxl_crtc_mode_set to emit a DESTROY/CREATE pair as the newly created framebuffer is not marked as being the primary one (not familiar enough with drm/clutter to know whether that's a reasonable behaviour or not). Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
On Tue, 2016-10-11 at 10:36 -0400, Marc-André Lureau wrote:
>
> - Original Message -
> > Hi,
> >
> > On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > - Original Message -
> > > > When the guest receives the monitor configuration message, it
> > > > replies
> > > > (through spice-server) by destroying the primary surface,
> > > > which
> > > > makes
> > > > the SpiceDisplay disabled if its "resize-guest" property is
> > > > used.
> > > > This change of the display state (disabled/enabled) leads to
> > > > sending
> > > > a new monitor config message (containing the disabled display)
> > > > after
> > > > a second. Then spice-server sends the monitor configuration
> > > > message,
> > > > spice-gtk replies back and we may end up with a loop of
> > > > monitor
> > > > configuration messages even if no real change of monitor
> > > > configuration
> > > > happens.
> > >
> > > It's hard follow, could you explain step by step what you mean.
> >
> > sorry, I overlooked your question
> >
> > I did more investigation and following is happening from clients
> > perspective:
> > 1. let window width be 401
> > 2. client sends request to resize
> > 3. client receives PRIMARY_DESTROY
> > 4. client marks the display as disabled (config in the main
> > channel)
> > 5. client receives PRIMARY_CREATE + dimensions of display - width
> > is
> > 400
> > 6. client sends a new resize request since resize-guest is on, and
> > display width != window width
> >
> > Flickering is PRIMARY_DESTROY & PRIMARY_CREATE
>
> Is there a way to prevent DESTROY & CREATE of same size on
> server/guest side? why not?
The problem is that it is not the same size, since client requests 401
and driver creates 400.
Maybe it would be possible to delay the DESTROY to the moment the new
surface is created and send it, as you said, only if there is a
change.
Pavel
> >
> > > > To avoid the loop, the new monitor configuration messages are
> > > > only
> > > > sent
> > > > if they are different from the current monitor configuration.
> > > >
> > > > All the issues are visible only with Wayland & QXL driver on
> > > > the
> > > > guest
> > > >
> > > > Fixes:
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=94950
> > > > ---
> > > > v3: rebased, fixed typos
> > >
> > > In v2, Jonathon raised a valid concern: "However, it's
> > > theoretically
> > > possible that the last sent configuration may not have been
> > > successful." For ex, vdagent wasn't yet started (whatever
> > > handles
> > > the monitor config change).
> > >
> > > Imho, it should be fine for the client to send the same monitor
> > > config, the server should however not notify of changes if none
> > > happened.
> > >
> > > > v2:
> > > > https://lists.freedesktop.org/archives/spice-devel/2015-Octobe
> > > > r/02
> > > > 2784.html
> > > > ---
> > > > src/channel-main.c | 59
> > > > +++---
> > > > 1 file changed, 38 insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > > index 990a06a..1a46819 100644
> > > > --- a/src/channel-main.c
> > > > +++ b/src/channel-main.c
> > > > @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> > > > guint agent_msg_pos;
> > > > uint8_t agent_msg_size;
> > > > uint32_tagent_caps[VD_AGENT_CAPS_SIZE
> > > > ];
> > > > -SpiceDisplayConfig display[MAX_DISPLAY];
> > > > +SpiceDisplayConfig display_current[MAX_DISPLAY];
> > > > /*
> > > > current
> > > > configuration of spice-widgets */
> > > > +SpiceDisplayConfig display_new[MAX_DISPLAY]; /*
> > > > new
> > > > requested
> > > > configuration */
> > > > ginttimer_id;
> > > > GQueue *agent_msg_queue;
> > > > GHashTable *file_xfer_tasks;
> > > > @@ -1098,11 +1099,11 @@ gboolean
> > > > spice_main_send_monitor_config(SpiceMainChannel *channel)
> > > >
> > > > if (spice_main_agent_test_capability(channel,
> > > > VD_AGENT_CAP_SPARSE_MONI
> > > > TORS
> > > > _CONFIG)) {
> > > > -monitors = SPICE_N_ELEMENTS(c->display);
> > > > +monitors = SPICE_N_ELEMENTS(c->display_new);
> > > > } else {
> > > > monitors = 0;
> > > > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > > > -if (c->display[i].display_state ==
> > > > DISPLAY_ENABLED)
> > > > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new);
> > > > i++) {
> > > > +if (c->display_new[i].display_state ==
> > > > DISPLAY_ENABLED)
> > > > monitors += 1;
> > > > }
> > > > }
> > > > @@ -1117,24 +1118,25 @@ gboolean
> > > > spice_main_send_monitor_config(SpiceMainChannel *channel)
> > > >
> > > >
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
- Original Message -
> Hi,
>
> On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote:
> > Hi
> >
> > - Original Message -
> > > When the guest receives the monitor configuration message, it
> > > replies
> > > (through spice-server) by destroying the primary surface, which
> > > makes
> > > the SpiceDisplay disabled if its "resize-guest" property is used.
> > > This change of the display state (disabled/enabled) leads to
> > > sending
> > > a new monitor config message (containing the disabled display)
> > > after
> > > a second. Then spice-server sends the monitor configuration
> > > message,
> > > spice-gtk replies back and we may end up with a loop of monitor
> > > configuration messages even if no real change of monitor
> > > configuration
> > > happens.
> >
> > It's hard follow, could you explain step by step what you mean.
>
> sorry, I overlooked your question
>
> I did more investigation and following is happening from clients
> perspective:
> 1. let window width be 401
> 2. client sends request to resize
> 3. client receives PRIMARY_DESTROY
> 4. client marks the display as disabled (config in the main channel)
> 5. client receives PRIMARY_CREATE + dimensions of display - width is
> 400
> 6. client sends a new resize request since resize-guest is on, and
> display width != window width
>
> Flickering is PRIMARY_DESTROY & PRIMARY_CREATE
Is there a way to prevent DESTROY & CREATE of same size on server/guest side?
why not?
>
> Pavel
>
> > > To avoid the loop, the new monitor configuration messages are only
> > > sent
> > > if they are different from the current monitor configuration.
> > >
> > > All the issues are visible only with Wayland & QXL driver on the
> > > guest
> > >
> > > Fixes:
> > > https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94950
> > > ---
> > > v3: rebased, fixed typos
> >
> > In v2, Jonathon raised a valid concern: "However, it's theoretically
> > possible that the last sent configuration may not have been
> > successful." For ex, vdagent wasn't yet started (whatever handles
> > the monitor config change).
> >
> > Imho, it should be fine for the client to send the same monitor
> > config, the server should however not notify of changes if none
> > happened.
> >
> > > v2:
> > > https://lists.freedesktop.org/archives/spice-devel/2015-October/02
> > > 2784.html
> > > ---
> > > src/channel-main.c | 59
> > > +++---
> > > 1 file changed, 38 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/src/channel-main.c b/src/channel-main.c
> > > index 990a06a..1a46819 100644
> > > --- a/src/channel-main.c
> > > +++ b/src/channel-main.c
> > > @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> > > guint agent_msg_pos;
> > > uint8_t agent_msg_size;
> > > uint32_tagent_caps[VD_AGENT_CAPS_SIZE];
> > > -SpiceDisplayConfig display[MAX_DISPLAY];
> > > +SpiceDisplayConfig display_current[MAX_DISPLAY]; /*
> > > current
> > > configuration of spice-widgets */
> > > +SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new
> > > requested
> > > configuration */
> > > ginttimer_id;
> > > GQueue *agent_msg_queue;
> > > GHashTable *file_xfer_tasks;
> > > @@ -1098,11 +1099,11 @@ gboolean
> > > spice_main_send_monitor_config(SpiceMainChannel *channel)
> > >
> > > if (spice_main_agent_test_capability(channel,
> > > VD_AGENT_CAP_SPARSE_MONITORS
> > > _CONFIG)) {
> > > -monitors = SPICE_N_ELEMENTS(c->display);
> > > +monitors = SPICE_N_ELEMENTS(c->display_new);
> > > } else {
> > > monitors = 0;
> > > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > > -if (c->display[i].display_state == DISPLAY_ENABLED)
> > > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > > +if (c->display_new[i].display_state ==
> > > DISPLAY_ENABLED)
> > > monitors += 1;
> > > }
> > > }
> > > @@ -1117,24 +1118,25 @@ gboolean
> > > spice_main_send_monitor_config(SpiceMainChannel *channel)
> > >
> > > CHANNEL_DEBUG(channel, "sending new monitors config to
> > > guest");
> > > j = 0;
> > > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > > -if (c->display[i].display_state != DISPLAY_ENABLED) {
> > > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > > +if (c->display_new[i].display_state != DISPLAY_ENABLED) {
> > > if (spice_main_agent_test_capability(channel,
> > > VD_AGENT_CAP_SPARSE_MONITORS
> > > _CONFIG))
> > > j++;
> > > continue;
> > > }
> > > mon->monitors[j].depth =
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Hi,
On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote:
> Hi
>
> - Original Message -
> > When the guest receives the monitor configuration message, it
> > replies
> > (through spice-server) by destroying the primary surface, which
> > makes
> > the SpiceDisplay disabled if its "resize-guest" property is used.
> > This change of the display state (disabled/enabled) leads to
> > sending
> > a new monitor config message (containing the disabled display)
> > after
> > a second. Then spice-server sends the monitor configuration
> > message,
> > spice-gtk replies back and we may end up with a loop of monitor
> > configuration messages even if no real change of monitor
> > configuration
> > happens.
>
> It's hard follow, could you explain step by step what you mean.
sorry, I overlooked your question
I did more investigation and following is happening from clients
perspective:
1. let window width be 401
2. client sends request to resize
3. client receives PRIMARY_DESTROY
4. client marks the display as disabled (config in the main channel)
5. client receives PRIMARY_CREATE + dimensions of display - width is
400
6. client sends a new resize request since resize-guest is on, and
display width != window width
Flickering is PRIMARY_DESTROY & PRIMARY_CREATE
Pavel
> > To avoid the loop, the new monitor configuration messages are only
> > sent
> > if they are different from the current monitor configuration.
> >
> > All the issues are visible only with Wayland & QXL driver on the
> > guest
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> > https://bugs.freedesktop.org/show_bug.cgi?id=94950
> > ---
> > v3: rebased, fixed typos
>
> In v2, Jonathon raised a valid concern: "However, it's theoretically
> possible that the last sent configuration may not have been
> successful." For ex, vdagent wasn't yet started (whatever handles
> the monitor config change).
>
> Imho, it should be fine for the client to send the same monitor
> config, the server should however not notify of changes if none
> happened.
>
> > v2:
> > https://lists.freedesktop.org/archives/spice-devel/2015-October/02
> > 2784.html
> > ---
> > src/channel-main.c | 59
> > +++---
> > 1 file changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 990a06a..1a46819 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> > guint agent_msg_pos;
> > uint8_t agent_msg_size;
> > uint32_tagent_caps[VD_AGENT_CAPS_SIZE];
> > -SpiceDisplayConfig display[MAX_DISPLAY];
> > +SpiceDisplayConfig display_current[MAX_DISPLAY]; /*
> > current
> > configuration of spice-widgets */
> > +SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new
> > requested
> > configuration */
> > ginttimer_id;
> > GQueue *agent_msg_queue;
> > GHashTable *file_xfer_tasks;
> > @@ -1098,11 +1099,11 @@ gboolean
> > spice_main_send_monitor_config(SpiceMainChannel *channel)
> >
> > if (spice_main_agent_test_capability(channel,
> > VD_AGENT_CAP_SPARSE_MONITORS
> > _CONFIG)) {
> > -monitors = SPICE_N_ELEMENTS(c->display);
> > +monitors = SPICE_N_ELEMENTS(c->display_new);
> > } else {
> > monitors = 0;
> > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > -if (c->display[i].display_state == DISPLAY_ENABLED)
> > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > +if (c->display_new[i].display_state ==
> > DISPLAY_ENABLED)
> > monitors += 1;
> > }
> > }
> > @@ -1117,24 +1118,25 @@ gboolean
> > spice_main_send_monitor_config(SpiceMainChannel *channel)
> >
> > CHANNEL_DEBUG(channel, "sending new monitors config to
> > guest");
> > j = 0;
> > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > -if (c->display[i].display_state != DISPLAY_ENABLED) {
> > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > +if (c->display_new[i].display_state != DISPLAY_ENABLED) {
> > if (spice_main_agent_test_capability(channel,
> > VD_AGENT_CAP_SPARSE_MONITORS
> > _CONFIG))
> > j++;
> > continue;
> > }
> > mon->monitors[j].depth = c->display_color_depth ?
> > c->display_color_depth : 32;
> > -mon->monitors[j].width = c->display[i].width;
> > -mon->monitors[j].height = c->display[i].height;
> > -mon->monitors[j].x = c->display[i].x;
> > -mon->monitors[j].y = c->display[i].y;
> > +mon->monitors[j].width = c->display_new[i].width;
> > +
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Hey, On Mon, Oct 10, 2016 at 05:12:53PM +0200, Pavel Grunt wrote: > On Mon, 2016-10-10 at 17:03 +0200, Christophe Fergeau wrote: > > Hey, > > > > On Mon, Oct 10, 2016 at 04:09:11PM +0200, Pavel Grunt wrote: > > > The configuration messages are sent only when agent is running. > > > > > > It is happening all the time that the configuration message was > > > technically not successful (linux guest supports only width of > > > multiple of 8) and in fact it is the reason of this flickering > > > bug. > > > > "linux guests supports only width of multiple of 8", I assume you > > mean > > when trying to set the resolution through direct use of KMS? Do you > > know > > why this seems to be working fine when the Xorg QXL driver is in > > use? > > I believe the gnome-settings-deamon and mutter are involved in this. In both cases, mutter is involved, though things are going through 2 different code paths, meta-monitor-manager-xrandr.c VS meta-monitor-manager-kms.c (and in the latter case some DRM ioctl failures can indeed be seen, I did not check yet what the xrandr code is doing). > gsd has problems getting edid for qxl under wayland, but I don't know > why Oh, edid has been removed even from the QXL xorg driver a while ago, I would not be surprised if we did not have any EDID information anywhere ( see https://cgit.freedesktop.org/xorg/driver/xf86-video-qxl/commit/?id=96e6be278896ea6ecb43984d7e6fe8eea3b75ab1 ) > > > > What is destroying the primary surface guest-side though? I guess > > this > > happens in the kernel KMS driver as part of the attempted resolution > > change? > > yes I think if the resolution change is going to fail, then the primary surface should not be destroyed. Dunno how unrelated the 2 events are... This would fix the blinking, but I suspect we'd keep attempting to change the resolution and fail. > > > > > The other option is to stop using QXL for kms and the patch can be > > > dropped. > > > > I guess you mean stop using a QXL device when wayland is in use? I > > don't > > think we can realistically do that. > > > No I meant to stop using qxl for fedora24+ and start using virtio-gpu > instead We could recommend not using qxl for f24+, but it's going to be hard to prevent it totally, and you cannot change it for existing VMs which are upgraded from f23 to f24 for example. I'd rather than we fix this :) Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
On Mon, 2016-10-10 at 17:03 +0200, Christophe Fergeau wrote: > Hey, > > On Mon, Oct 10, 2016 at 04:09:11PM +0200, Pavel Grunt wrote: > > The configuration messages are sent only when agent is running. > > > > It is happening all the time that the configuration message was > > technically not successful (linux guest supports only width of > > multiple of 8) and in fact it is the reason of this flickering > > bug. > > "linux guests supports only width of multiple of 8", I assume you > mean > when trying to set the resolution through direct use of KMS? Do you > know > why this seems to be working fine when the Xorg QXL driver is in > use? I believe the gnome-settings-deamon and mutter are involved in this. gsd has problems getting edid for qxl under wayland, but I don't know why > > > > , the server should however not notify of changes if none > > > happened. > > > > (the change has happened - the primary surface was destroyed) > > > > iow some component should just ignore the size request - it can be > > client, server or driver (eg. Windows driver ignores that, QXL > > +Xorg > > driver ignores that). > > > > Imho server does a correct job - it gets info from the driver that > > the > > primary surface was destroyed and it forwards the message to the > > client. > > What is destroying the primary surface guest-side though? I guess > this > happens in the kernel KMS driver as part of the attempted resolution > change? yes > > > The other option is to stop using QXL for kms and the patch can be > > dropped. > > I guess you mean stop using a QXL device when wayland is in use? I > don't > think we can realistically do that. No I meant to stop using qxl for fedora24+ and start using virtio-gpu instead Pavel > > Christophe ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Hey, On Mon, Oct 10, 2016 at 04:09:11PM +0200, Pavel Grunt wrote: > The configuration messages are sent only when agent is running. > > It is happening all the time that the configuration message was > technically not successful (linux guest supports only width of > multiple of 8) and in fact it is the reason of this flickering bug. "linux guests supports only width of multiple of 8", I assume you mean when trying to set the resolution through direct use of KMS? Do you know why this seems to be working fine when the Xorg QXL driver is in use? > > , the server should however not notify of changes if none happened. > (the change has happened - the primary surface was destroyed) > > iow some component should just ignore the size request - it can be > client, server or driver (eg. Windows driver ignores that, QXL +Xorg > driver ignores that). > > Imho server does a correct job - it gets info from the driver that the > primary surface was destroyed and it forwards the message to the > client. What is destroying the primary surface guest-side though? I guess this happens in the kernel KMS driver as part of the attempted resolution change? > The other option is to stop using QXL for kms and the patch can be > dropped. I guess you mean stop using a QXL device when wayland is in use? I don't think we can realistically do that. Christophe signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Hi Marc-André,
On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote:
> Hi
>
> - Original Message -
> > When the guest receives the monitor configuration message, it
> > replies
> > (through spice-server) by destroying the primary surface, which
> > makes
> > the SpiceDisplay disabled if its "resize-guest" property is used.
> > This change of the display state (disabled/enabled) leads to
> > sending
> > a new monitor config message (containing the disabled display)
> > after
> > a second. Then spice-server sends the monitor configuration
> > message,
> > spice-gtk replies back and we may end up with a loop of monitor
> > configuration messages even if no real change of monitor
> > configuration
> > happens.
>
> It's hard follow, could you explain step by step what you mean.
> > To avoid the loop, the new monitor configuration messages are only
> > sent
> > if they are different from the current monitor configuration.
> >
> > All the issues are visible only with Wayland & QXL driver on the
> > guest
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> > https://bugs.freedesktop.org/show_bug.cgi?id=94950
> > ---
> > v3: rebased, fixed typos
>
> In v2, Jonathon raised a valid concern: "However, it's theoretically
> possible that the last sent configuration may not have been
> successful." For ex, vdagent wasn't yet started (whatever handles
> the monitor config change).
The configuration messages are sent only when agent is running.
It is happening all the time that the configuration message was
technically not successful (linux guest supports only width of
multiple of 8) and in fact it is the reason of this flickering bug.
The problem is that client has no info that the message was
unsuccessful.
>
> Imho, it should be fine for the client to send the same monitor
> config
How should client know that it should resend the message ? Would it
require a new message ?
> , the server should however not notify of changes if none happened.
(the change has happened - the primary surface was destroyed)
iow some component should just ignore the size request - it can be
client, server or driver (eg. Windows driver ignores that, QXL +Xorg
driver ignores that).
Imho server does a correct job - it gets info from the driver that the
primary surface was destroyed and it forwards the message to the
client.
I think this patch is just a workaround of the real root cause. It is
only GNOME on Wayland having the issue (iirc QXL was blacklisted by
GNOME - maybe it is a part of the problem).
The other option is to stop using QXL for kms and the patch can be
dropped.
Jonathon and You are right that there is a risk of introducing a
regression - we don't have tests for this area
Pavel
>
> > v2:
> > https://lists.freedesktop.org/archives/spice-devel/2015-October/02
> > 2784.html
> > ---
> > src/channel-main.c | 59
> > +++---
> > 1 file changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/channel-main.c b/src/channel-main.c
> > index 990a06a..1a46819 100644
> > --- a/src/channel-main.c
> > +++ b/src/channel-main.c
> > @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> > guint agent_msg_pos;
> > uint8_t agent_msg_size;
> > uint32_tagent_caps[VD_AGENT_CAPS_SIZE];
> > -SpiceDisplayConfig display[MAX_DISPLAY];
> > +SpiceDisplayConfig display_current[MAX_DISPLAY]; /*
> > current
> > configuration of spice-widgets */
> > +SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new
> > requested
> > configuration */
> > ginttimer_id;
> > GQueue *agent_msg_queue;
> > GHashTable *file_xfer_tasks;
> > @@ -1098,11 +1099,11 @@ gboolean
> > spice_main_send_monitor_config(SpiceMainChannel *channel)
> >
> > if (spice_main_agent_test_capability(channel,
> > VD_AGENT_CAP_SPARSE_MONITORS
> > _CONFIG)) {
> > -monitors = SPICE_N_ELEMENTS(c->display);
> > +monitors = SPICE_N_ELEMENTS(c->display_new);
> > } else {
> > monitors = 0;
> > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > -if (c->display[i].display_state == DISPLAY_ENABLED)
> > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > +if (c->display_new[i].display_state ==
> > DISPLAY_ENABLED)
> > monitors += 1;
> > }
> > }
> > @@ -1117,24 +1118,25 @@ gboolean
> > spice_main_send_monitor_config(SpiceMainChannel *channel)
> >
> > CHANNEL_DEBUG(channel, "sending new monitors config to
> > guest");
> > j = 0;
> > -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> > -if (c->display[i].display_state != DISPLAY_ENABLED) {
> > +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> > +if
Re: [Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Hi
- Original Message -
> When the guest receives the monitor configuration message, it replies
> (through spice-server) by destroying the primary surface, which makes
> the SpiceDisplay disabled if its "resize-guest" property is used.
> This change of the display state (disabled/enabled) leads to sending
> a new monitor config message (containing the disabled display) after
> a second. Then spice-server sends the monitor configuration message,
> spice-gtk replies back and we may end up with a loop of monitor
> configuration messages even if no real change of monitor configuration
> happens.
It's hard follow, could you explain step by step what you mean.
> To avoid the loop, the new monitor configuration messages are only sent
> if they are different from the current monitor configuration.
>
> All the issues are visible only with Wayland & QXL driver on the guest
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> https://bugs.freedesktop.org/show_bug.cgi?id=94950
> ---
> v3: rebased, fixed typos
In v2, Jonathon raised a valid concern: "However, it's theoretically possible
that the last sent configuration may not have been successful." For ex, vdagent
wasn't yet started (whatever handles the monitor config change).
Imho, it should be fine for the client to send the same monitor config, the
server should however not notify of changes if none happened.
> v2:
> https://lists.freedesktop.org/archives/spice-devel/2015-October/022784.html
> ---
> src/channel-main.c | 59
> +++---
> 1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 990a06a..1a46819 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> guint agent_msg_pos;
> uint8_t agent_msg_size;
> uint32_tagent_caps[VD_AGENT_CAPS_SIZE];
> -SpiceDisplayConfig display[MAX_DISPLAY];
> +SpiceDisplayConfig display_current[MAX_DISPLAY]; /* current
> configuration of spice-widgets */
> +SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new requested
> configuration */
> ginttimer_id;
> GQueue *agent_msg_queue;
> GHashTable *file_xfer_tasks;
> @@ -1098,11 +1099,11 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>
> if (spice_main_agent_test_capability(channel,
> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> -monitors = SPICE_N_ELEMENTS(c->display);
> +monitors = SPICE_N_ELEMENTS(c->display_new);
> } else {
> monitors = 0;
> -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> -if (c->display[i].display_state == DISPLAY_ENABLED)
> +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> +if (c->display_new[i].display_state == DISPLAY_ENABLED)
> monitors += 1;
> }
> }
> @@ -1117,24 +1118,25 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>
> CHANNEL_DEBUG(channel, "sending new monitors config to guest");
> j = 0;
> -for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> -if (c->display[i].display_state != DISPLAY_ENABLED) {
> +for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> +if (c->display_new[i].display_state != DISPLAY_ENABLED) {
> if (spice_main_agent_test_capability(channel,
> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
> j++;
> continue;
> }
> mon->monitors[j].depth = c->display_color_depth ?
> c->display_color_depth : 32;
> -mon->monitors[j].width = c->display[i].width;
> -mon->monitors[j].height = c->display[i].height;
> -mon->monitors[j].x = c->display[i].x;
> -mon->monitors[j].y = c->display[i].y;
> +mon->monitors[j].width = c->display_new[i].width;
> +mon->monitors[j].height = c->display_new[i].height;
> +mon->monitors[j].x = c->display_new[i].x;
> +mon->monitors[j].y = c->display_new[i].y;
> CHANNEL_DEBUG(channel, "monitor #%d: %ux%u+%d+%d @ %u bpp", j,
>mon->monitors[j].width, mon->monitors[j].height,
>mon->monitors[j].x, mon->monitors[j].y,
>mon->monitors[j].depth);
> j++;
> }
> +memcpy(c->display_current, c->display_new, MAX_DISPLAY *
> sizeof(SpiceDisplayConfig));
>
> if (c->disable_display_align == FALSE)
> monitors_align(mon->monitors, mon->num_of_monitors);
> @@ -1469,13 +1471,23 @@ static gboolean
> any_display_has_dimensions(SpiceMainChannel *channel)
> c = channel->priv;
>
> for (i = 0; i < MAX_DISPLAY; i++) {
> -
[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
When the guest receives the monitor configuration message, it replies
(through spice-server) by destroying the primary surface, which makes
the SpiceDisplay disabled if its "resize-guest" property is used.
This change of the display state (disabled/enabled) leads to sending
a new monitor config message (containing the disabled display) after
a second. Then spice-server sends the monitor configuration message,
spice-gtk replies back and we may end up with a loop of monitor
configuration messages even if no real change of monitor configuration
happens.
To avoid the loop, the new monitor configuration messages are only sent
if they are different from the current monitor configuration.
All the issues are visible only with Wayland & QXL driver on the guest
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1266484
https://bugs.freedesktop.org/show_bug.cgi?id=94950
---
v3: rebased, fixed typos
v2: https://lists.freedesktop.org/archives/spice-devel/2015-October/022784.html
---
src/channel-main.c | 59 +++---
1 file changed, 38 insertions(+), 21 deletions(-)
diff --git a/src/channel-main.c b/src/channel-main.c
index 990a06a..1a46819 100644
--- a/src/channel-main.c
+++ b/src/channel-main.c
@@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
guint agent_msg_pos;
uint8_t agent_msg_size;
uint32_tagent_caps[VD_AGENT_CAPS_SIZE];
-SpiceDisplayConfig display[MAX_DISPLAY];
+SpiceDisplayConfig display_current[MAX_DISPLAY]; /* current
configuration of spice-widgets */
+SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new requested
configuration */
ginttimer_id;
GQueue *agent_msg_queue;
GHashTable *file_xfer_tasks;
@@ -1098,11 +1099,11 @@ gboolean
spice_main_send_monitor_config(SpiceMainChannel *channel)
if (spice_main_agent_test_capability(channel,
VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
-monitors = SPICE_N_ELEMENTS(c->display);
+monitors = SPICE_N_ELEMENTS(c->display_new);
} else {
monitors = 0;
-for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
-if (c->display[i].display_state == DISPLAY_ENABLED)
+for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
+if (c->display_new[i].display_state == DISPLAY_ENABLED)
monitors += 1;
}
}
@@ -1117,24 +1118,25 @@ gboolean
spice_main_send_monitor_config(SpiceMainChannel *channel)
CHANNEL_DEBUG(channel, "sending new monitors config to guest");
j = 0;
-for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
-if (c->display[i].display_state != DISPLAY_ENABLED) {
+for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
+if (c->display_new[i].display_state != DISPLAY_ENABLED) {
if (spice_main_agent_test_capability(channel,
VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
j++;
continue;
}
mon->monitors[j].depth = c->display_color_depth ?
c->display_color_depth : 32;
-mon->monitors[j].width = c->display[i].width;
-mon->monitors[j].height = c->display[i].height;
-mon->monitors[j].x = c->display[i].x;
-mon->monitors[j].y = c->display[i].y;
+mon->monitors[j].width = c->display_new[i].width;
+mon->monitors[j].height = c->display_new[i].height;
+mon->monitors[j].x = c->display_new[i].x;
+mon->monitors[j].y = c->display_new[i].y;
CHANNEL_DEBUG(channel, "monitor #%d: %ux%u+%d+%d @ %u bpp", j,
mon->monitors[j].width, mon->monitors[j].height,
mon->monitors[j].x, mon->monitors[j].y,
mon->monitors[j].depth);
j++;
}
+memcpy(c->display_current, c->display_new, MAX_DISPLAY *
sizeof(SpiceDisplayConfig));
if (c->disable_display_align == FALSE)
monitors_align(mon->monitors, mon->num_of_monitors);
@@ -1469,13 +1471,23 @@ static gboolean
any_display_has_dimensions(SpiceMainChannel *channel)
c = channel->priv;
for (i = 0; i < MAX_DISPLAY; i++) {
-if (c->display[i].width > 0 && c->display[i].height > 0)
+if (c->display_new[i].width > 0 && c->display_new[i].height > 0)
return TRUE;
}
return FALSE;
}
+static gboolean spice_monitor_configuration_changed(SpiceMainChannel *channel)
+{
+SpiceMainChannelPrivate *c;
+
+g_return_val_if_fail(SPICE_IS_MAIN_CHANNEL(channel), FALSE);
+c = channel->priv;
+
+return memcmp(c->display_current, c->display_new, MAX_DISPLAY *
sizeof(SpiceDisplayConfig)) != 0;
+}
+
/* main context*/
static gboolean timer_set_display(gpointer data)
{
@@ -1488,6 +1500,11 @@ static gboolean timer_set_display(gpointer data)
if
