ASoc: Handle multiple codecs with split playback / capture
diff mbox

Message ID alpine.DEB.2.02.1508131605170.27635@lnxricardw1.se.axis.com
State New
Headers show

Commit Message

Ricard Wanderlof Aug. 13, 2015, 2:12 p.m. UTC
(Referring to this thread:
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html)

This patch adds the capability to use multiple codecs on the same DAI 
link where one codec is used for playback and another one is used for 
capture.

As noted in the thread, there might be more places where the new 
snd_soc_dai_stream_valid() function is needed, however, these two were 
sufficient for our use case (SSM2518 playback and ICS-43432 capture).

Signed-off-by: Ricard Wanderlof <ricardw@xis.com>
---
 sound/soc/soc-pcm.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Mark Brown Aug. 14, 2015, 3:52 p.m. UTC | #1
On Thu, Aug 13, 2015 at 04:12:56PM +0200, Ricard Wanderlof wrote:

> (Referring to this thread:
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html)

I shouldn't need to read another mailing list thread as the first part
of a commit log, and if referring to external information you should
always include a human readable description of what you're referring to
so that people can understand your message without having to start
another program.  As it is I'm in a plane over the Atlantic so I can't
tell what you're talking about here, sorry.

> As noted in the thread, there might be more places where the new

Please write the commit log to describe the change without needing to
refer to other threads.

>  /**
> + * snd_soc_dai_stream_valid() - check if a DAI supports the given stream
> + *
> + * @dai: The DAI to check for stream support
> + * @stream: The stream type (playback / capture) to check for

You're adding externally readable kerneldoc on a static function so
nothing outside this file can use it.

> + *
> + * Checks if the DAI in question supports the indicated stream type.
> + * Needed to skip over codecs when iterating over
> + * snd_soc_pcm_runtime->codec_dais in order to skip codecs that are playback-
> + * or capture-only, which would otherwise inhibit capture or playback,
> + * respectively, to the other codec in a multiple codec / common I2S bus
> + * environment.
> + */

This mostly seems to describe a potential use of this function, not what
it is supposed to do.

> +static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
> +{
> +	struct snd_soc_pcm_stream *codec_stream;
> +
> +	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		codec_stream = &dai->driver->playback;
> +	else
> +		codec_stream = &dai->driver->capture;
> +
> +	/* If the codec specifies any rate at all, it supports the stream. */
> +	return !!codec_stream->rates;

The !! is redundant here.

>  	/* first calculate min/max only for CODECs in the DAI link */
>  	for (i = 0; i < rtd->num_codecs; i++) {
> +
> +		/* Skip codecs which don't support the current stream type. */
> +		if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],
> +					      substream->stream))
> +			continue;
> +
>  		codec_dai_drv = rtd->codec_dais[i]->driver;
>  		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  			codec_stream = &codec_dai_drv->playback;

So what happens if we end up in these loops and nothing matches?  It'd
be good to document the considerations there.

> @@ -827,6 +859,10 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
>  		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
>  		struct snd_pcm_hw_params codec_params;
>  
> +		/* Skip codecs which don't support the current stream type. */
> +		if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))
> +			continue;
> +
>  		/* copy params for each codec */
>  		codec_params = *params;

We're still calling hw_params() for all CODECs, even ones that happen to
not be suitable...  is that sensible?  It might be in a case where the
CODEC for one direction clocks everything but I can see problems
occurring if the transmit and receive paths are separately clocked which
is going to be quite common for things like i.MX systems.  If the two
are tied together there's going to be constraints between the two
directions.

I'd expect that we could take a pretty good guess as to what the best
thing to do is based on the symmetry constraints we have for the link
but I think we do want to think about this early.
Ricard Wanderlof Aug. 17, 2015, 8:02 a.m. UTC | #2
On Fri, 14 Aug 2015, Mark Brown wrote:

> On Thu, Aug 13, 2015 at 04:12:56PM +0200, Ricard Wanderlof wrote:
> 
> > (Referring to this thread:
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093214.html)
> 
> I shouldn't need to read another mailing list thread as the first part
> of a commit log, and if referring to external information you should
> always include a human readable description of what you're referring to
> so that people can understand your message without having to start
> another program.  As it is I'm in a plane over the Atlantic so I can't
> tell what you're talking about here, sorry.

