[alsa-devel] [PATCH 0/3] ASoC: sti: correction for HBRA support
This patchset is V2 of "[PATCH 2/2] ASoC: sti: correction for HBRA support"
Moise Gergaud (3): ALSA: pcm: add IEC958 channel status update ASoC: sti: use pcm iec958 channel status helper ASoC: sti: correction for HBRA (High Bit Rate Audio) support
include/sound/pcm_iec958.h | 19 ++++++++- sound/core/pcm_iec958.c | 78 +++++++++++++++++++++++++------------ sound/soc/sti/Kconfig | 1 + sound/soc/sti/uniperif_player.c | 86 +++++++++++++++++------------------------ 4 files changed, 107 insertions(+), 77 deletions(-)
Add a helper to update only the IEC958 channel status sampling freq and word length parameters from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- include/sound/pcm_iec958.h | 19 ++++++++++- sound/core/pcm_iec958.c | 78 +++++++++++++++++++++++++++++++--------------- 2 files changed, 71 insertions(+), 26 deletions(-)
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..0c84c69 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -3,7 +3,24 @@
#include <linux/types.h>
+#ifdef CONFIG_SND_PCM_IEC958 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, - size_t len); + size_t len); + +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len); +#else +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len) +{ + return len; +} + +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len) +{ + return len; +} +#endif
#endif diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c index 36b2d7a..abe3967 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -11,67 +11,72 @@ #include <sound/pcm.h> #include <sound/pcm_iec958.h>
-/** - * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status +/* + * snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status * @runtime: pcm runtime structure with ->rate filled in * @cs: channel status buffer, at least four bytes * @len: length of channel status buffer * - * Create the consumer format channel status data in @cs of maximum size - * @len corresponding to the parameters of the PCM runtime @runtime. - * - * Drivers may wish to tweak the contents of the buffer after creation. + * Update sampling frequency and word length parameters of the consumer format + * channel status data in @cs of maximum size @len. + * Values correspond to the rate and format parameters of the PCM runtime + * @runtime. * * Returns: length of buffer, or negative error code if something failed. */ -int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, - size_t len) +int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len) { - unsigned int fs, ws; - if (len < 4) return -EINVAL;
+ cs[3] &= ~IEC958_AES3_CON_FS; + switch (runtime->rate) { + case 22050: + cs[3] |= IEC958_AES3_CON_FS_22050; + break; case 32000: - fs = IEC958_AES3_CON_FS_32000; + cs[3] |= IEC958_AES3_CON_FS_32000; break; case 44100: - fs = IEC958_AES3_CON_FS_44100; + cs[3] |= IEC958_AES3_CON_FS_44100; break; case 48000: - fs = IEC958_AES3_CON_FS_48000; + cs[3] |= IEC958_AES3_CON_FS_48000; break; case 88200: - fs = IEC958_AES3_CON_FS_88200; + cs[3] |= IEC958_AES3_CON_FS_88200; break; case 96000: - fs = IEC958_AES3_CON_FS_96000; + cs[3] |= IEC958_AES3_CON_FS_96000; break; case 176400: - fs = IEC958_AES3_CON_FS_176400; + cs[3] |= IEC958_AES3_CON_FS_176400; break; case 192000: - fs = IEC958_AES3_CON_FS_192000; + cs[3] |= IEC958_AES3_CON_FS_192000; break; default: return -EINVAL; }
if (len > 4) { + cs[4] &= ~IEC958_AES4_CON_WORDLEN; + switch (snd_pcm_format_width(runtime->format)) { case 16: - ws = IEC958_AES4_CON_WORDLEN_20_16; + cs[4] |= IEC958_AES4_CON_WORDLEN_20_16; break; case 18: - ws = IEC958_AES4_CON_WORDLEN_22_18; + cs[4] |= IEC958_AES4_CON_WORDLEN_22_18; break; case 20: - ws = IEC958_AES4_CON_WORDLEN_20_16 | + cs[4] |= IEC958_AES4_CON_WORDLEN_20_16 | IEC958_AES4_CON_MAX_WORDLEN_24; break; case 24: - ws = IEC958_AES4_CON_WORDLEN_24_20 | + cs[4] |= IEC958_AES4_CON_WORDLEN_24_20 | IEC958_AES4_CON_MAX_WORDLEN_24; break;
@@ -80,15 +85,38 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, } }
+ return len; +} +EXPORT_SYMBOL(snd_pcm_update_iec958_consumer); + +/** + * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status + * @runtime: pcm runtime structure with ->rate filled in + * @cs: channel status buffer, at least four bytes + * @len: length of channel status buffer + * + * Create the consumer format channel status data in @cs of maximum size + * @len corresponding to the parameters of the PCM runtime @runtime. + * + * Drivers may wish to tweak the contents of the buffer after creation. + * + * Returns: length of buffer, or negative error code if something failed. + */ +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, + size_t len) +{ + int ret; + memset(cs, 0, len);
+ ret = snd_pcm_update_iec958_consumer(runtime, cs, len); + if (ret < 0) + return ret; + cs[0] = IEC958_AES0_CON_NOT_COPYRIGHT | IEC958_AES0_CON_EMPHASIS_NONE; cs[1] = IEC958_AES1_CON_GENERAL; cs[2] = IEC958_AES2_CON_SOURCE_UNSPEC | IEC958_AES2_CON_CHANNEL_UNSPEC; - cs[3] = IEC958_AES3_CON_CLOCK_1000PPM | fs; - - if (len > 4) - cs[4] = ws; + cs[3] |= IEC958_AES3_CON_CLOCK_1000PPM;
return len; }
On Wed, 02 Dec 2015 15:22:04 +0100, Moise Gergaud wrote:
Add a helper to update only the IEC958 channel status sampling freq and word length parameters from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com
include/sound/pcm_iec958.h | 19 ++++++++++- sound/core/pcm_iec958.c | 78 +++++++++++++++++++++++++++++++--------------- 2 files changed, 71 insertions(+), 26 deletions(-)
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..0c84c69 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -3,7 +3,24 @@
#include <linux/types.h>
+#ifdef CONFIG_SND_PCM_IEC958 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
- size_t len);
size_t len);+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len);+#else +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
These must be static inline. And, shouldn't they return an error?
Takashi
On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
Add a helper to update only the IEC958 channel status sampling freq and word length parameters from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com
NAK.
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..0c84c69 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -3,7 +3,24 @@
#include <linux/types.h>
+#ifdef CONFIG_SND_PCM_IEC958 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
- size_t len);
size_t len);+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len);+#else +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
Firstly, this is dangerous. If you don't have anything defined here, then the channel status ends up being undefined.
The intention here is that if you're a user of this, then select the config symbol. That makes providing stubs totally unnecessary and removes this problem.
+#endif
#endif diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c index 36b2d7a..abe3967 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -11,67 +11,72 @@ #include <sound/pcm.h> #include <sound/pcm_iec958.h>
-/**
- snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+/*
- snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status
- @runtime: pcm runtime structure with ->rate filled in
- @cs: channel status buffer, at least four bytes
- @len: length of channel status buffer
- Create the consumer format channel status data in @cs of maximum size
- @len corresponding to the parameters of the PCM runtime @runtime.
- Drivers may wish to tweak the contents of the buffer after creation.
- Update sampling frequency and word length parameters of the consumer format
- channel status data in @cs of maximum size @len.
- Values correspond to the rate and format parameters of the PCM runtime
*/
- @runtime.
- Returns: length of buffer, or negative error code if something failed.
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
- size_t len)
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len){
- unsigned int fs, ws;
- if (len < 4) return -EINVAL;
- cs[3] &= ~IEC958_AES3_CON_FS;
- switch (runtime->rate) {
- case 22050:
cs[3] |= IEC958_AES3_CON_FS_22050;break;
fs = IEC958_AES3_CON_FS_32000;
cs[3] |= IEC958_AES3_CON_FS_32000;
fs = IEC958_AES3_CON_FS_44100;
cs[3] |= IEC958_AES3_CON_FS_44100;
fs = IEC958_AES3_CON_FS_48000;
cs[3] |= IEC958_AES3_CON_FS_48000;
fs = IEC958_AES3_CON_FS_88200;
cs[3] |= IEC958_AES3_CON_FS_88200;
fs = IEC958_AES3_CON_FS_96000;
cs[3] |= IEC958_AES3_CON_FS_96000;
fs = IEC958_AES3_CON_FS_176400;
cs[3] |= IEC958_AES3_CON_FS_176400;
fs = IEC958_AES3_CON_FS_192000;
cs[3] |= IEC958_AES3_CON_FS_192000;
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
Add a helper to update only the IEC958 channel status sampling freq and word length parameters from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com
NAK.
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..0c84c69 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -3,7 +3,24 @@
#include <linux/types.h>
+#ifdef CONFIG_SND_PCM_IEC958 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
- size_t len);
size_t len);+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len);+#else +int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)+{
- return len;
+}
Firstly, this is dangerous. If you don't have anything defined here, then the channel status ends up being undefined.
The intention here is that if you're a user of this, then select the config symbol. That makes providing stubs totally unnecessary and removes this problem.
agree
+#endif
#endif diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c index 36b2d7a..abe3967 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -11,67 +11,72 @@ #include <sound/pcm.h> #include <sound/pcm_iec958.h>
-/**
- snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+/*
- snd_pcm_update_iec958_consumer - update consumer format IEC958 channel status
- @runtime: pcm runtime structure with ->rate filled in
- @cs: channel status buffer, at least four bytes
- @len: length of channel status buffer
- Create the consumer format channel status data in @cs of maximum size
- @len corresponding to the parameters of the PCM runtime @runtime.
- Drivers may wish to tweak the contents of the buffer after creation.
- Update sampling frequency and word length parameters of the consumer format
- channel status data in @cs of maximum size @len.
- Values correspond to the rate and format parameters of the PCM runtime
*/
- @runtime.
- Returns: length of buffer, or negative error code if something failed.
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
- size_t len)
+int snd_pcm_update_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
size_t len)
- unsigned int fs, ws;
- if (len < 4) return -EINVAL;
- cs[3] &= ~IEC958_AES3_CON_FS;
- switch (runtime->rate) {
- case 22050:
cs[3] |= IEC958_AES3_CON_FS_22050;break;
fs = IEC958_AES3_CON_FS_32000;
cs[3] |= IEC958_AES3_CON_FS_32000;
fs = IEC958_AES3_CON_FS_44100;
cs[3] |= IEC958_AES3_CON_FS_44100;
fs = IEC958_AES3_CON_FS_48000;
cs[3] |= IEC958_AES3_CON_FS_48000;
fs = IEC958_AES3_CON_FS_88200;
cs[3] |= IEC958_AES3_CON_FS_88200;
fs = IEC958_AES3_CON_FS_96000;
cs[3] |= IEC958_AES3_CON_FS_96000;
fs = IEC958_AES3_CON_FS_176400;
cs[3] |= IEC958_AES3_CON_FS_176400;
fs = IEC958_AES3_CON_FS_192000;
cs[3] |= IEC958_AES3_CON_FS_192000;Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer. So we propose a second function that only updates the sampling freq and word length. Other parameters are unchanged (set by user).
In a previous patch proposal, we managed channel status sampling freq directly in our driver. Mark suggested us to manage it in a generic function (could be useful for other drivers).
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
On Thu, 03 Dec 2015 12:13:04 +0100, Russell King - ARM Linux wrote:
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
That's true. OTOH, in most drivers, we try to be kind not to be too strict for this requirement and adjust the status bits accordingly. This comes from the lessons we learned how user-space doesn't care the things.
So, if we want to keep that good uncle attitude, this change doesn't look so bad to me.
thanks,
Takashi
On 12/03/2015 01:03 PM, Takashi Iwai wrote:
On Thu, 03 Dec 2015 12:13:04 +0100, Russell King - ARM Linux wrote:
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
That's true. OTOH, in most drivers, we try to be kind not to be too strict for this requirement and adjust the status bits accordingly. This comes from the lessons we learned how user-space doesn't care the things.
So, if we want to keep that good uncle attitude, this change doesn't look so bad to me.
shall I deliver a new version of this patch with header correction or shall I abandon this patch?
thanks,
Takashi
On Mon, 07 Dec 2015 09:46:02 +0100, Moise Gergaud wrote:
On 12/03/2015 01:03 PM, Takashi Iwai wrote:
On Thu, 03 Dec 2015 12:13:04 +0100, Russell King - ARM Linux wrote:
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
That's true. OTOH, in most drivers, we try to be kind not to be too strict for this requirement and adjust the status bits accordingly. This comes from the lessons we learned how user-space doesn't care the things.
So, if we want to keep that good uncle attitude, this change doesn't look so bad to me.
shall I deliver a new version of this patch with header correction or shall I abandon this patch?
It's up to you whether you want further convincing Russell :) I'd happily merge this once when we get his ack.
thanks,
Takashi
On 12/07/2015 10:51 AM, Takashi Iwai wrote:
On Mon, 07 Dec 2015 09:46:02 +0100, Moise Gergaud wrote:
On 12/03/2015 01:03 PM, Takashi Iwai wrote:
On Thu, 03 Dec 2015 12:13:04 +0100, Russell King - ARM Linux wrote:
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote:
Again, I'm not happy with this change. The intention here is that we validate all arguments before we change anything. This breaks that principle.
We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
That's true. OTOH, in most drivers, we try to be kind not to be too strict for this requirement and adjust the status bits accordingly. This comes from the lessons we learned how user-space doesn't care the things.
So, if we want to keep that good uncle attitude, this change doesn't look so bad to me.
shall I deliver a new version of this patch with header correction or shall I abandon this patch?
It's up to you whether you want further convincing Russell :) I'd happily merge this once when we get his ack.
thanks,
Takashi
I share Takashi's view; unfortunately, we cannot always trust user space. And so, for robustness reason, I think drivers shall ensure that channel status params are correct. For that purpose, the update helper function could be useful.
On Tue, Dec 08, 2015 at 03:30:22PM +0100, Moise Gergaud wrote:
On 12/07/2015 10:51 AM, Takashi Iwai wrote:
On Mon, 07 Dec 2015 09:46:02 +0100, Moise Gergaud wrote:
On 12/03/2015 01:03 PM, Takashi Iwai wrote:
On Thu, 03 Dec 2015 12:13:04 +0100, Russell King - ARM Linux wrote:
On Thu, Dec 03, 2015 at 11:58:14AM +0100, Moise Gergaud wrote:
On 12/02/2015 08:00 PM, Russell King - ARM Linux wrote: >Again, I'm not happy with this change. The intention here is that we >validate all arguments before we change anything. This breaks that >principle. > We propose this change for compressed mode (IEC61937/non linear PCM). In case of compressed mode, iec958 channel status byte 0 bit1 = 1, then we cannot use snd_pcm_create_iec958_consumer.
Why can you not use snd_pcm_create_iec958_consumer() for this case? You are allowed to modify the values you get afterwards in order to handle this case if you so wish to toggle this bit.
However, if you are using data mode, you really should be using the AES bytes passed by the application through alsalib and into the kernel via the IEC958 controls. There are three controls specified for this:
SNDRV_CTL_NAME_IEC958("", PLAYBACK, CON_MASK) - the bits which can be changed for consumer mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, PRO_MASK) - the bits which can be changed for professional mode. SNDRV_CTL_NAME_IEC958("", PLAYBACK, DEFAULT) - the value of the changable bits.
When using this, it is userspace's responsibility to set the sample rate appropriately for the stream.
The only case to use snd_pcm_create_iec958_consumer() is to generate the status bytes for PCM audio, where the userspace API doesn't facilitiate generating and/or passing the AES status bytes.
That's true. OTOH, in most drivers, we try to be kind not to be too strict for this requirement and adjust the status bits accordingly. This comes from the lessons we learned how user-space doesn't care the things.
So, if we want to keep that good uncle attitude, this change doesn't look so bad to me.
shall I deliver a new version of this patch with header correction or shall I abandon this patch?
It's up to you whether you want further convincing Russell :) I'd happily merge this once when we get his ack.
thanks,
Takashi
I share Takashi's view; unfortunately, we cannot always trust user space. And so, for robustness reason, I think drivers shall ensure that channel status params are correct. For that purpose, the update helper function could be useful.
I don't, and now that you mention about not trusting userspace, I'm entrenching into that position. I have hardware where userspace must get it correct: I need to use the IEC958 alsalib plugin, and that relies on userspace getting it right - the kernel plays no part in generating the channel status, it's all handled in userspace. I do have kernel-side channel status generation for 24-bit 4-byte PCM audio though, because it's easy to merge in the channel status information (basically doing what the IEC958 alsalib plugin does.) I'd much rather that was moved out to userspace though.
What you're suggesting creates two separate classes of driver: * those which always overwrite the channel status information from userspace, which will make userspace lazy and _more_ buggy. * those which have to use the channel status information from userspace.
If the second is a minority, what we end up with are bugs with channel status information being missed, because programs will work on the "majority" of drivers which overwrite the userspace specified channel status.
IMHO, that's really not a sane position.
On Wed, Dec 02, 2015 at 03:22:04PM +0100, Moise Gergaud wrote:
Add a helper to update only the IEC958 channel status sampling freq and word length parameters from an ALSA snd_pcm_runtime structure, taking account of the sample rate and sample size.
What's also missing is an explanation of why this change is needed.
Use pcm iec958 channel status helper to update sampling freq parameter.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- sound/soc/sti/Kconfig | 1 + sound/soc/sti/uniperif_player.c | 58 ++++++++--------------------------------- 2 files changed, 12 insertions(+), 47 deletions(-)
diff --git a/sound/soc/sti/Kconfig b/sound/soc/sti/Kconfig index 64a6900..8e616a4 100644 --- a/sound/soc/sti/Kconfig +++ b/sound/soc/sti/Kconfig @@ -6,6 +6,7 @@ menuconfig SND_SOC_STI depends on SND_SOC depends on ARCH_STI || COMPILE_TEST select SND_SOC_GENERIC_DMAENGINE_PCM + select SND_PCM_IEC958 help Say Y if you want to enable ASoC-support for any of the STI platforms (e.g. STIH416). diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index 7aca6b9..b55e412 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -12,6 +12,7 @@
#include <sound/asoundef.h> #include <sound/soc.h> +#include <sound/pcm_iec958.h>
#include "uniperif.h"
@@ -244,6 +245,7 @@ static void uni_player_set_channel_status(struct uniperif *player, { int n; unsigned int status; + unsigned char *aes = player->stream_settings.iec958.status;
/* * Some AVRs and TVs require the channel status to contain a correct @@ -251,51 +253,9 @@ static void uni_player_set_channel_status(struct uniperif *player, * set one. */ mutex_lock(&player->ctrl_lock); - if (runtime) { - switch (runtime->rate) { - case 22050: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_22050; - break; - case 44100: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_44100; - break; - case 88200: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_88200; - break; - case 176400: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_176400; - break; - case 24000: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_24000; - break; - case 48000: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_48000; - break; - case 96000: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_96000; - break; - case 192000: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_192000; - break; - case 32000: - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_32000; - break; - default: - /* Mark as sampling frequency not indicated */ - player->stream_settings.iec958.status[3] = - IEC958_AES3_CON_FS_NOTID; - break; - } - } + + /* update channel status sampling freq */ + snd_pcm_update_iec958_consumer(runtime, aes, 4);
/* Audio mode: * Use audio mode status to select PCM or encoded mode @@ -318,7 +278,7 @@ static void uni_player_set_channel_status(struct uniperif *player, /* Program the new channel status */ for (n = 0; n < 6; ++n) { status = - player->stream_settings.iec958.status[0 + (n * 4)] & 0xf; + player->stream_settings.iec958.status[0 + (n * 4)] & 0xff; status |= player->stream_settings.iec958.status[1 + (n * 4)] << 8; status |= @@ -563,6 +523,7 @@ static int uni_player_ctl_iec958_get(struct snd_kcontrol *kcontrol, ucontrol->value.iec958.status[1] = iec958->status[1]; ucontrol->value.iec958.status[2] = iec958->status[2]; ucontrol->value.iec958.status[3] = iec958->status[3]; + ucontrol->value.iec958.status[4] = iec958->status[4]; mutex_unlock(&player->ctrl_lock); return 0; } @@ -574,15 +535,18 @@ static int uni_player_ctl_iec958_put(struct snd_kcontrol *kcontrol, struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai); struct uniperif *player = priv->dai_data.uni; struct snd_aes_iec958 *iec958 = &player->stream_settings.iec958; + struct snd_pcm_substream *substream = player->substream;
mutex_lock(&player->ctrl_lock); iec958->status[0] = ucontrol->value.iec958.status[0]; iec958->status[1] = ucontrol->value.iec958.status[1]; iec958->status[2] = ucontrol->value.iec958.status[2]; iec958->status[3] = ucontrol->value.iec958.status[3]; + iec958->status[4] = ucontrol->value.iec958.status[4]; mutex_unlock(&player->ctrl_lock);
- uni_player_set_channel_status(player, NULL); + if (substream && substream->runtime) + uni_player_set_channel_status(player, substream->runtime);
return 0; }
On Wed, 02 Dec 2015 15:22:05 +0100, Moise Gergaud wrote:
Use pcm iec958 channel status helper to update sampling freq parameter.
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com
sound/soc/sti/Kconfig | 1 + sound/soc/sti/uniperif_player.c | 58 ++++++++--------------------------------- 2 files changed, 12 insertions(+), 47 deletions(-)
diff --git a/sound/soc/sti/Kconfig b/sound/soc/sti/Kconfig index 64a6900..8e616a4 100644 --- a/sound/soc/sti/Kconfig +++ b/sound/soc/sti/Kconfig @@ -6,6 +6,7 @@ menuconfig SND_SOC_STI depends on SND_SOC depends on ARCH_STI || COMPILE_TEST select SND_SOC_GENERIC_DMAENGINE_PCM
- select SND_PCM_IEC958 help Say Y if you want to enable ASoC-support for any of the STI platforms (e.g. STIH416).
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index 7aca6b9..b55e412 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -12,6 +12,7 @@
#include <sound/asoundef.h> #include <sound/soc.h> +#include <sound/pcm_iec958.h>
#include "uniperif.h"
@@ -244,6 +245,7 @@ static void uni_player_set_channel_status(struct uniperif *player, { int n; unsigned int status;
unsigned char *aes = player->stream_settings.iec958.status;
/*
- Some AVRs and TVs require the channel status to contain a correct
@@ -251,51 +253,9 @@ static void uni_player_set_channel_status(struct uniperif *player, * set one. */ mutex_lock(&player->ctrl_lock);
- if (runtime) {
switch (runtime->rate) {case 22050:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_22050;break;case 44100:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_44100;break;case 88200:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_88200;break;case 176400:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_176400;break;case 24000:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_24000;break;case 48000:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_48000;break;case 96000:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_96000;break;case 192000:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_192000;break;case 32000:player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_32000;break;default:/* Mark as sampling frequency not indicated */player->stream_settings.iec958.status[3] =IEC958_AES3_CON_FS_NOTID;break;}- }
/* update channel status sampling freq */
snd_pcm_update_iec958_consumer(runtime, aes, 4);
/* Audio mode:
- Use audio mode status to select PCM or encoded mode
The changes until this point are indeed about the conversion with a helper function. However....
@@ -318,7 +278,7 @@ static void uni_player_set_channel_status(struct uniperif *player, /* Program the new channel status */ for (n = 0; n < 6; ++n) { status =
player->stream_settings.iec958.status[0 + (n * 4)] & 0xf;
player->stream_settings.iec958.status[0 + (n * 4)] & 0xff;@@ -563,6 +523,7 @@ static int uni_player_ctl_iec958_get(struct snd_kcontrol *kcontrol, ucontrol->value.iec958.status[1] = iec958->status[1]; ucontrol->value.iec958.status[2] = iec958->status[2]; ucontrol->value.iec958.status[3] = iec958->status[3];
- ucontrol->value.iec958.status[4] = iec958->status[4]; mutex_unlock(&player->ctrl_lock); return 0;
} @@ -574,15 +535,18 @@ static int uni_player_ctl_iec958_put(struct snd_kcontrol *kcontrol, struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai); struct uniperif *player = priv->dai_data.uni; struct snd_aes_iec958 *iec958 = &player->stream_settings.iec958;
struct snd_pcm_substream *substream = player->substream;
mutex_lock(&player->ctrl_lock); iec958->status[0] = ucontrol->value.iec958.status[0]; iec958->status[1] = ucontrol->value.iec958.status[1]; iec958->status[2] = ucontrol->value.iec958.status[2]; iec958->status[3] = ucontrol->value.iec958.status[3];
iec958->status[4] = ucontrol->value.iec958.status[4]; mutex_unlock(&player->ctrl_lock);
These changes are basically irrelevant. They look like real fixes, and if it's really so, they should be split to a different patch.
Takashi
Detect HBRA based on rate and channels used in the pcm session In case of HBRA: - CH_STS repeat is disabled - channel status sampling freq is not set if already set (because it can be a multiple of the runtime rate)
Signed-off-by: Moise Gergaud moise.gergaud@st.com Acked-by: Arnaud Pouliquen arnaud.pouliquen@st.com --- sound/soc/sti/uniperif_player.c | 34 +++++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index b55e412..cf99dee 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -246,16 +246,39 @@ static void uni_player_set_channel_status(struct uniperif *player, int n; unsigned int status; unsigned char *aes = player->stream_settings.iec958.status; + const unsigned int cs_rate[] = { + 44100, 0, 48000, 32000, 22050, 24000, + 88200, 768000, 176400, 192000 + };
/* * Some AVRs and TVs require the channel status to contain a correct - * sampling frequency. If no sample rate is already specified, then - * set one. + * sampling freq. + * In case of HBRA is not detected: + * set the channel status sampling freq + * In case of HBRA is detected: + * channel status sampling freq is already set; it can be a multiple + * of runtime rate + * + * HBRA is detected when: + * runtime channels is 8 and runtime->rate < channel status rate */ + mutex_lock(&player->ctrl_lock);
- /* update channel status sampling freq */ - snd_pcm_update_iec958_consumer(runtime, aes, 4); + if ((runtime->channels == 8) && + (runtime->rate < cs_rate[aes[3] & IEC958_AES3_CON_FS])) { + /* + * Consecutive frames repetition of Z preamble needs to be + * disabled in case of HBRA + */ + SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_DISABLE(player); + } else { + SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_ENABLE(player); + + /* update channel status sampling freq */ + snd_pcm_update_iec958_consumer(runtime, aes, 4); + }
/* Audio mode: * Use audio mode status to select PCM or encoded mode @@ -358,9 +381,6 @@ static int uni_player_prepare_iec958(struct uniperif *player, /* Disable one-bit audio mode */ SET_UNIPERIF_CONFIG_ONE_BIT_AUD_DISABLE(player);
- /* Enable consecutive frames repetition of Z preamble (not for HBRA) */ - SET_UNIPERIF_CONFIG_REPEAT_CHL_STS_ENABLE(player); - /* Change to SUF0_SUBF1 and left/right channels swap! */ SET_UNIPERIF_CONFIG_SUBFRAME_SEL_SUBF1_SUBF0(player);
On 12/02/2015 03:22 PM, Moise Gergaud wrote:
This patchset is V2 of "[PATCH 2/2] ASoC: sti: correction for HBRA support"
Moise Gergaud (3): ALSA: pcm: add IEC958 channel status update ASoC: sti: use pcm iec958 channel status helper ASoC: sti: correction for HBRA (High Bit Rate Audio) support
include/sound/pcm_iec958.h | 19 ++++++++- sound/core/pcm_iec958.c | 78 +++++++++++++++++++++++++------------ sound/soc/sti/Kconfig | 1 + sound/soc/sti/uniperif_player.c | 86 +++++++++++++++++------------------------ 4 files changed, 107 insertions(+), 77 deletions(-)
patchset abandoned
participants (3)
-
Moise Gergaud -
Russell King - ARM Linux -
Takashi Iwai