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

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

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

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

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