[Qemu-devel] [PULL for-2.0 0/1] vnc bugfix

[Qemu-devel] [PULL for-2.0 0/1] vnc bugfix

[Gerd Hoffmann]

  Hi,

Most recent vnc update had a regression with vmware vga.
Here is the fix for it.

please pull,
  Gerd

The following changes since commit 315b59344126beab85a62b53582794b14436a5a4:

  Merge remote-tracking branch 'remotes/borntraeger/tags/kvm-s390-20140317' into staging (2014-03-17 22:31:33 +0000)

are available in the git repository at:


  git://git.kraxel.org/qemu tags/pull-vnc-2

for you to fetch changes up to 2f487a3d40faff1772e14da6b921900915501f9a:

  ui/vnc: fix vmware VGA incompatiblities (2014-03-18 08:21:24 +0100)

----------------------------------------------------------------
vnc: fix vmware VGA incompatiblities

----------------------------------------------------------------
Peter Lieven (1):
      ui/vnc: fix vmware VGA incompatiblities

 hw/display/vmware_vga.c |  3 ++-
 ui/vnc.c                | 10 +++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

[Qemu-devel] [PULL 1/1] ui/vnc: fix vmware VGA incompatiblities

[Gerd Hoffmann]

From: Peter Lieven <pl@kamp.de>

this fixes invalid rectangle updates observed after commit 12b316d
with the vmware VGA driver. The issues occured because the server
and client surface update seems to be out of sync at some points
and the max width of the surface is not dividable by
VNC_DIRTY_BITS_PER_PIXEL (16).

Reported-by: Serge Hallyn <serge.hallyn@ubuntu.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vmware_vga.c |  3 ++-
 ui/vnc.c                | 10 +++++++---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index bd2c108..6ae3348 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -25,6 +25,7 @@
 #include "hw/loader.h"
 #include "trace.h"
 #include "ui/console.h"
+#include "ui/vnc.h"
 #include "hw/pci/pci.h"
 
 #undef VERBOSE
@@ -218,7 +219,7 @@ enum {
 
 /* These values can probably be changed arbitrarily.  */
 #define SVGA_SCRATCH_SIZE               0x8000
-#define SVGA_MAX_WIDTH                  2360
+#define SVGA_MAX_WIDTH                  ROUND_UP(2360, VNC_DIRTY_PIXELS_PER_BIT)
 #define SVGA_MAX_HEIGHT                 1770
 
 #ifdef VERBOSE
diff --git a/ui/vnc.c b/ui/vnc.c
index 9c84b3e..5925774 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -888,7 +888,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
         VncDisplay *vd = vs->vd;
         VncJob *job;
         int y;
-        int height;
+        int height, width;
         int n = 0;
 
         if (vs->output.offset && !vs->audio_cap && !vs->force_update)
@@ -907,6 +907,7 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
         job = vnc_job_new(vs);
 
         height = MIN(pixman_image_get_height(vd->server), vs->client_height);
+        width = MIN(pixman_image_get_width(vd->server), vs->client_width);
 
         y = 0;
         for (;;) {
@@ -925,8 +926,11 @@ static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
                                     VNC_DIRTY_BPL(vs), x);
             bitmap_clear(vs->dirty[y], x, x2 - x);
             h = find_and_clear_dirty_height(vs, y, x, x2, height);
-            n += vnc_job_add_rect(job, x * VNC_DIRTY_PIXELS_PER_BIT, y,
-                                  (x2 - x) * VNC_DIRTY_PIXELS_PER_BIT, h);
+            x2 = MIN(x2, width / VNC_DIRTY_PIXELS_PER_BIT);
+            if (x2 > x) {
+                n += vnc_job_add_rect(job, x * VNC_DIRTY_PIXELS_PER_BIT, y,
+                                      (x2 - x) * VNC_DIRTY_PIXELS_PER_BIT, h);
+            }
         }
 
         vnc_job_push(job);
-- 
1.8.3.1

Re: [Qemu-devel] [PULL for-2.0 0/1] vnc bugfix

[Peter Maydell]

On 18 March 2014 07:25, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
> Most recent vnc update had a regression with vmware vga.
> Here is the fix for it.
>
> please pull,
>   Gerd
>
> The following changes since commit 315b59344126beab85a62b53582794b14436a5a4:
>
>   Merge remote-tracking branch 'remotes/borntraeger/tags/kvm-s390-20140317' into staging (2014-03-17 22:31:33 +0000)
>
> are available in the git repository at:
>
>
>   git://git.kraxel.org/qemu tags/pull-vnc-2
>
> for you to fetch changes up to 2f487a3d40faff1772e14da6b921900915501f9a:
>
>   ui/vnc: fix vmware VGA incompatiblities (2014-03-18 08:21:24 +0100)

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 1/1] ui/vnc: fix vmware VGA incompatiblities

