diff mbox

qxl: Fix initial screenshot with spice

Message ID f9fccd0aaff1c192d91ec915b6370da5f8108a10.1398359875.git.crobinso@redhat.com
State New
Headers show

Commit Message

Cole Robinson April 24, 2014, 5:20 p.m. UTC
1) Start an f20 VM with -device qxl-vga + spice, connect with remote-viewer
2) Wait till it boots to gdm
3) (qemu) screendump foo.ppm
4) (qemu) screendump bar.ppm

foo.ppm will actually show the last screen contents before the VM
switched out of VGA mode (right before switching to plymouth). bar.ppm
will show the correct contents.

The root issue is that the DisplaySurface tracked in QemuConsole is out
of date, once qxl transitions out of vga mode nothing calls
qxl_render_update which would update the console surface.

ui/console.c:qmp_screendump attempts to handle this by calling
graphic_hw_update, but qxl handle's that asynchronously, and it isn't
run until after the first screendump is performed. That's why the second
screendump is correct.

Fix this by triggering qxl_render_update whenever a non-vga surface is
created.

Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
All that said, I'm not sure if this is the correct solution, maybe
something else should be responsible for kicking qxl_render_update,
yet isn't.

 hw/display/qxl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Gerd Hoffmann April 25, 2014, 6:39 a.m. UTC | #1
Hi,

> ui/console.c:qmp_screendump attempts to handle this by calling
> graphic_hw_update, but qxl handle's that asynchronously, and it isn't
> run until after the first screendump is performed. That's why the second
> screendump is correct.

Known issue.  The graphic_hw_update makes sure it works sort-of ok when
you do snapshorts regularly (popular use case: autotest).  It's band
aid, but still better than nothing.

> Fix this by triggering qxl_render_update whenever a non-vga surface is
> created.

That only catches a small fraction of the problem.  You'll still face
the very same issue shortly thereafter.  Start firefox in your guest,
then do two snapshots again.  Different variant of the same bug: no
firefox window on the first snapshot.  And this patch doesn't help.

We'll basically need a better snapshot command, but qmp lacks
infrastructure.  We'll need something like blockjobs, but more general
and not limited to the block layer.  Then you can kick off the
screenshot request as job, get notified on completion, and we can get
the qxl case right then.

cheers,
  Gerd
Cole Robinson April 25, 2014, 1:46 p.m. UTC | #2
On 04/25/2014 02:39 AM, Gerd Hoffmann wrote:
>   Hi,
> 
>> ui/console.c:qmp_screendump attempts to handle this by calling
>> graphic_hw_update, but qxl handle's that asynchronously, and it isn't
>> run until after the first screendump is performed. That's why the second
>> screendump is correct.
> 
> Known issue.  The graphic_hw_update makes sure it works sort-of ok when
> you do snapshorts regularly (popular use case: autotest).  It's band
> aid, but still better than nothing.
> 
>> Fix this by triggering qxl_render_update whenever a non-vga surface is
>> created.
> 
> That only catches a small fraction of the problem.  You'll still face
> the very same issue shortly thereafter.  Start firefox in your guest,
> then do two snapshots again.  Different variant of the same bug: no
> firefox window on the first snapshot.  And this patch doesn't help.
> 

In fact I tried that exact test before sending the patch, but the screenshot
seemed up to date. But using something like xdaliclock clearly shows the
outdated surface data.

> We'll basically need a better snapshot command, but qmp lacks
> infrastructure.  We'll need something like blockjobs, but more general
> and not limited to the block layer.  Then you can kick off the
> screenshot request as job, get notified on completion, and we can get
> the qxl case right then.

Makes sense, thanks.

- Cole
diff mbox

Patch

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 47bbf1f..8d3645f 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1355,6 +1355,7 @@  static void qxl_create_guest_primary_complete(PCIQXLDevice *qxl)
 {
     /* for local rendering */
     qxl_render_resize(qxl);
+    qxl_render_update(qxl);
 }
 
 static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm,