Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Marian Postevca]

I'm trying to develop a sound machine driver based on the acp legacy driver.
The first version of the driver was sent for review on the alsa mailing list this
spring: https://lore.kernel.org/all/20230320203519.20137-1-posteuca@mutex.one

I'm trying to fix some of the issues that were brought up during the review back then,
but when I ported the patches to the latest commit on the for-next
branch, I noticed a regression where I couldn't hear any sound at all.

So I started a bisect session and found that the first bad commit is:
ASoC: amd: acp: add pm ops support for acp pci driver
commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2
https://lore.kernel.org/lkml/20230622152406.3709231-11-Syed.SabaKareem@amd.com

If I revert this commit sound works as expected. So I started tinkering a little bit
with it and I believe that what happens is that the acp pci driver
enters the autosuspend state and never leaves this state at all.
I noticed this because if I increase the autosuspend delay to a much
larger value, then the sound works until that delay passes.
I added traces and I can see that when the delay expires the suspend callback snd_acp_suspend()
gets called, but the resume callback snd_acp_resume() never gets called.

I'm no expert in runtime power management (though I did read a bit on it), so I don't understand
all the things that happen underneath, but one thing that is not clear to me is who's supposed
to mark activity on this device and keep it from entering autosuspend if the user wants to play
some sound? Shouldn't there be some counterpart that calls pm_runtime_mark_last_busy() ?
I looked through the code and can't find who's calling pm_runtime_mark_last_busy().

Some help here would be welcome. Is there something missing in my machine driver code, or
is the runtime pm handling in acp pci driver wrong?

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 951 bytes --]

On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> I'm no expert in runtime power management (though I did read a bit on
> it), so I don't understand all the things that happen underneath, but
> one thing that is not clear to me is who's supposed to mark activity
> on this device and keep it from entering autosuspend if the user wants
> to play some sound? Shouldn't there be some counterpart that calls
> pm_runtime_mark_last_busy() ?  I looked through the code and can't
> find who's calling pm_runtime_mark_last_busy().

The core will hold devices out of suspend through a combination of DAPM
and PCMs being held open, it just does get and puts rather than making
an effort to use autosuspend delays.  See the calls to
pm_runtime_get_sync() in the core.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[syed saba kareem]

On 8/4/23 00:52, Marian Postevca wrote:
> I'm trying to develop a sound machine driver based on the acp legacy driver.
> The first version of the driver was sent for review on the alsa mailing list this
> spring: https://lore.kernel.org/all/20230320203519.20137-1-posteuca@mutex.one
>
> I'm trying to fix some of the issues that were brought up during the review back then,
> but when I ported the patches to the latest commit on the for-next
> branch, I noticed a regression where I couldn't hear any sound at all.
>
> So I started a bisect session and found that the first bad commit is:
> ASoC: amd: acp: add pm ops support for acp pci driver
> commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2
> https://lore.kernel.org/lkml/20230622152406.3709231-11-Syed.SabaKareem@amd.com
>
> If I revert this commit sound works as expected. So I started tinkering a little bit
> with it and I believe that what happens is that the acp pci driver
> enters the autosuspend state and never leaves this state at all.
> I noticed this because if I increase the autosuspend delay to a much
> larger value, then the sound works until that delay passes.
> I added traces and I can see that when the delay expires the suspend callback snd_acp_suspend()
> gets called, but the resume callback snd_acp_resume() never gets called.
>
> I'm no expert in runtime power management (though I did read a bit on it), so I don't understand
> all the things that happen underneath, but one thing that is not clear to me is who's supposed
> to mark activity on this device and keep it from entering autosuspend if the user wants to play
> some sound? Shouldn't there be some counterpart that calls pm_runtime_mark_last_busy() ?
> I looked through the code and can't find who's calling pm_runtime_mark_last_busy().
>
> Some help here would be welcome. Is there something missing in my machine driver code, or
> is the runtime pm handling in acp pci driver wrong?

We haven't up streamed pm ops for Renoir platform.

That is the cause for the issue.

Will upstream them in a week.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Marian Postevca]

syed saba kareem <ssabakar@amd.com> writes:

> We haven't up streamed pm ops for Renoir platform.
>
> That is the cause for the issue.
>
> Will upstream them in a week.

Did you manage to upstream the pm ops for Renoir platform?
I have checked the latest commit on the for-next branch
( ed2232d49187cebc007ecf4e6374069b11ab3219 ) and the issue is still
there.
I still have to revert commit
088a40980efbc2c449b72f0f2c7ebd82f71d08e2 to have my driver function
properly.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
> I'm trying to develop a sound machine driver based on the acp legacy driver.
> The first version of the driver was sent for review on the alsa mailing list this
> spring: https://lore.kernel.org/all/20230320203519.20137-1-posteuca@mutex.one
> 
> I'm trying to fix some of the issues that were brought up during the review back then,
> but when I ported the patches to the latest commit on the for-next
> branch, I noticed a regression where I couldn't hear any sound at all.
> 
> So I started a bisect session and found that the first bad commit is:
> ASoC: amd: acp: add pm ops support for acp pci driver
> commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2
> https://lore.kernel.org/lkml/20230622152406.3709231-11-Syed.SabaKareem@amd.com
> 
> If I revert this commit sound works as expected. So I started tinkering a little bit
> with it and I believe that what happens is that the acp pci driver
> enters the autosuspend state and never leaves this state at all.
> I noticed this because if I increase the autosuspend delay to a much
> larger value, then the sound works until that delay passes.
> I added traces and I can see that when the delay expires the suspend callback snd_acp_suspend()
> gets called, but the resume callback snd_acp_resume() never gets called.
> 
> I'm no expert in runtime power management (though I did read a bit on it), so I don't understand
> all the things that happen underneath, but one thing that is not clear to me is who's supposed
> to mark activity on this device and keep it from entering autosuspend if the user wants to play
> some sound? Shouldn't there be some counterpart that calls pm_runtime_mark_last_busy() ?
> I looked through the code and can't find who's calling pm_runtime_mark_last_busy().
> 
> Some help here would be welcome. Is there something missing in my machine driver code, or
> is the runtime pm handling in acp pci driver wrong?

Thanks for the regression report. I'm adding it to regzbot:

#regzbot ^introduced: 088a40980efbc2

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[syed saba kareem]

On 11/3/23 04:41, Bagas Sanjaya wrote:
> On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
>> I'm trying to develop a sound machine driver based on the acp legacy driver.
>> The first version of the driver was sent for review on the alsa mailing list this
>> spring: https://lore.kernel.org/all/20230320203519.20137-1-posteuca@mutex.one
>>
>> I'm trying to fix some of the issues that were brought up during the review back then,
>> but when I ported the patches to the latest commit on the for-next
>> branch, I noticed a regression where I couldn't hear any sound at all.
>>
>> So I started a bisect session and found that the first bad commit is:
>> ASoC: amd: acp: add pm ops support for acp pci driver
>> commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2
>> https://lore.kernel.org/lkml/20230622152406.3709231-11-Syed.SabaKareem@amd.com
>>
>> If I revert this commit sound works as expected. So I started tinkering a little bit
>> with it and I believe that what happens is that the acp pci driver
>> enters the autosuspend state and never leaves this state at all.
>> I noticed this because if I increase the autosuspend delay to a much
>> larger value, then the sound works until that delay passes.
>> I added traces and I can see that when the delay expires the suspend callback snd_acp_suspend()
>> gets called, but the resume callback snd_acp_resume() never gets called.
>>
>> I'm no expert in runtime power management (though I did read a bit on it), so I don't understand
>> all the things that happen underneath, but one thing that is not clear to me is who's supposed
>> to mark activity on this device and keep it from entering autosuspend if the user wants to play
>> some sound? Shouldn't there be some counterpart that calls pm_runtime_mark_last_busy() ?
>> I looked through the code and can't find who's calling pm_runtime_mark_last_busy().
>>
>> Some help here would be welcome. Is there something missing in my machine driver code, or
>> is the runtime pm handling in acp pci driver wrong?
> Thanks for the regression report. I'm adding it to regzbot:
>
> #regzbot ^introduced: 088a40980efbc2
>
We were working on some other priority tasks, will upstream the changes

by next week.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Bagas Sanjaya]