[Peter Maydell]

On Tue, 18 Mar 2014 at 07:26, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> From: Peter Lieven <pl@kamp.de>
>
> this fixes invalid rectangle updates observed after commit 12b316d
> with the vmware VGA driver. The issues occured because the server
> and client surface update seems to be out of sync at some points
> and the max width of the surface is not dividable by
> VNC_DIRTY_BITS_PER_PIXEL (16).
>
> Reported-by: Serge Hallyn <serge.hallyn@ubuntu.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Hi all; I know this is a 6-year-old patch (now commit
2f487a3d40faff1 in git), but I was just looking
through some header file dependencies, and I noticed that
hw/display/vmware_vga.c includes ui/vnc.h, and that struck
me as pretty odd... I'm sending this email in the hope that
people might remember something of the situation beyond what's
described in the commit message.

> ---
>  hw/display/vmware_vga.c |  3 ++-
>  ui/vnc.c                | 10 +++++++---
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index bd2c108..6ae3348 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -25,6 +25,7 @@
>  #include "hw/loader.h"
>  #include "trace.h"
>  #include "ui/console.h"
> +#include "ui/vnc.h"
>  #include "hw/pci/pci.h"
>
>  #undef VERBOSE
> @@ -218,7 +219,7 @@ enum {
>
>  /* These values can probably be changed arbitrarily.  */
>  #define SVGA_SCRATCH_SIZE               0x8000
> -#define SVGA_MAX_WIDTH                  2360
> +#define SVGA_MAX_WIDTH                  ROUND_UP(2360, VNC_DIRTY_PIXELS_PER_BIT)
>  #define SVGA_MAX_HEIGHT                 1770
>
>  #ifdef VERBOSE

Here we pull in the VNC header in order to get the definition
of the VNC_DIRTY_PIXELS_PER_BIT constant, but I don't understand
why. The hw/display code should be agnostic of whatever the
UI display front-end is. Why does vmware_vga.c need to care
but not any other device?

Moreover, looking at the vmware_vga.c code, although the
SVGA_MAX_WIDTH value is made to be a multiple of VNC_DIRTY_PIXELS_PER_BIT
(which is 16), there seems to be nothing preventing the guest itself
from programming the device to some other width that isn't
a multiple of 16. So either this device needs to guard against
bad widths generally, or the problem is really in the VNC code.
I can't find anything in the vmware VGA device docs (though they
are pretty meagre) suggesting that there's a requirement for
the surface to be a multiple of 16, so I think that the VNC code
needs to be able to cope. (This should be no different from any other
display device model setting a non-multiple-of-16 width.)

So my feeling is that this vmware_vga.c portion of this commit
should be reverted, and if there's still a problem with non-multiple-of-16
surface widths then it needs to be handled in the common or VNC
specific UI code...

thanks
-- PMM

Re: [Qemu-devel] [PULL 1/1] ui/vnc: fix vmware VGA incompatiblities

[Gerd Hoffmann]

  Hi,

> > -#define SVGA_MAX_WIDTH                  2360
> > +#define SVGA_MAX_WIDTH                  ROUND_UP(2360, VNC_DIRTY_PIXELS_PER_BIT)

> Here we pull in the VNC header in order to get the definition
> of the VNC_DIRTY_PIXELS_PER_BIT constant, but I don't understand
> why. The hw/display code should be agnostic of whatever the
> UI display front-end is. Why does vmware_vga.c need to care
> but not any other device?

Yep, doesn't make sense.

> I can't find anything in the vmware VGA device docs (though they
> are pretty meagre) suggesting that there's a requirement for
> the surface to be a multiple of 16, so I think that the VNC code
> needs to be able to cope. (This should be no different from any other
> display device model setting a non-multiple-of-16 width.)

We had problems with that in the past but it should be fixed now.  vnc
wants a multiple of 16 still.  IIRC you'll get a small black bar filling
the room to the next multiple of 16 in case the display surface doesn't
match.

> So my feeling is that this vmware_vga.c portion of this commit
> should be reverted,

Agree.

Maybe even deprecate the whole device?  Not sure how useful it is these
days as it has seen pretty much no development in the last decade.  The
linux kernel modesetting driver complains about missing device features
and refuses to touch the device ...

take care,
  Gerd