| Message ID | 20220624134317.3656128-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Intel: Mark BE DAIs as nonatomic for hsw and | expand |
On 6/24/22 08:43, Cezary Rojewski wrote: > Address the warning: "Codec: dpcm_be_connect: FE is nonatomic but BE is > not, forcing BE as nonatomic" by marking BE DAI as nonatomic. Aligns > with what is already done for FE DAIs. > > This patchset iterates the change over all HSW and BDW related machine > board drivers. I don't think this is necessary, I was planning to demote this warning to a simple dev_dbg or possibly remove this message entirely. The BE DAIs can perfectly be declared as non-atomic in all Intel machine drivers, except for SoundWire where there's a known delay during the .trigger. > Cezary Rojewski (4): > ASoC: Intel: hsw_rt5640: Mark BE DAI as nonatomic > ASoC: Intel: bdw_rt286: Mark BE DAI as nonatomic > ASoC: Intel: bdw_rt5650: Mark BE DAI as nonatomic > ASoC: Intel: bdw_rt5677: Mark BE DAI as nonatomic > > sound/soc/intel/boards/bdw-rt5650.c | 1 + > sound/soc/intel/boards/bdw-rt5677.c | 1 + > sound/soc/intel/boards/bdw_rt286.c | 1 + > sound/soc/intel/boards/hsw_rt5640.c | 1 + > 4 files changed, 4 insertions(+) >
On 2022-06-24 3:52 PM, Pierre-Louis Bossart wrote: > On 6/24/22 08:43, Cezary Rojewski wrote: >> Address the warning: "Codec: dpcm_be_connect: FE is nonatomic but BE is >> not, forcing BE as nonatomic" by marking BE DAI as nonatomic. Aligns >> with what is already done for FE DAIs. >> >> This patchset iterates the change over all HSW and BDW related machine >> board drivers. > > I don't think this is necessary, I was planning to demote this warning > to a simple dev_dbg or possibly remove this message entirely. > > The BE DAIs can perfectly be declared as non-atomic in all Intel machine > drivers, except for SoundWire where there's a known delay during the > .trigger. Hmm.. that's a good feedback. Isn't ASoC's FE<->BE treated as a single PCM substream in sound/core/pcm_native.c though? If so, does it even make sense for card's BE DAI to be atomic, if it's FE counterpart is nonatomic already? Especially if it is specifying platform and cpu_dai that matches Intel's components which we know communicate using IPCs. Warning is one thing, but will you be also getting rid of the if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE is already declared as such? If the if-statement stays, I believe the declaring BE DAIs 'correctly' in the way to go. Regards, Czarek
On 6/25/22 03:29, Cezary Rojewski wrote: > On 2022-06-24 3:52 PM, Pierre-Louis Bossart wrote: >> On 6/24/22 08:43, Cezary Rojewski wrote: >>> Address the warning: "Codec: dpcm_be_connect: FE is nonatomic but BE is >>> not, forcing BE as nonatomic" by marking BE DAI as nonatomic. Aligns >>> with what is already done for FE DAIs. >>> >>> This patchset iterates the change over all HSW and BDW related machine >>> board drivers. >> >> I don't think this is necessary, I was planning to demote this warning >> to a simple dev_dbg or possibly remove this message entirely. >> >> The BE DAIs can perfectly be declared as non-atomic in all Intel machine >> drivers, except for SoundWire where there's a known delay during the >> .trigger. > > > Hmm.. that's a good feedback. Isn't ASoC's FE<->BE treated as a single > PCM substream in sound/core/pcm_native.c though? If so, does it even > make sense for card's BE DAI to be atomic, if it's FE counterpart is > nonatomic already? Especially if it is specifying platform and cpu_dai > that matches Intel's components which we know communicate using IPCs. I guess it depends on the cpu_dai implementation. Not all implementations implement a delay in the .trigger callback and/or rely on IPCs. > Warning is one thing, but will you be also getting rid of the > if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE > is already declared as such? If the if-statement stays, I believe the > declaring BE DAIs 'correctly' in the way to go. I meant just removing the dev_warn() only. See https://github.com/thesofproject/linux/pull/3723
On 2022-06-27 4:45 PM, Pierre-Louis Bossart wrote: > On 6/25/22 03:29, Cezary Rojewski wrote: >> Hmm.. that's a good feedback. Isn't ASoC's FE<->BE treated as a single >> PCM substream in sound/core/pcm_native.c though? If so, does it even >> make sense for card's BE DAI to be atomic, if it's FE counterpart is >> nonatomic already? Especially if it is specifying platform and cpu_dai >> that matches Intel's components which we know communicate using IPCs. > > I guess it depends on the cpu_dai implementation. Not all > implementations implement a delay in the .trigger callback and/or rely > on IPCs. > >> Warning is one thing, but will you be also getting rid of the >> if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE >> is already declared as such? If the if-statement stays, I believe the >> declaring BE DAIs 'correctly' in the way to go. > > I meant just removing the dev_warn() only. > > See https://github.com/thesofproject/linux/pull/3723 So the framework is still fixing the flag for the driver. Ideally we would like to have all the drivers assign correct values to ->nonatomic flag themselves. Now when I think about it, the message seems useful - at least as dev_dbg(). It _guides_ driver developer to the desired approach: setting the ->nonatomic flag for BE to '1' if the corresponding FE is already configured as such. I've provided similar answer in the linked thread. Regards, Czarek
On 6/27/22 10:41, Cezary Rojewski wrote: > On 2022-06-27 4:45 PM, Pierre-Louis Bossart wrote: >> On 6/25/22 03:29, Cezary Rojewski wrote: > >>> Hmm.. that's a good feedback. Isn't ASoC's FE<->BE treated as a single >>> PCM substream in sound/core/pcm_native.c though? If so, does it even >>> make sense for card's BE DAI to be atomic, if it's FE counterpart is >>> nonatomic already? Especially if it is specifying platform and cpu_dai >>> that matches Intel's components which we know communicate using IPCs. >> >> I guess it depends on the cpu_dai implementation. Not all >> implementations implement a delay in the .trigger callback and/or rely >> on IPCs. >> >>> Warning is one thing, but will you be also getting rid of the >>> if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE >>> is already declared as such? If the if-statement stays, I believe the >>> declaring BE DAIs 'correctly' in the way to go. >> >> I meant just removing the dev_warn() only. >> >> See https://github.com/thesofproject/linux/pull/3723 > > So the framework is still fixing the flag for the driver. Ideally we > would like to have all the drivers assign correct values to ->nonatomic > flag themselves. > > Now when I think about it, the message seems useful - at least as > dev_dbg(). It _guides_ driver developer to the desired approach: setting > the ->nonatomic flag for BE to '1' if the corresponding FE is already > configured as such. that would result in unnecessary changes to all machine drivers to get rid of the message...
On 2022-06-27 5:59 PM, Pierre-Louis Bossart wrote: > On 6/27/22 10:41, Cezary Rojewski wrote: >> So the framework is still fixing the flag for the driver. Ideally we >> would like to have all the drivers assign correct values to ->nonatomic >> flag themselves. >> >> Now when I think about it, the message seems useful - at least as >> dev_dbg(). It _guides_ driver developer to the desired approach: setting >> the ->nonatomic flag for BE to '1' if the corresponding FE is already >> configured as such. > > that would result in unnecessary changes to all machine drivers to get > rid of the message... I would have rather used the word _optional_ here. dev_dbg() is a diplomatic solution. As I'm responsible for catpt and avs drivers, this very series fixes the issue for the former. AVS driver have had the flag set appropriately from the get go, so no fixes needed.
On Mon, Jun 27, 2022 at 09:45:30AM -0500, Pierre-Louis Bossart wrote: > On 6/25/22 03:29, Cezary Rojewski wrote: > > Warning is one thing, but will you be also getting rid of the > > if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE > > is already declared as such? If the if-statement stays, I believe the > > declaring BE DAIs 'correctly' in the way to go. > I meant just removing the dev_warn() only. > See https://github.com/thesofproject/linux/pull/3723 Is something going to be upstreamed here? I don't really mind which solution is adopted here but right now Cezary's patches are the ones that were posted upstream.
On 2022-07-08 5:44 PM, Mark Brown wrote: > On Mon, Jun 27, 2022 at 09:45:30AM -0500, Pierre-Louis Bossart wrote: >> On 6/25/22 03:29, Cezary Rojewski wrote: > >>> Warning is one thing, but will you be also getting rid of the >>> if-statement in soc-pcm.c that actually forces nonatomic=1 on BE when FE >>> is already declared as such? If the if-statement stays, I believe the >>> declaring BE DAIs 'correctly' in the way to go. > >> I meant just removing the dev_warn() only. > >> See https://github.com/thesofproject/linux/pull/3723 > > Is something going to be upstreamed here? I don't really mind which > solution is adopted here but right now Cezary's patches are the ones > that were posted upstream. My 2 cents: Both series can co-exist. We should still encourage people to configure their DAIs properly so the if-statements like the one forcing ->nonatomic=1 in time are gone.
On Fri, 24 Jun 2022 15:43:13 +0200, Cezary Rojewski wrote: > Address the warning: "Codec: dpcm_be_connect: FE is nonatomic but BE is > not, forcing BE as nonatomic" by marking BE DAI as nonatomic. Aligns > with what is already done for FE DAIs. > > This patchset iterates the change over all HSW and BDW related machine > board drivers. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/4] ASoC: Intel: hsw_rt5640: Mark BE DAI as nonatomic commit: 58ef0d3d5716000c153acc5401109fd90afbdf09 [2/4] ASoC: Intel: bdw_rt286: Mark BE DAI as nonatomic commit: 6d7e011808504e0aabbbf3b66d4c14982394abae [3/4] ASoC: Intel: bdw_rt5650: Mark BE DAI as nonatomic commit: 5c4ef9529b12865c2402784a7506c880178effda [4/4] ASoC: Intel: bdw_rt5677: Mark BE DAI as nonatomic commit: bdd15ec4888a375848030cbf7d9fc16c7f430f48 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