On 03/11/2023 20:30, syed saba kareem wrote:
> 
> On 11/3/23 04:41, Bagas Sanjaya wrote:
>> On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
>>> I'm trying to develop a sound machine driver based on the acp legacy driver.
>>> The first version of the driver was sent for review on the alsa mailing list this
>>> spring: https://lore.kernel.org/all/20230320203519.20137-1-posteuca@mutex.one
>>>
>>> I'm trying to fix some of the issues that were brought up during the review back then,
>>> but when I ported the patches to the latest commit on the for-next
>>> branch, I noticed a regression where I couldn't hear any sound at all.
>>>
>>> So I started a bisect session and found that the first bad commit is:
>>> ASoC: amd: acp: add pm ops support for acp pci driver
>>> commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2
>>> https://lore.kernel.org/lkml/20230622152406.3709231-11-Syed.SabaKareem@amd.com
>>>
>>> If I revert this commit sound works as expected. So I started tinkering a little bit
>>> with it and I believe that what happens is that the acp pci driver
>>> enters the autosuspend state and never leaves this state at all.
>>> I noticed this because if I increase the autosuspend delay to a much
>>> larger value, then the sound works until that delay passes.
>>> I added traces and I can see that when the delay expires the suspend callback snd_acp_suspend()
>>> gets called, but the resume callback snd_acp_resume() never gets called.
>>>
>>> I'm no expert in runtime power management (though I did read a bit on it), so I don't understand
>>> all the things that happen underneath, but one thing that is not clear to me is who's supposed
>>> to mark activity on this device and keep it from entering autosuspend if the user wants to play
>>> some sound? Shouldn't there be some counterpart that calls pm_runtime_mark_last_busy() ?
>>> I looked through the code and can't find who's calling pm_runtime_mark_last_busy().
>>>
>>> Some help here would be welcome. Is there something missing in my machine driver code, or
>>> is the runtime pm handling in acp pci driver wrong?
>> Thanks for the regression report. I'm adding it to regzbot:
>>
>> #regzbot ^introduced: 088a40980efbc2
>>
> We were working on some other priority tasks, will upstream the changes
> 
> by next week.
> 

OK, thanks!

-- 
An old man doll... just what I always wanted! - Clara

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Linux regression tracking (Thorsten Leemhuis)]

On 03.11.23 14:30, syed saba kareem wrote:
> On 11/3/23 04:41, Bagas Sanjaya wrote:
>> On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
> [...]
>>> Some help here would be welcome. Is there something missing in my
>>> machine driver code, or
>>> is the runtime pm handling in acp pci driver wrong?
>>
> We were working on some other priority tasks, will upstream the changes
> 
> by next week.

Hi syed saba kareem! Did that happen? From here it looks like it did
not, so I assume the regression was not yet addressed. But it's easy to
miss something, hence this mail.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[syed saba kareem]

On 11/22/23 14:25, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 03.11.23 14:30, syed saba kareem wrote:
>> On 11/3/23 04:41, Bagas Sanjaya wrote:
>>> On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
>> [...]
>>>> Some help here would be welcome. Is there something missing in my
>>>> machine driver code, or
>>>> is the runtime pm handling in acp pci driver wrong?
>> We were working on some other priority tasks, will upstream the changes
>>
>> by next week.
> Hi syed saba kareem! Did that happen? From here it looks like it did
> not, so I assume the regression was not yet addressed. But it's easy to
> miss something, hence this mail.

Hi , We have up streamed the patch it is in review.

Please find the below link for the patch details.

https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/

> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
>
> #regzbot poke

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Linux regression tracking (Thorsten Leemhuis)]

On 22.11.23 10:32, syed saba kareem wrote:
> On 11/22/23 14:25, Linux regression tracking (Thorsten Leemhuis) wrote:
>> On 03.11.23 14:30, syed saba kareem wrote:
>>> On 11/3/23 04:41, Bagas Sanjaya wrote:
>>>> On Thu, Aug 03, 2023 at 10:22:07PM +0300, Marian Postevca wrote:
>>> [...]
>>>>> Some help here would be welcome. Is there something missing in my
>>>>> machine driver code, or
>>>>> is the runtime pm handling in acp pci driver wrong?
>>> We were working on some other priority tasks, will upstream the changes
>>>
>>> by next week.
>> Hi syed saba kareem! Did that happen? From here it looks like it did
>> not, so I assume the regression was not yet addressed. But it's easy to
>> miss something, hence this mail.
> 
> Hi , We have up streamed the patch it is in review.
> Please find the below link for the patch details.
> https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/

