diff mbox

[v2] ALSA: hda: Remove finite loop from snd_hdac_sync_power_state()

Message ID 1518517484-24488-1-git-send-email-abhijeet.kumar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Abhijeet Kumar Feb. 13, 2018, 10:24 a.m. UTC
From: Abhijeet Kumar <abhijeet.kumar@intel.com>

Finite loop and msleep was causing few igt@pm_rpm tests failure
for HSW and BDW. Thus removing them.

Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
		sync power state")
References: https://bugs.freedesktop.org/show_bug.cgi?id=105069

Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
---
Changes in v2:
1. Removed msleep as well.
2. Modified commit message.
 sound/hda/hdac_device.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Abhijeet Kumar Feb. 13, 2018, 12:41 p.m. UTC | #1
On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote:
> From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>
> Finite loop and msleep was causing few igt@pm_rpm tests failure
> for HSW and BDW. Thus removing them.
>
> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
> 		sync power state")
> References: https://bugs.freedesktop.org/show_bug.cgi?id=105069
>
> Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
> ---
> Changes in v2:
> 1. Removed msleep as well.
> 2. Modified commit message.
>   sound/hda/hdac_device.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> index 7ba100bb1c3f..678ef8950d0c 100644
> --- a/sound/hda/hdac_device.c
> +++ b/sound/hda/hdac_device.c
> @@ -1079,15 +1079,13 @@ 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;
> +	unsigned int state, actual_state;
>   
> -	for (count = 0; count < 500; count++) {
> +	for (; ;) {
>   		state = snd_hdac_codec_read(codec, nid, 0,
>   				AC_VERB_GET_POWER_STATE, 0);
> -		if (state & AC_PWRST_ERROR) {
> -			msleep(20);
> +		if (state & AC_PWRST_ERROR)
>   			break;
> -		}
>   		actual_state = (state >> 4) & 0x0f;
>   		if (actual_state == power_state)
>   			break;
The above changes is as good as revert. But we can still repro the issue.

https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7993/all.html

Are we sure that this is the only bad commit for regression ?



And by looking at the logs it seems like the reason for test failure is 
that after disabling all the screen

the device state has still not reached the suspended state. Thus timing 
out and asserting.

<7>[357.854160] [IGT] pm_rpm: starting subtest basic-pci-d3-state

<7>[357.854231] [drm:drm_mode_setcrtc] [CRTC:37:pipe A]

<7>[357.854519] [drm:intel_atomic_check [i915]] New cdclk calculated to 
be logical 337500 kHz, actual 337500 kHz

<7>[357.854617] [drm:intel_atomic_check [i915]] New voltage level 
calculated to be logical 2, actual 2

<7>[357.868799] [drm:intel_disable_pipe [i915]] disabling pipe A

<7>[357.887493] [drm:__intel_fbc_disable [i915]] Disabling FBC on pipe A

<7>[357.887600] [drm:intel_disable_shared_dpll [i915]] disable WRPLL 1 
(active 1, on? 1) for crtc 37

<7>[357.887695] [drm:intel_disable_shared_dpll [i915]] disabling WRPLL 1

<7>[357.887796] [drm:intel_atomic_commit_tail [i915]] [ENCODER:58:DDI B]

<7>[357.887884] [drm:intel_atomic_commit_tail [i915]] [ENCODER:64:DDI C]

<7>[357.887972] [drm:intel_atomic_commit_tail [i915]] [ENCODER:66:DP-MST A]

<7>[357.888052] [drm:intel_atomic_commit_tail [i915]] [ENCODER:67:DP-MST B]

<7>[357.888130] [drm:intel_atomic_commit_tail [i915]] [ENCODER:68:DP-MST C]

<7>[357.888363] [drm:verify_connector_state.isra.76 [i915]] 
[CONNECTOR:70:HDMI-A-2]

<7>[357.888490] [drm:verify_single_dpll_state.isra.77 [i915]] WRPLL 1

<7>[357.888608] [drm:verify_single_dpll_state.isra.77 [i915]] WRPLL 2

<7>[357.888727] [drm:verify_single_dpll_state.isra.77 [i915]] SPLL

<7>[ 357.888838] [drm:verify_single_dpll_state.isra.77 [i915]] LCPLL 810

<7>[357.888940] [drm:verify_single_dpll_state.isra.77 [i915]] LCPLL 1350

<7>[357.889052] [drm:verify_single_dpll_state.isra.77 [i915]] LCPLL 2700

<7>[357.889347] [drm:intel_atomic_commit_tail [i915]] [CRTC:37:pipe A]

<7>[357.889583] [drm:drm_mode_setcrtc] [CRTC:47:pipe B]

<7>[357.890214] [drm:drm_mode_setcrtc] [CRTC:57:pipe C]

<6>[358.533287] e1000e: enp0s25 NIC Link is Up 1000 Mbps Full Duplex, 
Flow Control: None

<7>[368.413068] [IGT] pm_rpm: exiting, ret=99 <- 10 secs diff. that's 
the timeout for wait_for_suspended().

<6>[368.423030] ahci 0000:00:1f.2: port does not support device sleep

Any clue why ata port is reporting such error when Device is trying to 
sleep ?


Warm Regards,

Abhijeet
Chris Wilson Feb. 13, 2018, 12:47 p.m. UTC | #2
Quoting Kumar, Abhijeet (2018-02-13 12:41:42)
> 
> 
> On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote:
> 
>     From: Abhijeet Kumar <abhijeet.kumar@intel.com>
> 
>     Finite loop and msleep was causing few igt@pm_rpm tests failure
>     for HSW and BDW. Thus removing them.
> 
>     Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
>                     sync power state")
>     References: https://bugs.freedesktop.org/show_bug.cgi?id=105069
> 
>     Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
>     ---
>     Changes in v2:
>     1. Removed msleep as well.
>     2. Modified commit message.
>      sound/hda/hdac_device.c | 8 +++-----
>      1 file changed, 3 insertions(+), 5 deletions(-)
> 
>     diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
>     index 7ba100bb1c3f..678ef8950d0c 100644
>     --- a/sound/hda/hdac_device.c
>     +++ b/sound/hda/hdac_device.c
>     @@ -1079,15 +1079,13 @@ 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;
>     +       unsigned int state, actual_state;
> 
>     -       for (count = 0; count < 500; count++) {
>     +       for (; ;) {
>                     state = snd_hdac_codec_read(codec, nid, 0,
>                                     AC_VERB_GET_POWER_STATE, 0);
>     -               if (state & AC_PWRST_ERROR) {
>     -                       msleep(20);
>     +               if (state & AC_PWRST_ERROR)
>                             break;
>     -               }
>                     actual_state = (state >> 4) & 0x0f;
>                     if (actual_state == power_state)
>                             break;
> 
> The above changes is as good as revert. But we can still repro the issue.

What about the different between snd_hda_codec_read() and
snd_hdac_codec_read() ?

It used to pass &codec->core and now it's just using codec.
 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7993/all.html
> 
> Are we sure that this is the only bad commit for regression ?

Absolutely.

> And by looking at the logs it seems like the reason for test failure is that
> after disabling all the screen
> 
> the device state has still not reached the suspended state. Thus timing out and
> asserting.

Correct.
-Chris
Jani Nikula Feb. 13, 2018, 12:50 p.m. UTC | #3
On Tue, 13 Feb 2018, abhijeet.kumar@intel.com wrote:
> From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>
> Finite loop and msleep was causing few igt@pm_rpm tests failure
> for HSW and BDW. Thus removing them.
>
> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
> 		sync power state")

Side note, you're not supposed to word-wrap tags like these.

BR,
Jani.
Abhijeet Kumar Feb. 13, 2018, 1:11 p.m. UTC | #4
On 2/13/2018 6:17 PM, Chris Wilson wrote:
> Quoting Kumar, Abhijeet (2018-02-13 12:41:42)
>>
>> On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote:
>>
>>      From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>
>>      Finite loop and msleep was causing few igt@pm_rpm tests failure
>>      for HSW and BDW. Thus removing them.
>>
>>      Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
>>                      sync power state")
>>      References: https://bugs.freedesktop.org/show_bug.cgi?id=105069
>>
>>      Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>      ---
>>      Changes in v2:
>>      1. Removed msleep as well.
>>      2. Modified commit message.
>>       sound/hda/hdac_device.c | 8 +++-----
>>       1 file changed, 3 insertions(+), 5 deletions(-)
>>
>>      diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
>>      index 7ba100bb1c3f..678ef8950d0c 100644
>>      --- a/sound/hda/hdac_device.c
>>      +++ b/sound/hda/hdac_device.c
>>      @@ -1079,15 +1079,13 @@ 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;
>>      +       unsigned int state, actual_state;
>>
>>      -       for (count = 0; count < 500; count++) {
>>      +       for (; ;) {
>>                      state = snd_hdac_codec_read(codec, nid, 0,
>>                                      AC_VERB_GET_POWER_STATE, 0);
>>      -               if (state & AC_PWRST_ERROR) {
>>      -                       msleep(20);
>>      +               if (state & AC_PWRST_ERROR)
>>                              break;
>>      -               }
>>                      actual_state = (state >> 4) & 0x0f;
>>                      if (actual_state == power_state)
>>                              break;
>>
>> The above changes is as good as revert. But we can still repro the issue.
> What about the different between snd_hda_codec_read() and
> snd_hdac_codec_read() ?
Both helper function boils down to same thing.
> It used to pass &codec->core and now it's just using codec.
Earlier codec was pointing to hda_codec struct
and now it's pointing to hdac_device (which is nothing but core) .
-Abhijeet
>   
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7993/all.html
>>
>> Are we sure that this is the only bad commit for regression ?
> Absolutely.

If that's the case then can we not try increasing the timeout from igt 
test just for a trial run?
To see whether it actually reaches to the suspended state or not?
>
>> And by looking at the logs it seems like the reason for test failure is that
>> after disabling all the screen
>>
>> the device state has still not reached the suspended state. Thus timing out and
>> asserting.
> Correct.
> -Chris
Takashi Iwai Feb. 13, 2018, 1:29 p.m. UTC | #5
On Tue, 13 Feb 2018 13:47:25 +0100,
Chris Wilson wrote:
> 
> Quoting Kumar, Abhijeet (2018-02-13 12:41:42)
> > 
> > 
> > On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote:
> > 
> >     From: Abhijeet Kumar <abhijeet.kumar@intel.com>
> > 
> >     Finite loop and msleep was causing few igt@pm_rpm tests failure
> >     for HSW and BDW. Thus removing them.
> > 
> >     Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
> >                     sync power state")
> >     References: https://bugs.freedesktop.org/show_bug.cgi?id=105069
> > 
> >     Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
> >     ---
> >     Changes in v2:
> >     1. Removed msleep as well.
> >     2. Modified commit message.
> >      sound/hda/hdac_device.c | 8 +++-----
> >      1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> >     diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
> >     index 7ba100bb1c3f..678ef8950d0c 100644
> >     --- a/sound/hda/hdac_device.c
> >     +++ b/sound/hda/hdac_device.c
> >     @@ -1079,15 +1079,13 @@ 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;
> >     +       unsigned int state, actual_state;
> > 
> >     -       for (count = 0; count < 500; count++) {
> >     +       for (; ;) {
> >                     state = snd_hdac_codec_read(codec, nid, 0,
> >                                     AC_VERB_GET_POWER_STATE, 0);
> >     -               if (state & AC_PWRST_ERROR) {
> >     -                       msleep(20);
> >     +               if (state & AC_PWRST_ERROR)
> >                             break;
> >     -               }
> >                     actual_state = (state >> 4) & 0x0f;
> >                     if (actual_state == power_state)
> >                             break;
> > 
> > The above changes is as good as revert. But we can still repro the issue.
> 
> What about the different between snd_hda_codec_read() and
> snd_hdac_codec_read() ?
> 
> It used to pass &codec->core and now it's just using codec.

It's identical.  "codec" in the earlier code is struct snd_hda_codec,
and it embeds struct hdac_device in codec->core field.  So
&codec->core points to hdac_device object to be passed to
snd_hdac_codec_read().


Takashi
Abhijeet Kumar Feb. 13, 2018, 1:51 p.m. UTC | #6
On 2/13/2018 6:17 PM, Chris Wilson wrote:
> Quoting Kumar, Abhijeet (2018-02-13 12:41:42)
>>
>> On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote:
>>
>>      From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>
>>      Finite loop and msleep was causing few igt@pm_rpm tests failure
>>      for HSW and BDW. Thus removing them.
>>
>>      Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
>>                      sync power state")
>>      References: https://bugs.freedesktop.org/show_bug.cgi?id=105069
>>
>>      Signed-off-by: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>      ---
>>      Changes in v2:
>>      1. Removed msleep as well.
>>      2. Modified commit message.
>>       sound/hda/hdac_device.c | 8 +++-----
>>       1 file changed, 3 insertions(+), 5 deletions(-)
>>
>>      diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
>>      index 7ba100bb1c3f..678ef8950d0c 100644
>>      --- a/sound/hda/hdac_device.c
>>      +++ b/sound/hda/hdac_device.c
>>      @@ -1079,15 +1079,13 @@ 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;
>>      +       unsigned int state, actual_state;
>>
>>      -       for (count = 0; count < 500; count++) {
>>      +       for (; ;) {
>>                      state = snd_hdac_codec_read(codec, nid, 0,
>>                                      AC_VERB_GET_POWER_STATE, 0);
>>      -               if (state & AC_PWRST_ERROR) {
>>      -                       msleep(20);
>>      +               if (state & AC_PWRST_ERROR)
>>                              break;
>>      -               }
>>                      actual_state = (state >> 4) & 0x0f;
>>                      if (actual_state == power_state)
>>                              break;
>>
>> The above changes is as good as revert. But we can still repro the issue.
> What about the different between snd_hda_codec_read() and
> snd_hdac_codec_read() ?
>
> It used to pass &codec->core and now it's just using codec.
>   
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7993/all.html
>>
>> Are we sure that this is the only bad commit for regression ?
> Absolutely.
>
>> And by looking at the logs it seems like the reason for test failure is that
>> after disabling all the screen
>>
>> the device state has still not reached the suspended state. Thus timing out and
>> asserting.

One more thing to note in kernel log is that we don't see display 
power-well going off despite the fact we
had unset all crtcs. Does that mean (not all but some) screen are still 
enabled ?
> Correct.
> -Chris
diff mbox

Patch

diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 7ba100bb1c3f..678ef8950d0c 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -1079,15 +1079,13 @@  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;
+	unsigned int state, actual_state;
 
-	for (count = 0; count < 500; count++) {
+	for (; ;) {
 		state = snd_hdac_codec_read(codec, nid, 0,
 				AC_VERB_GET_POWER_STATE, 0);
-		if (state & AC_PWRST_ERROR) {
-			msleep(20);
+		if (state & AC_PWRST_ERROR)
 			break;
-		}
 		actual_state = (state >> 4) & 0x0f;
 		if (actual_state == power_state)
 			break;