All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
@ 2016-08-16 16:30 Daniel P. Berrange
  2016-08-23  6:50 ` [Qemu-devel] [Qemu-stable] " Peter Lieven
  2016-08-29 13:44 ` Peter Lieven
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel P. Berrange @ 2016-08-16 16:30 UTC (permalink / raw)
  To: qemu-devel; +Cc: Gerd Hoffmann, qemu-stable, Daniel P. Berrange

In previous commit

  commit c7628bff4138ce906a3620d12e0820c1cf6c140d
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Fri Oct 30 12:10:09 2015 +0100

    vnc: only alloc server surface with clients connected

the VNC server was changed so that the 'vd->server' pixman
image was only allocated when a client is connected.

Since then if a client disconnects and then reconnects to
the VNC server all they will see is a black screen until
they do something that triggers a refresh. On a graphical
desktop this is not often noticed since there's many things
going on which cause a refresh. On a plain text console it
is really obvious since nothing refreshes frequently.

The problem is that the VNC server didn't update the guest
dirty bitmap, so still believes its server image is in sync
with the guest contents.

To fix this we must explicitly mark the entire guest desktop
as dirty after re-creating the server surface. Move this
logic into vnc_update_server_surface() so it is guaranteed
to be call in all code paths that re-create the surface
instead of only in vnc_dpy_switch()

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---

NB This should go into 2.5 & 2.6 stable branches too

 ui/vnc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 853b57e..d1087c9 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -692,6 +692,8 @@ void *vnc_server_fb_ptr(VncDisplay *vd, int x, int y)
 
 static void vnc_update_server_surface(VncDisplay *vd)
 {
+    int width, height;
+
     qemu_pixman_image_unref(vd->server);
     vd->server = NULL;
 
@@ -699,10 +701,15 @@ static void vnc_update_server_surface(VncDisplay *vd)
         return;
     }
 
+    width = vnc_width(vd);
+    height = vnc_height(vd);
     vd->server = pixman_image_create_bits(VNC_SERVER_FB_FORMAT,
-                                          vnc_width(vd),
-                                          vnc_height(vd),
+                                          width, height,
                                           NULL, 0);
+
+    memset(vd->guest.dirty, 0x00, sizeof(vd->guest.dirty));
+    vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0,
+                       width, height);
 }
 
 static void vnc_dpy_switch(DisplayChangeListener *dcl,
@@ -710,7 +717,6 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
 {
     VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
     VncState *vs;
-    int width, height;
 
     vnc_abort_display_jobs(vd);
     vd->ds = surface;
@@ -722,11 +728,6 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
     qemu_pixman_image_unref(vd->guest.fb);
     vd->guest.fb = pixman_image_ref(surface->image);
     vd->guest.format = surface->format;
-    width = vnc_width(vd);
-    height = vnc_height(vd);
-    memset(vd->guest.dirty, 0x00, sizeof(vd->guest.dirty));
-    vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0,
-                       width, height);
 
     QTAILQ_FOREACH(vs, &vd->clients, next) {
         vnc_colordepth(vs);
@@ -736,7 +737,8 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
         }
         memset(vs->dirty, 0x00, sizeof(vs->dirty));
         vnc_set_area_dirty(vs->dirty, vd, 0, 0,
-                           width, height);
+                           vnc_width(vd),
+                           vnc_height(vd));
     }
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-16 16:30 [Qemu-devel] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface Daniel P. Berrange
@ 2016-08-23  6:50 ` Peter Lieven
  2016-08-24 15:46   ` Peter Maydell
  2016-08-29 13:44 ` Peter Lieven
  1 sibling, 1 reply; 9+ messages in thread
From: Peter Lieven @ 2016-08-23  6:50 UTC (permalink / raw)
  To: Daniel P. Berrange, qemu-devel; +Cc: Gerd Hoffmann, qemu-stable

Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
> In previous commit
>
>    commit c7628bff4138ce906a3620d12e0820c1cf6c140d
>    Author: Gerd Hoffmann <kraxel@redhat.com>
>    Date:   Fri Oct 30 12:10:09 2015 +0100
>
>      vnc: only alloc server surface with clients connected
>
> the VNC server was changed so that the 'vd->server' pixman
> image was only allocated when a client is connected.
>
> Since then if a client disconnects and then reconnects to
> the VNC server all they will see is a black screen until
> they do something that triggers a refresh. On a graphical
> desktop this is not often noticed since there's many things
> going on which cause a refresh. On a plain text console it
> is really obvious since nothing refreshes frequently.
>
> The problem is that the VNC server didn't update the guest
> dirty bitmap, so still believes its server image is in sync
> with the guest contents.
>
> To fix this we must explicitly mark the entire guest desktop
> as dirty after re-creating the server surface. Move this
> logic into vnc_update_server_surface() so it is guaranteed
> to be call in all code paths that re-create the surface
> instead of only in vnc_dpy_switch()
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

I noticed that these patches is as well not in master yet and therefore
not included in the 2.7.0-rc4 tagged yesterday.

Peter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-23  6:50 ` [Qemu-devel] [Qemu-stable] " Peter Lieven
@ 2016-08-24 15:46   ` Peter Maydell
  2016-08-24 15:49     ` Daniel P. Berrange
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2016-08-24 15:46 UTC (permalink / raw)
  To: Peter Lieven
  Cc: Daniel P. Berrange, QEMU Developers, Gerd Hoffmann, qemu-stable

