[alsa-devel] [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
When we change the resolution of DP pannel or hot plug-unplug it while playing an audio clip,sometimes we observe a silent playback(no audio).
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue.With this changes the verb is sent to set the power state and response is received. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdac, nid, 0, + snd_hdac_codec_read(&edev->hdac, nid, 0, AC_VERB_SET_POWER_STATE, pwr_state); } }
![](https://secure.gravatar.com/avatar/8fc76980774cffd56f0df0531ea82ba7.jpg?s=120&d=mm&r=g)
On Thu, Jan 11, 2018 at 05:04:27PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
When we change the resolution of DP pannel or hot plug-unplug it while playing an audio clip,sometimes we observe a silent playback(no audio).
can you rephrase this please
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue.With this changes the verb is sent to set the power
space after .
state and response is received. Thus ensuring power state is set.
am not sure I fully understood the problem here
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com
sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state))
snd_hdac_codec_write(&edev->hdac, nid, 0,
snd_hdac_codec_read(&edev->hdac, nid, 0,
how does read help instead of write?
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
On 1/12/2018 11:16 AM, Vinod Koul wrote:
On Thu, Jan 11, 2018 at 05:04:27PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
When we change the resolution of DP pannel or hot plug-unplug it while playing an audio clip,sometimes we observe a silent playback(no audio).
can you rephrase this please
done please review v2!
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue.With this changes the verb is sent to set the power
space after .
state and response is received. Thus ensuring power state is set.
am not sure I fully understood the problem here
This appears to be an timing issue, while performing a stress test, we found out that sometimes either pin or converters are not powered up. Thus ensuring it that the power state is set correctly.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com
sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state))
snd_hdac_codec_write(&edev->hdac, nid, 0,
snd_hdac_codec_read(&edev->hdac, nid, 0,
how does read help instead of write?
Indeed i'm making use of read instead of write to send the set command.
But unlike codec_write, codec_read send the verb synchronously. Maybe if you read the comment while powering up and down in hdmi_codec_prepare and hdmi_codec_complete you would understand better.
"codec_read is preferred over codec_write to set the power state.
This way verb is send to set the power state and response is received. So setting power state is ensured without using loop to read the state."
![](https://secure.gravatar.com/avatar/5b19e9d0e834ea10ef75803718ad564b.jpg?s=120&d=mm&r=g)
On Fri, 12 Jan 2018 09:25:54 +0100, Kumar, Abhijeet wrote:
On 1/12/2018 11:16 AM, Vinod Koul wrote:
On Thu, Jan 11, 2018 at 05:04:27PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
When we change the resolution of DP pannel or hot plug-unplug it while playing an audio clip,sometimes we observe a silent playback(no audio).
can you rephrase this please
done please review v2!
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue.With this changes the verb is sent to set the power
space after .
state and response is received. Thus ensuring power state is set.
am not sure I fully understood the problem here
This appears to be an timing issue, while performing a stress test, we found out that sometimes either pin or converters are not powered up. Thus ensuring it that the power state is set correctly.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com
sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state))
snd_hdac_codec_write(&edev->hdac, nid, 0,
snd_hdac_codec_read(&edev->hdac, nid, 0,
how does read help instead of write?
Indeed i'm making use of read instead of write to send the set command.
But unlike codec_write, codec_read send the verb synchronously. Maybe if you read the comment while powering up and down in hdmi_codec_prepare and hdmi_codec_complete you would understand better.
"codec_read is preferred over codec_write to set the power state.
This way verb is send to set the power state and response is received. So setting power state is ensured without using loop to read the state."
It's better, but doesn't guarantee that the node reached the given power state. codec_read assures that the verb is sent and the codec gives the response. But it means only the target state gets updated, and doesn't mean that the actual state reached.
thanks,
Takashi
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
It's better, but doesn't guarantee that the node reached the given power state. codec_read assures that the verb is sent and the codec gives the response. But it means only the target state gets updated, and doesn't mean that the actual state reached.
Thanks Takashi for replying. I guess, I should make use of return value from codec_read and retry if power state is not set properly! I'll make those changes and update again. And just wondering, how does hdmi_codec_prepare() and hdmi_codec_complete() gurantee while powering up and down the afg ?
Warm Regards, Abhijeet
![](https://secure.gravatar.com/avatar/5b19e9d0e834ea10ef75803718ad564b.jpg?s=120&d=mm&r=g)
On Fri, 12 Jan 2018 11:37:28 +0100, Kumar, Abhijeet wrote:
It's better, but doesn't guarantee that the node reached the given power state. codec_read assures that the verb is sent and the codec gives the response. But it means only the target state gets updated, and doesn't mean that the actual state reached.
Thanks Takashi for replying. I guess, I should make use of return value from codec_read and retry if power state is not set properly! I'll make those changes and update again. And just wondering, how does hdmi_codec_prepare() and hdmi_codec_complete() gurantee while powering up and down the afg ?
There are two power states in the codec node: the target and the actual. By setting the power state, the target is set, but the actual state change may still take some time. In such a case, you'd need to wait until the actual state reaches. See hda_sync_power_state() in the hda legacy.
Takashi
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
can you rephrase this please
done please review v2! https://patchwork.kernel.org/patch/10159791/
am not sure I fully understood the problem here
This appears to be an timing issue, while performing a stress test, we found out that sometimes either pin or converters are not powered up. Thus ensuring it that the power state is set correctly.
how does read help instead of write?
Indeed i'm making use of read instead of write to send the set command. But unlike codec_write, codec_read send the verb synchronously. Maybe if you read the comment while powering up and down afg in hdmi_codec_prepare and hdmi_codec_complete you would understand better. "codec_read is preferred over codec_write to set the power state. This way verb is send to set the power state and response is received. So setting power state is ensured without using loop to read the state."
-----Original Message----- From: Koul, Vinod Sent: Friday, January 12, 2018 11:17 AM To: Kumar, Abhijeet abhijeet.kumar@intel.com Cc: Liam Girdwood lgirdwood@gmail.com; Mark Brown broonie@kernel.org; Jaroslav Kysela perex@perex.cz; Takashi Iwai tiwai@suse.com; Kp, Jeeja jeeja.kp@intel.com; Prusty, Subhransu S subhransu.s.prusty@intel.com; Singh, Guneshwor O guneshwor.o.singh@intel.com; Tayal, SandeepX sandeepx.tayal@intel.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
On Thu, Jan 11, 2018 at 05:04:27PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
When we change the resolution of DP pannel or hot plug-unplug it while playing an audio clip,sometimes we observe a silent playback(no audio).
can you rephrase this please
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue.With this changes the verb is sent to set the power
space after .
state and response is received. Thus ensuring power state is set.
am not sure I fully understood the problem here
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com
sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state))
snd_hdac_codec_write(&edev->hdac, nid, 0,
snd_hdac_codec_read(&edev->hdac, nid, 0,
how does read help instead of write?
-- ~Vinod
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets.
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue. With this changes the verb is sent to set the power state and response is received. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- Changes in v2: - update commit message
sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdac, nid, 0, + snd_hdac_codec_read(&edev->hdac, nid, 0, AC_VERB_SET_POWER_STATE, pwr_state); } }
![](https://secure.gravatar.com/avatar/d930951cb00393ecf9c3db3a56d78fa9.jpg?s=120&d=mm&r=g)
The patch
ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From a04ba2b3dc6c14bedd5efca442d6039690562951 Mon Sep 17 00:00:00 2001
From: Abhijeet Kumar abhijeet.kumar@intel.com Date: Fri, 12 Jan 2018 14:02:54 +0530 Subject: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets.
During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimzing the way we set the power could mitigate the issue. With this changes the verb is sent to set the power state and response is received. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/hdac_hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..e24caecf0a4f 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,7 +718,7 @@ static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, { if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdac, nid, 0, + snd_hdac_codec_read(&edev->hdac, nid, 0, AC_VERB_SET_POWER_STATE, pwr_state); } }
![](https://secure.gravatar.com/avatar/d930951cb00393ecf9c3db3a56d78fa9.jpg?s=120&d=mm&r=g)
On Fri, Jan 12, 2018 at 09:08:53PM +0000, Mark Brown wrote:
The patch
ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there?
I'm working on inputs from Takashi and Pierre. After much of testing I'll share the revision patch which waits until actual state reaches target state (similar is implemented in hda) by ww03.
Warm Regards, Abhijeet
-----Original Message----- From: Mark Brown [mailto:broonie@kernel.org] Sent: Saturday, January 13, 2018 2:50 AM To: Kumar, Abhijeet abhijeet.kumar@intel.com Cc: Liam Girdwood lgirdwood@gmail.com; Jaroslav Kysela perex@perex.cz; Takashi Iwai tiwai@suse.com; Kp, Jeeja jeeja.kp@intel.com; Koul, Vinod vinod.koul@intel.com; Prusty, Subhransu S subhransu.s.prusty@intel.com; Singh, Guneshwor O guneshwor.o.singh@intel.com; Tayal, SandeepX sandeepx.tayal@intel.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org Subject: Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree
On Fri, Jan 12, 2018 at 09:08:53PM +0000, Mark Brown wrote:
The patch
ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
![](https://secure.gravatar.com/avatar/8fc76980774cffd56f0df0531ea82ba7.jpg?s=120&d=mm&r=g)
On Fri, Jan 12, 2018 at 09:20:12PM +0000, Mark Brown wrote:
On Fri, Jan 12, 2018 at 09:08:53PM +0000, Mark Brown wrote:
The patch
ASoC: hdac_hdmi: Ensuring proper setting of output widget power state
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
Hey Mark,
I do not think this is ready yet. Takashi and me have few comments on this...
![](https://secure.gravatar.com/avatar/d930951cb00393ecf9c3db3a56d78fa9.jpg?s=120&d=mm&r=g)
On Mon, Jan 15, 2018 at 11:42:16AM +0530, Vinod Koul wrote:
On Fri, Jan 12, 2018 at 09:20:12PM +0000, Mark Brown wrote:
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
I do not think this is ready yet. Takashi and me have few comments on this...
There were comments on v1 but nobody responded to v2.
![](https://secure.gravatar.com/avatar/8fc76980774cffd56f0df0531ea82ba7.jpg?s=120&d=mm&r=g)
On Mon, Jan 15, 2018 at 10:30:40AM +0000, Mark Brown wrote:
On Mon, Jan 15, 2018 at 11:42:16AM +0530, Vinod Koul wrote:
On Fri, Jan 12, 2018 at 09:20:12PM +0000, Mark Brown wrote:
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
I do not think this is ready yet. Takashi and me have few comments on this...
There were comments on v1 but nobody responded to v2.
yeah we were still stuck on v1 :) I think Abhijeet will post an update once we align..
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
On 1/15/2018 4:11 PM, Vinod Koul wrote:
On Mon, Jan 15, 2018 at 10:30:40AM +0000, Mark Brown wrote:
On Mon, Jan 15, 2018 at 11:42:16AM +0530, Vinod Koul wrote:
On Fri, Jan 12, 2018 at 09:20:12PM +0000, Mark Brown wrote:
I've dropped this because it caused a conflict with the topic/hdac-hdmi branch - can you please check what's going on there? I merged that into the topic/intel branch so probably just a rebase is needed.
I do not think this is ready yet. Takashi and me have few comments on this...
There were comments on v1 but nobody responded to v2.
yeah we were still stuck on v1 :) I think Abhijeet will post an update once we align..
Hi, I 've posted the latest series on ALSAdev mailing list. Kindly review. :) https://patchwork.kernel.org/patch/10180821/ https://patchwork.kernel.org/patch/10180825/ https://patchwork.kernel.org/patch/10180827/
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
The current sync_power_state is local to hda code, moving it core so that other users apart from hda legacy can use it. The helper function ensures the actual state reaches the target state.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- include/sound/hdaudio.h | 2 ++ sound/hda/hdac_device.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 68169e3749de..4c93ff5301bd 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -146,6 +146,8 @@ int snd_hdac_codec_write(struct hdac_device *hdac, hda_nid_t nid, int flags, unsigned int verb, unsigned int parm); bool snd_hdac_check_power_state(struct hdac_device *hdac, hda_nid_t nid, unsigned int target_state); +unsigned int snd_hdac_sync_power_state(struct hdac_device *hdac, + hda_nid_t nid, unsigned int target_state); /** * snd_hdac_read_parm - read a codec parameter * @codec: the codec object diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index 06f845e293cb..7ba100bb1c3f 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -3,6 +3,7 @@ */
#include <linux/init.h> +#include <linux/delay.h> #include <linux/device.h> #include <linux/slab.h> #include <linux/module.h> @@ -1064,3 +1065,37 @@ bool snd_hdac_check_power_state(struct hdac_device *hdac, return (state == target_state); } EXPORT_SYMBOL_GPL(snd_hdac_check_power_state); +/** + * snd_hdac_sync_power_state - wait until actual power state matches + * with the target state + * + * @hdac: the HDAC device + * @nid: NID to send the command + * @target_state: target state to check for + * + * Return power state or PS_ERROR if codec rejects GET verb. + */ +unsigned int snd_hdac_sync_power_state(struct hdac_device *codec, + hda_nid_t nid, unsigned int power_state) +{ + unsigned long end_time = jiffies + msecs_to_jiffies(500); + unsigned int state, actual_state, count; + + for (count = 0; count < 500; count++) { + state = snd_hdac_codec_read(codec, nid, 0, + AC_VERB_GET_POWER_STATE, 0); + if (state & AC_PWRST_ERROR) { + msleep(20); + break; + } + actual_state = (state >> 4) & 0x0f; + if (actual_state == power_state) + break; + if (time_after_eq(jiffies, end_time)) + break; + /* wait until the codec reachs to the target state */ + msleep(1); + } + return state; +} +EXPORT_SYMBOL_GPL(snd_hdac_sync_power_state);
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
Since sync_power_state is moved to core it's better to use the helper function to ensure the actual power state reaches target instead of using the local helper functions already exsisting in hda code.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- sound/pci/hda/hda_codec.c | 28 +--------------------------- sound/pci/hda/hda_local.h | 6 +++++- 2 files changed, 6 insertions(+), 28 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index e018ecbf78a8..5bc3a7468e17 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2702,32 +2702,6 @@ void snd_hda_codec_set_power_to_all(struct hda_codec *codec, hda_nid_t fg, } EXPORT_SYMBOL_GPL(snd_hda_codec_set_power_to_all);
-/* - * wait until the state is reached, returns the current state - */ -static unsigned int hda_sync_power_state(struct hda_codec *codec, - hda_nid_t fg, - unsigned int power_state) -{ - unsigned long end_time = jiffies + msecs_to_jiffies(500); - unsigned int state, actual_state; - - for (;;) { - state = snd_hda_codec_read(codec, fg, 0, - AC_VERB_GET_POWER_STATE, 0); - if (state & AC_PWRST_ERROR) - break; - actual_state = (state >> 4) & 0x0f; - if (actual_state == power_state) - break; - if (time_after_eq(jiffies, end_time)) - break; - /* wait until the codec reachs to the target state */ - msleep(1); - } - return state; -} - /** * snd_hda_codec_eapd_power_filter - A power filter callback for EAPD * @codec: the HDA codec @@ -2790,7 +2764,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec, state); snd_hda_codec_set_power_to_all(codec, fg, power_state); } - state = hda_sync_power_state(codec, fg, power_state); + state = snd_hda_sync_power_state(codec, fg, power_state); if (!(state & AC_PWRST_ERROR)) break; } diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h index 5b5c324c99b9..321e78baa63c 100644 --- a/sound/pci/hda/hda_local.h +++ b/sound/pci/hda/hda_local.h @@ -622,7 +622,11 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec, { return snd_hdac_check_power_state(&codec->core, nid, target_state); } - +static inline bool snd_hda_sync_power_state(struct hda_codec *codec, + hda_nid_t nid, unsigned int target_state) +{ + return snd_hdac_sync_power_state(&codec->core, nid, target_state); +} unsigned int snd_hda_codec_eapd_power_filter(struct hda_codec *codec, hda_nid_t nid, unsigned int power_state);
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- sound/soc/codecs/hdac_hdmi.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..4dc9b9b71db9 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -716,10 +716,20 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct hdac_ext_device *edev, static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, hda_nid_t nid, unsigned int pwr_state) { + int count; + unsigned int state; if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) { - if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdac, nid, 0, - AC_VERB_SET_POWER_STATE, pwr_state); + if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) { + for (count = 0; count < 10; count++) { + snd_hdac_codec_read(&edev->hdac, nid, 0, + AC_VERB_SET_POWER_STATE, + pwr_state); + state = snd_hdac_sync_power_state(&edev->hdac, + nid, pwr_state); + if (!(state & AC_PWRST_ERROR)) + break; + } + } } }
![](https://secure.gravatar.com/avatar/5b19e9d0e834ea10ef75803718ad564b.jpg?s=120&d=mm&r=g)
On Tue, 23 Jan 2018 18:30:53 +0100, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com
I applied the patches 1 and 2 to topic/hda-sync-power branch. It's based on 4.16-rc1.
Mark, could you pull that into your intel branch and apply this fix?
thanks,
Takashi
sound/soc/codecs/hdac_hdmi.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index f3b4f4dfae6a..4dc9b9b71db9 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -716,10 +716,20 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct hdac_ext_device *edev, static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, hda_nid_t nid, unsigned int pwr_state) {
- int count;
- unsigned int state; if (get_wcaps(&edev->hdac, nid) & AC_WCAP_POWER) {
if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state))
snd_hdac_codec_write(&edev->hdac, nid, 0,
AC_VERB_SET_POWER_STATE, pwr_state);
if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) {
for (count = 0; count < 10; count++) {
snd_hdac_codec_read(&edev->hdac, nid, 0,
AC_VERB_SET_POWER_STATE,
pwr_state);
state = snd_hdac_sync_power_state(&edev->hdac,
nid, pwr_state);
if (!(state & AC_PWRST_ERROR))
break;
}
}}
}
-- 1.9.1
![](https://secure.gravatar.com/avatar/d930951cb00393ecf9c3db3a56d78fa9.jpg?s=120&d=mm&r=g)
On Tue, Jan 23, 2018 at 11:00:53PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
This doesn't apply after the recent refactoring to move to components - can you please rebase on current code and resend? I pulled Takashi's branch already, it's just this patch needs handling.
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
On 2/14/2018 9:50 PM, Mark Brown wrote:
On Tue, Jan 23, 2018 at 11:00:53PM +0530, abhijeet.kumar@intel.com wrote:
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
This doesn't apply after the recent refactoring to move to components - can you please rebase on current code and resend? I pulled Takashi's branch already, it's just this patch needs handling.
Please find the re-based patch here. https://patchwork.kernel.org/patch/10220549/
Warm Regards, Abhijeet
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- sound/soc/codecs/hdac_hdmi.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index dba6f4c5074a..0483afc40db6 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,10 +718,22 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct hdac_ext_device *edev, static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, hda_nid_t nid, unsigned int pwr_state) { + int count; + unsigned int state; + if (get_wcaps(&edev->hdev, nid) & AC_WCAP_POWER) { - if (!snd_hdac_check_power_state(&edev->hdev, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdev, nid, 0, - AC_VERB_SET_POWER_STATE, pwr_state); + if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) { + for (count = 0; count < 10; count++) { + snd_hdac_codec_read(&edev->hdac, nid, 0, + AC_VERB_SET_POWER_STATE, + pwr_state); + state = snd_hdac_sync_power_state(&edev->hdac, + nid, pwr_state); + if (!(state & AC_PWRST_ERROR)) + break; + } + } + } }
![](https://secure.gravatar.com/avatar/d930951cb00393ecf9c3db3a56d78fa9.jpg?s=120&d=mm&r=g)
The patch
ASoC: hdac_hdmi : Ensuring proper setting of output widget power state
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 753597fbb7031ff147d4f2862426699e4ad8efca Mon Sep 17 00:00:00 2001
From: Abhijeet Kumar abhijeet.kumar@intel.com Date: Thu, 15 Feb 2018 14:05:38 +0530 Subject: [PATCH] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/hdac_hdmi.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 0758927d1e06..60bea9d69fc0 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,10 +718,22 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct hdac_ext_device *edev, static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, hda_nid_t nid, unsigned int pwr_state) { + int count; + unsigned int state; + if (get_wcaps(&edev->hdev, nid) & AC_WCAP_POWER) { - if (!snd_hdac_check_power_state(&edev->hdev, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdev, nid, 0, - AC_VERB_SET_POWER_STATE, pwr_state); + if (!snd_hdac_check_power_state(&edev->hdac, nid, pwr_state)) { + for (count = 0; count < 10; count++) { + snd_hdac_codec_read(&edev->hdac, nid, 0, + AC_VERB_SET_POWER_STATE, + pwr_state); + state = snd_hdac_sync_power_state(&edev->hdac, + nid, pwr_state); + if (!(state & AC_PWRST_ERROR)) + break; + } + } + } }
![](https://secure.gravatar.com/avatar/70b9e11030685c19e03f6a6ce218bfdb.jpg?s=120&d=mm&r=g)
From: Abhijeet Kumar abhijeet.kumar@intel.com
In usecases like hot plug-unplug DP panel or modeset during a playback, sometimes we observe no audio after codec resets. During no audio condition, we have noticed that the power state of the pin or the connector is D3. Optimizing the way we set the power mitigates the issue. With this changes the verb is sent to set the power state and waits until actual state reaches target state. Thus ensuring power state is set.
Signed-off-by: Abhijeet Kumar abhijeet.kumar@intel.com --- sound/soc/codecs/hdac_hdmi.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index dba6f4c5074a..97fcd55205e6 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -718,10 +718,22 @@ static struct hdac_hdmi_pcm *hdac_hdmi_get_pcm(struct hdac_ext_device *edev, static void hdac_hdmi_set_power_state(struct hdac_ext_device *edev, hda_nid_t nid, unsigned int pwr_state) { + int count; + unsigned int state; + if (get_wcaps(&edev->hdev, nid) & AC_WCAP_POWER) { - if (!snd_hdac_check_power_state(&edev->hdev, nid, pwr_state)) - snd_hdac_codec_write(&edev->hdev, nid, 0, - AC_VERB_SET_POWER_STATE, pwr_state); + if (!snd_hdac_check_power_state(&edev->hdev, nid, pwr_state)) { + for (count = 0; count < 10; count++) { + snd_hdac_codec_read(&edev->hdev, nid, 0, + AC_VERB_SET_POWER_STATE, + pwr_state); + state = snd_hdac_sync_power_state(&edev->hdev, + nid, pwr_state); + if (!(state & AC_PWRST_ERROR)) + break; + } + } + } }
participants (5)
-
abhijeet.kumar@intel.com
-
Kumar, Abhijeet
-
Mark Brown
-
Takashi Iwai
-
Vinod Koul