Re: [Qemu-devel] [Qemu-stable] [PATCH v2 for 2.7] ui: fix refresh of VNC server surface

[Peter Lieven <pl@kamp.de>]

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