AWOOGA: Revert "ALSA: hda: Make use of core codec functions to sync power state"

Commit Message

Takashi Iwai Feb. 13, 2018, 8:14 a.m. UTC

On Tue, 13 Feb 2018 08:34:40 +0100,
Saarinen, Jani wrote:
> 
> HI, 
> > -----Original Message-----
> > From: Intel-gfx [mailto:[e-mail]] On Behalf Of
> > Takashi Iwai
> > Sent: tiistai 13. helmikuuta 2018 7.11
> > To: Chris Wilson <[e-mail]>
> > Cc: [e-mail]; Kumar, Abhijeet
> > <[e-mail]>
> > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
> > codec functions to sync power state"
> > 
> > On Mon, 12 Feb 2018 18:29:53 +0100,
> > Chris Wilson wrote:
> > >
> > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
> > >
> > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
> > > sync power state")
> > > Cc: Abhijeet Kumar <[e-mail]>
> > > Cc: Takashi Iwai <[e-mail]>
> > 
> > Did the patch break anything?
> > I don't understand it without any real context...
> Yes. See resutls from link what is fixed by reverting:
> https://patchwork.freedesktop.org/series/38097/
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 fail       -> PASS       (fi-hsw-4770)
>                 fail       -> PASS       (fi-bdw-5557u)
>         Subgroup basic-rte:
>                 fail       -> PASS       (fi-hsw-4770)
>                 fail       -> PASS       (fi-bdw-5557u)

Could you investigate why does the revert fix?
Two functions are almost identical.  The difference is that the new
one has a counter and quit at 500 iteration, and it has a msleep(200)
at the error exit.  But both shouldn't matter for the normal
operation...

FWIW, below is the patch to make the new function identical with the
original function.  Does it change the behavior?


thanks,

Takashi

Comments

Saarinen, Jani Feb. 13, 2018, 8:26 a.m. UTC | #1

Hi, 
> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: tiistai 13. helmikuuta 2018 10.14
> To: Saarinen, Jani <[e-mail]>
> Cc: Chris Wilson <[e-mail]>; [e-mail];
> Kumar, Abhijeet <[e-mail]>
> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
> codec functions to sync power state"
> 
> On Tue, 13 Feb 2018 08:34:40 +0100,
> Saarinen, Jani wrote:
> >
> > HI,
> > > -----Original Message-----
> > > From: Intel-gfx [mailto:[e-mail]] On
> > > Behalf Of Takashi Iwai
> > > Sent: tiistai 13. helmikuuta 2018 7.11
> > > To: Chris Wilson <[e-mail]>
> > > Cc: [e-mail]; Kumar, Abhijeet
> > > <[e-mail]>
> > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use
> > > of core codec functions to sync power state"
> > >
> > > On Mon, 12 Feb 2018 18:29:53 +0100,
> > > Chris Wilson wrote:
> > > >
> > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
> > > >
> > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
> > > > to sync power state")
> > > > Cc: Abhijeet Kumar <[e-mail]>
> > > > Cc: Takashi Iwai <[e-mail]>
> > >
> > > Did the patch break anything?
> > > I don't understand it without any real context...
> > Yes. See resutls from link what is fixed by reverting:
> > https://patchwork.freedesktop.org/series/38097/
> > Test pm_rpm:
> >         Subgroup basic-pci-d3-state:
> >                 fail       -> PASS       (fi-hsw-4770)
> >                 fail       -> PASS       (fi-bdw-5557u)
> >         Subgroup basic-rte:
> >                 fail       -> PASS       (fi-hsw-4770)
> >                 fail       -> PASS       (fi-bdw-5557u)
> 
> Could you investigate why does the revert fix?
Maybe you instead.

> Two functions are almost identical.  The difference is that the new one has a
> counter and quit at 500 iteration, and it has a msleep(200) at the error exit.  But
> both shouldn't matter for the normal operation...
> 
> FWIW, below is the patch to make the new function identical with the original
> function.  Does it change the behavior?
> 
Just send it for ml so that CI can test. 
> 
> thanks,
> 
> Takashi
Br,
Jani

> 
> 
> --- 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;

Abhijeet Kumar Feb. 13, 2018, 8:27 a.m. UTC | #2