Ahh, great. Has to Fixes: tag and no Link:/Closes: tag to this thread,
otherwise I would have noticed that change myself, but whatever.

Thx!

#regzbot monitor:
https://lore.kernel.org/all/20231113123345.2196504-2-Syed.SabaKareem@amd.com/
#regzbot fix: ASoC: amd: acp: add pm ops support for renoir platform
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Marian Postevca]

syed saba kareem <ssabakar@amd.com> writes:

>
> Hi , We have up streamed the patch it is in review.
>
> Please find the below link for the patch details.
>
> https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/
>

Sorry maybe I'm not understanding here something, when you are saying
that the patch is in review, where is this review being done?

As far as I can tell Mark Brown only pulled the first patch from your
series:

>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
>
> Thanks!
>
> [1/2] ASoC: amd: acp: add Kconfig options for acp7.0 based platform driver
>       commit: d3534684ada99ef8c0899eb28c62b4462483ee19
> [2/2] ASoC: amd: acp: add pm ops support for renoir platform
>       (no commit info)
>

I don't see the second patch in for-next.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[syed saba kareem]

On 11/23/23 03:25, Marian Postevca wrote:
> syed saba kareem <ssabakar@amd.com> writes:
>
>> Hi , We have up streamed the patch it is in review.
>>
>> Please find the below link for the patch details.
>>
>> https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/
>>
> Sorry maybe I'm not understanding here something, when you are saying
> that the patch is in review, where is this review being done?
>
> As far as I can tell Mark Brown only pulled the first patch from your
> series:

We got comments for the second patch and we addressed them.

It seems it may be stuck at review stage as one patch got merged.

Will resend the patch again.

You can find the received review comments for the patch in the below link.

https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/

>> Applied to
>>
>>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
>>
>> Thanks!
>>
>> [1/2] ASoC: amd: acp: add Kconfig options for acp7.0 based platform driver
>>        commit: d3534684ada99ef8c0899eb28c62b4462483ee19
>> [2/2] ASoC: amd: acp: add pm ops support for renoir platform
>>        (no commit info)
>>
> I don't see the second patch in for-next.

Re: Regression apparently caused by commit 088a40980efbc2c449b72f0f2c7ebd82f71d08e2 "ASoC: amd: acp: add pm ops support for acp pci driver"

[Linux regression tracking (Thorsten Leemhuis)]

On 23.11.23 14:58, syed saba kareem wrote:
> 
> On 11/23/23 03:25, Marian Postevca wrote:
>> syed saba kareem <ssabakar@amd.com> writes:
>>
>>> Hi , We have up streamed the patch it is in review.
>>>
>>> Please find the below link for the patch details.
>>>
>>> https://patchwork.kernel.org/project/alsa-devel/patch/20231113123345.2196504-2-Syed.SabaKareem@amd.com/
>>>
>> Sorry maybe I'm not understanding here something, when you are saying
>> that the patch is in review, where is this review being done?
>>
>> As far as I can tell Mark Brown only pulled the first patch from your
>> series:
> 
> We got comments for the second patch and we addressed them.
> 
> It seems it may be stuck at review stage as one patch got merged.
> 
> Will resend the patch again.
> 
> You can find the received review comments for the patch in the below link.

Any news? From a quick look it seems this didn't make any progress at
all, but I might be missing something.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

>>> Applied to
>>>
>>>     https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
>>> for-next
>>>
>>> Thanks!
>>>
>>> [1/2] ASoC: amd: acp: add Kconfig options for acp7.0 based platform
>>> driver
>>>        commit: d3534684ada99ef8c0899eb28c62b4462483ee19
>>> [2/2] ASoC: amd: acp: add pm ops support for renoir platform
>>>        (no commit info)
>>>
>> I don't see the second patch in for-next.
> 
>