From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Kumar, Abhijeet" Subject: Re: [PATCH v2] ALSA: hda: Remove finite loop from snd_hdac_sync_power_state() Date: Tue, 13 Feb 2018 18:11:42 +0530 Message-ID: References: <1518512988-24305-1-git-send-email-abhijeet.kumar@intel.com> <1518517484-24488-1-git-send-email-abhijeet.kumar@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0525995144==" Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 0D4BE6E0B2 for ; Tue, 13 Feb 2018 12:41:45 +0000 (UTC) In-Reply-To: <1518517484-24488-1-git-send-email-abhijeet.kumar@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Patchwork , intel-gfx@lists.freedesktop.org, Chris Wilson , "Saarinen, Jani" , Takashi Iwai List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0525995144== Content-Type: multipart/alternative; boundary="------------610D2CBB5D4E394FB9C0FB3A" Content-Language: en-US This is a multi-part message in MIME format. --------------610D2CBB5D4E394FB9C0FB3A Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 2/13/2018 3:54 PM, abhijeet.kumar@intel.com wrote: > From: Abhijeet Kumar > > 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 > --- > 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 --------------610D2CBB5D4E394FB9C0FB3A Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit



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





--------------610D2CBB5D4E394FB9C0FB3A-- --===============0525995144== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0525995144==--