[Qemu-devel] [PATCH] update server bits on vnc_update

[Qemu-devel] [PATCH] update server bits on vnc_update

[Glauber Costa]

Since the server/guest split in vnc architecture, we
no longer update the server bits on large updates. Result
is screen gets garbled, specially on scroll actions.

I must admit I don't fully understand our vnc code, but after
a careful reading, it seemed to me the proposed patch would fix
it, and it indeed, works.

Gerd, Anthony, please tell me what you think of this approach.

This fixes the following bugs for me:
https://bugzilla.redhat.com/show_bug.cgi?id=503156
https://bugzilla.redhat.com/show_bug.cgi?id=507626
https://bugs.launchpad.net/qemu/+bug/397212

Signed-off-by: Glauber Costa <glommer@redhat.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
---
 vnc.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/vnc.c b/vnc.c
index de0ff87..acebdaf 100644
--- a/vnc.c
+++ b/vnc.c
@@ -262,9 +262,8 @@ static inline int vnc_and_bits(const uint32_t *d1, const uint32_t *d2,
     return 0;
 }
 
-static void vnc_update(VncState *vs, int x, int y, int w, int h)
+static void do_vnc_update(struct VncSurface *s, int x, int y, int w, int h)
 {
-    struct VncSurface *s = &vs->guest;
     int i;
 
     h += y;
@@ -286,6 +285,13 @@ static void vnc_update(VncState *vs, int x, int y, int w, int h)
             vnc_set_bit(s->dirty[y], (x + i) / 16);
 }
 
+
+static void vnc_update(VncState *vs, int x, int y, int w, int h)
+{
+    do_vnc_update(&vs->guest, x, y, w, h);
+    do_vnc_update(&vs->server, x, y, w, h);
+}
+
 static void vnc_dpy_update(DisplayState *ds, int x, int y, int w, int h)
 {
     VncDisplay *vd = ds->opaque;
-- 
1.6.2.2

[Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Anthony Liguori]

Glauber Costa wrote:
> Since the server/guest split in vnc architecture, we
> no longer update the server bits on large updates. Result
> is screen gets garbled, specially on scroll actions.
>
> I must admit I don't fully understand our vnc code, but after
> a careful reading, it seemed to me the proposed patch would fix
> it, and it indeed, works.
>
> Gerd, Anthony, please tell me what you think of this approach.
>   

It seems to look reasonable to me but I'd like Gerd to comment too.

> This fixes the following bugs for me:
> https://bugzilla.redhat.com/show_bug.cgi?id=503156
> https://bugzilla.redhat.com/show_bug.cgi?id=507626
> https://bugs.launchpad.net/qemu/+bug/397212
>   

Very nice.

> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vnc.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/vnc.c b/vnc.c
> index de0ff87..acebdaf 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -262,9 +262,8 @@ static inline int vnc_and_bits(const uint32_t *d1, const uint32_t *d2,
>      return 0;
>  }
>
> -static void vnc_update(VncState *vs, int x, int y, int w, int h)
> +static void do_vnc_update(struct VncSurface *s, int x, int y, int w, int h)
>  {
> -    struct VncSurface *s = &vs->guest;
>   

Not worth spinning a new patch, but it's nice to avoid these sort of 
changes in a bug fix.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] update server bits on vnc_update

[Lucas Meneghel Rodrigues]

On Sat, Jul 18, 2009 at 1:47 AM, Glauber Costa<glommer@redhat.com> wrote:
> Since the server/guest split in vnc architecture, we
> no longer update the server bits on large updates. Result
> is screen gets garbled, specially on scroll actions.
>
> I must admit I don't fully understand our vnc code, but after
> a careful reading, it seemed to me the proposed patch would fix
> it, and it indeed, works.
>
> Gerd, Anthony, please tell me what you think of this approach.
>
> This fixes the following bugs for me:
> https://bugzilla.redhat.com/show_bug.cgi?id=503156
> https://bugzilla.redhat.com/show_bug.cgi?id=507626
> https://bugs.launchpad.net/qemu/+bug/397212

Works for me as well. I've rebuilt the virt-preview repo rpms with
Glauber's patch and I don't have garbled VNC anymore.

Thanks for fixing this up, Glauber!

> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vnc.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/vnc.c b/vnc.c
> index de0ff87..acebdaf 100644
> --- a/vnc.c
> +++ b/vnc.c
> @@ -262,9 +262,8 @@ static inline int vnc_and_bits(const uint32_t *d1, const uint32_t *d2,
>     return 0;
>  }
>
> -static void vnc_update(VncState *vs, int x, int y, int w, int h)
> +static void do_vnc_update(struct VncSurface *s, int x, int y, int w, int h)
>  {
> -    struct VncSurface *s = &vs->guest;
>     int i;
>
>     h += y;
> @@ -286,6 +285,13 @@ static void vnc_update(VncState *vs, int x, int y, int w, int h)
>             vnc_set_bit(s->dirty[y], (x + i) / 16);
>  }
>
> +
> +static void vnc_update(VncState *vs, int x, int y, int w, int h)
> +{
> +    do_vnc_update(&vs->guest, x, y, w, h);
> +    do_vnc_update(&vs->server, x, y, w, h);
> +}
> +
>  static void vnc_dpy_update(DisplayState *ds, int x, int y, int w, int h)
>  {
>     VncDisplay *vd = ds->opaque;
> --
> 1.6.2.2
>
>
>
>



-- 
Lucas Meneghel

[Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Gerd Hoffmann]

On 07/18/09 06:47, Glauber Costa wrote:
> Since the server/guest split in vnc architecture, we
> no longer update the server bits on large updates. Result
> is screen gets garbled, specially on scroll actions.
>
> I must admit I don't fully understand our vnc code, but after
> a careful reading, it seemed to me the proposed patch would fix
> it, and it indeed, works.

NAK.

VNC screen update workflow is this:

   (1) vnc_update() is called to mark a region as dirty,
       the guest bitmap is updated accordingly.  May happen
       multiple times.
   (2) vnc_update_client() is called by timer.  It walks
       the guest bitmap.  For dirty regions it checks whenever
       guest and server bitmap are *really* different, if so
       it does a guest->server copy and updates the server
       dirty bitmap.
   (3) vnc_update_client() walks the server dirty map and sends
       over the changes to the client.

Your patch kills the optimization in (2) and papers over the real bug.

Some extra care is needed in vnc_dpy_copy() which can send a bitblit 
command to the guest (VNC_ENCODING_COPYRECT).

And while looking at the code I think I've spotted the actual bug:  When 
sending a bitblit to the client we do *not* update the local server 
surface.  Result is the vnc clients and the vnc servers idea of the 
screen content are not in sync any more.  Which in turn will break the 
optimization in (2) and can easily result in a corrupted screen ...

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 565 bytes --]

On 07/21/09 10:09, Gerd Hoffmann wrote:
> On 07/18/09 06:47, Glauber Costa wrote:
> And while looking at the code I think I've spotted the actual bug: When
> sending a bitblit to the client we do *not* update the local server
> surface. Result is the vnc clients and the vnc servers idea of the
> screen content are not in sync any more. Which in turn will break the
> optimization in (2) and can easily result in a corrupted screen ...

Quick test:  Disabling copyrect should make the screen corruption go 
away then.  Real fix will follow later.

cheers,
   Gerd

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 529 bytes --]

