[alsa-devel] [RFC PATCH] ALSA: hda - Implement a poll loop for jacks as a module parameter
Takashi Iwai
tiwai at suse.de
Tue Oct 9 16:57:47 CEST 2012
At Tue, 09 Oct 2012 16:28:07 +0200,
David Henningsson wrote:
>
> On 10/09/2012 03:32 PM, Takashi Iwai wrote:
> > At Tue, 9 Oct 2012 15:19:44 +0200,
> > David Henningsson wrote:
> >>
> >> Now that we have a generic unsol mechanism, we can implement a generic
> >> poll loop, which can be used for debugging, or if a codec's unsol
> >> mechanism is broken.
>
> Oh, and I was also considering not turning on unsol at all (in
> snd_hda_jack_detect_enable) when this is enabled. Then it would help
> against "unstable" pins which trigger repeatedly on/off even though
> nothing is happening physically.
In that case, it's fine. But it's no broken codec's unsol mechanism.
It's a broken jack detection in hardware, i.e. it's no fault of
codec.
> >> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> >
> > The patch almost looks good, but I postpone as a 3.8 thing.
>
> Ok. I never understood the release timings, but I figured anything
> before rc1 would be okay to merge as a feature...
No, basically when the merge window opens, all new features should be
frozen. I took a few your patches yesterday just because I've been
traveling in the last weeks, and the generic unsol event code itself
is mostly nothing but a code shuffling from here to there.
> (And I couldn't start
> a week earlier; because I didn't know if you would accept the generic
> unsol event.)
> That said, it's not very important to me personally which kernel this
> goes into.
OK.
> >> I'm also considering activating it by default if we go into single_cmd mode
> >> due to codec not responding. What do you think?
> >
> > Well, I don't buy it. Falling back to single_cmd doesn't mean that we
> > are saving the world perfectly.I recently even think it might be
> > even better not to do that, otherwise people won't notice the
> > breakage.
>
> This is almost philosophical, but if a bug occurs and no user notices,
> is it really a bug? :-)
Yes, it performs worse secretly. The fallback to single_cmd is really
a last resort and it's no peaceful place to live at all.
> Or, to make it a bit more pragmatic, what other things are broken with
> single_cmd? Could you give an example where this change would be harmful?
The single cmd mode itself was introduced as a sort of rescue command
without CORB/RIRB. We shouldn't use it in normal situations. It's
simply no considered use case.
With a lack of unsol event, you can't handle the volume knob, or any
GPIO events, too.
Takashi
> >> + codec->jackpoll_interval =
> >> + msecs_to_jiffies(jackpoll_ms[chip->dev_index]);
> >
> > Better to add a sanity check of the interval value.
>
> Ok. Attaching modified patch.
Well, do you want to allow 1ms polling?
Takashi
>
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> [2 0001-ALSA-hda-Implement-a-poll-loop-for-jacks-as-a-module.patch <text/x-patch (7bit)>]
> >From 67a8012f1e8eef1b67119f8d6699ddf5b88d9fa0 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson at canonical.com>
> Date: Tue, 9 Oct 2012 15:04:21 +0200
> Subject: [PATCH] ALSA: hda - Implement a poll loop for jacks as a module
> parameter
>
> Now that we have a generic unsol mechanism, we can implement a generic
> poll loop, which can be used for debugging, or if a codec's unsol
> mechanism is broken.
>
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
> sound/pci/hda/hda_codec.c | 32 ++++++++++++++++++++++++++++----
> sound/pci/hda/hda_codec.h | 2 ++
> sound/pci/hda/hda_intel.c | 11 +++++++++++
> sound/pci/hda/hda_jack.c | 22 ++++++++++++++++++++++
> sound/pci/hda/hda_jack.h | 2 ++
> 5 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index c0ab72c..626e45b 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -1135,6 +1135,19 @@ static void restore_shutup_pins(struct hda_codec *codec)
> }
> #endif
>
> +static void hda_jackpoll_work(struct work_struct *work)
> +{
> + struct hda_codec *codec =
> + container_of(work, struct hda_codec, jackpoll_work.work);
> + if (!codec->jackpoll_interval)
> + return;
> +
> + snd_hda_jack_set_dirty_all(codec);
> + snd_hda_jack_poll_all(codec);
> + queue_delayed_work(codec->bus->workq, &codec->jackpoll_work,
> + codec->jackpoll_interval);
> +}
> +
> static void init_hda_cache(struct hda_cache_rec *cache,
> unsigned int record_size);
> static void free_hda_cache(struct hda_cache_rec *cache);
> @@ -1190,6 +1203,7 @@ static void snd_hda_codec_free(struct hda_codec *codec)
> {
> if (!codec)
> return;
> + cancel_delayed_work_sync(&codec->jackpoll_work);
> snd_hda_jack_tbl_clear(codec);
> restore_init_pincfgs(codec);
> #ifdef CONFIG_PM
> @@ -1273,6 +1287,7 @@ int /*__devinit*/ snd_hda_codec_new(struct hda_bus *bus,
> snd_array_init(&codec->cvt_setups, sizeof(struct hda_cvt_setup), 8);
> snd_array_init(&codec->conn_lists, sizeof(hda_nid_t), 64);
> snd_array_init(&codec->spdif_out, sizeof(struct hda_spdif_out), 16);
> + INIT_DELAYED_WORK(&codec->jackpoll_work, hda_jackpoll_work);
>
> #ifdef CONFIG_PM
> spin_lock_init(&codec->power_lock);
> @@ -2349,7 +2364,7 @@ int snd_hda_codec_reset(struct hda_codec *codec)
> return -EBUSY;
>
> /* OK, let it free */
> -
> + cancel_delayed_work_sync(&codec->jackpoll_work);
> #ifdef CONFIG_PM
> cancel_delayed_work_sync(&codec->power_work);
> codec->power_on = 0;
> @@ -3646,7 +3661,6 @@ static void hda_call_codec_resume(struct hda_codec *codec)
> restore_pincfgs(codec); /* restore all current pin configs */
> restore_shutup_pins(codec);
> hda_exec_init_verbs(codec);
> - snd_hda_jack_set_dirty_all(codec);
> if (codec->patch_ops.resume)
> codec->patch_ops.resume(codec);
> else {
> @@ -3655,7 +3669,13 @@ static void hda_call_codec_resume(struct hda_codec *codec)
> snd_hda_codec_resume_amp(codec);
> snd_hda_codec_resume_cache(codec);
> }
> - snd_hda_jack_report_sync(codec);
> +
> + if (codec->jackpoll_interval)
> + hda_jackpoll_work(&codec->jackpoll_work.work);
> + else {
> + snd_hda_jack_set_dirty_all(codec);
> + snd_hda_jack_report_sync(codec);
> + }
> snd_hda_power_down(codec); /* flag down before returning */
> }
> #endif /* CONFIG_PM */
> @@ -3737,7 +3757,10 @@ int snd_hda_codec_build_controls(struct hda_codec *codec)
> if (err < 0)
> return err;
>
> - snd_hda_jack_report_sync(codec); /* call at the last init point */
> + if (codec->jackpoll_interval)
> + hda_jackpoll_work(&codec->jackpoll_work.work);
> + else
> + snd_hda_jack_report_sync(codec); /* call at the last init point */
> return 0;
> }
>
> @@ -5128,6 +5151,7 @@ int snd_hda_suspend(struct hda_bus *bus)
> struct hda_codec *codec;
>
> list_for_each_entry(codec, &bus->codec_list, list) {
> + cancel_delayed_work_sync(&codec->jackpoll_work);
> if (hda_codec_is_power_on(codec))
> hda_call_codec_suspend(codec, false);
> }
> diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
> index 507fe8a..10a03b0 100644
> --- a/sound/pci/hda/hda_codec.h
> +++ b/sound/pci/hda/hda_codec.h
> @@ -884,6 +884,8 @@ struct hda_codec {
>
> /* jack detection */
> struct snd_array jacktbl;
> + unsigned long jackpoll_interval; /* In jiffies. Zero means no poll, rely on unsol events */
> + struct delayed_work jackpoll_work;
>
> #ifdef CONFIG_SND_HDA_INPUT_JACK
> /* jack detection */
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index f09ff6c..5f0f276 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -68,6 +68,7 @@ static int position_fix[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
> static int bdl_pos_adj[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
> static int probe_mask[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
> static int probe_only[SNDRV_CARDS];
> +static int jackpoll_ms[SNDRV_CARDS];
> static bool single_cmd;
> static int enable_msi = -1;
> #ifdef CONFIG_SND_HDA_PATCH_LOADER
> @@ -95,6 +96,8 @@ module_param_array(probe_mask, int, NULL, 0444);
> MODULE_PARM_DESC(probe_mask, "Bitmask to probe codecs (default = -1).");
> module_param_array(probe_only, int, NULL, 0444);
> MODULE_PARM_DESC(probe_only, "Only probing and no codec initialization.");
> +module_param_array(jackpoll_ms, int, NULL, 0444);
> +MODULE_PARM_DESC(jackpoll_ms, "Ms between polling for jack events (default = 0, using unsol events only)");
> module_param(single_cmd, bool, 0444);
> MODULE_PARM_DESC(single_cmd, "Use single command to communicate with codecs "
> "(for debugging only).");
> @@ -1597,6 +1600,7 @@ static int DELAYED_INIT_MARK azx_codec_create(struct azx *chip, const char *mode
> struct hda_bus_template bus_temp;
> int c, codecs, err;
> int max_slots;
> + unsigned int jackpoll_interval = 0;
>
> memset(&bus_temp, 0, sizeof(bus_temp));
> bus_temp.private_data = chip;
> @@ -1625,6 +1629,12 @@ static int DELAYED_INIT_MARK azx_codec_create(struct azx *chip, const char *mode
> if (!max_slots)
> max_slots = AZX_DEFAULT_CODECS;
>
> + if (jackpoll_ms[chip->dev_index] < 0 || jackpoll_ms[chip->dev_index] > 60000)
> + snd_printk(KERN_WARNING SFX "jackpoll_ms value out of range: %d\n",
> + jackpoll_ms[chip->dev_index]);
> + else if (jackpoll_ms[chip->dev_index] > 0)
> + jackpoll_interval = msecs_to_jiffies(jackpoll_ms[chip->dev_index]);
> +
> /* First try to probe all given codec slots */
> for (c = 0; c < max_slots; c++) {
> if ((chip->codec_mask & (1 << c)) & chip->codec_probe_mask) {
> @@ -1666,6 +1676,7 @@ static int DELAYED_INIT_MARK azx_codec_create(struct azx *chip, const char *mode
> err = snd_hda_codec_new(chip->bus, c, &codec);
> if (err < 0)
> continue;
> + codec->jackpoll_interval = jackpoll_interval;
> codec->beep_mode = chip->beep_mode;
> codecs++;
> }
> diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
> index 5c690cb..07d5288 100644
> --- a/sound/pci/hda/hda_jack.c
> +++ b/sound/pci/hda/hda_jack.c
> @@ -439,3 +439,25 @@ void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res)
> }
> EXPORT_SYMBOL_HDA(snd_hda_jack_unsol_event);
>
> +void snd_hda_jack_poll_all(struct hda_codec *codec)
> +{
> + struct hda_jack_tbl *jack = codec->jacktbl.list;
> + int i, changes = 0;
> +
> + for (i = 0; i < codec->jacktbl.used; i++, jack++) {
> + unsigned int old_sense;
> + if (!jack->nid || !jack->jack_dirty || jack->phantom_jack)
> + continue;
> + old_sense = get_jack_plug_state(jack->pin_sense);
> + jack_detect_update(codec, jack);
> + if (old_sense == get_jack_plug_state(jack->pin_sense))
> + continue;
> + changes = 1;
> + if (jack->callback)
> + jack->callback(codec, jack);
> + }
> + if (changes)
> + snd_hda_jack_report_sync(codec);
> +}
> +EXPORT_SYMBOL_HDA(snd_hda_jack_poll_all);
> +
> diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
> index af8dd47..4487785 100644
> --- a/sound/pci/hda/hda_jack.h
> +++ b/sound/pci/hda/hda_jack.h
> @@ -84,4 +84,6 @@ void snd_hda_jack_report_sync(struct hda_codec *codec);
>
> void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res);
>
> +void snd_hda_jack_poll_all(struct hda_codec *codec);
> +
> #endif /* __SOUND_HDA_JACK_H */
> --
> 1.7.9.5
>
More information about the Alsa-devel
mailing list