From owner-svn-src-all@freebsd.org Mon Nov 7 19:46:31 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 547EAC354D9; Mon, 7 Nov 2016 19:46:31 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (mail.turbocat.net [IPv6:2a01:4f8:d16:4514::2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E3ACAB7B; Mon, 7 Nov 2016 19:46:30 +0000 (UTC) (envelope-from hps@selasky.org) Received: from laptop015.home.selasky.org (unknown [62.141.129.119]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 105DB1FE022; Mon, 7 Nov 2016 20:46:29 +0100 (CET) Subject: Re: svn commit: r308424 - head/sys/arm/broadcom/bcm2835 To: Oleksandr Tymoshenko References: <201611071738.uA7HceYu045944@repo.freebsd.org> <680D84F2-65BF-48DD-8D11-311B1F65A634@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org From: Hans Petter Selasky Message-ID: <2b03afc6-9bfa-1124-7b70-2532751bfed9@selasky.org> Date: Mon, 7 Nov 2016 20:51:39 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------470FDF2C15B0D23E906DFD84" X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Nov 2016 19:46:31 -0000 This is a multi-part message in MIME format. --------------470FDF2C15B0D23E906DFD84 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 11/07/16 20:32, Hans Petter Selasky wrote: > On 11/07/16 20:23, Oleksandr Tymoshenko wrote: >> >>> On Nov 7, 2016, at 10:27 AM, Hans Petter Selasky >>> wrote: >>> >>> On 11/07/16 18:38, Oleksandr Tymoshenko wrote: >>>> + bcm2835_audio_unlock(sc); >>>> + cv_signal(&sc->worker_cv); >>> >>> >>> Shouldn't cv_signal() be done locked, so that you don't loose any >>> transactions? CV's only wakeup the treads that are sleeping right >>> there and then. >> >> Hi Hans, >> >> In this case it doesn’t matter. bcm2835_audio_xxx lock functions are >> used to keep channel state consistent. The actual audio hw >> reprogramming happens in worker thread which only picks up latest >> state of the virtual channel, there is no need to run every >> transaction in sequence. >> > > Hi, > > It is not about running in sequence, but that if the worker thread is > not sleeping, but on the way to sleep, it will never get woken up unless > you use proper locks here! > > --HPS Hi, Also the teardown sequence for the worker thread looks a bit broken, that it doesn't wait for the thread to exit. I've made a patch, attached which I think is the right way to do it. Try opening and closing /dev/dsp in a loop with some DSP ioctls and see what happens. --HPS --------------470FDF2C15B0D23E906DFD84 Content-Type: text/x-patch; name="bcm2835_audio.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="bcm2835_audio.diff" Index: sys/arm/broadcom/bcm2835/bcm2835_audio.c =================================================================== --- sys/arm/broadcom/bcm2835/bcm2835_audio.c (revision 308426) +++ sys/arm/broadcom/bcm2835/bcm2835_audio.c (working copy) @@ -149,6 +149,14 @@ } static void +bcm2835_wakeup_worker(struct bcm2835_audio_info *sc) +{ + sx_xlock(&sc->worker_lock); + cv_signal(&sc->worker_cv); + sx_xunlock(&sc->worker_lock); +} + +static void bcm2835_audio_callback(void *param, const VCHI_CALLBACK_REASON_T reason, void *msg_handle) { struct bcm2835_audio_info *sc = (struct bcm2835_audio_info *)param; @@ -540,6 +548,8 @@ } } } + sc->unloading = 2; + cv_signal(&sc->worker_cv); sx_sunlock(&sc->worker_lock); kproc_exit(0); @@ -586,8 +596,9 @@ } sc->parameters_update_pending = true; - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); + return ch; } @@ -615,7 +626,7 @@ sc->parameters_update_pending = true; bcm2835_audio_unlock(sc); - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); return 0; } @@ -631,7 +642,7 @@ sc->parameters_update_pending = true; bcm2835_audio_unlock(sc); - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); return ch->spd; } @@ -662,8 +673,7 @@ bcm2835_audio_unlock(sc); /* kickstart data flow */ chn_intr(sc->pch.channel); - /* wakeup worker thread */ - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); break; case PCMTRIG_STOP: @@ -671,7 +681,7 @@ bcm2835_audio_lock(sc); ch->playback_state = PLAYBACK_STOPPING; bcm2835_audio_unlock(sc); - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); break; default: @@ -738,7 +748,8 @@ sc->volume = left; sc->controls_update_pending = true; bcm2835_audio_unlock(sc); - cv_signal(&sc->worker_cv); + + bcm2835_wakeup_worker(sc); break; default: @@ -776,7 +787,7 @@ sc->controls_update_pending = true; bcm2835_audio_unlock(sc); - cv_signal(&sc->worker_cv); + bcm2835_wakeup_worker(sc); device_printf(sc->dev, "destination set to %s\n", dest_description(val)); return (0); @@ -898,8 +909,11 @@ /* Stop worker thread */ sx_xlock(&sc->worker_lock); sc->unloading = 1; + cv_signal(&sc->worker_cv); + /* Wait for thread to exit */ + while (sc->unloading != 2) + cv_wait_sig(&sc->worker_cv, &sc->worker_lock); sx_xunlock(&sc->worker_lock); - cv_signal(&sc->worker_cv); r = pcm_unregister(dev); if (r) --------------470FDF2C15B0D23E906DFD84--