Skip to content

ASoC: core: only convert non DPCM link to DPCM link#2143

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bardliao wants to merge 1 commit into from
Closed

ASoC: core: only convert non DPCM link to DPCM link #2143

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions sound/soc/soc-core.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -1657,8 +1657,13 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)

/* convert non BE into BE */
dai_link->no_pcm = 1;
dai_link->dpcm_playback = 1;
dai_link->dpcm_capture = 1;

/* convert a normal link to a DPCM link */

@dbalutadbalutaMay 28, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

At this moment in code we can reach if dai_link is a normal link (no_pcm = 0, dynamic=0) or a BE link (no_pcm=1).

If the link is already BE, we should do nothing.
if the link is normal link, we should convert it to BE.
-> no_pcm = 1
-> now here is the challenge as dai_link doesn't have a .capture or .playback field.

It has:

-> playback_only=1
-> .dpcm_playback = 1
-> dpcm_capture = 0
-> capture_only =1
-> .dpcm_playback = 0
-> .dpcm_capture =1
-> BUT what does capture_only = 0 and playback_only = 0 means, logically it could be either bot playback/capture supported or none.

@bardliaobardliaoMay 28, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

BUT what does capture_only = 0 and playback_only = 0 means, logically it could be either bot playback/capture supported or none.

I thought about the capture_only and playback_only flag, too. But it seems it is set by a few machine driver only. I am not sure if we can relay on the capture_only and playback_only flag or not. The other question is that is there any link supports none of playback and capture?

@dbalutadbalutaMay 28, 2020•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao If I would have to guess I would say that not-supporting playback/capture makes no sense. So, I think we can infer that by default if capture_only is not set and playback_only is not set then both playback/capture are supported.
I have sent an email to alsa mailing list to clarify this situation:

https://mailman.alsa-project.org/pipermail/alsa-devel/2020-May/168352.html

I think the best way to fix this is as follows:

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 8321e75ff244..464004648bd0 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1722,9 +1722,18 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
dai_link->platforms->name = component->name;
/* convert non BE into BE */
- dai_link->no_pcm = 1;
- dai_link->dpcm_playback = 1;
- dai_link->dpcm_capture = 1;
+ if (!dai_link->no_pcm) {
+
+ dai_link->no_pcm = 1;
+
+ dai_link->dpcm_playback = 1;
+ dai_link->dpcm_capture = 1;
+
+ if (dai_link->playback_only)
+ dai_link->dpcm_capture = 0;
+ if (dai_link->capture_only)
+ dai_link->dpcm_playback = 0;
+ }

@plbossartplbossartMay 28, 2020•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@dbaluta I asked the question on alsa-devel on why we have separate flags for dpcm and non-dpcm solutions. I think it's a complete mess. We should have .pcm_playback and .pcm_capture flags only, you can infer playback_only and capture_only when only one of the two is set.

if (!(dai_link->dpcm_playback ||
dai_link->dpcm_capture)) {
dai_link->dpcm_playback = 1;
dai_link->dpcm_capture = 1;
}

@RanderWangRanderWangMay 28, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bardliao why we get BE here ? this function only deal with FE in topology.

@bardliaobardliaoMay 28, 2020

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@RanderWang If soc_check_tplg_fes find a matched machine (like sof_sdw), it will overwrite all preset links. See for_each_card_prelinks(card, i, dai_link)

@RanderWangRanderWangMay 28, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

so soc_check_tplg_fes doesn't only check fes. why force dpcm_playback & dpcm_capture to 1 ? I don't get it.

@dbalutadbalutaMay 28, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@RanderWang@bardliao i think I'm at fault for this

commit 218fe9b7ec7f32c10a07539365488d80af7b0084
Author: Daniel Baluta <daniel.baluta@nxp.com>
Date: Wed Dec 4 17:13:33 2019 +0200
ASoC: soc-core: Set dpcm_playback / dpcm_capture
When converting a normal link to a DPCM link we need
to set dpcm_playback / dpcm_capture otherwise playback/capture
streams will not be created resulting in errors like this:
[ 36.039111] sai1-wm8960-hifi: ASoC: no backend playback stream
Fixes: a655de808cbde ("ASoC: core: Allow topology to override machine driver FE DAI link config")

I think the proper fix would be to check if the original link has playback OR capture capability and update only that capability instead of updating both playback and capture.


/* override any BE fixups */
dai_link->be_hw_params_fixup =
Expand Down