On 23 August 2016 at 07:50, Peter Lieven <pl@kamp.de> wrote:
> Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
>>
>> In previous commit
>>
>>    commit c7628bff4138ce906a3620d12e0820c1cf6c140d
>>    Author: Gerd Hoffmann <kraxel@redhat.com>
>>    Date:   Fri Oct 30 12:10:09 2015 +0100
>>
>>      vnc: only alloc server surface with clients connected
>>
>> the VNC server was changed so that the 'vd->server' pixman
>> image was only allocated when a client is connected.
>>
>> Since then if a client disconnects and then reconnects to
>> the VNC server all they will see is a black screen until
>> they do something that triggers a refresh. On a graphical
>> desktop this is not often noticed since there's many things
>> going on which cause a refresh. On a plain text console it
>> is really obvious since nothing refreshes frequently.
>>
>> The problem is that the VNC server didn't update the guest
>> dirty bitmap, so still believes its server image is in sync
>> with the guest contents.
>>
>> To fix this we must explicitly mark the entire guest desktop
>> as dirty after re-creating the server surface. Move this
>> logic into vnc_update_server_surface() so it is guaranteed
>> to be call in all code paths that re-create the surface
>> instead of only in vnc_dpy_switch()
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>
>
> I noticed that these patches is as well not in master yet and therefore
> not included in the 2.7.0-rc4 tagged yesterday.

Dan, Gerd -- we're going to need an rc5 anyway -- can you
comment on whether this patch is "should be in rc5"
material? (If it is I can commit it to master directly.)

thanks
-- PMM

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-24 15:46   ` Peter Maydell
@ 2016-08-24 15:49     ` Daniel P. Berrange
  2016-08-25  7:15       ` Peter Lieven
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel P. Berrange @ 2016-08-24 15:49 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Peter Lieven, QEMU Developers, Gerd Hoffmann, qemu-stable

On Wed, Aug 24, 2016 at 04:46:31PM +0100, Peter Maydell wrote:
> On 23 August 2016 at 07:50, Peter Lieven <pl@kamp.de> wrote:
> > Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
> >>
> >> In previous commit
> >>
> >>    commit c7628bff4138ce906a3620d12e0820c1cf6c140d
> >>    Author: Gerd Hoffmann <kraxel@redhat.com>
> >>    Date:   Fri Oct 30 12:10:09 2015 +0100
> >>
> >>      vnc: only alloc server surface with clients connected
> >>
> >> the VNC server was changed so that the 'vd->server' pixman
> >> image was only allocated when a client is connected.
> >>
> >> Since then if a client disconnects and then reconnects to
> >> the VNC server all they will see is a black screen until
> >> they do something that triggers a refresh. On a graphical
> >> desktop this is not often noticed since there's many things
> >> going on which cause a refresh. On a plain text console it
> >> is really obvious since nothing refreshes frequently.
> >>
> >> The problem is that the VNC server didn't update the guest
> >> dirty bitmap, so still believes its server image is in sync
> >> with the guest contents.
> >>
> >> To fix this we must explicitly mark the entire guest desktop
> >> as dirty after re-creating the server surface. Move this
> >> logic into vnc_update_server_surface() so it is guaranteed
> >> to be call in all code paths that re-create the surface
> >> instead of only in vnc_dpy_switch()
> >>
> >> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> >
> >
> > I noticed that these patches is as well not in master yet and therefore
> > not included in the 2.7.0-rc4 tagged yesterday.
> 
> Dan, Gerd -- we're going to need an rc5 anyway -- can you
> comment on whether this patch is "should be in rc5"
> material? (If it is I can commit it to master directly.)

I think it should be, as the VNC server is pretty unusable when it does
not refresh the screen when you connect, but I'll let Gerd decide in case
there's some implication in my patch that I've mis-understood.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-24 15:49     ` Daniel P. Berrange
@ 2016-08-25  7:15       ` Peter Lieven
  2016-08-25 12:46         ` Daniel P. Berrange
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Lieven @ 2016-08-25  7:15 UTC (permalink / raw)
  To: Daniel P. Berrange, Peter Maydell
  Cc: qemu-stable, QEMU Developers, Gerd Hoffmann

Am 24.08.2016 um 17:49 schrieb Daniel P. Berrange:
> On Wed, Aug 24, 2016 at 04:46:31PM +0100, Peter Maydell wrote:
>> On 23 August 2016 at 07:50, Peter Lieven <pl@kamp.de> wrote:
>>> Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
>>>> In previous commit
>>>>
>>>>     commit c7628bff4138ce906a3620d12e0820c1cf6c140d
>>>>     Author: Gerd Hoffmann <kraxel@redhat.com>
>>>>     Date:   Fri Oct 30 12:10:09 2015 +0100
>>>>
>>>>       vnc: only alloc server surface with clients connected
>>>>
>>>> the VNC server was changed so that the 'vd->server' pixman
>>>> image was only allocated when a client is connected.
>>>>
>>>> Since then if a client disconnects and then reconnects to
>>>> the VNC server all they will see is a black screen until
>>>> they do something that triggers a refresh. On a graphical
>>>> desktop this is not often noticed since there's many things
>>>> going on which cause a refresh. On a plain text console it
>>>> is really obvious since nothing refreshes frequently.
>>>>
>>>> The problem is that the VNC server didn't update the guest
>>>> dirty bitmap, so still believes its server image is in sync
>>>> with the guest contents.
>>>>
>>>> To fix this we must explicitly mark the entire guest desktop
>>>> as dirty after re-creating the server surface. Move this
>>>> logic into vnc_update_server_surface() so it is guaranteed
>>>> to be call in all code paths that re-create the surface
>>>> instead of only in vnc_dpy_switch()
>>>>
>>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>>
>>> I noticed that these patches is as well not in master yet and therefore
>>> not included in the 2.7.0-rc4 tagged yesterday.
>> Dan, Gerd -- we're going to need an rc5 anyway -- can you
>> comment on whether this patch is "should be in rc5"
>> material? (If it is I can commit it to master directly.)
> I think it should be, as the VNC server is pretty unusable when it does
> not refresh the screen when you connect, but I'll let Gerd decide in case
> there's some implication in my patch that I've mis-understood.

Hi Daniel,