diff --git a/vnc.c b/vnc.c
index de0ff87..86acfee 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1541,9 +1541,11 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
         case VNC_ENCODING_RAW:
             vs->vnc_encoding = enc;
             break;
+#if 0
         case VNC_ENCODING_COPYRECT:
             vs->features |= VNC_FEATURE_COPYRECT_MASK;
             break;
+#endif
         case VNC_ENCODING_HEXTILE:
             vs->features |= VNC_FEATURE_HEXTILE_MASK;
             vs->vnc_encoding = enc;

Re: [Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Stefano Stabellini]

On Tue, 21 Jul 2009, Gerd Hoffmann wrote:
> On 07/18/09 06:47, Glauber Costa wrote:
> > Since the server/guest split in vnc architecture, we
> > no longer update the server bits on large updates. Result
> > is screen gets garbled, specially on scroll actions.
> >
> > I must admit I don't fully understand our vnc code, but after
> > a careful reading, it seemed to me the proposed patch would fix
> > it, and it indeed, works.
> 
> NAK.
> 
> VNC screen update workflow is this:
> 
>    (1) vnc_update() is called to mark a region as dirty,
>        the guest bitmap is updated accordingly.  May happen
>        multiple times.
>    (2) vnc_update_client() is called by timer.  It walks
>        the guest bitmap.  For dirty regions it checks whenever
>        guest and server bitmap are *really* different, if so
>        it does a guest->server copy and updates the server
>        dirty bitmap.
>    (3) vnc_update_client() walks the server dirty map and sends
>        over the changes to the client.
> 

As a side note: I was reading the new vnc multiclient code to understand
how the refresh works now, and I have just noticed something that I
think is wrong:

we call vga_hw_update in vnc_update_client, that means we call it once
per vnc client connected! We should call it only once, probably in a
separate timer, that is enabled only if at least one client is connect.


I also don't like the fact that we are allocating a server surface per
client connected, this can become a problem very fast: I think we could
probably get away with just one server surface for all the clients.

Re: [Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Stefano Stabellini]

On Tue, 21 Jul 2009, Stefano Stabellini wrote:
> As a side note: I was reading the new vnc multiclient code to understand
> how the refresh works now, and I have just noticed something that I
> think is wrong:
> 
> we call vga_hw_update in vnc_update_client, that means we call it once
> per vnc client connected! We should call it only once, probably in a
> separate timer, that is enabled only if at least one client is connect.
> 
> 
> I also don't like the fact that we are allocating a server surface per
> client connected, this can become a problem very fast: I think we could
> probably get away with just one server surface for all the clients.
> 
> 

Obviously these are suggestions for improvements and I am willing to
write the patches if you agree with the ideas.

Re: [Qemu-devel] Re: [PATCH] update server bits on vnc_update

[Gerd Hoffmann]

On 07/21/09 14:56, Stefano Stabellini wrote:
> we call vga_hw_update in vnc_update_client, that means we call it once
> per vnc client connected! We should call it only once, probably in a
> separate timer, that is enabled only if at least one client is connect.

Good point.  Even better would be a timer shared by all clients, then 
loop over all clients (pretty much like vnc_dpy_update calls vnc_update 
for all connected clients).  That would make moving vga_hw_update out of 
the loop trivial ;)

> I also don't like the fact that we are allocating a server surface per
> client connected, this can become a problem very fast: I think we could
> probably get away with just one server surface for all the clients.

I don't think so.  server surface matches the vnc clients view of the 
framebuffer.  We need that to avoid sending screen updates for regions 
which didn't change.  Different clients can have a different views here 
for a number of reasons:  Because they support different feature sets. 
Because we try to skip frames if the pipe is full.  Thus sharing the 
server surface is non-trivial and we'll lose certain optimization when 
doing that.

cheers,
   Gerd