[PATCH 0/1] ASoC: TWL4030: Constraint setting rework

[PATCH 0/1] ASoC: TWL4030: Constraint setting rework

[Peter Ujfalusi]

Hello,

The current implementation of constraints in TWL4030 codec needs some tuning.

An interesting case can be generated by using gstreamer:
gst-launch-0.10 alsasrc device=hw:0 ! alsasink device=hw:0 sync=false

What would (since the current implementation blocks it) happen in this case is:
playback stream: open
capture stream: open
capture stream: hw_params
playback_stream: hw_params

In capture open we set the constraints to the capture stream based on the
playback streams runtime parameters, and they are 0 (rate = 0, sample_bits = 0),
since the hw_param has not been called for that stream yet...

The following patch fixes this issue among other possible scenarios regarding to
constraint handling.

Note that I'm not using the just introduced symmetric_rates feature of ASoC
introduced by Mark Brown recently, since I think that also suffers from this
problem (or not, I have to try it out first to be sure about it)

---
Peter Ujfalusi (1):
  ASoC: TWL4030: Fix for the constraint handling

 sound/soc/codecs/twl4030.c |   85 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 66 insertions(+), 19 deletions(-)

[PATCH 1/1] ASoC: TWL4030: Fix for the constraint handling

[Peter Ujfalusi]

The original implementation of the constraints were good against sane
applications.
If the opening sequence is:
stream1_open, stream1_hw_params, stream2_open, stream2_hw_params -> the
constraints are set correctly for stream2.

But if the sequence is:
stream1_open, stream2_open, stream2_hw_params, stream1_hw_params -> than stream2
would receive constraint rate = 0, sample_bits = 0, since the stream1 has not
yet called hw_params...

The command to trigger this event:
gst-launch-0.10 alsasrc device=hw:0 ! alsasink device=hw:0 sync=false

This patch does some 'black magic' in order to always set the correct
constraints and sets it only when it is needed for the other stream.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
 sound/soc/codecs/twl4030.c |   85 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 66 insertions(+), 19 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 921b205..a1b76d7 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -125,6 +125,11 @@ struct twl4030_priv {
 
 	struct snd_pcm_substream *master_substream;
 	struct snd_pcm_substream *slave_substream;
+
+	unsigned int configured;
+	unsigned int rate;
+	unsigned int sample_bits;
+	unsigned int channels;
 };
 
 /*
@@ -1220,6 +1225,36 @@ static int twl4030_set_bias_level(struct snd_soc_codec *codec,
 	return 0;
 }
 
+static void twl4030_constraints(struct twl4030_priv *twl4030,
+				struct snd_pcm_substream *mst_substream)
+{
+	struct snd_pcm_substream *slv_substream;
+
+	/* Pick the stream, which need to be constrained */
+	if (mst_substream == twl4030->master_substream)
+		slv_substream = twl4030->slave_substream;
+	else if (mst_substream == twl4030->slave_substream)
+		slv_substream = twl4030->master_substream;
+	else /* This should not happen.. */
+		return;
+
+	/* Set the constraints according to the already configured stream */
+	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
+				SNDRV_PCM_HW_PARAM_RATE,
+				twl4030->rate,
+				twl4030->rate);
+
+	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
+				SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
+				twl4030->sample_bits,
+				twl4030->sample_bits);
+
+	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
+				SNDRV_PCM_HW_PARAM_CHANNELS,
+				twl4030->channels,
+				twl4030->channels);
+}
+
 static int twl4030_startup(struct snd_pcm_substream *substream,
 			   struct snd_soc_dai *dai)
 {
@@ -1228,26 +1263,16 @@ static int twl4030_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = socdev->card->codec;
 	struct twl4030_priv *twl4030 = codec->private_data;
 
-	/* If we already have a playback or capture going then constrain
-	 * this substream to match it.
-	 */
 	if (twl4030->master_substream) {
-		struct snd_pcm_runtime *master_runtime;
-		master_runtime = twl4030->master_substream->runtime;
-
-		snd_pcm_hw_constraint_minmax(substream->runtime,
-					     SNDRV_PCM_HW_PARAM_RATE,
-					     master_runtime->rate,
-					     master_runtime->rate);
-
-		snd_pcm_hw_constraint_minmax(substream->runtime,
-					     SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
-					     master_runtime->sample_bits,
-					     master_runtime->sample_bits);
-
 		twl4030->slave_substream = substream;
-	} else
+		/* The DAI has one configuration for playback and capture, so
+		 * if the DAI has been already configured then constrain this
+		 * substream to match it. */
+		if (twl4030->configured)
+			twl4030_constraints(twl4030, twl4030->master_substream);
+	} else {
 		twl4030->master_substream = substream;
+	}
 
 	return 0;
 }
@@ -1264,6 +1289,13 @@ static void twl4030_shutdown(struct snd_pcm_substream *substream,
 		twl4030->master_substream = twl4030->slave_substream;
 
 	twl4030->slave_substream = NULL;
+
+	/* If all streams are closed, or the remaining stream has not yet
+	 * been configured than set the DAI as not configured. */
+	if (!twl4030->master_substream)
+		twl4030->configured = 0;
+	 else if (!twl4030->master_substream->runtime->channels)
+		twl4030->configured = 0;
 }
 
 static int twl4030_hw_params(struct snd_pcm_substream *substream,
@@ -1276,8 +1308,8 @@ static int twl4030_hw_params(struct snd_pcm_substream *substream,
 	struct twl4030_priv *twl4030 = codec->private_data;
 	u8 mode, old_mode, format, old_format;
 
-	if (substream == twl4030->slave_substream)
-		/* Ignoring hw_params for slave substream */
+	if (twl4030->configured)
+		/* Ignoring hw_params for already configured DAI */
 		return 0;
 
 	/* bit rate */
@@ -1357,6 +1389,21 @@ static int twl4030_hw_params(struct snd_pcm_substream *substream,
 		/* set CODECPDZ afterwards */
 		twl4030_codec_enable(codec, 1);
 	}
+
+	/* Store the important parameters for the DAI configuration and set
+	 * the DAI as configured */
+	twl4030->configured = 1;
+	twl4030->rate = params_rate(params);
+	twl4030->sample_bits = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_SAMPLE_BITS)->min;
+	twl4030->channels = params_channels(params);
+
+	/* If both playback and capture streams are open, and one of them
+	 * is setting the hw parameters right now (since we are here), set
+	 * constraints to the other stream to match the current one. */
+	if (twl4030->slave_substream)
+		twl4030_constraints(twl4030, substream);
+
 	return 0;
 }
 
