| Message ID | 1518517484-24488-1-git-send-email-abhijeet.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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 --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;