From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=44697 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OKoLf-00041x-BO for qemu-devel@nongnu.org; Sat, 05 Jun 2010 04:03:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OKoLc-0003kX-0Y for qemu-devel@nongnu.org; Sat, 05 Jun 2010 04:03:19 -0400 Received: from mail-bw0-f45.google.com ([209.85.214.45]:47010) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OKoLb-0003kD-Cd for qemu-devel@nongnu.org; Sat, 05 Jun 2010 04:03:15 -0400 Received: by bwz4 with SMTP id 4so594588bwz.4 for ; Sat, 05 Jun 2010 01:03:14 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1275657620-26226-1-git-send-email-corentincj@iksaif.net> <1275657620-26226-3-git-send-email-corentincj@iksaif.net> Date: Sat, 5 Jun 2010 10:03:14 +0200 Message-ID: From: Corentin Chary Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH v2 2/2] vnc: threaded VNC server List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Anthony Liguori , Qemu-development List , Gautham R Shenoy On Fri, Jun 4, 2010 at 3:44 PM, Alexander Graf wrote: > > On 04.06.2010, at 15:20, Corentin Chary wrote: > >> Implement a threaded VNC server using the producer-consumer model. >> The main thread will push encoding jobs (a list a rectangles to update) >> in a queue, and the VNC worker thread will consume that queue and send >> framebuffer updates to the output buffer. >> >> The threaded VNC server can be enabled with ./configure --enable-vnc-thr= ead. >> >> If you don't want it, just use ./configure --disable-vnc-thread and a sy= ncrhonous >> queue of job will be used (which as exactly the same behavior as the old= queue). >> If you disable the VNC thread, all thread related code will not be built= and there will >> be no overhead. >> >> Signed-off-by: Corentin Chary >> --- >> Makefile.objs =A0 =A0 =A0| =A0 =A07 +- >> configure =A0 =A0 =A0 =A0 =A0| =A0 13 ++ >> ui/vnc-jobs-sync.c | =A0 65 ++++++++++ >> ui/vnc-jobs.c =A0 =A0 =A0| =A0351 ++++++++++++++++++++++++++++++++++++++= ++++++++++++++ >> ui/vnc.c =A0 =A0 =A0 =A0 =A0 | =A0169 ++++++++++++++++++++++---- >> ui/vnc.h =A0 =A0 =A0 =A0 =A0 | =A0 75 +++++++++++ >> 6 files changed, 657 insertions(+), 23 deletions(-) >> create mode 100644 ui/vnc-jobs-sync.c >> create mode 100644 ui/vnc-jobs.c >> >> diff --git a/Makefile.objs b/Makefile.objs >> index 22622a9..0c6334b 100644 >> --- a/Makefile.objs >> +++ b/Makefile.objs >> @@ -109,10 +109,15 @@ ui-obj-y +=3D vnc-enc-tight.o >> ui-obj-$(CONFIG_VNC_TLS) +=3D vnc-tls.o vnc-auth-vencrypt.o >> ui-obj-$(CONFIG_VNC_SASL) +=3D vnc-auth-sasl.o >> ui-obj-$(CONFIG_COCOA) +=3D cocoa.o >> +ifdef CONFIG_VNC_THREAD >> +ui-obj-y +=3D vnc-jobs.o >> +else >> +ui-obj-y +=3D vnc-jobs-sync.o >> +endif >> common-obj-y +=3D $(addprefix ui/, $(ui-obj-y)) >> >> common-obj-y +=3D iov.o acl.o >> -common-obj-$(CONFIG_IOTHREAD) +=3D qemu-thread.o >> +common-obj-$(CONFIG_THREAD) +=3D qemu-thread.o >> common-obj-y +=3D notify.o event_notifier.o >> common-obj-y +=3D qemu-timer.o >> >> diff --git a/configure b/configure >> index 679f2fc..6f2e3a7 100755 >> --- a/configure >> +++ b/configure >> @@ -264,6 +264,7 @@ vde=3D"" >> vnc_tls=3D"" >> vnc_sasl=3D"" >> vnc_jpeg=3D"" >> +vnc_thread=3D"" >> xen=3D"" >> linux_aio=3D"" >> vhost_net=3D"" >> @@ -552,6 +553,10 @@ for opt do >> =A0 ;; >> =A0 --enable-vnc-jpeg) vnc_jpeg=3D"yes" >> =A0 ;; >> + =A0--disable-vnc-thread) vnc_thread=3D"no" >> + =A0;; >> + =A0--enable-vnc-thread) vnc_thread=3D"yes" >> + =A0;; >> =A0 --disable-slirp) slirp=3D"no" >> =A0 ;; >> =A0 --disable-uuid) uuid=3D"no" >> @@ -786,6 +791,8 @@ echo " =A0--disable-vnc-sasl =A0 =A0 =A0 disable SAS= L encryption for VNC server" >> echo " =A0--enable-vnc-sasl =A0 =A0 =A0 =A0enable SASL encryption for VN= C server" >> echo " =A0--disable-vnc-jpeg =A0 =A0 =A0 disable JPEG lossy compression = for VNC server" >> echo " =A0--enable-vnc-jpeg =A0 =A0 =A0 =A0enable JPEG lossy compression= for VNC server" >> +echo " =A0--disable-vnc-thread =A0 =A0 disable threaded VNC server" >> +echo " =A0--enable-vnc-thread =A0 =A0 =A0enable threaded VNC server" >> echo " =A0--disable-curses =A0 =A0 =A0 =A0 disable curses output" >> echo " =A0--enable-curses =A0 =A0 =A0 =A0 =A0enable curses output" >> echo " =A0--disable-curl =A0 =A0 =A0 =A0 =A0 disable curl connectivity" >> @@ -2048,6 +2055,7 @@ echo "Mixer emulation =A0 $mixemu" >> echo "VNC TLS support =A0 $vnc_tls" >> echo "VNC SASL support =A0$vnc_sasl" >> echo "VNC JPEG support =A0$vnc_jpeg" >> +echo "VNC thread =A0 =A0 =A0 =A0$vnc_thread" >> if test -n "$sparc_cpu"; then >> =A0 =A0 echo "Target Sparc Arch $sparc_cpu" >> fi >> @@ -2191,6 +2199,10 @@ if test "$vnc_jpeg" =3D "yes" ; then >> =A0 echo "CONFIG_VNC_JPEG=3Dy" >> $config_host_mak >> =A0 echo "VNC_JPEG_CFLAGS=3D$vnc_jpeg_cflags" >> $config_host_mak >> fi >> +if test "$vnc_thread" =3D "yes" ; then > > So it's disabled by default? Sounds like a pretty cool and useful feature= to me that should be enabled by default. Because it's does not work on windows (qemu-thread.c only uses pthread) and because I don't want to break everything :) >> + =A0echo "CONFIG_VNC_THREAD=3Dy" >> $config_host_mak >> + =A0echo "CONFIG_THREAD=3Dy" >> $config_host_mak >> +fi >> if test "$fnmatch" =3D "yes" ; then >> =A0 echo "CONFIG_FNMATCH=3Dy" >> $config_host_mak >> fi >> @@ -2267,6 +2279,7 @@ if test "$xen" =3D "yes" ; then >> fi >> if test "$io_thread" =3D "yes" ; then >> =A0 echo "CONFIG_IOTHREAD=3Dy" >> $config_host_mak >> + =A0echo "CONFIG_THREAD=3Dy" >> $config_host_mak >> fi >> if test "$linux_aio" =3D "yes" ; then >> =A0 echo "CONFIG_LINUX_AIO=3Dy" >> $config_host_mak >> diff --git a/ui/vnc-jobs-sync.c b/ui/vnc-jobs-sync.c >> new file mode 100644 >> index 0000000..9f138f5 >> --- /dev/null >> +++ b/ui/vnc-jobs-sync.c >> @@ -0,0 +1,65 @@ >> +/* >> + * QEMU VNC display driver >> + * >> + * Copyright (C) 2006 Anthony Liguori >> + * Copyright (C) 2006 Fabrice Bellard >> + * Copyright (C) 2009 Red Hat, Inc >> + * Copyright (C) 2010 Corentin Chary >> + * >> + * Permission is hereby granted, free of charge, to any person obtainin= g a copy >> + * of this software and associated documentation files (the "Software")= , to deal >> + * in the Software without restriction, including without limitation th= e rights >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or= sell >> + * copies of the Software, and to permit persons to whom the Software i= s >> + * furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be inclu= ded in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPR= ESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABIL= ITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SH= ALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR= OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARIS= ING FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALIN= GS IN >> + * THE SOFTWARE. >> + */ >> + >> + >> +#include "vnc.h" >> + >> +VncJob *vnc_job_new(VncState *vs) >> +{ >> + =A0 =A0vs->job.vs =3D vs; >> + =A0 =A0vs->job.rectangles =3D 0; >> + >> + =A0 =A0vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> + =A0 =A0vnc_write_u8(vs, 0); > > Creating a job writes out a framebuffer update message? Why? This is vnc-job-sync.c, since it's synchroneous, we have to start the update here. >> + =A0 =A0vs->job.saved_offset =3D vs->output.offset; >> + =A0 =A0vnc_write_u16(vs, 0); >> + =A0 =A0return &vs->job; >> +} >> + >> +void vnc_job_push(VncJob *job) >> +{ >> + =A0 =A0VncState *vs =3D job->vs; >> + >> + =A0 =A0vs->output.buffer[job->saved_offset] =3D (job->rectangles >> 8)= & 0xFF; >> + =A0 =A0vs->output.buffer[job->saved_offset + 1] =3D job->rectangles & = 0xFF; > > There's a 16 bit little endian pointer helper somewhere. Probably better = to use that - makes the code more readable. Hum right, >> + =A0 =A0vnc_flush(job->vs); >> +} >> + >> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h) >> +{ >> + =A0 =A0int n; >> + >> + =A0 =A0n =3D vnc_send_framebuffer_update(job->vs, x, y, w, h); >> + =A0 =A0if (n >=3D 0) >> + =A0 =A0 =A0 =A0job->rectangles +=3D n; > > Coding style. > >> + =A0 =A0return n; >> +} >> + >> +bool vnc_has_job(VncState *vs) >> +{ >> + =A0 =A0return false; >> +} >> diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c >> new file mode 100644 >> index 0000000..65ce5f8 >> --- /dev/null >> +++ b/ui/vnc-jobs.c >> @@ -0,0 +1,351 @@ >> +/* >> + * QEMU VNC display driver >> + * >> + * Copyright (C) 2006 Anthony Liguori >> + * Copyright (C) 2006 Fabrice Bellard >> + * Copyright (C) 2009 Red Hat, Inc >> + * Copyright (C) 2010 Corentin Chary >> + * >> + * Permission is hereby granted, free of charge, to any person obtainin= g a copy >> + * of this software and associated documentation files (the "Software")= , to deal >> + * in the Software without restriction, including without limitation th= e rights >> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or= sell >> + * copies of the Software, and to permit persons to whom the Software i= s >> + * furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be inclu= ded in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPR= ESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABIL= ITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SH= ALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR= OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARIS= ING FROM, >> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALIN= GS IN >> + * THE SOFTWARE. >> + */ >> + >> + >> +#include "vnc.h" >> + >> +/* >> + * Locking: >> + * >> + * There is three levels of locking: >> + * - jobs queue lock: for each operation on the queue (push, pop, isEmp= ty?) >> + * - VncDisplay global lock: mainly used for framebuffer updates to avo= id >> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0screen corruption if the = framebuffer is updated >> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 while the worker is doing someth= ing. >> + * - VncState::output lock: used to make sure the output buffer is not = corrupted >> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if two threads try to write o= n it at the same time >> + * >> + * While the VNC worker thread is working, the VncDisplay global lock i= s hold >> + * to avoid screen corruptions (this does not block vnc_refresh() becau= se it >> + * uses trylock()) but the output lock is not hold because the thread w= ork on >> + * its own output buffer. >> + * When the encoding job is done, the worker thread will hold the outpu= t lock >> + * and copy its output buffer in vs->output. >> +*/ >> + >> +struct VncJobQueue { >> + =A0 =A0QemuCond cond; >> + =A0 =A0QemuMutex mutex; >> + =A0 =A0QemuThread thread; >> + =A0 =A0Buffer buffer; >> + =A0 =A0bool exit; >> + =A0 =A0QTAILQ_HEAD(, VncJob) jobs; >> +}; >> + >> +typedef struct VncJobQueue VncJobQueue; >> + >> +/* >> + * We use a single global queue, but most of the functions are >> + * already reetrant, so we can easilly add more than one encoding threa= d >> + */ >> +static VncJobQueue *queue; >> + >> +static void vnc_lock_queue(VncJobQueue *queue) >> +{ >> + =A0 =A0qemu_mutex_lock(&queue->mutex); >> +} >> + >> +static void vnc_unlock_queue(VncJobQueue *queue) >> +{ >> + =A0 =A0qemu_mutex_unlock(&queue->mutex); >> +} >> + >> +VncJob *vnc_job_new(VncState *vs) >> +{ >> + =A0 =A0VncJob *job =3D qemu_mallocz(sizeof(VncJob)); >> + >> + =A0 =A0job->vs =3D vs; >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0QLIST_INIT(&job->rectangles); >> + =A0 =A0vnc_unlock_queue(queue); >> + =A0 =A0return job; >> +} >> + >> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h) >> +{ >> + =A0 =A0VncRectEntry *entry =3D qemu_mallocz(sizeof(VncRectEntry)); >> + >> + =A0 =A0entry->rect.x =3D x; >> + =A0 =A0entry->rect.y =3D y; >> + =A0 =A0entry->rect.w =3D w; >> + =A0 =A0entry->rect.h =3D h; >> + >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0QLIST_INSERT_HEAD(&job->rectangles, entry, next); >> + =A0 =A0vnc_unlock_queue(queue); >> + =A0 =A0return 1; >> +} >> + >> +void vnc_job_push(VncJob *job) >> +{ >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0if (QLIST_EMPTY(&job->rectangles)) { >> + =A0 =A0 =A0 =A0qemu_free(job); >> + =A0 =A0} else { >> + =A0 =A0 =A0 =A0QTAILQ_INSERT_TAIL(&queue->jobs, job, next); >> + =A0 =A0 =A0 =A0qemu_cond_broadcast(&queue->cond); >> + =A0 =A0} >> + =A0 =A0vnc_unlock_queue(queue); >> +} >> + >> +static bool vnc_has_job_locked(VncState *vs) >> +{ >> + =A0 =A0VncJob *job; >> + >> + =A0 =A0QTAILQ_FOREACH(job, &queue->jobs, next) { >> + =A0 =A0 =A0 =A0if (job->vs =3D=3D vs || !vs) { >> + =A0 =A0 =A0 =A0 =A0 =A0return true; >> + =A0 =A0 =A0 =A0} >> + =A0 =A0} >> + =A0 =A0return false; >> +} >> + >> +bool vnc_has_job(VncState *vs) >> +{ >> + =A0 =A0bool ret; >> + >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0ret =3D vnc_has_job_locked(vs); >> + =A0 =A0vnc_unlock_queue(queue); >> + =A0 =A0return ret; >> +} >> + >> +void vnc_jobs_clear(VncState *vs) >> +{ >> + =A0 =A0VncJob *job, *tmp; >> + >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0QTAILQ_FOREACH_SAFE(job, &queue->jobs, next, tmp) { >> + =A0 =A0 =A0 =A0if (job->vs =3D=3D vs || !vs) >> + =A0 =A0 =A0 =A0 =A0 =A0QTAILQ_REMOVE(&queue->jobs, job, next); > > Coding style... > >> + =A0 =A0} >> + =A0 =A0vnc_unlock_queue(queue); >> +} >> + >> +void vnc_jobs_join(VncState *vs) >> +{ >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0while (vnc_has_job_locked(vs)) { >> + =A0 =A0 =A0 =A0qemu_cond_wait(&queue->cond, &queue->mutex); >> + =A0 =A0} >> + =A0 =A0vnc_unlock_queue(queue); >> +} >> + >> +/* >> + * Copy data for local use >> + * FIXME: isolate what we use in a specific structure >> + * to avoid invalid usage in vnc-encoding-*.c and avoid copying >> + * because what we want is only want is only swapping VncState::output >> + * with the queue buffer >> + */ >> +static void vnc_async_encoding_start(VncState *orig, VncState *local) >> +{ >> + =A0 =A0local->vnc_encoding =3D orig->vnc_encoding; >> + =A0 =A0local->features =3D orig->features; >> + =A0 =A0local->ds =3D orig->ds; >> + =A0 =A0local->vd =3D orig->vd; >> + =A0 =A0local->write_pixels =3D orig->write_pixels; >> + =A0 =A0local->clientds =3D orig->clientds; >> + =A0 =A0local->tight_quality =3D orig->tight_quality; >> + =A0 =A0local->tight_compression =3D orig->tight_compression; >> + =A0 =A0local->tight_pixel24 =3D 0; >> + =A0 =A0local->tight =3D orig->tight; >> + =A0 =A0local->tight_tmp =3D orig->tight_tmp; >> + =A0 =A0local->tight_zlib =3D orig->tight_zlib; >> + =A0 =A0local->tight_gradient =3D orig->tight_gradient; >> + =A0 =A0local->tight_jpeg =3D orig->tight_jpeg; >> + =A0 =A0memcpy(local->tight_levels, orig->tight_levels, sizeof(orig->ti= ght_levels)); >> + =A0 =A0memcpy(local->tight_stream, orig->tight_stream, sizeof(orig->ti= ght_stream)); >> + =A0 =A0local->send_hextile_tile =3D orig->send_hextile_tile; >> + =A0 =A0local->zlib =3D orig->zlib; >> + =A0 =A0local->zlib_tmp =3D orig->zlib_tmp; >> + =A0 =A0local->zlib_stream =3D orig->zlib_stream; >> + =A0 =A0local->zlib_level =3D orig->zlib_level; >> + =A0 =A0local->output =3D =A0queue->buffer; >> + =A0 =A0local->csock =3D -1; /* Don't do any network work on this threa= d */ > > Wow, this looks like a lot of copies. How about a *local =3D *orig? Then = just clean up the 3 variables you have to. > >> + >> + =A0 =A0buffer_reset(&local->output); >> +} >> + >> +static void vnc_async_encoding_end(VncState *orig, VncState *local) >> +{ >> + =A0 =A0orig->tight_quality =3D local->tight_quality; >> + =A0 =A0orig->tight_compression =3D local->tight_compression; >> + =A0 =A0orig->tight =3D local->tight; >> + =A0 =A0orig->tight_tmp =3D local->tight_tmp; >> + =A0 =A0orig->tight_zlib =3D local->tight_zlib; >> + =A0 =A0orig->tight_gradient =3D local->tight_gradient; >> + =A0 =A0orig->tight_jpeg =3D local->tight_jpeg; >> + =A0 =A0memcpy(orig->tight_levels, local->tight_levels, sizeof(local->t= ight_levels)); >> + =A0 =A0memcpy(orig->tight_stream, local->tight_stream, sizeof(local->t= ight_stream)); >> + =A0 =A0orig->zlib =3D local->zlib; >> + =A0 =A0orig->zlib_tmp =3D local->zlib_tmp; >> + =A0 =A0orig->zlib_stream =3D local->zlib_stream; >> + =A0 =A0orig->zlib_level =3D local->zlib_level; > > This is probably a bit more complicated to get clean. How about putting t= he tight and the zlib information in structs, so you can just move those ar= ound? orig->tight =3D local->tight; Yep, I should use specific structures, but this will come in a later patch. >> +} >> + >> +static int vnc_worker_thread_loop(VncJobQueue *queue) >> +{ >> + =A0 =A0VncJob *job; >> + =A0 =A0VncRectEntry *entry, *tmp; >> + =A0 =A0VncState vs; >> + =A0 =A0int n_rectangles; >> + =A0 =A0int saved_offset; >> + =A0 =A0bool flush; >> + >> + =A0 =A0vnc_lock_queue(queue); >> + =A0 =A0if (QTAILQ_EMPTY(&queue->jobs)) { >> + =A0 =A0 =A0 =A0qemu_cond_wait(&queue->cond, &queue->mutex); >> + =A0 =A0} >> + >> + =A0 =A0/* If the queue is empty, it's an exit order */ >> + =A0 =A0if (QTAILQ_EMPTY(&queue->jobs)) { >> + =A0 =A0 =A0 =A0vnc_unlock_queue(queue); >> + =A0 =A0 =A0 =A0return -1; >> + =A0 =A0} >> + >> + =A0 =A0job =3D QTAILQ_FIRST(&queue->jobs); >> + =A0 =A0vnc_unlock_queue(queue); >> + >> + =A0 =A0qemu_mutex_lock(&job->vs->output_mutex); >> + =A0 =A0if (job->vs->csock =3D=3D -1 || job->vs->abording =3D=3D true) = { >> + =A0 =A0 =A0 =A0goto disconnected; >> + =A0 =A0} >> + =A0 =A0qemu_mutex_unlock(&job->vs->output_mutex); >> + >> + =A0 =A0/* Make a local copy of vs and switch output buffers */ >> + =A0 =A0vnc_async_encoding_start(job->vs, &vs); >> + >> + =A0 =A0/* Start sending rectangles */ >> + =A0 =A0n_rectangles =3D 0; >> + =A0 =A0vnc_write_u8(&vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> + =A0 =A0vnc_write_u8(&vs, 0); >> + =A0 =A0saved_offset =3D vs.output.offset; >> + =A0 =A0vnc_write_u16(&vs, 0); > > This too strikes me as odd. > >> + >> + =A0 =A0qemu_mutex_lock(&job->vs->vd->mutex); >> + =A0 =A0QLIST_FOREACH_SAFE(entry, &job->rectangles, next, tmp) { >> + =A0 =A0 =A0 =A0int n; >> + >> + =A0 =A0 =A0 =A0if (job->vs->csock =3D=3D -1) { >> + =A0 =A0 =A0 =A0 =A0 =A0qemu_mutex_unlock(&job->vs->vd->mutex); >> + =A0 =A0 =A0 =A0 =A0 =A0goto disconnected; >> + =A0 =A0 =A0 =A0} >> + >> + =A0 =A0 =A0 =A0n =3D vnc_send_framebuffer_update(&vs, entry->rect.x, e= ntry->rect.y, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0entry->rect.w, entry->rect.h); > > Wow, wait. You're coming from a rect, put everything in individual parame= ters and form them into a rect again afterwards? Just keep the rect :). > >> + >> + =A0 =A0 =A0 =A0if (n >=3D 0) >> + =A0 =A0 =A0 =A0 =A0 =A0n_rectangles +=3D n; >> + =A0 =A0 =A0 =A0qemu_free(entry); >> + =A0 =A0} >> + =A0 =A0qemu_mutex_unlock(&job->vs->vd->mutex); >> + >> + =A0 =A0/* Put n_rectangles at the beginning of the message */ >> + =A0 =A0vs.output.buffer[saved_offset] =3D (n_rectangles >> 8) & 0xFF; >> + =A0 =A0vs.output.buffer[saved_offset + 1] =3D n_rectangles & 0xFF; > > Deja vu? > >> + >> + =A0 =A0/* Switch back buffers */ >> + =A0 =A0qemu_mutex_lock(&job->vs->output_mutex); >> + =A0 =A0if (job->vs->csock =3D=3D -1) { >> + =A0 =A0 =A0 =A0goto disconnected; >> + =A0 =A0} >> + >> + =A0 =A0vnc_write(job->vs, vs.output.buffer, vs.output.offset); >> + >> +disconnected: >> + =A0 =A0/* Copy persistent encoding data */ >> + =A0 =A0vnc_async_encoding_end(job->vs, &vs); >> + =A0 =A0flush =3D (job->vs->csock !=3D -1 && job->vs->abording !=3D tru= e); >> + =A0 =A0qemu_mutex_unlock(&job->vs->output_mutex); >> + >> + =A0 =A0if (flush) { >> + =A0 =A0 =A0 =A0vnc_flush(job->vs); >> + =A0 =A0} >> + >> + =A0 =A0qemu_mutex_lock(&queue->mutex); >> + =A0 =A0QTAILQ_REMOVE(&queue->jobs, job, next); >> + =A0 =A0qemu_mutex_unlock(&queue->mutex); >> + =A0 =A0qemu_cond_broadcast(&queue->cond); >> + =A0 =A0qemu_free(job); >> + =A0 =A0return 0; >> +} >> + >> +static VncJobQueue *vnc_queue_init(void) >> +{ >> + =A0 =A0VncJobQueue *queue =3D qemu_mallocz(sizeof(VncJobQueue)); >> + >> + =A0 =A0qemu_cond_init(&queue->cond); >> + =A0 =A0qemu_mutex_init(&queue->mutex); >> + =A0 =A0QTAILQ_INIT(&queue->jobs); >> + =A0 =A0return queue; >> +} >> + >> +static void vnc_queue_clear(VncJobQueue *q) >> +{ >> + =A0 =A0qemu_cond_destroy(&queue->cond); >> + =A0 =A0qemu_mutex_destroy(&queue->mutex); >> + =A0 =A0buffer_free(&queue->buffer); >> + =A0 =A0qemu_free(q); >> + =A0 =A0queue =3D NULL; /* Unset global queue */ >> +} >> + >> +static void *vnc_worker_thread(void *arg) >> +{ >> + =A0 =A0VncJobQueue *queue =3D arg; >> + >> + =A0 =A0while (!vnc_worker_thread_loop(queue)) ; >> + =A0 =A0vnc_queue_clear(queue); >> + =A0 =A0return NULL; >> +} >> + >> +void vnc_start_worker_thread(void) >> +{ >> + =A0 =A0VncJobQueue *q; >> + >> + =A0 =A0if (vnc_worker_thread_running()) >> + =A0 =A0 =A0 =A0return ; >> + >> + =A0 =A0q =3D vnc_queue_init(); >> + =A0 =A0qemu_thread_create(&q->thread, vnc_worker_thread, q); >> + =A0 =A0queue =3D q; /* Set global queue */ >> +} >> + >> +bool vnc_worker_thread_running(void) >> +{ >> + =A0 =A0return queue; /* Check global queue */ >> +} >> + >> +void vnc_stop_worker_thread(void) >> +{ >> + =A0 =A0if (!vnc_worker_thread_running()) >> + =A0 =A0 =A0 =A0return ; >> + >> + =A0 =A0/* Remove all jobs and wake up the thread */ >> + =A0 =A0vnc_jobs_clear(NULL); >> + =A0 =A0qemu_cond_broadcast(&queue->cond); >> +} >> diff --git a/ui/vnc.c b/ui/vnc.c >> index e3ef315..f6475b3 100644 >> --- a/ui/vnc.c >> +++ b/ui/vnc.c >> @@ -45,6 +45,36 @@ >> =A0 =A0 } \ >> } >> >> +#ifdef CONFIG_VNC_THREAD >> +static int vnc_trylock_display(VncDisplay *vd) >> +{ >> + =A0 =A0return qemu_mutex_trylock(&vd->mutex); >> +} >> + >> +static void vnc_unlock_display(VncDisplay *vd) >> +{ >> + =A0 =A0qemu_mutex_unlock(&vd->mutex); >> +} >> + >> +static void vnc_lock_output(VncState *vs) >> +{ >> + =A0 =A0qemu_mutex_lock(&vs->output_mutex); >> +} >> + >> +static void vnc_unlock_output(VncState *vs) >> +{ >> + =A0 =A0qemu_mutex_unlock(&vs->output_mutex); >> +} > > Not static, but static inline. I guess. But then again that doesn't reall= y hurt - we're not in a header file. I think it's better to let gcc decide if this should be inlined or not. (Here it will probably be inline with -O2). >> +#else >> +static int vnc_trylock_display(VncDisplay *vd) >> +{ >> + =A0 =A0return 0; >> +} >> + >> +#define vnc_unlock_display(vs) (void) (vs); >> +#define vnc_lock_output(vs) (void) (vs); >> +#define vnc_unlock_output(vs) (void) (vs); > > Please make those static functions too. Ok, >> +#endif >> >> static VncDisplay *vnc_display; /* needed for info vnc */ >> static DisplayChangeListener *dcl; >> @@ -363,6 +393,7 @@ static inline uint32_t vnc_has_feature(VncState *vs,= int feature) { >> */ >> >> static int vnc_update_client(VncState *vs, int has_dirty); >> +static int vnc_update_client_sync(VncState *vs, int has_dirty); >> static void vnc_disconnect_start(VncState *vs); >> static void vnc_disconnect_finish(VncState *vs); >> static void vnc_init_timer(VncDisplay *vd); >> @@ -493,6 +524,7 @@ void buffer_append(Buffer *buffer, const void *data,= size_t len) >> =A0 =A0 buffer->offset +=3D len; >> } >> >> + > > Huh? > >> static void vnc_desktop_resize(VncState *vs) >> { >> =A0 =A0 DisplayState *ds =3D vs->ds; >> @@ -506,19 +538,46 @@ static void vnc_desktop_resize(VncState *vs) >> =A0 =A0 } >> =A0 =A0 vs->client_width =3D ds_get_width(ds); >> =A0 =A0 vs->client_height =3D ds_get_height(ds); >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 vnc_write_u16(vs, 1); /* number of rects */ >> =A0 =A0 vnc_framebuffer_update(vs, 0, 0, vs->client_width, vs->client_he= ight, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0VNC_ENCODING_DESK= TOPRESIZE); >> + =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 vnc_flush(vs); >> } >> >> +#ifdef CONFIG_VNC_THREAD >> +static void vnc_abort_display_jobs(VncDisplay *vd) >> +{ >> + =A0 =A0VncState *vs; >> + >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> + =A0 =A0 =A0 =A0vs->abording =3D true; >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> + =A0 =A0} >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_jobs_join(vs); >> + =A0 =A0} >> + =A0 =A0QTAILQ_FOREACH(vs, &vd->clients, next) { >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> + =A0 =A0 =A0 =A0vs->abording =3D false; >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> + =A0 =A0} >> +} >> +#else >> +#define vnc_abort_display_jobs(vd) > > see above > >> +#endif >> + >> static void vnc_dpy_resize(DisplayState *ds) >> { >> =A0 =A0 VncDisplay *vd =3D ds->opaque; >> =A0 =A0 VncState *vs; >> >> + =A0 =A0vnc_abort_display_jobs(vd); >> + >> =A0 =A0 /* server surface */ >> =A0 =A0 if (!vd->server) >> =A0 =A0 =A0 =A0 vd->server =3D qemu_mallocz(sizeof(*vd->server)); >> @@ -646,7 +705,7 @@ int vnc_raw_send_framebuffer_update(VncState *vs, in= t x, int y, int w, int h) >> =A0 =A0 return 1; >> } >> >> -static int send_framebuffer_update(VncState *vs, int x, int y, int w, i= nt h) >> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int = h) >> { >> =A0 =A0 int n =3D 0; >> >> @@ -672,12 +731,14 @@ static int send_framebuffer_update(VncState *vs, i= nt x, int y, int w, int h) >> static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int = dst_y, int w, int h) >> { >> =A0 =A0 /* send bitblit op to the vnc client */ >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 vnc_write_u16(vs, 1); /* number of rects */ >> =A0 =A0 vnc_framebuffer_update(vs, dst_x, dst_y, w, h, VNC_ENCODING_COPY= RECT); >> =A0 =A0 vnc_write_u16(vs, src_x); >> =A0 =A0 vnc_write_u16(vs, src_y); >> + =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 vnc_flush(vs); >> } >> >> @@ -694,7 +755,7 @@ static void vnc_dpy_copy(DisplayState *ds, int src_x= , int src_y, int dst_x, int >> =A0 =A0 QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) { >> =A0 =A0 =A0 =A0 if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) { >> =A0 =A0 =A0 =A0 =A0 =A0 vs->force_update =3D 1; >> - =A0 =A0 =A0 =A0 =A0 =A0vnc_update_client(vs, 1); >> + =A0 =A0 =A0 =A0 =A0 =A0vnc_update_client_sync(vs, 1); >> =A0 =A0 =A0 =A0 =A0 =A0 /* vs might be free()ed here */ >> =A0 =A0 =A0 =A0 } >> =A0 =A0 } >> @@ -814,15 +875,29 @@ static int find_and_clear_dirty_height(struct VncS= tate *vs, >> =A0 =A0 return h; >> } >> >> +#ifdef CONFIG_VNC_THREAD >> +static int vnc_update_client_sync(VncState *vs, int has_dirty) >> +{ >> + =A0 =A0int ret =3D vnc_update_client(vs, has_dirty); >> + =A0 =A0vnc_jobs_join(vs); >> + =A0 =A0return ret; >> +} >> +#else >> +static int vnc_update_client_sync(VncState *vs, int has_dirty) >> +{ >> + =A0 =A0return vnc_update_client(vs, has_dirty); >> +} >> +#endif >> + >> static int vnc_update_client(VncState *vs, int has_dirty) >> { >> =A0 =A0 if (vs->need_update && vs->csock !=3D -1) { >> =A0 =A0 =A0 =A0 VncDisplay *vd =3D vs->vd; >> + =A0 =A0 =A0 =A0VncJob *job; >> =A0 =A0 =A0 =A0 int y; >> - =A0 =A0 =A0 =A0int n_rectangles; >> - =A0 =A0 =A0 =A0int saved_offset; >> =A0 =A0 =A0 =A0 int width, height; >> - =A0 =A0 =A0 =A0int n; >> + =A0 =A0 =A0 =A0int n =3D 0; >> + >> >> =A0 =A0 =A0 =A0 if (vs->output.offset && !vs->audio_cap && !vs->force_up= date) >> =A0 =A0 =A0 =A0 =A0 =A0 /* kernel send buffers are full -> drop frames t= o throttle */ >> @@ -837,11 +912,7 @@ static int vnc_update_client(VncState *vs, int has_= dirty) >> =A0 =A0 =A0 =A0 =A0* happening in parallel don't disturb us, the next pa= ss will >> =A0 =A0 =A0 =A0 =A0* send them to the client. >> =A0 =A0 =A0 =A0 =A0*/ >> - =A0 =A0 =A0 =A0n_rectangles =3D 0; >> - =A0 =A0 =A0 =A0vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> - =A0 =A0 =A0 =A0vnc_write_u8(vs, 0); >> - =A0 =A0 =A0 =A0saved_offset =3D vs->output.offset; >> - =A0 =A0 =A0 =A0vnc_write_u16(vs, 0); >> + =A0 =A0 =A0 =A0job =3D vnc_job_new(vs); >> >> =A0 =A0 =A0 =A0 width =3D MIN(vd->server->width, vs->client_width); >> =A0 =A0 =A0 =A0 height =3D MIN(vd->server->height, vs->client_height); >> @@ -858,25 +929,23 @@ static int vnc_update_client(VncState *vs, int has= _dirty) >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (last_x !=3D -1) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int h =3D find_and_clear= _dirty_height(vs, y, last_x, x); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n =3D send_framebuffer_= update(vs, last_x * 16, y, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(x - last_x) * 16, h); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n_rectangles +=3D n; >> + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n +=3D vnc_job_add_rect= (job, last_x * 16, y, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0(x - last_x) * 16, h); >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 last_x =3D -1; >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 =A0 =A0 if (last_x !=3D -1) { >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int h =3D find_and_clear_dirty_height(vs= , y, last_x, x); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n =3D send_framebuffer_update(vs, last_= x * 16, y, >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0(x - last_x) * 16, h); >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n_rectangles +=3D n; >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n +=3D vnc_job_add_rect(job, last_x * 1= 6, y, >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0(x - last_x) * 16, h); > > Oh, so that's why. Well, better keep the individual parameters then. > >> =A0 =A0 =A0 =A0 =A0 =A0 } >> =A0 =A0 =A0 =A0 } >> - =A0 =A0 =A0 =A0vs->output.buffer[saved_offset] =3D (n_rectangles >> 8)= & 0xFF; >> - =A0 =A0 =A0 =A0vs->output.buffer[saved_offset + 1] =3D n_rectangles & = 0xFF; >> - =A0 =A0 =A0 =A0vnc_flush(vs); >> + >> + =A0 =A0 =A0 =A0vnc_job_push(job); >> =A0 =A0 =A0 =A0 vs->force_update =3D 0; >> - =A0 =A0 =A0 =A0return n_rectangles; >> + =A0 =A0 =A0 =A0return n; > > So by now the rest of the code thought you successfully processed that re= ct, right? Yes. >> =A0 =A0 } >> >> =A0 =A0 if (vs->csock =3D=3D -1) >> @@ -892,16 +961,20 @@ static void audio_capture_notify(void *opaque, aud= cnotification_e cmd) >> >> =A0 =A0 switch (cmd) { >> =A0 =A0 case AUD_CNOTIFY_DISABLE: >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO); >> =A0 =A0 =A0 =A0 vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_END); >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 =A0 =A0 vnc_flush(vs); >> =A0 =A0 =A0 =A0 break; >> >> =A0 =A0 case AUD_CNOTIFY_ENABLE: >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO); >> =A0 =A0 =A0 =A0 vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_BEGIN); >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 =A0 =A0 vnc_flush(vs); >> =A0 =A0 =A0 =A0 break; >> =A0 =A0 } >> @@ -915,11 +988,13 @@ static void audio_capture(void *opaque, void *buf,= int size) >> { >> =A0 =A0 VncState *vs =3D opaque; >> >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO); >> =A0 =A0 vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA); >> =A0 =A0 vnc_write_u32(vs, size); >> =A0 =A0 vnc_write(vs, buf, size); >> + =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 vnc_flush(vs); >> } >> >> @@ -961,6 +1036,9 @@ static void vnc_disconnect_start(VncState *vs) >> >> static void vnc_disconnect_finish(VncState *vs) >> { >> + =A0 =A0vnc_jobs_join(vs); /* Wait encoding jobs */ >> + >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_qmp_event(vs, QEVENT_VNC_DISCONNECTED); >> >> =A0 =A0 buffer_free(&vs->input); >> @@ -989,6 +1067,11 @@ static void vnc_disconnect_finish(VncState *vs) >> =A0 =A0 vnc_remove_timer(vs->vd); >> =A0 =A0 if (vs->vd->lock_key_sync) >> =A0 =A0 =A0 =A0 qemu_remove_led_event_handler(vs->led); >> + =A0 =A0vnc_unlock_output(vs); >> + >> +#ifdef CONFIG_VNC_THREAD >> + =A0 =A0qemu_mutex_destroy(&vs->output_mutex); >> +#endif >> =A0 =A0 qemu_free(vs); >> } >> >> @@ -1108,7 +1191,7 @@ static long vnc_client_write_plain(VncState *vs) >> =A0* the client socket. Will delegate actual work according to whether >> =A0* SASL SSF layers are enabled (thus requiring encryption calls) >> =A0*/ >> -void vnc_client_write(void *opaque) >> +static void vnc_client_write_locked(void *opaque) >> { >> =A0 =A0 VncState *vs =3D opaque; >> >> @@ -1122,6 +1205,19 @@ void vnc_client_write(void *opaque) >> =A0 =A0 =A0 =A0 vnc_client_write_plain(vs); >> } >> >> +void vnc_client_write(void *opaque) >> +{ >> + =A0 =A0VncState *vs =3D opaque; >> + >> + =A0 =A0vnc_lock_output(vs); >> + =A0 =A0if (vs->output.offset) { >> + =A0 =A0 =A0 =A0vnc_client_write_locked(opaque); >> + =A0 =A0} else { >> + =A0 =A0 =A0 =A0qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, = NULL, vs); >> + =A0 =A0} >> + =A0 =A0vnc_unlock_output(vs); >> +} >> + >> void vnc_read_when(VncState *vs, VncReadEvent *func, size_t expecting) >> { >> =A0 =A0 vs->read_handler =3D func; >> @@ -1232,6 +1328,7 @@ void vnc_write(VncState *vs, const void *data, siz= e_t len) >> { >> =A0 =A0 buffer_reserve(&vs->output, len); >> >> + > > Eeh. > >> =A0 =A0 if (vs->csock !=3D -1 && buffer_empty(&vs->output)) { >> =A0 =A0 =A0 =A0 qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, v= nc_client_write, vs); >> =A0 =A0 } >> @@ -1273,8 +1370,10 @@ void vnc_write_u8(VncState *vs, uint8_t value) >> >> void vnc_flush(VncState *vs) >> { >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 if (vs->csock !=3D -1 && vs->output.offset) >> - =A0 =A0 =A0 =A0vnc_client_write(vs); >> + =A0 =A0 =A0 =A0vnc_client_write_locked(vs); > > Please change the brackets while you're at it. Some genius thought it'd b= e a good idea to define a "new" coding style that's different from all the = current qemu code. But consistency is better than none, so we need to stick= with it now. > >> + =A0 =A0vnc_unlock_output(vs); >> } >> >> uint8_t read_u8(uint8_t *data, size_t offset) >> @@ -1309,12 +1408,14 @@ static void check_pointer_type_change(Notifier *= notifier) >> =A0 =A0 int absolute =3D kbd_mouse_is_absolute(); >> >> =A0 =A0 if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE) && vs->= absolute !=3D absolute) { >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 =A0 =A0 vnc_write_u16(vs, 1); >> =A0 =A0 =A0 =A0 vnc_framebuffer_update(vs, absolute, 0, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ds_get_wi= dth(vs->ds), ds_get_height(vs->ds), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0VNC_ENCOD= ING_POINTER_TYPE_CHANGE); > > Man I really think those framebuffer update message headers should be fol= ded into the respective update function. Yep >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 =A0 =A0 vnc_flush(vs); >> =A0 =A0 } >> =A0 =A0 vs->absolute =3D absolute; >> @@ -1618,21 +1719,25 @@ static void framebuffer_update_request(VncState = *vs, int incremental, >> >> static void send_ext_key_event_ack(VncState *vs) >> { >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 vnc_write_u16(vs, 1); >> =A0 =A0 vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_he= ight(vs->ds), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0VNC_ENCODING_EXT_= KEY_EVENT); >> + =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 vnc_flush(vs); >> } >> >> static void send_ext_audio_ack(VncState *vs) >> { >> + =A0 =A0vnc_lock_output(vs); >> =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 vnc_write_u16(vs, 1); >> =A0 =A0 vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), ds_get_he= ight(vs->ds), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0VNC_ENCODING_AUDI= O); >> + =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 vnc_flush(vs); >> } >> >> @@ -1791,12 +1896,14 @@ static void vnc_colordepth(VncState *vs) >> { >> =A0 =A0 if (vnc_has_feature(vs, VNC_FEATURE_WMVI)) { >> =A0 =A0 =A0 =A0 /* Sending a WMVi message to notify the client*/ >> + =A0 =A0 =A0 =A0vnc_lock_output(vs); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, VNC_MSG_SERVER_FRAMEBUFFER_UPDATE); >> =A0 =A0 =A0 =A0 vnc_write_u8(vs, 0); >> =A0 =A0 =A0 =A0 vnc_write_u16(vs, 1); /* number of rects */ >> =A0 =A0 =A0 =A0 vnc_framebuffer_update(vs, 0, 0, ds_get_width(vs->ds), >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ds_get_he= ight(vs->ds), VNC_ENCODING_WMVi); >> =A0 =A0 =A0 =A0 pixel_format_message(vs); >> + =A0 =A0 =A0 =A0vnc_unlock_output(vs); >> =A0 =A0 =A0 =A0 vnc_flush(vs); >> =A0 =A0 } else { >> =A0 =A0 =A0 =A0 set_pixel_conversion(vs); >> @@ -2224,12 +2331,21 @@ static void vnc_refresh(void *opaque) >> >> =A0 =A0 vga_hw_update(); >> >> + =A0 =A0if (vnc_trylock_display(vd)) { >> + =A0 =A0 =A0 =A0vd->timer_interval =3D VNC_REFRESH_INTERVAL_BASE; >> + =A0 =A0 =A0 =A0qemu_mod_timer(vd->timer, qemu_get_clock(rt_clock) + >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 vd->timer_interval); >> + =A0 =A0 =A0 =A0return; >> + =A0 =A0} >> + >> =A0 =A0 has_dirty =3D vnc_refresh_server_surface(vd); >> + =A0 =A0vnc_unlock_display(vd); >> >> =A0 =A0 QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) { >> =A0 =A0 =A0 =A0 rects +=3D vnc_update_client(vs, has_dirty); >> =A0 =A0 =A0 =A0 /* vs might be free()ed here */ >> =A0 =A0 } >> + > > ... > >> =A0 =A0 /* vd->timer could be NULL now if the last client disconnected, >> =A0 =A0 =A0* in this case don't update the timer */ >> =A0 =A0 if (vd->timer =3D=3D NULL) >> @@ -2288,6 +2404,10 @@ static void vnc_connect(VncDisplay *vd, int csock= ) >> =A0 =A0 vs->as.fmt =3D AUD_FMT_S16; >> =A0 =A0 vs->as.endianness =3D 0; >> >> +#ifdef CONFIG_VNC_THREAD >> + =A0 =A0qemu_mutex_init(&vs->output_mutex); >> +#endif >> + >> =A0 =A0 QTAILQ_INSERT_HEAD(&vd->clients, vs, next); >> >> =A0 =A0 vga_hw_update(); >> @@ -2345,6 +2465,11 @@ void vnc_display_init(DisplayState *ds) >> =A0 =A0 if (!vs->kbd_layout) >> =A0 =A0 =A0 =A0 exit(1); >> >> +#ifdef CONFIG_VNC_THREAD >> + =A0 =A0qemu_mutex_init(&vs->mutex); >> + =A0 =A0vnc_start_worker_thread(); >> +#endif >> + >> =A0 =A0 dcl->dpy_copy =3D vnc_dpy_copy; >> =A0 =A0 dcl->dpy_update =3D vnc_dpy_update; >> =A0 =A0 dcl->dpy_resize =3D vnc_dpy_resize; >> diff --git a/ui/vnc.h b/ui/vnc.h >> index cca1946..9405e61 100644 >> --- a/ui/vnc.h >> +++ b/ui/vnc.h >> @@ -29,6 +29,9 @@ >> >> #include "qemu-common.h" >> #include "qemu-queue.h" >> +#ifdef CONFIG_VNC_THREAD >> +#include "qemu-thread.h" >> +#endif >> #include "console.h" >> #include "monitor.h" >> #include "audio/audio.h" >> @@ -59,6 +62,9 @@ typedef struct Buffer >> } Buffer; >> >> typedef struct VncState VncState; >> +typedef struct VncJob VncJob; >> +typedef struct VncRect VncRect; >> +typedef struct VncRectEntry VncRectEntry; >> >> typedef int VncReadEvent(VncState *vs, uint8_t *data, size_t len); >> >> @@ -101,6 +107,9 @@ struct VncDisplay >> =A0 =A0 DisplayState *ds; >> =A0 =A0 kbd_layout_t *kbd_layout; >> =A0 =A0 int lock_key_sync; >> +#ifdef CONFIG_VNC_THREAD >> + =A0 =A0QemuMutex mutex; >> +#endif >> >> =A0 =A0 QEMUCursor *cursor; >> =A0 =A0 int cursor_msize; >> @@ -122,6 +131,38 @@ struct VncDisplay >> #endif >> }; >> >> + >> +#ifdef CONFIG_VNC_THREAD >> +struct VncRect >> +{ >> + =A0 =A0int x; >> + =A0 =A0int y; >> + =A0 =A0int w; >> + =A0 =A0int h; >> +}; >> + >> +struct VncRectEntry >> +{ >> + =A0 =A0struct VncRect rect; >> + =A0 =A0QLIST_ENTRY(VncRectEntry) next; >> +}; >> + >> +struct VncJob >> +{ >> + =A0 =A0VncState *vs; >> + >> + =A0 =A0QLIST_HEAD(, VncRectEntry) rectangles; >> + =A0 =A0QTAILQ_ENTRY(VncJob) next; >> +}; >> +#else >> +struct VncJob >> +{ >> + =A0 =A0VncState *vs; >> + =A0 =A0int rectangles; >> + =A0 =A0size_t saved_offset; >> +}; >> +#endif >> + >> struct VncState >> { >> =A0 =A0 int csock; >> @@ -170,6 +211,12 @@ struct VncState >> =A0 =A0 QEMUPutLEDEntry *led; >> >> =A0 =A0 /* Encoding specific */ >> + =A0 =A0bool abording; >> +#ifndef CONFIG_VNC_THREAD > > Please stay on your positive attitude :) > >> + =A0 =A0VncJob job; >> +#else >> + =A0 =A0QemuMutex output_mutex; >> +#endif >> >> =A0 =A0 /* Tight */ >> =A0 =A0 uint8_t tight_quality; >> @@ -412,6 +459,8 @@ void vnc_framebuffer_update(VncState *vs, int x, int= y, int w, int h, >> void vnc_convert_pixel(VncState *vs, uint8_t *buf, uint32_t v); >> >> /* Encodings */ >> +int vnc_send_framebuffer_update(VncState *vs, int x, int y, int w, int = h); >> + >> int vnc_raw_send_framebuffer_update(VncState *vs, int x, int y, int w, i= nt h); >> >> int vnc_hextile_send_framebuffer_update(VncState *vs, int x, >> @@ -427,4 +476,30 @@ void vnc_zlib_clear(VncState *vs); >> int vnc_tight_send_framebuffer_update(VncState *vs, int x, int y, int w,= int h); >> void vnc_tight_clear(VncState *vs); >> >> +/* Jobs */ >> +#ifdef CONFIG_VNC_THREAD >> + >> +VncJob *vnc_job_new(VncState *vs); >> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h); >> +void vnc_job_push(VncJob *job); >> +bool vnc_has_job(VncState *vs); >> +void vnc_jobs_clear(VncState *vs); >> +void vnc_jobs_join(VncState *vs); >> +void vnc_start_worker_thread(void); >> +bool vnc_worker_thread_running(void); >> +void vnc_stop_worker_thread(void); >> + >> +#else >> + >> +#define vnc_jobs_clear(vs) (void) (vs); >> +#define vnc_jobs_join(vs) (void) (vs); > > static inline functions please. > >> + >> +VncJob *vnc_job_new(VncState *vs); >> +bool vnc_has_job(VncState *vs); >> +int vnc_job_add_rect(VncJob *job, int x, int y, int w, int h); >> +bool vnc_worker_thread_running(void); >> +void vnc_job_push(VncJob *job); >> + >> +#endif /* CONFIG_VNC_THREAD */ >> + >> #endif /* __QEMU_VNC_H */ >> -- >> 1.7.1 >> > > --=20 Corentin Chary http://xf.iksaif.net