[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] make screendump an async command
|
From: |
Alon Levy |
|
Subject: |
Re: [Qemu-devel] [PATCH] make screendump an async command |
|
Date: |
Fri, 14 Jun 2013 19:55:21 -0400 |
On Fri, 2013-06-14 at 13:21 -0500, Anthony Liguori wrote:
> Alon Levy <address@hidden> writes:
>
> > This fixes the broken screendump behavior for qxl devices in native mode
> > since 81fb6f1504fb9ef71f2382f44af34756668296e8.
> >
> > Note: due to QAPI not generating async commands yet I had to remove the
> > schema screendump definition.
> >
> > Related RHBZ: 973374
> > This patch is not enough to fix said bz, with the linux qxl driver you get
> > garbage still, just not out of date garbage.
> >
> > Signed-off-by: Alon Levy <address@hidden>
>
> Async commands are badly broken with respect to error handling.
Are there any ideas on how this should work? From the user perspective
nothing changes, so this is an internal qemu design issue afaict.
>
> This needs to be done after we get the new QMP server.
Is there any ETA on this? I'm not pressuring, I'm just trying to figure
out if it is eminent or else I'll add a temporary patch to the
downstream (which is what I'm trying to avoid by sending this patch in
the first place).
>
> Why is QXL unable to do a synchronous screendump?
The qxl device needs to flush all the commands that haven't been
rendered. The rendering is done off thread in the worker thread for that
device. The communication between the threads happens via a pipe that
the io thread is listening to. That is the same thread the qmp command
is received on, so there is no option to block unless I start a new
event loop in there, which is ugly. i.e. I need some kind of bottom
half, like the async command provides, which client_migrate_info uses.
>
> 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
>
>