From patchwork Fri Jan 16 17:19:22 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takashi Iwai X-Patchwork-Id: 5649681 Return-Path: X-Original-To: patchwork-alsa-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 4CC749F358 for ; Fri, 16 Jan 2015 17:21:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2C36C20353 for ; Fri, 16 Jan 2015 17:21:15 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) by mail.kernel.org (Postfix) with ESMTP id A55932010F for ; Fri, 16 Jan 2015 17:21:13 +0000 (UTC) Received: by alsa0.perex.cz (Postfix, from userid 1000) id D31BD26049A; Fri, 16 Jan 2015 18:21:12 +0100 (CET) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from alsa0.perex.cz (localhost [IPv6:::1]) by alsa0.perex.cz (Postfix) with ESMTP id EC0AE260415; Fri, 16 Jan 2015 18:21:03 +0100 (CET) X-Original-To: alsa-devel@alsa-project.org Delivered-To: alsa-devel@alsa-project.org Received: by alsa0.perex.cz (Postfix, from userid 1000) id F10AF260427; Fri, 16 Jan 2015 18:20:57 +0100 (CET) Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id DC0F42654B5 for ; Fri, 16 Jan 2015 18:19:22 +0100 (CET) Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id B5F90AB43; Fri, 16 Jan 2015 17:19:22 +0000 (UTC) Date: Fri, 16 Jan 2015 18:19:22 +0100 Message-ID: From: Takashi Iwai To: Pavel Hofman In-Reply-To: References: <54B28174.7060008@ivitera.com> <54B2B992.7000909@ivitera.com> <54B2E48B.7020000@ivitera.com> <54B3872C.2010501@ivitera.com> <54B82E02.3080008@ivitera.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.4 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: alsa-devel Subject: Re: [alsa-devel] ESI Juli@ crash with external clock switch - patch X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org X-Virus-Scanned: ClamAV using ClamSMTP At Fri, 16 Jan 2015 18:13:09 +0100, Takashi Iwai wrote: > > At Thu, 15 Jan 2015 22:15:46 +0100, > Pavel Hofman wrote: > > > > Dne 12.1.2015 v 16:43 Takashi Iwai napsal(a): > > > At Mon, 12 Jan 2015 09:34:52 +0100, > > > Pavel Hofman wrote: > > >> > > >> > > >> I wish I could help but unfortunately my practical knowledge of kernel > > >> workqueues is close to zero :-( Of course I will test the patches and > > >> will extend them for quartet with testing too. > > > > > > How about the patch below? This is a quick fix for 3.19 (and > > > stable). More better fixes will follow later once after it's > > > confirmed to work. > > > > Hi, > > > > Thanks a lot, the patch seems to run fine. Only ak4114_init_regs has to > > be called every samplerate change, otherwise the receiver does not > > re-run the samplerate detection and its register with detected > > samplerate does not update its value. > > OK, I'm going to send a fix series including the relevant correction. > Give it a try later. > > > > > The following patch seems to run ok: > > > > diff --git a/include/sound/ak4114.h b/include/sound/ak4114.h > > index 52f02a6..796834b 100644 > > --- a/include/sound/ak4114.h > > +++ b/include/sound/ak4114.h > > @@ -169,6 +169,7 @@ struct ak4114 { > > ak4114_read_t * read; > > void * private_data; > > unsigned int init: 1; > > + bool in_workq; > > spinlock_t lock; > > unsigned char regmap[6]; > > unsigned char txcsb[5]; > > diff --git a/sound/i2c/other/ak4114.c b/sound/i2c/other/ak4114.c > > index c7f5633..6d643b7 100644 > > --- a/sound/i2c/other/ak4114.c > > +++ b/sound/i2c/other/ak4114.c > > @@ -152,10 +152,12 @@ static void ak4114_init_regs(struct ak4114 *chip) > > > > void snd_ak4114_reinit(struct ak4114 *chip) > > { > > + ak4114_init_regs(chip); > > + if (chip->in_workq) > > + return; > > chip->init = 1; > > mb(); > > - flush_delayed_work(&chip->work); > > - ak4114_init_regs(chip); > > + cancel_delayed_work_sync(&chip->work); > > /* bring up statistics / event queing */ > > chip->init = 0; > > if (chip->kctls[0]) > > @@ -612,10 +614,12 @@ static void ak4114_stats(struct work_struct *work) > > { > > struct ak4114 *chip = container_of(work, struct ak4114, work.work); > > > > - if (!chip->init) > > + chip->in_workq = true; > > + if (!chip->init) { > > snd_ak4114_check_rate_and_errors(chip, chip->check_flags); > > - > > - schedule_delayed_work(&chip->work, HZ / 10); > > + schedule_delayed_work(&chip->work, HZ / 10); > > + } > > + chip->in_workq = false; > > } > > > > EXPORT_SYMBOL(snd_ak4114_create); > > > > > > > > > >>> The HZ/10 isn't that bad, but the problem is that it's unconditionally > > >>> running even if user doesn't need/want. > > >> > > >> It is useful only for the external clock mode. In fact the detection of > > >> incoming SPDIF rate is not reliable for internal clock in Juli (while it > > >> works just fine in Quartet, its FPGA pins configure the SPDIF receiver > > >> differently). IMO the thread could be running only when clock is > > >> switched to external. > > > > > > Yeah, we can do some smart task change in addition to manual on/off. > > > Maybe it's good to have an enum control for that. > > > > I am not sure users would want/need to disable a feature which detects > > incoming samplerate. IMO if the work thread is running only in the > > external clock mode, nothing more is needed. > > Hm, but you can still see the other attributes of SPDIF input frames, > right? Or all these useless when the clock is set to internal? > If so, it'd be easy to add the dynamic turn on/off per the clock > mode. The patch below can be applied on top of the patch series I've sent. Takashi diff --git a/include/sound/ak4114.h b/include/sound/ak4114.h index b6feb7e225f2..9adcd06e4134 100644 --- a/include/sound/ak4114.h +++ b/include/sound/ak4114.h @@ -199,6 +199,7 @@ int snd_ak4114_build(struct ak4114 *ak4114, struct snd_pcm_substream *capture_substream); int snd_ak4114_external_rate(struct ak4114 *ak4114); int snd_ak4114_check_rate_and_errors(struct ak4114 *ak4114, unsigned int flags); +void snd_ak4114_enable_check_rate(struct ak4114 *chip, bool enable); #ifdef CONFIG_PM void snd_ak4114_suspend(struct ak4114 *chip); diff --git a/sound/i2c/other/ak4114.c b/sound/i2c/other/ak4114.c index 5a4cf3fab4ae..497d50e0a6d5 100644 --- a/sound/i2c/other/ak4114.c +++ b/sound/i2c/other/ak4114.c @@ -64,10 +64,21 @@ static void reg_dump(struct ak4114 *ak4114) } #endif -static void snd_ak4114_free(struct ak4114 *chip) +static void snd_ak4114_disable_work(struct ak4114 *chip) { atomic_inc(&chip->wq_processing); /* don't schedule new work */ cancel_delayed_work_sync(&chip->work); +} + +static void snd_ak4114_enable_work(struct ak4114 *chip) +{ + if (atomic_dec_and_test(&chip->wq_processing)) + schedule_delayed_work(&chip->work, HZ / 10); +} + +static void snd_ak4114_free(struct ak4114 *chip) +{ + snd_ak4114_disable_work(chip); kfree(chip); } @@ -161,8 +172,7 @@ void snd_ak4114_reinit(struct ak4114 *chip) ak4114_init_regs(chip); mutex_unlock(&chip->reinit_mutex); /* bring up statistics / event queing */ - if (atomic_dec_and_test(&chip->wq_processing)) - schedule_delayed_work(&chip->work, HZ / 10); + snd_ak4114_enable_work(chip); } EXPORT_SYMBOL(snd_ak4114_reinit); @@ -506,6 +516,7 @@ int snd_ak4114_build(struct ak4114 *ak4114, } snd_ak4114_proc_init(ak4114); /* trigger workq */ + ak4114->check_rate_enabled = true; schedule_delayed_work(&ak4114->work, HZ / 10); return 0; } @@ -621,15 +632,27 @@ static void ak4114_stats(struct work_struct *work) if (atomic_inc_return(&chip->wq_processing) == 1) snd_ak4114_check_rate_and_errors(chip, chip->check_flags); - if (atomic_dec_and_test(&chip->wq_processing)) - schedule_delayed_work(&chip->work, HZ / 10); + snd_ak4114_enable_work(chip); +} + +void snd_ak4114_enable_check_rate(struct ak4114 *chip, bool enable) +{ + mutex_lock(&chip->reinit_mutex); + changed = chip->check_rate_enabled != enable; + chip->check_rate_enabled = enable; + mutex_unlock(&chip->reinit_mutex); + if (!changed) + return; + if (enable) + snd_ak4114_enable_work(chip); + else + snd_ak4114_disable_work(chip); } #ifdef CONFIG_PM void snd_ak4114_suspend(struct ak4114 *chip) { - atomic_inc(&chip->wq_processing); /* don't schedule new work */ - cancel_delayed_work_sync(&chip->work); + snd_ak4114_disable_work(chip); } EXPORT_SYMBOL(snd_ak4114_suspend); diff --git a/sound/pci/ice1712/juli.c b/sound/pci/ice1712/juli.c index 4f0213427152..5134833d0fa8 100644 --- a/sound/pci/ice1712/juli.c +++ b/sound/pci/ice1712/juli.c @@ -475,8 +475,13 @@ static int juli_add_controls(struct snd_ice1712 *ice) return err; /* only capture SPDIF over AK4114 */ - return snd_ak4114_build(spec->ak4114, NULL, + err = snd_ak4114_build(spec->ak4114, NULL, ice->pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream); + if (err < 0) + return err; + + snd_ak4114_enable_check_rate(spec->ak4114, ice->is_spdif_master(ice)); + return 0; } /* @@ -530,6 +535,7 @@ static unsigned int juli_get_rate(struct snd_ice1712 *ice) /* setting new rate */ static void juli_set_rate(struct snd_ice1712 *ice, unsigned int rate) { + struct juli_spec *spec = ice->spec; unsigned int old, new; unsigned char val; @@ -543,6 +549,7 @@ static void juli_set_rate(struct snd_ice1712 *ice, unsigned int rate) /* switching to external clock - supplied by external circuits */ val = inb(ICEMT1724(ice, RATE)); outb(val | VT1724_SPDIF_MASTER, ICEMT1724(ice, RATE)); + snd_ak4114_enable_check_rate(spec->ak4114, false); } static inline unsigned char juli_set_mclk(struct snd_ice1712 *ice, @@ -555,11 +562,13 @@ static inline unsigned char juli_set_mclk(struct snd_ice1712 *ice, /* setting clock to external - SPDIF */ static int juli_set_spdif_clock(struct snd_ice1712 *ice, int type) { + struct juli_spec *spec = ice->spec; unsigned int old; old = ice->gpio.get_data(ice); /* external clock (= 0), multiply 1x, 48kHz */ ice->gpio.set_data(ice, (old & ~GPIO_RATE_MASK) | GPIO_MULTI_1X | GPIO_FREQ_48KHZ); + snd_ak4114_enable_check_rate(spec->ak4114, true); return 0; }