[v8,1/6] ALSA: pcm: add IEC958 channel status helper for hw_params
diff mbox

Message ID 164484552294777b0b9b88d7982bd6bfb0da9a9f.1458214526.git.jsarha@ti.com
State New
Headers show

Commit Message

Jyri Sarha March 17, 2016, 12:22 p.m. UTC
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(-)

Comments

Mark Brown March 28, 2016, 7:38 p.m. UTC | #1
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?
Takashi Iwai March 29, 2016, 8:54 a.m. UTC | #2
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
Russell King - ARM Linux March 29, 2016, 5:23 p.m. UTC | #3
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.
Takashi Iwai March 30, 2016, 6:21 a.m. UTC | #4
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
Jyri Sarha March 30, 2016, 8:25 a.m. UTC | #5
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
Takashi Iwai March 30, 2016, 10:24 a.m. UTC | #6
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

Patch
diff mbox

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);