Date: Mon, 7 Nov 2016 20:51:39 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Oleksandr Tymoshenko <gonzo@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r308424 - head/sys/arm/broadcom/bcm2835 Message-ID: <2b03afc6-9bfa-1124-7b70-2532751bfed9@selasky.org> In-Reply-To: <c7cd871d-9007-6de4-7063-2680e259713f@selasky.org> References: <201611071738.uA7HceYu045944@repo.freebsd.org> <f8169b11-56d8-4566-f9e9-e387dd9b939e@selasky.org> <680D84F2-65BF-48DD-8D11-311B1F65A634@freebsd.org> <c7cd871d-9007-6de4-7063-2680e259713f@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <hps@selasky.org>
>>> 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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2b03afc6-9bfa-1124-7b70-2532751bfed9>