| Message ID | 164484552294777b0b9b88d7982bd6bfb0da9a9f.1458214526.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 17, 2016 at 02:22:29PM +0200, Jyri Sarha wrote: > Add IEC958 channel status helper that gets the audio properties from > snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to > produce the channel status bits already in audio stream configuration > phase. Takashi?
On Thu, 17 Mar 2016 13:22:29 +0100, Jyri Sarha wrote: > > Add IEC958 channel status helper that gets the audio properties from > snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to > produce the channel status bits already in audio stream configuration > phase. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> This patch looks almost good to me, but... > @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, > IEC958_AES4_CON_MAX_WORDLEN_24; > break; > case 24: > + case 32: /* Assume 24-bit width for 32-bit samples. */ > ws = IEC958_AES4_CON_WORDLEN_24_20 | > IEC958_AES4_CON_MAX_WORDLEN_24; > break; ... this change is silently slipped in. It should be mentioned in the changelog, or split to another patch, as this is basically an orthogonal change. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote: > On Thu, 17 Mar 2016 13:22:29 +0100, > Jyri Sarha wrote: > > > > Add IEC958 channel status helper that gets the audio properties from > > snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to > > produce the channel status bits already in audio stream configuration > > phase. > > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > This patch looks almost good to me, but... > > > @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, > > IEC958_AES4_CON_MAX_WORDLEN_24; > > break; > > case 24: > > + case 32: /* Assume 24-bit width for 32-bit samples. */ > > ws = IEC958_AES4_CON_WORDLEN_24_20 | > > IEC958_AES4_CON_MAX_WORDLEN_24; > > break; > > ... this change is silently slipped in. It should be mentioned in the > changelog, or split to another patch, as this is basically an > orthogonal change. Does it even make sense - AES doesn't have support for 32-bit samples, it can only ever truncate them down to 24-bit.
On Tue, 29 Mar 2016 19:23:12 +0200, Russell King - ARM Linux wrote: > > On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote: > > On Thu, 17 Mar 2016 13:22:29 +0100, > > Jyri Sarha wrote: > > > > > > Add IEC958 channel status helper that gets the audio properties from > > > snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to > > > produce the channel status bits already in audio stream configuration > > > phase. > > > > > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > > > This patch looks almost good to me, but... > > > > > @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, > > > IEC958_AES4_CON_MAX_WORDLEN_24; > > > break; > > > case 24: > > > + case 32: /* Assume 24-bit width for 32-bit samples. */ > > > ws = IEC958_AES4_CON_WORDLEN_24_20 | > > > IEC958_AES4_CON_MAX_WORDLEN_24; > > > break; > > > > ... this change is silently slipped in. It should be mentioned in the > > changelog, or split to another patch, as this is basically an > > orthogonal change. > > Does it even make sense - AES doesn't have support for 32-bit samples, > it can only ever truncate them down to 24-bit. Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on real devices, mostly on PCI ones. So, adding the value 32 check there itself is valid, I suppose. It's rather due to a bad design in the current API. The actual bits should be specified hw_params.msbits field, instead. But, not all drivers set this properly, unfortunately. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/30/16 09:21, Takashi Iwai wrote: > On Tue, 29 Mar 2016 19:23:12 +0200, > Russell King - ARM Linux wrote: >> >> On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote: >>> On Thu, 17 Mar 2016 13:22:29 +0100, >>> Jyri Sarha wrote: >>>> >>>> Add IEC958 channel status helper that gets the audio properties from >>>> snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to >>>> produce the channel status bits already in audio stream configuration >>>> phase. >>>> >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> >>> >>> This patch looks almost good to me, but... >>> >>>> @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, >>>> IEC958_AES4_CON_MAX_WORDLEN_24; >>>> break; >>>> case 24: >>>> + case 32: /* Assume 24-bit width for 32-bit samples. */ >>>> ws = IEC958_AES4_CON_WORDLEN_24_20 | >>>> IEC958_AES4_CON_MAX_WORDLEN_24; >>>> break; >>> >>> ... this change is silently slipped in. It should be mentioned in the >>> changelog, or split to another patch, as this is basically an >>> orthogonal change. >> >> Does it even make sense - AES doesn't have support for 32-bit samples, >> it can only ever truncate them down to 24-bit. > > Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on > real devices, mostly on PCI ones. So, adding the value 32 check there > itself is valid, I suppose. It's rather due to a bad design in the > current API. > That also happens on SoC environment i2s interfaces for various reasons. For instance if the i2s bit-clock for sample-rate * 24 * 2 is not available, but sample-rate * 32 * 2 is. > The actual bits should be specified hw_params.msbits field, instead. > But, not all drivers set this properly, unfortunately. > How about allowing the 32-bit format only if hw_params.msbits is set to 24 bits? I can set the hw_params.msbits to 24 in ASoC hdmi-codec's hw_params, can't I? I could also fake the whole hw_params struct that I pass to snd_pcm_create_iec958_consumer_hw_params(), but would that make sense? What ever the approach, I can of course split the 32 bit support into a separate subsequent patch, but for instance Beaglebone-black 48kHz 24-bit HDMI audio needs 32 bit format on i2s bus because of the available i2s bit-clocks. Best regards, Jyri -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 30 Mar 2016 10:25:52 +0200, Jyri Sarha wrote: > > On 03/30/16 09:21, Takashi Iwai wrote: > > On Tue, 29 Mar 2016 19:23:12 +0200, > > Russell King - ARM Linux wrote: > >> > >> On Tue, Mar 29, 2016 at 10:54:08AM +0200, Takashi Iwai wrote: > >>> On Thu, 17 Mar 2016 13:22:29 +0100, > >>> Jyri Sarha wrote: > >>>> > >>>> Add IEC958 channel status helper that gets the audio properties from > >>>> snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to > >>>> produce the channel status bits already in audio stream configuration > >>>> phase. > >>>> > >>>> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >>> > >>> This patch looks almost good to me, but... > >>> > >>>> @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, > >>>> IEC958_AES4_CON_MAX_WORDLEN_24; > >>>> break; > >>>> case 24: > >>>> + case 32: /* Assume 24-bit width for 32-bit samples. */ > >>>> ws = IEC958_AES4_CON_WORDLEN_24_20 | > >>>> IEC958_AES4_CON_MAX_WORDLEN_24; > >>>> break; > >>> > >>> ... this change is silently slipped in. It should be mentioned in the > >>> changelog, or split to another patch, as this is basically an > >>> orthogonal change. > >> > >> Does it even make sense - AES doesn't have support for 32-bit samples, > >> it can only ever truncate them down to 24-bit. > > > > Well, using SNDRV_PCM_FORMAT_S32_* for 24 bit samples is seen often on > > real devices, mostly on PCI ones. So, adding the value 32 check there > > itself is valid, I suppose. It's rather due to a bad design in the > > current API. > > > > That also happens on SoC environment i2s interfaces for various reasons. > For instance if the i2s bit-clock for sample-rate * 24 * 2 is not > available, but sample-rate * 32 * 2 is. > > > The actual bits should be specified hw_params.msbits field, instead. > > But, not all drivers set this properly, unfortunately. > > > > How about allowing the 32-bit format only if hw_params.msbits is set to > 24 bits? I'm thinking whether msbits is updated automatically upon hw_params refinement, or it's just used as the constraint. If msbits is always set, we can refer this value reliably. But, I guess just accepting the format width 32 would be good enough as a start. The driver is responsible to fill in the right data format, and the helper doesn't care much about that. Once when we figure out that msbits can be used reliably, we can switch to it later. > I can set the hw_params.msbits to 24 in ASoC hdmi-codec's hw_params, > can't I? I could also fake the whole hw_params struct that I pass to > snd_pcm_create_iec958_consumer_hw_params(), but would that make sense? The setup of msbits is anyway good no matter whether we add the check in iec958 layer. > What ever the approach, I can of course split the 32 bit support into a > separate subsequent patch, but for instance Beaglebone-black 48kHz > 24-bit HDMI audio needs 32 bit format on i2s bus because of the > available i2s bit-clocks. Yes, splitting the 32bit support is more appropriate than hiding in a patch to add the new helper. Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h index 0eed397..36f023a 100644 --- a/include/sound/pcm_iec958.h +++ b/include/sound/pcm_iec958.h @@ -6,4 +6,6 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, size_t len); +int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params, + u8 *cs, size_t len); #endif diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c index 36b2d7a..5e6aed6 100644 --- a/sound/core/pcm_iec958.c +++ b/sound/core/pcm_iec958.c @@ -9,30 +9,18 @@ #include <linux/types.h> #include <sound/asoundef.h> #include <sound/pcm.h> +#include <sound/pcm_params.h> #include <sound/pcm_iec958.h> -/** - * 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) +static int create_iec958_consumer(uint rate, uint sample_width, + u8 *cs, size_t len) { unsigned int fs, ws; if (len < 4) return -EINVAL; - switch (runtime->rate) { + switch (rate) { case 32000: fs = IEC958_AES3_CON_FS_32000; break; @@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, } if (len > 4) { - switch (snd_pcm_format_width(runtime->format)) { + switch (sample_width) { case 16: ws = IEC958_AES4_CON_WORDLEN_20_16; break; @@ -71,6 +59,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, IEC958_AES4_CON_MAX_WORDLEN_24; break; case 24: + case 32: /* Assume 24-bit width for 32-bit samples. */ ws = IEC958_AES4_CON_WORDLEN_24_20 | IEC958_AES4_CON_MAX_WORDLEN_24; break; @@ -92,4 +81,46 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs, return len; } + +/** + * 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) +{ + return create_iec958_consumer(runtime->rate, + snd_pcm_format_width(runtime->format), + cs, len); +} EXPORT_SYMBOL(snd_pcm_create_iec958_consumer); + +/** + * snd_pcm_create_iec958_consumer_hw_params - create IEC958 channel status + * @hw_params: the hw_params instance for extracting rate and sample format + * @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_hw_params(struct snd_pcm_hw_params *params, + u8 *cs, size_t len) +{ + return create_iec958_consumer(params_rate(params), params_width(params), + cs, len); +} +EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);
Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- include/sound/pcm_iec958.h | 2 ++ sound/core/pcm_iec958.c | 65 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 50 insertions(+), 17 deletions(-)