The reference isn't necessary for the patch, it was intended to provide a 
link to a previous discussion that led up to it.

I'll restructure it to keep links out of the commit message.

Should the commit message have any information as to which specific use 
case led to the creation of the patch?

> >  /**
> > + * snd_soc_dai_stream_valid() - check if a DAI supports the given stream
> > + *
> > + * @dai: The DAI to check for stream support
> > + * @stream: The stream type (playback / capture) to check for
> 
> You're adding externally readable kerneldoc on a static function so
> nothing outside this file can use it.

My mistake. Probably started out by having it global then realized 
it was not necessary as it is not needed anywhere else for the moment at 
least.

> > + *
> > + * Checks if the DAI in question supports the indicated stream type.
> > + * Needed to skip over codecs when iterating over
> > + * snd_soc_pcm_runtime->codec_dais in order to skip codecs that are playback-
> > + * or capture-only, which would otherwise inhibit capture or playback,
> > + * respectively, to the other codec in a multiple codec / common I2S bus
> > + * environment.
> > + */
> 
> This mostly seems to describe a potential use of this function, not what
> it is supposed to do.

Ok, I'll rephrase it.

> > +	return !!codec_stream->rates;
> 
> The !! is redundant here.

I still felt it made sense as rates is an int and the function actually 
returns a bool, i.e. the !! indicates to a human reader that the author of 
the function is aware that rates is not a bool yet the function is 
declared as one.

> >  	/* first calculate min/max only for CODECs in the DAI link */
> >  	for (i = 0; i < rtd->num_codecs; i++) {
> > +
> > +		/* Skip codecs which don't support the current stream type. */
> > +		if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],
> > +					      substream->stream))
> > +			continue;
> > +
> >  		codec_dai_drv = rtd->codec_dais[i]->driver;
> >  		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >  			codec_stream = &codec_dai_drv->playback;
> 
> So what happens if we end up in these loops and nothing matches?  It'd
> be good to document the considerations there.

Ok, I'll add this.

> > @@ -827,6 +859,10 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> >  		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
> >  		struct snd_pcm_hw_params codec_params;
> >  
> > +		/* Skip codecs which don't support the current stream type. */
> > +		if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))
> > +			continue;
> > +
> >  		/* copy params for each codec */
> >  		codec_params = *params;
> 
> We're still calling hw_params() for all CODECs, even ones that happen to
> not be suitable...  is that sensible? 

Hm, how so, the soc_dai_hw_params(..., codec_dai) is further on inside the 
loop so the continue skips over it?

However, if nothing matches, soc_dai_hw_params(..., cpu_dai) would still 
be called, just after the loop, which might not make sense. 

> It might be in a case where the CODEC for one direction clocks 
> everything but I can see problems occurring if the transmit and receive 
> paths are separately clocked which is going to be quite common for 
> things like i.MX systems.  If the two are tied together there's going to 
> be constraints between the two directions.
> 
> I'd expect that we could take a pretty good guess as to what the best
> thing to do is based on the symmetry constraints we have for the link
> but I think we do want to think about this early.

I guess it would be better just to 'goto out' after the loop if a suitable 
codec could not be found.

/Ricard
Mark Brown Aug. 17, 2015, 8:44 p.m. UTC | #3
On Mon, Aug 17, 2015 at 10:02:29AM +0200, Ricard Wanderlof wrote:
> On Fri, 14 Aug 2015, Mark Brown wrote:
> > On Thu, Aug 13, 2015 at 04:12:56PM +0200, Ricard Wanderlof wrote:

> Should the commit message have any information as to which specific use 
> case led to the creation of the patch?

That's often good to mention, yes.

> > > +	return !!codec_stream->rates;

> > The !! is redundant here.

> I still felt it made sense as rates is an int and the function actually 
> returns a bool, i.e. the !! indicates to a human reader that the author of 
> the function is aware that rates is not a bool yet the function is 
> declared as one.

