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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxx wrote:

     From: Abhijeet Kumar <abhijeet.kumar@xxxxxxxxx>

     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@xxxxxxxxx>
     ---
     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

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux