[alsa-devel] ESI Juli@ crash with external clock switch - patch
Takashi Iwai
tiwai at suse.de
Fri Jan 16 18:19:22 CET 2015
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;
}
More information about the Alsa-devel
mailing list