On 6/17/20 4:01 AM, Stephan Gerhold wrote:
On Tue, Jun 16, 2020 at 12:05:49PM -0500, Pierre-Louis Bossart wrote:
simple-card.c and audio-graph-card do hard-code but that's done
with C in
the driver:
ret = asoc_simple_parse_daifmt(dev, cpu_ep, codec_ep,
NULL, &dai_link->dai_fmt);
if (ret < 0)
goto out_put_node;
dai_link->dpcm_playback = 1;
dai_link->dpcm_capture = 1;
that that should be fixed based on the DAI format used in that
dai_link - in
other words we can make sure the capabilities of the dailink are aligned
with the dais while parsing the DT blobs.
But how do you know which capabilities to set? The device tree doesn't
tells us that. We could add some code to look up the snd_soc_dai_driver
early, based on the references in the device tree (basically something
like snd_soc_of_get_dai_name(), see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-core.c#n2988)
At least to me that function doesn't exactly look trivial though,
and that's just to properly fill in the dpcm_playback/capture
parameters. Essentially those parameters only complicate the current
device tree use case, where you want the DAI link to be for both
playback/capture, but restricted to the capabilities of the DAI.
Just wondering if setting up dpcm_playback/capture properly is worth it
at all in this case. This isn't necessary for the non-DPCM case either,
there we automatically set it based on the DAI capabilities.
We can add a simple loop for each direction that relies on
snd_soc_dai_stream_valid() to identify if each DAI is capable of doing
playback/capture.
see below completely untested diff to show what I had in mind: we already
make use of snd_soc_dai_stream_valid() in other parts of the core so we
should be able to determine dpcm_playback/capture based on the same
information already used.
diff --git a/sound/soc/generic/audio-graph-card.c
b/sound/soc/generic/audio-graph-card.c
index 9ad35d9940fe..4c67f1f65eb4 100644
--- a/sound/soc/generic/audio-graph-card.c
+++ b/sound/soc/generic/audio-graph-card.c
@@ -215,7 +215,9 @@ static int graph_dai_link_of_dpcm(struct
asoc_simple_priv *priv,
struct asoc_simple_dai *dai;
struct snd_soc_dai_link_component *cpus = dai_link->cpus;
struct snd_soc_dai_link_component *codecs = dai_link->codecs;
+ int stream;
int ret;
+ int i;
/* Do it all CPU endpoint, and 1st Codec endpoint */
if (!li->cpu && dup_codec)
@@ -317,8 +319,34 @@ static int graph_dai_link_of_dpcm(struct
asoc_simple_priv *priv,
if (ret < 0)
goto out_put_node;
- dai_link->dpcm_playback = 1;
- dai_link->dpcm_capture = 1;
+ for_each_pcm_streams(stream) {
+ struct snd_soc_dai_link_component *cpu;
+ struct snd_soc_dai_link_component *codec;
+ struct snd_soc_dai *d;
+ bool dpcm_direction = true;
+
+ for_each_link_cpus(dai_link, i, cpu) {
+ d = snd_soc_find_dai(cpu);
+ if (!d || !snd_soc_dai_stream_valid(d, stream)) {
+ dpcm_direction = false;
+ break;
+ }
+ }
+ for_each_link_codecs(dai_link, i, codec) {
+ d = snd_soc_find_dai(codec);
+ if (!d || !snd_soc_dai_stream_valid(d, stream)) {
+ dpcm_direction = false;
+ break;
+ }
+ }
+ if (dpcm_direction) {
+ if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+ dai_link->dpcm_playback = 1;
+ if (stream == SNDRV_PCM_STREAM_CAPTURE)
+ dai_link->dpcm_capture = 1;
+ }
+ }
+
dai_link->ops = &graph_ops;
dai_link->init = asoc_simple_dai_init;
Thanks for the diff! I tested it for my case and it seems to work fine
so far. I'm fine with this solution given that it fixes the problem
I mentioned. We would need to patch it into at least
simple-audio-card.c, audio-graph-card.c and soc/qcom/common.c
(probably best to create a shared function in soc-core.c then).
Yes, I worked on a helper in soc-dai.c and have a tentative proposal in
https://github.com/thesofproject/linux/pull/2203
However, personally I still think that dpcm_playback/capture essentially
just duplicate the capabilities that are already exposed as part of the
DAI drivers. We don't need that duplication in the non-DPCM case,
so I wonder if we really need it for DPCM.
Fully agree, but removing
dpcm_playback/capture/playback_only/capture_only should be done in a
follow-up patchset. It's just too complicated to both fix the current
DPCM configurations and clean-up at the same time, it'd prefer to do
this cleanup in two steps.
With your diff we go over all the DAIs to set dpcm_playback/capture
correctly so that soc_new_pcm() can then verify that they were set
correctly. IMO it would be much simpler to restore the previous behavior
and just make soc_new_pcm() rely on the DAI capabilities to decide
if playback/capture is supported, without producing the error.
I don't understand what previous behavior you are referring to (it's not
something I personally changed), and these flags are also hard-coded in
static dailink descriptors for machine drivers.
