[PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Hui Wang]

This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
for all hda controllers").

The reverted patch already introduced some regressions on some
machines:
 - on gemini-lake machines, the error of "azx_get_response timeout"
   happens in the hda driver.
 - on the machines with alc662 codec, the audio jack detection doesn't
   work anymore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
Cc: <stable@vger.kernel.org>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 sound/pci/hda/hda_intel.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e699873c8293..e34a4d5d047c 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
 
 	if (azx_has_pm_runtime(chip)) {
 		pm_runtime_use_autosuspend(&pci->dev);
-		pm_runtime_allow(&pci->dev);
 		pm_runtime_put_autosuspend(&pci->dev);
 	}
 
-- 
2.17.1

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Takashi Iwai]

On Mon, 03 Aug 2020 08:46:38 +0200,
Hui Wang wrote:
> 
> This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
> for all hda controllers").
> 
> The reverted patch already introduced some regressions on some
> machines:
>  - on gemini-lake machines, the error of "azx_get_response timeout"
>    happens in the hda driver.
>  - on the machines with alc662 codec, the audio jack detection doesn't
>    work anymore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Thanks, applied.


Takashi

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Pierre-Louis Bossart]

On 8/3/20 1:46 AM, Hui Wang wrote:
> This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
> for all hda controllers").
> 
> The reverted patch already introduced some regressions on some
> machines:
>   - on gemini-lake machines, the error of "azx_get_response timeout"
>     happens in the hda driver.
>   - on the machines with alc662 codec, the audio jack detection doesn't
>     work anymore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> ---
>   sound/pci/hda/hda_intel.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e699873c8293..e34a4d5d047c 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
>   
>   	if (azx_has_pm_runtime(chip)) {
>   		pm_runtime_use_autosuspend(&pci->dev);
> -		pm_runtime_allow(&pci->dev);
>   		pm_runtime_put_autosuspend(&pci->dev);
>   	}

Do I get this right that this permanently disables pm_runtime on all 
Intel HDaudio controllers?

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Takashi Iwai]

On Mon, 03 Aug 2020 17:27:12 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 8/3/20 1:46 AM, Hui Wang wrote:
> > This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
> > for all hda controllers").
> >
> > The reverted patch already introduced some regressions on some
> > machines:
> >   - on gemini-lake machines, the error of "azx_get_response timeout"
> >     happens in the hda driver.
> >   - on the machines with alc662 codec, the audio jack detection doesn't
> >     work anymore.
> >
> > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> > ---
> >   sound/pci/hda/hda_intel.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index e699873c8293..e34a4d5d047c 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
> >     	if (azx_has_pm_runtime(chip)) {
> >   		pm_runtime_use_autosuspend(&pci->dev);
> > -		pm_runtime_allow(&pci->dev);
> >   		pm_runtime_put_autosuspend(&pci->dev);
> >   	}
> 
> Do I get this right that this permanently disables pm_runtime on all
> Intel HDaudio controllers?

It just drops the unconditional enablement of runtime PM.
It can be enabled via sysfs, and that's the old default (let admin
enabling it via udev or whatever).


thanks,

Takashi

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Pierre-Louis Bossart]

On 8/3/20 11:36 AM, Takashi Iwai wrote:
> On Mon, 03 Aug 2020 17:27:12 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>> On 8/3/20 1:46 AM, Hui Wang wrote:
>>> This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
>>> for all hda controllers").
>>>
>>> The reverted patch already introduced some regressions on some
>>> machines:
>>>    - on gemini-lake machines, the error of "azx_get_response timeout"
>>>      happens in the hda driver.
>>>    - on the machines with alc662 codec, the audio jack detection doesn't
>>>      work anymore.
>>>
>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>> ---
>>>    sound/pci/hda/hda_intel.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>> index e699873c8293..e34a4d5d047c 100644
>>> --- a/sound/pci/hda/hda_intel.c
>>> +++ b/sound/pci/hda/hda_intel.c
>>> @@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
>>>      	if (azx_has_pm_runtime(chip)) {
>>>    		pm_runtime_use_autosuspend(&pci->dev);
>>> -		pm_runtime_allow(&pci->dev);
>>>    		pm_runtime_put_autosuspend(&pci->dev);
>>>    	}
>>
>> Do I get this right that this permanently disables pm_runtime on all
>> Intel HDaudio controllers?
> 
> It just drops the unconditional enablement of runtime PM.
> It can be enabled via sysfs, and that's the old default (let admin
> enabling it via udev or whatever).

Sorry I am confused now.
Kai seemed to suggest in the Bugzilla comments that this would be 
temporary, until these problems with i915 and ALC662 get fixed?

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Takashi Iwai]

