[alsa-devel] [PATCH 0/1] ASoC: TWL4030: Constraint setting rework
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(-)
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; }
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.
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.
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).
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.
participants (2)
-
Mark Brown -
Peter Ujfalusi