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 <h...@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
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)
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