I have had a look at the code and currently I do not understand why you
ran into a blank screen. The server surface is not created if there is no
client connected, this is correct. But the dirty bitmap for the server
exists even if there is no client connected. So the status of the dirty bitmap
should be the same with or without your patch.

Can you tell what exactly you tried out to reproduce a potential issue?

Thanks,
Peter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-25  7:15       ` Peter Lieven
@ 2016-08-25 12:46         ` Daniel P. Berrange
  2016-08-26 11:38           ` Peter Lieven
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel P. Berrange @ 2016-08-25 12:46 UTC (permalink / raw)
  To: Peter Lieven; +Cc: Peter Maydell, qemu-stable, QEMU Developers, Gerd Hoffmann

On Thu, Aug 25, 2016 at 09:15:52AM +0200, Peter Lieven wrote:
> Am 24.08.2016 um 17:49 schrieb Daniel P. Berrange:
> > On Wed, Aug 24, 2016 at 04:46:31PM +0100, Peter Maydell wrote:
> > > On 23 August 2016 at 07:50, Peter Lieven <pl@kamp.de> wrote:
> > > > Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
> > > > > In previous commit
> > > > > 
> > > > >     commit c7628bff4138ce906a3620d12e0820c1cf6c140d
> > > > >     Author: Gerd Hoffmann <kraxel@redhat.com>
> > > > >     Date:   Fri Oct 30 12:10:09 2015 +0100
> > > > > 
> > > > >       vnc: only alloc server surface with clients connected
> > > > > 
> > > > > the VNC server was changed so that the 'vd->server' pixman
> > > > > image was only allocated when a client is connected.
> > > > > 
> > > > > Since then if a client disconnects and then reconnects to
> > > > > the VNC server all they will see is a black screen until
> > > > > they do something that triggers a refresh. On a graphical
> > > > > desktop this is not often noticed since there's many things
> > > > > going on which cause a refresh. On a plain text console it
> > > > > is really obvious since nothing refreshes frequently.
> > > > > 
> > > > > The problem is that the VNC server didn't update the guest
> > > > > dirty bitmap, so still believes its server image is in sync
> > > > > with the guest contents.
> > > > > 
> > > > > To fix this we must explicitly mark the entire guest desktop
> > > > > as dirty after re-creating the server surface. Move this
> > > > > logic into vnc_update_server_surface() so it is guaranteed
> > > > > to be call in all code paths that re-create the surface
> > > > > instead of only in vnc_dpy_switch()
> > > > > 
> > > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > > 
> > > > I noticed that these patches is as well not in master yet and therefore
> > > > not included in the 2.7.0-rc4 tagged yesterday.
> > > Dan, Gerd -- we're going to need an rc5 anyway -- can you
> > > comment on whether this patch is "should be in rc5"
> > > material? (If it is I can commit it to master directly.)
> > I think it should be, as the VNC server is pretty unusable when it does
> > not refresh the screen when you connect, but I'll let Gerd decide in case
> > there's some implication in my patch that I've mis-understood.
> 
> Hi Daniel,
> 
> I have had a look at the code and currently I do not understand why you
> ran into a blank screen. The server surface is not created if there is no
> client connected, this is correct. But the dirty bitmap for the server
> exists even if there is no client connected. So the status of the dirty bitmap
> should be the same with or without your patch.

IIUC, the dirty bitmap is used to decide when to update the server surface
from the guest framebuffer.  When the new client connects and we create a
new server surface, the dirty bitmap is clean, so QEMU never copies the
guest framebuffer into the new server surface hence I just get a blank
screen.

> Can you tell what exactly you tried out to reproduce a potential issue?

Boot a Fedora guest, in text mode (ie no Xorg graphics). Then simply
disconnect & connect and I'll always get a black screen until I do something
that causes Linux to update the screen.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-25 12:46         ` Daniel P. Berrange
@ 2016-08-26 11:38           ` Peter Lieven
  0 siblings, 0 replies; 9+ messages in thread
From: Peter Lieven @ 2016-08-26 11:38 UTC (permalink / raw)
  To: Daniel P. Berrange
  Cc: Peter Maydell, qemu-stable, QEMU Developers, Gerd Hoffmann

Am 25.08.2016 um 14:46 schrieb Daniel P. Berrange:
> On Thu, Aug 25, 2016 at 09:15:52AM +0200, Peter Lieven wrote:
>> Am 24.08.2016 um 17:49 schrieb Daniel P. Berrange:
>>> On Wed, Aug 24, 2016 at 04:46:31PM +0100, Peter Maydell wrote:
>>>> On 23 August 2016 at 07:50, Peter Lieven <pl@kamp.de> wrote:
>>>>> Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
>>>>>> In previous commit
>>>>>>
>>>>>>      commit c7628bff4138ce906a3620d12e0820c1cf6c140d
>>>>>>      Author: Gerd Hoffmann <kraxel@redhat.com>
>>>>>>      Date:   Fri Oct 30 12:10:09 2015 +0100
>>>>>>
>>>>>>        vnc: only alloc server surface with clients connected
>>>>>>
>>>>>> the VNC server was changed so that the 'vd->server' pixman
>>>>>> image was only allocated when a client is connected.
>>>>>>
>>>>>> Since then if a client disconnects and then reconnects to
>>>>>> the VNC server all they will see is a black screen until
>>>>>> they do something that triggers a refresh. On a graphical
>>>>>> desktop this is not often noticed since there's many things
>>>>>> going on which cause a refresh. On a plain text console it
>>>>>> is really obvious since nothing refreshes frequently.
>>>>>>
>>>>>> The problem is that the VNC server didn't update the guest
>>>>>> dirty bitmap, so still believes its server image is in sync
>>>>>> with the guest contents.
>>>>>>
>>>>>> To fix this we must explicitly mark the entire guest desktop
>>>>>> as dirty after re-creating the server surface. Move this
>>>>>> logic into vnc_update_server_surface() so it is guaranteed
>>>>>> to be call in all code paths that re-create the surface
>>>>>> instead of only in vnc_dpy_switch()
>>>>>>
>>>>>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>>>> I noticed that these patches is as well not in master yet and therefore
>>>>> not included in the 2.7.0-rc4 tagged yesterday.
>>>> Dan, Gerd -- we're going to need an rc5 anyway -- can you
>>>> comment on whether this patch is "should be in rc5"
>>>> material? (If it is I can commit it to master directly.)
>>> I think it should be, as the VNC server is pretty unusable when it does
>>> not refresh the screen when you connect, but I'll let Gerd decide in case
>>> there's some implication in my patch that I've mis-understood.
>> Hi Daniel,
>>
>> I have had a look at the code and currently I do not understand why you
>> ran into a blank screen. The server surface is not created if there is no
>> client connected, this is correct. But the dirty bitmap for the server
>> exists even if there is no client connected. So the status of the dirty bitmap
>> should be the same with or without your patch.
> IIUC, the dirty bitmap is used to decide when to update the server surface
> from the guest framebuffer.  When the new client connects and we create a
> new server surface, the dirty bitmap is clean, so QEMU never copies the
> guest framebuffer into the new server surface hence I just get a blank
> screen.

Okay, but this can only happen at the second connection. The first
vnc connection works, you disconnect, reconnect and then get a blank screen.
Indeed in this case there is in theory no vnc_dpy_switch in between. However,
I have the feeling that something else is additionally wrong - see below.

>
>> Can you tell what exactly you tried out to reproduce a potential issue?
> Boot a Fedora guest, in text mode (ie no Xorg graphics). Then simply
> disconnect & connect and I'll always get a black screen until I do something
> that causes Linux to update the screen.

I can reproduce an issue if I have 2 VNC connections in paralell.

If I open the first VNC connection to a VM that does not update the screen, I have the following
calls to vnc_dpy_switch and vnc_update_server_surface.

# qemu start
vnc_update_server_surface: no clients
vnc_dpy_switch: 640 480

# first vnc client connects
vnc_update_server_surface: no clients
vnc_dpy_switch: 720 400
vnc_connect: 0x5582ceb56000
vnc_init_state: 0x5582ceb56000 (first_client 1)
vnc_update_server_surface
vnc_update_server_surface
vnc_dpy_switch: 720 400

# first client disconnects
vnc_disconnect_finished: 0x562b79aa3850 (last_client 1)
vnc_update_server_surface: no clients

# first client connects again
vnc_connect: 0x562b79aa3850
vnc_init_state: 0x562b79aa3850 (first_client 1)
vnc_update_server_surface
vnc_update_server_surface
vnc_dpy_switch: 720 400

# second client connects
vnc_connect: 0x562b79a6d890
vnc_init_state: 0x562b79a6d890 (first_client 0)
vnc_update_server_surface
vnc_dpy_switch: 720 400

# second client disconnects
vnc_disconnect_finished: 0x562b79a6d890 (last_client 0)

# first client disconnects
vnc_disconnect_finished: 0x562b79aa3850 (last_client 1)
vnc_update_server_surface: no clients

Strange enough, I see no issues with the first client, but I wonder why each vnc
connect triggers multiple vnc_update_server_surface and vnc_dpy_switch and secondly
my second client has a blank screen.

So at first I think your patch is correct, we need to mark the server surface dirty
in vnc_update_server_surface cause there are other callers to it than vnc_dpy_switch.

But there seems to be another issue. I try to figure out what it is.

Peter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-16 16:30 [Qemu-devel] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface Daniel P. Berrange
  2016-08-23  6:50 ` [Qemu-devel] [Qemu-stable] " Peter Lieven
