[alsa-devel] [PATCH 0/3] ALSA: PM-related fixes/cleanups
Hi,
this is a patch set addressing some minor issues found in PM-related stuff, as well as the Intel HDMI fallback issue. The first patch covers the unusual error paths from runtime PM, the second one is a code refactoring and the last one fixes the superfluous (rather harmful) binding with the generic HDMI driver as fallback of i915 component binding.
Takashi
===
Takashi Iwai (3): ALSA: hda - Handle error from snd_hda_power_up*() ALSA: hda - Move in_pm accessors to HDA core ALSA: hda/hdmi - Don't fall back to generic when i915 binding fails
include/sound/hdaudio.h | 27 +++++++++++++++++ sound/pci/hda/hda_beep.c | 3 +- sound/pci/hda/hda_codec.c | 25 ++++++---------- sound/pci/hda/hda_controller.c | 4 ++- sound/pci/hda/hda_proc.c | 3 +- sound/pci/hda/hda_sysfs.c | 4 ++- sound/pci/hda/patch_ca0132.c | 53 +++++++++++++++++++++++++++------- sound/pci/hda/patch_hdmi.c | 4 ++- sound/pci/hda/patch_realtek.c | 3 +- 9 files changed, 93 insertions(+), 33 deletions(-)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_beep.c | 3 +- sound/pci/hda/hda_codec.c | 4 ++- sound/pci/hda/hda_controller.c | 4 ++- sound/pci/hda/hda_proc.c | 3 +- sound/pci/hda/hda_sysfs.c | 4 ++- sound/pci/hda/patch_ca0132.c | 53 +++++++++++++++++++++++++++------- sound/pci/hda/patch_realtek.c | 3 +- 7 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 066b5b59c4d7..e9d5fbd6c13a 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone) struct hda_codec *codec = beep->codec;
if (tone && !beep->playing) { - snd_hda_power_up(codec); + if (snd_hda_power_up(codec) < 0) + return; if (beep->power_hook) beep->power_hook(beep, true); beep->playing = 1; diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 20a171ac4bb2..44165f3e344e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd, return -1;
again: - snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err; mutex_lock(&bus->core.cmd_mutex); if (flags & HDA_RW_NO_RESPONSE_FALLBACK) bus->no_response_fallback = 1; diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index a12e594d4e3b..4273be1f3eaa 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream) buff_step); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, buff_step); - snd_hda_power_up(apcm->codec); + err = snd_hda_power_up(apcm->codec); + if (err < 0) + return err; if (hinfo->ops.open) err = hinfo->ops.open(hinfo, apcm->codec, substream); else diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c index c6b778b2580c..4206749d5130 100644 --- a/sound/pci/hda/hda_proc.c +++ b/sound/pci/hda/hda_proc.c @@ -760,7 +760,8 @@ static void print_codec_info(struct snd_info_entry *entry, fg = codec->core.afg; if (!fg) return; - snd_hda_power_up(codec); + if (snd_hda_power_up(codec) < 0) + return; snd_iprintf(buffer, "Default PCM:\n"); print_pcm_caps(buffer, codec, fg); snd_iprintf(buffer, "Default Amp-In caps: "); diff --git a/sound/pci/hda/hda_sysfs.c b/sound/pci/hda/hda_sysfs.c index 6ec79c58d48d..d17a808e72d2 100644 --- a/sound/pci/hda/hda_sysfs.c +++ b/sound/pci/hda/hda_sysfs.c @@ -130,7 +130,9 @@ static int reconfig_codec(struct hda_codec *codec) { int err;
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; codec_info(codec, "hda-codec: reconfiguring\n"); err = snd_hda_codec_reset(codec); if (err < 0) { diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 4ff5320378e2..3b58f6032250 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -3575,12 +3575,15 @@ static int tuning_ctl_set(struct hda_codec *codec, hda_nid_t nid, unsigned int *lookup, int idx) { int i = 0; + int err;
for (i = 0; i < TUNING_CTLS_COUNT; i++) if (nid == ca0132_tuning_ctls[i].nid) break;
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; dspio_set_param(codec, ca0132_tuning_ctls[i].mid, 0x20, ca0132_tuning_ctls[i].req, &(lookup[idx]), sizeof(unsigned int)); @@ -3801,7 +3804,9 @@ static int ca0132_select_out(struct hda_codec *codec)
codec_dbg(codec, "ca0132_select_out\n");
- snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err;
auto_jack = spec->vnode_lswitch[VNID_HP_ASEL - VNODE_START_NID];
@@ -3914,7 +3919,9 @@ static int ca0132_alt_select_out(struct hda_codec *codec)
codec_dbg(codec, "%s\n", __func__);
- snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err;
auto_jack = spec->vnode_lswitch[VNID_HP_ASEL - VNODE_START_NID];
@@ -4225,10 +4232,13 @@ static int ca0132_select_mic(struct hda_codec *codec) struct ca0132_spec *spec = codec->spec; int jack_present; int auto_jack; + int err;
codec_dbg(codec, "ca0132_select_mic\n");
- snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err;
auto_jack = spec->vnode_lswitch[VNID_AMIC1_ASEL - VNODE_START_NID];
@@ -4276,10 +4286,13 @@ static int ca0132_alt_select_in(struct hda_codec *codec) { struct ca0132_spec *spec = codec->spec; unsigned int tmp; + int err;
codec_dbg(codec, "%s\n", __func__);
- snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err;
chipio_set_stream_control(codec, 0x03, 0); chipio_set_stream_control(codec, 0x04, 0); @@ -4701,6 +4714,8 @@ static int ca0132_alt_slider_ctl_set(struct hda_codec *codec, hda_nid_t nid, { int i = 0; unsigned int y; + int err; + /* * For X_BASS, req 2 is actually crossover freq instead of * effect level @@ -4710,7 +4725,9 @@ static int ca0132_alt_slider_ctl_set(struct hda_codec *codec, hda_nid_t nid, else y = 1;
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; if (nid == XBASS_XOVER) { for (i = 0; i < OUT_EFFECTS_COUNT; i++) if (ca0132_effects[i].nid == X_BASS) @@ -5221,11 +5238,14 @@ static int ca0132_switch_put(struct snd_kcontrol *kcontrol, int ch = get_amp_channels(kcontrol); long *valp = ucontrol->value.integer.value; int changed = 1; + int err;
codec_dbg(codec, "ca0132_switch_put: nid=0x%x, val=%ld\n", nid, *valp);
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; /* vnode */ if ((nid >= VNODE_START_NID) && (nid < VNODE_END_NID)) { if (ch & 1) { @@ -5390,6 +5410,7 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol, hda_nid_t shared_nid = 0; bool effective; int changed = 1; + int err;
/* store the left and right volume */ if (ch & 1) { @@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol, int dir = get_amp_direction(kcontrol); unsigned long pval;
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; mutex_lock(&codec->control_mutex); pval = kcontrol->private_value; kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch, @@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, long *valp = ucontrol->value.integer.value; hda_nid_t vnid = 0; int changed = 1; + int err;
switch (nid) { case 0x02: @@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, valp++; }
- snd_hda_power_up(codec); + err = snd_hda_power_up(codec); + if (err < 0) + return err; ca0132_alt_dsp_volume_put(codec, vnid); mutex_lock(&codec->control_mutex); changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol); @@ -7190,6 +7216,7 @@ static int ca0132_init(struct hda_codec *codec) struct auto_pin_cfg *cfg = &spec->autocfg; int i; bool dsp_loaded; + int err;
/* * If the DSP is already downloaded, and init has been entered again, @@ -7220,7 +7247,9 @@ static int ca0132_init(struct hda_codec *codec) if (spec->quirk == QUIRK_SBZ) sbz_region2_startup(codec);
- snd_hda_power_up_pm(codec); + err = snd_hda_power_up_pm(codec); + if (err < 0) + return err;
ca0132_init_unsol(codec); ca0132_init_params(codec); @@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec) struct ca0132_spec *spec = codec->spec;
cancel_delayed_work_sync(&spec->unsol_hp_work); - snd_hda_power_up(codec); + if (snd_hda_power_up(codec) < 0) + goto skip_shutdown; switch (spec->quirk) { case QUIRK_SBZ: sbz_exit_chip(codec); @@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec) break; } snd_hda_power_down(codec); + skip_shutdown: if (spec->mem_base) iounmap(spec->mem_base); kfree(spec->spec_init_verbs); diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 5ad6c7e5f92e..b0d757cf7aab 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled) pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80; if (spec->mute_led_nid) { /* temporarily power up/down for setting VREF */ - snd_hda_power_up_pm(codec); + if (snd_hda_power_up_pm(codec) < 0) + return; snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval); snd_hda_power_down_pm(codec); }
Quoting Takashi Iwai (2018-06-27 10:10:32)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
@@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol, int dir = get_amp_direction(kcontrol); unsigned long pval;
snd_hda_power_up(codec);
err = snd_hda_power_up(codec);if (err < 0)return err; mutex_lock(&codec->control_mutex); pval = kcontrol->private_value; kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,
Should this be deferred until pm is next acquired? Or are we regarding pm can only fail nonrecoverably?
@@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, long *valp = ucontrol->value.integer.value; hda_nid_t vnid = 0; int changed = 1;
int err; switch (nid) { case 0x02:@@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, valp++; }
snd_hda_power_up(codec);
err = snd_hda_power_up(codec);if (err < 0)return err; ca0132_alt_dsp_volume_put(codec, vnid); mutex_lock(&codec->control_mutex); changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 5ad6c7e5f92e..b0d757cf7aab 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled) pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80; if (spec->mute_led_nid) { /* temporarily power up/down for setting VREF */
snd_hda_power_up_pm(codec);
if (snd_hda_power_up_pm(codec) < 0)return; snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval); snd_hda_power_down_pm(codec); }
Similar questions as for deferred application.
I did not find any imbalance introduced and the error is propagated or handled, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Wed, 27 Jun 2018 11:36:12 +0200, Chris Wilson wrote:
Quoting Takashi Iwai (2018-06-27 10:10:32)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
@@ -5407,7 +5428,9 @@ static int ca0132_volume_put(struct snd_kcontrol *kcontrol, int dir = get_amp_direction(kcontrol); unsigned long pval;
snd_hda_power_up(codec);
err = snd_hda_power_up(codec);if (err < 0)return err; mutex_lock(&codec->control_mutex); pval = kcontrol->private_value; kcontrol->private_value = HDA_COMPOSE_AMP_VAL(shared_nid, ch,Should this be deferred until pm is next acquired? Or are we regarding pm can only fail nonrecoverably?
The power up path is already pm_runtime_get_sync(), so it's a fatal error. So I suppose that the failure must be very exceptional, and no need to complicate things too much.
@@ -5436,6 +5459,7 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, long *valp = ucontrol->value.integer.value; hda_nid_t vnid = 0; int changed = 1;
int err; switch (nid) { case 0x02:@@ -5456,7 +5480,9 @@ static int ca0132_alt_volume_put(struct snd_kcontrol *kcontrol, valp++; }
snd_hda_power_up(codec);
err = snd_hda_power_up(codec);if (err < 0)return err; ca0132_alt_dsp_volume_put(codec, vnid); mutex_lock(&codec->control_mutex); changed = snd_hda_mixer_amp_volume_put(kcontrol, ucontrol);diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 5ad6c7e5f92e..b0d757cf7aab 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -3606,7 +3606,8 @@ static void alc269_fixup_mic_mute_hook(void *private_data, int enabled) pinval |= enabled ? AC_PINCTL_VREF_HIZ : AC_PINCTL_VREF_80; if (spec->mute_led_nid) { /* temporarily power up/down for setting VREF */
snd_hda_power_up_pm(codec);
if (snd_hda_power_up_pm(codec) < 0)return; snd_hda_set_pin_ctl_cache(codec, spec->mute_led_nid, pinval); snd_hda_power_down_pm(codec); }Similar questions as for deferred application.
I did not find any imbalance introduced and the error is propagated or handled, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Thanks!
Takashi
Quoting Takashi Iwai (2018-06-27 10:10:32)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
Verdict from CI,
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html
is that this one causes a bunch of pm fallout.
Do you mind doing a quick revert? Or working with our CI to find the bad chunk?
On a second look,
@@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec) struct ca0132_spec *spec = codec->spec;
cancel_delayed_work_sync(&spec->unsol_hp_work);
snd_hda_power_up(codec);
if (snd_hda_power_up(codec) < 0)goto skip_shutdown; switch (spec->quirk) { case QUIRK_SBZ: sbz_exit_chip(codec);@@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec) break; } snd_hda_power_down(codec);
- skip_shutdown: if (spec->mem_base) iounmap(spec->mem_base); kfree(spec->spec_init_verbs);
would seem to be the only chunk more complicated than the rest. -Chris
On Wed, 27 Jun 2018 17:54:31 +0200, Chris Wilson wrote:
Quoting Takashi Iwai (2018-06-27 10:10:32)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
Verdict from CI,
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html
is that this one causes a bunch of pm fallout.
Do you mind doing a quick revert? Or working with our CI to find the bad chunk?
Hrm, it doesn't look good -- I revert the branch merge now. Thanks for the quick heads up.
Takashi
On a second look,
@@ -7310,7 +7339,8 @@ static void ca0132_free(struct hda_codec *codec) struct ca0132_spec *spec = codec->spec;
cancel_delayed_work_sync(&spec->unsol_hp_work);
snd_hda_power_up(codec);
if (snd_hda_power_up(codec) < 0)goto skip_shutdown; switch (spec->quirk) { case QUIRK_SBZ: sbz_exit_chip(codec);@@ -7326,6 +7356,7 @@ static void ca0132_free(struct hda_codec *codec) break; } snd_hda_power_down(codec);
- skip_shutdown: if (spec->mem_base) iounmap(spec->mem_base); kfree(spec->spec_init_verbs);
would seem to be the only chunk more complicated than the rest. -Chris
On Wed, 27 Jun 2018 18:09:35 +0200, Takashi Iwai wrote:
On Wed, 27 Jun 2018 17:54:31 +0200, Chris Wilson wrote:
Quoting Takashi Iwai (2018-06-27 10:10:32)
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
Verdict from CI,
https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_2430/issues.html
is that this one causes a bunch of pm fallout.
Do you mind doing a quick revert? Or working with our CI to find the bad chunk?
Hrm, it doesn't look good -- I revert the branch merge now. Thanks for the quick heads up.
After a deeper look, I found that it's an error -EACCES from pm_runtime_get_sync(). Actually it's no real error but indicates that the runtime PM is disabled. That's the reason it broke things easily...
That is, dealing a negative code always as a fatal error is simply wrong. We may filter out -EACCES, but I think we need more careful checks. So the patch isn't worth, so far.
thanks,
Takashi
On Wed, Jun 27, 2018 at 11:10:32AM +0200, Takashi Iwai wrote:
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_beep.c | 3 +- sound/pci/hda/hda_codec.c | 4 ++- sound/pci/hda/hda_controller.c | 4 ++- sound/pci/hda/hda_proc.c | 3 +- sound/pci/hda/hda_sysfs.c | 4 ++- sound/pci/hda/patch_ca0132.c | 53 +++++++++++++++++++++++++++------- sound/pci/hda/patch_realtek.c | 3 +- 7 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 066b5b59c4d7..e9d5fbd6c13a 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone) struct hda_codec *codec = beep->codec;
if (tone && !beep->playing) {
snd_hda_power_up(codec);
if (snd_hda_power_up(codec) < 0)return;diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 20a171ac4bb2..44165f3e344e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd, return -1;
again:
- snd_hda_power_up_pm(codec);
- err = snd_hda_power_up_pm(codec);
- if (err < 0)
return err;diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index a12e594d4e3b..4273be1f3eaa 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream) buff_step); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, buff_step);
- snd_hda_power_up(apcm->codec);
- err = snd_hda_power_up(apcm->codec);
- if (err < 0)
return err;
Missing azx_release_device() here?
if (hinfo->ops.open) err = hinfo->ops.open(hinfo, apcm->codec, substream); else
On Wed, 27 Jun 2018 18:12:06 +0200, Ville Syrjälä wrote:
On Wed, Jun 27, 2018 at 11:10:32AM +0200, Takashi Iwai wrote:
Although snd_hda_power_up() and snd_hda_power_up_pm() may fail, we haven't dealt with the error properly in many places. It's an unusual situation but still possible.
This patch spots these places and adds the proper error paths.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/hda_beep.c | 3 +- sound/pci/hda/hda_codec.c | 4 ++- sound/pci/hda/hda_controller.c | 4 ++- sound/pci/hda/hda_proc.c | 3 +- sound/pci/hda/hda_sysfs.c | 4 ++- sound/pci/hda/patch_ca0132.c | 53 +++++++++++++++++++++++++++------- sound/pci/hda/patch_realtek.c | 3 +- 7 files changed, 57 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/hda_beep.c b/sound/pci/hda/hda_beep.c index 066b5b59c4d7..e9d5fbd6c13a 100644 --- a/sound/pci/hda/hda_beep.c +++ b/sound/pci/hda/hda_beep.c @@ -26,7 +26,8 @@ static void generate_tone(struct hda_beep *beep, int tone) struct hda_codec *codec = beep->codec;
if (tone && !beep->playing) {
snd_hda_power_up(codec);
if (snd_hda_power_up(codec) < 0)return;diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 20a171ac4bb2..44165f3e344e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -65,7 +65,9 @@ static int codec_exec_verb(struct hdac_device *dev, unsigned int cmd, return -1;
again:
- snd_hda_power_up_pm(codec);
- err = snd_hda_power_up_pm(codec);
- if (err < 0)
return err;diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index a12e594d4e3b..4273be1f3eaa 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -645,7 +645,9 @@ static int azx_pcm_open(struct snd_pcm_substream *substream) buff_step); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, buff_step);
- snd_hda_power_up(apcm->codec);
- err = snd_hda_power_up(apcm->codec);
- if (err < 0)
return err;Missing azx_release_device() here?
Yes, and even worse, it skips the mutex unlock :-< Let's scratch this patch.
thanks,
Takashi
Hi Takashi,
I love your patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/Takashi-Iwai/ALSA-hda-Handle-error-... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
smatch warnings: sound/pci/hda/hda_controller.c:688 azx_pcm_open() warn: inconsistent returns 'mutex:&chip->open_mutex'. Locked on: line 650 Unlocked on: line 681
# https://github.com/0day-ci/linux/commit/2e036d491f4b7a4a100b2e1cd7056fac6386... git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout 2e036d491f4b7a4a100b2e1cd7056fac63868245 vim +688 sound/pci/hda/hda_controller.c
05e84878 Dylan Reid 2014-02-28 591 05e84878 Dylan Reid 2014-02-28 592 static int azx_pcm_open(struct snd_pcm_substream *substream) 05e84878 Dylan Reid 2014-02-28 593 { 05e84878 Dylan Reid 2014-02-28 594 struct azx_pcm *apcm = snd_pcm_substream_chip(substream); 820cc6cf Takashi Iwai 2015-02-20 595 struct hda_pcm_stream *hinfo = to_hda_pcm_stream(substream); 05e84878 Dylan Reid 2014-02-28 596 struct azx *chip = apcm->chip; 05e84878 Dylan Reid 2014-02-28 597 struct azx_dev *azx_dev; 05e84878 Dylan Reid 2014-02-28 598 struct snd_pcm_runtime *runtime = substream->runtime; 05e84878 Dylan Reid 2014-02-28 599 int err; 05e84878 Dylan Reid 2014-02-28 600 int buff_step; 05e84878 Dylan Reid 2014-02-28 601 9a6246ff Takashi Iwai 2015-02-27 602 snd_hda_codec_pcm_get(apcm->info); 05e84878 Dylan Reid 2014-02-28 603 mutex_lock(&chip->open_mutex); 05e84878 Dylan Reid 2014-02-28 604 azx_dev = azx_assign_device(chip, substream); 18486508 Libin Yang 2015-05-12 605 trace_azx_pcm_open(chip, azx_dev); 05e84878 Dylan Reid 2014-02-28 606 if (azx_dev == NULL) { 61ca4107 Takashi Iwai 2015-02-27 607 err = -EBUSY; 61ca4107 Takashi Iwai 2015-02-27 608 goto unlock; 05e84878 Dylan Reid 2014-02-28 609 } ccc98865 Takashi Iwai 2015-04-14 610 runtime->private_data = azx_dev; 50279d9b Guneshwor Singh 2016-08-04 611 50279d9b Guneshwor Singh 2016-08-04 612 if (chip->gts_present) 50279d9b Guneshwor Singh 2016-08-04 613 azx_pcm_hw.info = azx_pcm_hw.info | 50279d9b Guneshwor Singh 2016-08-04 614 SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME; 50279d9b Guneshwor Singh 2016-08-04 615 05e84878 Dylan Reid 2014-02-28 616 runtime->hw = azx_pcm_hw; 05e84878 Dylan Reid 2014-02-28 617 runtime->hw.channels_min = hinfo->channels_min; 05e84878 Dylan Reid 2014-02-28 618 runtime->hw.channels_max = hinfo->channels_max; 05e84878 Dylan Reid 2014-02-28 619 runtime->hw.formats = hinfo->formats; 05e84878 Dylan Reid 2014-02-28 620 runtime->hw.rates = hinfo->rates; 05e84878 Dylan Reid 2014-02-28 621 snd_pcm_limit_hw_rates(runtime); 05e84878 Dylan Reid 2014-02-28 622 snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); 05e84878 Dylan Reid 2014-02-28 623 05e84878 Dylan Reid 2014-02-28 624 /* avoid wrap-around with wall-clock */ 05e84878 Dylan Reid 2014-02-28 625 snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_TIME, 05e84878 Dylan Reid 2014-02-28 626 20, 05e84878 Dylan Reid 2014-02-28 627 178000000); 05e84878 Dylan Reid 2014-02-28 628 05e84878 Dylan Reid 2014-02-28 629 if (chip->align_buffer_size) 05e84878 Dylan Reid 2014-02-28 630 /* constrain buffer sizes to be multiple of 128 05e84878 Dylan Reid 2014-02-28 631 bytes. This is more efficient in terms of memory 05e84878 Dylan Reid 2014-02-28 632 access but isn't required by the HDA spec and 05e84878 Dylan Reid 2014-02-28 633 prevents users from specifying exact period/buffer 05e84878 Dylan Reid 2014-02-28 634 sizes. For example for 44.1kHz, a period size set 05e84878 Dylan Reid 2014-02-28 635 to 20ms will be rounded to 19.59ms. */ 05e84878 Dylan Reid 2014-02-28 636 buff_step = 128; 05e84878 Dylan Reid 2014-02-28 637 else 05e84878 Dylan Reid 2014-02-28 638 /* Don't enforce steps on buffer sizes, still need to 05e84878 Dylan Reid 2014-02-28 639 be multiple of 4 bytes (HDA spec). Tested on Intel 05e84878 Dylan Reid 2014-02-28 640 HDA controllers, may not work on all devices where 05e84878 Dylan Reid 2014-02-28 641 option needs to be disabled */ 05e84878 Dylan Reid 2014-02-28 642 buff_step = 4; 05e84878 Dylan Reid 2014-02-28 643 05e84878 Dylan Reid 2014-02-28 644 snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 05e84878 Dylan Reid 2014-02-28 645 buff_step); 05e84878 Dylan Reid 2014-02-28 646 snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 05e84878 Dylan Reid 2014-02-28 647 buff_step); 2e036d49 Takashi Iwai 2018-06-27 648 err = snd_hda_power_up(apcm->codec); 2e036d49 Takashi Iwai 2018-06-27 649 if (err < 0) 2e036d49 Takashi Iwai 2018-06-27 650 return err; ^^^^^^^^^^ Needs to be a "goto unlock;"
61ca4107 Takashi Iwai 2015-02-27 651 if (hinfo->ops.open) 05e84878 Dylan Reid 2014-02-28 652 err = hinfo->ops.open(hinfo, apcm->codec, substream); 61ca4107 Takashi Iwai 2015-02-27 653 else 61ca4107 Takashi Iwai 2015-02-27 654 err = -ENODEV; 05e84878 Dylan Reid 2014-02-28 655 if (err < 0) { 05e84878 Dylan Reid 2014-02-28 656 azx_release_device(azx_dev); 61ca4107 Takashi Iwai 2015-02-27 657 goto powerdown; 05e84878 Dylan Reid 2014-02-28 658 } 05e84878 Dylan Reid 2014-02-28 659 snd_pcm_limit_hw_rates(runtime); 05e84878 Dylan Reid 2014-02-28 660 /* sanity check */ 05e84878 Dylan Reid 2014-02-28 661 if (snd_BUG_ON(!runtime->hw.channels_min) || 05e84878 Dylan Reid 2014-02-28 662 snd_BUG_ON(!runtime->hw.channels_max) || 05e84878 Dylan Reid 2014-02-28 663 snd_BUG_ON(!runtime->hw.formats) || 05e84878 Dylan Reid 2014-02-28 664 snd_BUG_ON(!runtime->hw.rates)) { 05e84878 Dylan Reid 2014-02-28 665 azx_release_device(azx_dev); 61ca4107 Takashi Iwai 2015-02-27 666 if (hinfo->ops.close) 05e84878 Dylan Reid 2014-02-28 667 hinfo->ops.close(hinfo, apcm->codec, substream); 61ca4107 Takashi Iwai 2015-02-27 668 err = -EINVAL; 61ca4107 Takashi Iwai 2015-02-27 669 goto powerdown; 05e84878 Dylan Reid 2014-02-28 670 } 05e84878 Dylan Reid 2014-02-28 671 9e94df3a Pierre-Louis Bossart 2015-02-13 672 /* disable LINK_ATIME timestamps for capture streams 05e84878 Dylan Reid 2014-02-28 673 until we figure out how to handle digital inputs */ 9e94df3a Pierre-Louis Bossart 2015-02-13 674 if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { 9e94df3a Pierre-Louis Bossart 2015-02-13 675 runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */ 9e94df3a Pierre-Louis Bossart 2015-02-13 676 runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME; 9e94df3a Pierre-Louis Bossart 2015-02-13 677 } 05e84878 Dylan Reid 2014-02-28 678 05e84878 Dylan Reid 2014-02-28 679 snd_pcm_set_sync(substream); 05e84878 Dylan Reid 2014-02-28 680 mutex_unlock(&chip->open_mutex); 05e84878 Dylan Reid 2014-02-28 681 return 0; 61ca4107 Takashi Iwai 2015-02-27 682 61ca4107 Takashi Iwai 2015-02-27 683 powerdown: 61ca4107 Takashi Iwai 2015-02-27 684 snd_hda_power_down(apcm->codec); 61ca4107 Takashi Iwai 2015-02-27 685 unlock: 61ca4107 Takashi Iwai 2015-02-27 686 mutex_unlock(&chip->open_mutex); 9a6246ff Takashi Iwai 2015-02-27 687 snd_hda_codec_pcm_put(apcm->info); 61ca4107 Takashi Iwai 2015-02-27 @688 return err; 05e84878 Dylan Reid 2014-02-28 689 } 05e84878 Dylan Reid 2014-02-28 690
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
The in_pm atomic in hdac_device is an important field used as a flag as well as a refcount for PM. The existing snd_hdac_power_up/down helpers already refer to it in the HD-audio core code, while the code to actually setting the value (atomic_inc() / _dec()) is open-coded in HDA legacy side, which is hard to find.
This patch adds the helper functions to set/reset the in_pm counter to HDA core and use them in HDA legacy side, for making it clearer who / where the PM is managed.
There is no functional changes, just code refactoring.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/hdaudio.h | 27 +++++++++++++++++++++++++++ sound/pci/hda/hda_codec.c | 21 ++++++--------------- sound/pci/hda/patch_hdmi.c | 2 +- 3 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index c052afc27547..294a5a21937b 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -8,6 +8,7 @@
#include <linux/device.h> #include <linux/interrupt.h> +#include <linux/pm_runtime.h> #include <linux/timecounter.h> #include <sound/core.h> #include <sound/memalloc.h> @@ -171,12 +172,38 @@ int snd_hdac_power_down(struct hdac_device *codec); int snd_hdac_power_up_pm(struct hdac_device *codec); int snd_hdac_power_down_pm(struct hdac_device *codec); int snd_hdac_keep_power_up(struct hdac_device *codec); + +/* call this at entering into suspend/resume callbacks in codec driver */ +static inline void snd_hdac_enter_pm(struct hdac_device *codec) +{ + atomic_inc(&codec->in_pm); +} + +/* call this at leaving from suspend/resume callbacks in codec driver */ +static inline void snd_hdac_leave_pm(struct hdac_device *codec) +{ + atomic_dec(&codec->in_pm); +} + +static inline bool snd_hdac_is_in_pm(struct hdac_device *codec) +{ + return atomic_read(&codec->in_pm); +} + +static inline bool snd_hdac_is_power_on(struct hdac_device *codec) +{ + return !pm_runtime_suspended(&codec->dev); +} #else static inline int snd_hdac_power_up(struct hdac_device *codec) { return 0; } static inline int snd_hdac_power_down(struct hdac_device *codec) { return 0; } static inline int snd_hdac_power_up_pm(struct hdac_device *codec) { return 0; } static inline int snd_hdac_power_down_pm(struct hdac_device *codec) { return 0; } static inline int snd_hdac_keep_power_up(struct hdac_device *codec) { return 0; } +static inline void snd_hdac_enter_pm(struct hdac_device *codec) {} +static inline void snd_hdac_leave_pm(struct hdac_device *codec) {} +static inline bool snd_hdac_is_in_pm(struct hdac_device *codec) { return 0; } +static inline bool snd_hdac_is_power_on(struct hdac_device *codec) { return 1; } #endif
/* diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 44165f3e344e..27ce6ff6354e 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -37,15 +37,8 @@ #include "hda_jack.h" #include <sound/hda_hwdep.h>
-#ifdef CONFIG_PM -#define codec_in_pm(codec) atomic_read(&(codec)->core.in_pm) -#define hda_codec_is_power_on(codec) \ - (!pm_runtime_suspended(hda_codec_dev(codec))) -#else -#define codec_in_pm(codec) 0 -#define hda_codec_is_power_on(codec) 1 -#endif - +#define codec_in_pm(codec) snd_hdac_is_in_pm(&codec->core) +#define hda_codec_is_power_on(codec) snd_hdac_is_power_on(&codec->core) #define codec_has_epss(codec) \ ((codec)->core.power_caps & AC_PWRST_EPSS) #define codec_has_clkstop(codec) \ @@ -2848,14 +2841,13 @@ static unsigned int hda_call_codec_suspend(struct hda_codec *codec) { unsigned int state;
- atomic_inc(&codec->core.in_pm); - + snd_hdac_enter_pm(&codec->core); if (codec->patch_ops.suspend) codec->patch_ops.suspend(codec); hda_cleanup_all_streams(codec); state = hda_set_power_state(codec, AC_PWRST_D3); update_power_acct(codec, true); - atomic_dec(&codec->core.in_pm); + snd_hdac_leave_pm(&codec->core); return state; }
@@ -2864,8 +2856,7 @@ static unsigned int hda_call_codec_suspend(struct hda_codec *codec) */ static void hda_call_codec_resume(struct hda_codec *codec) { - atomic_inc(&codec->core.in_pm); - + snd_hdac_enter_pm(&codec->core); if (codec->core.regmap) regcache_mark_dirty(codec->core.regmap);
@@ -2888,7 +2879,7 @@ static void hda_call_codec_resume(struct hda_codec *codec) hda_jackpoll_work(&codec->jackpoll_work.work); else snd_hda_jack_report_sync(codec); - atomic_dec(&codec->core.in_pm); + snd_hdac_leave_pm(&codec->core); }
static int hda_codec_runtime_suspend(struct device *dev) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e7d99a802bcc..c182e619ad58 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2489,7 +2489,7 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe) if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) return; /* ditto during suspend/resume process itself */ - if (atomic_read(&(codec)->core.in_pm)) + if (snd_hdac_is_in_pm(&codec->core)) return;
snd_hdac_i915_set_bclk(&codec->bus->core);
Quoting Takashi Iwai (2018-06-27 10:10:33)
The in_pm atomic in hdac_device is an important field used as a flag as well as a refcount for PM. The existing snd_hdac_power_up/down helpers already refer to it in the HD-audio core code, while the code to actually setting the value (atomic_inc() / _dec()) is open-coded in HDA legacy side, which is hard to find.
This patch adds the helper functions to set/reset the in_pm counter to HDA core and use them in HDA legacy side, for making it clearer who / where the PM is managed.
There is no functional changes, just code refactoring.
Signed-off-by: Takashi Iwai tiwai@suse.de
Mechanical code change that helps explain what it is doing, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris
On Wed, 27 Jun 2018 11:38:40 +0200, Chris Wilson wrote:
Quoting Takashi Iwai (2018-06-27 10:10:33)
The in_pm atomic in hdac_device is an important field used as a flag as well as a refcount for PM. The existing snd_hdac_power_up/down helpers already refer to it in the HD-audio core code, while the code to actually setting the value (atomic_inc() / _dec()) is open-coded in HDA legacy side, which is hard to find.
This patch adds the helper functions to set/reset the in_pm counter to HDA core and use them in HDA legacy side, for making it clearer who / where the PM is managed.
There is no functional changes, just code refactoring.
Signed-off-by: Takashi Iwai tiwai@suse.de
Mechanical code change that helps explain what it is doing, Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
This patch still seems OK, so merged to for-next branch alone.
Takashi
When i915 component binding fails, it means that HDMI isn't applicable anyway. Although the probe with the generic HDMI parser would still work, it's essentially useless, hence better to be left unbound.
This patch mimics the probe_id field at failing the i915 component binding so that the generic HDMI won't be bound after that.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index c182e619ad58..1fed59f8ed32 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2542,6 +2542,8 @@ static int alloc_intel_hdmi(struct hda_codec *codec) /* requires i915 binding */ if (!codec->bus->core.audio_component) { codec_info(codec, "No i915 binding for Intel HDMI/DP codec\n"); + /* set probe_id here to prevent generic fallback binding */ + codec->probe_id = HDA_CODEC_ID_GENERIC_HDMI; return -ENODEV; }
Quoting Takashi Iwai (2018-06-27 10:10:31)
Hi,
this is a patch set addressing some minor issues found in PM-related stuff, as well as the Intel HDMI fallback issue. The first patch covers the unusual error paths from runtime PM, the second one is a code refactoring and the last one fixes the superfluous (rather harmful) binding with the generic HDMI driver as fallback of i915 component binding.
Hmm, our CI casts a shade of doubt over these.
https://intel-gfx-ci.01.org/tree/drm-tip/combined-issues-by-run.html
From CI_DRM_4385 onwards, a small explosion
79527147382a drm-tip: 2018y-06m-27d-13h-05m-01s UTC integration manifest 67ca07e7ac10 drm/i915/icl: Add power well support 828e10e61e0a drm-tip: 2018y-06m-27d-12h-10m-06s UTC integration manifest 43c489277bdd ALSA: hda/hdmi - Don't fall back to generic when i915 binding fails bd0afa8b3573 ALSA: hda - Move in_pm accessors to HDA core 644a790aed11 ALSA: hda - Handle error from snd_hda_power_up*() 401caff70cd3 ALSA: hda - Kill snd_hda_codec_update_cache()
I haven't ruled out the icl patch, but I presume that was run past CI for pre-merge checking.
Just a heads up, -Chris
participants (4)
-
Chris Wilson -
Dan Carpenter -
Takashi Iwai -
Ville Syrjälä