On 2/13/2018 1:44 PM, Takashi Iwai wrote:
> On Tue, 13 Feb 2018 08:34:40 +0100,
> Saarinen, Jani wrote:
>> HI,
>>> -----Original Message-----
>>> From: Intel-gfx [mailto:[e-mail]] On Behalf Of
>>> Takashi Iwai
>>> Sent: tiistai 13. helmikuuta 2018 7.11
>>> To: Chris Wilson <[e-mail]>
>>> Cc: [e-mail]; Kumar, Abhijeet
>>> <[e-mail]>
>>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
>>> codec functions to sync power state"
>>>
>>> On Mon, 12 Feb 2018 18:29:53 +0100,
>>> Chris Wilson wrote:
>>>> This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
>>>>
>>>> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions to
>>>> sync power state")
>>>> Cc: Abhijeet Kumar <[e-mail]>
>>>> Cc: Takashi Iwai <[e-mail]>
>>> Did the patch break anything?
>>> I don't understand it without any real context...
>> Yes. See resutls from link what is fixed by reverting:
>> https://patchwork.freedesktop.org/series/38097/
>> Test pm_rpm:
>>          Subgroup basic-pci-d3-state:
>>                  fail       -> PASS       (fi-hsw-4770)
>>                  fail       -> PASS       (fi-bdw-5557u)
>>          Subgroup basic-rte:
>>                  fail       -> PASS       (fi-hsw-4770)
>>                  fail       -> PASS       (fi-bdw-5557u)
> Could you investigate why does the revert fix?
> Two functions are almost identical.  The difference is that the new
> one has a counter and quit at 500 iteration, and it has a msleep(200)
> at the error exit.  But both shouldn't matter for the normal
> operation...
>
> FWIW, below is the patch to make the new function identical with the
> original function.  Does it change the behavior?
>
>
> thanks,
>
> Takashi
>
>
> --- 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 (;;) {
I believe changing it back to finite should fix the regression.
>   		state = snd_hdac_codec_read(codec, nid, 0,
>   				AC_VERB_GET_POWER_STATE, 0);
> -		if (state & AC_PWRST_ERROR) {
> -			msleep(20);

When device raises the error state bit, we might endup sending two verbs 
in back to
back manner.I would still recommend to have a 20ms sleep in-order to let 
the device
clean up it's internal state.
> +		if (state & AC_PWRST_ERROR)
>   			break;
> -		}
>   		actual_state = (state >> 4) & 0x0f;
>   		if (actual_state == power_state)
>   			break;

Takashi Iwai Feb. 13, 2018, 8:30 a.m. UTC | #3

On Tue, 13 Feb 2018 09:26:32 +0100,
Saarinen, Jani wrote:
> 
> Hi, 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: tiistai 13. helmikuuta 2018 10.14
> > To: Saarinen, Jani <[e-mail]>
> > Cc: Chris Wilson <[e-mail]>; [e-mail];
> > Kumar, Abhijeet <[e-mail]>
> > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
> > codec functions to sync power state"
> > 
> > On Tue, 13 Feb 2018 08:34:40 +0100,
> > Saarinen, Jani wrote:
> > >
> > > HI,
> > > > -----Original Message-----
> > > > From: Intel-gfx [mailto:[e-mail]] On
> > > > Behalf Of Takashi Iwai
> > > > Sent: tiistai 13. helmikuuta 2018 7.11
> > > > To: Chris Wilson <[e-mail]>
> > > > Cc: [e-mail]; Kumar, Abhijeet
> > > > <[e-mail]>
> > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use
> > > > of core codec functions to sync power state"
> > > >
> > > > On Mon, 12 Feb 2018 18:29:53 +0100,
> > > > Chris Wilson wrote:
> > > > >
> > > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
> > > > >
> > > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
> > > > > to sync power state")
> > > > > Cc: Abhijeet Kumar <[e-mail]>
> > > > > Cc: Takashi Iwai <[e-mail]>
> > > >
> > > > Did the patch break anything?
> > > > I don't understand it without any real context...
> > > Yes. See resutls from link what is fixed by reverting:
> > > https://patchwork.freedesktop.org/series/38097/
> > > Test pm_rpm:
> > >         Subgroup basic-pci-d3-state:
> > >                 fail       -> PASS       (fi-hsw-4770)
> > >                 fail       -> PASS       (fi-bdw-5557u)
> > >         Subgroup basic-rte:
> > >                 fail       -> PASS       (fi-hsw-4770)
> > >                 fail       -> PASS       (fi-bdw-5557u)
> > 
> > Could you investigate why does the revert fix?
> Maybe you instead.

Once when a proper bug report is sent to us upstream :)

> > Two functions are almost identical.  The difference is that the new one has a
> > counter and quit at 500 iteration, and it has a msleep(200) at the error exit.  But
> > both shouldn't matter for the normal operation...
> > 
> > FWIW, below is the patch to make the new function identical with the original
> > function.  Does it change the behavior?
> > 
> Just send it for ml so that CI can test. 

I guess Abhijeet can manage it better.  It's Intel stuff, after all.


Takashi

Abhijeet Kumar Feb. 13, 2018, 8:32 a.m. UTC | #4

On 2/13/2018 2:00 PM, Takashi Iwai wrote:
> On Tue, 13 Feb 2018 09:26:32 +0100,
> Saarinen, Jani wrote:
>> Hi,
>>> -----Original Message-----
>>> From: Takashi Iwai [mailto:[e-mail]]
>>> Sent: tiistai 13. helmikuuta 2018 10.14
>>> To: Saarinen, Jani <[e-mail]>
>>> Cc: Chris Wilson <[e-mail]>; [e-mail];
>>> Kumar, Abhijeet <[e-mail]>
>>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
>>> codec functions to sync power state"
>>>
>>> On Tue, 13 Feb 2018 08:34:40 +0100,
>>> Saarinen, Jani wrote:
>>>> HI,
>>>>> -----Original Message-----
>>>>> From: Intel-gfx [mailto:[e-mail]] On
>>>>> Behalf Of Takashi Iwai
>>>>> Sent: tiistai 13. helmikuuta 2018 7.11
>>>>> To: Chris Wilson <[e-mail]>
>>>>> Cc: [e-mail]; Kumar, Abhijeet
>>>>> <[e-mail]>
>>>>> Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use
>>>>> of core codec functions to sync power state"
>>>>>
>>>>> On Mon, 12 Feb 2018 18:29:53 +0100,
>>>>> Chris Wilson wrote:
>>>>>> This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
>>>>>>
>>>>>> Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
>>>>>> to sync power state")
>>>>>> Cc: Abhijeet Kumar <[e-mail]>
>>>>>> Cc: Takashi Iwai <[e-mail]>
>>>>> Did the patch break anything?
>>>>> I don't understand it without any real context...
>>>> Yes. See resutls from link what is fixed by reverting:
>>>> https://patchwork.freedesktop.org/series/38097/
>>>> Test pm_rpm:
>>>>          Subgroup basic-pci-d3-state:
>>>>                  fail       -> PASS       (fi-hsw-4770)
>>>>                  fail       -> PASS       (fi-bdw-5557u)
>>>>          Subgroup basic-rte:
>>>>                  fail       -> PASS       (fi-hsw-4770)
>>>>                  fail       -> PASS       (fi-bdw-5557u)
>>> Could you investigate why does the revert fix?
>> Maybe you instead.
> Once when a proper bug report is sent to us upstream :)
>
>>> Two functions are almost identical.  The difference is that the new one has a
>>> counter and quit at 500 iteration, and it has a msleep(200) at the error exit.  But
>>> both shouldn't matter for the normal operation...
>>>
>>> FWIW, below is the patch to make the new function identical with the original
>>> function.  Does it change the behavior?
>>>
>> Just send it for ml so that CI can test.
> I guess Abhijeet can manage it better.  It's Intel stuff, after all.
I would make those changes and share it to ml.
>
> Takashi

Chris Wilson Feb. 13, 2018, 8:37 a.m. UTC | #5

Quoting Takashi Iwai (2018-02-13 08:30:52)
> On Tue, 13 Feb 2018 09:26:32 +0100,
> Saarinen, Jani wrote:
> > 
> > Hi, 
> > > -----Original Message-----
> > > From: Takashi Iwai [mailto:[e-mail]]
> > > Sent: tiistai 13. helmikuuta 2018 10.14
> > > To: Saarinen, Jani <[e-mail]>
> > > Cc: Chris Wilson <[e-mail]>; [e-mail];
> > > Kumar, Abhijeet <[e-mail]>
> > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use of core
> > > codec functions to sync power state"
> > > 
> > > On Tue, 13 Feb 2018 08:34:40 +0100,
> > > Saarinen, Jani wrote:
> > > >
> > > > HI,
> > > > > -----Original Message-----
> > > > > From: Intel-gfx [mailto:[e-mail]] On
> > > > > Behalf Of Takashi Iwai
> > > > > Sent: tiistai 13. helmikuuta 2018 7.11
> > > > > To: Chris Wilson <[e-mail]>
> > > > > Cc: [e-mail]; Kumar, Abhijeet
> > > > > <[e-mail]>
> > > > > Subject: Re: [Intel-gfx] [PATCH] AWOOGA: Revert "ALSA: hda: Make use
> > > > > of core codec functions to sync power state"
> > > > >
> > > > > On Mon, 12 Feb 2018 18:29:53 +0100,
> > > > > Chris Wilson wrote:
> > > > > >
> > > > > > This reverts commit 3b5b899ca67db07a4c4825911072221f99e157e2.
> > > > > >
> > > > > > Fixes: 3b5b899ca67d ("ALSA: hda: Make use of core codec functions
> > > > > > to sync power state")
> > > > > > Cc: Abhijeet Kumar <[e-mail]>
> > > > > > Cc: Takashi Iwai <[e-mail]>
> > > > >
> > > > > Did the patch break anything?
> > > > > I don't understand it without any real context...
> > > > Yes. See resutls from link what is fixed by reverting:
> > > > https://patchwork.freedesktop.org/series/38097/
> > > > Test pm_rpm:
> > > >         Subgroup basic-pci-d3-state:
> > > >                 fail       -> PASS       (fi-hsw-4770)
> > > >                 fail       -> PASS       (fi-bdw-5557u)
> > > >         Subgroup basic-rte:
> > > >                 fail       -> PASS       (fi-hsw-4770)
> > > >                 fail       -> PASS       (fi-bdw-5557u)
> > > 
> > > Could you investigate why does the revert fix?
> > Maybe you instead.
> 
> Once when a proper bug report is sent to us upstream :)

Now there's a challenge for the dim masters, make dim fixes provide the
right link. As it was it only gave the authors' email, and I thought the
heads up was appropriate.
-Chris

Patch

--- 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;