[alsa-devel] [PATCH] ALSA: hda - Fix runtime PM accounting
The flag codec->d3_clk_stop_ok isn't always set when the codec is powered off. If it's not set, the controller driver doesn't change the runtime PM count. This causes an unblanced accounting when multiple codecs are present. Usually, the first codec is powered down with CLK_STOP_OK=0, then the next codec with CLK_STOP_OK=1. At this moment, the first codec is changed also to CLK_STOP_OK=1, but this isn't informed to the controller. Thus it still thinks as if one codec is active.
For fixing this issue, move the check of CLK_STOP_OK bit at the runtime suspend time, and simple up/down by the codec power save in azx_power_notifier(). For CLK_STOP_OK bit checks of all codecs, a new helper function is introduced.
With this change, d3_clk_stop_ok field is no longer referred, thus removed. Also, d3_clk_stop bit field is moved to a place where more efficiently packed.
Signed-off-by: Takashi Iwai tiwai@suse.de ---
I faced this issue while I'm checking the pm issue. Mengdong, could you verify whether this change is OK?
sound/pci/hda/hda_codec.c | 30 ++++++++++++++++++++---------- sound/pci/hda/hda_codec.h | 6 +++--- sound/pci/hda/hda_intel.c | 4 ++-- 3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 4a2f35c..b7ac1ff 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -3546,10 +3546,6 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg, int count; unsigned int state;
-#ifdef CONFIG_SND_HDA_POWER_SAVE - codec->d3_stop_clk_ok = 0; -#endif - if (codec->patch_ops.set_power_state) { codec->patch_ops.set_power_state(codec, fg, power_state); return; @@ -3572,12 +3568,6 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg, if (!(state & AC_PWRST_ERROR)) break; } - -#ifdef CONFIG_SND_HDA_POWER_SAVE - if ((power_state == AC_PWRST_D3) - && codec->d3_stop_clk && (state & AC_PWRST_CLK_STOP_OK)) - codec->d3_stop_clk_ok = 1; -#endif }
#ifdef CONFIG_SND_HDA_HWDEP @@ -4532,6 +4522,26 @@ void snd_hda_power_save(struct hda_codec *codec, int delta, bool d3wait) } EXPORT_SYMBOL_HDA(snd_hda_power_save);
+/* check whether all codecs are ready to stop clock */ +bool snd_hda_bus_ready_for_d3(struct hda_bus *bus) +{ + struct hda_codec *codec; + + list_for_each_entry(codec, &bus->codec_list, list) { + unsigned int state; + int fg = codec->afg ? codec->afg : codec->mfg; + + if (!codec->d3_stop_clk) + return false; + state = snd_hda_codec_read(codec, fg, 0, + AC_VERB_GET_POWER_STATE, 0); + if (!(state & AC_PWRST_CLK_STOP_OK)) + return false; + } + return true; +} +EXPORT_SYMBOL_HDA(snd_hda_bus_ready_for_d3); + /** * snd_hda_check_amp_list_power - Check the amp list and update the power * @codec: HD-audio codec diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 13c834f..abd85e5 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -868,6 +868,7 @@ struct hda_codec { unsigned int pcm_format_first:1; /* PCM format must be set first */ #ifdef CONFIG_SND_HDA_POWER_SAVE unsigned int power_on :1; /* current (global) power-state */ + unsigned int d3_stop_clk:1; /* support D3 operation without BCLK */ int power_transition; /* power-state in transition */ int power_count; /* current (global) power refcount */ struct delayed_work power_work; /* delayed task for powerdown */ @@ -875,9 +876,6 @@ struct hda_codec { unsigned long power_off_acct; unsigned long power_jiffies; spinlock_t power_lock; - - unsigned int d3_stop_clk:1; /* support D3 operation without BCLK */ - unsigned int d3_stop_clk_ok:1; /* BCLK can stop */ #endif
/* codec-specific additional proc output */ @@ -1068,9 +1066,11 @@ const char *snd_hda_get_jack_location(u32 cfg); #ifdef CONFIG_SND_HDA_POWER_SAVE void snd_hda_power_save(struct hda_codec *codec, int delta, bool d3wait); void snd_hda_update_power_acct(struct hda_codec *codec); +bool snd_hda_bus_ready_for_d3(struct hda_bus *bus); #else static inline void snd_hda_power_save(struct hda_codec *codec, int delta, bool d3wait) {} +static inline bool snd_hda_bus_ready_for_d3(struct hda_bus *bus) { return false; } #endif
/** diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c index 1c9c779..9b02174 100644 --- a/sound/pci/hda/hda_intel.c +++ b/sound/pci/hda/hda_intel.c @@ -2410,7 +2410,7 @@ static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec) { struct azx *chip = bus->private_data;
- if (bus->power_keep_link_on || !codec->d3_stop_clk_ok) + if (bus->power_keep_link_on) return;
if (codec->power_on) @@ -2529,7 +2529,7 @@ static int azx_runtime_suspend(struct device *dev) struct azx *chip = card->private_data;
#ifdef CONFIG_SND_HDA_POWER_SAVE - if (!power_save_controller) + if (!power_save_controller || !snd_hda_bus_ready_for_d3(chip->bus)) return -EAGAIN; #endif
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Sunday, August 26, 2012 10:26 PM
The flag codec->d3_clk_stop_ok isn't always set when the codec is powered off. If it's not set, the controller driver doesn't change the runtime PM count. This causes an unblanced accounting when multiple codecs are present.
If the flag codec->d3_clk_stop_ok is not set on changes to D3, this flag will keep 0 and the same codec will not call pm_runtime_get_sync() on change back to D0. So I feel the runtime PM count is still balanced although the controller may lose a chance to suspend itself.
Usually, the first codec is powered down with CLK_STOP_OK=0,
then the next codec with CLK_STOP_OK=1. At this moment, the first codec is changed also to CLK_STOP_OK=1, but this isn't informed to the controller. Thus it still thinks as if one codec is active.
Does this conflict with spec? The spec says that PS-ClkStopOk should be set or cleared *before* entering D3: "if the codec requires BCLK to be available in D3 state, while a pass through loop is active, then it must indicate so by clearing the PS-ClkStopOk bit before entering D3 state, otherwise it must set that bit." If this is not true, then checking later is necessary.
For fixing this issue, move the check of CLK_STOP_OK bit at the runtime suspend time, and simple up/down by the codec power save in azx_power_notifier(). For CLK_STOP_OK bit checks of all codecs, a new helper function is introduced.
I've tested your patch but with a little change. Because one of my codecs cannot support stop-clock, I pretend it can by setting codec->d3_clk_stop. Then I found snd_hda_bus_ready_for_d3() blocks when getting this codec's power state, and so the runtime PM status keeps SUSPENDING. I'm not sure it's because this codec cannot support 'get state' after entering D3 for some time, or other reason. I've just got a new codec that should support 'stop-clk' in D3 and I'll test the new codec. My platform is not stable and it may take some time to do the test. Thanks!
+/* check whether all codecs are ready to stop clock */ bool +snd_hda_bus_ready_for_d3(struct hda_bus *bus) {
- struct hda_codec *codec;
- list_for_each_entry(codec, &bus->codec_list, list) {
unsigned int state;int fg = codec->afg ? codec->afg : codec->mfg;if (!codec->d3_stop_clk)return false;state = snd_hda_codec_read(codec, fg, 0,AC_VERB_GET_POWER_STATE, 0); ... blocked here.if (!(state & AC_PWRST_CLK_STOP_OK))return false;- }
- return true;
+}
Thanks Mengdong
At Tue, 28 Aug 2012 08:14:43 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Sunday, August 26, 2012 10:26 PM
The flag codec->d3_clk_stop_ok isn't always set when the codec is powered off. If it's not set, the controller driver doesn't change the runtime PM count. This causes an unblanced accounting when multiple codecs are present.
If the flag codec->d3_clk_stop_ok is not set on changes to D3, this flag will keep 0 and the same codec will not call pm_runtime_get_sync() on change back to D0. So I feel the runtime PM count is still balanced although the controller may lose a chance to suspend itself.
Yeah, my analysis was wrong. The refcounting is OK during the driver is being operated. But it messes up in some corner cases. See below.
Usually, the first codec is powered down with CLK_STOP_OK=0,
then the next codec with CLK_STOP_OK=1. At this moment, the first codec is changed also to CLK_STOP_OK=1, but this isn't informed to the controller. Thus it still thinks as if one codec is active.
Does this conflict with spec? The spec says that PS-ClkStopOk should be set or cleared *before* entering D3: "if the codec requires BCLK to be available in D3 state, while a pass through loop is active, then it must indicate so by clearing the PS-ClkStopOk bit before entering D3 state, otherwise it must set that bit." If this is not true, then checking later is necessary.
I did a deeper look, and it seems that the codec on my machine (IDT) requires the longer delay even though it advertises EPSS. This means we break potentially others, and we need some way to override the EPSS check.
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.
For fixing this issue, move the check of CLK_STOP_OK bit at the runtime suspend time, and simple up/down by the codec power save in azx_power_notifier(). For CLK_STOP_OK bit checks of all codecs, a new helper function is introduced.
I've tested your patch but with a little change. Because one of my codecs cannot support stop-clock, I pretend it can by setting codec->d3_clk_stop. Then I found snd_hda_bus_ready_for_d3() blocks when getting this codec's power state, and so the runtime PM status keeps SUSPENDING. I'm not sure it's because this codec cannot support 'get state' after entering D3 for some time, or other reason. I've just got a new codec that should support 'stop-clk' in D3 and I'll test the new codec. My platform is not stable and it may take some time to do the test. Thanks!
Drop my previous patch. It's a crap. It shouldn't call the parameter read at that place at all...
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.
- The D3-stop-clock check isn't done properly for the codecs providing own set_power_state ops.
- Ditto for the repeated power-set sequence, the error check, etc.
Now I fixed these things and uploaded to topic/hda-fix branch in sound git tree. Please take a look and check whether it's OK.
Meanwhile, ESSP fix must go to 3.6, so I queued it to for-linus branch and merged back to for-next branch (due to merge conflicts).
thanks,
Takashi
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.
- The D3-stop-clock check isn't done properly for the codecs providing own set_power_state ops.
I cannot test the branch 'if (codec->patch_ops.set_power_state)' in hda_set_power_state() for lack of such a codec. But the logic seems right and the other branch works well for me :-)
- Ditto for the repeated power-set sequence, the error check, etc.
In hda_sync_power_state(), if ((state & 0xff) == power_state) ... should be if ((state & 0xf0)>> 4 == power_state), because PS-Act is bits 7:4 break;
And would you please explain why changing to D3 need not wait? One of my codec does have a delay on change from D0 to D3.
Thanks Mengdong
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...
- The D3-stop-clock check isn't done properly for the codecs providing own set_power_state ops.
I cannot test the branch 'if (codec->patch_ops.set_power_state)' in hda_set_power_state() for lack of such a codec. But the logic seems right and the other branch works well for me :-)
- Ditto for the repeated power-set sequence, the error check, etc.
In hda_sync_power_state(), if ((state & 0xff) == power_state) ... should be if ((state & 0xf0)>> 4 == power_state), because PS-Act is bits 7:4 break;
Ah thanks, I forgot that.
And would you please explain why changing to D3 need not wait? One of my codec does have a delay on change from D0 to D3.
It was skipped because the driver doesn't need to wait. It's just a power down and doesn't matter even if you proceed if you go to the next codec. But as we are checking stop-clock bit, D3 should be sync'ed as well, indeed. I'm going to remove the exception of D3, too.
thanks,
Takashi
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@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@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);
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, August 29, 2012 10:59 PM
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.
Hi Takashi,
Usage of the new pm_notify() looks nice. The v3 patch works well on my platform.
However, maybe we can add some check before notifying the codec power-down in snd_hda_codec_free().
static void snd_hda_codec_free(struct hda_codec *codec) { ... restore_init_pincfgs(codec); #ifdef CONFIG_SND_HDA_POWER_SAVE cancel_delayed_work(&codec->power_work); flush_workqueue(codec->bus->workq); #endif ... if (codec->patch_ops.free) codec->patch_ops.free(codec); module_put(codec->owner); hda_call_pm_notify(codec->bus, false); ... }
restore_init_pincfgs() will resume the codec to D0, and since the power_save timeout is at least 1second, so it's almost always safe to call hda_call_pm_notify(codec->bus, false) here. But an extremely low possibility is: the codec will be suspended again and call hda_call_pm_notify(,false) in hda_power_work() before the work is cancelled and queue is flushed. And the flag 'codec->d3_stop_clk_ok' also cannot tell us whether a pm_notify(, false) has already been called or not, because codec->patch_ops.free() may call hda_set_power_state (D3) unexpectedly.
So, I think that each codec still needs a flag 'power_count' and let the pm_notify modify the count:
static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec, bool power_up) { struct azx *chip = bus->private_data;
if (power_up) { pm_runtime_get_sync(&chip->pci->dev); codec->power_count ++; } else { pm_runtime_put_sync(&chip->pci->dev); codec->power_count --; } }
And static void snd_hda_codec_free(struct hda_codec *codec) { ... +#ifdef CONFIG_SND_HDA_POWER_SAVE + If(codec-> power_count) hda_call_pm_notify(codec->bus, false); +#endif ... }
Thanks Mengdong
At Thu, 30 Aug 2012 14:17:19 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, August 29, 2012 10:59 PM
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.
Hi Takashi,
Usage of the new pm_notify() looks nice. The v3 patch works well on my platform.
However, maybe we can add some check before notifying the codec power-down in snd_hda_codec_free().
static void snd_hda_codec_free(struct hda_codec *codec) { ... restore_init_pincfgs(codec); #ifdef CONFIG_SND_HDA_POWER_SAVE cancel_delayed_work(&codec->power_work); flush_workqueue(codec->bus->workq); #endif ... if (codec->patch_ops.free) codec->patch_ops.free(codec); module_put(codec->owner); hda_call_pm_notify(codec->bus, false); ... }
restore_init_pincfgs() will resume the codec to D0, and since the power_save timeout is at least 1second, so it's almost always safe to call hda_call_pm_notify(codec->bus, false) here. But an extremely low possibility is: the codec will be suspended again and call hda_call_pm_notify(,false) in hda_power_work() before the work is cancelled and queue is flushed.
Yeah, the check seems slipped from my previous post by some reason. Maybe I cherry-picked a wrong branch. It should be if (codec->power_on) hda_call_pm_notify(codec->bus, false); and it should be called before module_put().
And the flag 'codec->d3_stop_clk_ok' also cannot tell us whether a pm_notify(, false) has already been called or not, because codec->patch_ops.free() may call hda_set_power_state (D3) unexpectedly. So, I think that each codec still needs a flag 'power_count' and let the pm_notify modify the count:
static void azx_power_notify(struct hda_bus *bus, struct hda_codec *codec, bool power_up) { struct azx *chip = bus->private_data;
if (power_up) { pm_runtime_get_sync(&chip->pci->dev); codec->power_count ++; } else { pm_runtime_put_sync(&chip->pci->dev); codec->power_count --; } }
And static void snd_hda_codec_free(struct hda_codec *codec) { ... +#ifdef CONFIG_SND_HDA_POWER_SAVE
- If(codec-> power_count) hda_call_pm_notify(codec->bus, false);
+#endif ... }
It's not needed. In the case of snd_hda_codec_free(), the only interest is whether pm_notify down was already called for this this particular codec. And this can be checked by codec->power_on flag.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, August 30, 2012 10:45 PM
Yeah, the check seems slipped from my previous post by some reason. Maybe I cherry-picked a wrong branch. It should be if (codec->power_on) hda_call_pm_notify(codec->bus, false); and it should be called before module_put().
It's not needed. In the case of snd_hda_codec_free(), the only interest is whether pm_notify down was already called for this this particular codec. And this can be checked by codec->power_on flag.
Yes, an extra power flag is not needed.
I think 'codec->d3_stop_clk_ok' flag is more suitable than 'codec->power_on' for this check: if (!codec->d3_stop_clk_ok) ... either not suspended or d3_stop_clk_ok is not set for last suspending hda_call_pm_notify(codec->bus, false);
It's because the codec will call pm_notify down only when PS-ClkStopOk is set on its last suspending to D3. And PS-ClkStopOk is dynamic flag while CLKSTOP is static. It's possible that a codec supports CLKSTOP but does not set PS-ClkStopOk on every transition to D3.
Thanks Mengdong
At Fri, 31 Aug 2012 01:57:09 +0000, Lin, Mengdong wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Thursday, August 30, 2012 10:45 PM
Yeah, the check seems slipped from my previous post by some reason. Maybe I cherry-picked a wrong branch. It should be if (codec->power_on) hda_call_pm_notify(codec->bus, false); and it should be called before module_put().
It's not needed. In the case of snd_hda_codec_free(), the only interest is whether pm_notify down was already called for this this particular codec. And this can be checked by codec->power_on flag.
Yes, an extra power flag is not needed.
I think 'codec->d3_stop_clk_ok' flag is more suitable than 'codec->power_on' for this check: if (!codec->d3_stop_clk_ok) ... either not suspended or d3_stop_clk_ok is not set for last suspending hda_call_pm_notify(codec->bus, false);
Right, this should be something like that.
It's overall confusing to check codec->d3_stop_clk_ok at that point, though. And the confusion comes from the fact that we are using this flag for multiple purposes -- the flag indicating the hardware status and the flag indicating whether the pm-notification has been done.
So I did a few clean ups in my tree now. Essentially I replaced codec->d3_stop_clk_ok with codec->pm_down_notified to make the meaning clearer. Also the check of D3_STOP_CLK_OK bit was moved to the hda_power_down_work() so that it's performed only in the power save.
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Saturday, September 01, 2012 12:17 AM
I think 'codec->d3_stop_clk_ok' flag is more suitable than
'codec->power_on' for this check:
if (!codec->d3_stop_clk_ok) ... either not suspended or
d3_stop_clk_ok is not set for last suspending
hda_call_pm_notify(codec->bus, false);
Right, this should be something like that.
It's overall confusing to check codec->d3_stop_clk_ok at that point, though. And the confusion comes from the fact that we are using this flag for multiple purposes -- the flag indicating the hardware status and the flag indicating whether the pm-notification has been done.
So I did a few clean ups in my tree now. Essentially I replaced codec->d3_stop_clk_ok with codec->pm_down_notified to make the meaning clearer. Also the check of D3_STOP_CLK_OK bit was moved to the hda_power_down_work() so that it's performed only in the power save.
Yeah, the flag's new name is more direct and make code easy to understand.
Thanks Mengdong
EPSS parameter should be static, so we can read it once and remember. This also allows more easily to override the wrong EPSS capability reported from a codec by changing the flag in the codec initialization step.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 10 ++++++++-- sound/pci/hda/hda_codec.h | 1 + 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index f560051..f25c24c 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -1209,6 +1209,9 @@ static void snd_hda_codec_free(struct hda_codec *codec) kfree(codec); }
+static bool snd_hda_codec_get_supported_ps(struct hda_codec *codec, + hda_nid_t fg, unsigned int power_state); + static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg, unsigned int power_state);
@@ -1317,6 +1320,10 @@ int /*__devinit*/ snd_hda_codec_new(struct hda_bus *bus, AC_VERB_GET_SUBSYSTEM_ID, 0); }
+ codec->epss = snd_hda_codec_get_supported_ps(codec, + codec->afg ? codec->afg : codec->mfg, + AC_PWRST_EPSS); + /* power-up all before initialization */ hda_set_power_state(codec, codec->afg ? codec->afg : codec->mfg, @@ -3543,8 +3550,7 @@ static void hda_set_power_state(struct hda_codec *codec, hda_nid_t fg, /* this delay seems necessary to avoid click noise at power-down */ if (power_state == AC_PWRST_D3) { /* transition time less than 10ms for power down */ - bool epss = snd_hda_codec_get_supported_ps(codec, fg, AC_PWRST_EPSS); - msleep(epss ? 10 : 100); + msleep(codec->epss ? 10 : 100); }
/* repeat power states setting at most 10 times*/ diff --git a/sound/pci/hda/hda_codec.h b/sound/pci/hda/hda_codec.h index 7fbc1bc..e5a7e19 100644 --- a/sound/pci/hda/hda_codec.h +++ b/sound/pci/hda/hda_codec.h @@ -862,6 +862,7 @@ struct hda_codec { unsigned int ignore_misc_bit:1; /* ignore MISC_NO_PRESENCE bit */ unsigned int no_jack_detect:1; /* Machine has no jack-detection */ unsigned int pcm_format_first:1; /* PCM format must be set first */ + unsigned int epss:1; /* supporting EPSS? */ #ifdef CONFIG_SND_HDA_POWER_SAVE unsigned int power_on :1; /* current (global) power-state */ int power_transition; /* power-state in transition */
These codecs seem reporting EPSS but require longer delay for the proper D3 transition. For example, D3_STOP_CLOCK_OK bit won't be set correctly even after D3.
In this patch, codec->epss flag is overridden for avoid the misbehavior.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_sigmatel.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/pci/hda/patch_sigmatel.c b/sound/pci/hda/patch_sigmatel.c index 3edd73c..6f806d3 100644 --- a/sound/pci/hda/patch_sigmatel.c +++ b/sound/pci/hda/patch_sigmatel.c @@ -5534,6 +5534,7 @@ static int patch_stac92hd83xxx(struct hda_codec *codec) snd_hda_codec_set_pincfg(codec, 0xf, 0x2181205e); }
+ codec->epss = 0; /* longer delay needed for D3 */ codec->no_trigger_sense = 1; codec->spec = spec;
participants (2)
-
Lin, Mengdong -
Takashi Iwai