[PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[abhijeet.kumar]

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);
 	}
 }
-- 
1.9.1

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Vinod Koul]

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

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Kumar, Abhijeet]

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."

>

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Takashi Iwai]

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

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Kumar, Abhijeet]

> 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

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Takashi Iwai]

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

Re: [PATCH] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[Kumar, Abhijeet]

> 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

[PATCH v2] ASoC: hdac_hdmi: Ensuring proper setting of output widget power state

[abhijeet.kumar]

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);
 	}
 }
-- 
1.9.1

Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Mark Brown]

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);
 	}
 }
-- 
2.15.1

Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 485 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Kumar, Abhijeet]

> 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.

Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Vinod Koul]

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...

-- 
~Vinod

Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 478 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Vinod Koul]

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..

-- 
~Vinod

Re: Applied "ASoC: hdac_hdmi: Ensuring proper setting of output widget power state" to the asoc tree

[Kumar, Abhijeet]

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/
>

[PATCH 1/3] ALSA: hda: Copying sync power state helper to core

[abhijeet.kumar]

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);
-- 
1.9.1

[PATCH 2/3] ALSA: hda: Make use of core codec functions to sync power state

[abhijeet.kumar]

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);
-- 
1.9.1

[PATCH 3/3] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[abhijeet.kumar]

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;
+			}
+		}
 	}
 }
 
-- 
1.9.1

Re: [PATCH 3/3] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[Takashi Iwai]

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
> 
> 

Re: [PATCH 3/3] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[Kumar, Abhijeet]

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

[PATCH] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[abhijeet.kumar]

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;
+			}
+		}
+
 	}
 }
 
-- 
1.9.1

Applied "ASoC: hdac_hdmi : Ensuring proper setting of output widget power state" to the asoc tree

[Mark Brown]

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;
+			}
+		}
+
 	}
 }
 
-- 
2.16.1

[PATCH v2] ASoC: hdac_hdmi : Ensuring proper setting of output widget power state

[abhijeet.kumar]

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;
+			}
+		}
+
 	}
 }
 
-- 
1.9.1