From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=56249 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pzt9M-0003xr-JW for qemu-devel@nongnu.org; Wed, 16 Mar 2011 12:00:42 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pzt9J-0006tn-Nc for qemu-devel@nongnu.org; Wed, 16 Mar 2011 12:00:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:1025) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pzt9J-0006tN-EQ for qemu-devel@nongnu.org; Wed, 16 Mar 2011 12:00:37 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p2GG0VO9019355 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 16 Mar 2011 12:00:31 -0400 Message-ID: <4D80DF1D.9080806@redhat.com> Date: Wed, 16 Mar 2011 17:02:37 +0100 From: Hans de Goede MIME-Version: 1.0 References: <1300290769-31155-1-git-send-email-alevy@redhat.com> <1300290769-31155-5-git-send-email-alevy@redhat.com> In-Reply-To: <1300290769-31155-5-git-send-email-alevy@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v3 4/4] hw/qxl-render: drop cursor locks, replace with pipe List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: uril@redhat.com, qemu-devel@nongnu.org, gleb@redhat.com Looks good now, ack: Acked-by: Hans de Goede On 03/16/2011 04:52 PM, Alon Levy wrote: > Switching locking protection of ds->cursor_set/cursor_move to moving > every call to these functions into the iothread and using the ssd->pipe > to transfer that, adding QXL_SERVER_CURSOR_SET, QXL_SERVER_CURSOR_MOVE. > > This is tested with both -vnc :0 -spice and -sdl -spice. > --- > hw/qxl-render.c | 25 +++++++++----- > hw/qxl.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/qxl.h | 4 ++ > ui/spice-display.h | 13 +++++++- > 4 files changed, 122 insertions(+), 10 deletions(-) > > diff --git a/hw/qxl-render.c b/hw/qxl-render.c > index 58965e0..6759edb 100644 > --- a/hw/qxl-render.c > +++ b/hw/qxl-render.c > @@ -178,7 +178,6 @@ fail: > return NULL; > } > > - > /* called from spice server thread context only */ > void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext) > { > @@ -209,18 +208,26 @@ void qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext) > if (c == NULL) { > c = cursor_builtin_left_ptr(); > } > - qemu_mutex_lock_iothread(); > - qxl->ssd.ds->cursor_define(c); > - qxl->ssd.ds->mouse_set(x, y, 1); > - qemu_mutex_unlock_iothread(); > - cursor_put(c); > + qxl_server_request_cursor_set(qxl, c, x, y); > break; > case QXL_CURSOR_MOVE: > x = cmd->u.position.x; > y = cmd->u.position.y; > - qemu_mutex_lock_iothread(); > - qxl->ssd.ds->mouse_set(x, y, 1); > - qemu_mutex_unlock_iothread(); > + qxl_server_request_cursor_move(qxl, x, y); > break; > } > } > + > +/* called from iothread only (via qxl.c:pipe_read) */ > +void qxl_render_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y) > +{ > + ssd->ds->cursor_define(c); > + ssd->ds->mouse_set(x, y, 1); > + cursor_put(c); > +} > + > +/* called from iothread only (via qxl.c:pipe_read) */ > +void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y) > +{ > + ssd->ds->mouse_set(x, y, 1); > +} > diff --git a/hw/qxl.c b/hw/qxl.c > index cf3c938..4c27deb 100644 > --- a/hw/qxl.c > +++ b/hw/qxl.c > @@ -117,6 +117,27 @@ static QXLMode qxl_modes[] = { > #endif > }; > > +typedef struct __attribute__ ((__packed__)) { > + QEMUCursor *c; > + int x; > + int y; > +} QXLServerCursorSet; > + > +typedef struct __attribute__ ((__packed__)) { > + int x; > + int y; > +} QXLServerCursorMove; > + > +typedef struct __attribute__ ((__packed__)) { > + unsigned char req; > + QXLServerCursorMove data; > +} QXLServerCursorMoveRequest; > + > +typedef struct __attribute__ ((__packed__)) { > + unsigned char req; > + QXLServerCursorSet data; > +} QXLServerCursorSetRequest; > + > static PCIQXLDevice *qxl0; > > static void qxl_send_events(PCIQXLDevice *d, uint32_t events); > @@ -337,6 +358,33 @@ static void interface_get_init_info(QXLInstance *sin, QXLDevInitInfo *info) > } > > /* called from spice server thread context only */ > +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y) > +{ > + QXLServerCursorSetRequest req; > + int r; > + > + req.req = QXL_SERVER_CURSOR_SET; > + req.data.c = c; > + req.data.x = x; > + req.data.y = y; > + r = write(qxl->ssd.pipe[1],&req, sizeof(req)); > + assert(r == sizeof(req)); > +} > + > +/* called from spice server thread context only */ > +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y) > +{ > + QXLServerCursorMoveRequest req; > + int r; > + > + req.req = QXL_SERVER_CURSOR_MOVE; > + req.data.x = x; > + req.data.y = y; > + r = write(qxl->ssd.pipe[1],&req, sizeof(req)); > + assert(r == sizeof(req)); > +} > + > +/* called from spice server thread context only */ > static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext) > { > PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl); > @@ -1055,12 +1103,37 @@ static void qxl_map(PCIDevice *pci, int region_num, > } > } > > +static void read_bytes(int fd, void *buf, int len_requested) > +{ > + int len; > + int total_len = 0; > + > + do { > + len = read(fd, buf, len_requested - total_len); > + if (len< 0) { > + if (errno == EINTR || errno == EAGAIN) { > + continue; > + } > + perror("qxl: pipe_read: read failed"); > + /* will abort once it's out of the while loop */ > + break; > + } > + total_len += len; > + buf = (uint8_t *)buf + len; > + } while (total_len< len_requested); > + assert(total_len == len_requested); > +} > + > static void pipe_read(void *opaque) > { > SimpleSpiceDisplay *ssd = opaque; > unsigned char cmd; > int len, set_irq = 0; > int create_update = 0; > + int cursor_set = 0; > + int cursor_move = 0; > + QXLServerCursorSet cursor_set_data; > + QXLServerCursorMove cursor_move_data; > > while (1) { > cmd = 0; > @@ -1082,6 +1155,17 @@ static void pipe_read(void *opaque) > case QXL_SERVER_CREATE_UPDATE: > create_update = 1; > break; > + case QXL_SERVER_CURSOR_SET: > + if (cursor_set == 1) { > + cursor_put(cursor_set_data.c); > + } > + cursor_set = 1; > + read_bytes(ssd->pipe[0],&cursor_set_data, sizeof(cursor_set_data)); > + break; > + case QXL_SERVER_CURSOR_MOVE: > + cursor_move = 1; > + read_bytes(ssd->pipe[0],&cursor_move_data, sizeof(cursor_move_data)); > + break; > default: > fprintf(stderr, "%s: unknown cmd %u\n", __FUNCTION__, cmd); > abort(); > @@ -1099,6 +1183,12 @@ static void pipe_read(void *opaque) > if (set_irq) { > qxl_set_irq(container_of(ssd, PCIQXLDevice, ssd)); > } > + if (cursor_move) { > + qxl_render_cursor_move(ssd, cursor_move_data.x, cursor_move_data.y); > + } > + if (cursor_set) { > + qxl_render_cursor_set(ssd, cursor_set_data.c, cursor_set_data.x, cursor_set_data.y); > + } > } > > static void qxl_send_events(PCIQXLDevice *d, uint32_t events) > diff --git a/hw/qxl.h b/hw/qxl.h > index 701245f..f4f99ec 100644 > --- a/hw/qxl.h > +++ b/hw/qxl.h > @@ -93,6 +93,8 @@ typedef struct PCIQXLDevice { > > /* qxl.c */ > void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id); > +void qxl_server_request_cursor_set(PCIQXLDevice *qxl, QEMUCursor *c, int x, int y); > +void qxl_server_request_cursor_move(PCIQXLDevice *qxl, int x, int y); > > /* qxl-logger.c */ > void qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id); > @@ -102,3 +104,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_cursor_set(SimpleSpiceDisplay *ssd, QEMUCursor *c, int x, int y); > +void qxl_render_cursor_move(SimpleSpiceDisplay *ssd, int x, int y); > diff --git a/ui/spice-display.h b/ui/spice-display.h > index 2be6a34..bbfd689 100644 > --- a/ui/spice-display.h > +++ b/ui/spice-display.h > @@ -31,11 +31,22 @@ > > #define NUM_SURFACES 1024 > > -/* For commands/requests from server thread to iothread */ > +/* > + * Commands/requests from server thread to iothread. > + * Note that CREATE_UPDATE is used both with qxl and without it (spice-display) > + * the others are only used with the qxl device. > + * > + * SET_IRQ - just the request is sent (1 byte) > + * CREATE_UPDATE - jus the request is sent (1 byte) > + * CURSOR_SET - send QXLServerRequestCursorSet > + * CURSOR_MOVE - send QXLServerRequestCursorMove > + */ > #define QXL_EMPTY_UPDATE ((void *)-1) > enum { > QXL_SERVER_SET_IRQ = 1, > QXL_SERVER_CREATE_UPDATE, > + QXL_SERVER_CURSOR_SET, > + QXL_SERVER_CURSOR_MOVE > }; > > struct SimpleSpiceUpdate;