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

[Peter Lieven <pl@kamp.de>]

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