* [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
@ 2012-01-16 18:41 Mark Brown
2012-01-16 18:41 ` [PATCH 2/2] ASoC: 24 bits are significant on the WM8996 audio interfaces Mark Brown
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Mark Brown @ 2012-01-16 18:41 UTC (permalink / raw)
To: Liam Girdwood, Peter Ujfalusi; +Cc: alsa-devel, patches, Mark Brown
Most devices accept data in formats that don't correspond directly to
their internal format. ALSA allows us to set a msbits constraint which
tells userspace about this in case it finds it useful (for example, in
order to avoid wasting effort dithering bits that will be ignored when
raising the sample size of data) so provide a mechanism for drivers to
specify the number of bits that are actually significant on a DAI and
add the appropriate constraints along with all the others.
This is done slightly awkwardly as the constraint is specified per sample
size - we loop over every possible sample size, including ones that the
device doesn't support and including ones that have fewer bits than are
actually used, but this is harmless as the upper layers do the right thing
in these cases.
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
include/sound/soc.h | 1 +
sound/soc/soc-pcm.c | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 0a56767..346a458 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -519,6 +519,7 @@ struct snd_soc_pcm_stream {
unsigned int rate_max; /* max rate */
unsigned int channels_min; /* min channels */
unsigned int channels_max; /* max channels */
+ unsigned int sig_bits; /* number of bits of content */
};
/* SoC audio ops */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index cdc860a..8bb1793 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -63,6 +63,39 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
}
/*
+ * List of sample sizes that might go over the bus for parameter
+ * application. There ought to be a wildcard sample size for things
+ * like the DAC/ADC resolution to use but there isn't right now.
+ */
+static int sample_sizes[] = {
+ 8, 16, 24, 32,
+};
+
+static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ int ret, i, bits;
+
+ if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+ bits = dai->driver->playback.sig_bits;
+ else
+ bits = dai->driver->capture.sig_bits;
+
+ 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);
+ if (ret != 0)
+ dev_warn(dai->dev,
+ "Failed to set MSB %d/%d: %d\n",
+ bits, sample_sizes[i], ret);
+ }
+}
+
+/*
* Called by ALSA when a PCM substream is opened, the runtime->hw record is
* then initialized and any private data can be allocated. This also calls
* startup for the cpu DAI, platform, machine and codec DAI.
@@ -187,6 +220,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
goto config_err;
}
+ soc_pcm_apply_msb(substream, codec_dai);
+ soc_pcm_apply_msb(substream, cpu_dai);
+
/* Symmetry only applies if we've already got an active stream. */
if (cpu_dai->active) {
ret = soc_pcm_apply_symmetry(substream, cpu_dai);
--
1.7.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] ASoC: 24 bits are significant on the WM8996 audio interfaces
2012-01-16 18:41 [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Mark Brown
@ 2012-01-16 18:41 ` Mark Brown
2012-01-16 22:26 ` [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Girdwood, Liam
2012-01-17 8:55 ` Peter Ujfalusi
2 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2012-01-16 18:41 UTC (permalink / raw)
To: Liam Girdwood, Peter Ujfalusi; +Cc: alsa-devel, patches, Mark Brown
Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
sound/soc/codecs/wm8996.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/wm8996.c b/sound/soc/codecs/wm8996.c
index d8da10f..691ae4e 100644
--- a/sound/soc/codecs/wm8996.c
+++ b/sound/soc/codecs/wm8996.c
@@ -3082,6 +3082,7 @@ static struct snd_soc_dai_driver wm8996_dai[] = {
.channels_max = 6,
.rates = WM8996_RATES,
.formats = WM8996_FORMATS,
+ .sig_bits = 24,
},
.capture = {
.stream_name = "AIF1 Capture",
@@ -3089,6 +3090,7 @@ static struct snd_soc_dai_driver wm8996_dai[] = {
.channels_max = 6,
.rates = WM8996_RATES,
.formats = WM8996_FORMATS,
+ .sig_bits = 24,
},
.ops = &wm8996_dai_ops,
},
@@ -3100,6 +3102,7 @@ static struct snd_soc_dai_driver wm8996_dai[] = {
.channels_max = 2,
.rates = WM8996_RATES,
.formats = WM8996_FORMATS,
+ .sig_bits = 24,
},
.capture = {
.stream_name = "AIF2 Capture",
@@ -3107,6 +3110,7 @@ static struct snd_soc_dai_driver wm8996_dai[] = {
.channels_max = 2,
.rates = WM8996_RATES,
.formats = WM8996_FORMATS,
+ .sig_bits = 24,
},
.ops = &wm8996_dai_ops,
},
--
1.7.7.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-16 18:41 [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Mark Brown
2012-01-16 18:41 ` [PATCH 2/2] ASoC: 24 bits are significant on the WM8996 audio interfaces Mark Brown
@ 2012-01-16 22:26 ` Girdwood, Liam
2012-01-16 22:42 ` Mark Brown
2012-01-17 8:55 ` Peter Ujfalusi
2 siblings, 1 reply; 17+ messages in thread
From: Girdwood, Liam @ 2012-01-16 22:26 UTC (permalink / raw)
To: Mark Brown; +Cc: Peter Ujfalusi, alsa-devel, patches
On 16 January 2012 18:41, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Most devices accept data in formats that don't correspond directly to
> their internal format. ALSA allows us to set a msbits constraint which
> tells userspace about this in case it finds it useful (for example, in
> order to avoid wasting effort dithering bits that will be ignored when
> raising the sample size of data) so provide a mechanism for drivers to
> specify the number of bits that are actually significant on a DAI and
> add the appropriate constraints along with all the others.
>
> This is done slightly awkwardly as the constraint is specified per sample
> size - we loop over every possible sample size, including ones that the
> device doesn't support and including ones that have fewer bits than are
> actually used, but this is harmless as the upper layers do the right thing
> in these cases.
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
Acked-by: Liam Girdwood <lrg@ti.com>
Btw, will you be reworking Peter's patch ?
Liam
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-16 22:26 ` [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Girdwood, Liam
@ 2012-01-16 22:42 ` Mark Brown
2012-01-17 8:56 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-16 22:42 UTC (permalink / raw)
To: Girdwood, Liam; +Cc: Peter Ujfalusi, alsa-devel, patches
On Mon, Jan 16, 2012 at 10:26:11PM +0000, Girdwood, Liam wrote:
> Btw, will you be reworking Peter's patch ?
Probably if he doesn't get around to it, though I figured that the patch
is so tiny it's probably going to be less work to just write it when
testing rather than to apply a patch from mail so it'd be easier for him
to let him do it.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-16 18:41 [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Mark Brown
2012-01-16 18:41 ` [PATCH 2/2] ASoC: 24 bits are significant on the WM8996 audio interfaces Mark Brown
2012-01-16 22:26 ` [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Girdwood, Liam
@ 2012-01-17 8:55 ` Peter Ujfalusi
2012-01-17 11:38 ` Mark Brown
2 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 8:55 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/16/2012 07:41 PM, Mark Brown wrote:
> /*
> + * List of sample sizes that might go over the bus for parameter
> + * application. There ought to be a wildcard sample size for things
> + * like the DAC/ADC resolution to use but there isn't right now.
> + */
> +static int sample_sizes[] = {
> + 8, 16, 24, 32,
> +};
> +
> +static void soc_pcm_apply_msb(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + int ret, i, bits;
> +
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + bits = dai->driver->playback.sig_bits;
> + else
> + bits = dai->driver->capture.sig_bits;
> +
> + 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);
Should we apply the constraint only if the sample size is bigger than
the msbit request:
if (sample_sizes[i] > bits) {
ret = snd_pcm_hw_constraint_msbits();
}
Might be not an issue to say that we have 24msbit on the 8bit sample,
but it does not sound right.
> + if (ret != 0)
> + dev_warn(dai->dev,
> + "Failed to set MSB %d/%d: %d\n",
> + bits, sample_sizes[i], ret);
> + }
> +}
> +
> +/*
> * Called by ALSA when a PCM substream is opened, the runtime->hw record is
> * then initialized and any private data can be allocated. This also calls
> * startup for the cpu DAI, platform, machine and codec DAI.
> @@ -187,6 +220,9 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
> goto config_err;
> }
>
> + soc_pcm_apply_msb(substream, codec_dai);
> + soc_pcm_apply_msb(substream, cpu_dai);
> +
> /* Symmetry only applies if we've already got an active stream. */
> if (cpu_dai->active) {
> ret = soc_pcm_apply_symmetry(substream, cpu_dai);
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-16 22:42 ` Mark Brown
@ 2012-01-17 8:56 ` Peter Ujfalusi
0 siblings, 0 replies; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 8:56 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Girdwood, Liam
On 01/16/2012 11:42 PM, Mark Brown wrote:
> On Mon, Jan 16, 2012 at 10:26:11PM +0000, Girdwood, Liam wrote:
>
>> Btw, will you be reworking Peter's patch ?
>
> Probably if he doesn't get around to it, though I figured that the patch
> is so tiny it's probably going to be less work to just write it when
> testing rather than to apply a patch from mail so it'd be easier for him
> to let him do it.
I'll update the drivers (cpu dais, and codecs I know have this feature).
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 8:55 ` Peter Ujfalusi
@ 2012-01-17 11:38 ` Mark Brown
2012-01-17 13:06 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-17 11:38 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 09:55:23AM +0100, Peter Ujfalusi wrote:
Peter, I shouldn't need to have to remind you to delete unneeded context
from your replies.
> Should we apply the constraint only if the sample size is bigger than
> the msbit request:
> if (sample_sizes[i] > bits) {
> ret = snd_pcm_hw_constraint_msbits();
> }
> Might be not an issue to say that we have 24msbit on the 8bit sample,
> but it does not sound right.
It shouldn't hurt and it is potentially useful to the application to
know that things will be converted up by the hardware; unless there's a
great reason to do so I'd rather not hide the information.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 11:38 ` Mark Brown
@ 2012-01-17 13:06 ` Peter Ujfalusi
2012-01-17 13:19 ` Mark Brown
0 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 13:06 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/17/2012 12:38 PM, Mark Brown wrote:
>> Might be not an issue to say that we have 24msbit on the 8bit sample,
>> but it does not sound right.
>
> It shouldn't hurt and it is potentially useful to the application to
> know that things will be converted up by the hardware; unless there's a
> great reason to do so I'd rather not hide the information.
I can only speak in behalf of OMAP, twl4030, tlv320dac33 here, but the
24msbit only applies to 32bit samples. In case of 16 bit the samples are
not converted in any way, they are processed as 16 bit data.
So if we say 24msbit for 16bit sample we are not correct. It is correct
for 32bit sample.
I would think most of the codecs/cpus are working in a same way.
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 13:06 ` Peter Ujfalusi
@ 2012-01-17 13:19 ` Mark Brown
2012-01-17 14:18 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-17 13:19 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 02:06:44PM +0100, Peter Ujfalusi wrote:
> I can only speak in behalf of OMAP, twl4030, tlv320dac33 here, but the
> 24msbit only applies to 32bit samples. In case of 16 bit the samples are
> not converted in any way, they are processed as 16 bit data.
> So if we say 24msbit for 16bit sample we are not correct. It is correct
> for 32bit sample.
> I would think most of the codecs/cpus are working in a same way.
For the CODECs if you look at what they're doing you'll probably find
that the device is actually operating at a fixed sample size internally
and converting the data somehow at the interface (zero extension being
one option when converting up, but more fancy approaches are also
possible). This is fairly obvious when you think about how things are
likely to be implemented in hardware, it's going to increase complexity
a lot to be able to genuinely switch the entire chip from one sample
size to another.
On the CPU side specifying significant bits would normally only be
appropriate on PDM interfaces as they have most of a DAC or ADC in them
to move between the sampled and PDM formats. I'd be surprised to see
anything else setting these flags, most of the hardware is just passing
the data straight through.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 13:19 ` Mark Brown
@ 2012-01-17 14:18 ` Peter Ujfalusi
2012-01-17 14:56 ` Mark Brown
0 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 14:18 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/17/2012 02:19 PM, Mark Brown wrote:
> For the CODECs if you look at what they're doing you'll probably find
> that the device is actually operating at a fixed sample size internally
> and converting the data somehow at the interface (zero extension being
> one option when converting up, but more fancy approaches are also
> possible). This is fairly obvious when you think about how things are
> likely to be implemented in hardware, it's going to increase complexity
> a lot to be able to genuinely switch the entire chip from one sample
> size to another.
It is mostly true. DAC33 can be configured to operate internally 16bit
or 24msbit/32bit for example.
But none of this matters really for application. If the codec works
internally with 32/64 or whatever, it does not matter for the
application. What matters is that if it sends data in 32bit only 24msb
will be actually going to be taken, and the rest will be ignored at the
interface level. What happens within the codec is out of the scope.
Applications like PulseAudio can do digital volume control. For them it
is important to know if they can operate on the full 32 bit, or only the
24msbit will be taken into account by the HW at interface level.
It does not give any useful information for application that the codec
will upsample the 16bit data internally to 24 bits. It does not really
matter for them since all 16bit data will be used by the codec.
> On the CPU side specifying significant bits would normally only be
> appropriate on PDM interfaces as they have most of a DAC or ADC in them
> to move between the sampled and PDM formats. I'd be surprised to see
> anything else setting these flags, most of the hardware is just passing
> the data straight through.
True, the CPU side mostly passes the data as it is, it does not care
about msbits. For McPDM it is different since the internal FIFO in 24bit
long word lines, so if application would use all 32bit we it will loose
8bit lsb. This can make difference for PA when applying the digital gain
in SW.
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 14:18 ` Peter Ujfalusi
@ 2012-01-17 14:56 ` Mark Brown
2012-01-17 16:08 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-17 14:56 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 03:18:35PM +0100, Peter Ujfalusi wrote:
> It is mostly true. DAC33 can be configured to operate internally 16bit
> or 24msbit/32bit for example.
Well, if it's doing something more complicated that doesn't fit in the
framework then it shouldn't be doing that.
> It does not give any useful information for application that the codec
> will upsample the 16bit data internally to 24 bits. It does not really
> matter for them since all 16bit data will be used by the codec.
Oh, I dunno - I'm sure someone could think of a use for it.
> > On the CPU side specifying significant bits would normally only be
> > appropriate on PDM interfaces as they have most of a DAC or ADC in them
> > to move between the sampled and PDM formats. I'd be surprised to see
> > anything else setting these flags, most of the hardware is just passing
> > the data straight through.
> True, the CPU side mostly passes the data as it is, it does not care
> about msbits. For McPDM it is different since the internal FIFO in 24bit
> long word lines, so if application would use all 32bit we it will loose
Right, like I say that's because it's got most of a DAC in it.
> 8bit lsb. This can make difference for PA when applying the digital gain
> in SW.
Well, it saves it a bit of effort but that's about it.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 14:56 ` Mark Brown
@ 2012-01-17 16:08 ` Peter Ujfalusi
2012-01-17 16:44 ` Mark Brown
0 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 16:08 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/17/2012 03:56 PM, Mark Brown wrote:
> On Tue, Jan 17, 2012 at 03:18:35PM +0100, Peter Ujfalusi wrote:
>
>> It is mostly true. DAC33 can be configured to operate internally 16bit
>> or 24msbit/32bit for example.
>
> Well, if it's doing something more complicated that doesn't fit in the
> framework then it shouldn't be doing that.
What do you mean? If user plays 16bit audio we configure the codec in
16bit mode. If it is opened in 24msbit/32 mode it is configured accordingly.
>> It does not give any useful information for application that the codec
>> will upsample the 16bit data internally to 24 bits. It does not really
>> matter for them since all 16bit data will be used by the codec.
>
> Oh, I dunno - I'm sure someone could think of a use for it.
Sure it might be good to know, but it is something we do not have
control over. There's a datasheet if someone is interested.
Even if you could select the algorithm for the in HW resampling it could
be configured via kcontrol, or via qos.
For application it does not matter.
>> True, the CPU side mostly passes the data as it is, it does not care
>> about msbits. For McPDM it is different since the internal FIFO in 24bit
>> long word lines, so if application would use all 32bit we it will loose
>
> Right, like I say that's because it's got most of a DAC in it.
The McPDM does not have codec, the internal FIFO has this layout which
dictates the 24msbit. It just cuts the 8lsb.
>
>> 8bit lsb. This can make difference for PA when applying the digital gain
>> in SW.
>
> Well, it saves it a bit of effort but that's about it.
This is not a point. If it does it's internal digital volume on the full
32bit resolution from which the HW just discards 8bits we will loose
resolution. If PA knows that out of the 32bit only the 24bit msbit is
going to be used it can apply the gain correctly.
If we tell PA that the codec internally works in 24bit, and we play
16bit audio (in 16bit mode) PA needs to apply the gain in 16bit
resolution. The information about the codec working internally in 24bit
irrelevant.
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 16:08 ` Peter Ujfalusi
@ 2012-01-17 16:44 ` Mark Brown
2012-01-17 17:55 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-17 16:44 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 05:08:02PM +0100, Peter Ujfalusi wrote:
> On 01/17/2012 03:56 PM, Mark Brown wrote:
> > Well, if it's doing something more complicated that doesn't fit in the
> > framework then it shouldn't be doing that.
> What do you mean? If user plays 16bit audio we configure the codec in
> 16bit mode. If it is opened in 24msbit/32 mode it is configured accordingly.
The framework feature is much simpler than that, it just supports a
fixed number of bits that the device uses internally regardless of what
the bus carries. If the hardware actually does something substantial
internally then it doesn't really fit in with that.
> >> It does not give any useful information for application that the codec
> >> will upsample the 16bit data internally to 24 bits. It does not really
> >> matter for them since all 16bit data will be used by the codec.
> > Oh, I dunno - I'm sure someone could think of a use for it.
> Sure it might be good to know, but it is something we do not have
> control over. There's a datasheet if someone is interested.
The datasheet isn't terribly machine parseable.
> Even if you could select the algorithm for the in HW resampling it could
> be configured via kcontrol, or via qos.
> For application it does not matter.
I don't recall suggesting configuring the hardware algorithm here?
> > Right, like I say that's because it's got most of a DAC in it.
> The McPDM does not have codec, the internal FIFO has this layout which
> dictates the 24msbit. It just cuts the 8lsb.
Look at what a PDM output actually does compared to a sampled interface
and compare that to what a CODEC is doing - an awful lot of devices are
do the actual D/A and A/D conversions on an oversampled bitstream which
is what a PDM output is, generating that from the samples at some point
in the chain.
> >> 8bit lsb. This can make difference for PA when applying the digital gain
> >> in SW.
> > Well, it saves it a bit of effort but that's about it.
> This is not a point. If it does it's internal digital volume on the full
> 32bit resolution from which the HW just discards 8bits we will loose
> resolution. If PA knows that out of the 32bit only the 24bit msbit is
> going to be used it can apply the gain correctly.
This isn't a correctness thing really, it's just that it's doing more
work than it needs to because nothing is going to pay attention to the
the lower bits it outputs. A gain applied with 24 bits just has an
output with a bit less resolution than one applied with 32 bits but it
shouldn't be substantially different.
> If we tell PA that the codec internally works in 24bit, and we play
> 16bit audio (in 16bit mode) PA needs to apply the gain in 16bit
> resolution. The information about the codec working internally in 24bit
> irrelevant.
I can't think why Pulse might want to use it. On the other hand it's
not going to hurt to tell it.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 16:44 ` Mark Brown
@ 2012-01-17 17:55 ` Peter Ujfalusi
2012-01-17 18:17 ` Mark Brown
0 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 17:55 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/17/2012 05:44 PM, Mark Brown wrote:
>> Sure it might be good to know, but it is something we do not have
>> control over. There's a datasheet if someone is interested.
>
> The datasheet isn't terribly machine parseable.
True, and the information that the chip internally works in 24 bit mode
is irrelevant. Why would any application care when it plays 16bit sample
that the HW will internally convert it to 24 bit?
>> Even if you could select the algorithm for the in HW resampling it could
>> be configured via kcontrol, or via qos.
>> For application it does not matter.
>
> I don't recall suggesting configuring the hardware algorithm here?
For what other reason application would use the fact that the 16bit
sample will converted to 24bit by the HW other that you might want to
influence the algorithm used by the HW when it does it?
>> This is not a point. If it does it's internal digital volume on the full
>> 32bit resolution from which the HW just discards 8bits we will loose
>> resolution. If PA knows that out of the 32bit only the 24bit msbit is
>> going to be used it can apply the gain correctly.
>
> This isn't a correctness thing really, it's just that it's doing more
> work than it needs to because nothing is going to pay attention to the
> the lower bits it outputs. A gain applied with 24 bits just has an
> output with a bit less resolution than one applied with 32 bits but it
> shouldn't be substantially different.
Yeah, but it is not correct. If it does not know this we have 8bit
'latency' in gain control. Pulse can change the gain which will have no
effect.
But I'm not arguing against the constraint on the 32bit sample for
24msbits...
>> If we tell PA that the codec internally works in 24bit, and we play
>> 16bit audio (in 16bit mode) PA needs to apply the gain in 16bit
>> resolution. The information about the codec working internally in 24bit
>> irrelevant.
>
> I can't think why Pulse might want to use it. On the other hand it's
> not going to hurt to tell it.
If you look at this:
Its setup is:
stream : PLAYBACK
access : RW_INTERLEAVED
format : S16_LE
subformat : STD
channels : 2
rate : 48000
exact rate : 48000 (48000/1)
msbits : 24
buffer_size : 24000
period_size : 6000
period_time : 125000
It just does not feel right.
What if application takes this literally, goes and applies the digital
gain on a 16bit sample with 24bit resolution?
I know the application need to be fixed, but no application expect to be
told that out of the 16bit they can use 24bit..
I'm not convinced that this is a good idea. We should apply the
constraints on the sample size where it actually make sense.
IMHO.
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 17:55 ` Peter Ujfalusi
@ 2012-01-17 18:17 ` Mark Brown
2012-01-17 18:51 ` Peter Ujfalusi
0 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2012-01-17 18:17 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 06:55:29PM +0100, Peter Ujfalusi wrote:
> On 01/17/2012 05:44 PM, Mark Brown wrote:
> > The datasheet isn't terribly machine parseable.
> True, and the information that the chip internally works in 24 bit mode
> is irrelevant. Why would any application care when it plays 16bit sample
> that the HW will internally convert it to 24 bit?
Off the top of my head it may decide that if it happens to have 24 bit
data then passing it straight through was sensible. But frankly I'm
just working on the basis that if it's more effort to hide information
than to provide it then providing it seems like the way forwards. I've
certainly got no intention of writing any code here myself unless
there's some issue found.
> >> This is not a point. If it does it's internal digital volume on the full
> >> 32bit resolution from which the HW just discards 8bits we will loose
> >> resolution. If PA knows that out of the 32bit only the 24bit msbit is
> >> going to be used it can apply the gain correctly.
> > This isn't a correctness thing really, it's just that it's doing more
> > work than it needs to because nothing is going to pay attention to the
> > the lower bits it outputs. A gain applied with 24 bits just has an
> > output with a bit less resolution than one applied with 32 bits but it
> > shouldn't be substantially different.
> Yeah, but it is not correct. If it does not know this we have 8bit
> 'latency' in gain control. Pulse can change the gain which will have no
> effect.
Which will have the same overall effect as if it doesn't do anything
based on knowing the resolution.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 18:17 ` Mark Brown
@ 2012-01-17 18:51 ` Peter Ujfalusi
2012-01-17 18:59 ` Mark Brown
0 siblings, 1 reply; 17+ messages in thread
From: Peter Ujfalusi @ 2012-01-17 18:51 UTC (permalink / raw)
To: Mark Brown; +Cc: alsa-devel, patches, Liam Girdwood
On 01/17/2012 07:17 PM, Mark Brown wrote:
> Off the top of my head it may decide that if it happens to have 24 bit
> data then passing it straight through was sensible. But frankly I'm
> just working on the basis that if it's more effort to hide information
> than to provide it then providing it seems like the way forwards. I've
> certainly got no intention of writing any code here myself unless
> there's some issue found.
We have the sample formats (S16_LE, S32_LE, etc) to tell application
about the supported level of bit depth.
They can choose to use 16bit if that is better for them, or choose
32bit. They make the decision based on the sample is playing,
configuration, whatever.
It does not help them if we tell when they use 16bit audio: hey, but you
could use 24bit.
When they use 32bit on the other hand it make sense to let them know
that out of the 32bit only the 24msb will be used, so they can align
their processing accordingly (if they care).
>> Yeah, but it is not correct. If it does not know this we have 8bit
>> 'latency' in gain control. Pulse can change the gain which will have no
>> effect.
>
> Which will have the same overall effect as if it doesn't do anything
> based on knowing the resolution.
But we still 'loose' 8bits. It might be not a big loss at the end when
PA for example applies the gain, but for sure we will loose resolution
(8bit worth).
My only problem is to say this to application: "out of 8/16bit you
should use 24msb". AFAIK this is the meaning of the constraint. This
constraint makes sense for 32bit samples: "out of 32bit you should use
24msb".
--
Péter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI
2012-01-17 18:51 ` Peter Ujfalusi
@ 2012-01-17 18:59 ` Mark Brown
0 siblings, 0 replies; 17+ messages in thread
From: Mark Brown @ 2012-01-17 18:59 UTC (permalink / raw)
To: Peter Ujfalusi; +Cc: alsa-devel, patches, Liam Girdwood
On Tue, Jan 17, 2012 at 07:51:29PM +0100, Peter Ujfalusi wrote:
> >> Yeah, but it is not correct. If it does not know this we have 8bit
> >> 'latency' in gain control. Pulse can change the gain which will have no
> >> effect.
> > Which will have the same overall effect as if it doesn't do anything
> > based on knowing the resolution.
> But we still 'loose' 8bits. It might be not a big loss at the end when
> PA for example applies the gain, but for sure we will loose resolution
> (8bit worth).
Whereas if you do the gain to the lower resolution you'll discard the
same amount of information...
> My only problem is to say this to application: "out of 8/16bit you
> should use 24msb". AFAIK this is the meaning of the constraint. This
> constraint makes sense for 32bit samples: "out of 32bit you should use
> 24msb".
Well, like I say I don't see this as a problem and have no intention of
writing any code to handle that myself.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-01-17 18:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-16 18:41 [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Mark Brown
2012-01-16 18:41 ` [PATCH 2/2] ASoC: 24 bits are significant on the WM8996 audio interfaces Mark Brown
2012-01-16 22:26 ` [PATCH 1/2] ASoC: Allow drivers to specify how many bits are significant on a DAI Girdwood, Liam
2012-01-16 22:42 ` Mark Brown
2012-01-17 8:56 ` Peter Ujfalusi
2012-01-17 8:55 ` Peter Ujfalusi
2012-01-17 11:38 ` Mark Brown
2012-01-17 13:06 ` Peter Ujfalusi
2012-01-17 13:19 ` Mark Brown
2012-01-17 14:18 ` Peter Ujfalusi
2012-01-17 14:56 ` Mark Brown
2012-01-17 16:08 ` Peter Ujfalusi
2012-01-17 16:44 ` Mark Brown
2012-01-17 17:55 ` Peter Ujfalusi
2012-01-17 18:17 ` Mark Brown
2012-01-17 18:51 ` Peter Ujfalusi
2012-01-17 18:59 ` Mark Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).