From patchwork Tue Feb 21 21:39:36 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alon Levy X-Patchwork-Id: 142374 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 757F7B6EF3 for ; Wed, 22 Feb 2012 09:22:59 +1100 (EST) Received: from localhost ([::1]:46764 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rzy6r-0002Up-QE for incoming@patchwork.ozlabs.org; Tue, 21 Feb 2012 17:22:57 -0500 Received: from eggs.gnu.org ([140.186.70.92]:59815) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RzxRa-0000gN-OS for qemu-devel@nongnu.org; Tue, 21 Feb 2012 16:40:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RzxRT-0005om-Ng for qemu-devel@nongnu.org; Tue, 21 Feb 2012 16:40:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:30355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RzxRT-0005oU-Fj for qemu-devel@nongnu.org; Tue, 21 Feb 2012 16:40:11 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q1LLe8XZ004912 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 21 Feb 2012 16:40:09 -0500 Received: from garlic.redhat.com (vpn-200-36.tlv.redhat.com [10.35.200.36]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q1LLdc6A001985; Tue, 21 Feb 2012 16:40:04 -0500 From: Alon Levy To: qemu-devel@nongnu.org, kraxel@redhat.com, spice-devel@freedesktop.org, yhalperi@redhat.com, elmarco@redhat.com Date: Tue, 21 Feb 2012 23:39:36 +0200 Message-Id: <1329860377-6284-9-git-send-email-alevy@redhat.com> In-Reply-To: <1329860377-6284-1-git-send-email-alevy@redhat.com> References: <1329860377-6284-1-git-send-email-alevy@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [RFC v4 8/9] qxl: make qxl_render_update async X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org RHBZ# 747011 Removes the last user of QXL_SYNC when using update drivers that use the _ASYNC io ports. The last user is qxl_render_update, it is called both by qxl_hw_update which is the vga_hw_update_ptr passed to graphic_console_init, and by qxl_hw_screen_dump. At the same time the QXLRect area being passed to the red_worker thread is passed as a copy, as part of the QXLCookie. The implementation uses a bh to make sure dpy_update and qxl_flip are called from the io thread, otherwise the vga->ds->surface.data can change under our feet. With this patch sdl+spice works fine. But spice by itself doesn't produce the expected screendumps unless repeated a few times, due to ppm_save being called before update_area (rendering done in spice server thread) having a chance to complete. Fixed by next patch, but see commit message for problem introduced by it. Signed-off-by: Alon Levy --- hw/qxl-render.c | 140 +++++++++++++++++++++++++++++++++++++++------------ hw/qxl.c | 80 +++++++++++++++++++++++++++-- hw/qxl.h | 12 +++++ ui/spice-display.h | 6 ++ 4 files changed, 199 insertions(+), 39 deletions(-) diff --git a/hw/qxl-render.c b/hw/qxl-render.c index 0127856..23e6929 100644 --- a/hw/qxl-render.c +++ b/hw/qxl-render.c @@ -31,6 +31,8 @@ static void qxl_flip(PCIQXLDevice *qxl, QXLRect *rect) dprint(qxl, 1, "%s: initializing guest_primary.data\n", __func__); qxl->guest_primary.data = memory_region_get_ram_ptr(&qxl->vga.vram); } + dprint(qxl, 1, "%s: %d, %d, %d, %d\n", __func__, + rect->left, rect->right, rect->top, rect->bottom); src = qxl->guest_primary.data; src += (qxl->guest_primary.surface.height - rect->top - 1) * qxl->guest_primary.abs_stride; @@ -76,13 +78,78 @@ void qxl_render_resize(PCIQXLDevice *qxl) } } -void qxl_render_update(PCIQXLDevice *qxl) +static void qxl_render_clear_update_redraw_unlocked(PCIQXLDevice *qxl) +{ + qxl->render_update_redraw = 0; + memset(&qxl->render_update_redraw_area, 0, + sizeof(qxl->render_update_redraw_area)); + qxl->num_dirty_rects = 0; +} + +static void qxl_displaysurface_resize(PCIQXLDevice *qxl) { VGACommonState *vga = &qxl->vga; - QXLRect dirty[32], update; - int i, redraw = 0; + + 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); + qxl->render_update_redraw = 1; + qxl->render_update_redraw_area.left = 0; + qxl->render_update_redraw_area.right = + qxl->guest_primary.surface.width; + qxl->render_update_redraw_area.top = 0; + qxl->render_update_redraw_area.bottom = + qxl->guest_primary.surface.height; +} + +static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl) +{ + VGACommonState *vga = &qxl->vga; + int i; DisplaySurface *surface = vga->ds->surface; + if (surface->width != qxl->guest_primary.surface.width || + surface->height != qxl->guest_primary.surface.height) { + qxl_displaysurface_resize(qxl); + } + if (qxl->render_update_redraw) { + qxl->dirty[0] = qxl->render_update_redraw_area; + qxl->num_dirty_rects = 1; + qxl_render_clear_update_redraw_unlocked(qxl); + dprint(qxl, 1, "%s: redrawing %d,%d,%d,%d\n", __func__, + qxl->dirty[0].left, + qxl->dirty[0].right, + qxl->dirty[0].top, + qxl->dirty[0].bottom); + } + for (i = 0; i < qxl->num_dirty_rects; i++) { + if (qemu_spice_rect_is_empty(qxl->dirty+i)) { + break; + } + qxl_flip(qxl, qxl->dirty+i); + dpy_update(vga->ds, + qxl->dirty[i].left, qxl->dirty[i].top, + qxl->dirty[i].right - qxl->dirty[i].left, + qxl->dirty[i].bottom - qxl->dirty[i].top); + } + qxl->num_dirty_rects = 0; +} + +/* + * 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) +{ + int redraw = 0; + QXLCookie *cookie; + + qemu_mutex_lock(&qxl->ssd.lock); + if (qxl->guest_primary.resized) { qxl->guest_primary.resized = 0; @@ -94,43 +161,50 @@ void qxl_render_update(PCIQXLDevice *qxl) qxl->guest_primary.qxl_stride, qxl->guest_primary.bytes_pp, qxl->guest_primary.bits_pp); + qxl_displaysurface_resize(qxl); } if (!qxl->guest_primary.commands) { + qemu_mutex_unlock(&qxl->ssd.lock); return; } qxl->guest_primary.commands = 0; + cookie = qxl_cookie_new(QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, + 0); + qxl->render_update_redraw = MAX(qxl->render_update_redraw, redraw); + cookie->u.render.area.left = 0; + cookie->u.render.area.right = qxl->guest_primary.surface.width; + cookie->u.render.area.top = 0; + cookie->u.render.area.bottom = qxl->guest_primary.surface.height; + qxl->render_update_redraw_area = cookie->u.render.area; + qxl->render_update_cookie_num++; + qemu_mutex_unlock(&qxl->ssd.lock); + qxl_spice_update_area(qxl, 0, &cookie->u.render.area, NULL, + 0, 1 /* clear_dirty_region */, QXL_ASYNC, cookie); +} - update.left = 0; - update.right = qxl->guest_primary.surface.width; - update.top = 0; - update.bottom = qxl->guest_primary.surface.height; - - memset(dirty, 0, sizeof(dirty)); - qxl_spice_update_area(qxl, 0, &update, - dirty, ARRAY_SIZE(dirty), 1, QXL_SYNC, NULL); - if (redraw) { - memset(dirty, 0, sizeof(dirty)); - dirty[0] = update; - } - if (surface->width != qxl->guest_primary.surface.width || - surface->height != qxl->guest_primary.surface.height) { - 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); - } - for (i = 0; i < ARRAY_SIZE(dirty); i++) { - if (qemu_spice_rect_is_empty(dirty+i)) { - break; - } - qxl_flip(qxl, dirty+i); - dpy_update(vga->ds, - dirty[i].left, dirty[i].top, - dirty[i].right - dirty[i].left, - dirty[i].bottom - dirty[i].top); - } +/* + * Called from main thread, via bh + * + * Since it doesn't involve a cookie there is no way to match it to a specific + * qxl_render_update operation (there may be multiple). So ignore it if there + * are none pending (render_update_cookie_num == 0). + */ +void qxl_render_update_area_bh(void *opaque) +{ + PCIQXLDevice *qxl = opaque; + + qemu_mutex_lock(&qxl->ssd.lock); + qxl_render_update_area_unlocked(qxl); + qemu_mutex_unlock(&qxl->ssd.lock); +} + +void qxl_render_update_area_done(PCIQXLDevice *qxl, QXLCookie *cookie) +{ + qemu_mutex_lock(&qxl->ssd.lock); + qxl->render_update_cookie_num--; + 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 6f94edb..b4f84f2 100644 --- a/hw/qxl.c +++ b/hw/qxl.c @@ -180,7 +180,7 @@ static void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id, static void qxl_spice_flush_surfaces_async(PCIQXLDevice *qxl) { - spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, + spice_qxl_flush_surfaces_async(&qxl->ssd.qxl, (uint64_t)qxl_cookie_new(QXL_COOKIE_TYPE_IO, QXL_IO_FLUSH_SURFACES_ASYNC)); } @@ -746,6 +746,11 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie) } switch (current_async) { + case QXL_IO_MEMSLOT_ADD_ASYNC: + case QXL_IO_DESTROY_PRIMARY_ASYNC: + case QXL_IO_UPDATE_AREA_ASYNC: + case QXL_IO_FLUSH_SURFACES_ASYNC: + break; case QXL_IO_CREATE_PRIMARY_ASYNC: qxl_create_guest_primary_complete(qxl); break; @@ -755,11 +760,63 @@ static void interface_async_complete_io(PCIQXLDevice *qxl, QXLCookie *cookie) case QXL_IO_DESTROY_SURFACE_ASYNC: qxl_spice_destroy_surface_wait_complete(qxl, cookie->u.surface_id); break; + default: + fprintf(stderr, "qxl: %s: unexpected current_async %d\n", __func__, + current_async); } qxl_send_events(qxl, QXL_INTERRUPT_IO_CMD); } /* called from spice server thread context only */ +static void interface_update_area_complete(QXLInstance *sin, + uint32_t surface_id, + QXLRect *dirty, uint32_t num_updated_rects) +{ + PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); + int i; + int qxl_i; + int num_merged; + int num_unmerged; + + if (surface_id != 0 || !qxl->render_update_cookie_num) { + return; + } + qemu_mutex_lock(&qxl->ssd.lock); + if (qxl->render_update_redraw) { + /* don't bother copying them over since we will ignore them */ + qxl->num_dirty_rects += num_updated_rects; + dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", + __func__, qxl->num_dirty_rects); + qemu_bh_schedule(qxl->update_area_bh); + qemu_mutex_unlock(&qxl->ssd.lock); + return; + } + if (qxl->num_dirty_rects + num_updated_rects > QXL_NUM_DIRTY_RECTS) { + /* + * overflow - merge all remaining rects. Hoping this is not + * common so doesn't need to be optimized + */ + } + num_merged = MAX(0, + (int)qxl->num_dirty_rects + (int)num_updated_rects + - QXL_NUM_DIRTY_RECTS); + num_unmerged = num_updated_rects - num_merged; + qxl_i = qxl->num_dirty_rects; + for (i = 0; i < num_unmerged; i++) { + qxl->dirty[qxl_i++] = dirty[i]; + } + qxl->num_dirty_rects += num_unmerged; + dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n", + __func__, qxl->num_dirty_rects); + for (i = 0; i < num_merged; i++) { + qemu_spice_rect_union(&qxl->dirty[QXL_NUM_DIRTY_RECTS - 1], + &dirty[i + num_unmerged]); + } + qemu_bh_schedule(qxl->update_area_bh); + qemu_mutex_unlock(&qxl->ssd.lock); +} + +/* called from spice server thread context only */ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token) { PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); @@ -768,11 +825,15 @@ static void interface_async_complete(QXLInstance *sin, uint64_t cookie_token) switch (cookie->type) { case QXL_COOKIE_TYPE_IO: interface_async_complete_io(qxl, cookie); + g_free(cookie); + break; + case QXL_COOKIE_TYPE_RENDER_UPDATE_AREA: + qxl_render_update_area_done(qxl, cookie); break; default: fprintf(stderr, "qxl: %s: unexpected cookie type %d\n", __func__, cookie->type); + g_free(cookie); } - g_free(cookie); } static const QXLInterface qxl_interface = { @@ -795,6 +856,7 @@ static const QXLInterface qxl_interface = { .notify_update = interface_notify_update, .flush_resources = interface_flush_resources, .async_complete = interface_async_complete, + .update_area_complete = interface_update_area_complete, }; static void qxl_enter_vga_mode(PCIQXLDevice *d) @@ -1211,11 +1273,15 @@ async_common: switch (io_port) { case QXL_IO_UPDATE_AREA: { - QXLRect update = d->ram->update_area; + QXLCookie *cookie = NULL; + + if (async == QXL_ASYNC) { + cookie = qxl_cookie_new(QXL_COOKIE_TYPE_IO, + QXL_IO_UPDATE_AREA_ASYNC); + cookie->u.area = d->ram->update_area; + } qxl_spice_update_area(d, d->ram->update_surface, - &update, NULL, 0, 0, async, - qxl_cookie_new(QXL_COOKIE_TYPE_IO, - QXL_IO_UPDATE_AREA_ASYNC)); + &cookie->u.area, NULL, 0, 0, async, cookie); break; } case QXL_IO_NOTIFY_CMD: @@ -1596,6 +1662,8 @@ static int qxl_init_common(PCIQXLDevice *qxl) init_pipe_signaling(qxl); qxl_reset_state(qxl); + qxl->update_area_bh = qemu_bh_new(qxl_render_update_area_bh, qxl); + return 0; } diff --git a/hw/qxl.h b/hw/qxl.h index ed297cc..57a94ca 100644 --- a/hw/qxl.h +++ b/hw/qxl.h @@ -18,6 +18,8 @@ enum qxl_mode { #define QXL_UNDEFINED_IO UINT32_MAX +#define QXL_NUM_DIRTY_RECTS 64 + typedef struct PCIQXLDevice { PCIDevice pci; SimpleSpiceDisplay ssd; @@ -89,6 +91,14 @@ typedef struct PCIQXLDevice { /* io bar */ MemoryRegion io_bar; + + /* qxl_render_update state */ + int render_update_cookie_num; + int render_update_redraw; + QXLRect render_update_redraw_area; + int num_dirty_rects; + QXLRect dirty[QXL_NUM_DIRTY_RECTS]; + QEMUBH *update_area_bh; } PCIQXLDevice; #define PANIC_ON(x) if ((x)) { \ @@ -130,3 +140,5 @@ void qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext); void qxl_render_resize(PCIQXLDevice *qxl); void qxl_render_update(PCIQXLDevice *qxl); 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 93ac78d..81fa1e4 100644 --- a/ui/spice-display.h +++ b/ui/spice-display.h @@ -50,6 +50,7 @@ typedef enum qxl_async_io { enum { QXL_COOKIE_TYPE_IO, + QXL_COOKIE_TYPE_RENDER_UPDATE_AREA, }; typedef struct QXLCookie { @@ -57,6 +58,11 @@ typedef struct QXLCookie { uint64_t io; union { uint32_t surface_id; + QXLRect area; + struct { + QXLRect area; + int redraw; + } render; } u; } QXLCookie;