Skip to content

fixup! ASoC: soc-pcm: dpcm: fix playback/capture checks#2070

Merged
plbossart merged 4 commits intothesofproject:topic/sof-devfrom
plbossart:fix/sof-new-pcm-take2
Jun 4, 2020
Merged

fixup! ASoC: soc-pcm: dpcm: fix playback/capture checks#2070
plbossart merged 4 commits intothesofproject:topic/sof-devfrom
plbossart:fix/sof-new-pcm-take2

Conversation

@plbossart
Copy link
Member

Recent changes in the ASoC core prevent multi-cpu BE dailinks from
being used. DPCM does support multi-cpu DAIs for BE Dailinks, but not
for FE.

Handle the FE checks first, and make sure all DAIs support the same
capabilities within the same dailink.

BugLink: #2031
Fixes: 9b5db05 ("ASoC: soc-pcm: dpcm: Only allow playback/capture if supported")
Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

@plbossart
Copy link
Member Author

Correct PR #2058 merged too quickly

bardliao
bardliao previously approved these changes Apr 30, 2020
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: was there a bug in the previous version or was it just less pretty?

RanderWang
RanderWang previously approved these changes May 25, 2020
@plbossart plbossart dismissed stale reviews from RanderWang and bardliao via 4fb042b May 26, 2020 16:12
@plbossart plbossart force-pushed the fix/sof-new-pcm-take2 branch from db35317 to 4fb042b Compare May 26, 2020 16:12
@plbossart
Copy link
Member Author

@lyakh I followed your second example, I don't think we can avoid parsing all cpu_dais separately for capture and playback.

lyakh
lyakh previously approved these changes May 26, 2020
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a common loop with all ifs embedded would work too, but this version does perhaps look better - it avoids re-testing the same condition and re-writing playback and capture variables on each iteration.

RanderWang
RanderWang previously approved these changes May 27, 2020
Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't merge it for now. It may not related to this PR, but somehow rtd->dai_link->dpcm_playback and rtd->dai_link->dpcm_capture are both 1 for every dai links. So, we will see "CPU DAI iDisp1 for rtd iDisp1 does not support capture" error. I checked sof_sdw.c and we set dpcm_playback and dpcm_capture properly.

bardliao
bardliao previously approved these changes May 28, 2020
Copy link
Collaborator

@bardliao bardliao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "CPU DAI iDisp1 for rtd iDisp1 does not support capture" error is not caused by this PR. But we may want to fix it before merging this. See #2143

@plbossart plbossart dismissed stale reviews from bardliao, RanderWang, and lyakh via 473c24a May 28, 2020 20:19
@plbossart plbossart force-pushed the fix/sof-new-pcm-take2 branch from 4fb042b to 473c24a Compare May 28, 2020 20:19
It's not clear why specific FE dailinks use capture_only flags, likely
blind copy/paste from Chromebook driver to the other.  Replace by
dpcm_capture, this will make future alignment and removal of flags
easier.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart dismissed stale reviews from RanderWang, dbaluta, and bardliao via b38efd6 June 2, 2020 18:48
@plbossart plbossart force-pushed the fix/sof-new-pcm-take2 branch from 473c24a to b38efd6 Compare June 2, 2020 18:48
@plbossart
Copy link
Member Author

@dbaluta @bardliao @RanderWang @lyakh minor update with @lyakh 's optimization.

lyakh
lyakh previously approved these changes Jun 3, 2020
bardliao
bardliao previously approved these changes Jun 3, 2020
dbaluta
dbaluta previously approved these changes Jun 3, 2020
@plbossart
Copy link
Member Author

Still an error in APL-nocodec mode:
jf-apl-up2-nocodec-1 kernel: [ 3.000418] sof-nocodec sof-nocodec: CPU DAI DMIC01 Pin for rtd NoCodec-6 does not support playback

WTH?

With additional checks on dailinks, we see errors such as

[ 3.000418] sof-nocodec sof-nocodec: CPU DAI DMIC01 Pin for rtd
NoCodec-6 does not support playback

It's not clear why we set the dpcm_playback and dpcm_capture flags
unconditionally, add a check on number of channels for each direction
to avoid invalid configurations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart dismissed stale reviews from dbaluta, bardliao, and lyakh via b4d0ae3 June 3, 2020 19:48
@plbossart plbossart requested a review from libinyang as a code owner June 3, 2020 19:48
@plbossart
Copy link
Member Author

Added one more commit with the following diff

diff --git a/sound/soc/sof/nocodec.c b/sound/soc/sof/nocodec.c
index ce053ba8f2e8..d03b5be31255 100644
--- a/sound/soc/sof/nocodec.c
+++ b/sound/soc/sof/nocodec.c
@@ -52,8 +52,10 @@ static int sof_nocodec_bes_setup(struct device *dev,
                links[i].platforms->name = dev_name(dev);
                links[i].codecs->dai_name = "snd-soc-dummy-dai";
                links[i].codecs->name = "snd-soc-dummy";
-               links[i].dpcm_playback = 1;
-               links[i].dpcm_capture = 1;
+               if (ops->drv[i].playback.channels_min)
+                       links[i].dpcm_playback = 1;
+               if (ops->drv[i].capture.channels_min)
+                       links[i].dpcm_capture = 1;
        }
 
        card->dai_link = links;

Not sure how all of this ever worked :-(

@plbossart plbossart requested review from bardliao, dbaluta and lyakh June 3, 2020 20:30
@plbossart
Copy link
Member Author

Tests all good now, need reviews again. Thanks @dbaluta @lyakh @bardliao @kv2019i

@plbossart plbossart merged commit 66408e2 into thesofproject:topic/sof-dev Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants