Skip to content

ASoC: soc-pcm: dpcm: fix playback/capture checks#2058

Merged
ranj063 merged 1 commit intothesofproject:topic/sof-devfrom
plbossart:fix/soc-new-pcm
Apr 29, 2020
Merged

ASoC: soc-pcm: dpcm: fix playback/capture checks#2058
ranj063 merged 1 commit intothesofproject:topic/sof-devfrom
plbossart:fix/soc-new-pcm

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

Fixes #2031

@plbossart
Copy link
Member Author

@Minecrell FYI

bardliao
bardliao previously approved these changes Apr 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.

LGTM

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

I run out of time a bit -- it seems I'd need to study the surrounding code more to really understand whether this is ok or not.


if (rtd->dai_link->dpcm_playback) {
stream = SNDRV_PCM_STREAM_PLAYBACK;
if (snd_soc_dai_stream_valid(cpu_dai, stream)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, ASoC makes my head hurt again.

I don't quite get why "snd_soc_dai_stream_valid()==true" must apply for the first CPU DAI of the rtd, but for 2..N CPU DAIs, stream_valid check can fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

the idea was to check that ALL cpu_dais in a dailink that says it can do playack can indeed do so. So my proposal is to bail if you encounter one DAI that doesn't support playback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plbossart I was thinking of following sequence:

CPU DAI #1 capture only but dai_link->dpcm_playback is set for the rtd so we go to L2923
CPU DAI #2 playback -> ok, playback set as 1
CPU DAI #3 another capture DAI -> as playback was set, we will return error that "CPU DAI #3 does not support playback"

... I guess this is not a case that can practically happen. If you think this is ok, I'm ok as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

no, you're right this doesn't work in all cases.
Maybe the correct thing to do is just increment a counter for valid dais, and check after the look that they are all valid.

Copy link
Collaborator

@ranj063 ranj063 Apr 29, 2020

Choose a reason for hiding this comment

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

@kv2019i this check is good to catch human errors with the dai drv array declaration in the driver.

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: thesofproject#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>
@ranj063 ranj063 merged commit f97d55a into thesofproject:topic/sof-dev Apr 29, 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.

6 participants