[alsa-devel] [PATCH v5] ASoC: Apply msbits constraint for sample size bigger than the msbits
In order to avoid confusing the applications with msbits bigger than the selected sample size apply the msbits constraint only to sample seze bigger than the requested msbits.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com ---
Hi Mark,
Converted to use while loop. We run the loop as long as the sample size is bigger than the requested msbits. Most of the drivers require 24/32 configuration, so not point of looping for smaller sample sizes.
Regards, Peter
sound/soc/soc-pcm.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8bb1793..ef41a39 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -68,13 +68,14 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream, * like the DAC/ADC resolution to use but there isn't right now. */ static int sample_sizes[] = { - 8, 16, 24, 32, + 32, 24, 16, 8, };
static void soc_pcm_apply_msb(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { - int ret, i, bits; + int ret, bits; + int i = 0;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) bits = dai->driver->playback.sig_bits; @@ -84,14 +85,14 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream, if (!bits) return;
- for (i = 0; i < ARRAY_SIZE(sample_sizes); i++) { - ret = snd_pcm_hw_constraint_msbits(substream->runtime, - 0, sample_sizes[i], - bits); + /* Apply constraint only for sample size bigger than requested msbits */ + while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) { + ret = snd_pcm_hw_constraint_msbits(substream->runtime, 0, + sample_sizes[i], bits); if (ret != 0) - dev_warn(dai->dev, - "Failed to set MSB %d/%d: %d\n", + dev_warn(dai->dev, "Failed to set MSB %d/%d: %d\n", bits, sample_sizes[i], ret); + i++; } }
On Wed, Jan 18, 2012 at 04:05:14PM +0100, Peter Ujfalusi wrote:
Converted to use while loop. We run the loop as long as the sample size is bigger than the requested msbits.
I'm not sure this is actually a legibility improvement, if anything it's probably less clear than the original as now the setup of the loop is spread even further around the function.
Most of the drivers require 24/32 configuration, so not point of looping for smaller sample sizes.
Performance isn't really a concern in this path unless we do something totally insane. Thinking time on the part of the reader needs to be considered too...
On 01/18/2012 04:29 PM, Mark Brown wrote:
I'm not sure this is actually a legibility improvement, if anything it's probably less clear than the original as now the setup of the loop is spread even further around the function.
Would it make it clearer if I set i to 0 right before the while?
Most of the drivers require 24/32 configuration, so not point of looping for smaller sample sizes.
Performance isn't really a concern in this path unless we do something totally insane. Thinking time on the part of the reader needs to be considered too...
Sure it is not a concern. These small 'Performance isn't really a concern in this path' at the end ads up that we need faster CPUs to have the same perceived perfomrance.
On Wed, Jan 18, 2012 at 05:43:28PM +0100, Peter Ujfalusi wrote:
On 01/18/2012 04:29 PM, Mark Brown wrote:
I'm not sure this is actually a legibility improvement, if anything it's probably less clear than the original as now the setup of the loop is spread even further around the function.
Would it make it clearer if I set i to 0 right before the while?
That'd help a bit. Though I'd just go with a for loop, the while clearly doesn't look any better - I was just suggesting it without actually having tried writing it out.
Performance isn't really a concern in this path unless we do something totally insane. Thinking time on the part of the reader needs to be considered too...
Sure it is not a concern. These small 'Performance isn't really a concern in this path' at the end ads up that we need faster CPUs to have the same perceived perfomrance.
Yeah, but if that percieved performance is already instantaneous there's no need to worry :) My first thought would've been to just continue on sample rates we don't like rather than trying to break out of an iteration of 4 steps early.
On 01/18/2012 06:46 PM, Mark Brown wrote:
That'd help a bit. Though I'd just go with a for loop, the while clearly doesn't look any better - I was just suggesting it without actually having tried writing it out.
Are you going to take the for version of patch, or should I resend it? I can add a comment to explain that we bail out, and not checking the sample sizes lower than the requested msbits, if it helps.
On Thu, Jan 19, 2012 at 09:27:33AM +0100, Peter Ujfalusi wrote:
Are you going to take the for version of patch, or should I resend it? I can add a comment to explain that we bail out, and not checking the sample sizes lower than the requested msbits, if it helps.
I'd just replace it with a continue on sizes we don't want, much simpler.
On 01/19/2012 11:48 AM, Mark Brown wrote:
On Thu, Jan 19, 2012 at 09:27:33AM +0100, Peter Ujfalusi wrote:
Are you going to take the for version of patch, or should I resend it? I can add a comment to explain that we bail out, and not checking the sample sizes lower than the requested msbits, if it helps.
I'd just replace it with a continue on sizes we don't want, much simpler.
If I can do this (bits == 24): i = 0 check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) apply constraint i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) go out
-------------------------------------- I will not do this (bits == 24): static int sample_sizes[] = { 8, 16, 24, 32, };
i = 0 check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] > bits) i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] > bits) i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] > bits) i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] > bits) apply constraint i++ check (i < ARRAY_SIZE(sample_sizes)) go out
-------------------------------------- nor this (bits == 24): static int sample_sizes[] = { 32, 24, 16, 8, };
i = 0 check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) apply constraint i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) i++ check (i < ARRAY_SIZE(sample_sizes)) check (sample_sizes[i] <= bits) i++ check (i < ARRAY_SIZE(sample_sizes)) go out
On Thu, Jan 19, 2012 at 01:40:35PM +0100, Peter Ujfalusi wrote:
On 01/19/2012 11:48 AM, Mark Brown wrote:
I'd just replace it with a continue on sizes we don't want, much simpler.
If I can do this (bits == 24):
This mail took me a little while to parse but I think you're trying to say you insist on the microoptimisation. Unless you can come up with a microoptimisation which doesn't take effort to read please write something simple. The reason I wasn't happy with the original patch was that what should be a trivial bit of code took far too long to check. Maintainability is vastly more important than microoptimising a slow path, having to read things again later was the whole reason I didn't do any filtering in the first place.
Peter Ujfalusi wrote:
We run the loop as long as the sample size is bigger than the requested msbits.
- while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
This will overrun sample_sizes[] if bits < 8.
Regards, Clemens
On 01/18/2012 04:35 PM, Clemens Ladisch wrote:
Peter Ujfalusi wrote:
We run the loop as long as the sample size is bigger than the requested msbits.
- while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
This will overrun sample_sizes[] if bits < 8.
which is prevented by the: && i < ARRAY_SIZE(sample_sizes)
On 01/18/2012 04:43 PM, Peter Ujfalusi wrote:
On 01/18/2012 04:35 PM, Clemens Ladisch wrote:
Peter Ujfalusi wrote:
We run the loop as long as the sample size is bigger than the requested msbits.
- while (sample_sizes[i] > bits && i < ARRAY_SIZE(sample_sizes)) {
This will overrun sample_sizes[] if bits < 8.
which is prevented by the: && i < ARRAY_SIZE(sample_sizes)
I take it back. True, we overrun. These need to be swapped.
participants (3)
-
Clemens Ladisch -
Mark Brown -
Peter Ujfalusi