@ 2016-08-29 13:44 ` Peter Lieven
  2016-08-30 10:48   ` Peter Maydell
  1 sibling, 1 reply; 9+ messages in thread
From: Peter Lieven @ 2016-08-29 13:44 UTC (permalink / raw)
  To: Daniel P. Berrange, qemu-devel; +Cc: Gerd Hoffmann, qemu-stable, Peter Maydell

Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
> In previous commit
>
>    commit c7628bff4138ce906a3620d12e0820c1cf6c140d
>    Author: Gerd Hoffmann <kraxel@redhat.com>
>    Date:   Fri Oct 30 12:10:09 2015 +0100
>
>      vnc: only alloc server surface with clients connected
>
> the VNC server was changed so that the 'vd->server' pixman
> image was only allocated when a client is connected.
>
> Since then if a client disconnects and then reconnects to
> the VNC server all they will see is a black screen until
> they do something that triggers a refresh. On a graphical
> desktop this is not often noticed since there's many things
> going on which cause a refresh. On a plain text console it
> is really obvious since nothing refreshes frequently.
>
> The problem is that the VNC server didn't update the guest
> dirty bitmap, so still believes its server image is in sync
> with the guest contents.
>
> To fix this we must explicitly mark the entire guest desktop
> as dirty after re-creating the server surface. Move this
> logic into vnc_update_server_surface() so it is guaranteed
> to be call in all code paths that re-create the surface
> instead of only in vnc_dpy_switch()
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>
> NB This should go into 2.5 & 2.6 stable branches too
>
>   ui/vnc.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 853b57e..d1087c9 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -692,6 +692,8 @@ void *vnc_server_fb_ptr(VncDisplay *vd, int x, int y)
>   
>   static void vnc_update_server_surface(VncDisplay *vd)
>   {
> +    int width, height;
> +
>       qemu_pixman_image_unref(vd->server);
>       vd->server = NULL;
>   
> @@ -699,10 +701,15 @@ static void vnc_update_server_surface(VncDisplay *vd)
>           return;
>       }
>   
> +    width = vnc_width(vd);
> +    height = vnc_height(vd);
>       vd->server = pixman_image_create_bits(VNC_SERVER_FB_FORMAT,
> -                                          vnc_width(vd),
> -                                          vnc_height(vd),
> +                                          width, height,
>                                             NULL, 0);
> +
> +    memset(vd->guest.dirty, 0x00, sizeof(vd->guest.dirty));
> +    vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0,
> +                       width, height);
>   }
>   
>   static void vnc_dpy_switch(DisplayChangeListener *dcl,
> @@ -710,7 +717,6 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>   {
>       VncDisplay *vd = container_of(dcl, VncDisplay, dcl);
>       VncState *vs;
> -    int width, height;
>   
>       vnc_abort_display_jobs(vd);
>       vd->ds = surface;
> @@ -722,11 +728,6 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>       qemu_pixman_image_unref(vd->guest.fb);
>       vd->guest.fb = pixman_image_ref(surface->image);
>       vd->guest.format = surface->format;
> -    width = vnc_width(vd);
> -    height = vnc_height(vd);
> -    memset(vd->guest.dirty, 0x00, sizeof(vd->guest.dirty));
> -    vnc_set_area_dirty(vd->guest.dirty, vd, 0, 0,
> -                       width, height);
>   
>       QTAILQ_FOREACH(vs, &vd->clients, next) {
>           vnc_colordepth(vs);
> @@ -736,7 +737,8 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
>           }
>           memset(vs->dirty, 0x00, sizeof(vs->dirty));
>           vnc_set_area_dirty(vs->dirty, vd, 0, 0,
> -                           width, height);
> +                           vnc_width(vd),
> +                           vnc_height(vd));
>       }
>   }
>   

