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

[Peter Lieven]

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 <address@hidden> wrote:
> > > > > Am 16.08.2016 um 18:30 schrieb Daniel P. Berrange:
> > > > > > In previous commit
> > > > > >
> > > > > >      commit c7628bff4138ce906a3620d12e0820c1cf6c140d
> > > > > >      Author: Gerd Hoffmann <address@hidden>
> > > > > >      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 <address@hidden>
> > > > > 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