On Mon, 03 Aug 2020 19:00:30 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 8/3/20 11:36 AM, Takashi Iwai wrote:
> > On Mon, 03 Aug 2020 17:27:12 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >>
> >>
> >> On 8/3/20 1:46 AM, Hui Wang wrote:
> >>> This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
> >>> for all hda controllers").
> >>>
> >>> The reverted patch already introduced some regressions on some
> >>> machines:
> >>>    - on gemini-lake machines, the error of "azx_get_response timeout"
> >>>      happens in the hda driver.
> >>>    - on the machines with alc662 codec, the audio jack detection doesn't
> >>>      work anymore.
> >>>
> >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
> >>> ---
> >>>    sound/pci/hda/hda_intel.c | 1 -
> >>>    1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> >>> index e699873c8293..e34a4d5d047c 100644
> >>> --- a/sound/pci/hda/hda_intel.c
> >>> +++ b/sound/pci/hda/hda_intel.c
> >>> @@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
> >>>      	if (azx_has_pm_runtime(chip)) {
> >>>    		pm_runtime_use_autosuspend(&pci->dev);
> >>> -		pm_runtime_allow(&pci->dev);
> >>>    		pm_runtime_put_autosuspend(&pci->dev);
> >>>    	}
> >>
> >> Do I get this right that this permanently disables pm_runtime on all
> >> Intel HDaudio controllers?
> >
> > It just drops the unconditional enablement of runtime PM.
> > It can be enabled via sysfs, and that's the old default (let admin
> > enabling it via udev or whatever).
> 
> Sorry I am confused now.
> Kai seemed to suggest in the Bugzilla comments that this would be
> temporary, until these problems with i915 and ALC662 get fixed?

Right, that's the plan.  This patch revert to the old state before the
forced-all-enable call we've taken in 5.7.  On 5.7 and onwards, all
HD-audio controllers are enforced to use the runtime PM.  Before that
version, the runtime PM was enabled *as default* only for limited
devices (typically the ones bound with GPU); for other devices, the
runtime PM is manually enabled from user-space via sysfs (and many
distros enable them in anyway).

The forced enablement was merged with a hope that now all HD-audio
controllers behave nicely, but it turned out to cause a regression, so
it was reverted.  Once when we find out the real cause, we can flip
the flag again.


Takashi

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Pierre-Louis Bossart]

>>>> Do I get this right that this permanently disables pm_runtime on all
>>>> Intel HDaudio controllers?
>>>
>>> It just drops the unconditional enablement of runtime PM.
>>> It can be enabled via sysfs, and that's the old default (let admin
>>> enabling it via udev or whatever).
>>
>> Sorry I am confused now.
>> Kai seemed to suggest in the Bugzilla comments that this would be
>> temporary, until these problems with i915 and ALC662 get fixed?
> 
> Right, that's the plan.  This patch revert to the old state before the
> forced-all-enable call we've taken in 5.7.  On 5.7 and onwards, all
> HD-audio controllers are enforced to use the runtime PM.  Before that
> version, the runtime PM was enabled *as default* only for limited
> devices (typically the ones bound with GPU); for other devices, the
> runtime PM is manually enabled from user-space via sysfs (and many
> distros enable them in anyway).
> 
> The forced enablement was merged with a hope that now all HD-audio
> controllers behave nicely, but it turned out to cause a regression, so
> it was reverted.  Once when we find out the real cause, we can flip
> the flag again.

ok, sounds good. I was concerned mainly because on the SOF driver side 
we enable pm_runtime by default, so that's a difference in configuration 
we need to be aware of when dealing with 'my speaker is silent' support 
questions.

Re: [PATCH] Revert "ALSA: hda: call runtime_allow() for all hda controllers"

[Hui Wang]

On 2020/8/4 上午1:00, Pierre-Louis Bossart wrote:
>
>
> On 8/3/20 11:36 AM, Takashi Iwai wrote:
>> On Mon, 03 Aug 2020 17:27:12 +0200,
>> Pierre-Louis Bossart wrote:
>>>
>>>
>>>
>>> On 8/3/20 1:46 AM, Hui Wang wrote:
>>>> This reverts commit 9a6418487b56 ("ALSA: hda: call runtime_allow()
>>>> for all hda controllers").
>>>>
>>>> The reverted patch already introduced some regressions on some
>>>> machines:
>>>>    - on gemini-lake machines, the error of "azx_get_response timeout"
>>>>      happens in the hda driver.
>>>>    - on the machines with alc662 codec, the audio jack detection 
>>>> doesn't
>>>>      work anymore.
>>>>
>>>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=208511
>>>> Cc: <stable@vger.kernel.org>
>>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>>>> ---
>>>>    sound/pci/hda/hda_intel.c | 1 -
>>>>    1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
>>>> index e699873c8293..e34a4d5d047c 100644
>>>> --- a/sound/pci/hda/hda_intel.c
>>>> +++ b/sound/pci/hda/hda_intel.c
>>>> @@ -2352,7 +2352,6 @@ static int azx_probe_continue(struct azx *chip)
>>>>          if (azx_has_pm_runtime(chip)) {
>>>>            pm_runtime_use_autosuspend(&pci->dev);
>>>> -        pm_runtime_allow(&pci->dev);
>>>>            pm_runtime_put_autosuspend(&pci->dev);
>>>>        }
>>>
>>> Do I get this right that this permanently disables pm_runtime on all
>>> Intel HDaudio controllers?
>>
>> It just drops the unconditional enablement of runtime PM.
>> It can be enabled via sysfs, and that's the old default (let admin
>> enabling it via udev or whatever).
>
> Sorry I am confused now.
> Kai seemed to suggest in the Bugzilla comments that this would be 
> temporary, until these problems with i915 and ALC662 get fixed?

I planed to debug the issue of ALC662, but so far, I haven't got the 
machine,  It is difficult to debug power issues without a physical 
machine. Once I get the machine, I will debug it and try to find the 
root cause.

Thanks,

Hui.