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

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

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:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>>> Takashi Iwai
>>> Sent: tiistai 13. helmikuuta 2018 7.11
>>> To: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: intel-gfx@lists.freedesktop.org; Kumar, Abhijeet
>>> <abhijeet.kumar@intel.com>
>>> 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 <abhijeet.kumar@intel.com>
>>>> Cc: Takashi Iwai <tiwai@suse.de>
>>> 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;

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