diff mbox series

[RFC,3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

Message ID 20220331000449.41062-4-povik+lin@cutebit.org (mailing list archive)
State New, archived
Headers show
Series Apple Macs machine-level ASoC driver | expand

Commit Message

Martin Povišer March 31, 2022, 12:04 a.m. UTC
Tolerate N-to-M DAI links while using the first CPU DAI to decide
playback/capture abilities.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/soc-pcm.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Mark Brown April 4, 2022, 12:28 p.m. UTC | #1
On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote:

> +#if 0
>  				dev_err(rtd->card->dev,
>  					"N cpus to M codecs link is not supported yet\n");
>  				return -EINVAL;
> +#endif
> +				cpu_dai = asoc_rtd_to_cpu(rtd, 0);

We need to figure out an interface for describing which CODEC/CPU
combinations are connected to each other.  I'm not seeing a great way to
do that right now, probably some side data table is going to be needed,
or perhaps the CPU DAI drivers can be persuaded to only have one DAI
actually register and claim to support more channels?  I'm not sure how
a configuraiton like this is going to work at userspace level if the
multiple CPU DAIs end up being visible...
Martin Povišer April 22, 2022, 2:06 p.m. UTC | #2
> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote:
> 
>> +#if 0
>> 				dev_err(rtd->card->dev,
>> 					"N cpus to M codecs link is not supported yet\n");
>> 				return -EINVAL;
>> +#endif
>> +				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> 
> We need to figure out an interface for describing which CODEC/CPU
> combinations are connected to each other.  I'm not seeing a great way to
> do that right now, probably some side data table is going to be needed,
> or perhaps the CPU DAI drivers can be persuaded to only have one DAI
> actually register and claim to support more channels?  I'm not sure how
> a configuraiton like this is going to work at userspace level if the
> multiple CPU DAIs end up being visible...

To understand the issue better: How could the multiple CPU DAIs be
visible from userspace?

What about this interim solution: In case of N-to-M links we put in
the most restrictive condition for checking capture/playback stream
validity: we check all of the CPU DAIs. Whatever ends up being the
proper solution later can only be less restrictive than this.

As a reminder what happens on the Macs: the platform driver drives
all the CPU-side I2S ports that belong to the link with the same data,
so the particular CPU/CODEC wiring doesn’t matter.
Mark Brown April 25, 2022, 12:25 p.m. UTC | #3
On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote:
> > On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:

> > We need to figure out an interface for describing which CODEC/CPU
> > combinations are connected to each other.  I'm not seeing a great way to
> > do that right now, probably some side data table is going to be needed,
> > or perhaps the CPU DAI drivers can be persuaded to only have one DAI
> > actually register and claim to support more channels?  I'm not sure how
> > a configuraiton like this is going to work at userspace level if the
> > multiple CPU DAIs end up being visible...

> To understand the issue better: How could the multiple CPU DAIs be
> visible from userspace?

If you register two separate DAIs (well, links) with the API without
doing anything else the API will just expose them to userspace as two
separate things with no indication that they're related.

> What about this interim solution: In case of N-to-M links we put in
> the most restrictive condition for checking capture/playback stream
> validity: we check all of the CPU DAIs. Whatever ends up being the
> proper solution later can only be less restrictive than this.

That's not the issue here?  

> As a reminder what happens on the Macs: the platform driver drives
> all the CPU-side I2S ports that belong to the link with the same data,
> so the particular CPU/CODEC wiring doesn’t matter.

Oh, that's not something I was aware of.  In that case this is the wrong
API - you should be using DPCM to map one front end onto multiple back
ends (Kirkwood does something similar IIRC, there will be other examples
but that's probably the simplest).  The back ends probably don't really
need to know that they're on the same physical bus (if indeed they are).
Martin Povišer April 25, 2022, 12:34 p.m. UTC | #4
> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote:
>>> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:
> 
>>> We need to figure out an interface for describing which CODEC/CPU
>>> combinations are connected to each other.  I'm not seeing a great way to
>>> do that right now, probably some side data table is going to be needed,
>>> or perhaps the CPU DAI drivers can be persuaded to only have one DAI
>>> actually register and claim to support more channels?  I'm not sure how
>>> a configuraiton like this is going to work at userspace level if the
>>> multiple CPU DAIs end up being visible...
> 
>> To understand the issue better: How could the multiple CPU DAIs be
>> visible from userspace?
> 
> If you register two separate DAIs (well, links) with the API without
> doing anything else the API will just expose them to userspace as two
> separate things with no indication that they're related.

Sure, but what I am addressing here is a single DAI link with multiple
CPU DAIs, invoked in DT like this:

	dai-link@0 {
		link-name = "Speakers";
		mclk-fs = <256>;

		cpu {
			sound-dai = <&mca 0>, <&mca 1>;
		};
		codec {
			sound-dai = <&speaker_left_woof1>,
				<&speaker_right_woof1>,
				<&speaker_left_tweet>,
				<&speaker_right_tweet>,
				<&speaker_left_woof2>,
				<&speaker_right_woof2>;
		};
	};

>> What about this interim solution: In case of N-to-M links we put in
>> the most restrictive condition for checking capture/playback stream
>> validity: we check all of the CPU DAIs. Whatever ends up being the
>> proper solution later can only be less restrictive than this.
> 
> That's not the issue here?

Well to me it looks like it is. Because if I invoke the DAI link like
I quoted above, and the platform driver supports it, the playback/capture
stream validity check is the only place it breaks down. Notwithstanding
this may be the wrong API as you wrote.

>> As a reminder what happens on the Macs: the platform driver drives
>> all the CPU-side I2S ports that belong to the link with the same data,
>> so the particular CPU/CODEC wiring doesn’t matter.
> 
> Oh, that's not something I was aware of.  In that case this is the wrong
> API - you should be using DPCM to map one front end onto multiple back
> ends (Kirkwood does something similar IIRC, there will be other examples
> but that's probably the simplest).  The back ends probably don't really
> need to know that they're on the same physical bus (if indeed they are).

I guess I need to look into that.
Mark Brown April 25, 2022, 12:55 p.m. UTC | #5
On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote:
> > On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:

> > If you register two separate DAIs (well, links) with the API without
> > doing anything else the API will just expose them to userspace as two
> > separate things with no indication that they're related.

> Sure, but what I am addressing here is a single DAI link with multiple
> CPU DAIs, invoked in DT like this:

> 	dai-link@0 {
> 		link-name = "Speakers";
> 		mclk-fs = <256>;
> 
> 		cpu {
> 			sound-dai = <&mca 0>, <&mca 1>;
> 		};
> 		codec {
> 			sound-dai = <&speaker_left_woof1>,
> 				<&speaker_right_woof1>,
> 				<&speaker_left_tweet>,
> 				<&speaker_right_tweet>,
> 				<&speaker_left_woof2>,
> 				<&speaker_right_woof2>;
> 		};
> 	};

You could parse this into two separate links for the benefit of the
framewokr if you're using a custom machine driver (which I suspect you
probably have to).

> >> What about this interim solution: In case of N-to-M links we put in
> >> the most restrictive condition for checking capture/playback stream
> >> validity: we check all of the CPU DAIs. Whatever ends up being the
> >> proper solution later can only be less restrictive than this.

> > That's not the issue here?

> Well to me it looks like it is. Because if I invoke the DAI link like
> I quoted above, and the platform driver supports it, the playback/capture
> stream validity check is the only place it breaks down. Notwithstanding
> this may be the wrong API as you wrote.

I am surprised that doesn't otherwise explode TBH - at the very least
I'd expect it to show two PCMs to userspace which if I'm understanding
your description correctly isn't really what's going on.
Martin Povišer April 25, 2022, 1:11 p.m. UTC | #6
> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote:
>>> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:
> 
>>> If you register two separate DAIs (well, links) with the API without
>>> doing anything else the API will just expose them to userspace as two
>>> separate things with no indication that they're related.
> 
>> Sure, but what I am addressing here is a single DAI link with multiple
>> CPU DAIs, invoked in DT like this:
> 
>> 	dai-link@0 {
>> 		link-name = "Speakers";
>> 		mclk-fs = <256>;
>> 
>> 		cpu {
>> 			sound-dai = <&mca 0>, <&mca 1>;
>> 		};
>> 		codec {
>> 			sound-dai = <&speaker_left_woof1>,
>> 				<&speaker_right_woof1>,
>> 				<&speaker_left_tweet>,
>> 				<&speaker_right_tweet>,
>> 				<&speaker_left_woof2>,
>> 				<&speaker_right_woof2>;
>> 		};
>> 	};
> 
> You could parse this into two separate links for the benefit of the
> framewokr if you're using a custom machine driver (which I suspect you
> probably have to).

Yeah, this is parsed by the ‘macaudio’ machine driver from the series.

>>>> What about this interim solution: In case of N-to-M links we put in
>>>> the most restrictive condition for checking capture/playback stream
>>>> validity: we check all of the CPU DAIs. Whatever ends up being the
>>>> proper solution later can only be less restrictive than this.
> 
>>> That's not the issue here?
> 
>> Well to me it looks like it is. Because if I invoke the DAI link like
>> I quoted above, and the platform driver supports it, the playback/capture
>> stream validity check is the only place it breaks down. Notwithstanding
>> this may be the wrong API as you wrote.
> 
> I am surprised that doesn't otherwise explode TBH - at the very least
> I'd expect it to show two PCMs to userspace which if I'm understanding
> your description correctly isn't really what's going on.

I fill in a single snd_soc_dai_link, it exposes a single PCM and works
like a charm. That is as long as I patch the playback/capture check in
question.

I read that to be the clear intention of ASoC code: a DAI link becomes
one snd_soc_pcm_runtime.
Mark Brown April 25, 2022, 1:46 p.m. UTC | #7
On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote:
> > On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:

> > I am surprised that doesn't otherwise explode TBH - at the very least
> > I'd expect it to show two PCMs to userspace which if I'm understanding
> > your description correctly isn't really what's going on.

> I fill in a single snd_soc_dai_link, it exposes a single PCM and works
> like a charm. That is as long as I patch the playback/capture check in
> question.

> I read that to be the clear intention of ASoC code: a DAI link becomes
> one snd_soc_pcm_runtime.

Yes, so long as you boil it down to a single link it works fine but the
bit on top of the binding where you tie the two CPU DAIs to what is
actually exposed is all in code.  The reason this stuff isn't filled in
is that connecting the thing that applications see to the physical links
isn't at all obvious and needs at least some driver sitting in the
middle to make the links - I'd imagine there's a DSP sitting there which
probably has quite a bit of flexability about how the various hardware
components available are actually related.  This makes figuring out what
to do with the relationship between the multiple CPU DAIs hard.
Martin Povišer April 25, 2022, 1:55 p.m. UTC | #8
> On 25. 4. 2022, at 15:46, Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote:
>>> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:
> 
>>> I am surprised that doesn't otherwise explode TBH - at the very least
>>> I'd expect it to show two PCMs to userspace which if I'm understanding
>>> your description correctly isn't really what's going on.
> 
>> I fill in a single snd_soc_dai_link, it exposes a single PCM and works
>> like a charm. That is as long as I patch the playback/capture check in
>> question.
> 
>> I read that to be the clear intention of ASoC code: a DAI link becomes
>> one snd_soc_pcm_runtime.
> 
> Yes, so long as you boil it down to a single link it works fine but the
> bit on top of the binding where you tie the two CPU DAIs to what is
> actually exposed is all in code.  The reason this stuff isn't filled in
> is that connecting the thing that applications see to the physical links
> isn't at all obvious and needs at least some driver sitting in the
> middle to make the links - I'd imagine there's a DSP sitting there which
> probably has quite a bit of flexability about how the various hardware
> components available are actually related.  This makes figuring out what
> to do with the relationship between the multiple CPU DAIs hard.

I get the gist. Anyway unless you tell me otherwise I will assume I need
to move to DPCM with the platform/machine driver.
diff mbox series

Patch

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 9a954680d492..770cf367a147 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2781,9 +2781,12 @@  static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 			} else if (rtd->num_cpus == rtd->num_codecs) {
 				cpu_dai = asoc_rtd_to_cpu(rtd, i);
 			} else {
+#if 0
 				dev_err(rtd->card->dev,
 					"N cpus to M codecs link is not supported yet\n");
 				return -EINVAL;
+#endif
+				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 			}
 
 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&