Patchwork [PATCHv3] qxl-render/qxl: split out qxl_save_ppm

login
register
mail settings
Submitter Alon Levy
Date July 12, 2011, 1:55 p.m.
Message ID <1310478932-25370-14-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/104394/
State New
Headers show

Comments

Alon Levy - July 12, 2011, 1:55 p.m.
Later the save will happen asynchronously on surface_updated callback.
---
 hw/qxl-render.c |   10 ++++++++++
 hw/qxl.c        |    2 +-
 hw/qxl.h        |    2 ++
 3 files changed, 13 insertions(+), 1 deletions(-)
Gerd Hoffmann - July 13, 2011, 7:10 a.m.
On 07/12/11 15:55, Alon Levy wrote:
> Later the save will happen asynchronously on surface_updated callback.

Hmm.  I can see why you are doing that.  It makes the file being written 
*after* the monitor command finishes though, which I think we should avoid.

cheers,
   Gerd
Alon Levy - July 13, 2011, 9:29 a.m.
On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> On 07/12/11 15:55, Alon Levy wrote:
> >Later the save will happen asynchronously on surface_updated callback.
> 
> Hmm.  I can see why you are doing that.  It makes the file being
> written *after* the monitor command finishes though, which I think
> we should avoid.

I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?

> 
> cheers,
>   Gerd
>
Gerd Hoffmann - July 13, 2011, 10:41 a.m.
On 07/13/11 11:29, Alon Levy wrote:
> On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
>> On 07/12/11 15:55, Alon Levy wrote:
>>> Later the save will happen asynchronously on surface_updated callback.
>>
>> Hmm.  I can see why you are doing that.  It makes the file being
>> written *after* the monitor command finishes though, which I think
>> we should avoid.
>
> I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?

Not sure.  Luiz, do we have async monitor commands meanwhile?

Background: screendump for qxl vga can take a while as the spice-server 
might have to render everything first ...

cheers,
   Gerd
Daniel P. Berrange - July 13, 2011, 10:50 a.m.
On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> On 07/12/11 15:55, Alon Levy wrote:
> >Later the save will happen asynchronously on surface_updated callback.
> 
> Hmm.  I can see why you are doing that.  It makes the file being
> written *after* the monitor command finishes though, which I think
> we should avoid.

Yes, that will very likely break applications using the screenshot
command which expect to be able to load/read the whole image
immediately after the screnshot command returns from the monitor

Daniel
Daniel P. Berrange - July 13, 2011, 10:54 a.m.
On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote:
> On 07/13/11 11:29, Alon Levy wrote:
> >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> >>On 07/12/11 15:55, Alon Levy wrote:
> >>>Later the save will happen asynchronously on surface_updated callback.
> >>
> >>Hmm.  I can see why you are doing that.  It makes the file being
> >>written *after* the monitor command finishes though, which I think
> >>we should avoid.
> >
> >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> 
> Not sure.  Luiz, do we have async monitor commands meanwhile?
> 
> Background: screendump for qxl vga can take a while as the
> spice-server might have to render everything first ...

Another option would be for the screenshot command to allow a
pre-opened FD to be passed in.

So libvirt would create a pipe(2) pass the write end of the
pipe to the 'screenshot' command. The screenshot command
can return from the monitor the moment it has validated that
it can perform the screenshot. QEMU can then write data to the
pipe in the background. libvirt (or equiv app) can likewise
safely read data out of the pipe as it becomes available without
any race condition.

This kind of approach would actually fit better with what libvirt
wants from a screenshot command, because we don't really want to
have the screenshot saved to disk at all. Our API just provides
applications with a stream to data the screenshot data from.
Currently we create a temporary file, save the screenshot to it,
and then immediately unlink it and stream data back to the app
from the deleted file. If we could just use a anonymous pipe it
would be much nicer

Daniel
Alon Levy - July 13, 2011, 11:29 a.m.
On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote:
> On 07/13/11 11:29, Alon Levy wrote:
> >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> >>On 07/12/11 15:55, Alon Levy wrote:
> >>>Later the save will happen asynchronously on surface_updated callback.
> >>
> >>Hmm.  I can see why you are doing that.  It makes the file being
> >>written *after* the monitor command finishes though, which I think
> >>we should avoid.
> >
> >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> 
> Not sure.  Luiz, do we have async monitor commands meanwhile?
> 
> Background: screendump for qxl vga can take a while as the
> spice-server might have to render everything first ...

I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump
even more. Is turning do_screen_dump to async viable? I think I'll work on it.

> 
> cheers,
>   Gerd
>
Gerd Hoffmann - July 13, 2011, 11:46 a.m.
Hi,

> I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
> ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump
> even more. Is turning do_screen_dump to async viable? I think I'll work on it.

Daniel's suggestion is a nice option which goes on top ;)

The filename-based screendump has to continue working for backward 
compatibility reasons.

cheers,
   Gerd
Luiz Capitulino - July 13, 2011, 12:32 p.m.
On Wed, 13 Jul 2011 12:41:48 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

> On 07/13/11 11:29, Alon Levy wrote:
> > On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> >> On 07/12/11 15:55, Alon Levy wrote:
> >>> Later the save will happen asynchronously on surface_updated callback.
> >>
> >> Hmm.  I can see why you are doing that.  It makes the file being
> >> written *after* the monitor command finishes though, which I think
> >> we should avoid.
> >
> > I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> 
> Not sure.  Luiz, do we have async monitor commands meanwhile?

Not yet, this is a QAPI feature that should land soon, but it's not
available yet.

> 
> Background: screendump for qxl vga can take a while as the spice-server 
> might have to render everything first ...
> 
> cheers,
>    Gerd
>
Luiz Capitulino - July 13, 2011, 12:33 p.m.
On Wed, 13 Jul 2011 14:29:16 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote:
> > On 07/13/11 11:29, Alon Levy wrote:
> > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> > >>On 07/12/11 15:55, Alon Levy wrote:
> > >>>Later the save will happen asynchronously on surface_updated callback.
> > >>
> > >>Hmm.  I can see why you are doing that.  It makes the file being
> > >>written *after* the monitor command finishes though, which I think
> > >>we should avoid.
> > >
> > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> > 
> > Not sure.  Luiz, do we have async monitor commands meanwhile?
> > 
> > Background: screendump for qxl vga can take a while as the
> > spice-server might have to render everything first ...
> 
> I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
> ugly.

IIRC, that interface doesn't work as expected and is going to be replaced
by the QAPI's one.

> Also I guess what Daniel described is possible, but it changes the usage of screendump
> even more. Is turning do_screen_dump to async viable? I think I'll work on it.
> 
> > 
> > cheers,
> >   Gerd
> > 
>
Luiz Capitulino - July 13, 2011, 12:39 p.m.
On Wed, 13 Jul 2011 13:46:50 +0200
Gerd Hoffmann <kraxel@redhat.com> wrote:

>    Hi,
> 
> > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
> > ugly. Also I guess what Daniel described is possible, but it changes the usage of screendump
> > even more. Is turning do_screen_dump to async viable? I think I'll work on it.
> 
> Daniel's suggestion is a nice option which goes on top ;)

Some months ago it was suggested that we returned the screenshot as a base64
string. But this might have issues if the screenshot is/becomes too large,
as we're imposing some limits on the json token size (I think it's 64MB today).

Daniel's idea seems to be a good one though.

> 
> The filename-based screendump has to continue working for backward 
> compatibility reasons.
> 
> cheers,
>    Gerd
>
Alon Levy - July 13, 2011, 12:56 p.m.
On Wed, Jul 13, 2011 at 09:33:26AM -0300, Luiz Capitulino wrote:
> On Wed, 13 Jul 2011 14:29:16 +0300
> Alon Levy <alevy@redhat.com> wrote:
> 
> > On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote:
> > > On 07/13/11 11:29, Alon Levy wrote:
> > > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> > > >>On 07/12/11 15:55, Alon Levy wrote:
> > > >>>Later the save will happen asynchronously on surface_updated callback.
> > > >>
> > > >>Hmm.  I can see why you are doing that.  It makes the file being
> > > >>written *after* the monitor command finishes though, which I think
> > > >>we should avoid.
> > > >
> > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> > > 
> > > Not sure.  Luiz, do we have async monitor commands meanwhile?
> > > 
> > > Background: screendump for qxl vga can take a while as the
> > > spice-server might have to render everything first ...
> > 
> > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
> > ugly.
> 
> IIRC, that interface doesn't work as expected and is going to be replaced
> by the QAPI's one.

In what way is in wrong? it seems to work fine here - monitor is blocked until
I call the callback, after which it returns. Didn't test the qmp part though.

> 
> > Also I guess what Daniel described is possible, but it changes the usage of screendump
> > even more. Is turning do_screen_dump to async viable? I think I'll work on it.
> > 
> > > 
> > > cheers,
> > >   Gerd
> > > 
> > 
>
Luiz Capitulino - July 13, 2011, 1:15 p.m.
On Wed, 13 Jul 2011 15:56:55 +0300
Alon Levy <alevy@redhat.com> wrote:

> On Wed, Jul 13, 2011 at 09:33:26AM -0300, Luiz Capitulino wrote:
> > On Wed, 13 Jul 2011 14:29:16 +0300
> > Alon Levy <alevy@redhat.com> wrote:
> > 
> > > On Wed, Jul 13, 2011 at 12:41:48PM +0200, Gerd Hoffmann wrote:
> > > > On 07/13/11 11:29, Alon Levy wrote:
> > > > >On Wed, Jul 13, 2011 at 09:10:19AM +0200, Gerd Hoffmann wrote:
> > > > >>On 07/12/11 15:55, Alon Levy wrote:
> > > > >>>Later the save will happen asynchronously on surface_updated callback.
> > > > >>
> > > > >>Hmm.  I can see why you are doing that.  It makes the file being
> > > > >>written *after* the monitor command finishes though, which I think
> > > > >>we should avoid.
> > > > >
> > > > >I think the simplest thing would be to add a specific cond for this - ppm_save_filename_cond. ok?
> > > > 
> > > > Not sure.  Luiz, do we have async monitor commands meanwhile?
> > > > 
> > > > Background: screendump for qxl vga can take a while as the
> > > > spice-server might have to render everything first ...
> > > 
> > > I'd rather try the MONITOR_CMD_ASYNC thing then the cond variable, it's becoming pretty
> > > ugly.
> > 
> > IIRC, that interface doesn't work as expected and is going to be replaced
> > by the QAPI's one.
> 
> In what way is in wrong? it seems to work fine here - monitor is blocked until
> I call the callback, after which it returns. Didn't test the qmp part though.

One problem is that the error is global and could be overwritten by other
handlers. Another problem is that there's no way for a client to kill an async
handler that got stuck, this could cause a client to wait for the handler
forever.

> 
> > 
> > > Also I guess what Daniel described is possible, but it changes the usage of screendump
> > > even more. Is turning do_screen_dump to async viable? I think I'll work on it.
> > > 
> > > > 
> > > > cheers,
> > > >   Gerd
> > > > 
> > > 
> > 
>
Gerd Hoffmann - July 13, 2011, 1:45 p.m.
On 07/13/11 14:32, Luiz Capitulino wrote:
>> Not sure.  Luiz, do we have async monitor commands meanwhile?
>
> Not yet, this is a QAPI feature that should land soon, but it's not
> available yet.

Hmm.  Alon, is it an option to just leave the whole qxl-render stuff in 
sync mode for now and convert it later?  Or will that have bad 
interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest?

cheers,
   Gerd
Alon Levy - July 13, 2011, 2:10 p.m.
On Wed, Jul 13, 2011 at 03:45:16PM +0200, Gerd Hoffmann wrote:
> On 07/13/11 14:32, Luiz Capitulino wrote:
> >>Not sure.  Luiz, do we have async monitor commands meanwhile?
> >
> >Not yet, this is a QAPI feature that should land soon, but it's not
> >available yet.
> 
> Hmm.  Alon, is it an option to just leave the whole qxl-render stuff
> in sync mode for now and convert it later?  Or will that have bad
> interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest?
> 
It's not a problem. I do have a working version using async monitor command, but
I will gladly put it off if the monitor async is considered harmful.

> cheers,
>   Gerd
>
Gerd Hoffmann - July 13, 2011, 2:25 p.m.
Hi,

>> Hmm.  Alon, is it an option to just leave the whole qxl-render stuff
>> in sync mode for now and convert it later?  Or will that have bad
>> interactions with QXL_IO_UPDATE_AREA_ASYNC being used by the guest?
>>
> It's not a problem. I do have a working version using async monitor command, but
> I will gladly put it off if the monitor async is considered harmful.

Good, so we know the libspice-server API is sane.  Lets put it off for 
now and resume later when QAPI and the other stuff is sorted, async i/o 
commands and s3/s4 support are more important now.

cheers,
   Gerd

Patch

diff --git a/hw/qxl-render.c b/hw/qxl-render.c
index 60b822d..e64b646 100644
--- a/hw/qxl-render.c
+++ b/hw/qxl-render.c
@@ -70,6 +70,15 @@  void qxl_render_resize(PCIQXLDevice *qxl)
     }
 }
 
+static void qxl_save_ppm(PCIQXLDevice *qxl)
+{
+    if (qxl->ppm_save_filename) {
+        ppm_save(qxl->ppm_save_filename, qxl->ssd.ds->surface);
+        free(qxl->ppm_save_filename);
+        qxl->ppm_save_filename = NULL;
+    }
+}
+
 void qxl_render_update(PCIQXLDevice *qxl)
 {
     VGACommonState *vga = &qxl->vga;
@@ -139,6 +148,7 @@  void qxl_render_update(PCIQXLDevice *qxl)
                    dirty[i].right - dirty[i].left,
                    dirty[i].bottom - dirty[i].top);
     }
+    qxl_save_ppm(qxl);
 }
 
 static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor)
diff --git a/hw/qxl.c b/hw/qxl.c
index bd540c0..de93efa 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1462,8 +1462,8 @@  static void qxl_hw_screen_dump(void *opaque, const char *filename)
     switch (qxl->mode) {
     case QXL_MODE_COMPAT:
     case QXL_MODE_NATIVE:
+        qxl->ppm_save_filename = qemu_strdup(filename);
         qxl_render_update(qxl);
-        ppm_save(filename, qxl->ssd.ds->surface);
         break;
     case QXL_MODE_VGA:
         vga->screen_dump(vga, filename);
diff --git a/hw/qxl.h b/hw/qxl.h
index 064311a..2c7f94a 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -32,6 +32,8 @@  typedef struct PCIQXLDevice {
     int32_t            num_memslots;
     int32_t            num_surfaces;
 
+    char              *ppm_save_filename;
+
 #if SPICE_INTERFACE_QXL_MINOR >= 1
     uint32_t           current_async;
     QemuMutex          async_lock;