* [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend
@ 2012-03-11 16:39 Alon Levy
2012-03-11 16:39 ` [Qemu-devel] [PATCH 1/4] qxl: switch qxl.c to trace-events Alon Levy
` (5 more replies)
0 siblings, 6 replies; 35+ messages in thread
From: Alon Levy @ 2012-03-11 16:39 UTC (permalink / raw)
To: qemu-devel, kraxel, lcapitulino, aliguori
This patchset starts and ends with trace event additions that make it easier to see the change.
It applies on top of http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01784.html
due to trace-events.
The problem addressed by this patchset is that after recent fixes (81fb6f)
screendump with a qxl device in native mode saves a stale screen dump.
The solution is to use monitor_suspend and monitor_release in qxl's implementation.
This is done by:
1. introducing an extra parameter to vga_hw_screen_dump & hw_vga_dump
console: pass Monitor to vga_hw_screen_dump/hw_vga_dump
2. using it in qxl via a bh.
qxl-render: call ppm_save on bh
Additional patches add trace events to qxl and qxl_render, making it easy to see the difference:
events setup: (using stderr backend)
(qemu) trace-event ppm_save on
(qemu) trace-event qxl* on
(qemu) trace-event qxl_interface_get_command_enter off
(qemu) trace-event qxl_interface_release_resource off
(qemu) trace-event qxl_interface_get_command_ret off
before: ppm_save done before update:
(qemu) screendump /tmp/a.ppm
ppm_save /tmp/a.ppm surface=0x7fc0267b3ad0
qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1
qxl_interface_update_area_complete_schedule_bh #dirty=1
qxl_render_update_area_done 0x7fc02b603db0
(qemu) qxl_blit stride=-2560 [152, 160, 464, 480]
after:
(qemu) screendump /tmp/a.ppm
qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1
qxl_interface_update_area_complete_schedule_bh #dirty=1
qxl_render_update_area_done 0x7f407af72210
qxl_render_ppm_save_bh 0x7f407f845b60 (primary 0x7f401bc00000)
qxl_blit stride=-2560 [152, 160, 464, 480]
ppm_save /tmp/a.ppm surface=0x7f4077204ad0
(qemu)
Note: This doesn't address a possible libvirt problem that was mentioned in
length before, but since it has not been reproduced it will be fixed when it
is. Meanwhile other users like autotest will be fixed by this patch (by fix I
mean screendump will produce the correct output).
Alon Levy (4):
qxl: switch qxl.c to trace-events
qxl/qxl_render.c: add trace events
console: pass Monitor to vga_hw_screen_dump/hw_vga_dump
qxl-render: call ppm_save on bh
console.c | 4 +-
console.h | 5 +-
hw/blizzard.c | 2 +-
hw/g364fb.c | 3 +-
hw/omap_dss.c | 4 +-
hw/omap_lcdc.c | 3 +-
hw/qxl-render.c | 95 +++++++++++++++++++++++++++------
hw/qxl.c | 150 ++++++++++++++++++++++++---------------------------
hw/qxl.h | 2 +-
hw/sm501.c | 4 +-
hw/tcx.c | 12 +++--
hw/vga.c | 6 ++-
hw/vmware_vga.c | 5 +-
monitor.c | 2 +-
trace-events | 55 +++++++++++++++++++
ui/spice-display.h | 3 +
16 files changed, 240 insertions(+), 115 deletions(-)
--
1.7.9.1
^ permalink raw reply [flat|nested] 35+ messages in thread* [Qemu-devel] [PATCH 1/4] qxl: switch qxl.c to trace-events 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy @ 2012-03-11 16:39 ` Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 2/4] qxl/qxl_render.c: add trace events Alon Levy ` (4 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 16:39 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori dprint is still used for qxl_init_common one time prints. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl.c | 140 +++++++++++++++++++++++++++------------------------------ trace-events | 47 +++++++++++++++++++ 2 files changed, 113 insertions(+), 74 deletions(-) diff --git a/hw/qxl.c b/hw/qxl.c index e17b0e3..7857731 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -23,6 +23,7 @@ #include "qemu-queue.h" #include "monitor.h" #include "sysemu.h" +#include "trace.h" #include "qxl.h" @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s:\n", __FUNCTION__); + trace_qxl_interface_attach_worker(); qxl->ssd.worker = qxl_worker; } @@ -417,7 +418,7 @@ static void interface_set_compression_level(QXLInstance *sin, int level) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s: %d\n", __FUNCTION__, level); + trace_qxl_interface_set_compression_level(level); qxl->shadow_rom.compression_level = cpu_to_le32(level); qxl->rom->compression_level = cpu_to_le32(level); qxl_rom_set_dirty(qxl); @@ -436,7 +437,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s:\n", __FUNCTION__); + trace_qxl_interface_get_init_info(); info->memslot_gen_bits = MEMSLOT_GENERATION_BITS; info->memslot_id_bits = MEMSLOT_SLOT_BITS; info->num_memslots = NUM_MEMSLOTS; @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) QXLCommand *cmd; int notify, ret; + trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode)); + switch (qxl->mode) { case QXL_MODE_VGA: - dprint(qxl, 2, "%s: vga\n", __FUNCTION__); ret = false; qemu_mutex_lock(&qxl->ssd.lock); if (qxl->ssd.update != NULL) { @@ -518,19 +520,18 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) } qemu_mutex_unlock(&qxl->ssd.lock); if (ret) { - dprint(qxl, 2, "%s %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); + trace_qxl_interface_get_command_ret(qxl_mode_to_string(qxl->mode)); qxl_log_command(qxl, "vga", ext); } return ret; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: case QXL_MODE_UNDEFINED: - dprint(qxl, 4, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); ring = &qxl->ram->cmd_ring; if (SPICE_RING_IS_EMPTY(ring)) { return false; } - dprint(qxl, 2, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); + trace_qxl_interface_get_command_ret(qxl_mode_to_string(qxl->mode)); SPICE_RING_CONS_ITEM(ring, cmd); ext->cmd = *cmd; ext->group_id = MEMSLOT_GROUP_GUEST; @@ -592,7 +593,7 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) } SPICE_RING_PUSH(ring, notify); - dprint(d, 2, "free: push %d items, notify %s, ring %d/%d [%d,%d]\n", + trace_qxl_push_free_res( d->num_free_res, notify ? "yes" : "no", ring->prod - ring->cons, ring->num_items, ring->prod, ring->cons); @@ -642,7 +643,7 @@ static void interface_release_resource(QXLInstance *sin, } qxl->last_release = ext.info; qxl->num_free_res++; - dprint(qxl, 3, "%4d\r", qxl->num_free_res); + trace_qxl_interface_release_resource(qxl->num_free_res); qxl_push_free_res(qxl, 0); } @@ -716,7 +717,7 @@ static int interface_flush_resources(QXLInstance *sin) PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); int ret; - dprint(qxl, 1, "free: guest flush (have %d)\n", qxl->num_free_res); + trace_qxl_interface_flush_resources(qxl->num_free_res); ret = qxl->num_free_res; if (ret) { qxl_push_free_res(qxl, 1); @@ -736,7 +737,7 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie) qxl->current_async = QXL_UNDEFINED_IO; qemu_mutex_unlock(&qxl->async_lock); - dprint(qxl, 2, "async_complete: %d (%p) done\n", current_async, cookie); + trace_qxl_interface_async_complete_io(current_async, cookie); if (!cookie) { fprintf(stderr, "qxl: %s: error, cookie is NULL\n", __func__); return; @@ -782,11 +783,13 @@ static void interface_update_area_complete(QXLInstance *sin, qemu_mutex_unlock(&qxl->ssd.lock); return; } + trace_qxl_interface_update_area_complete(surface_id, dirty->left, + dirty->right, dirty->top, dirty->bottom, num_updated_rects); if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) { /* * overflow - treat this as a full update. Not expected to be common. */ - dprint(qxl, 1, "%s: overflow of dirty rects\n", __func__); + trace_qxl_interface_update_area_complete_overflow(QXL_NUM_DIRTY_RECTS); qxl->guest_primary.resized = 1; } if (qxl->guest_primary.resized) { @@ -802,8 +805,7 @@ static void interface_update_area_complete(QXLInstance *sin, qxl->dirty[qxl_i++] = dirty[i]; } qxl->num_dirty_rects += num_updated_rects; - dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", - __func__, qxl->num_dirty_rects); + trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects); qemu_bh_schedule(qxl->update_area_bh); qemu_mutex_unlock(&qxl->ssd.lock); } @@ -857,7 +859,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d) if (d->mode == QXL_MODE_VGA) { return; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_enter_vga_mode(); qemu_spice_create_host_primary(&d->ssd); d->mode = QXL_MODE_VGA; memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty)); @@ -868,7 +870,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d) if (d->mode != QXL_MODE_VGA) { return; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_exit_vga_mode(); qxl_destroy_primary(d, QXL_SYNC); } @@ -905,7 +907,7 @@ static void qxl_reset_state(PCIQXLDevice *d) static void qxl_soft_reset(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_soft_reset(); qxl_check_state(d); if (d->id == 0) { @@ -917,8 +919,7 @@ static void qxl_soft_reset(PCIQXLDevice *d) static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) { - dprint(d, 1, "%s: start%s\n", __FUNCTION__, - loadvm ? " (loadvm)" : ""); + trace_qxl_hard_reset_enter(loadvm); qxl_spice_reset_cursor(d); qxl_spice_reset_image_cache(d); @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) qemu_spice_create_host_memslot(&d->ssd); qxl_soft_reset(d); - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_hard_reset_exit(); } static void qxl_reset_handler(DeviceState *dev) @@ -949,7 +950,7 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) PCIQXLDevice *qxl = container_of(vga, PCIQXLDevice, vga); if (qxl->mode != QXL_MODE_VGA) { - dprint(qxl, 1, "%s\n", __FUNCTION__); + trace_qxl_vga_ioport_while_not_in_vga_mode(); qxl_destroy_primary(qxl, QXL_SYNC); qxl_soft_reset(qxl); } @@ -990,9 +991,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, guest_start = le64_to_cpu(d->guest_slots[slot_id].slot.mem_start); guest_end = le64_to_cpu(d->guest_slots[slot_id].slot.mem_end); - dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 "\n", - __FUNCTION__, slot_id, - guest_start, guest_end); + trace_qxl_add_memslot_guest(slot_id, guest_start, guest_end); PANIC_ON(slot_id >= NUM_MEMSLOTS); PANIC_ON(guest_start > guest_end); @@ -1039,9 +1038,8 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, memslot.generation = d->rom->slot_generation = 0; qxl_rom_set_dirty(d); - dprint(d, 1, "%s: slot %d: host virt 0x%lx - 0x%lx\n", - __FUNCTION__, memslot.slot_id, - memslot.virt_start, memslot.virt_end); + trace_qxl_add_memslot_host(memslot.slot_id, memslot.virt_start, + memslot.virt_end); qemu_spice_add_memslot(&d->ssd, &memslot, async); d->guest_slots[slot_id].ptr = (void*)memslot.virt_start; @@ -1052,21 +1050,21 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, static void qxl_del_memslot(PCIQXLDevice *d, uint32_t slot_id) { - dprint(d, 1, "%s: slot %d\n", __FUNCTION__, slot_id); + trace_qxl_del_memslot(slot_id); qemu_spice_del_memslot(&d->ssd, MEMSLOT_GROUP_HOST, slot_id); d->guest_slots[slot_id].active = 0; } static void qxl_reset_memslots(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_reset_memslots(); qxl_spice_reset_memslots(d); memset(&d->guest_slots, 0, sizeof(d->guest_slots)); } static void qxl_reset_surfaces(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_reset_surfaces(); d->mode = QXL_MODE_UNDEFINED; qxl_spice_destroy_surfaces(d, QXL_SYNC); } @@ -1108,9 +1106,6 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm, assert(qxl->mode != QXL_MODE_NATIVE); qxl_exit_vga_mode(qxl); - dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__, - le32_to_cpu(sc->width), le32_to_cpu(sc->height)); - surface.format = le32_to_cpu(sc->format); surface.height = le32_to_cpu(sc->height); surface.mem = le64_to_cpu(sc->mem); @@ -1119,6 +1114,9 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm, surface.width = le32_to_cpu(sc->width); surface.type = le32_to_cpu(sc->type); surface.flags = le32_to_cpu(sc->flags); + trace_qxl_create_guest_primary(sc->width, sc->height, sc->mem, sc->format, + sc->position, sc->stride, sc->type, + sc->flags); surface.mouse_mode = true; surface.group_id = MEMSLOT_GROUP_GUEST; @@ -1142,7 +1140,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async) if (d->mode == QXL_MODE_UNDEFINED) { return 0; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_destroy_primary(); d->mode = QXL_MODE_UNDEFINED; qemu_spice_destroy_primary_surface(&d->ssd, 0, async); qxl_spice_reset_cursor(d); @@ -1169,8 +1167,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm) .mem = devmem + d->shadow_rom.draw_area_offset, }; - dprint(d, 1, "%s: mode %d [ %d x %d @ %d bpp devmem 0x%" PRIx64 " ]\n", - __func__, modenr, mode->x_res, mode->y_res, mode->bits, devmem); + trace_qxl_set_mode(modenr, mode->x_res, mode->y_res, mode->bits, devmem); if (!loadvm) { qxl_hard_reset(d, 0); } @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr, if (d->mode != QXL_MODE_VGA) { break; } - dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n", - __func__, io_port, io_port_to_string(io_port)); + trace_qxl_io_unexpected_vga_mode( + io_port, io_port_to_string(io_port)); /* be nice to buggy guest drivers */ if (io_port >= QXL_IO_UPDATE_AREA_ASYNC && io_port <= QXL_IO_DESTROY_ALL_SURFACES_ASYNC) { @@ -1259,7 +1256,7 @@ async_common: } d->current_async = orig_io_port; qemu_mutex_unlock(&d->async_lock); - dprint(d, 2, "start async %d (%"PRId64")\n", io_port, val); + trace_qxl_io_start_async(io_port, val); break; default: break; @@ -1299,7 +1296,7 @@ async_common: d->oom_running = 0; break; case QXL_IO_SET_MODE: - dprint(d, 1, "QXL_SET_MODE %d\n", (int)val); + trace_qxl_io_set_mode(val); qxl_set_mode(d, val, 0); break; case QXL_IO_LOG: @@ -1309,7 +1306,7 @@ async_common: } break; case QXL_IO_RESET: - dprint(d, 1, "QXL_IO_RESET\n"); + trace_qxl_io_reset(); qxl_hard_reset(d, 0); break; case QXL_IO_MEMSLOT_ADD: @@ -1337,7 +1334,7 @@ async_common: async); goto cancel_async; } - dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async); + trace_qxl_io_create_primary(async); d->guest_primary.surface = d->ram->create_surface; qxl_create_guest_primary(d, 0, async); break; @@ -1347,11 +1344,11 @@ async_common: async); goto cancel_async; } - dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (async=%d) (%s)\n", async, - qxl_mode_to_string(d->mode)); + trace_qxl_io_destroy_primary(async, + qxl_mode_to_string(d->mode)); if (!qxl_destroy_primary(d, async)) { - dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC in %s, ignored\n", - qxl_mode_to_string(d->mode)); + trace_qxl_io_destroy_primary_ignored( + qxl_mode_to_string(d->mode)); goto cancel_async; } break; @@ -1371,14 +1368,12 @@ async_common: ring->prod, ring->cons); } qxl_push_free_res(d, 1 /* flush */); - dprint(d, 1, "QXL_IO_FLUSH_RELEASE exit (%s, s#=%d, res#=%d,%p)\n", - qxl_mode_to_string(d->mode), d->guest_surfaces.count, - d->num_free_res, d->last_release); + trace_qxl_io_flush_release(qxl_mode_to_string(d->mode), + d->guest_surfaces.count, d->num_free_res, d->last_release); break; } case QXL_IO_FLUSH_SURFACES_ASYNC: - dprint(d, 1, "QXL_IO_FLUSH_SURFACES_ASYNC" - " (%"PRId64") (%s, s#=%d, res#=%d)\n", + trace_qxl_io_flush_surfaces_async( val, qxl_mode_to_string(d->mode), d->guest_surfaces.count, d->num_free_res); qxl_spice_flush_surfaces_async(d); @@ -1404,9 +1399,7 @@ cancel_async: static uint64_t ioport_read(void *opaque, target_phys_addr_t addr, unsigned size) { - PCIQXLDevice *d = opaque; - - dprint(d, 1, "%s: unexpected\n", __FUNCTION__); + trace_qxl_io_read_unexpected(); return 0xff; } @@ -1445,23 +1438,24 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events) qxl_update_irq(d); } else { if (write(d->pipe[1], d, 1) != 1) { - dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__); + trace_qxl_send_events_write_to_pipe_failed(); } } } static void init_pipe_signaling(PCIQXLDevice *d) { - if (pipe(d->pipe) < 0) { - dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__); - return; - } - fcntl(d->pipe[0], F_SETFL, O_NONBLOCK); - fcntl(d->pipe[1], F_SETFL, O_NONBLOCK); - fcntl(d->pipe[0], F_SETOWN, getpid()); + if (pipe(d->pipe) < 0) { + fprintf(stderr, "%s:%s: qxl pipe creation failed\n", + __FILE__, __func__); + exit(1); + } + fcntl(d->pipe[0], F_SETFL, O_NONBLOCK); + fcntl(d->pipe[1], F_SETFL, O_NONBLOCK); + fcntl(d->pipe[0], F_SETOWN, getpid()); - qemu_thread_get_self(&d->main); - qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d); + qemu_thread_get_self(&d->main); + qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d); } /* graphics console */ @@ -1556,8 +1550,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl) surface_offset -= vram_start; surface_size = cmd->u.surface_create.height * abs(cmd->u.surface_create.stride); - dprint(qxl, 3, "%s: dirty surface %d, offset %d, size %d\n", __func__, - i, (int)surface_offset, surface_size); + trace_qxl_dirty_surfaces(i, (int)surface_offset, surface_size); qxl_set_dirty(&qxl->vram_bar, surface_offset, surface_size); } } @@ -1791,7 +1784,7 @@ static void qxl_pre_save(void *opaque) PCIQXLDevice* d = opaque; uint8_t *ram_start = d->vga.vram_ptr; - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_pre_save(); if (d->last_release == NULL) { d->last_release_offset = 0; } else { @@ -1804,10 +1797,10 @@ static int qxl_pre_load(void *opaque) { PCIQXLDevice* d = opaque; - dprint(d, 1, "%s: start\n", __FUNCTION__); + trace_qxl_pre_load_enter(); qxl_hard_reset(d, 1); qxl_exit_vga_mode(d); - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_pre_load_exit(); return 0; } @@ -1819,7 +1812,7 @@ static void qxl_create_memslots(PCIQXLDevice *d) if (!d->guest_slots[i].active) { continue; } - dprint(d, 1, "%s: restoring guest slot %d\n", __func__, i); + trace_qxl_create_memslots(i); qxl_add_memslot(d, i, 0, QXL_SYNC); } } @@ -1831,7 +1824,7 @@ static int qxl_post_load(void *opaque, int version) QXLCommandExt *cmds; int in, out, newmode; - dprint(d, 1, "%s: start\n", __FUNCTION__); + trace_qxl_post_load_enter(); assert(d->last_release_offset < d->vga.vram_size); if (d->last_release_offset == 0) { @@ -1842,8 +1835,7 @@ static int qxl_post_load(void *opaque, int version) d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset); - dprint(d, 1, "%s: restore mode (%s)\n", __FUNCTION__, - qxl_mode_to_string(d->mode)); + trace_qxl_post_load_restore_mode(qxl_mode_to_string(d->mode)); newmode = d->mode; d->mode = QXL_MODE_UNDEFINED; @@ -1885,7 +1877,7 @@ static int qxl_post_load(void *opaque, int version) qxl_set_mode(d, d->shadow_rom.mode, 1); break; } - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_post_load_exit(); return 0; } diff --git a/trace-events b/trace-events index dfe28ed..0853a1b 100644 --- a/trace-events +++ b/trace-events @@ -665,3 +665,50 @@ displaysurface_resize(void *display_state, void *display_surface, int width, int # vga.c ppm_save(const char *filename, void *display_surface) "%s surface=%p" + +# hw/qxl.c +qxl_io_create_primary(bool async) "async=%d" +qxl_create_guest_primary(uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position, int32_t stride, uint32_t type, uint32_t flags) "%dx%d mem=%lx %d,%d,%d,%d,%d" +qxl_interface_attach_worker(void) "" +qxl_interface_set_compression_level(int64_t level) "%"PRId64 +qxl_interface_get_init_info(void) "" +qxl_enter_vga_mode(void) "" +qxl_exit_vga_mode(void) "" +qxl_soft_reset(void) "" +qxl_hard_reset_enter(int64_t loadvm) "loadvm=%"PRId64"" +qxl_hard_reset_exit(void) "" +qxl_vga_ioport_while_not_in_vga_mode(void) "(reset to VGA mode because of VGA io)" +qxl_add_memslot_guest(uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%u: guest phys 0x%"PRIx64 " - 0x%" PRIx64 +qxl_add_memslot_host(uint32_t slot_id, unsigned long virt_start, unsigned long virt_end) "%u: host virt 0x%lx - 0x%lx" +qxl_del_memslot(uint32_t slot_id) "%u" +qxl_reset_memslots(void) "" +qxl_reset_surfaces(void) "" +qxl_destroy_primary(void) "" +qxl_set_mode(int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]" +qxl_io_unexpected_vga_mode(uint32_t io_port, const char *desc) "0x%x (%s)" +qxl_io_start_async(uint32_t io_port, int64_t val) "%u (%"PRId64")" +qxl_io_set_mode(uint64_t val) "%"PRIu64 +qxl_io_reset(void) "" +qxl_io_destroy_primary(int async, const char *mode) "%d (%s)" +qxl_io_destroy_primary_ignored(const char *mode) "%s" +qxl_io_flush_release(const char *mode, uint32_t surface_count, uint32_t num_free_res, void *last_release) "flush complete %s, s#=%d, res#=%d, %p" +qxl_io_flush_surfaces_async(uint64_t val, const char *mode, uint32_t surface_count, uint32_t num_free_res) "val=%"PRIu64", %s, s#=%d, res#=%d" +qxl_io_read_unexpected(void) "" +qxl_send_events_write_to_pipe_failed(void) "" +qxl_interface_get_command_enter(const char *mode) "%s" +qxl_interface_get_command_ret(const char *mode) "%s" +qxl_push_free_res(uint32_t free_res, const char *notify, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d items, notify %s, ring %d/%d [%d,%d]" +qxl_interface_release_resource(uint32_t free_res) "%4d" +qxl_interface_flush_resources(uint32_t free_res) "%d" +qxl_interface_async_complete_io(uint32_t current_async, void *cookie) "%d (%p) done" +qxl_interface_update_area_complete_overflow(int max) "(%d)" +qxl_interface_update_area_complete_schedule_bh(uint32_t num_dirty) "#dirty=%d" +qxl_dirty_surfaces(int surface, int offset, int size) "surface=%d offset=%d size=%d" +qxl_pre_save(void) "" +qxl_pre_load_enter(void) "" +qxl_pre_load_exit(void) "" +qxl_create_memslots(int slot) "%d" +qxl_post_load_enter(void) "" +qxl_post_load_restore_mode(const char *mode) "%s" +qxl_post_load_exit(void) "" +qxl_interface_update_area_complete(uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom, uint32_t num_updated_rects) "surface=%d [%d,%d,%d,%d] #=%d" -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 2/4] qxl/qxl_render.c: add trace events 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 1/4] qxl: switch qxl.c to trace-events Alon Levy @ 2012-03-11 16:39 ` Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 3/4] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy ` (3 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 16:39 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 13 ++++--------- trace-events | 6 ++++++ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 25857f6..74e7ea3 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -31,11 +31,10 @@ static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect) return; } if (!qxl->guest_primary.data) { - dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__); + trace_qxl_blit_guest_primary_initialized(); qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); } - dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__, - qxl->guest_primary.qxl_stride, + trace_qxl_blit(qxl->guest_primary.qxl_stride, rect->left, rect->right, rect->top, rect->bottom); src = qxl->guest_primary.data; if (qxl->guest_primary.qxl_stride < 0) { @@ -107,8 +106,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); qxl_set_rect_to_surface(qxl, &qxl->dirty[0]); qxl->num_dirty_rects = 1; - dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d\n", - __FUNCTION__, + trace_qxl_guest_primary_resized( qxl->guest_primary.surface.width, qxl->guest_primary.surface.height, qxl->guest_primary.qxl_stride, @@ -118,8 +116,6 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) if (surface->width != qxl->guest_primary.surface.width || surface->height != qxl->guest_primary.surface.height) { if (qxl->guest_primary.qxl_stride > 0) { - dprint(qxl, 1, "%s: using guest_primary for displaysurface\n", - __func__); qemu_free_displaysurface(vga->ds); qemu_create_displaysurface_from(qxl->guest_primary.surface.width, qxl->guest_primary.surface.height, @@ -127,8 +123,6 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) qxl->guest_primary.abs_stride, qxl->guest_primary.data); } else { - dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n", - __func__); qemu_resize_displaysurface(vga->ds, qxl->guest_primary.surface.width, qxl->guest_primary.surface.height); @@ -187,6 +181,7 @@ void qxl_render_update_area_bh(void *opaque) void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) { qemu_mutex_lock(&qxl->ssd.lock); + trace_qxl_render_update_area_done(cookie); qemu_bh_schedule(qxl->update_area_bh); qxl->render_update_cookie_num--; qemu_mutex_unlock(&qxl->ssd.lock); diff --git a/trace-events b/trace-events index 0853a1b..a66aee8 100644 --- a/trace-events +++ b/trace-events @@ -712,3 +712,9 @@ qxl_post_load_enter(void) "" qxl_post_load_restore_mode(const char *mode) "%s" qxl_post_load_exit(void) "" qxl_interface_update_area_complete(uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom, uint32_t num_updated_rects) "surface=%d [%d,%d,%d,%d] #=%d" + +# hw/qxl-render.c +qxl_blit_guest_primary_initialized(void) "" +qxl_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]" +qxl_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d" +qxl_render_update_area_done(void *cookie) "%p" -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 3/4] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 1/4] qxl: switch qxl.c to trace-events Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 2/4] qxl/qxl_render.c: add trace events Alon Levy @ 2012-03-11 16:39 ` Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 4/4] qxl-render: call ppm_save on bh Alon Levy ` (2 subsequent siblings) 5 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 16:39 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori Passes the Monitor ptr to the screendump implementation to all for monitor suspend and resume for qxl to fix screendump regression. graphics_console_init signature change required touching every implemented of screen_dump. There is no change other then an added parameter. qxl will make use of it in the next patch. compiles with ./configure Signed-off-by: Alon Levy <alevy@redhat.com> --- console.c | 4 ++-- console.h | 5 +++-- hw/blizzard.c | 2 +- hw/g364fb.c | 3 ++- hw/omap_dss.c | 4 +++- hw/omap_lcdc.c | 3 ++- hw/qxl.c | 5 +++-- hw/sm501.c | 4 ++-- hw/tcx.c | 12 ++++++++---- hw/vga.c | 6 ++++-- hw/vmware_vga.c | 5 +++-- monitor.c | 2 +- 12 files changed, 34 insertions(+), 21 deletions(-) diff --git a/console.c b/console.c index 6a463f5..3e386fc 100644 --- a/console.c +++ b/console.c @@ -173,7 +173,7 @@ void vga_hw_invalidate(void) active_console->hw_invalidate(active_console->hw); } -void vga_hw_screen_dump(const char *filename) +void vga_hw_screen_dump(const char *filename, Monitor *mon) { TextConsole *previous_active_console; bool cswitch; @@ -187,7 +187,7 @@ void vga_hw_screen_dump(const char *filename) console_select(0); } if (consoles[0] && consoles[0]->hw_screen_dump) { - consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch); + consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, mon); } else { error_report("screen dump not implemented"); } diff --git a/console.h b/console.h index 4334db5..0d2cf30 100644 --- a/console.h +++ b/console.h @@ -343,7 +343,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch) typedef void (*vga_hw_update_ptr)(void *); typedef void (*vga_hw_invalidate_ptr)(void *); -typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch); +typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch, + Monitor *mon); typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *); DisplayState *graphic_console_init(vga_hw_update_ptr update, @@ -354,7 +355,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, void vga_hw_update(void); void vga_hw_invalidate(void); -void vga_hw_screen_dump(const char *filename); +void vga_hw_screen_dump(const char *filename, Monitor *mon); void vga_hw_text_update(console_ch_t *chardata); int is_graphic_console(void); diff --git a/hw/blizzard.c b/hw/blizzard.c index c7d844d..8ccea7f 100644 --- a/hw/blizzard.c +++ b/hw/blizzard.c @@ -933,7 +933,7 @@ static void blizzard_update_display(void *opaque) } static void blizzard_screen_dump(void *opaque, const char *filename, - bool cswitch) + bool cswitch, Monitor *mon) { BlizzardState *s = (BlizzardState *) opaque; diff --git a/hw/g364fb.c b/hw/g364fb.c index 3a0b68f..f89000c 100644 --- a/hw/g364fb.c +++ b/hw/g364fb.c @@ -289,7 +289,8 @@ static void g364fb_reset(G364State *s) g364fb_invalidate_display(s); } -static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch) +static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { G364State *s = opaque; int y, x; diff --git a/hw/omap_dss.c b/hw/omap_dss.c index 86ed6ea..b4a1a93 100644 --- a/hw/omap_dss.c +++ b/hw/omap_dss.c @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta, #if 0 s->state = graphic_console_init(omap_update_display, - omap_invalidate_display, omap_screen_dump, s); + omap_invalidate_display, + omap_screen_dump, + NULL, s); #endif return s; diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c index f172093..ed2325d 100644 --- a/hw/omap_lcdc.c +++ b/hw/omap_lcdc.c @@ -264,7 +264,8 @@ static int ppm_save(const char *filename, uint8_t *data, return 0; } -static void omap_screen_dump(void *opaque, const char *filename, bool cswitch) +static void omap_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { struct omap_lcd_panel_s *omap_lcd = opaque; if (cswitch) { diff --git a/hw/qxl.c b/hw/qxl.c index 7857731..cabea3b 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1486,7 +1486,8 @@ static void qxl_hw_invalidate(void *opaque) vga->invalidate(vga); } -static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch) +static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { PCIQXLDevice *qxl = opaque; VGACommonState *vga = &qxl->vga; @@ -1498,7 +1499,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch) ppm_save(filename, qxl->ssd.ds->surface); break; case QXL_MODE_VGA: - vga->screen_dump(vga, filename, cswitch); + vga->screen_dump(vga, filename, cswitch, mon); break; default: break; diff --git a/hw/sm501.c b/hw/sm501.c index 786e076..eedcb8e 100644 --- a/hw/sm501.c +++ b/hw/sm501.c @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, } /* create qemu graphic console */ - s->ds = graphic_console_init(sm501_update_display, NULL, - NULL, NULL, s); + s->ds = graphic_console_init(sm501_update_display, NULL, NULL, + NULL, s); } diff --git a/hw/tcx.c b/hw/tcx.c index ac7dcb4..e800cb5 100644 --- a/hw/tcx.c +++ b/hw/tcx.c @@ -56,8 +56,10 @@ typedef struct TCXState { uint8_t dac_index, dac_state; } TCXState; -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch); -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch); +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); static void tcx_set_dirty(TCXState *s) { @@ -574,7 +576,8 @@ static int tcx_init1(SysBusDevice *dev) return 0; } -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch) +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { TCXState *s = opaque; FILE *f; @@ -601,7 +604,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch) return; } -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch) +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { TCXState *s = opaque; FILE *f; diff --git a/hw/vga.c b/hw/vga.c index 6dc98f6..9af231e 100644 --- a/hw/vga.c +++ b/hw/vga.c @@ -163,7 +163,8 @@ static uint32_t expand4[256]; static uint16_t expand2[256]; static uint8_t expand4to8[16]; -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch); +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); static void vga_update_memory_access(VGACommonState *s) { @@ -2409,7 +2410,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds) /* save the vga display in a PPM image even if no display is available */ -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch) +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { VGACommonState *s = opaque; diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index 142d9f4..803118c 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -1003,11 +1003,12 @@ static void vmsvga_invalidate_display(void *opaque) /* save the vga display in a PPM image even if no display is available */ -static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch) +static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { struct vmsvga_state_s *s = opaque; if (!s->enable) { - s->vga.screen_dump(&s->vga, filename, cswitch); + s->vga.screen_dump(&s->vga, filename, cswitch, mon); return; } diff --git a/monitor.c b/monitor.c index cbdfbad..cdae23f 100644 --- a/monitor.c +++ b/monitor.c @@ -895,7 +895,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data) { - vga_hw_screen_dump(qdict_get_str(qdict, "filename")); + vga_hw_screen_dump(qdict_get_str(qdict, "filename"), mon); return 0; } -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH 4/4] qxl-render: call ppm_save on bh 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy ` (2 preceding siblings ...) 2012-03-11 16:39 ` [Qemu-devel] [PATCH 3/4] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy @ 2012-03-11 16:39 ` Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 19:33 ` [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy 5 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 16:39 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori Uses the passed Monitor* to suspend and resume the monitor. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++---- hw/qxl.c | 5 +-- hw/qxl.h | 2 +- trace-events | 2 + ui/spice-display.h | 3 ++ 5 files changed, 83 insertions(+), 11 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 74e7ea3..16340d0 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -19,6 +19,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "console.h" #include "qxl.h" static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect) @@ -142,12 +143,74 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) } /* + * struct used just for ppm save bh. We don't actually support multiple qxl + * screendump yet, but a) we will, and b) exporting qxl0 from qxl.c looks + * uglier imo. + */ +typedef struct QXLPPMSaveBHData { + PCIQXLDevice *qxl; + QXLCookie *cookie; +} QXLPPMSaveBHData; + +static void qxl_render_ppm_save_bh(void *opaque); + +static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename, + Monitor *mon) +{ + QXLPPMSaveBHData *ppm_save_bh_data; + QEMUBH *ppm_save_bh; + QXLCookie *cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, + 0); + + qxl_set_rect_to_surface(qxl, &cookie->u.render.area); + if (filename) { + ppm_save_bh_data = g_malloc0(sizeof(*ppm_save_bh_data)); + ppm_save_bh_data->qxl = qxl; + ppm_save_bh_data->cookie = cookie; + ppm_save_bh = qemu_bh_new(qxl_render_ppm_save_bh, ppm_save_bh_data); + cookie->u.render.filename = g_strdup(filename); + cookie->u.render.ppm_save_bh = ppm_save_bh; + cookie->u.render.mon = mon; + monitor_suspend(mon); + } + return cookie; +} + +static void qxl_cookie_render_free(PCIQXLDevice *qxl, QXLCookie *cookie) +{ + g_free(cookie->u.render.filename); + if (cookie->u.render.mon) { + monitor_resume(cookie->u.render.mon); + } + g_free(cookie); + --qxl->render_update_cookie_num; +} + +static void qxl_render_ppm_save_bh(void *opaque) +{ + QXLPPMSaveBHData *data = opaque; + PCIQXLDevice *qxl = data->qxl; + QXLCookie *cookie = data->cookie; + QEMUBH *bh = cookie->u.render.ppm_save_bh; + + qemu_mutex_lock(&qxl->ssd.lock); + trace_qxl_render_ppm_save_bh( + qxl->ssd.ds->surface->data, qxl->guest_primary.data); + qxl_render_update_area_unlocked(qxl); + ppm_save(cookie->u.render.filename, qxl->ssd.ds->surface); + qxl_cookie_render_free(qxl, cookie); + qemu_mutex_unlock(&qxl->ssd.lock); + g_free(data); + qemu_bh_delete(bh); +} + +/* * use ssd.lock to protect render_update_cookie_num. * qxl_render_update is called by io thread or vcpu thread, and the completion * callbacks are called by spice_server thread, defering to bh called from the * io thread. */ -void qxl_render_update(PCIQXLDevice *qxl) +void qxl_render_update(PCIQXLDevice *qxl, const char *filename, Monitor *mon) { QXLCookie *cookie; @@ -155,6 +218,10 @@ void qxl_render_update(PCIQXLDevice *qxl) if (!runstate_is_running() || !qxl->guest_primary.commands) { qxl_render_update_area_unlocked(qxl); + if (filename) { + trace_qxl_render_update_screendump_no_update(); + ppm_save(filename, qxl->ssd.ds->surface); + } qemu_mutex_unlock(&qxl->ssd.lock); return; } @@ -162,9 +229,7 @@ void qxl_render_update(PCIQXLDevice *qxl) qxl->guest_primary.commands = 0; qxl->render_update_cookie_num++; qemu_mutex_unlock(&qxl->ssd.lock); - cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, - 0); - qxl_set_rect_to_surface(qxl, &cookie->u.render.area); + cookie = qxl_cookie_render_new(qxl, filename, mon); qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL, 0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie); } @@ -182,10 +247,13 @@ void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) { qemu_mutex_lock(&qxl->ssd.lock); trace_qxl_render_update_area_done(cookie); - qemu_bh_schedule(qxl->update_area_bh); - qxl->render_update_cookie_num--; + if (cookie->u.render.filename) { + qemu_bh_schedule(cookie->u.render.ppm_save_bh); + } else { + qemu_bh_schedule(qxl->update_area_bh); + qxl_cookie_render_free(qxl, cookie); + } qemu_mutex_unlock(&qxl->ssd.lock); - g_free(cookie); } static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor) diff --git a/hw/qxl.c b/hw/qxl.c index cabea3b..fae5be8 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1471,7 +1471,7 @@ static void qxl_hw_update(void *opaque) break; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); + qxl_render_update(qxl, NULL, NULL); break; default: break; @@ -1495,8 +1495,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch, switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); - ppm_save(filename, qxl->ssd.ds->surface); + qxl_render_update(qxl, filename, mon); break; case QXL_MODE_VGA: vga->screen_dump(vga, filename, cswitch, mon); diff --git a/hw/qxl.h b/hw/qxl.h index 11a0db3..219e149 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -147,7 +147,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext); /* qxl-render.c */ void qxl_render_resize(PCIQXLDevice *qxl); -void qxl_render_update(PCIQXLDevice *qxl); +void qxl_render_update(PCIQXLDevice *qxl, const char *filename, Monitor *mon); void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext); void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie); void qxl_render_update_area_bh(void *opaque); diff --git a/trace-events b/trace-events index a66aee8..2f045c4 100644 --- a/trace-events +++ b/trace-events @@ -718,3 +718,5 @@ qxl_blit_guest_primary_initialized(void) "" qxl_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]" qxl_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d" qxl_render_update_area_done(void *cookie) "%p" +qxl_render_update_screendump_no_update(void) "" +qxl_render_ppm_save_bh(void *data, void *primary) "%p (primary %p)" diff --git a/ui/spice-display.h b/ui/spice-display.h index 12e50b6..2d01f51 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -62,6 +62,9 @@ typedef struct QXLCookie { struct { QXLRect area; int redraw; + char *filename; + QEMUBH *ppm_save_bh; + Monitor *mon; } render; } u; } QXLCookie; -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy ` (3 preceding siblings ...) 2012-03-11 16:39 ` [Qemu-devel] [PATCH 4/4] qxl-render: call ppm_save on bh Alon Levy @ 2012-03-11 19:26 ` Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events Alon Levy ` (4 more replies) 2012-03-11 19:33 ` [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy 5 siblings, 5 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori v2 changes: rearranged and split the last patch: the console change to add Monitor to vga_hw_screen_dump can be moved past the qxl change, making it easier to see the changes, and adding an intermediate point where ppm_save happens after the update, but the do_screen_dump returns before ppm_save. The title of the patchset is wrong, only hmp is fixed, but qmp is now broken in a different way: there is no file saved when it expects it. The solution to this can be a change to libvirt to use hmp command for screendump when a qxl device is used, until the QAPI async monitor commands change lands. This patchset starts and ends with trace event additions that make it easier to see the change. It applies on top of http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01784.html due to trace-events. The problem addressed by this patchset is that after recent fixes (81fb6f) screendump with a qxl device in native mode saves a stale screen dump. The solution is to use monitor_suspend and monitor_release in qxl's implementation. This is done by: 1. introducing an extra parameter to vga_hw_screen_dump & hw_vga_dump console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2. using it in qxl via a bh. qxl-render: call ppm_save on bh Additional patches add trace events to qxl and qxl_render, making it easy to see the difference: events setup: (using stderr backend) (qemu) trace-event ppm_save on (qemu) trace-event qxl* on (qemu) trace-event qxl_interface_get_command_enter off (qemu) trace-event qxl_interface_release_resource off (qemu) trace-event qxl_interface_get_command_ret off before: ppm_save done before update: (qemu) screendump /tmp/a.ppm ppm_save /tmp/a.ppm surface=0x7fc0267b3ad0 qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1 qxl_interface_update_area_complete_schedule_bh #dirty=1 qxl_render_update_area_done 0x7fc02b603db0 (qemu) qxl_blit stride=-2560 [152, 160, 464, 480] after: (qemu) screendump /tmp/a.ppm qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1 qxl_interface_update_area_complete_schedule_bh #dirty=1 qxl_render_update_area_done 0x7f407af72210 qxl_render_ppm_save_bh 0x7f407f845b60 (primary 0x7f401bc00000) qxl_blit stride=-2560 [152, 160, 464, 480] ppm_save /tmp/a.ppm surface=0x7f4077204ad0 (qemu) Note: This doesn't address a possible libvirt problem that was mentioned in length before, but since it has not been reproduced it will be fixed when it is. Meanwhile other users like autotest will be fixed by this patch (by fix I mean screendump will produce the correct output). Alon Levy (5): qxl: switch qxl.c to trace-events qxl/qxl_render.c: add trace events qxl-render: call ppm_save on bh console: pass Monitor to vga_hw_screen_dump/hw_vga_dump qxl: screendump: use provided Monitor console.c | 4 +- console.h | 5 +- hw/blizzard.c | 2 +- hw/g364fb.c | 3 +- hw/omap_dss.c | 4 +- hw/omap_lcdc.c | 3 +- hw/qxl-render.c | 95 +++++++++++++++++++++++++++------ hw/qxl.c | 150 ++++++++++++++++++++++++--------------------------- hw/qxl.h | 2 +- hw/sm501.c | 4 +- hw/tcx.c | 12 +++-- hw/vga.c | 6 ++- hw/vmware_vga.c | 5 +- monitor.c | 2 +- trace-events | 55 +++++++++++++++++++ ui/spice-display.h | 3 + 16 files changed, 240 insertions(+), 115 deletions(-) -- 1.7.9.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy @ 2012-03-11 19:26 ` Alon Levy 2012-03-12 10:20 ` Gerd Hoffmann 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 2/5] qxl/qxl_render.c: add trace events Alon Levy ` (3 subsequent siblings) 4 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori dprint is still used for qxl_init_common one time prints. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl.c | 140 +++++++++++++++++++++++++++------------------------------ trace-events | 47 +++++++++++++++++++ 2 files changed, 113 insertions(+), 74 deletions(-) diff --git a/hw/qxl.c b/hw/qxl.c index e17b0e3..7857731 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -23,6 +23,7 @@ #include "qemu-queue.h" #include "monitor.h" #include "sysemu.h" +#include "trace.h" #include "qxl.h" @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s:\n", __FUNCTION__); + trace_qxl_interface_attach_worker(); qxl->ssd.worker = qxl_worker; } @@ -417,7 +418,7 @@ static void interface_set_compression_level(QXLInstance *sin, int level) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s: %d\n", __FUNCTION__, level); + trace_qxl_interface_set_compression_level(level); qxl->shadow_rom.compression_level = cpu_to_le32(level); qxl->rom->compression_level = cpu_to_le32(level); qxl_rom_set_dirty(qxl); @@ -436,7 +437,7 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); - dprint(qxl, 1, "%s:\n", __FUNCTION__); + trace_qxl_interface_get_init_info(); info->memslot_gen_bits = MEMSLOT_GENERATION_BITS; info->memslot_id_bits = MEMSLOT_SLOT_BITS; info->num_memslots = NUM_MEMSLOTS; @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) QXLCommand *cmd; int notify, ret; + trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode)); + switch (qxl->mode) { case QXL_MODE_VGA: - dprint(qxl, 2, "%s: vga\n", __FUNCTION__); ret = false; qemu_mutex_lock(&qxl->ssd.lock); if (qxl->ssd.update != NULL) { @@ -518,19 +520,18 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) } qemu_mutex_unlock(&qxl->ssd.lock); if (ret) { - dprint(qxl, 2, "%s %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); + trace_qxl_interface_get_command_ret(qxl_mode_to_string(qxl->mode)); qxl_log_command(qxl, "vga", ext); } return ret; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: case QXL_MODE_UNDEFINED: - dprint(qxl, 4, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); ring = &qxl->ram->cmd_ring; if (SPICE_RING_IS_EMPTY(ring)) { return false; } - dprint(qxl, 2, "%s: %s\n", __FUNCTION__, qxl_mode_to_string(qxl->mode)); + trace_qxl_interface_get_command_ret(qxl_mode_to_string(qxl->mode)); SPICE_RING_CONS_ITEM(ring, cmd); ext->cmd = *cmd; ext->group_id = MEMSLOT_GROUP_GUEST; @@ -592,7 +593,7 @@ static inline void qxl_push_free_res(PCIQXLDevice *d, int flush) } SPICE_RING_PUSH(ring, notify); - dprint(d, 2, "free: push %d items, notify %s, ring %d/%d [%d,%d]\n", + trace_qxl_push_free_res( d->num_free_res, notify ? "yes" : "no", ring->prod - ring->cons, ring->num_items, ring->prod, ring->cons); @@ -642,7 +643,7 @@ static void interface_release_resource(QXLInstance *sin, } qxl->last_release = ext.info; qxl->num_free_res++; - dprint(qxl, 3, "%4d\r", qxl->num_free_res); + trace_qxl_interface_release_resource(qxl->num_free_res); qxl_push_free_res(qxl, 0); } @@ -716,7 +717,7 @@ static int interface_flush_resources(QXLInstance *sin) PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); int ret; - dprint(qxl, 1, "free: guest flush (have %d)\n", qxl->num_free_res); + trace_qxl_interface_flush_resources(qxl->num_free_res); ret = qxl->num_free_res; if (ret) { qxl_push_free_res(qxl, 1); @@ -736,7 +737,7 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie) qxl->current_async = QXL_UNDEFINED_IO; qemu_mutex_unlock(&qxl->async_lock); - dprint(qxl, 2, "async_complete: %d (%p) done\n", current_async, cookie); + trace_qxl_interface_async_complete_io(current_async, cookie); if (!cookie) { fprintf(stderr, "qxl: %s: error, cookie is NULL\n", __func__); return; @@ -782,11 +783,13 @@ static void interface_update_area_complete(QXLInstance *sin, qemu_mutex_unlock(&qxl->ssd.lock); return; } + trace_qxl_interface_update_area_complete(surface_id, dirty->left, + dirty->right, dirty->top, dirty->bottom, num_updated_rects); if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) { /* * overflow - treat this as a full update. Not expected to be common. */ - dprint(qxl, 1, "%s: overflow of dirty rects\n", __func__); + trace_qxl_interface_update_area_complete_overflow(QXL_NUM_DIRTY_RECTS); qxl->guest_primary.resized = 1; } if (qxl->guest_primary.resized) { @@ -802,8 +805,7 @@ static void interface_update_area_complete(QXLInstance *sin, qxl->dirty[qxl_i++] = dirty[i]; } qxl->num_dirty_rects += num_updated_rects; - dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", - __func__, qxl->num_dirty_rects); + trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects); qemu_bh_schedule(qxl->update_area_bh); qemu_mutex_unlock(&qxl->ssd.lock); } @@ -857,7 +859,7 @@ static void qxl_enter_vga_mode(PCIQXLDevice *d) if (d->mode == QXL_MODE_VGA) { return; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_enter_vga_mode(); qemu_spice_create_host_primary(&d->ssd); d->mode = QXL_MODE_VGA; memset(&d->ssd.dirty, 0, sizeof(d->ssd.dirty)); @@ -868,7 +870,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d) if (d->mode != QXL_MODE_VGA) { return; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_exit_vga_mode(); qxl_destroy_primary(d, QXL_SYNC); } @@ -905,7 +907,7 @@ static void qxl_reset_state(PCIQXLDevice *d) static void qxl_soft_reset(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_soft_reset(); qxl_check_state(d); if (d->id == 0) { @@ -917,8 +919,7 @@ static void qxl_soft_reset(PCIQXLDevice *d) static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) { - dprint(d, 1, "%s: start%s\n", __FUNCTION__, - loadvm ? " (loadvm)" : ""); + trace_qxl_hard_reset_enter(loadvm); qxl_spice_reset_cursor(d); qxl_spice_reset_image_cache(d); @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) qemu_spice_create_host_memslot(&d->ssd); qxl_soft_reset(d); - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_hard_reset_exit(); } static void qxl_reset_handler(DeviceState *dev) @@ -949,7 +950,7 @@ static void qxl_vga_ioport_write(void *opaque, uint32_t addr, uint32_t val) PCIQXLDevice *qxl = container_of(vga, PCIQXLDevice, vga); if (qxl->mode != QXL_MODE_VGA) { - dprint(qxl, 1, "%s\n", __FUNCTION__); + trace_qxl_vga_ioport_while_not_in_vga_mode(); qxl_destroy_primary(qxl, QXL_SYNC); qxl_soft_reset(qxl); } @@ -990,9 +991,7 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, guest_start = le64_to_cpu(d->guest_slots[slot_id].slot.mem_start); guest_end = le64_to_cpu(d->guest_slots[slot_id].slot.mem_end); - dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 "\n", - __FUNCTION__, slot_id, - guest_start, guest_end); + trace_qxl_add_memslot_guest(slot_id, guest_start, guest_end); PANIC_ON(slot_id >= NUM_MEMSLOTS); PANIC_ON(guest_start > guest_end); @@ -1039,9 +1038,8 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, memslot.generation = d->rom->slot_generation = 0; qxl_rom_set_dirty(d); - dprint(d, 1, "%s: slot %d: host virt 0x%lx - 0x%lx\n", - __FUNCTION__, memslot.slot_id, - memslot.virt_start, memslot.virt_end); + trace_qxl_add_memslot_host(memslot.slot_id, memslot.virt_start, + memslot.virt_end); qemu_spice_add_memslot(&d->ssd, &memslot, async); d->guest_slots[slot_id].ptr = (void*)memslot.virt_start; @@ -1052,21 +1050,21 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta, static void qxl_del_memslot(PCIQXLDevice *d, uint32_t slot_id) { - dprint(d, 1, "%s: slot %d\n", __FUNCTION__, slot_id); + trace_qxl_del_memslot(slot_id); qemu_spice_del_memslot(&d->ssd, MEMSLOT_GROUP_HOST, slot_id); d->guest_slots[slot_id].active = 0; } static void qxl_reset_memslots(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_reset_memslots(); qxl_spice_reset_memslots(d); memset(&d->guest_slots, 0, sizeof(d->guest_slots)); } static void qxl_reset_surfaces(PCIQXLDevice *d) { - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_reset_surfaces(); d->mode = QXL_MODE_UNDEFINED; qxl_spice_destroy_surfaces(d, QXL_SYNC); } @@ -1108,9 +1106,6 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm, assert(qxl->mode != QXL_MODE_NATIVE); qxl_exit_vga_mode(qxl); - dprint(qxl, 1, "%s: %dx%d\n", __FUNCTION__, - le32_to_cpu(sc->width), le32_to_cpu(sc->height)); - surface.format = le32_to_cpu(sc->format); surface.height = le32_to_cpu(sc->height); surface.mem = le64_to_cpu(sc->mem); @@ -1119,6 +1114,9 @@ static void qxl_create_guest_primary(PCIQXLDevice *qxl, int loadvm, surface.width = le32_to_cpu(sc->width); surface.type = le32_to_cpu(sc->type); surface.flags = le32_to_cpu(sc->flags); + trace_qxl_create_guest_primary(sc->width, sc->height, sc->mem, sc->format, + sc->position, sc->stride, sc->type, + sc->flags); surface.mouse_mode = true; surface.group_id = MEMSLOT_GROUP_GUEST; @@ -1142,7 +1140,7 @@ static int qxl_destroy_primary(PCIQXLDevice *d, qxl_async_io async) if (d->mode == QXL_MODE_UNDEFINED) { return 0; } - dprint(d, 1, "%s\n", __FUNCTION__); + trace_qxl_destroy_primary(); d->mode = QXL_MODE_UNDEFINED; qemu_spice_destroy_primary_surface(&d->ssd, 0, async); qxl_spice_reset_cursor(d); @@ -1169,8 +1167,7 @@ static void qxl_set_mode(PCIQXLDevice *d, int modenr, int loadvm) .mem = devmem + d->shadow_rom.draw_area_offset, }; - dprint(d, 1, "%s: mode %d [ %d x %d @ %d bpp devmem 0x%" PRIx64 " ]\n", - __func__, modenr, mode->x_res, mode->y_res, mode->bits, devmem); + trace_qxl_set_mode(modenr, mode->x_res, mode->y_res, mode->bits, devmem); if (!loadvm) { qxl_hard_reset(d, 0); } @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr, if (d->mode != QXL_MODE_VGA) { break; } - dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n", - __func__, io_port, io_port_to_string(io_port)); + trace_qxl_io_unexpected_vga_mode( + io_port, io_port_to_string(io_port)); /* be nice to buggy guest drivers */ if (io_port >= QXL_IO_UPDATE_AREA_ASYNC && io_port <= QXL_IO_DESTROY_ALL_SURFACES_ASYNC) { @@ -1259,7 +1256,7 @@ async_common: } d->current_async = orig_io_port; qemu_mutex_unlock(&d->async_lock); - dprint(d, 2, "start async %d (%"PRId64")\n", io_port, val); + trace_qxl_io_start_async(io_port, val); break; default: break; @@ -1299,7 +1296,7 @@ async_common: d->oom_running = 0; break; case QXL_IO_SET_MODE: - dprint(d, 1, "QXL_SET_MODE %d\n", (int)val); + trace_qxl_io_set_mode(val); qxl_set_mode(d, val, 0); break; case QXL_IO_LOG: @@ -1309,7 +1306,7 @@ async_common: } break; case QXL_IO_RESET: - dprint(d, 1, "QXL_IO_RESET\n"); + trace_qxl_io_reset(); qxl_hard_reset(d, 0); break; case QXL_IO_MEMSLOT_ADD: @@ -1337,7 +1334,7 @@ async_common: async); goto cancel_async; } - dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async); + trace_qxl_io_create_primary(async); d->guest_primary.surface = d->ram->create_surface; qxl_create_guest_primary(d, 0, async); break; @@ -1347,11 +1344,11 @@ async_common: async); goto cancel_async; } - dprint(d, 1, "QXL_IO_DESTROY_PRIMARY (async=%d) (%s)\n", async, - qxl_mode_to_string(d->mode)); + trace_qxl_io_destroy_primary(async, + qxl_mode_to_string(d->mode)); if (!qxl_destroy_primary(d, async)) { - dprint(d, 1, "QXL_IO_DESTROY_PRIMARY_ASYNC in %s, ignored\n", - qxl_mode_to_string(d->mode)); + trace_qxl_io_destroy_primary_ignored( + qxl_mode_to_string(d->mode)); goto cancel_async; } break; @@ -1371,14 +1368,12 @@ async_common: ring->prod, ring->cons); } qxl_push_free_res(d, 1 /* flush */); - dprint(d, 1, "QXL_IO_FLUSH_RELEASE exit (%s, s#=%d, res#=%d,%p)\n", - qxl_mode_to_string(d->mode), d->guest_surfaces.count, - d->num_free_res, d->last_release); + trace_qxl_io_flush_release(qxl_mode_to_string(d->mode), + d->guest_surfaces.count, d->num_free_res, d->last_release); break; } case QXL_IO_FLUSH_SURFACES_ASYNC: - dprint(d, 1, "QXL_IO_FLUSH_SURFACES_ASYNC" - " (%"PRId64") (%s, s#=%d, res#=%d)\n", + trace_qxl_io_flush_surfaces_async( val, qxl_mode_to_string(d->mode), d->guest_surfaces.count, d->num_free_res); qxl_spice_flush_surfaces_async(d); @@ -1404,9 +1399,7 @@ cancel_async: static uint64_t ioport_read(void *opaque, target_phys_addr_t addr, unsigned size) { - PCIQXLDevice *d = opaque; - - dprint(d, 1, "%s: unexpected\n", __FUNCTION__); + trace_qxl_io_read_unexpected(); return 0xff; } @@ -1445,23 +1438,24 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events) qxl_update_irq(d); } else { if (write(d->pipe[1], d, 1) != 1) { - dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__); + trace_qxl_send_events_write_to_pipe_failed(); } } } static void init_pipe_signaling(PCIQXLDevice *d) { - if (pipe(d->pipe) < 0) { - dprint(d, 1, "%s: pipe creation failed\n", __FUNCTION__); - return; - } - fcntl(d->pipe[0], F_SETFL, O_NONBLOCK); - fcntl(d->pipe[1], F_SETFL, O_NONBLOCK); - fcntl(d->pipe[0], F_SETOWN, getpid()); + if (pipe(d->pipe) < 0) { + fprintf(stderr, "%s:%s: qxl pipe creation failed\n", + __FILE__, __func__); + exit(1); + } + fcntl(d->pipe[0], F_SETFL, O_NONBLOCK); + fcntl(d->pipe[1], F_SETFL, O_NONBLOCK); + fcntl(d->pipe[0], F_SETOWN, getpid()); - qemu_thread_get_self(&d->main); - qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d); + qemu_thread_get_self(&d->main); + qemu_set_fd_handler(d->pipe[0], pipe_read, NULL, d); } /* graphics console */ @@ -1556,8 +1550,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl) surface_offset -= vram_start; surface_size = cmd->u.surface_create.height * abs(cmd->u.surface_create.stride); - dprint(qxl, 3, "%s: dirty surface %d, offset %d, size %d\n", __func__, - i, (int)surface_offset, surface_size); + trace_qxl_dirty_surfaces(i, (int)surface_offset, surface_size); qxl_set_dirty(&qxl->vram_bar, surface_offset, surface_size); } } @@ -1791,7 +1784,7 @@ static void qxl_pre_save(void *opaque) PCIQXLDevice* d = opaque; uint8_t *ram_start = d->vga.vram_ptr; - dprint(d, 1, "%s:\n", __FUNCTION__); + trace_qxl_pre_save(); if (d->last_release == NULL) { d->last_release_offset = 0; } else { @@ -1804,10 +1797,10 @@ static int qxl_pre_load(void *opaque) { PCIQXLDevice* d = opaque; - dprint(d, 1, "%s: start\n", __FUNCTION__); + trace_qxl_pre_load_enter(); qxl_hard_reset(d, 1); qxl_exit_vga_mode(d); - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_pre_load_exit(); return 0; } @@ -1819,7 +1812,7 @@ static void qxl_create_memslots(PCIQXLDevice *d) if (!d->guest_slots[i].active) { continue; } - dprint(d, 1, "%s: restoring guest slot %d\n", __func__, i); + trace_qxl_create_memslots(i); qxl_add_memslot(d, i, 0, QXL_SYNC); } } @@ -1831,7 +1824,7 @@ static int qxl_post_load(void *opaque, int version) QXLCommandExt *cmds; int in, out, newmode; - dprint(d, 1, "%s: start\n", __FUNCTION__); + trace_qxl_post_load_enter(); assert(d->last_release_offset < d->vga.vram_size); if (d->last_release_offset == 0) { @@ -1842,8 +1835,7 @@ static int qxl_post_load(void *opaque, int version) d->modes = (QXLModes*)((uint8_t*)d->rom + d->rom->modes_offset); - dprint(d, 1, "%s: restore mode (%s)\n", __FUNCTION__, - qxl_mode_to_string(d->mode)); + trace_qxl_post_load_restore_mode(qxl_mode_to_string(d->mode)); newmode = d->mode; d->mode = QXL_MODE_UNDEFINED; @@ -1885,7 +1877,7 @@ static int qxl_post_load(void *opaque, int version) qxl_set_mode(d, d->shadow_rom.mode, 1); break; } - dprint(d, 1, "%s: done\n", __FUNCTION__); + trace_qxl_post_load_exit(); return 0; } diff --git a/trace-events b/trace-events index dfe28ed..0853a1b 100644 --- a/trace-events +++ b/trace-events @@ -665,3 +665,50 @@ displaysurface_resize(void *display_state, void *display_surface, int width, int # vga.c ppm_save(const char *filename, void *display_surface) "%s surface=%p" + +# hw/qxl.c +qxl_io_create_primary(bool async) "async=%d" +qxl_create_guest_primary(uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position, int32_t stride, uint32_t type, uint32_t flags) "%dx%d mem=%lx %d,%d,%d,%d,%d" +qxl_interface_attach_worker(void) "" +qxl_interface_set_compression_level(int64_t level) "%"PRId64 +qxl_interface_get_init_info(void) "" +qxl_enter_vga_mode(void) "" +qxl_exit_vga_mode(void) "" +qxl_soft_reset(void) "" +qxl_hard_reset_enter(int64_t loadvm) "loadvm=%"PRId64"" +qxl_hard_reset_exit(void) "" +qxl_vga_ioport_while_not_in_vga_mode(void) "(reset to VGA mode because of VGA io)" +qxl_add_memslot_guest(uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%u: guest phys 0x%"PRIx64 " - 0x%" PRIx64 +qxl_add_memslot_host(uint32_t slot_id, unsigned long virt_start, unsigned long virt_end) "%u: host virt 0x%lx - 0x%lx" +qxl_del_memslot(uint32_t slot_id) "%u" +qxl_reset_memslots(void) "" +qxl_reset_surfaces(void) "" +qxl_destroy_primary(void) "" +qxl_set_mode(int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]" +qxl_io_unexpected_vga_mode(uint32_t io_port, const char *desc) "0x%x (%s)" +qxl_io_start_async(uint32_t io_port, int64_t val) "%u (%"PRId64")" +qxl_io_set_mode(uint64_t val) "%"PRIu64 +qxl_io_reset(void) "" +qxl_io_destroy_primary(int async, const char *mode) "%d (%s)" +qxl_io_destroy_primary_ignored(const char *mode) "%s" +qxl_io_flush_release(const char *mode, uint32_t surface_count, uint32_t num_free_res, void *last_release) "flush complete %s, s#=%d, res#=%d, %p" +qxl_io_flush_surfaces_async(uint64_t val, const char *mode, uint32_t surface_count, uint32_t num_free_res) "val=%"PRIu64", %s, s#=%d, res#=%d" +qxl_io_read_unexpected(void) "" +qxl_send_events_write_to_pipe_failed(void) "" +qxl_interface_get_command_enter(const char *mode) "%s" +qxl_interface_get_command_ret(const char *mode) "%s" +qxl_push_free_res(uint32_t free_res, const char *notify, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d items, notify %s, ring %d/%d [%d,%d]" +qxl_interface_release_resource(uint32_t free_res) "%4d" +qxl_interface_flush_resources(uint32_t free_res) "%d" +qxl_interface_async_complete_io(uint32_t current_async, void *cookie) "%d (%p) done" +qxl_interface_update_area_complete_overflow(int max) "(%d)" +qxl_interface_update_area_complete_schedule_bh(uint32_t num_dirty) "#dirty=%d" +qxl_dirty_surfaces(int surface, int offset, int size) "surface=%d offset=%d size=%d" +qxl_pre_save(void) "" +qxl_pre_load_enter(void) "" +qxl_pre_load_exit(void) "" +qxl_create_memslots(int slot) "%d" +qxl_post_load_enter(void) "" +qxl_post_load_restore_mode(const char *mode) "%s" +qxl_post_load_exit(void) "" +qxl_interface_update_area_complete(uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom, uint32_t num_updated_rects) "surface=%d [%d,%d,%d,%d] #=%d" -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events Alon Levy @ 2012-03-12 10:20 ` Gerd Hoffmann 2012-03-12 11:43 ` Alon Levy 0 siblings, 1 reply; 35+ messages in thread From: Gerd Hoffmann @ 2012-03-12 10:20 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, qemu-devel, lcapitulino On 03/11/12 20:26, Alon Levy wrote: > dprint is still used for qxl_init_common one time prints. I think we shouldn't simply convert the dprintf's into trace-points. We should look at each dprintf and check whenever it makes sense at all, whenever it makes sense at that place before converting it over to a tracepoint. > @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker) > { > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); > > - dprint(qxl, 1, "%s:\n", __FUNCTION__); > + trace_qxl_interface_attach_worker(); > qxl->ssd.worker = qxl_worker; > } For example: Do we really need that one? > @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > QXLCommand *cmd; > int notify, ret; > > + trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode)); > + Why this? > - dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", > - __func__, qxl->num_dirty_rects); > + trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects); I think it makes more sense to have the tracepoint in the bottom half handler instead. > static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > { > - dprint(d, 1, "%s: start%s\n", __FUNCTION__, > - loadvm ? " (loadvm)" : ""); > + trace_qxl_hard_reset_enter(loadvm); > > qxl_spice_reset_cursor(d); > qxl_spice_reset_image_cache(d); > @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > qemu_spice_create_host_memslot(&d->ssd); > qxl_soft_reset(d); > > - dprint(d, 1, "%s: done\n", __FUNCTION__); > + trace_qxl_hard_reset_exit(); > } Do we need the exit tracepoint? > static void qxl_reset_memslots(PCIQXLDevice *d) > { > - dprint(d, 1, "%s:\n", __FUNCTION__); > + trace_qxl_reset_memslots(); > qxl_spice_reset_memslots(d); > memset(&d->guest_slots, 0, sizeof(d->guest_slots)); > } Do we need that one? qxl_hard_reset is the only caller of that function ... > @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr, > if (d->mode != QXL_MODE_VGA) { > break; > } > - dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n", > - __func__, io_port, io_port_to_string(io_port)); > + trace_qxl_io_unexpected_vga_mode( > + io_port, io_port_to_string(io_port)); We might want raise an error irq here, and have a tracepoint in qxl_guest_bug() of course ... > case QXL_IO_SET_MODE: > - dprint(d, 1, "QXL_SET_MODE %d\n", (int)val); > + trace_qxl_io_set_mode(val); > qxl_set_mode(d, val, 0); Needed? There is a tracepoint in qxl_set_mode() ... > case QXL_IO_RESET: > - dprint(d, 1, "QXL_IO_RESET\n"); > + trace_qxl_io_reset(); > qxl_hard_reset(d, 0); ... likewise ... > break; > case QXL_IO_MEMSLOT_ADD: > @@ -1337,7 +1334,7 @@ async_common: > async); > goto cancel_async; > } > - dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async); > + trace_qxl_io_create_primary(async); > d->guest_primary.surface = d->ram->create_surface; > qxl_create_guest_primary(d, 0, async); ... here too ... We might want to have a "trace_qxl_io_write(addr, val)" at the start of the function, so we see all guest writes. Traces for the actual ops (if needed at all) are probably much better placed into the functions executing the op as they can trace more details (i.e. qxl_set_mode has the tracepoint *after* looking up the mode so we can stick the mode info into the trace too). cheers, Gerd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-12 10:20 ` Gerd Hoffmann @ 2012-03-12 11:43 ` Alon Levy 2012-03-12 15:50 ` Alon Levy 0 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-12 11:43 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, lcapitulino On Mon, Mar 12, 2012 at 11:20:55AM +0100, Gerd Hoffmann wrote: > On 03/11/12 20:26, Alon Levy wrote: > > dprint is still used for qxl_init_common one time prints. > > I think we shouldn't simply convert the dprintf's into trace-points. > > We should look at each dprintf and check whenever it makes sense at all, > whenever it makes sense at that place before converting it over to a > tracepoint. I had two changes of heart about this. Initially I started mechanically converting, then I realized it only makes sense for recurring events, and things we want to see come out of the same output. But then I noticed a lot of existing users do use it for as verbose usage as we do (bh calls) and it is not meant as a stable interface to anyone - clearly something that should fit the developer and user, and if the subsystem changes then the events can change. Bottom line I think having most of the dprints as trace_events makes sense, and we can use consistent naming to make enabling/disabling them easy for systemtap/stderr (with monitor trace-events command) easy. I only left very few dprints. > > > @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker) > > { > > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); > > > > - dprint(qxl, 1, "%s:\n", __FUNCTION__); > > + trace_qxl_interface_attach_worker(); > > qxl->ssd.worker = qxl_worker; > > } > > For example: Do we really need that one? I look at it the other way around - can it repeat? yes, it's a callback from the spice server which we don't control. does it serve the same purpose as the dprint? yes. > > > @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > > QXLCommand *cmd; > > int notify, ret; > > > > + trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode)); > > + > > Why this? Simplyfication of the dprints to avoid introducing an additional trace event. We had a dprint for level 1 for both VGA and other modes, so I moved it up. This trace is for each request from the server, as opposed to the _ret that is for each returned command (much less frequent). > > > - dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", > > - __func__, qxl->num_dirty_rects); > > + trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects); > > I think it makes more sense to have the tracepoint in the bottom half > handler instead. Why instead? I could add another one at the bottom half. > > > static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > > { > > - dprint(d, 1, "%s: start%s\n", __FUNCTION__, > > - loadvm ? " (loadvm)" : ""); > > + trace_qxl_hard_reset_enter(loadvm); > > > > qxl_spice_reset_cursor(d); > > qxl_spice_reset_image_cache(d); > > @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > > qemu_spice_create_host_memslot(&d->ssd); > > qxl_soft_reset(d); > > > > - dprint(d, 1, "%s: done\n", __FUNCTION__); > > + trace_qxl_hard_reset_exit(); > > } > > Do we need the exit tracepoint? With systemtap I'd say the whole function could be traced, and that would mean both entry and exit. But you can't do that with the tracing framework, so this is the only way to have both. If having both dprints makes no sense, I guess having both trace events makes none too. > > > static void qxl_reset_memslots(PCIQXLDevice *d) > > { > > - dprint(d, 1, "%s:\n", __FUNCTION__); > > + trace_qxl_reset_memslots(); > > qxl_spice_reset_memslots(d); > > memset(&d->guest_slots, 0, sizeof(d->guest_slots)); > > } > > Do we need that one? qxl_hard_reset is the only caller of that function ... Agree, I'll drop it. But maybe I should add trace events for all the qxl_spice_* calls? > > > @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr, > > if (d->mode != QXL_MODE_VGA) { > > break; > > } > > - dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n", > > - __func__, io_port, io_port_to_string(io_port)); > > + trace_qxl_io_unexpected_vga_mode( > > + io_port, io_port_to_string(io_port)); > > We might want raise an error irq here, and have a tracepoint in > qxl_guest_bug() of course ... ok, I think I can add the tracepoint for qxl_guest_bug. Raise an error irq I'll do in another patch. > > > case QXL_IO_SET_MODE: > > - dprint(d, 1, "QXL_SET_MODE %d\n", (int)val); > > + trace_qxl_io_set_mode(val); > > qxl_set_mode(d, val, 0); > > Needed? There is a tracepoint in qxl_set_mode() ... But if qxl_set_mode can be called from multiple places it isn't equivalent. > > > case QXL_IO_RESET: > > - dprint(d, 1, "QXL_IO_RESET\n"); > > + trace_qxl_io_reset(); > > qxl_hard_reset(d, 0); > > ... likewise ... Same answer. > > > break; > > case QXL_IO_MEMSLOT_ADD: > > @@ -1337,7 +1334,7 @@ async_common: > > async); > > goto cancel_async; > > } > > - dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async); > > + trace_qxl_io_create_primary(async); > > d->guest_primary.surface = d->ram->create_surface; > > qxl_create_guest_primary(d, 0, async); > > ... here too ... Ditto. The traceevents are named qxl_io_* on purpose, they are guest io triggered, there can be other calls to qxl_create_guest_primary. Perhaps I'll have a single .. oh, you wrote the same thing below :) > > We might want to have a "trace_qxl_io_write(addr, val)" at the start of > the function, so we see all guest writes. Traces for the actual ops (if > needed at all) are probably much better placed into the functions > executing the op as they can trace more details (i.e. qxl_set_mode has > the tracepoint *after* looking up the mode so we can stick the mode info > into the trace too). Ok, that works. > > cheers, > Gerd > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-12 11:43 ` Alon Levy @ 2012-03-12 15:50 ` Alon Levy 2012-03-13 6:42 ` Gerd Hoffmann 0 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-12 15:50 UTC (permalink / raw) To: Gerd Hoffmann, aliguori, qemu-devel, lcapitulino On Mon, Mar 12, 2012 at 01:43:11PM +0200, Alon Levy wrote: > On Mon, Mar 12, 2012 at 11:20:55AM +0100, Gerd Hoffmann wrote: > > On 03/11/12 20:26, Alon Levy wrote: > > > dprint is still used for qxl_init_common one time prints. > > > > I think we shouldn't simply convert the dprintf's into trace-points. > > > > We should look at each dprintf and check whenever it makes sense at all, > > whenever it makes sense at that place before converting it over to a > > tracepoint. I'll also add qxl_spice_* trace points for the next patch. Does that sound excessive? you could just trace the qxl_io_write to get the io itself, or trace just qxl_spice_* to get the qxl<->spice interface, or both (qxl_*). > > I had two changes of heart about this. Initially I started mechanically > converting, then I realized it only makes sense for recurring events, > and things we want to see come out of the same output. But then I > noticed a lot of existing users do use it for as verbose usage as we do > (bh calls) and it is not meant as a stable interface to anyone - clearly > something that should fit the developer and user, and if the subsystem > changes then the events can change. > > Bottom line I think having most of the dprints as trace_events makes > sense, and we can use consistent naming to make enabling/disabling them > easy for systemtap/stderr (with monitor trace-events command) easy. > > I only left very few dprints. > > > > > > @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, QXLWorker *qxl_worker) > > > { > > > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); > > > > > > - dprint(qxl, 1, "%s:\n", __FUNCTION__); > > > + trace_qxl_interface_attach_worker(); > > > qxl->ssd.worker = qxl_worker; > > > } > > > > For example: Do we really need that one? > > I look at it the other way around - can it repeat? yes, it's a callback > from the spice server which we don't control. does it serve the > same purpose as the dprint? yes. > > > > > > @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > > > QXLCommand *cmd; > > > int notify, ret; > > > > > > + trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode)); > > > + > > > > Why this? > > Simplyfication of the dprints to avoid introducing an additional trace > event. We had a dprint for level 1 for both VGA and other modes, so I > moved it up. This trace is for each request from the server, as opposed > to the _ret that is for each returned command (much less frequent). > > > > > > - dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", > > > - __func__, qxl->num_dirty_rects); > > > + trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects); > > > > I think it makes more sense to have the tracepoint in the bottom half > > handler instead. > > Why instead? I could add another one at the bottom half. > > > > > > static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > > > { > > > - dprint(d, 1, "%s: start%s\n", __FUNCTION__, > > > - loadvm ? " (loadvm)" : ""); > > > + trace_qxl_hard_reset_enter(loadvm); > > > > > > qxl_spice_reset_cursor(d); > > > qxl_spice_reset_image_cache(d); > > > @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm) > > > qemu_spice_create_host_memslot(&d->ssd); > > > qxl_soft_reset(d); > > > > > > - dprint(d, 1, "%s: done\n", __FUNCTION__); > > > + trace_qxl_hard_reset_exit(); > > > } > > > > Do we need the exit tracepoint? > > With systemtap I'd say the whole function could be traced, and that > would mean both entry and exit. But you can't do that with the tracing > framework, so this is the only way to have both. > > If having both dprints makes no sense, I guess having both trace events > makes none too. > > > > > > static void qxl_reset_memslots(PCIQXLDevice *d) > > > { > > > - dprint(d, 1, "%s:\n", __FUNCTION__); > > > + trace_qxl_reset_memslots(); > > > qxl_spice_reset_memslots(d); > > > memset(&d->guest_slots, 0, sizeof(d->guest_slots)); > > > } > > > > Do we need that one? qxl_hard_reset is the only caller of that function ... > > Agree, I'll drop it. > > But maybe I should add trace events for all the qxl_spice_* calls? > > > > > > @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, target_phys_addr_t addr, > > > if (d->mode != QXL_MODE_VGA) { > > > break; > > > } > > > - dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n", > > > - __func__, io_port, io_port_to_string(io_port)); > > > + trace_qxl_io_unexpected_vga_mode( > > > + io_port, io_port_to_string(io_port)); > > > > We might want raise an error irq here, and have a tracepoint in > > qxl_guest_bug() of course ... > > ok, I think I can add the tracepoint for qxl_guest_bug. Raise an error > irq I'll do in another patch. > > > > > > case QXL_IO_SET_MODE: > > > - dprint(d, 1, "QXL_SET_MODE %d\n", (int)val); > > > + trace_qxl_io_set_mode(val); > > > qxl_set_mode(d, val, 0); > > > > Needed? There is a tracepoint in qxl_set_mode() ... > > But if qxl_set_mode can be called from multiple places it isn't > equivalent. > > > > > > case QXL_IO_RESET: > > > - dprint(d, 1, "QXL_IO_RESET\n"); > > > + trace_qxl_io_reset(); > > > qxl_hard_reset(d, 0); > > > > ... likewise ... > > Same answer. > > > > > > break; > > > case QXL_IO_MEMSLOT_ADD: > > > @@ -1337,7 +1334,7 @@ async_common: > > > async); > > > goto cancel_async; > > > } > > > - dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async); > > > + trace_qxl_io_create_primary(async); > > > d->guest_primary.surface = d->ram->create_surface; > > > qxl_create_guest_primary(d, 0, async); > > > > ... here too ... > > Ditto. The traceevents are named qxl_io_* on purpose, they are guest io > triggered, there can be other calls to qxl_create_guest_primary. > > Perhaps I'll have a single .. oh, you wrote the same thing below :) > > > > > We might want to have a "trace_qxl_io_write(addr, val)" at the start of > > the function, so we see all guest writes. Traces for the actual ops (if > > needed at all) are probably much better placed into the functions > > executing the op as they can trace more details (i.e. qxl_set_mode has > > the tracepoint *after* looking up the mode so we can stick the mode info > > into the trace too). > > Ok, that works. > > > > > cheers, > > Gerd > > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-12 15:50 ` Alon Levy @ 2012-03-13 6:42 ` Gerd Hoffmann 2012-03-13 9:35 ` Alon Levy 0 siblings, 1 reply; 35+ messages in thread From: Gerd Hoffmann @ 2012-03-13 6:42 UTC (permalink / raw) To: aliguori, qemu-devel, lcapitulino On 03/12/12 16:50, Alon Levy wrote: > On Mon, Mar 12, 2012 at 01:43:11PM +0200, Alon Levy wrote: >> On Mon, Mar 12, 2012 at 11:20:55AM +0100, Gerd Hoffmann wrote: >>> On 03/11/12 20:26, Alon Levy wrote: >>>> dprint is still used for qxl_init_common one time prints. >>> >>> I think we shouldn't simply convert the dprintf's into trace-points. >>> >>> We should look at each dprintf and check whenever it makes sense at all, >>> whenever it makes sense at that place before converting it over to a >>> tracepoint. > > I'll also add qxl_spice_* trace points for the next patch. Does that > sound excessive? you could just trace the qxl_io_write to get the io > itself, or trace just qxl_spice_* to get the qxl<->spice interface, or > both (qxl_*). Makes sense to place trace points systematically like that. cheers, Gerd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-13 6:42 ` Gerd Hoffmann @ 2012-03-13 9:35 ` Alon Levy 2012-03-13 9:47 ` Gerd Hoffmann 0 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-13 9:35 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, lcapitulino On Tue, Mar 13, 2012 at 07:42:17AM +0100, Gerd Hoffmann wrote: > On 03/12/12 16:50, Alon Levy wrote: > > On Mon, Mar 12, 2012 at 01:43:11PM +0200, Alon Levy wrote: > >> On Mon, Mar 12, 2012 at 11:20:55AM +0100, Gerd Hoffmann wrote: > >>> On 03/11/12 20:26, Alon Levy wrote: > >>>> dprint is still used for qxl_init_common one time prints. > >>> > >>> I think we shouldn't simply convert the dprintf's into trace-points. > >>> > >>> We should look at each dprintf and check whenever it makes sense at all, > >>> whenever it makes sense at that place before converting it over to a > >>> tracepoint. > > > > I'll also add qxl_spice_* trace points for the next patch. Does that > > sound excessive? you could just trace the qxl_io_write to get the io > > itself, or trace just qxl_spice_* to get the qxl<->spice interface, or > > both (qxl_*). > > Makes sense to place trace points systematically like that. > What about having the frequent (read: too frequent to use stderr to dump them since they clutter the screen, unless you 'stop' before each monitor command) have a postfix "_freq"? This is a stopgap, but helpful one, you can then do: trace-event qxl* on trace-event qxl*freq off Instead of remembering / having conveniently ready a longer list: trace-event qxl* on trace-event qxl_interface_get_command_enter off trace-event qxl_interface_release_resource off trace-event qxl_interface_get_command_ret off trace-event qxl_push_free_res off > cheers, > Gerd > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-13 9:35 ` Alon Levy @ 2012-03-13 9:47 ` Gerd Hoffmann 2012-03-13 10:18 ` Alon Levy 2012-03-13 10:26 ` Alon Levy 0 siblings, 2 replies; 35+ messages in thread From: Gerd Hoffmann @ 2012-03-13 9:47 UTC (permalink / raw) To: aliguori, qemu-devel, lcapitulino Hi, > What about having the frequent (read: too frequent to use stderr to dump > them since they clutter the screen, unless you 'stop' before each > monitor command) have a postfix "_freq"? This is a stopgap, but helpful > one, you can then do: > trace-event qxl* on > trace-event qxl*freq off > > Instead of remembering / having conveniently ready a longer list: > trace-event qxl* on > trace-event qxl_interface_get_command_enter off > trace-event qxl_interface_release_resource off > trace-event qxl_interface_get_command_ret off > trace-event qxl_push_free_res off Hmm, I'd suggest to just try find better names. These all are about ring management (well, free_res is a bit special, but still ...), so maybe: qxl_ring_{command,cursor}_check (check whenever stuff is in there) qxl_ring_{command,cursor}_get (take item out of the ring) qxl_ring_res_put (stuff item into the ring) Then you can match them likewise easy with "qxl_ring_*", but you have descriptive names without the IMHO ugly _freq suffix. cheers, Gerd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-13 9:47 ` Gerd Hoffmann @ 2012-03-13 10:18 ` Alon Levy 2012-03-13 10:26 ` Alon Levy 1 sibling, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-13 10:18 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, lcapitulino On Tue, Mar 13, 2012 at 10:47:54AM +0100, Gerd Hoffmann wrote: > Hi, > > > What about having the frequent (read: too frequent to use stderr to dump > > them since they clutter the screen, unless you 'stop' before each > > monitor command) have a postfix "_freq"? This is a stopgap, but helpful > > one, you can then do: > > trace-event qxl* on > > trace-event qxl*freq off > > > > Instead of remembering / having conveniently ready a longer list: > > trace-event qxl* on > > trace-event qxl_interface_get_command_enter off > > trace-event qxl_interface_release_resource off > > trace-event qxl_interface_get_command_ret off > > trace-event qxl_push_free_res off > > Hmm, I'd suggest to just try find better names. These all are about > ring management (well, free_res is a bit special, but still ...), so maybe: > > qxl_ring_{command,cursor}_check (check whenever stuff is in there) > qxl_ring_{command,cursor}_get (take item out of the ring) > qxl_ring_res_put (stuff item into the ring) > > Then you can match them likewise easy with "qxl_ring_*", but you have > descriptive names without the IMHO ugly _freq suffix. Sounds good. > > cheers, > Gerd > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events 2012-03-13 9:47 ` Gerd Hoffmann 2012-03-13 10:18 ` Alon Levy @ 2012-03-13 10:26 ` Alon Levy 1 sibling, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-13 10:26 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, lcapitulino On Tue, Mar 13, 2012 at 10:47:54AM +0100, Gerd Hoffmann wrote: > Hi, > > > What about having the frequent (read: too frequent to use stderr to dump > > them since they clutter the screen, unless you 'stop' before each > > monitor command) have a postfix "_freq"? This is a stopgap, but helpful > > one, you can then do: > > trace-event qxl* on > > trace-event qxl*freq off > > > > Instead of remembering / having conveniently ready a longer list: > > trace-event qxl* on > > trace-event qxl_interface_get_command_enter off > > trace-event qxl_interface_release_resource off > > trace-event qxl_interface_get_command_ret off > > trace-event qxl_push_free_res off > > Hmm, I'd suggest to just try find better names. These all are about > ring management (well, free_res is a bit special, but still ...), so maybe: > > qxl_ring_{command,cursor}_check (check whenever stuff is in there) > qxl_ring_{command,cursor}_get (take item out of the ring) > qxl_ring_res_put (stuff item into the ring) Hmm, actually for vga mode the names are misleading. I think I'll stick with them anyway, they do have a mode argument so you can see it's in vga mode. > > Then you can match them likewise easy with "qxl_ring_*", but you have > descriptive names without the IMHO ugly _freq suffix. > > cheers, > Gerd > > ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] qxl/qxl_render.c: add trace events 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events Alon Levy @ 2012-03-11 19:26 ` Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh Alon Levy ` (2 subsequent siblings) 4 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 13 ++++--------- trace-events | 6 ++++++ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 25857f6..74e7ea3 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -31,11 +31,10 @@ static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect) return; } if (!qxl->guest_primary.data) { - dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__); + trace_qxl_blit_guest_primary_initialized(); qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); } - dprint(qxl, 2, "%s: stride %d, [%d, %d, %d, %d]\n", __func__, - qxl->guest_primary.qxl_stride, + trace_qxl_blit(qxl->guest_primary.qxl_stride, rect->left, rect->right, rect->top, rect->bottom); src = qxl->guest_primary.data; if (qxl->guest_primary.qxl_stride < 0) { @@ -107,8 +106,7 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); qxl_set_rect_to_surface(qxl, &qxl->dirty[0]); qxl->num_dirty_rects = 1; - dprint(qxl, 1, "%s: %dx%d, stride %d, bpp %d, depth %d\n", - __FUNCTION__, + trace_qxl_guest_primary_resized( qxl->guest_primary.surface.width, qxl->guest_primary.surface.height, qxl->guest_primary.qxl_stride, @@ -118,8 +116,6 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) if (surface->width != qxl->guest_primary.surface.width || surface->height != qxl->guest_primary.surface.height) { if (qxl->guest_primary.qxl_stride > 0) { - dprint(qxl, 1, "%s: using guest_primary for displaysurface\n", - __func__); qemu_free_displaysurface(vga->ds); qemu_create_displaysurface_from(qxl->guest_primary.surface.width, qxl->guest_primary.surface.height, @@ -127,8 +123,6 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) qxl->guest_primary.abs_stride, qxl->guest_primary.data); } else { - dprint(qxl, 1, "%s: resizing displaysurface to guest_primary\n", - __func__); qemu_resize_displaysurface(vga->ds, qxl->guest_primary.surface.width, qxl->guest_primary.surface.height); @@ -187,6 +181,7 @@ void qxl_render_update_area_bh(void *opaque) void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) { qemu_mutex_lock(&qxl->ssd.lock); + trace_qxl_render_update_area_done(cookie); qemu_bh_schedule(qxl->update_area_bh); qxl->render_update_cookie_num--; qemu_mutex_unlock(&qxl->ssd.lock); diff --git a/trace-events b/trace-events index 0853a1b..a66aee8 100644 --- a/trace-events +++ b/trace-events @@ -712,3 +712,9 @@ qxl_post_load_enter(void) "" qxl_post_load_restore_mode(const char *mode) "%s" qxl_post_load_exit(void) "" qxl_interface_update_area_complete(uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom, uint32_t num_updated_rects) "surface=%d [%d,%d,%d,%d] #=%d" + +# hw/qxl-render.c +qxl_blit_guest_primary_initialized(void) "" +qxl_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]" +qxl_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d" +qxl_render_update_area_done(void *cookie) "%p" -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 2/5] qxl/qxl_render.c: add trace events Alon Levy @ 2012-03-11 19:26 ` Alon Levy 2012-03-13 13:22 ` Luiz Capitulino 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 5/5] qxl: screendump: use provided Monitor Alon Levy 4 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori With this change ppm_save is called after rendering, and not before. There are two lose ends: hmp: monitor will be active before ppm_save is complete. qmp: return will be emitted before ppm_save is complete. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++----- hw/qxl.c | 5 +-- hw/qxl.h | 2 +- trace-events | 2 + ui/spice-display.h | 2 + 5 files changed, 76 insertions(+), 11 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 74e7ea3..b281766 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -19,6 +19,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include "console.h" #include "qxl.h" static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect) @@ -142,12 +143,68 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) } /* + * struct used just for ppm save bh. We don't actually support multiple qxl + * screendump yet, but a) we will, and b) exporting qxl0 from qxl.c looks + * uglier imo. + */ +typedef struct QXLPPMSaveBHData { + PCIQXLDevice *qxl; + QXLCookie *cookie; +} QXLPPMSaveBHData; + +static void qxl_render_ppm_save_bh(void *opaque); + +static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename) +{ + QXLPPMSaveBHData *ppm_save_bh_data; + QEMUBH *ppm_save_bh; + QXLCookie *cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, + 0); + + qxl_set_rect_to_surface(qxl, &cookie->u.render.area); + if (filename) { + ppm_save_bh_data = g_malloc0(sizeof(*ppm_save_bh_data)); + ppm_save_bh_data->qxl = qxl; + ppm_save_bh_data->cookie = cookie; + ppm_save_bh = qemu_bh_new(qxl_render_ppm_save_bh, ppm_save_bh_data); + cookie->u.render.filename = g_strdup(filename); + cookie->u.render.ppm_save_bh = ppm_save_bh; + } + return cookie; +} + +static void qxl_cookie_render_free(PCIQXLDevice *qxl, QXLCookie *cookie) +{ + g_free(cookie->u.render.filename); + g_free(cookie); + --qxl->render_update_cookie_num; +} + +static void qxl_render_ppm_save_bh(void *opaque) +{ + QXLPPMSaveBHData *data = opaque; + PCIQXLDevice *qxl = data->qxl; + QXLCookie *cookie = data->cookie; + QEMUBH *bh = cookie->u.render.ppm_save_bh; + + qemu_mutex_lock(&qxl->ssd.lock); + trace_qxl_render_ppm_save_bh( + qxl->ssd.ds->surface->data, qxl->guest_primary.data); + qxl_render_update_area_unlocked(qxl); + ppm_save(cookie->u.render.filename, qxl->ssd.ds->surface); + qxl_cookie_render_free(qxl, cookie); + qemu_mutex_unlock(&qxl->ssd.lock); + g_free(data); + qemu_bh_delete(bh); +} + +/* * use ssd.lock to protect render_update_cookie_num. * qxl_render_update is called by io thread or vcpu thread, and the completion * callbacks are called by spice_server thread, defering to bh called from the * io thread. */ -void qxl_render_update(PCIQXLDevice *qxl) +void qxl_render_update(PCIQXLDevice *qxl, const char *filename) { QXLCookie *cookie; @@ -155,6 +212,10 @@ void qxl_render_update(PCIQXLDevice *qxl) if (!runstate_is_running() || !qxl->guest_primary.commands) { qxl_render_update_area_unlocked(qxl); + if (filename) { + trace_qxl_render_update_screendump_no_update(); + ppm_save(filename, qxl->ssd.ds->surface); + } qemu_mutex_unlock(&qxl->ssd.lock); return; } @@ -162,9 +223,7 @@ void qxl_render_update(PCIQXLDevice *qxl) qxl->guest_primary.commands = 0; qxl->render_update_cookie_num++; qemu_mutex_unlock(&qxl->ssd.lock); - cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, - 0); - qxl_set_rect_to_surface(qxl, &cookie->u.render.area); + cookie = qxl_cookie_render_new(qxl, filename); qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL, 0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie); } @@ -182,10 +241,13 @@ void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) { qemu_mutex_lock(&qxl->ssd.lock); trace_qxl_render_update_area_done(cookie); - qemu_bh_schedule(qxl->update_area_bh); - qxl->render_update_cookie_num--; + if (cookie->u.render.filename) { + qemu_bh_schedule(cookie->u.render.ppm_save_bh); + } else { + qemu_bh_schedule(qxl->update_area_bh); + qxl_cookie_render_free(qxl, cookie); + } qemu_mutex_unlock(&qxl->ssd.lock); - g_free(cookie); } static QEMUCursor *qxl_cursor(PCIQXLDevice *qxl, QXLCursor *cursor) diff --git a/hw/qxl.c b/hw/qxl.c index 7857731..bcfd661 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1471,7 +1471,7 @@ static void qxl_hw_update(void *opaque) break; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); + qxl_render_update(qxl, NULL); break; default: break; @@ -1494,8 +1494,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch) switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl); - ppm_save(filename, qxl->ssd.ds->surface); + qxl_render_update(qxl, filename); break; case QXL_MODE_VGA: vga->screen_dump(vga, filename, cswitch); diff --git a/hw/qxl.h b/hw/qxl.h index 11a0db3..417ab28 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -147,7 +147,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext); /* qxl-render.c */ void qxl_render_resize(PCIQXLDevice *qxl); -void qxl_render_update(PCIQXLDevice *qxl); +void qxl_render_update(PCIQXLDevice *qxl, const char *filename); void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext); void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie); void qxl_render_update_area_bh(void *opaque); diff --git a/trace-events b/trace-events index a66aee8..2f045c4 100644 --- a/trace-events +++ b/trace-events @@ -718,3 +718,5 @@ qxl_blit_guest_primary_initialized(void) "" qxl_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]" qxl_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d" qxl_render_update_area_done(void *cookie) "%p" +qxl_render_update_screendump_no_update(void) "" +qxl_render_ppm_save_bh(void *data, void *primary) "%p (primary %p)" diff --git a/ui/spice-display.h b/ui/spice-display.h index 12e50b6..ec1fc24 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -62,6 +62,8 @@ typedef struct QXLCookie { struct { QXLRect area; int redraw; + char *filename; + QEMUBH *ppm_save_bh; } render; } u; } QXLCookie; -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh Alon Levy @ 2012-03-13 13:22 ` Luiz Capitulino 0 siblings, 0 replies; 35+ messages in thread From: Luiz Capitulino @ 2012-03-13 13:22 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, qemu-devel, kraxel On Sun, 11 Mar 2012 21:26:42 +0200 Alon Levy <alevy@redhat.com> wrote: > With this change ppm_save is called after rendering, and not before. > There are two lose ends: > hmp: monitor will be active before ppm_save is complete. The plan is to lock hmp's shell until rendering completes and the file is saved. Looks ok to me. > qmp: return will be emitted before ppm_save is complete. Let see if I got this right, please correct me with I'm wrong: o Before this commit: when screendump returns, there's an out of date screendump file available in the FS o After this commit: when screendump returns, there's no screendump file available yet There's a behavior change, which is better done via a new command, which would need to be async and we don't support that today. Also, having an out of date screendump is not exactly useful anyway. Honestly, I don't know what's the best thing to do in this case, but I'll be fine with it if Gerd acks this patch. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy ` (2 preceding siblings ...) 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh Alon Levy @ 2012-03-11 19:26 ` Alon Levy 2012-03-13 13:35 ` Luiz Capitulino 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 5/5] qxl: screendump: use provided Monitor Alon Levy 4 siblings, 1 reply; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori Passes the Monitor ptr to the screendump implementation to all for monitor suspend and resume for qxl to fix screendump regression. graphics_console_init signature change required touching every implemented of screen_dump. There is no change other then an added parameter. qxl will make use of it in the next patch. compiles with ./configure Signed-off-by: Alon Levy <alevy@redhat.com> --- console.c | 4 ++-- console.h | 5 +++-- hw/blizzard.c | 2 +- hw/g364fb.c | 3 ++- hw/omap_dss.c | 4 +++- hw/omap_lcdc.c | 3 ++- hw/qxl.c | 5 +++-- hw/sm501.c | 4 ++-- hw/tcx.c | 12 ++++++++---- hw/vga.c | 6 ++++-- hw/vmware_vga.c | 5 +++-- monitor.c | 2 +- 12 files changed, 34 insertions(+), 21 deletions(-) diff --git a/console.c b/console.c index 6a463f5..3e386fc 100644 --- a/console.c +++ b/console.c @@ -173,7 +173,7 @@ void vga_hw_invalidate(void) active_console->hw_invalidate(active_console->hw); } -void vga_hw_screen_dump(const char *filename) +void vga_hw_screen_dump(const char *filename, Monitor *mon) { TextConsole *previous_active_console; bool cswitch; @@ -187,7 +187,7 @@ void vga_hw_screen_dump(const char *filename) console_select(0); } if (consoles[0] && consoles[0]->hw_screen_dump) { - consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch); + consoles[0]->hw_screen_dump(consoles[0]->hw, filename, cswitch, mon); } else { error_report("screen dump not implemented"); } diff --git a/console.h b/console.h index 4334db5..0d2cf30 100644 --- a/console.h +++ b/console.h @@ -343,7 +343,8 @@ static inline void console_write_ch(console_ch_t *dest, uint32_t ch) typedef void (*vga_hw_update_ptr)(void *); typedef void (*vga_hw_invalidate_ptr)(void *); -typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch); +typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch, + Monitor *mon); typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *); DisplayState *graphic_console_init(vga_hw_update_ptr update, @@ -354,7 +355,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr update, void vga_hw_update(void); void vga_hw_invalidate(void); -void vga_hw_screen_dump(const char *filename); +void vga_hw_screen_dump(const char *filename, Monitor *mon); void vga_hw_text_update(console_ch_t *chardata); int is_graphic_console(void); diff --git a/hw/blizzard.c b/hw/blizzard.c index c7d844d..8ccea7f 100644 --- a/hw/blizzard.c +++ b/hw/blizzard.c @@ -933,7 +933,7 @@ static void blizzard_update_display(void *opaque) } static void blizzard_screen_dump(void *opaque, const char *filename, - bool cswitch) + bool cswitch, Monitor *mon) { BlizzardState *s = (BlizzardState *) opaque; diff --git a/hw/g364fb.c b/hw/g364fb.c index 3a0b68f..f89000c 100644 --- a/hw/g364fb.c +++ b/hw/g364fb.c @@ -289,7 +289,8 @@ static void g364fb_reset(G364State *s) g364fb_invalidate_display(s); } -static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch) +static void g364fb_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { G364State *s = opaque; int y, x; diff --git a/hw/omap_dss.c b/hw/omap_dss.c index 86ed6ea..b4a1a93 100644 --- a/hw/omap_dss.c +++ b/hw/omap_dss.c @@ -1072,7 +1072,9 @@ struct omap_dss_s *omap_dss_init(struct omap_target_agent_s *ta, #if 0 s->state = graphic_console_init(omap_update_display, - omap_invalidate_display, omap_screen_dump, s); + omap_invalidate_display, + omap_screen_dump, + NULL, s); #endif return s; diff --git a/hw/omap_lcdc.c b/hw/omap_lcdc.c index f172093..ed2325d 100644 --- a/hw/omap_lcdc.c +++ b/hw/omap_lcdc.c @@ -264,7 +264,8 @@ static int ppm_save(const char *filename, uint8_t *data, return 0; } -static void omap_screen_dump(void *opaque, const char *filename, bool cswitch) +static void omap_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { struct omap_lcd_panel_s *omap_lcd = opaque; if (cswitch) { diff --git a/hw/qxl.c b/hw/qxl.c index bcfd661..d21b508 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1486,7 +1486,8 @@ static void qxl_hw_invalidate(void *opaque) vga->invalidate(vga); } -static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch) +static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { PCIQXLDevice *qxl = opaque; VGACommonState *vga = &qxl->vga; @@ -1497,7 +1498,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch) qxl_render_update(qxl, filename); break; case QXL_MODE_VGA: - vga->screen_dump(vga, filename, cswitch); + vga->screen_dump(vga, filename, cswitch, mon); break; default: break; diff --git a/hw/sm501.c b/hw/sm501.c index 786e076..eedcb8e 100644 --- a/hw/sm501.c +++ b/hw/sm501.c @@ -1442,6 +1442,6 @@ void sm501_init(MemoryRegion *address_space_mem, uint32_t base, } /* create qemu graphic console */ - s->ds = graphic_console_init(sm501_update_display, NULL, - NULL, NULL, s); + s->ds = graphic_console_init(sm501_update_display, NULL, NULL, + NULL, s); } diff --git a/hw/tcx.c b/hw/tcx.c index ac7dcb4..e800cb5 100644 --- a/hw/tcx.c +++ b/hw/tcx.c @@ -56,8 +56,10 @@ typedef struct TCXState { uint8_t dac_index, dac_state; } TCXState; -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch); -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch); +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); static void tcx_set_dirty(TCXState *s) { @@ -574,7 +576,8 @@ static int tcx_init1(SysBusDevice *dev) return 0; } -static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch) +static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { TCXState *s = opaque; FILE *f; @@ -601,7 +604,8 @@ static void tcx_screen_dump(void *opaque, const char *filename, bool cswitch) return; } -static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch) +static void tcx24_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { TCXState *s = opaque; FILE *f; diff --git a/hw/vga.c b/hw/vga.c index 6dc98f6..9af231e 100644 --- a/hw/vga.c +++ b/hw/vga.c @@ -163,7 +163,8 @@ static uint32_t expand4[256]; static uint16_t expand2[256]; static uint8_t expand4to8[16]; -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch); +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon); static void vga_update_memory_access(VGACommonState *s) { @@ -2409,7 +2410,8 @@ int ppm_save(const char *filename, struct DisplaySurface *ds) /* save the vga display in a PPM image even if no display is available */ -static void vga_screen_dump(void *opaque, const char *filename, bool cswitch) +static void vga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { VGACommonState *s = opaque; diff --git a/hw/vmware_vga.c b/hw/vmware_vga.c index 142d9f4..803118c 100644 --- a/hw/vmware_vga.c +++ b/hw/vmware_vga.c @@ -1003,11 +1003,12 @@ static void vmsvga_invalidate_display(void *opaque) /* save the vga display in a PPM image even if no display is available */ -static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch) +static void vmsvga_screen_dump(void *opaque, const char *filename, bool cswitch, + Monitor *mon) { struct vmsvga_state_s *s = opaque; if (!s->enable) { - s->vga.screen_dump(&s->vga, filename, cswitch); + s->vga.screen_dump(&s->vga, filename, cswitch, mon); return; } diff --git a/monitor.c b/monitor.c index cbdfbad..cdae23f 100644 --- a/monitor.c +++ b/monitor.c @@ -895,7 +895,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict, static int do_screen_dump(Monitor *mon, const QDict *qdict, QObject **ret_data) { - vga_hw_screen_dump(qdict_get_str(qdict, "filename")); + vga_hw_screen_dump(qdict_get_str(qdict, "filename"), mon); return 0; } -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy @ 2012-03-13 13:35 ` Luiz Capitulino 2012-03-13 14:46 ` Alon Levy 0 siblings, 1 reply; 35+ messages in thread From: Luiz Capitulino @ 2012-03-13 13:35 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, qemu-devel, kraxel On Sun, 11 Mar 2012 21:26:43 +0200 Alon Levy <alevy@redhat.com> wrote: > Passes the Monitor ptr to the screendump implementation to all for > monitor suspend and resume for qxl to fix screendump regression. > > graphics_console_init signature change required touching every > implemented of screen_dump. There is no change other then an added > parameter. qxl will make use of it in the next patch. NACK on this one. The Monitor object should be restricted to HMP. This patch spreads it to what's going to be the QMP implementation of screendump. The first step here should be to convert the screendump command to the qapi, and lock the HMP shell in hmp_screendump(). However, this brings a new interesting problem: the HMP implementation is actually a QMP client, meaning that it won't have a way to figure out screendump completion either :) Some solutions that come to my mind: 1. Pool the screendump file creation from a timer. Cons: it may return before the file is fully written to disk 2. Use inotify Cons: what about windows? 3. Introduce query-screendump that returns the last screendump status Cons: this is actually making screendump async Anthony, do you have any ideas? Btw, I've started doing the screendump conversion to the qapi, I'll post it soon. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 13:35 ` Luiz Capitulino @ 2012-03-13 14:46 ` Alon Levy 2012-03-13 15:59 ` Luiz Capitulino 2012-03-14 8:14 ` Gerd Hoffmann 0 siblings, 2 replies; 35+ messages in thread From: Alon Levy @ 2012-03-13 14:46 UTC (permalink / raw) To: Luiz Capitulino; +Cc: aliguori, qemu-devel, kraxel On Tue, Mar 13, 2012 at 10:35:55AM -0300, Luiz Capitulino wrote: > On Sun, 11 Mar 2012 21:26:43 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > Passes the Monitor ptr to the screendump implementation to all for > > monitor suspend and resume for qxl to fix screendump regression. > > > > graphics_console_init signature change required touching every > > implemented of screen_dump. There is no change other then an added > > parameter. qxl will make use of it in the next patch. > > NACK on this one. > > The Monitor object should be restricted to HMP. This patch spreads it to > what's going to be the QMP implementation of screendump. > > The first step here should be to convert the screendump command to the qapi, > and lock the HMP shell in hmp_screendump(). > > However, this brings a new interesting problem: the HMP implementation is > actually a QMP client, meaning that it won't have a way to figure out > screendump completion either :) > > Some solutions that come to my mind: > > 1. Pool the screendump file creation from a timer. > > Cons: it may return before the file is fully written to disk > We know what the file size should be, so we can poll for the actual size. Actually why do we need to poll? we could add a "internal.screendump.complete" or "internal-query-screendump", no? > 2. Use inotify > > Cons: what about windows? > > 3. Introduce query-screendump that returns the last screendump status > > Cons: this is actually making screendump async > > > Anthony, do you have any ideas? > > Btw, I've started doing the screendump conversion to the qapi, I'll post it > soon. I've already sent patches once for a new qapi command, I don't mind you doing this of course. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 14:46 ` Alon Levy @ 2012-03-13 15:59 ` Luiz Capitulino 2012-03-13 17:35 ` Alon Levy 2012-03-14 8:14 ` Gerd Hoffmann 1 sibling, 1 reply; 35+ messages in thread From: Luiz Capitulino @ 2012-03-13 15:59 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, qemu-devel, kraxel On Tue, 13 Mar 2012 16:46:12 +0200 Alon Levy <alevy@redhat.com> wrote: > On Tue, Mar 13, 2012 at 10:35:55AM -0300, Luiz Capitulino wrote: > > On Sun, 11 Mar 2012 21:26:43 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > > > Passes the Monitor ptr to the screendump implementation to all for > > > monitor suspend and resume for qxl to fix screendump regression. > > > > > > graphics_console_init signature change required touching every > > > implemented of screen_dump. There is no change other then an added > > > parameter. qxl will make use of it in the next patch. > > > > NACK on this one. > > > > The Monitor object should be restricted to HMP. This patch spreads it to > > what's going to be the QMP implementation of screendump. > > > > The first step here should be to convert the screendump command to the qapi, > > and lock the HMP shell in hmp_screendump(). > > > > However, this brings a new interesting problem: the HMP implementation is > > actually a QMP client, meaning that it won't have a way to figure out > > screendump completion either :) > > > > Some solutions that come to my mind: > > > > 1. Pool the screendump file creation from a timer. > > > > Cons: it may return before the file is fully written to disk > > > > We know what the file size should be, so we can poll for the actual > size. Actually why do we need to poll? we could add a > "internal.screendump.complete" or "internal-query-screendump", no? Neither is possible because hmp.c really uses QMP as client, except that it's done via C. > > 2. Use inotify > > > > Cons: what about windows? > > > > 3. Introduce query-screendump that returns the last screendump status > > > > Cons: this is actually making screendump async > > > > > > Anthony, do you have any ideas? > > > > Btw, I've started doing the screendump conversion to the qapi, I'll post it > > soon. > > I've already sent patches once for a new qapi command, I don't mind you > doing this of course. It would actually help me if you do it, I have an initial version at: git://repo.or.cz/qemu/qmp-unstable.git qmp-wip/qapi-commands-conv/screendump/rfc It's old and it's on top of some uninteresting stuff, the only commits that matter there are 48e7c01b and 0f5509a8. But this rfc is just to have an idea how the final command will look like. From what I barely remember, I'd suggest you to do the following: 1. Pass the Error object to hw_screen_dump() 2. Convert the screendump command to the qapi 3. Report errors from ppm_save() via Error Important note: my code reports only QERR_OPEN_FILE_FAILED in ppm_save(), but the right thing to do is to report most likely errors like EACCESS, ENOSPC, EPERM, EIO etc. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 15:59 ` Luiz Capitulino @ 2012-03-13 17:35 ` Alon Levy 2012-03-13 18:07 ` Luiz Capitulino 2012-03-14 8:25 ` Gerd Hoffmann 0 siblings, 2 replies; 35+ messages in thread From: Alon Levy @ 2012-03-13 17:35 UTC (permalink / raw) To: Luiz Capitulino; +Cc: aliguori, qemu-devel, kraxel On Tue, Mar 13, 2012 at 12:59:17PM -0300, Luiz Capitulino wrote: > On Tue, 13 Mar 2012 16:46:12 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > On Tue, Mar 13, 2012 at 10:35:55AM -0300, Luiz Capitulino wrote: > > > On Sun, 11 Mar 2012 21:26:43 +0200 > > > Alon Levy <alevy@redhat.com> wrote: > > > > > > > Passes the Monitor ptr to the screendump implementation to all for > > > > monitor suspend and resume for qxl to fix screendump regression. > > > > > > > > graphics_console_init signature change required touching every > > > > implemented of screen_dump. There is no change other then an added > > > > parameter. qxl will make use of it in the next patch. > > > > > > NACK on this one. > > > > > > The Monitor object should be restricted to HMP. This patch spreads it to > > > what's going to be the QMP implementation of screendump. > > > > > > The first step here should be to convert the screendump command to the qapi, > > > and lock the HMP shell in hmp_screendump(). > > > > > > However, this brings a new interesting problem: the HMP implementation is > > > actually a QMP client, meaning that it won't have a way to figure out > > > screendump completion either :) > > > > > > Some solutions that come to my mind: > > > > > > 1. Pool the screendump file creation from a timer. > > > > > > Cons: it may return before the file is fully written to disk > > > > > > > We know what the file size should be, so we can poll for the actual > > size. Actually why do we need to poll? we could add a > > "internal.screendump.complete" or "internal-query-screendump", no? > > Neither is possible because hmp.c really uses QMP as client, except that it's > done via C. > > > > 2. Use inotify > > > > > > Cons: what about windows? > > > > > > 3. Introduce query-screendump that returns the last screendump status > > > > > > Cons: this is actually making screendump async > > > > > > > > > Anthony, do you have any ideas? > > > > > > Btw, I've started doing the screendump conversion to the qapi, I'll post it > > > soon. > > > > I've already sent patches once for a new qapi command, I don't mind you > > doing this of course. > > It would actually help me if you do it, I have an initial version at: > > git://repo.or.cz/qemu/qmp-unstable.git qmp-wip/qapi-commands-conv/screendump/rfc > > It's old and it's on top of some uninteresting stuff, the only commits that > matter there are 48e7c01b and 0f5509a8. But this rfc is just to have an idea > how the final command will look like. > > From what I barely remember, I'd suggest you to do the following: > > 1. Pass the Error object to hw_screen_dump() > 2. Convert the screendump command to the qapi > 3. Report errors from ppm_save() via Error > > Important note: my code reports only QERR_OPEN_FILE_FAILED in ppm_save(), but > the right thing to do is to report most likely errors like EACCESS, ENOSPC, > EPERM, EIO etc. ok, I'll take it. Note that I'm going to not send the qxl screendump behavior changing patch again, I think all alternatives to a real async screendump suck, including libvirt changes, and the current behavior of giving an old screendump is good enough hopefully for the real users, which are: boxes - refreshing to get updated thumbnails for it's users - will have 5 seconds old screenshot until async monitor commands land. autotest - I hope they don't compare image contents (that would be hard to do), otherwise it's just for logging sake, and so it will lag a little, too bad. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 17:35 ` Alon Levy @ 2012-03-13 18:07 ` Luiz Capitulino 2012-03-14 8:25 ` Gerd Hoffmann 1 sibling, 0 replies; 35+ messages in thread From: Luiz Capitulino @ 2012-03-13 18:07 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, qemu-devel, kraxel On Tue, 13 Mar 2012 19:35:24 +0200 Alon Levy <alevy@redhat.com> wrote: > On Tue, Mar 13, 2012 at 12:59:17PM -0300, Luiz Capitulino wrote: > > On Tue, 13 Mar 2012 16:46:12 +0200 > > Alon Levy <alevy@redhat.com> wrote: > > > > > On Tue, Mar 13, 2012 at 10:35:55AM -0300, Luiz Capitulino wrote: > > > > On Sun, 11 Mar 2012 21:26:43 +0200 > > > > Alon Levy <alevy@redhat.com> wrote: > > > > > > > > > Passes the Monitor ptr to the screendump implementation to all for > > > > > monitor suspend and resume for qxl to fix screendump regression. > > > > > > > > > > graphics_console_init signature change required touching every > > > > > implemented of screen_dump. There is no change other then an added > > > > > parameter. qxl will make use of it in the next patch. > > > > > > > > NACK on this one. > > > > > > > > The Monitor object should be restricted to HMP. This patch spreads it to > > > > what's going to be the QMP implementation of screendump. > > > > > > > > The first step here should be to convert the screendump command to the qapi, > > > > and lock the HMP shell in hmp_screendump(). > > > > > > > > However, this brings a new interesting problem: the HMP implementation is > > > > actually a QMP client, meaning that it won't have a way to figure out > > > > screendump completion either :) > > > > > > > > Some solutions that come to my mind: > > > > > > > > 1. Pool the screendump file creation from a timer. > > > > > > > > Cons: it may return before the file is fully written to disk > > > > > > > > > > We know what the file size should be, so we can poll for the actual > > > size. Actually why do we need to poll? we could add a > > > "internal.screendump.complete" or "internal-query-screendump", no? > > > > Neither is possible because hmp.c really uses QMP as client, except that it's > > done via C. > > > > > > 2. Use inotify > > > > > > > > Cons: what about windows? > > > > > > > > 3. Introduce query-screendump that returns the last screendump status > > > > > > > > Cons: this is actually making screendump async > > > > > > > > > > > > Anthony, do you have any ideas? > > > > > > > > Btw, I've started doing the screendump conversion to the qapi, I'll post it > > > > soon. > > > > > > I've already sent patches once for a new qapi command, I don't mind you > > > doing this of course. > > > > It would actually help me if you do it, I have an initial version at: > > > > git://repo.or.cz/qemu/qmp-unstable.git qmp-wip/qapi-commands-conv/screendump/rfc > > > > It's old and it's on top of some uninteresting stuff, the only commits that > > matter there are 48e7c01b and 0f5509a8. But this rfc is just to have an idea > > how the final command will look like. > > > > From what I barely remember, I'd suggest you to do the following: > > > > 1. Pass the Error object to hw_screen_dump() > > 2. Convert the screendump command to the qapi > > 3. Report errors from ppm_save() via Error > > > > Important note: my code reports only QERR_OPEN_FILE_FAILED in ppm_save(), but > > the right thing to do is to report most likely errors like EACCESS, ENOSPC, > > EPERM, EIO etc. > > ok, I'll take it. Note that I'm going to not send the qxl screendump > behavior changing patch again, I think all alternatives to a real async > screendump suck, including libvirt changes, and the current behavior of > giving an old screendump is good enough hopefully for the real users, > which are: Fine with me, you can add a note in screendump's documentation in qapi-schema.json explaining qxl's behavior. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 17:35 ` Alon Levy 2012-03-13 18:07 ` Luiz Capitulino @ 2012-03-14 8:25 ` Gerd Hoffmann 2012-03-14 8:32 ` Alon Levy 1 sibling, 1 reply; 35+ messages in thread From: Gerd Hoffmann @ 2012-03-14 8:25 UTC (permalink / raw) To: Luiz Capitulino, qemu-devel, aliguori Hi, > ok, I'll take it. Note that I'm going to not send the qxl screendump > behavior changing patch again, I think all alternatives to a real async > screendump suck, including libvirt changes, and the current behavior of > giving an old screendump is good enough hopefully for the real users, > which are: Agree here. > boxes - refreshing to get updated thumbnails for it's users - will have > 5 seconds old screenshot until async monitor commands land. > autotest - I hope they don't compare image contents (that would be hard > to do), otherwise it's just for logging sake, and so it will lag a > little, too bad. autotest has (used to have?) a install mode where it looked at the screen. I think it isn't used any more these days and unattended install modes (kickstart etc.) of the guests are used instead. But even when checking the screen content it should still work fine, just take a little longer because the screen shots are a bit behind. cheers, Gerd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 8:25 ` Gerd Hoffmann @ 2012-03-14 8:32 ` Alon Levy 0 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-14 8:32 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, Luiz Capitulino On Wed, Mar 14, 2012 at 09:25:29AM +0100, Gerd Hoffmann wrote: > Hi, > > > ok, I'll take it. Note that I'm going to not send the qxl screendump > > behavior changing patch again, I think all alternatives to a real async > > screendump suck, including libvirt changes, and the current behavior of > > giving an old screendump is good enough hopefully for the real users, > > which are: > > Agree here. > > > boxes - refreshing to get updated thumbnails for it's users - will have > > 5 seconds old screenshot until async monitor commands land. > > autotest - I hope they don't compare image contents (that would be hard > > to do), otherwise it's just for logging sake, and so it will lag a > > little, too bad. > > autotest has (used to have?) a install mode where it looked at the > screen. I think it isn't used any more these days and unattended > install modes (kickstart etc.) of the guests are used instead. But even > when checking the screen content it should still work fine, just take a > little longer because the screen shots are a bit behind. > Right. Actually I thought about install mode and assumed it would break it but you're right, it shouldn't. > cheers, > Gerd > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-13 14:46 ` Alon Levy 2012-03-13 15:59 ` Luiz Capitulino @ 2012-03-14 8:14 ` Gerd Hoffmann 2012-03-14 8:37 ` Daniel P. Berrange 1 sibling, 1 reply; 35+ messages in thread From: Gerd Hoffmann @ 2012-03-14 8:14 UTC (permalink / raw) To: Luiz Capitulino, qemu-devel, aliguori Hi, >> Some solutions that come to my mind: >> >> 1. Pool the screendump file creation from a timer. >> >> Cons: it may return before the file is fully written to disk >> > > We know what the file size should be, so we can poll for the actual > size. Actually why do we need to poll? we could add a > "internal.screendump.complete" or "internal-query-screendump", no? Marc-Andre currently looks at adding support for other file formats. I think it would be good to team up with him. First, with this applied you will not know the size in advance. Also one of the approaches discussed is to allow passing in a file handle. That is a possible way to handle async screendumps too: just write to the passed file handle and close it when done. Obvious drawback is that this will not cover the classic way of specifying the output filename as argument. cheers, Gerd ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 8:14 ` Gerd Hoffmann @ 2012-03-14 8:37 ` Daniel P. Berrange 2012-03-14 12:32 ` Luiz Capitulino 0 siblings, 1 reply; 35+ messages in thread From: Daniel P. Berrange @ 2012-03-14 8:37 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: aliguori, qemu-devel, Luiz Capitulino On Wed, Mar 14, 2012 at 09:14:46AM +0100, Gerd Hoffmann wrote: > Hi, > > >> Some solutions that come to my mind: > >> > >> 1. Pool the screendump file creation from a timer. > >> > >> Cons: it may return before the file is fully written to disk > >> > > > > We know what the file size should be, so we can poll for the actual > > size. Actually why do we need to poll? we could add a > > "internal.screendump.complete" or "internal-query-screendump", no? > > Marc-Andre currently looks at adding support for other file formats. > I think it would be good to team up with him. > > First, with this applied you will not know the size in advance. Also > one of the approaches discussed is to allow passing in a file handle. > That is a possible way to handle async screendumps too: just write to > the passed file handle and close it when done. Obvious drawback is that > this will not cover the classic way of specifying the output filename as > argument. This would not be a problem from libvirt's POV - we don't really want a file on disk anyway, nor do we want to pull the whole image into memory. Our ideal approach is to just have an pipe FD with QEMU, which we just incrementally read image data from, and forward to the client app via a libvirt stream object. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 8:37 ` Daniel P. Berrange @ 2012-03-14 12:32 ` Luiz Capitulino 2012-03-14 13:14 ` Alon Levy 0 siblings, 1 reply; 35+ messages in thread From: Luiz Capitulino @ 2012-03-14 12:32 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: aliguori, Gerd Hoffmann, qemu-devel On Wed, 14 Mar 2012 08:37:13 +0000 "Daniel P. Berrange" <berrange@redhat.com> wrote: > > First, with this applied you will not know the size in advance. Also > > one of the approaches discussed is to allow passing in a file handle. > > That is a possible way to handle async screendumps too: just write to > > the passed file handle and close it when done. Obvious drawback is that > > this will not cover the classic way of specifying the output filename as > > argument. As Daniel explains below, this is not a drawback and there's no problem supporting multiple ways of returning the image. The real drawback of making this w/o async support is that you can't detect errors. > This would not be a problem from libvirt's POV - we don't really want a > file on disk anyway, nor do we want to pull the whole image into memory. > Our ideal approach is to just have an pipe FD with QEMU, which we just > incrementally read image data from, and forward to the client app via > a libvirt stream object. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 12:32 ` Luiz Capitulino @ 2012-03-14 13:14 ` Alon Levy 2012-03-14 13:17 ` Daniel P. Berrange 2012-03-14 13:18 ` Luiz Capitulino 0 siblings, 2 replies; 35+ messages in thread From: Alon Levy @ 2012-03-14 13:14 UTC (permalink / raw) To: Luiz Capitulino; +Cc: aliguori, Gerd Hoffmann, qemu-devel On Wed, Mar 14, 2012 at 09:32:58AM -0300, Luiz Capitulino wrote: > On Wed, 14 Mar 2012 08:37:13 +0000 > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > > First, with this applied you will not know the size in advance. Also > > > one of the approaches discussed is to allow passing in a file handle. > > > That is a possible way to handle async screendumps too: just write to > > > the passed file handle and close it when done. Obvious drawback is that > > > this will not cover the classic way of specifying the output filename as > > > argument. > > As Daniel explains below, this is not a drawback and there's no problem > supporting multiple ways of returning the image. > > The real drawback of making this w/o async support is that you can't detect > errors. > You also can't detect when the writing is done (unless you continuously try to parse the file yourself..) > > This would not be a problem from libvirt's POV - we don't really want a > > file on disk anyway, nor do we want to pull the whole image into memory. > > Our ideal approach is to just have an pipe FD with QEMU, which we just > > incrementally read image data from, and forward to the client app via > > a libvirt stream object. > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 13:14 ` Alon Levy @ 2012-03-14 13:17 ` Daniel P. Berrange 2012-03-14 13:18 ` Luiz Capitulino 1 sibling, 0 replies; 35+ messages in thread From: Daniel P. Berrange @ 2012-03-14 13:17 UTC (permalink / raw) To: Luiz Capitulino, aliguori, Gerd Hoffmann, qemu-devel On Wed, Mar 14, 2012 at 03:14:10PM +0200, Alon Levy wrote: > On Wed, Mar 14, 2012 at 09:32:58AM -0300, Luiz Capitulino wrote: > > On Wed, 14 Mar 2012 08:37:13 +0000 > > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > > > > First, with this applied you will not know the size in advance. Also > > > > one of the approaches discussed is to allow passing in a file handle. > > > > That is a possible way to handle async screendumps too: just write to > > > > the passed file handle and close it when done. Obvious drawback is that > > > > this will not cover the classic way of specifying the output filename as > > > > argument. > > > > As Daniel explains below, this is not a drawback and there's no problem > > supporting multiple ways of returning the image. > > > > The real drawback of making this w/o async support is that you can't detect > > errors. > > > > You also can't detect when the writing is done (unless you continuously > try to parse the file yourself..) That's one of the nice benefits of using a pipe - you'll just get EOF when reading from it, when QEMU is done. > > > > This would not be a problem from libvirt's POV - we don't really want a > > > file on disk anyway, nor do we want to pull the whole image into memory. > > > Our ideal approach is to just have an pipe FD with QEMU, which we just > > > incrementally read image data from, and forward to the client app via > > > a libvirt stream object. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 13:14 ` Alon Levy 2012-03-14 13:17 ` Daniel P. Berrange @ 2012-03-14 13:18 ` Luiz Capitulino 2012-03-14 13:43 ` Alon Levy 1 sibling, 1 reply; 35+ messages in thread From: Luiz Capitulino @ 2012-03-14 13:18 UTC (permalink / raw) To: Alon Levy; +Cc: aliguori, Gerd Hoffmann, qemu-devel On Wed, 14 Mar 2012 15:14:10 +0200 Alon Levy <alevy@redhat.com> wrote: > On Wed, Mar 14, 2012 at 09:32:58AM -0300, Luiz Capitulino wrote: > > On Wed, 14 Mar 2012 08:37:13 +0000 > > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > > > > First, with this applied you will not know the size in advance. Also > > > > one of the approaches discussed is to allow passing in a file handle. > > > > That is a possible way to handle async screendumps too: just write to > > > > the passed file handle and close it when done. Obvious drawback is that > > > > this will not cover the classic way of specifying the output filename as > > > > argument. > > > > As Daniel explains below, this is not a drawback and there's no problem > > supporting multiple ways of returning the image. > > > > The real drawback of making this w/o async support is that you can't detect > > errors. > > > > You also can't detect when the writing is done (unless you continuously > try to parse the file yourself..) Oh, really? For some reason I had a pipe in my mind, but I honestly don't what's the semantics of fd passing in that regard. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump 2012-03-14 13:18 ` Luiz Capitulino @ 2012-03-14 13:43 ` Alon Levy 0 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-14 13:43 UTC (permalink / raw) To: Luiz Capitulino; +Cc: aliguori, Gerd Hoffmann, qemu-devel On Wed, Mar 14, 2012 at 10:18:12AM -0300, Luiz Capitulino wrote: > On Wed, 14 Mar 2012 15:14:10 +0200 > Alon Levy <alevy@redhat.com> wrote: > > > On Wed, Mar 14, 2012 at 09:32:58AM -0300, Luiz Capitulino wrote: > > > On Wed, 14 Mar 2012 08:37:13 +0000 > > > "Daniel P. Berrange" <berrange@redhat.com> wrote: > > > > > > > > First, with this applied you will not know the size in advance. Also > > > > > one of the approaches discussed is to allow passing in a file handle. > > > > > That is a possible way to handle async screendumps too: just write to > > > > > the passed file handle and close it when done. Obvious drawback is that > > > > > this will not cover the classic way of specifying the output filename as > > > > > argument. > > > > > > As Daniel explains below, this is not a drawback and there's no problem > > > supporting multiple ways of returning the image. > > > > > > The real drawback of making this w/o async support is that you can't detect > > > errors. > > > > > > > You also can't detect when the writing is done (unless you continuously > > try to parse the file yourself..) > > Oh, really? For some reason I had a pipe in my mind, but I honestly don't > what's the semantics of fd passing in that regard. Like Daniel pointed out, I am dead wrong here, sorry for the noise. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] qxl: screendump: use provided Monitor 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy ` (3 preceding siblings ...) 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy @ 2012-03-11 19:26 ` Alon Levy 4 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:26 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori This fixes the hmp loose end by suspending the monitor and resuming it after ppm_save has been called. For qmp this is redundant, and actually wrong, since a qmp command ends up suspending the hmp monitor and then resuming it. On the other hand I'm not sure how much of a problem this is. The real problem is that qmp users still end up with a completed "screendump" before ppm_save has completed. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/qxl-render.c | 12 +++++++++--- hw/qxl.c | 4 ++-- hw/qxl.h | 2 +- ui/spice-display.h | 1 + 4 files changed, 13 insertions(+), 6 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index b281766..16340d0 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -154,7 +154,8 @@ typedef struct QXLPPMSaveBHData { static void qxl_render_ppm_save_bh(void *opaque); -static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename) +static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename, + Monitor *mon) { QXLPPMSaveBHData *ppm_save_bh_data; QEMUBH *ppm_save_bh; @@ -169,6 +170,8 @@ static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename) ppm_save_bh = qemu_bh_new(qxl_render_ppm_save_bh, ppm_save_bh_data); cookie->u.render.filename = g_strdup(filename); cookie->u.render.ppm_save_bh = ppm_save_bh; + cookie->u.render.mon = mon; + monitor_suspend(mon); } return cookie; } @@ -176,6 +179,9 @@ static QXLCookie *qxl_cookie_render_new(PCIQXLDevice *qxl, const char *filename) static void qxl_cookie_render_free(PCIQXLDevice *qxl, QXLCookie *cookie) { g_free(cookie->u.render.filename); + if (cookie->u.render.mon) { + monitor_resume(cookie->u.render.mon); + } g_free(cookie); --qxl->render_update_cookie_num; } @@ -204,7 +210,7 @@ static void qxl_render_ppm_save_bh(void *opaque) * callbacks are called by spice_server thread, defering to bh called from the * io thread. */ -void qxl_render_update(PCIQXLDevice *qxl, const char *filename) +void qxl_render_update(PCIQXLDevice *qxl, const char *filename, Monitor *mon) { QXLCookie *cookie; @@ -223,7 +229,7 @@ void qxl_render_update(PCIQXLDevice *qxl, const char *filename) qxl->guest_primary.commands = 0; qxl->render_update_cookie_num++; qemu_mutex_unlock(&qxl->ssd.lock); - cookie = qxl_cookie_render_new(qxl, filename); + cookie = qxl_cookie_render_new(qxl, filename, mon); qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL, 0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie); } diff --git a/hw/qxl.c b/hw/qxl.c index d21b508..fae5be8 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -1471,7 +1471,7 @@ static void qxl_hw_update(void *opaque) break; case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl, NULL); + qxl_render_update(qxl, NULL, NULL); break; default: break; @@ -1495,7 +1495,7 @@ static void qxl_hw_screen_dump(void *opaque, const char *filename, bool cswitch, switch (qxl->mode) { case QXL_MODE_COMPAT: case QXL_MODE_NATIVE: - qxl_render_update(qxl, filename); + qxl_render_update(qxl, filename, mon); break; case QXL_MODE_VGA: vga->screen_dump(vga, filename, cswitch, mon); diff --git a/hw/qxl.h b/hw/qxl.h index 417ab28..219e149 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -147,7 +147,7 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext); /* qxl-render.c */ void qxl_render_resize(PCIQXLDevice *qxl); -void qxl_render_update(PCIQXLDevice *qxl, const char *filename); +void qxl_render_update(PCIQXLDevice *qxl, const char *filename, Monitor *mon); void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext); void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie); void qxl_render_update_area_bh(void *opaque); diff --git a/ui/spice-display.h b/ui/spice-display.h index ec1fc24..2d01f51 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -64,6 +64,7 @@ typedef struct QXLCookie { int redraw; char *filename; QEMUBH *ppm_save_bh; + Monitor *mon; } render; } u; } QXLCookie; -- 1.7.9.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy ` (4 preceding siblings ...) 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy @ 2012-03-11 19:33 ` Alon Levy 5 siblings, 0 replies; 35+ messages in thread From: Alon Levy @ 2012-03-11 19:33 UTC (permalink / raw) To: qemu-devel, kraxel, lcapitulino, aliguori On Sun, Mar 11, 2012 at 06:39:33PM +0200, Alon Levy wrote: > This patchset starts and ends with trace event additions that make it easier to see the change. Self NACK to v1, see v2 on the same thread. > > It applies on top of http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg01784.html > due to trace-events. > > The problem addressed by this patchset is that after recent fixes (81fb6f) > screendump with a qxl device in native mode saves a stale screen dump. > > The solution is to use monitor_suspend and monitor_release in qxl's implementation. > > This is done by: > 1. introducing an extra parameter to vga_hw_screen_dump & hw_vga_dump > console: pass Monitor to vga_hw_screen_dump/hw_vga_dump > 2. using it in qxl via a bh. > qxl-render: call ppm_save on bh > > Additional patches add trace events to qxl and qxl_render, making it easy to see the difference: > > events setup: (using stderr backend) > > (qemu) trace-event ppm_save on > (qemu) trace-event qxl* on > (qemu) trace-event qxl_interface_get_command_enter off > (qemu) trace-event qxl_interface_release_resource off > (qemu) trace-event qxl_interface_get_command_ret off > > before: ppm_save done before update: > > (qemu) screendump /tmp/a.ppm > ppm_save /tmp/a.ppm surface=0x7fc0267b3ad0 > qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1 > qxl_interface_update_area_complete_schedule_bh #dirty=1 > qxl_render_update_area_done 0x7fc02b603db0 > (qemu) qxl_blit stride=-2560 [152, 160, 464, 480] > > after: > > (qemu) screendump /tmp/a.ppm > qxl_interface_update_area_complete surface=0 [152,160,464,480] #=1 > qxl_interface_update_area_complete_schedule_bh #dirty=1 > qxl_render_update_area_done 0x7f407af72210 > qxl_render_ppm_save_bh 0x7f407f845b60 (primary 0x7f401bc00000) > qxl_blit stride=-2560 [152, 160, 464, 480] > ppm_save /tmp/a.ppm surface=0x7f4077204ad0 > (qemu) > > Note: This doesn't address a possible libvirt problem that was mentioned in > length before, but since it has not been reproduced it will be fixed when it > is. Meanwhile other users like autotest will be fixed by this patch (by fix I > mean screendump will produce the correct output). > > Alon Levy (4): > qxl: switch qxl.c to trace-events > qxl/qxl_render.c: add trace events > console: pass Monitor to vga_hw_screen_dump/hw_vga_dump > qxl-render: call ppm_save on bh > > console.c | 4 +- > console.h | 5 +- > hw/blizzard.c | 2 +- > hw/g364fb.c | 3 +- > hw/omap_dss.c | 4 +- > hw/omap_lcdc.c | 3 +- > hw/qxl-render.c | 95 +++++++++++++++++++++++++++------ > hw/qxl.c | 150 ++++++++++++++++++++++++--------------------------- > hw/qxl.h | 2 +- > hw/sm501.c | 4 +- > hw/tcx.c | 12 +++-- > hw/vga.c | 6 ++- > hw/vmware_vga.c | 5 +- > monitor.c | 2 +- > trace-events | 55 +++++++++++++++++++ > ui/spice-display.h | 3 + > 16 files changed, 240 insertions(+), 115 deletions(-) > > -- > 1.7.9.1 > > ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2012-03-14 13:43 UTC | newest] Thread overview: 35+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-11 16:39 [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 1/4] qxl: switch qxl.c to trace-events Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 2/4] qxl/qxl_render.c: add trace events Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 3/4] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy 2012-03-11 16:39 ` [Qemu-devel] [PATCH 4/4] qxl-render: call ppm_save on bh Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 0/5] fix qxl screendump using monitor_suspend Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events Alon Levy 2012-03-12 10:20 ` Gerd Hoffmann 2012-03-12 11:43 ` Alon Levy 2012-03-12 15:50 ` Alon Levy 2012-03-13 6:42 ` Gerd Hoffmann 2012-03-13 9:35 ` Alon Levy 2012-03-13 9:47 ` Gerd Hoffmann 2012-03-13 10:18 ` Alon Levy 2012-03-13 10:26 ` Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 2/5] qxl/qxl_render.c: add trace events Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 3/5] qxl-render: call ppm_save on bh Alon Levy 2012-03-13 13:22 ` Luiz Capitulino 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 4/5] console: pass Monitor to vga_hw_screen_dump/hw_vga_dump Alon Levy 2012-03-13 13:35 ` Luiz Capitulino 2012-03-13 14:46 ` Alon Levy 2012-03-13 15:59 ` Luiz Capitulino 2012-03-13 17:35 ` Alon Levy 2012-03-13 18:07 ` Luiz Capitulino 2012-03-14 8:25 ` Gerd Hoffmann 2012-03-14 8:32 ` Alon Levy 2012-03-14 8:14 ` Gerd Hoffmann 2012-03-14 8:37 ` Daniel P. Berrange 2012-03-14 12:32 ` Luiz Capitulino 2012-03-14 13:14 ` Alon Levy 2012-03-14 13:17 ` Daniel P. Berrange 2012-03-14 13:18 ` Luiz Capitulino 2012-03-14 13:43 ` Alon Levy 2012-03-11 19:26 ` [Qemu-devel] [PATCH v2 5/5] qxl: screendump: use provided Monitor Alon Levy 2012-03-11 19:33 ` [Qemu-devel] [PATCH 0/4] fix qxl screendump using monitor_suspend Alon Levy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).