It's sufficiently obvious that you're just checking if rates has
anything set at all here I think.

> > > @@ -827,6 +859,10 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
> > >  		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
> > >  		struct snd_pcm_hw_params codec_params;

> > > +		/* Skip codecs which don't support the current stream type. */
> > > +		if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))
> > > +			continue;

> > >  		/* copy params for each codec */
> > >  		codec_params = *params;

> > We're still calling hw_params() for all CODECs, even ones that happen to
> > not be suitable...  is that sensible? 

> Hm, how so, the soc_dai_hw_params(..., codec_dai) is further on inside the 
> loop so the continue skips over it?

> However, if nothing matches, soc_dai_hw_params(..., cpu_dai) would still 
> be called, just after the loop, which might not make sense. 

I think that's the bit I was thinking of, sorry.

> > It might be in a case where the CODEC for one direction clocks 
> > everything but I can see problems occurring if the transmit and receive 
> > paths are separately clocked which is going to be quite common for 
> > things like i.MX systems.  If the two are tied together there's going to 
> > be constraints between the two directions.

> > I'd expect that we could take a pretty good guess as to what the best
> > thing to do is based on the symmetry constraints we have for the link
> > but I think we do want to think about this early.

> I guess it would be better just to 'goto out' after the loop if a suitable 
> codec could not be found.

Well, there's two things here: one is if we've got a completely invalid
configuration with no CODECs being configured and the other is if we
should only be configuring matching CODECs in the first place.  For
systems where the LRCLK and BCLK are shared between playback and capture
we may want to always configure them even if the CODEC driving the
clocks isn't the one handling the audio.
Ricard Wanderlof Aug. 19, 2015, 2:15 p.m. UTC | #4
On Mon, 17 Aug 2015, Mark Brown wrote:

> On Mon, Aug 17, 2015 at 10:02:29AM +0200, Ricard Wanderlof wrote:
> > On Fri, 14 Aug 2015, Mark Brown wrote:
> > > On Thu, Aug 13, 2015 at 04:12:56PM +0200, Ricard Wanderlof wrote:
> 
> > Should the commit message have any information as to which specific 
> > use case led to the creation of the patch?
> 
> That's often good to mention, yes.

Ok, will do.

> > > > +	return !!codec_stream->rates;
> 
> > > The !! is redundant here.
> 
> > I still felt it made sense as rates is an int and the function 
> > actually returns a bool, i.e. the !! indicates to a human reader that 
> > the author of the function is aware that rates is not a bool yet the 
> > function is declared as one.
> 
> It's sufficiently obvious that you're just checking if rates has anything 
> set at all here I think.

Ok. I will change it then.

> > > > +		/* Skip codecs which don't support the current stream 
> type. */
> > > > +		if (!snd_soc_dai_stream_valid(codec_dai, 
> substream->stream))
> > > > +			continue;
> 
> > > >  		/* copy params for each codec */
> > > >  		codec_params = *params;
> 
> > > We're still calling hw_params() for all CODECs, even ones that 
> > > happen to not be suitable...  is that sensible?
> 
> > Hm, how so, the soc_dai_hw_params(..., codec_dai) is further on inside 
> > the loop so the continue skips over it?
> 
> > However, if nothing matches, soc_dai_hw_params(..., cpu_dai) would 
> > still be called, just after the loop, which might not make sense.
> 
> I think that's the bit I was thinking of, sorry.