-- 
1.6.2.3

Re: [PATCH 1/1] ASoC: TWL4030: Fix for the constraint handling

[Mark Brown]

On Fri, Apr 17, 2009 at 03:55:08PM +0300, Peter Ujfalusi wrote:

> +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> +				SNDRV_PCM_HW_PARAM_RATE,
> +				twl4030->rate,
> +				twl4030->rate);

You did note this yourself but there's support for this in the core;
it'd be good to switch over to that (and push handling for the other
constraints into core).

> +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> +				SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> +				twl4030->sample_bits,
> +				twl4030->sample_bits);
> +
> +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> +				SNDRV_PCM_HW_PARAM_CHANNELS,
> +				twl4030->channels,
> +				twl4030->channels);

Are you sure that these need to match exactly and don't set maxima?

Your overall approach in the patch looks good so I'll apply it - it's
an improvement on the current situation.

Re: [PATCH 1/1] ASoC: TWL4030: Fix for the constraint handling

[Peter Ujfalusi]

On Friday 17 April 2009 17:26:42 ext Mark Brown wrote:
> On Fri, Apr 17, 2009 at 03:55:08PM +0300, Peter Ujfalusi wrote:
> > +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> > +				SNDRV_PCM_HW_PARAM_RATE,
> > +				twl4030->rate,
> > +				twl4030->rate);
>
> You did note this yourself but there's support for this in the core;
> it'd be good to switch over to that (and push handling for the other
> constraints into core).

Yes it would be better to move these constraints handling to the core over 
time. As I explained it in the introduction mail, I think that the way the 
core handles the constraint for the rate is not without issues (you also noted 
that).
I have been thinking of implementing this in core in a similar manner as I 
have implemented in the twl4030 codec, but I'm not sure that the assumption of 
having a maximum of two streams (one for playback and one for capture) holds 
for all codec/machine pairs. If it does, I can improve the core's constraint 
handling with the approach taken in the twl4030 codec. With my limited tests 
it passed all cases that I could come up...

>
> > +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> > +				SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> > +				twl4030->sample_bits,
> > +				twl4030->sample_bits);
> > +
> > +	snd_pcm_hw_constraint_minmax(slv_substream->runtime,
> > +				SNDRV_PCM_HW_PARAM_CHANNELS,
> > +				twl4030->channels,
> > +				twl4030->channels);
>
> Are you sure that these need to match exactly and don't set maxima?

These settings are for the audio interface (HiFi interface), which means they 
are changing the interface format for both playback and capture. On top of 
that in 2 channel mode the interface has to be in I2S mode, in 4 channel mode 
it has to be in DSP_A mode. So I think they have to be exact match in order to 
not to break the existing stream.

>
> Your overall approach in the patch looks good so I'll apply it - it's
> an improvement on the current situation.

-- 
Péter

Re: [PATCH 1/1] ASoC: TWL4030: Fix for the constraint handling

[Mark Brown]

On Mon, Apr 20, 2009 at 10:23:30AM +0300, Peter Ujfalusi wrote:

> I have been thinking of implementing this in core in a similar manner as I 
> have implemented in the twl4030 codec, but I'm not sure that the assumption of 
> having a maximum of two streams (one for playback and one for capture) holds 
> for all codec/machine pairs. If it does, I can improve the core's constraint 
> handling with the approach taken in the twl4030 codec. With my limited tests 
> it passed all cases that I could come up...

The single streams are assumed in the ASoC core so there's not much
concern there.  Without working on the ALSA core itself there's not much
you can do to fully plug all the holes, though.

> > > +				SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
> > > +				SNDRV_PCM_HW_PARAM_CHANNELS,

> > Are you sure that these need to match exactly and don't set maxima?

> These settings are for the audio interface (HiFi interface), which means they 
> are changing the interface format for both playback and capture. On top of 
> that in 2 channel mode the interface has to be in I2S mode, in 4 channel mode 
> it has to be in DSP_A mode. So I think they have to be exact match in order to 
> not to break the existing stream.

Oh, nice.  With some care it should be possible to allow at least a
maximum bit rate setting (since lower bit rates are going to be wire
compatible if enough clocks are generated).

Re: [PATCH 0/1] ASoC: TWL4030: Constraint setting rework

[Mark Brown]

On Fri, Apr 17, 2009 at 03:55:07PM +0300, Peter Ujfalusi wrote:

> Note that I'm not using the just introduced symmetric_rates feature of ASoC
> introduced by Mark Brown recently, since I think that also suffers from this
> problem (or not, I have to try it out first to be sure about it)

Yes, pretty much all the code to handle this sort of issue has that
problem in some form.  There's issues with the constraints not getting
seen in time - people had a look at it when we started doing constraints
like this properly.

The symmetric rate setting tries to cover it a bit by enforcing the last
constraint that was seen so you only get the zero problem on first open.
For a lot of applications this actually ends up doing the right thing,
though obviously it's not ideal.