[PATCH] ASoC: tegra: Fix Master Volume Control

[PATCH] ASoC: tegra: Fix Master Volume Control

[Jon Hunter]

Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
the PCM wait_time calculations and in doing so reduced the calculated
wait_time. This exposed an issue with the Tegra Master Volume Control
(MVC) device where the reduced wait_time caused the MVC to fail. For now
fix this by setting the default wait_time for Tegra to be 500ms.

Fixes: 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations")
Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 sound/soc/tegra/tegra_pcm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/tegra/tegra_pcm.c b/sound/soc/tegra/tegra_pcm.c
index 468c8e77de21..0b69cebc9a33 100644
--- a/sound/soc/tegra/tegra_pcm.c
+++ b/sound/soc/tegra/tegra_pcm.c
@@ -117,6 +117,9 @@ int tegra_pcm_open(struct snd_soc_component *component,
 		return ret;
 	}
 
+	/* Set wait time to 500ms by default */
+	substream->wait_time = 500;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tegra_pcm_open);
-- 
2.34.1

Re: [PATCH] ASoC: tegra: Fix Master Volume Control

[Takashi Iwai]

On Tue, 13 Jun 2023 11:34:53 +0200,
Jon Hunter wrote:
> 
> Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
> the PCM wait_time calculations and in doing so reduced the calculated
> wait_time. This exposed an issue with the Tegra Master Volume Control
> (MVC) device where the reduced wait_time caused the MVC to fail. For now
> fix this by setting the default wait_time for Tegra to be 500ms.
> 
> Fixes: 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Hm, it's still not clear why it fails.  The commit above changes the
timeout of wait_for_avail() to the full-buffer + 10% margin.  In
thoery, the loop should abort after the full buffer read, and that
must be enough.  If there were a large FIFO behind, it might be
overflow, but the fifo_size of Tegra driver seems 4, so it's
negligible.

If extending the timeout "fixes" the problem, it might indicate that
the period update IRQ is triggered too late.  Could you measure the
timing of each snd_pcm_period_elapsed() and wait_for_avail() call?


thanks,

Takashi


> ---
>  sound/soc/tegra/tegra_pcm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/tegra/tegra_pcm.c b/sound/soc/tegra/tegra_pcm.c
> index 468c8e77de21..0b69cebc9a33 100644
> --- a/sound/soc/tegra/tegra_pcm.c
> +++ b/sound/soc/tegra/tegra_pcm.c
> @@ -117,6 +117,9 @@ int tegra_pcm_open(struct snd_soc_component *component,
>  		return ret;
>  	}
>  
> +	/* Set wait time to 500ms by default */
> +	substream->wait_time = 500;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tegra_pcm_open);
> -- 
> 2.34.1
> 

Re: [PATCH] ASoC: tegra: Fix Master Volume Control

[Takashi Iwai]

On Tue, 13 Jun 2023 11:55:16 +0200,
Takashi Iwai wrote:
> 
> On Tue, 13 Jun 2023 11:34:53 +0200,
> Jon Hunter wrote:
> > 
> > Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
> > the PCM wait_time calculations and in doing so reduced the calculated
> > wait_time. This exposed an issue with the Tegra Master Volume Control
> > (MVC) device where the reduced wait_time caused the MVC to fail. For now
> > fix this by setting the default wait_time for Tegra to be 500ms.
> > 
> > Fixes: 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations")
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Hm, it's still not clear why it fails.  The commit above changes the
> timeout of wait_for_avail() to the full-buffer + 10% margin.  In
> thoery, the loop should abort after the full buffer read, and that
> must be enough.  If there were a large FIFO behind, it might be
> overflow, but the fifo_size of Tegra driver seems 4, so it's
> negligible.
> 
> If extending the timeout "fixes" the problem, it might indicate that
> the period update IRQ is triggered too late.  Could you measure the
> timing of each snd_pcm_period_elapsed() and wait_for_avail() call?

OTOH, it's already at a pretty late stage for 6.4, and we need an
urgent regression fix.  So it's better to paper over it now, while
hunting further for the real culprit.


Takashi

Re: [PATCH] ASoC: tegra: Fix Master Volume Control

[Jon Hunter]

On 13/06/2023 10:57, Takashi Iwai wrote:
> On Tue, 13 Jun 2023 11:55:16 +0200,
> Takashi Iwai wrote:
>>
>> On Tue, 13 Jun 2023 11:34:53 +0200,
>> Jon Hunter wrote:
>>>
>>> Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
>>> the PCM wait_time calculations and in doing so reduced the calculated
>>> wait_time. This exposed an issue with the Tegra Master Volume Control
>>> (MVC) device where the reduced wait_time caused the MVC to fail. For now
>>> fix this by setting the default wait_time for Tegra to be 500ms.
>>>
>>> Fixes: 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations")
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>
>> Hm, it's still not clear why it fails.  The commit above changes the
>> timeout of wait_for_avail() to the full-buffer + 10% margin.  In
>> thoery, the loop should abort after the full buffer read, and that
>> must be enough.  If there were a large FIFO behind, it might be
>> overflow, but the fifo_size of Tegra driver seems 4, so it's
>> negligible.
>>
>> If extending the timeout "fixes" the problem, it might indicate that
>> the period update IRQ is triggered too late.  Could you measure the
>> timing of each snd_pcm_period_elapsed() and wait_for_avail() call?
> 
> OTOH, it's already at a pretty late stage for 6.4, and we need an
> urgent regression fix.  So it's better to paper over it now, while
> hunting further for the real culprit.


I have filed a bug report internally to investigate this, but yes for 
now was hoping to get something in place for v6.4 to avoid any regressions.

Thanks
Jon

-- 
nvpublic

Re: [PATCH] ASoC: tegra: Fix Master Volume Control

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 13.06.23 11:34, Jon Hunter wrote:
> Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
> the PCM wait_time calculations and in doing so reduced the calculated
> wait_time. This exposed an issue with the Tegra Master Volume Control
> (MVC) device where the reduced wait_time caused the MVC to fail. For now
> fix this by setting the default wait_time for Tegra to be 500ms.
> 
> Fixes: 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations")
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> [...]

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 3ed2b549b39f
#regzbot title ASoC: tegra: Master Volume Control broken
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

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: [PATCH] ASoC: tegra: Fix Master Volume Control

[Mark Brown]

On Tue, 13 Jun 2023 10:34:53 +0100, Jon Hunter wrote:
> Commit 3ed2b549b39f ("ALSA: pcm: fix wait_time calculations") corrected
> the PCM wait_time calculations and in doing so reduced the calculated
> wait_time. This exposed an issue with the Tegra Master Volume Control
> (MVC) device where the reduced wait_time caused the MVC to fail. For now
> fix this by setting the default wait_time for Tegra to be 500ms.
> 
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: tegra: Fix Master Volume Control
      commit: f9fd804aa0a36f15a35ca070ec4c52650876cc29

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark