| Message ID | 54B82E02.3080008@ivitera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Takashi
Dne 16.1.2015 v 18:13 Takashi Iwai napsal(a): > OK, I'm going to send a fix series including the relevant correction. > Give it a try later. Thanks a lot for the patches, the external rate switching on Juli now works perfectly (not tested on Quartet yet). One can tell you are seasoned in kernel development, I would get lost in the synchronization and workqueue facilities. >> 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. You are right, that would disable update of other controls informing about incoming SPDIF details. These are useful in internal clock mode too - if the soundcard is master for the spdif chain. A new control would make sense then. I am leaving for a week, then I will test quartet and the PM features. Again, thanks a lot for the patches. Pavel.
At Fri, 16 Jan 2015 21:36:10 +0100, Pavel Hofman wrote: > > Dne 16.1.2015 v 18:13 Takashi Iwai napsal(a): > > OK, I'm going to send a fix series including the relevant correction. > > Give it a try later. > > Thanks a lot for the patches, the external rate switching on Juli now > works perfectly (not tested on Quartet yet). One can tell you are > seasoned in kernel development, I would get lost in the synchronization > and workqueue facilities. > > >> 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. > > You are right, that would disable update of other controls informing > about incoming SPDIF details. These are useful in internal clock mode > too - if the soundcard is master for the spdif chain. A new control > would make sense then. Alright. > I am leaving for a week, then I will test quartet and the PM features. There should be no change regarding quartet, also about PM. The patches doesn't add the PM support to quartet, but rather robustify only the PM of Juli@ (and ak4113/4114 codec side support). In anyway, I'm going to merge them once when you confirm them working. thanks, Takashi
Dne 16.1.2015 v 21:39 Takashi Iwai napsal(a): > At Fri, 16 Jan 2015 21:36:10 +0100, > > There should be no change regarding quartet, also about PM. > The patches doesn't add the PM support to quartet, but rather > robustify only the PM of Juli@ (and ak4113/4114 codec side support). > > In anyway, I'm going to merge them once when you confirm them > working. OK, I will do so. Thanks, Pavel.
Dne 16.1.2015 v 21:39 Takashi Iwai napsal(a): > At Fri, 16 Jan 2015 21:36:10 +0100, > Pavel Hofman wrote: >> >> Dne 16.1.2015 v 18:13 Takashi Iwai napsal(a): >>> OK, I'm going to send a fix series including the relevant correction. >>> Give it a try later. >> >> Thanks a lot for the patches, the external rate switching on Juli now >> works perfectly (not tested on Quartet yet). One can tell you are >> seasoned in kernel development, I would get lost in the synchronization >> and workqueue facilities. >> >>>> 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. >> >> You are right, that would disable update of other controls informing >> about incoming SPDIF details. These are useful in internal clock mode >> too - if the soundcard is master for the spdif chain. A new control >> would make sense then. > > Alright. > >> I am leaving for a week, then I will test quartet and the PM features. > > There should be no change regarding quartet, also about PM. > The patches doesn't add the PM support to quartet, but rather > robustify only the PM of Juli@ (and ak4113/4114 codec side support). > > In anyway, I'm going to merge them once when you confirm them > working. I confirm the patches are working both for Juli and Quartet. Juli tested also for pm-suspend, working fine after resume. Thanks a lot, Pavel.
At Wed, 28 Jan 2015 21:51:14 +0100, Pavel Hofman wrote: > > Dne 16.1.2015 v 21:39 Takashi Iwai napsal(a): > > At Fri, 16 Jan 2015 21:36:10 +0100, > > Pavel Hofman wrote: > >> > >> Dne 16.1.2015 v 18:13 Takashi Iwai napsal(a): > >>> OK, I'm going to send a fix series including the relevant correction. > >>> Give it a try later. > >> > >> Thanks a lot for the patches, the external rate switching on Juli now > >> works perfectly (not tested on Quartet yet). One can tell you are > >> seasoned in kernel development, I would get lost in the synchronization > >> and workqueue facilities. > >> > >>>> 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. > >> > >> You are right, that would disable update of other controls informing > >> about incoming SPDIF details. These are useful in internal clock mode > >> too - if the soundcard is master for the spdif chain. A new control > >> would make sense then. > > > > Alright. > > > >> I am leaving for a week, then I will test quartet and the PM features. > > > > There should be no change regarding quartet, also about PM. > > The patches doesn't add the PM support to quartet, but rather > > robustify only the PM of Juli@ (and ak4113/4114 codec side support). > > > > In anyway, I'm going to merge them once when you confirm them > > working. > > > I confirm the patches are working both for Juli and Quartet. Juli tested > also for pm-suspend, working fine after resume. OK, I'm going to merge the patches. Thanks for testing. Takashi
Dne 28.1.2015 v 22:21 Takashi Iwai napsal(a): >> >> >> I confirm the patches are working both for Juli and Quartet. Juli tested >> also for pm-suspend, working fine after resume. > > OK, I'm going to merge the patches. > > Thanks for testing. I do thank a lot for the patches. Regards, Pavel.
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);