[in light of this, rephrasing Mark's comment to:]
We're still calling soc_dai_hw_params() even if no CODEC DAI matches.
> > > ...  is that sensible? It might be in a 
> > > case where the CODEC for one direction clocks everything but I can 
> > > see problems occurring if the transmit and receive paths are 
> > > separately clocked which is going to be quite common for things like 
> > > i.MX systems.  If the two are tied together there's going to be 
> > > constraints between the two directions.
> 
> > > I'd expect that we could take a pretty good guess as to what the 
> > > best thing to do is based on the symmetry constraints we have for 
> > > the link but I think we do want to think about this early.
> 
> > I guess it would be better just to 'goto out' after the loop if a 
> > suitable codec could not be found.
> 
> Well, there's two things here: one is if we've got a completely invalid 
> configuration with no CODECs being configured and the other is if we 
> should only be configuring matching CODECs in the first place.  For 
> systems where the LRCLK and BCLK are shared between playback and capture 
> we may want to always configure them even if the CODEC driving the clocks 
> isn't the one handling the audio.

Considering the first thing, i.e. a completely invalid configuration, such 
as trying to play back on a capture-only DAI link, then I would expect 
that would be taken care of long before we get to even attempt to 
configure anything, resulting in something like 'Unknown PCM' at the user 
level. We could of course bail out of this function before attempting to 
set up the CPU DAI in the event that that type of situation occurs anyway. 
I can add code to do just that.

The second thing is which codecs to configure. Should we attempt to 
configure all of them, even those that don't match the stream type, or (as 
the patch is currently written) only the ones that match the stream type. 
Let me see if I get the issues right here:

In the former case, IIUC, this might be needed if we have a codec that is 
generating clocks even though it is not actually used for the transfer, 
e.g. a playback codec generating LRCLK and/or BCLK for the whole link, 
including the capture codec. In this case we would want to set up both 
codecs even though only one is actually used.

In the latter case there might be systems which need to set up different 
clocking configurations for playback and capture on the CPU DAI, in which 
case we only want to configure matching codecs.

So how do we differentiate between these two cases? It seems to me that we 
cannot determine this using software means as it is really part of how the 
hardware is designed, so it must either be specified in the machine driver 
or via DT.

Since the current situation is that we can't support separate playback 
and capture codecs on a DAI link at all without this patch, my suggestion 
is to let the patch handle the case at hand, and add a comment as to which 
considerations have been made regarding which codecs are to be configured 
in soc_pcm_hw_params(), so that when the need arises to handle cases with 
different CPU DAI settings for both directions that can be handled 
appropriately.

/Ricard

Patch
diff mbox

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 6b0136e..df1f261 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -35,6 +35,32 @@ 
 #define DPCM_MAX_BE_USERS	8
 
 /**
+ * snd_soc_dai_stream_valid() - check if a DAI supports the given stream
+ *
+ * @dai: The DAI to check for stream support
+ * @stream: The stream type (playback / capture) to check for
+ *
+ * Checks if the DAI in question supports the indicated stream type.
+ * Needed to skip over codecs when iterating over
+ * snd_soc_pcm_runtime->codec_dais in order to skip codecs that are playback-
+ * or capture-only, which would otherwise inhibit capture or playback,
+ * respectively, to the other codec in a multiple codec / common I2S bus
+ * environment.
+ */
+static bool snd_soc_dai_stream_valid(struct snd_soc_dai *dai, int stream)
+{
+	struct snd_soc_pcm_stream *codec_stream;
+
+	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+		codec_stream = &dai->driver->playback;
+	else
+		codec_stream = &dai->driver->capture;
+
+	/* If the codec specifies any rate at all, it supports the stream. */
+	return !!codec_stream->rates;
+}
+
+/**
  * snd_soc_runtime_activate() - Increment active count for PCM runtime components
  * @rtd: ASoC PCM runtime that is activated
  * @stream: Direction of the PCM stream
@@ -371,6 +397,12 @@  static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream)
 
 	/* first calculate min/max only for CODECs in the DAI link */
 	for (i = 0; i < rtd->num_codecs; i++) {
+
+		/* Skip codecs which don't support the current stream type. */
+		if (!snd_soc_dai_stream_valid(rtd->codec_dais[i],
+					      substream->stream))
+			continue;
+
 		codec_dai_drv = rtd->codec_dais[i]->driver;
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
 			codec_stream = &codec_dai_drv->playback;
@@ -827,6 +859,10 @@  static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		struct snd_soc_dai *codec_dai = rtd->codec_dais[i];
 		struct snd_pcm_hw_params codec_params;
 
+		/* Skip codecs which don't support the current stream type. */
+		if (!snd_soc_dai_stream_valid(codec_dai, substream->stream))
+			continue;
+
 		/* copy params for each codec */
 		codec_params = *params;