From owner-svn-src-all@freebsd.org Mon Nov 7 17:38:41 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 0804DC353AF; Mon, 7 Nov 2016 17:38:41 +0000 (UTC) (envelope-from gonzo@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (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 D93A81F09; Mon, 7 Nov 2016 17:38:40 +0000 (UTC) (envelope-from gonzo@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id uA7Hce59045945; Mon, 7 Nov 2016 17:38:40 GMT (envelope-from gonzo@FreeBSD.org) Received: (from gonzo@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id uA7HceYu045944; Mon, 7 Nov 2016 17:38:40 GMT (envelope-from gonzo@FreeBSD.org) Message-Id: <201611071738.uA7HceYu045944@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: gonzo set sender to gonzo@FreeBSD.org using -f From: Oleksandr Tymoshenko Date: Mon, 7 Nov 2016 17:38:40 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r308424 - head/sys/arm/broadcom/bcm2835 X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 17:38:41 -0000 Author: gonzo Date: Mon Nov 7 17:38:39 2016 New Revision: 308424 URL: https://svnweb.freebsd.org/changeset/base/308424 Log: Fix locking in bcm2835_audio driver - Move all VCHI activity to worker thread: channel methods are called with non-sleepable lock held and VCHI uses sleepable lock. - In worker thread use sx(9) lock instead of mutex(9) for the same reason. PR: 213801, 205979 Modified: head/sys/arm/broadcom/bcm2835/bcm2835_audio.c Modified: head/sys/arm/broadcom/bcm2835/bcm2835_audio.c ============================================================================== --- head/sys/arm/broadcom/bcm2835/bcm2835_audio.c Mon Nov 7 17:34:19 2016 (r308423) +++ head/sys/arm/broadcom/bcm2835/bcm2835_audio.c Mon Nov 7 17:38:39 2016 (r308424) @@ -104,14 +104,17 @@ struct bcm2835_audio_info { struct intr_config_hook intr_hook; /* VCHI data */ - struct mtx vchi_lock; + struct sx vchi_lock; VCHI_INSTANCE_T vchi_instance; VCHI_CONNECTION_T *vchi_connection; VCHI_SERVICE_HANDLE_T vchi_handle; - struct mtx data_lock; - struct cv data_cv; + struct sx worker_lock; + struct cv worker_cv; + + bool parameters_update_pending; + bool controls_update_pending; /* Unloadign module */ int unloading; @@ -121,8 +124,8 @@ struct bcm2835_audio_info { #define bcm2835_audio_unlock(_ess) snd_mtxunlock((_ess)->lock) #define bcm2835_audio_lock_assert(_ess) snd_mtxassert((_ess)->lock) -#define VCHIQ_VCHI_LOCK(sc) mtx_lock(&(sc)->vchi_lock) -#define VCHIQ_VCHI_UNLOCK(sc) mtx_unlock(&(sc)->vchi_lock) +#define VCHIQ_VCHI_LOCK(sc) sx_xlock(&(sc)->vchi_lock) +#define VCHIQ_VCHI_UNLOCK(sc) sx_xunlock(&(sc)->vchi_lock) static const char * dest_description(uint32_t dest) @@ -175,7 +178,7 @@ bcm2835_audio_callback(void *param, cons chn_intr(sc->pch.channel); if (perr || ch->free_buffer >= VCHIQ_AUDIO_PACKET_SIZE) - cv_signal(&sc->data_cv); + cv_signal(&sc->worker_cv); } else printf("%s: unknown m.type: %d\n", __func__, m.type); } @@ -261,8 +264,6 @@ bcm2835_audio_start(struct bcm2835_audio if (sc->vchi_handle != VCHIQ_SERVICE_HANDLE_INVALID) { vchi_service_use(sc->vchi_handle); - bcm2835_audio_reset_channel(ch); - m.type = VC_AUDIO_MSG_TYPE_START; ret = vchi_msg_queue(sc->vchi_handle, &m, sizeof m, VCHI_FLAGS_BLOCK_UNTIL_QUEUED, NULL); @@ -324,7 +325,7 @@ bcm2835_audio_open(struct bcm2835_audio_ } static void -bcm2835_audio_update_controls(struct bcm2835_audio_info *sc) +bcm2835_audio_update_controls(struct bcm2835_audio_info *sc, uint32_t volume, uint32_t dest) { VC_AUDIO_MSG_T m; int ret, db; @@ -334,10 +335,10 @@ bcm2835_audio_update_controls(struct bcm vchi_service_use(sc->vchi_handle); m.type = VC_AUDIO_MSG_TYPE_CONTROL; - m.u.control.dest = sc->dest; - if (sc->volume > 99) - sc->volume = 99; - db = db_levels[sc->volume/5]; + m.u.control.dest = dest; + if (volume > 99) + volume = 99; + db = db_levels[volume/5]; m.u.control.volume = VCHIQ_AUDIO_VOLUME(db); ret = vchi_msg_queue(sc->vchi_handle, @@ -352,7 +353,7 @@ bcm2835_audio_update_controls(struct bcm } static void -bcm2835_audio_update_params(struct bcm2835_audio_info *sc, struct bcm2835_audio_chinfo *ch) +bcm2835_audio_update_params(struct bcm2835_audio_info *sc, uint32_t fmt, uint32_t speed) { VC_AUDIO_MSG_T m; int ret; @@ -362,9 +363,9 @@ bcm2835_audio_update_params(struct bcm28 vchi_service_use(sc->vchi_handle); m.type = VC_AUDIO_MSG_TYPE_CONFIG; - m.u.config.channels = AFMT_CHANNEL(ch->fmt); - m.u.config.samplerate = ch->spd; - m.u.config.bps = AFMT_BIT(ch->fmt); + m.u.config.channels = AFMT_CHANNEL(fmt); + m.u.config.samplerate = speed; + m.u.config.bps = AFMT_BIT(fmt); ret = vchi_msg_queue(sc->vchi_handle, &m, sizeof m, VCHI_FLAGS_BLOCK_UNTIL_QUEUED, NULL); @@ -474,29 +475,61 @@ bcm2835_audio_worker(void *data) { struct bcm2835_audio_info *sc = (struct bcm2835_audio_info *)data; struct bcm2835_audio_chinfo *ch = &sc->pch; - mtx_lock(&sc->data_lock); + uint32_t speed, format; + uint32_t volume, dest; + bool parameters_changed, controls_changed; + + sx_slock(&sc->worker_lock); while(1) { if (sc->unloading) break; + parameters_changed = false; + controls_changed = false; + bcm2835_audio_lock(sc); + if (sc->parameters_update_pending) { + /* TODO: update parameters */ + speed = ch->spd; + format = ch->fmt; + sc->parameters_update_pending = false; + parameters_changed = true; + } + + if (sc->controls_update_pending) { + volume = sc->volume; + dest = sc->dest; + sc->controls_update_pending = false; + controls_changed = true; + } + + bcm2835_audio_unlock(sc); + + if (parameters_changed) { + bcm2835_audio_update_params(sc, format, speed); + } + + if (controls_changed) { + bcm2835_audio_update_controls(sc, volume, dest); + } + if (ch->playback_state == PLAYBACK_IDLE) { - cv_wait_sig(&sc->data_cv, &sc->data_lock); + cv_wait_sig(&sc->worker_cv, &sc->worker_lock); continue; } if (ch->playback_state == PLAYBACK_STOPPING) { + bcm2835_audio_stop(ch); bcm2835_audio_reset_channel(&sc->pch); ch->playback_state = PLAYBACK_IDLE; continue; } if (ch->free_buffer < vchiq_unbuffered_bytes(ch)) { - cv_timedwait_sig(&sc->data_cv, &sc->data_lock, 10); + cv_timedwait_sig(&sc->worker_cv, &sc->worker_lock, 10); continue; } - bcm2835_audio_write_samples(ch); if (ch->playback_state == PLAYBACK_STARTING) { @@ -507,7 +540,7 @@ bcm2835_audio_worker(void *data) } } } - mtx_unlock(&sc->data_lock); + sx_sunlock(&sc->worker_lock); kproc_exit(0); } @@ -547,11 +580,13 @@ bcmchan_init(kobj_t obj, void *devinfo, buffer = malloc(sc->bufsz, M_DEVBUF, M_WAITOK | M_ZERO); if (sndbuf_setup(ch->buffer, buffer, sc->bufsz) != 0) { + device_printf(sc->dev, "sndbuf_setup failed\n"); free(buffer, M_DEVBUF); return NULL; } - bcm2835_audio_update_params(sc, ch); + sc->parameters_update_pending = true; + cv_signal(&sc->worker_cv); return ch; } @@ -576,12 +611,12 @@ bcmchan_setformat(kobj_t obj, void *data struct bcm2835_audio_info *sc = ch->parent; bcm2835_audio_lock(sc); - ch->fmt = format; - bcm2835_audio_update_params(sc, ch); - + sc->parameters_update_pending = true; bcm2835_audio_unlock(sc); + cv_signal(&sc->worker_cv); + return 0; } @@ -592,12 +627,12 @@ bcmchan_setspeed(kobj_t obj, void *data, struct bcm2835_audio_info *sc = ch->parent; bcm2835_audio_lock(sc); - ch->spd = speed; - bcm2835_audio_update_params(sc, ch); - + sc->parameters_update_pending = true; bcm2835_audio_unlock(sc); + cv_signal(&sc->worker_cv); + return ch->spd; } @@ -618,26 +653,30 @@ bcmchan_trigger(kobj_t obj, void *data, if (!PCMTRIG_COMMON(go)) return (0); - bcm2835_audio_lock(sc); switch (go) { case PCMTRIG_START: + bcm2835_audio_lock(sc); + bcm2835_audio_reset_channel(ch); ch->playback_state = PLAYBACK_STARTING; + bcm2835_audio_unlock(sc); + /* kickstart data flow */ + chn_intr(sc->pch.channel); /* wakeup worker thread */ - cv_signal(&sc->data_cv); + cv_signal(&sc->worker_cv); break; case PCMTRIG_STOP: case PCMTRIG_ABORT: + bcm2835_audio_lock(sc); ch->playback_state = PLAYBACK_STOPPING; - bcm2835_audio_stop(ch); + bcm2835_audio_unlock(sc); + cv_signal(&sc->worker_cv); break; default: break; } - - bcm2835_audio_unlock(sc); return 0; } @@ -695,8 +734,11 @@ bcmmix_set(struct snd_mixer *m, unsigned switch (dev) { case SOUND_MIXER_VOLUME: + bcm2835_audio_lock(sc); sc->volume = left; - bcm2835_audio_update_controls(sc); + sc->controls_update_pending = true; + bcm2835_audio_unlock(sc); + cv_signal(&sc->worker_cv); break; default: @@ -729,9 +771,13 @@ sysctl_bcm2835_audio_dest(SYSCTL_HANDLER if ((val < 0) || (val > 2)) return (EINVAL); + bcm2835_audio_lock(sc); sc->dest = val; + sc->controls_update_pending = true; + bcm2835_audio_unlock(sc); + + cv_signal(&sc->worker_cv); device_printf(sc->dev, "destination set to %s\n", dest_description(val)); - bcm2835_audio_update_controls(sc); return (0); } @@ -821,9 +867,9 @@ bcm2835_audio_attach(device_t dev) sc->lock = snd_mtxcreate(device_get_nameunit(dev), "bcm2835_audio softc"); - mtx_init(&sc->vchi_lock, "bcm2835_audio", "vchi_lock", MTX_DEF); - mtx_init(&sc->data_lock, "data_mtx", "data_mtx", MTX_DEF); - cv_init(&sc->data_cv, "data_cv"); + sx_init(&sc->vchi_lock, device_get_nameunit(dev)); + sx_init(&sc->worker_lock, "bcm_audio_worker_lock"); + cv_init(&sc->worker_cv, "worker_cv"); sc->vchi_handle = VCHIQ_SERVICE_HANDLE_INVALID; /* @@ -850,16 +896,18 @@ bcm2835_audio_detach(device_t dev) sc = pcm_getdevinfo(dev); /* Stop worker thread */ + sx_xlock(&sc->worker_lock); sc->unloading = 1; - cv_signal(&sc->data_cv); + sx_xunlock(&sc->worker_lock); + cv_signal(&sc->worker_cv); r = pcm_unregister(dev); if (r) return r; - mtx_destroy(&sc->vchi_lock); - mtx_destroy(&sc->data_lock); - cv_destroy(&sc->data_cv); + sx_destroy(&sc->vchi_lock); + sx_destroy(&sc->worker_lock); + cv_destroy(&sc->worker_cv); bcm2835_audio_release(sc);