[PATCH 1/2] AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"

[PATCH 1/2] AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"

[Chris Wilson]

This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.

Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to sync power state")
Cc: Abhijeet Kumar <abhijeet.kumar@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c | 28 +++++++++++++++++++++++++++-
 sound/pci/hda/hda_local.h |  6 +-----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5bc3a7468e17..e018ecbf78a8 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2702,6 +2702,32 @@ void snd_hda_codec_set_power_to_all(struct hda_codec *codec, hda_nid_t fg,
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_set_power_to_all);
 
+/*
+ * wait until the state is reached, returns the current state
+ */
+static unsigned int hda_sync_power_state(struct hda_codec *codec,
+					 hda_nid_t fg,
+					 unsigned int power_state)
+{
+	unsigned long end_time = jiffies + msecs_to_jiffies(500);
+	unsigned int state, actual_state;
+
+	for (;;) {
+		state = snd_hda_codec_read(codec, fg, 0,
+					   AC_VERB_GET_POWER_STATE, 0);
+		if (state & AC_PWRST_ERROR)
+			break;
+		actual_state = (state >> 4) & 0x0f;
+		if (actual_state == power_state)
+			break;
+		if (time_after_eq(jiffies, end_time))
+			break;
+		/* wait until the codec reachs to the target state */
+		msleep(1);
+	}
+	return state;
+}
+
 /**
  * snd_hda_codec_eapd_power_filter - A power filter callback for EAPD
  * @codec: the HDA codec
@@ -2764,7 +2790,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
 						   state);
 			snd_hda_codec_set_power_to_all(codec, fg, power_state);
 		}
-		state = snd_hda_sync_power_state(codec, fg, power_state);
+		state = hda_sync_power_state(codec, fg, power_state);
 		if (!(state & AC_PWRST_ERROR))
 			break;
 	}
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 321e78baa63c..5b5c324c99b9 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -622,11 +622,7 @@ snd_hda_check_power_state(struct hda_codec *codec, hda_nid_t nid,
 {
 	return snd_hdac_check_power_state(&codec->core, nid, target_state);
 }
-static inline bool snd_hda_sync_power_state(struct hda_codec *codec,
-			   hda_nid_t nid, unsigned int target_state)
-{
-	return snd_hdac_sync_power_state(&codec->core, nid, target_state);
-}
+
 unsigned int snd_hda_codec_eapd_power_filter(struct hda_codec *codec,
 					     hda_nid_t nid,
 					     unsigned int power_state);
-- 
2.16.1

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

[PATCH 2/2] redo

[Chris Wilson]

---
 sound/pci/hda/hda_codec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e018ecbf78a8..afa5f5155220 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2711,12 +2711,15 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
 {
 	unsigned long end_time = jiffies + msecs_to_jiffies(500);
 	unsigned int state, actual_state;
+	int count;
 
-	for (;;) {
+	for (count = 0; count < 500; count++) {
 		state = snd_hda_codec_read(codec, fg, 0,
 					   AC_VERB_GET_POWER_STATE, 0);
-		if (state & AC_PWRST_ERROR)
+		if (state & AC_PWRST_ERROR) {
+			msleep(20);
 			break;
+		}
 		actual_state = (state >> 4) & 0x0f;
 		if (actual_state == power_state)
 			break;
-- 
2.16.1

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

Re: [PATCH 2/2] redo

[Chris Wilson]

Quoting Chris Wilson (2018-02-13 19:29:18)
> ---
>  sound/pci/hda/hda_codec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index e018ecbf78a8..afa5f5155220 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2711,12 +2711,15 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>  {
>         unsigned long end_time = jiffies + msecs_to_jiffies(500);
>         unsigned int state, actual_state;
> +       int count;
>  
> -       for (;;) {
> +       for (count = 0; count < 500; count++) {
>                 state = snd_hda_codec_read(codec, fg, 0,
>                                            AC_VERB_GET_POWER_STATE, 0);
> -               if (state & AC_PWRST_ERROR)
> +               if (state & AC_PWRST_ERROR) {
> +                       msleep(20);
>                         break;
> +               }

This was just to demonstrate that applying the obvious functional
changes from the common routine to hda_codec.c didn't break it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] Revert "ALSA: hda: Make use of core codec functions to sync power state"

[abhijeet.kumar]

From: Abhijeet Kumar <abhijeet.kumar@intel.com>

This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
---
 sound/pci/hda/hda_codec.c | 28 +++++++++++++++++++++++++++-
 sound/pci/hda/hda_local.h |  6 +-----
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 5bc3a7468e17..e018ecbf78a8 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2702,6 +2702,32 @@ void snd_hda_codec_set_power_to_all(struct hda_codec *codec, hda_nid_t fg,
 }
 EXPORT_SYMBOL_GPL(snd_hda_codec_set_power_to_all);
 
+/*
+ * wait until the state is reached, returns the current state
+ */
+static unsigned int hda_sync_power_state(struct hda_codec *codec,
+					 hda_nid_t fg,
+					 unsigned int power_state)
+{
+	unsigned long end_time = jiffies + msecs_to_jiffies(500);
+	unsigned int state, actual_state;
+
+	for (;;) {
+		state = snd_hda_codec_read(codec, fg, 0,
+					   AC_VERB_GET_POWER_STATE, 0);
+		if (state & AC_PWRST_ERROR)
+			break;
+		actual_state = (state >> 4) & 0x0f;
+		if (actual_state == power_state)
+			break;
+		if (time_after_eq(jiffies, end_time))
+			break;
+		/* wait until the codec reachs to the target state */
+		msleep(1);
+	}
+	return state;
+}
+
 /**
  * snd_hda_codec_eapd_power_filter - A power filter callback for EAPD
  * @codec: the HDA codec
@@ -2764,7 +2790,7 @@ static unsigned int hda_set_power_state(struct hda_codec *codec,
 						   state);
 			snd_hda_codec_set_power_to_all(codec, fg, power_state);
 		}
-		state = snd_hda_sync_power_state(codec, fg, power_state);
+		state = hda_sync_power_state(codec, fg, power_state);
 		if (!(state & AC_PWRST_ERROR))
 			break;
 	}
diff --git a/sound/pci/hda/hda_local.h b/sound/pci/hda/hda_local.h
index 321e78baa63c..5b5c324c99b9 100644
--- a/sound/pci/hda/hda_local.h
+++ b/sound/pci/hda/hda_local.h
@@ -622,11 +622,7 @@ int snd_hda_check_amp_list_power(struct hda_codec *codec,
 {
 	return snd_hdac_check_power_state(&codec->core, nid, target_state);
 }
-static inline bool snd_hda_sync_power_state(struct hda_codec *codec,
-			   hda_nid_t nid, unsigned int target_state)
-{
-	return snd_hdac_sync_power_state(&codec->core, nid, target_state);
-}
+
 unsigned int snd_hda_codec_eapd_power_filter(struct hda_codec *codec,
 					     hda_nid_t nid,
 					     unsigned int power_state);
-- 
1.9.1

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

[PATCH 2/3] redo

[abhijeet.kumar]

From: Abhijeet Kumar <abhijeet.kumar@intel.com>

---
 sound/pci/hda/hda_codec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index e018ecbf78a8..8c1b07e300a8 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2711,12 +2711,15 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
 {
 	unsigned long end_time = jiffies + msecs_to_jiffies(500);
 	unsigned int state, actual_state;
+	int count;
 
-	for (;;) {
+	for (count = 0;count < 500; count++) {
 		state = snd_hda_codec_read(codec, fg, 0,
 					   AC_VERB_GET_POWER_STATE, 0);
-		if (state & AC_PWRST_ERROR)
+		if (state & AC_PWRST_ERROR){
+			msleep(20);
 			break;
+		}
 		actual_state = (state >> 4) & 0x0f;
 		if (actual_state == power_state)
 			break;
-- 
1.9.1

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

[PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[abhijeet.kumar]

From: Abhijeet Kumar <abhijeet.kumar@intel.com>

---
 sound/pci/hda/hda_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 8c1b07e300a8..377d5719b4cd 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
 	int count;
 
 	for (count = 0;count < 500; count++) {
-		state = snd_hda_codec_read(codec, fg, 0,
+		state = snd_hdac_codec_read(&codec->core, fg, 0,
 					   AC_VERB_GET_POWER_STATE, 0);
 		if (state & AC_PWRST_ERROR){
 			msleep(20);
-- 
1.9.1

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

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

[-- Attachment #1.1: Type: text/plain, Size: 1153 bytes --]



On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
> From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>
> ---
>   sound/pci/hda/hda_codec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> index 8c1b07e300a8..377d5719b4cd 100644
> --- a/sound/pci/hda/hda_codec.c
> +++ b/sound/pci/hda/hda_codec.c
> @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>   	int count;
>   
>   	for (count = 0;count < 500; count++) {
> -		state = snd_hda_codec_read(codec, fg, 0,
> +		state = snd_hdac_codec_read(&codec->core, fg, 0,
>   					   AC_VERB_GET_POWER_STATE, 0);
>   		if (state & AC_PWRST_ERROR){
>   			msleep(20);

Both tests are passing on hsw and bdw devices.I can conclude that none 
of my changes
in "ALSA: hda: Make use of core codec functions to sync power state" is 
"*directly*" causing the regression.
As this patch series changes the previously defined sync function 
similar to the latest one (the one defined
in the defaulter patch).

Sidenote- Sorry for the missing commit message, it was just a HACK patch. :)

-Abhijeet

[-- Attachment #1.2: Type: text/html, Size: 1766 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Chris Wilson]

Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
> 
> 
> On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
> 
>     From: Abhijeet Kumar <abhijeet.kumar@intel.com>
> 
>     ---
>      sound/pci/hda/hda_codec.c | 2 +-
>      1 file changed, 1 insertion(+), 1 deletion(-)
> 
>     diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>     index 8c1b07e300a8..377d5719b4cd 100644
>     --- a/sound/pci/hda/hda_codec.c
>     +++ b/sound/pci/hda/hda_codec.c
>     @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>             int count;
> 
>             for (count = 0;count < 500; count++) {
>     -               state = snd_hda_codec_read(codec, fg, 0,
>     +               state = snd_hdac_codec_read(&codec->core, fg, 0,
>                                                AC_VERB_GET_POWER_STATE, 0);
>                     if (state & AC_PWRST_ERROR){
>                             msleep(20);
> 
> 
> Both tests are passing on hsw and bdw devices.I can conclude that none of my
> changes

Where did you run this against CI? (Due to the nature of patchwork it
will not have picked this up as a new revision.)

> in "ALSA: hda: Make use of core codec functions to sync power state" is "
> directly" causing the regression.
> As this patch series changes the previously defined sync function similar to
> the latest one (the one defined
> in the defaulter patch).

If you have no answer, we will apply the revert to our CI so that we do
not lose coverage.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

On 2/14/2018 2:17 PM, Chris Wilson wrote:
> Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
>>
>> On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
>>
>>      From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>
>>      ---
>>       sound/pci/hda/hda_codec.c | 2 +-
>>       1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>      diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>>      index 8c1b07e300a8..377d5719b4cd 100644
>>      --- a/sound/pci/hda/hda_codec.c
>>      +++ b/sound/pci/hda/hda_codec.c
>>      @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>>              int count;
>>
>>              for (count = 0;count < 500; count++) {
>>      -               state = snd_hda_codec_read(codec, fg, 0,
>>      +               state = snd_hdac_codec_read(&codec->core, fg, 0,
>>                                                 AC_VERB_GET_POWER_STATE, 0);
>>                      if (state & AC_PWRST_ERROR){
>>                              msleep(20);
>>
>>
>> Both tests are passing on hsw and bdw devices.I can conclude that none of my
>> changes
> Where did you run this against CI? (Due to the nature of patchwork it
> will not have picked this up as a new revision.)

You can find it here https://patchwork.freedesktop.org/series/38212/.
I've reverted my patch and made my changes in hda_codec inorder to 
demonstrate my changes is not
breaking it.
>
>> in "ALSA: hda: Make use of core codec functions to sync power state" is "
>> directly" causing the regression.
>> As this patch series changes the previously defined sync function similar to
>> the latest one (the one defined
>> in the defaulter patch).
> If you have no answer, we will apply the revert to our CI so that we do
> not lose coverage.

I guess, I don't have any issue by reverting this single patch alone as 
i already said this patch had
no functional change! It just had few optimization which  i believe we 
can skip for now.  :)

+Takashi to comment.

-Abhijeet
> -Chris

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

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Takashi Iwai]

On Wed, 14 Feb 2018 10:06:19 +0100,
Kumar, Abhijeet wrote:
> 
> 
> 
> On 2/14/2018 2:17 PM, Chris Wilson wrote:
> > Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
> >>
> >> On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
> >>
> >>      From: Abhijeet Kumar <abhijeet.kumar@intel.com>
> >>
> >>      ---
> >>       sound/pci/hda/hda_codec.c | 2 +-
> >>       1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>      diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
> >>      index 8c1b07e300a8..377d5719b4cd 100644
> >>      --- a/sound/pci/hda/hda_codec.c
> >>      +++ b/sound/pci/hda/hda_codec.c
> >>      @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
> >>              int count;
> >>
> >>              for (count = 0;count < 500; count++) {
> >>      -               state = snd_hda_codec_read(codec, fg, 0,
> >>      +               state = snd_hdac_codec_read(&codec->core, fg, 0,
> >>                                                 AC_VERB_GET_POWER_STATE, 0);
> >>                      if (state & AC_PWRST_ERROR){
> >>                              msleep(20);
> >>
> >>
> >> Both tests are passing on hsw and bdw devices.I can conclude that none of my
> >> changes
> > Where did you run this against CI? (Due to the nature of patchwork it
> > will not have picked this up as a new revision.)
> 
> You can find it here https://patchwork.freedesktop.org/series/38212/.
> I've reverted my patch and made my changes in hda_codec inorder to
> demonstrate my changes is not
> breaking it.
> >
> >> in "ALSA: hda: Make use of core codec functions to sync power state" is "
> >> directly" causing the regression.
> >> As this patch series changes the previously defined sync function similar to
> >> the latest one (the one defined
> >> in the defaulter patch).
> > If you have no answer, we will apply the revert to our CI so that we do
> > not lose coverage.
> 
> I guess, I don't have any issue by reverting this single patch alone
> as i already said this patch had
> no functional change! It just had few optimization which  i believe we
> can skip for now.  :)

Well, it still doesn't explain.  The loop count is 500 and we have
msleep(1), so it should be almost identical with the jiffies timeout.

We need more investigation, in which code path the bug is triggered.


thanks,

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

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Kumar, Abhijeet]

On 2/14/2018 4:48 PM, Takashi Iwai wrote:
> On Wed, 14 Feb 2018 10:06:19 +0100,
> Kumar, Abhijeet wrote:
>>
>>
>> On 2/14/2018 2:17 PM, Chris Wilson wrote:
>>> Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
>>>> On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
>>>>
>>>>       From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>>>
>>>>       ---
>>>>        sound/pci/hda/hda_codec.c | 2 +-
>>>>        1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>>       diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>>>>       index 8c1b07e300a8..377d5719b4cd 100644
>>>>       --- a/sound/pci/hda/hda_codec.c
>>>>       +++ b/sound/pci/hda/hda_codec.c
>>>>       @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>>>>               int count;
>>>>
>>>>               for (count = 0;count < 500; count++) {
>>>>       -               state = snd_hda_codec_read(codec, fg, 0,
>>>>       +               state = snd_hdac_codec_read(&codec->core, fg, 0,
>>>>                                                  AC_VERB_GET_POWER_STATE, 0);
>>>>                       if (state & AC_PWRST_ERROR){
>>>>                               msleep(20);
>>>>
>>>>
>>>> Both tests are passing on hsw and bdw devices.I can conclude that none of my
>>>> changes
>>> Where did you run this against CI? (Due to the nature of patchwork it
>>> will not have picked this up as a new revision.)
>> You can find it here https://patchwork.freedesktop.org/series/38212/.
>> I've reverted my patch and made my changes in hda_codec inorder to
>> demonstrate my changes is not
>> breaking it.
>>>> in "ALSA: hda: Make use of core codec functions to sync power state" is "
>>>> directly" causing the regression.
>>>> As this patch series changes the previously defined sync function similar to
>>>> the latest one (the one defined
>>>> in the defaulter patch).
>>> If you have no answer, we will apply the revert to our CI so that we do
>>> not lose coverage.
>> I guess, I don't have any issue by reverting this single patch alone
>> as i already said this patch had
>> no functional change! It just had few optimization which  i believe we
>> can skip for now.  :)
> Well, it still doesn't explain.  The loop count is 500 and we have
> msleep(1), so it should be almost identical with the jiffies timeout.

Even when the loop count is 500 and we have msleep(1) in earlier defined 
sync function.
We dont see wait_for_suspended assertion failing. See results for rev1 here
https://patchwork.freedesktop.org/series/38188/
> We need more investigation, in which code path the bug is triggered.

I'm convinced.It seems like some other issue which was hidden earlier 
had come into play
because of trivial code movement. It needs more triage. Can we enable 
debugs logs and
try the test again? I'm afraid I dont have the hardware with me locally 
right now.I'll try arranging.
Meanwhile maybe someone from CI team can help with logs ?

https://bugs.freedesktop.org/show_bug.cgi?id=105069

Warm Regards,
Abhijeet
>
>
> thanks,
>
> Takashi

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

Re: [PATCH 3/3] just some guess work to findout the culprit. If this breaks then we know what do.

[Jani Nikula]

On Wed, 14 Feb 2018, "Kumar, Abhijeet" <abhijeet.kumar@intel.com> wrote:
> On 2/14/2018 4:48 PM, Takashi Iwai wrote:
>> On Wed, 14 Feb 2018 10:06:19 +0100,
>> Kumar, Abhijeet wrote:
>>>
>>>
>>> On 2/14/2018 2:17 PM, Chris Wilson wrote:
>>>> Quoting Kumar, Abhijeet (2018-02-14 04:53:57)
>>>>> On 2/14/2018 9:36 AM, abhijeet.kumar@intel.com wrote:
>>>>>
>>>>>       From: Abhijeet Kumar <abhijeet.kumar@intel.com>
>>>>>
>>>>>       ---
>>>>>        sound/pci/hda/hda_codec.c | 2 +-
>>>>>        1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>>       diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
>>>>>       index 8c1b07e300a8..377d5719b4cd 100644
>>>>>       --- a/sound/pci/hda/hda_codec.c
>>>>>       +++ b/sound/pci/hda/hda_codec.c
>>>>>       @@ -2714,7 +2714,7 @@ static unsigned int hda_sync_power_state(struct hda_codec *codec,
>>>>>               int count;
>>>>>
>>>>>               for (count = 0;count < 500; count++) {
>>>>>       -               state = snd_hda_codec_read(codec, fg, 0,
>>>>>       +               state = snd_hdac_codec_read(&codec->core, fg, 0,
>>>>>                                                  AC_VERB_GET_POWER_STATE, 0);
>>>>>                       if (state & AC_PWRST_ERROR){
>>>>>                               msleep(20);
>>>>>
>>>>>
>>>>> Both tests are passing on hsw and bdw devices.I can conclude that none of my
>>>>> changes
>>>> Where did you run this against CI? (Due to the nature of patchwork it
>>>> will not have picked this up as a new revision.)
>>> You can find it here https://patchwork.freedesktop.org/series/38212/.
>>> I've reverted my patch and made my changes in hda_codec inorder to
>>> demonstrate my changes is not
>>> breaking it.
>>>>> in "ALSA: hda: Make use of core codec functions to sync power state" is "
>>>>> directly" causing the regression.
>>>>> As this patch series changes the previously defined sync function similar to
>>>>> the latest one (the one defined
>>>>> in the defaulter patch).
>>>> If you have no answer, we will apply the revert to our CI so that we do
>>>> not lose coverage.
>>> I guess, I don't have any issue by reverting this single patch alone
>>> as i already said this patch had
>>> no functional change! It just had few optimization which  i believe we
>>> can skip for now.  :)
>> Well, it still doesn't explain.  The loop count is 500 and we have
>> msleep(1), so it should be almost identical with the jiffies timeout.
>
> Even when the loop count is 500 and we have msleep(1) in earlier defined 
> sync function.
> We dont see wait_for_suspended assertion failing. See results for rev1 here
> https://patchwork.freedesktop.org/series/38188/
>> We need more investigation, in which code path the bug is triggered.
>
> I'm convinced.It seems like some other issue which was hidden earlier 
> had come into play
> because of trivial code movement. It needs more triage. Can we enable 
> debugs logs and
> try the test again? I'm afraid I dont have the hardware with me locally 
> right now.I'll try arranging.
> Meanwhile maybe someone from CI team can help with logs ?
>
> https://bugs.freedesktop.org/show_bug.cgi?id=105069

This remains an issue. The regressing commit is now in v4.17-rc1. We've
reverted it in our local branches to unblock CI.

Can we please get some closure here, if nothing else works then please
revert and go back to the drawing board.

BR,
Jani.



>
> Warm Regards,
> Abhijeet
>>
>>
>> thanks,
>>
>> Takashi
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/2] AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"

[Patchwork]

== Series Details ==

Series: series starting with [1/2] AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"
URL   : https://patchwork.freedesktop.org/series/38188/
State : failure

== Summary ==

Series 38188v1 series starting with [1/2] AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"
https://patchwork.freedesktop.org/api/1.0/series/38188/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-cnl-y3) fdo#105058
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> PASS       (fi-hsw-4770) fdo#105069 +3

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#105058 https://bugs.freedesktop.org/show_bug.cgi?id=105058
fdo#105069 https://bugs.freedesktop.org/show_bug.cgi?id=105069

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:485s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:288s
fi-bxt-dsi       total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:480s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:468s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:460s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:567s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:281s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:510s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:391s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:411s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:413s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:498s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:501s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:588s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:530s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:493s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:402s
Blacklisted hosts:
fi-glk-dsi       total:117  pass:105  dwarn:0   dfail:0   fail:0   skip:12 
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s

55dfdd18a566b5c632cbb34086b2a03410e810fb drm-tip: 2018y-02m-13d-18h-37m-11s UTC integration manifest
bd0e703cb15e redo
306b5cc96ff5 AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7999/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx