Re: [GIT PULL] sound updates for 5.9
On Mon, Aug 10, 2020 at 10:06 AM Srinivas Kandagatla wrote: > > Hi John, > Thanks for reporting this. > > On 08/08/2020 01:23, John Stultz wrote: > > q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: > > error at soc_component_read_no_lock on > > remoteproc-adsp:glink-edge:apr:: -5 > > This is an -EIO error which is reported when the ASoC component driver > does not have both regmap and read callback and someone is trying to > read a register! > > In q6routing case all the dapm widgets reuse reg field in > snd_soc_dapm_widget to store offset information or routing table indexs > or some DSP related id and so on... These are not real registers. > > I think the core is trying to read the state of these widgets during > startup, Which will fail in qdsp6 case as we do not have any regmap or > read callback associated with this ASoC component. > > Previously we never had chance to see these messages so we did not > implement any dummy read callback. > > Adding a dummy callback to q6routing and q6afe-dai ASoC component will > fix this issue at-least in Qualcomm case! Yea, just to confirm. The patch you sent me privately that does the above seems to work! Thanks so much! -john
Re: [GIT PULL] sound updates for 5.9
On Mon, Aug 10, 2020 at 06:06:14PM +0100, Srinivas Kandagatla wrote: > In q6routing case all the dapm widgets reuse reg field in > snd_soc_dapm_widget to store offset information or routing table indexs or > some DSP related id and so on... These are not real registers. > I think the core is trying to read the state of these widgets during > startup, Which will fail in qdsp6 case as we do not have any regmap or read > callback associated with this ASoC component. Yes, it will try to figure out the current state of things during startup. > Previously we never had chance to see these messages so we did not implement > any dummy read callback. So I guess this is another instance of the issues with other things, just having an effect beyond the cosmetic this time :/ > Adding a dummy callback to q6routing and q6afe-dai ASoC component will fix > this issue at-least in Qualcomm case! Yes, that's going to be better for robustness regardless of changes in the core - just pick a default state if the underlying thing is undefined. signature.asc Description: PGP signature
Re: [GIT PULL] sound updates for 5.9
Hi John, Thanks for reporting this. On 08/08/2020 01:23, John Stultz wrote: q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: error at soc_component_read_no_lock on remoteproc-adsp:glink-edge:apr:: -5 This is an -EIO error which is reported when the ASoC component driver does not have both regmap and read callback and someone is trying to read a register! In q6routing case all the dapm widgets reuse reg field in snd_soc_dapm_widget to store offset information or routing table indexs or some DSP related id and so on... These are not real registers. I think the core is trying to read the state of these widgets during startup, Which will fail in qdsp6 case as we do not have any regmap or read callback associated with this ASoC component. Previously we never had chance to see these messages so we did not implement any dummy read callback. Adding a dummy callback to q6routing and q6afe-dai ASoC component will fix this issue at-least in Qualcomm case! thanks, srini
Re: [GIT PULL] sound updates for 5.9
On Mon, 10 Aug 2020 14:22:54 +0200, Mark Brown wrote: > > On Sat, Aug 08, 2020 at 10:07:36AM +0200, Takashi Iwai wrote: > > Takashi Iwai wrote: > > > > Does the patch below fix the bug? If so, it's rather a bug in the > > > commit cf6e26c71bfd ("ASoC: soc-component: merge > > > snd_soc_component_read() and snd_soc_component_read32()"). > > > That said, the commit cf6e26c71bfd dropped the capability of returning > > an error code from snd_soc_component_read() completely, while many > > code still expect an error gets returned. The assumption mentioned in > > the patch (the error can be ignored) looks too naive. > > I did an audit of the users when the series was posted and wasn't able > to turn up any code doing anything constructive with the return values, > but then once you're past probe error handling often makes things worse > if you try. This is the first one which actually seems to have had an > impact. > > > Morimoto-san, Mark, could you address it? IMO, we may still need two > > variants in the end again: the former snd_soc_component_read32() that > > returns the value directly and snd_soc_component_read() that returns 0 > > or an error. Only once after we deal with the error handling in each > > caller side, we can unify the read functions. > > I'm not sure if that specifically is what we need but yeah we should do > something, if it fixes things your change is certainly good for the > immediate problem so could you send it with a signoff please? OK, will do soon later. > With the new code we do now have the core code printing an error message > if the I/O fails, before they were just being ignored more often than > not. This did turn up a couple of cases where drivers were relying on > being able to do things like silently read from registers that just > don't exist or aren't currently accessible without any diagnostics which > is it's own problem :/ (especially the volatile cases). Yeah, we may need some raw access helper for such a case... thanks, Takashi
Re: [GIT PULL] sound updates for 5.9
On Sat, Aug 08, 2020 at 10:07:36AM +0200, Takashi Iwai wrote: > Takashi Iwai wrote: > > Does the patch below fix the bug? If so, it's rather a bug in the > > commit cf6e26c71bfd ("ASoC: soc-component: merge > > snd_soc_component_read() and snd_soc_component_read32()"). > That said, the commit cf6e26c71bfd dropped the capability of returning > an error code from snd_soc_component_read() completely, while many > code still expect an error gets returned. The assumption mentioned in > the patch (the error can be ignored) looks too naive. I did an audit of the users when the series was posted and wasn't able to turn up any code doing anything constructive with the return values, but then once you're past probe error handling often makes things worse if you try. This is the first one which actually seems to have had an impact. > Morimoto-san, Mark, could you address it? IMO, we may still need two > variants in the end again: the former snd_soc_component_read32() that > returns the value directly and snd_soc_component_read() that returns 0 > or an error. Only once after we deal with the error handling in each > caller side, we can unify the read functions. I'm not sure if that specifically is what we need but yeah we should do something, if it fixes things your change is certainly good for the immediate problem so could you send it with a signoff please? With the new code we do now have the core code printing an error message if the I/O fails, before they were just being ignored more often than not. This did turn up a couple of cases where drivers were relying on being able to do things like silently read from registers that just don't exist or aren't currently accessible without any diagnostics which is it's own problem :/ (especially the volatile cases). signature.asc Description: PGP signature
Re: [GIT PULL] sound updates for 5.9
On Fri, Aug 7, 2020 at 11:46 PM Takashi Iwai wrote: > > On Sat, 08 Aug 2020 02:23:24 +0200, > John Stultz wrote: > > > > On Thu, Aug 6, 2020 at 3:33 AM Takashi Iwai wrote: > > > > > > Linus, > > > > > > please pull sound updates for v5.9 from: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > > > tags/sound-5.9-rc1 > > > > > > The topmost commit is c7fabbc51352f50cc58242a6dc3b9c1a3599849b > > > > > > > > > > > > sound updates for 5.9 > > > > > > This became wide and scattered updates all over the sound tree as > > > diffstat shows: lots of (still ongoing) refactoring works in ASoC, > > > fixes and cleanups caught by static analysis, inclusive term > > > conversions as well as lots of new drivers. Below are highlights: > > > > > > ASoC core: > > > * API cleanups and conversions to the unified mute_stream() call > > > * Simplify I/O helper functions > > > * Use helper macros to retrieve RTD from substreams > > ... > > > Kuninori Morimoto (90): > > > ASoC: soc-component: add soc_component_pin() and share code > > > ASoC: soc-component: move snd_soc_component_xxx_regmap() to > > > soc-component > > > ASoC: soc-component: move snd_soc_component_initialize() to > > > soc-component.c > > > ASoC: soc-component: add soc_component_err() > > > ASoC: soc-component: add snd_soc_pcm_component_prepare() > > > ASoC: soc-component: add snd_soc_pcm_component_hw_params() > > > ASoC: soc-component: add snd_soc_pcm_component_hw_free() > > > ASoC: soc-component: add snd_soc_pcm_component_trigger() > > > ASoC: soc-component: add snd_soc_component_init() > > > ASoC: soc-component: merge soc-io.c into soc-component.c > > > > So oddly, today I bisected down the change "ASoC: soc-component: merge > > soc-io.c into soc-component.c": > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=460b42d162e3cf634586999e6a84e74ca52e626d > > > > as causing audio regressions on Dragonboard 845c running AOSP. > > > > On boot I was seeing tons of: > > q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: > > error at soc_component_read_no_lock on > > remoteproc-adsp:glink-edge:apr:: -5 > > > > And when audio was supposed to play I'd see: > > [ 227.462986] qcom-q6afe aprsvc:apr-service:4:4: cmd = 0x100e5 > > returned error = 0x9 > > [ 227.470720] qcom-q6afe aprsvc:apr-service:4:4: DSP returned error[9] > > [ 227.477168] qcom-q6afe aprsvc:apr-service:4:4: AFE enable for port > > 0x4000 failed -22 > > [ 227.485038] q6afe-dai > > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: fail to start AFE > > port 2 > > [ 227.494013] q6afe-dai > > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: ASoC: error at > > snd_soc_pcm_dai_prepare on SLIMBUS_0_RX: -22 > > [ 227.506034] SLIM Playback: ASoC: DAI prepare error: -22 > > [ 227.511415] SLIM Playback: ASoC: backend prepare failed -22 > > > > Its strange, as the bisected patch is really just moving code around > > and there's very little in the way of logic changes. After minimizing > > the code movement and just focusing on what changed I forward ported a > > revert to mainline and minimized it until things were working. > > > > The resulting patch is a twoliner here: > > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/db845c-mainline-WIP&id=a3527193f39b1224d59bf1519fce3ef8c57d0f5e > > > > I'm a bit baffled as to why this patch works. Logically we are > > returning the same value. I suspect when we hit the error, all the > > extra error print messages on the console slow things down and end up > > causing some timing related initialization failure? > > Does the patch below fix the bug? If so, it's rather a bug in the > commit cf6e26c71bfd ("ASoC: soc-component: merge > snd_soc_component_read() and snd_soc_component_read32()"). > > > thanks, > > Takashi > > --- a/sound/soc/soc-component.c > +++ b/sound/soc/soc-component.c > @@ -406,7 +406,7 @@ static unsigned int soc_component_read_no_lock( > ret = -EIO; > > if (ret < 0) > - soc_component_ret(component, ret); > + return soc_component_ret(component, ret); Oh, that's so obvious now! I can't believe I was staring at that code and just didn't see it! Thanks so much for pointing this out! I'm sure this will fix it, but will validate on monday when I am working with the board. thanks! -john
Re: [GIT PULL] sound updates for 5.9
On Sat, 08 Aug 2020 08:46:18 +0200, Takashi Iwai wrote: > > On Sat, 08 Aug 2020 02:23:24 +0200, > John Stultz wrote: > > > > On Thu, Aug 6, 2020 at 3:33 AM Takashi Iwai wrote: > > > > > > Linus, > > > > > > please pull sound updates for v5.9 from: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > > > tags/sound-5.9-rc1 > > > > > > The topmost commit is c7fabbc51352f50cc58242a6dc3b9c1a3599849b > > > > > > > > > > > > sound updates for 5.9 > > > > > > This became wide and scattered updates all over the sound tree as > > > diffstat shows: lots of (still ongoing) refactoring works in ASoC, > > > fixes and cleanups caught by static analysis, inclusive term > > > conversions as well as lots of new drivers. Below are highlights: > > > > > > ASoC core: > > > * API cleanups and conversions to the unified mute_stream() call > > > * Simplify I/O helper functions > > > * Use helper macros to retrieve RTD from substreams > > ... > > > Kuninori Morimoto (90): > > > ASoC: soc-component: add soc_component_pin() and share code > > > ASoC: soc-component: move snd_soc_component_xxx_regmap() to > > > soc-component > > > ASoC: soc-component: move snd_soc_component_initialize() to > > > soc-component.c > > > ASoC: soc-component: add soc_component_err() > > > ASoC: soc-component: add snd_soc_pcm_component_prepare() > > > ASoC: soc-component: add snd_soc_pcm_component_hw_params() > > > ASoC: soc-component: add snd_soc_pcm_component_hw_free() > > > ASoC: soc-component: add snd_soc_pcm_component_trigger() > > > ASoC: soc-component: add snd_soc_component_init() > > > ASoC: soc-component: merge soc-io.c into soc-component.c > > > > So oddly, today I bisected down the change "ASoC: soc-component: merge > > soc-io.c into soc-component.c": > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=460b42d162e3cf634586999e6a84e74ca52e626d > > > > as causing audio regressions on Dragonboard 845c running AOSP. > > > > On boot I was seeing tons of: > > q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: > > error at soc_component_read_no_lock on > > remoteproc-adsp:glink-edge:apr:: -5 > > > > And when audio was supposed to play I'd see: > > [ 227.462986] qcom-q6afe aprsvc:apr-service:4:4: cmd = 0x100e5 > > returned error = 0x9 > > [ 227.470720] qcom-q6afe aprsvc:apr-service:4:4: DSP returned error[9] > > [ 227.477168] qcom-q6afe aprsvc:apr-service:4:4: AFE enable for port > > 0x4000 failed -22 > > [ 227.485038] q6afe-dai > > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: fail to start AFE > > port 2 > > [ 227.494013] q6afe-dai > > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: ASoC: error at > > snd_soc_pcm_dai_prepare on SLIMBUS_0_RX: -22 > > [ 227.506034] SLIM Playback: ASoC: DAI prepare error: -22 > > [ 227.511415] SLIM Playback: ASoC: backend prepare failed -22 > > > > Its strange, as the bisected patch is really just moving code around > > and there's very little in the way of logic changes. After minimizing > > the code movement and just focusing on what changed I forward ported a > > revert to mainline and minimized it until things were working. > > > > The resulting patch is a twoliner here: > > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/db845c-mainline-WIP&id=a3527193f39b1224d59bf1519fce3ef8c57d0f5e > > > > I'm a bit baffled as to why this patch works. Logically we are > > returning the same value. I suspect when we hit the error, all the > > extra error print messages on the console slow things down and end up > > causing some timing related initialization failure? > > Does the patch below fix the bug? If so, it's rather a bug in the > commit cf6e26c71bfd ("ASoC: soc-component: merge > snd_soc_component_read() and snd_soc_component_read32()"). That said, the commit cf6e26c71bfd dropped the capability of returning an error code from snd_soc_component_read() completely, while many code still expect an error gets returned. The assumption mentioned in the patch (the error can be ignored) looks too naive. Morimoto-san, Mark, could you address it? IMO, we may still need two variants in the end again: the former snd_soc_component_read32() that returns the value directly and snd_soc_component_read() that returns 0 or an error. Only once after we deal with the error handling in each caller side, we can unify the read functions. Takashi > > thanks, > > Takashi > > --- a/sound/soc/soc-component.c > +++ b/sound/soc/soc-component.c > @@ -406,7 +406,7 @@ static unsigned int soc_component_read_no_lock( > ret = -EIO; > > if (ret < 0) > - soc_component_ret(component, ret); > + return soc_component_ret(component, ret); > > return val; > }
Re: [GIT PULL] sound updates for 5.9
On Sat, 08 Aug 2020 02:23:24 +0200, John Stultz wrote: > > On Thu, Aug 6, 2020 at 3:33 AM Takashi Iwai wrote: > > > > Linus, > > > > please pull sound updates for v5.9 from: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > > tags/sound-5.9-rc1 > > > > The topmost commit is c7fabbc51352f50cc58242a6dc3b9c1a3599849b > > > > > > > > sound updates for 5.9 > > > > This became wide and scattered updates all over the sound tree as > > diffstat shows: lots of (still ongoing) refactoring works in ASoC, > > fixes and cleanups caught by static analysis, inclusive term > > conversions as well as lots of new drivers. Below are highlights: > > > > ASoC core: > > * API cleanups and conversions to the unified mute_stream() call > > * Simplify I/O helper functions > > * Use helper macros to retrieve RTD from substreams > ... > > Kuninori Morimoto (90): > > ASoC: soc-component: add soc_component_pin() and share code > > ASoC: soc-component: move snd_soc_component_xxx_regmap() to > > soc-component > > ASoC: soc-component: move snd_soc_component_initialize() to > > soc-component.c > > ASoC: soc-component: add soc_component_err() > > ASoC: soc-component: add snd_soc_pcm_component_prepare() > > ASoC: soc-component: add snd_soc_pcm_component_hw_params() > > ASoC: soc-component: add snd_soc_pcm_component_hw_free() > > ASoC: soc-component: add snd_soc_pcm_component_trigger() > > ASoC: soc-component: add snd_soc_component_init() > > ASoC: soc-component: merge soc-io.c into soc-component.c > > So oddly, today I bisected down the change "ASoC: soc-component: merge > soc-io.c into soc-component.c": > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=460b42d162e3cf634586999e6a84e74ca52e626d > > as causing audio regressions on Dragonboard 845c running AOSP. > > On boot I was seeing tons of: > q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: > error at soc_component_read_no_lock on > remoteproc-adsp:glink-edge:apr:: -5 > > And when audio was supposed to play I'd see: > [ 227.462986] qcom-q6afe aprsvc:apr-service:4:4: cmd = 0x100e5 > returned error = 0x9 > [ 227.470720] qcom-q6afe aprsvc:apr-service:4:4: DSP returned error[9] > [ 227.477168] qcom-q6afe aprsvc:apr-service:4:4: AFE enable for port > 0x4000 failed -22 > [ 227.485038] q6afe-dai > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: fail to start AFE > port 2 > [ 227.494013] q6afe-dai > remoteproc-adsp:glink-edge:apr:apr-service@4:dais: ASoC: error at > snd_soc_pcm_dai_prepare on SLIMBUS_0_RX: -22 > [ 227.506034] SLIM Playback: ASoC: DAI prepare error: -22 > [ 227.511415] SLIM Playback: ASoC: backend prepare failed -22 > > Its strange, as the bisected patch is really just moving code around > and there's very little in the way of logic changes. After minimizing > the code movement and just focusing on what changed I forward ported a > revert to mainline and minimized it until things were working. > > The resulting patch is a twoliner here: > https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/db845c-mainline-WIP&id=a3527193f39b1224d59bf1519fce3ef8c57d0f5e > > I'm a bit baffled as to why this patch works. Logically we are > returning the same value. I suspect when we hit the error, all the > extra error print messages on the console slow things down and end up > causing some timing related initialization failure? Does the patch below fix the bug? If so, it's rather a bug in the commit cf6e26c71bfd ("ASoC: soc-component: merge snd_soc_component_read() and snd_soc_component_read32()"). thanks, Takashi --- a/sound/soc/soc-component.c +++ b/sound/soc/soc-component.c @@ -406,7 +406,7 @@ static unsigned int soc_component_read_no_lock( ret = -EIO; if (ret < 0) - soc_component_ret(component, ret); + return soc_component_ret(component, ret); return val; }
Re: [GIT PULL] sound updates for 5.9
On Thu, Aug 6, 2020 at 3:33 AM Takashi Iwai wrote: > > Linus, > > please pull sound updates for v5.9 from: > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > tags/sound-5.9-rc1 > > The topmost commit is c7fabbc51352f50cc58242a6dc3b9c1a3599849b > > > > sound updates for 5.9 > > This became wide and scattered updates all over the sound tree as > diffstat shows: lots of (still ongoing) refactoring works in ASoC, > fixes and cleanups caught by static analysis, inclusive term > conversions as well as lots of new drivers. Below are highlights: > > ASoC core: > * API cleanups and conversions to the unified mute_stream() call > * Simplify I/O helper functions > * Use helper macros to retrieve RTD from substreams ... > Kuninori Morimoto (90): > ASoC: soc-component: add soc_component_pin() and share code > ASoC: soc-component: move snd_soc_component_xxx_regmap() to > soc-component > ASoC: soc-component: move snd_soc_component_initialize() to > soc-component.c > ASoC: soc-component: add soc_component_err() > ASoC: soc-component: add snd_soc_pcm_component_prepare() > ASoC: soc-component: add snd_soc_pcm_component_hw_params() > ASoC: soc-component: add snd_soc_pcm_component_hw_free() > ASoC: soc-component: add snd_soc_pcm_component_trigger() > ASoC: soc-component: add snd_soc_component_init() > ASoC: soc-component: merge soc-io.c into soc-component.c So oddly, today I bisected down the change "ASoC: soc-component: merge soc-io.c into soc-component.c": https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=460b42d162e3cf634586999e6a84e74ca52e626d as causing audio regressions on Dragonboard 845c running AOSP. On boot I was seeing tons of: q6routing remoteproc-adsp:glink-edge:apr:apr-service@8:routing: ASoC: error at soc_component_read_no_lock on remoteproc-adsp:glink-edge:apr:: -5 And when audio was supposed to play I'd see: [ 227.462986] qcom-q6afe aprsvc:apr-service:4:4: cmd = 0x100e5 returned error = 0x9 [ 227.470720] qcom-q6afe aprsvc:apr-service:4:4: DSP returned error[9] [ 227.477168] qcom-q6afe aprsvc:apr-service:4:4: AFE enable for port 0x4000 failed -22 [ 227.485038] q6afe-dai remoteproc-adsp:glink-edge:apr:apr-service@4:dais: fail to start AFE port 2 [ 227.494013] q6afe-dai remoteproc-adsp:glink-edge:apr:apr-service@4:dais: ASoC: error at snd_soc_pcm_dai_prepare on SLIMBUS_0_RX: -22 [ 227.506034] SLIM Playback: ASoC: DAI prepare error: -22 [ 227.511415] SLIM Playback: ASoC: backend prepare failed -22 Its strange, as the bisected patch is really just moving code around and there's very little in the way of logic changes. After minimizing the code movement and just focusing on what changed I forward ported a revert to mainline and minimized it until things were working. The resulting patch is a twoliner here: https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/db845c-mainline-WIP&id=a3527193f39b1224d59bf1519fce3ef8c57d0f5e I'm a bit baffled as to why this patch works. Logically we are returning the same value. I suspect when we hit the error, all the extra error print messages on the console slow things down and end up causing some timing related initialization failure? Anyway, I wanted to raise the issue in case anyone had ideas what might be going wrong. thanks -john
Re: [GIT PULL] sound updates for 5.9
The pull request you sent on Thu, 06 Aug 2020 12:21:10 +0200: > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > tags/sound-5.9-rc1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/3f9df56480fc8ce492fc9e988d67bdea884ed15c Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html