Re: [Qemu-devel] [PATCH] make screendump an async command

[Gerd Hoffmann]

  Hi,

> Why is QXL unable to do a synchronous screendump?

Lazy rendering.  By default spice-server doesn't render anything, it
just sends over all drawing ops to the client who does the actual
rendering for the user.

qemu can kick spice-server and ask it to render stuff locally if needed.
 Happens when either the guest asks for it or when qemu needs it for its
own purposes.  One reason is screendump, the other is qxl being used
with gtk/sdl/vnc ui instead of spice.

Today we kick the spice server worker thread to do the rendering, but we
don't wait for it completing the rendering before writing out the
screendump.  That isn't perfect of course because you are not guaranteed
to get a up-to-date dump.  But works good enougth for some use cases
like autotest, which does screendumps each second anyway, so the actual
screen dump is at most one second old.


Hmm, while thinking about it:  There is another screendump change in the
pipeline: allow screen dumps from *any* device.  So, I think this is
actually a very good reason to implement a new screendump command as I
think we can kill two birds with one stone then:

First we can add a new parameter to specify the device we want dump
from.  And second we are not bound to the behavior of the existing
screendump command.  So we could simply send out a qmp event as
completion (or error) notification.

Comments?

cheers,
  Gerd

> 
> Regards,
> 
> Anthony Liguori
> 
>> ---
>>  hmp.c                     |  2 +-
>>  hw/display/qxl-render.c   |  1 +
>>  hw/display/vga.c          |  1 +
>>  include/qapi/qmp/qerror.h |  6 +++++
>>  include/ui/console.h      | 10 ++++++++
>>  qapi-schema.json          | 13 -----------
>>  qmp-commands.hx           |  3 ++-
>>  ui/console.c              | 58 
>> ++++++++++++++++++++++++++++++++++++++++++++++-
>>  8 files changed, 78 insertions(+), 16 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 494a9aa..2a37975 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -1346,7 +1346,7 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict)
>>      const char *filename = qdict_get_str(qdict, "filename");
>>      Error *err = NULL;
>>  
>> -    qmp_screendump(filename, &err);
>> +    hmp_screen_dump_helper(filename, &err);
>>      hmp_handle_error(mon, &err);
>>  }
>>  
>> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
>> index f511a62..d719448 100644
>> --- a/hw/display/qxl-render.c
>> +++ b/hw/display/qxl-render.c
>> @@ -139,6 +139,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice 
>> *qxl)
>>                         qxl->dirty[i].bottom - qxl->dirty[i].top);
>>      }
>>      qxl->num_dirty_rects = 0;
>> +    console_screendump_complete(vga->con);
>>  }
>>  
>>  /*
>> diff --git a/hw/display/vga.c b/hw/display/vga.c
>> index 21a108d..1fc52eb 100644
>> --- a/hw/display/vga.c
>> +++ b/hw/display/vga.c
>> @@ -1922,6 +1922,7 @@ static void vga_update_display(void *opaque)
>>              break;
>>          }
>>      }
>> +    console_screendump_complete(s->con);
>>  }
>>  
>>  /* force a full display refresh */
>> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
>> index 6c0a18d..1bd7efa 100644
>> --- a/include/qapi/qmp/qerror.h
>> +++ b/include/qapi/qmp/qerror.h
>> @@ -237,6 +237,12 @@ void assert_no_error(Error *err);
>>  #define QERR_VIRTFS_FEATURE_BLOCKS_MIGRATION \
>>      ERROR_CLASS_GENERIC_ERROR, "Migration is disabled when VirtFS export 
>> path '%s' is mounted in the guest using mount_tag '%s'"
>>  
>> +#define QERR_SCREENDUMP_NOT_AVAILABLE \
>> +    ERROR_CLASS_GENERIC_ERROR, "There is no QemuConsole I can screendump 
>> from"
>> +
>> +#define QERR_SCREENDUMP_IN_PROGRESS \
>> +    ERROR_CLASS_GENERIC_ERROR, "Existing screendump in progress"
>> +
>>  #define QERR_SOCKET_CONNECT_FAILED \
>>      ERROR_CLASS_GENERIC_ERROR, "Failed to connect to socket"
>>  
>> diff --git a/include/ui/console.h b/include/ui/console.h
>> index 4307b5f..643da45 100644
>> --- a/include/ui/console.h
>> +++ b/include/ui/console.h
>> @@ -341,4 +341,14 @@ int index_from_keycode(int code);
>>  void early_gtk_display_init(void);
>>  void gtk_display_init(DisplayState *ds);
>>  
>> +/* hw/display/vga.c hw/display/qxl.c */
>> +void console_screendump_complete(QemuConsole *con);
>> +
>> +/* monitor.c */
>> +int qmp_screendump(Monitor *mon, const QDict *qdict,
>> +                   MonitorCompletion cb, void *opaque);
>> +
>> +/* hmp.c */
>> +void hmp_screen_dump_helper(const char *filename, Error **errp);
>> +
>>  #endif
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5ad6894..f5fdc2f 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3112,19 +3112,6 @@
>>    'data': { 'keys': ['KeyValue'], '*hold-time': 'int' } }
>>  
>>  ##
>> -# @screendump:
>> -#
>> -# Write a PPM of the VGA screen to a file.
>> -#
>> -# @filename: the path of a new PPM file to store the image
>> -#
>> -# Returns: Nothing on success
>> -#
>> -# Since: 0.14.0
>> -##
>> -{ 'command': 'screendump', 'data': {'filename': 'str'} }
>> -
>> -##
>>  # @nbd-server-start:
>>  #
>>  # Start an NBD server listening on the given host and port.  Block
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index 8cea5e5..bde8f43 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -146,7 +146,8 @@ EQMP
>>      {
>>          .name       = "screendump",
>>          .args_type  = "filename:F",
>> -        .mhandler.cmd_new = qmp_marshal_input_screendump,
>> +        .mhandler.cmd_async = qmp_screendump,
>> +        .flags      = MONITOR_CMD_ASYNC,
>>      },
>>  
>>  SQMP
>> diff --git a/ui/console.c b/ui/console.c
>> index 28bba6d..0efd588 100644
>> --- a/ui/console.c
>> +++ b/ui/console.c
>> @@ -116,6 +116,12 @@ typedef enum {
>>  struct QemuConsole {
>>      Object parent;
>>  
>> +    struct {
>> +        char *filename;
>> +        MonitorCompletion *completion_cb;
>> +        void *completion_opaque;
>> +    } screendump;
>> +
>>      int index;
>>      console_type_t console_type;
>>      DisplayState *ds;
>> @@ -313,11 +319,61 @@ write_err:
>>      goto out;
>>  }
>>  
>> -void qmp_screendump(const char *filename, Error **errp)
>> +int qmp_screendump(Monitor *mon, const QDict *qdict,
>> +                   MonitorCompletion cb, void *opaque)
>>  {
>>      QemuConsole *con = qemu_console_lookup_by_index(0);
>> +    const char *filename = qdict_get_str(qdict, "filename");
>> +
>> +    if (con == NULL) {
>> +        qerror_report(QERR_SCREENDUMP_NOT_AVAILABLE);
>> +        return -1;
>> +    }
>> +    if (filename == NULL) {
>> +        qerror_report(QERR_MISSING_PARAMETER, "filename");
>> +        return -1;
>> +    }
>> +    if (con->screendump.filename != NULL) {
>> +        qerror_report(QERR_SCREENDUMP_IN_PROGRESS);
>> +        return -1;
>> +    }
>> +    con->screendump.filename = g_strdup(filename);
>> +    con->screendump.completion_cb = cb;
>> +    con->screendump.completion_opaque = opaque;
>> +
>> +    graphic_hw_update(con);
>> +    return 0;
>> +}
>> +
>> +void console_screendump_complete(QemuConsole *con)
>> +{
>> +    Error *errp = NULL;
>>      DisplaySurface *surface;
>>  
>> +    if (!con->screendump.filename) {
>> +        return;
>> +    }
>> +    assert(con->screendump.completion_cb);
>> +    surface = qemu_console_surface(con);
>> +    ppm_save(con->screendump.filename, surface, &errp);
>> +    if (errp) {
>> +        /*
>> +         * TODO: return data? raise error somehow? read qmp-spec for async
>> +         * error reporting.
>> +         */
>> +    }
>> +    con->screendump.completion_cb(con->screendump.completion_opaque, NULL);
>> +    g_free(con->screendump.filename);
>> +    con->screendump.filename = NULL;
>> +    con->screendump.completion_cb = NULL;
>> +    con->screendump.completion_opaque = NULL;
>> +}
>> +
>> +void hmp_screen_dump_helper(const char *filename, Error **errp)
>> +{
>> +    DisplaySurface *surface;
>> +    QemuConsole *con = qemu_console_lookup_by_index(0);
>> +
>>      if (con == NULL) {
>>          error_setg(errp, "There is no QemuConsole I can screendump from.");
>>          return;
>> -- 
>> 1.8.2.1
>