[alsa-devel] [PATCH] ALSA: hda - Fix runtime PM accounting
Takashi Iwai
tiwai at suse.de
Wed Aug 29 16:59:16 CEST 2012
At Wed, 29 Aug 2012 15:34:35 +0200,
Takashi Iwai wrote:
>
> At Wed, 29 Aug 2012 12:19:32 +0000,
> Lin, Mengdong wrote:
> >
> > > Now I introduced codec->epss flag that is initialized once in
> > > snd_hda_codec_new(), and let the codec driver overrides this flag.
> > > One bonus is that you can avoid the unnecessary codec parameter read at
> > > each power change.
> >
> > Yes, this is better. And the patch works well for me - [PATCH 1/2] ALSA: hda - Avoid unnecessary parameter read for EPSS.
> >
> > > In anyway, there are a few issues:
> > > - The refcounting gets broken after module unload. It's because we
> > > leave the codec's pm_runtime usages after removal.
> >
> > Thanks for pointing out this! But I think the following codec can introduce risk of race among multiple codecs to modify ' chip->power_count',
> > because only one codec's power on/off is serialized.
> > static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec)
> > {
> > ...
> > if (codec->power_on) {
> > pm_runtime_get_sync(&chip->pci->dev);
> > chip->power_count++;
> > } else {
> > pm_runtime_put_sync(&chip->pci->dev);
> > chip->power_count--;
> > }
> > }
> >
> > How about calling pm_runtime_put_noidle() when freeing a codec in D0?
> > Maybe we can add a bus notify function like this:
> > Static void azx_codec_free_notify(struct hda_codec *codec)
> > {
> > If(codec->power_on)
> > pm_runtime_put_noidle(&codec->bus->pci->dev);
> > }
> > And snd_hda_codec_free() can trigger this notify.
>
> That's my second idea but I didn't want to introduce yet another op
> point. But maybe it shall be the choice in the end...
On the second thought, another reason I didn't take that approach was
that the code snippet you suggested in the above won't work
correctly. snd_hda_codec_free() is called also in the error path. In
that case, it's before calling pm_runtime_get_noresume().
Also, depending on which place to call the callback, refcounts might
get broken.
The patch below is the revised version. I think it's a good cleanup
at the same time. Basically it makes pm_notify callback simply
turning up/down the controller power without checking extra flags
there. The conditions are checked in the caller side instead.
The pm_notify is called at codec creation and deletion times (but
without checking extra flags), thus the refcounts can be managed more
easily.
Takashi
---
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH v3] ALSA: hda - Fix runtime PM leftover refcounts
When the HD-audio is removed, it leaves the refcounts when codecs are
powered up (usually yes) in the destructor. For fixing the unbalance,
and cleaning up the code mess, this patch changes the following:
- change pm_notify callback to take the explicit power on/off state,
- check of D3 stop-clock and keep_link_on flags is moved to the caller
side,
- call pm_notify callback in snd_hda_codec_new() and snd_hda_codec_free()
so that the refcounts are proprely updated.
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
sound/pci/hda/hda_codec.c | 18 +++++++++++++-----
sound/pci/hda/hda_codec.h | 2 +-
sound/pci/hda/hda_intel.c | 19 +++----------------
3 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 1b35115..2445148 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -98,9 +98,15 @@ EXPORT_SYMBOL_HDA(snd_hda_delete_codec_preset);
static void hda_power_work(struct work_struct *work);
static void hda_keep_power_on(struct hda_codec *codec);
#define hda_codec_is_power_on(codec) ((codec)->power_on)
+static inline void hda_call_pm_notify(struct hda_bus *bus, bool power_up)
+{
+ if (bus->ops.pm_notify)
+ bus->ops.pm_notify(bus, power_up);
+}
#else
static inline void hda_keep_power_on(struct hda_codec *codec) {}
#define hda_codec_is_power_on(codec) 1
+#define hda_call_pm_notify(bus, state) {}
#endif
/**
@@ -1200,6 +1206,7 @@ static void snd_hda_codec_free(struct hda_codec *codec)
if (codec->patch_ops.free)
codec->patch_ops.free(codec);
module_put(codec->owner);
+ hda_call_pm_notify(codec->bus, false);
free_hda_cache(&codec->amp_cache);
free_hda_cache(&codec->cmd_cache);
kfree(codec->vendor_name);
@@ -1271,6 +1278,7 @@ int /*__devinit*/ snd_hda_codec_new(struct hda_bus *bus,
* phase.
*/
hda_keep_power_on(codec);
+ hda_call_pm_notify(bus, true);
#endif
if (codec->bus->modelname) {
@@ -3576,7 +3584,7 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg,
}
#ifdef CONFIG_SND_HDA_POWER_SAVE
- if ((power_state == AC_PWRST_D3)
+ if (!codec->bus->power_keep_link_on && power_state == AC_PWRST_D3
&& codec->d3_stop_clk && (state & AC_PWRST_CLK_STOP_OK))
codec->d3_stop_clk_ok = 1;
#endif
@@ -4430,8 +4438,8 @@ static void hda_power_work(struct work_struct *work)
spin_unlock(&codec->power_lock);
hda_call_codec_suspend(codec);
- if (bus->ops.pm_notify)
- bus->ops.pm_notify(bus, codec);
+ if (codec->d3_stop_clk_ok)
+ hda_call_pm_notify(bus, false);
}
static void hda_keep_power_on(struct hda_codec *codec)
@@ -4488,8 +4496,8 @@ static void __snd_hda_power_up(struct hda_codec *codec, bool wait_power_down)
codec->power_transition = 1; /* avoid reentrance */
spin_unlock(&codec->power_lock);
- if (bus->ops.pm_notify)
- bus->ops.pm_notify(bus, codec);
+ if (codec->d3_stop_clk_ok) /* flag set at suspend */
+ hda_call_pm_notify(bus, true);
hda_call_codec_resume(codec);
spin_lock(&codec->power_lock);
diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h
index 72477cc..2e5a22f 100644
--- a/sound/pci/hda/hda_codec.h
+++ b/sound/pci/hda/hda_codec.h
@@ -616,7 +616,7 @@ struct hda_bus_ops {
void (*bus_reset)(struct hda_bus *bus);
#ifdef CONFIG_SND_HDA_POWER_SAVE
/* notify power-up/down from codec to controller */
- void (*pm_notify)(struct hda_bus *bus, struct hda_codec *codec);
+ void (*pm_notify)(struct hda_bus *bus, bool power_up);
#endif
};
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 1c9c779..1b6e856 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1033,7 +1033,7 @@ static unsigned int azx_get_response(struct hda_bus *bus,
}
#ifdef CONFIG_SND_HDA_POWER_SAVE
-static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec);
+static void azx_power_notify(struct hda_bus *bus, bool power_up);
#endif
/* reset codec link */
@@ -2406,14 +2406,11 @@ static void azx_stop_chip(struct azx *chip)
#ifdef CONFIG_SND_HDA_POWER_SAVE
/* power-up/down the controller */
-static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec)
+static void azx_power_notify(struct hda_bus *bus, bool power_up)
{
struct azx *chip = bus->private_data;
- if (bus->power_keep_link_on || !codec->d3_stop_clk_ok)
- return;
-
- if (codec->power_on)
+ if (power_up)
pm_runtime_get_sync(&chip->pci->dev);
else
pm_runtime_put_sync(&chip->pci->dev);
@@ -3273,15 +3270,6 @@ static void azx_firmware_cb(const struct firmware *fw, void *context)
}
#endif
-static void rpm_get_all_codecs(struct azx *chip)
-{
- struct hda_codec *codec;
-
- list_for_each_entry(codec, &chip->bus->codec_list, list) {
- pm_runtime_get_noresume(&chip->pci->dev);
- }
-}
-
static int __devinit azx_probe(struct pci_dev *pci,
const struct pci_device_id *pci_id)
{
@@ -3388,7 +3376,6 @@ static int DELAYED_INIT_MARK azx_probe_continue(struct azx *chip)
goto out_free;
chip->running = 1;
- rpm_get_all_codecs(chip); /* all codecs are active */
power_down_all_codecs(chip);
azx_notifier_register(chip);
azx_add_card_list(chip);
--
1.7.11.4
More information about the Alsa-devel
mailing list