diff mbox

[RFC,v4,2/8] ALSA: pcm: add IEC958 channel status helper for hw_params

Message ID a7b69fb075b0ae993846cfb65abb20ecab5c9805.1442572860.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha Sept. 18, 2015, 11:06 a.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    | 52 +++++++++++++++++++++++++++++++---------------
 2 files changed, 37 insertions(+), 17 deletions(-)

Comments

Mark Brown Sept. 19, 2015, 5:46 p.m. UTC | #1
On Fri, Sep 18, 2015 at 02:06:39PM +0300, 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>
> ---
>  include/sound/pcm_iec958.h |  2 ++
>  sound/core/pcm_iec958.c    | 52 +++++++++++++++++++++++++++++++---------------

This is core ALSA code, you need to CC Takashi (added him, not cutting
context).

>  2 files changed, 37 insertions(+), 17 deletions(-)
> 
> 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..c9f8b66 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;
> @@ -92,4 +80,34 @@ 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);
> +
> +
> +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);
> -- 
> 1.9.1
> 
>
Russell King - ARM Linux Sept. 21, 2015, 9:37 a.m. UTC | #2
On Fri, Sep 18, 2015 at 02:06:39PM +0300, 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.

What is the reason for doing this early?

ALSA documentation (which may be out of date) says that the hw_params
callback can be called multiple times during stream setup.  Do we want
to be repeatedly programming the HDMI infoframe with different settings,
potentially confusing the attached device, or would it be better to do
it slightly later (in the prepare callback) after the parameters have
been fully negotiated?
Jyri Sarha Sept. 21, 2015, 2:39 p.m. UTC | #3
On 09/21/15 12:37, Russell King - ARM Linux wrote:
> On Fri, Sep 18, 2015 at 02:06:39PM +0300, 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.
>
> What is the reason for doing this early?
>

After some thinking, there is no good reason. It makes the codec bit 
more complicated, but that is not a good reason. I'll change that and 
use the prepare callback.

This patch can be dropped.

> ALSA documentation (which may be out of date) says that the hw_params
> callback can be called multiple times during stream setup.  Do we want
> to be repeatedly programming the HDMI infoframe with different settings,
> potentially confusing the attached device, or would it be better to do
> it slightly later (in the prepare callback) after the parameters have
> been fully negotiated?
>

If it is possible that hw_params() can be called multiple times, at 
least it does not happen very often. I wonder if it would ever happen 
again after a successful hw_params() call. But there is no downside in 
using prepare callback. I was just too much following the way ASoC 
codecs usually do the thing by writing everything to iron as the 
callbacks come in.

Best regards,
Jyri
Clemens Ladisch Sept. 21, 2015, 3:08 p.m. UTC | #4
Jyri Sarha wrote:
> On 09/21/15 12:37, Russell King - ARM Linux wrote:
>> ALSA documentation (which may be out of date) says that the hw_params
>> callback can be called multiple times during stream setup.  Do we want
>> to be repeatedly programming the HDMI infoframe with different settings,
>> potentially confusing the attached device, or would it be better to do
>> it slightly later (in the prepare callback) after the parameters have
>> been fully negotiated?
>
> If it is possible that hw_params() can be called multiple times, at
> least it does not happen very often. I wonder if it would ever happen
> again after a successful hw_params() call.

It is certainly allowed, and it happens regularly when using OSS
emulation.  AFAIK PulseAudio does something similar when enumerating
devices.

> But there is no downside in using prepare callback.

The prepare callback also can be called multiple times.  This certainly
happens when the stream is stopped/restarted multiple times.

If something does not need to be done again when a stream is restarted,
then it should be done in the hw_params callback.


Regards,
Clemens
Jyri Sarha Sept. 21, 2015, 5:25 p.m. UTC | #5
On 09/21/15 18:08, Clemens Ladisch wrote:
>> >But there is no downside in using prepare callback.
> The prepare callback also can be called multiple times.  This certainly
> happens when the stream is stopped/restarted multiple times.
>
> If something does not need to be done again when a stream is restarted,
> then it should be done in the hw_params callback.

Oh, forgot about that. This would suggest to keep the stream header 
configuration in the hw_params().

Thanks,
Jyri
diff mbox

Patch

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..c9f8b66 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;
@@ -92,4 +80,34 @@  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);
+
+
+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);