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; }