I can confirm this issue now. Its only visible if the VNC client does not send a SET_PIXEL_FORMAT message.
Which my client does and this masked the issue for me. The set pixel format routine invalidates
the whole display which results in a vnc_dpy_switch which masks the guest fb dirty. Why this invalidation
takes place I do not understand, but this can be sorted out lated.

For this patch:
Tested-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Peter Lieven <pl@kamp.de>

Peter

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface
  2016-08-29 13:44 ` Peter Lieven
@ 2016-08-30 10:48   ` Peter Maydell
  0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2016-08-30 10:48 UTC (permalink / raw)
  To: Peter Lieven
  Cc: Daniel P. Berrange, QEMU Developers, Gerd Hoffmann, qemu-stable

On 29 August 2016 at 14:44, Peter Lieven <pl@kamp.de> wrote:
> Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:

>> To fix this we must explicitly mark the entire guest desktop
>> as dirty after re-creating the server surface. Move this
>> logic into vnc_update_server_surface() so it is guaranteed
>> to be call in all code paths that re-create the surface
>> instead of only in vnc_dpy_switch()
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

> I can confirm this issue now. Its only visible if the VNC client does not
> send a SET_PIXEL_FORMAT message.
> Which my client does and this masked the issue for me. The set pixel format
> routine invalidates
> the whole display which results in a vnc_dpy_switch which masks the guest fb
> dirty. Why this invalidation
> takes place I do not understand, but this can be sorted out lated.
>
> For this patch:
> Tested-by: Peter Lieven <pl@kamp.de>
> Reviewed-by: Peter Lieven <pl@kamp.de>

Applied to master for 2.7, thanks.

-- PMM

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-08-30 10:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-16 16:30 [Qemu-devel] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface Daniel P. Berrange
2016-08-23  6:50 ` [Qemu-devel] [Qemu-stable] " Peter Lieven
2016-08-24 15:46   ` Peter Maydell
2016-08-24 15:49     ` Daniel P. Berrange
2016-08-25  7:15       ` Peter Lieven
2016-08-25 12:46         ` Daniel P. Berrange
2016-08-26 11:38           ` Peter Lieven
2016-08-29 13:44 ` Peter Lieven
2016-08-30 10:48   ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.