| Message ID | 20230613093453.13927-1-jonathanh@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | ASoC: tegra: Fix Master Volume Control | expand |
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 >
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
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
[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.
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
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);
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(+)