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

["Kumar, Abhijeet" <abhijeet.kumar@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx