[PATCH 0/3] ALSA: pcm:

[PATCH 0/3] ALSA: pcm:

[Takashi Sakamoto]

Hi,

This patchset is a revised version of my former RFC.


[alsa-devel] [PATCH RFC 00/21] ALSA: pcm: add tracepoints for PCM params operation
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120548.html

In ALSA PCM interface, applications can get hardware capability as
'struct snd_pcm_hw_params' type of data returned by a call of ioctl(2)
with SNDRV_PCM_IOCTL_HW_REFINE/SNDRV_PCM_IOCTL_HW_PARAMS' commands.
Results of these commands should be the same in a certain conditions but
actually it's not.

This is a part of layout of the structure.

struct snd_pcm_hw_params {
    ...
    unsigned int info;
    unsigned int msbits;
    unsigned int rate_num;
    unsigned int rate_den;
    snd_pcm_uframes_t fifo_size;
    ...
};

For example, although the 'msbits', 'rate_num' and 'rate_den' fields are
filled in a result of HW_REFINE command conditionally, they're never
filled in a result of HW_PARAMS command. This seems a bug.

This patchset attempts to fix the bug. This affects userspace applications
in cotents of the result, but it's acceptrable with a merit for the
applications to get enough parameters.

Takashi Sakamoto (3):
  ALSA: pcm: use helper functions to refer parameters as constants
  ALSA: pcm: calculate non-mask/non-interval parameters always when
    possible
  ALSA: pcm: move fixup of info flag after selecting single parameters

 sound/core/pcm_native.c | 88 +++++++++++++++++++++++++++++--------------------
 1 file changed, 53 insertions(+), 35 deletions(-)

-- 
2.11.0

[PATCH 1/3] ALSA: pcm: use helper functions to refer parameters as constants

[Takashi Sakamoto]

To fixup some parameters, ALSA PCM core refers the other parameters as
constants. There're some macros for this purpose.

This commit replaces codes with them.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 422ee4629698..87a507f12f2f 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -444,8 +444,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		      struct snd_pcm_hw_params *params)
 {
 	struct snd_pcm_hardware *hw;
-	struct snd_interval *i = NULL;
-	struct snd_mask *m = NULL;
+	const struct snd_interval *i;
+	const struct snd_mask *m;
 	int err;
 
 	params->info = 0;
@@ -470,13 +470,13 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		return err;
 
 	if (!params->msbits) {
-		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
 		if (snd_interval_single(i))
 			params->msbits = snd_interval_value(i);
 	}
 
 	if (!params->rate_den) {
-		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE);
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE);
 		if (snd_interval_single(i)) {
 			params->rate_num = snd_interval_value(i);
 			params->rate_den = 1;
@@ -492,8 +492,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 					  SNDRV_PCM_INFO_MMAP_VALID);
 	}
 	if (!params->fifo_size) {
-		m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
-		i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS);
+		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS);
 		if (snd_mask_single(m) && snd_interval_single(i)) {
 			err = substream->ops->ioctl(substream,
 					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
-- 
2.11.0

[PATCH 2/3] ALSA: pcm: calculate non-mask/non-interval parameters always when possible

[Takashi Sakamoto]

A structure for parameters of PCM runtime has parameters which are
not classified as mask/interval type. They are decided only when
corresponding normal parameters have unique values.
 * struct snd_pcm_hw_params.msbits
 * struct snd_pcm_hw_params.rate_num
 * struct snd_pcm_hw_params.rate_den
 * struct snd_pcm_hw_params.fifo_size

Current implementation of hw_params ioctl sometimes doesn't decide these
parameters even if corresponding parameters are fixed, because these
parameters are evaluated before a call of snd_pcm_hw_params_choose().

This commit adds a helper function to process the parameters and call it
in proper positions.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 70 +++++++++++++++++++++++++++++++------------------
 1 file changed, 44 insertions(+), 26 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 87a507f12f2f..dfe6113a6a60 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -440,12 +440,45 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
+				     struct snd_pcm_hw_params *params)
+{
+	const struct snd_interval *i;
+	const struct snd_mask *m;
+	int err;
+
+	if (!params->msbits) {
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
+		if (snd_interval_single(i))
+			params->msbits = snd_interval_value(i);
+	}
+
+	if (!params->rate_den) {
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE);
+		if (snd_interval_single(i)) {
+			params->rate_num = snd_interval_value(i);
+			params->rate_den = 1;
+		}
+	}
+
+	if (!params->fifo_size) {
+		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
+		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS);
+		if (snd_mask_single(m) && snd_interval_single(i)) {
+			err = substream->ops->ioctl(substream,
+					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
+			if (err < 0)
+				return err;
+		}
+	}
+
+	return 0;
+}
+
 int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		      struct snd_pcm_hw_params *params)
 {
 	struct snd_pcm_hardware *hw;
-	const struct snd_interval *i;
-	const struct snd_mask *m;
 	int err;
 
 	params->info = 0;
@@ -469,20 +502,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	if (err < 0)
 		return err;
 
-	if (!params->msbits) {
-		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);
-		if (snd_interval_single(i))
-			params->msbits = snd_interval_value(i);
-	}
-
-	if (!params->rate_den) {
-		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE);
-		if (snd_interval_single(i)) {
-			params->rate_num = snd_interval_value(i);
-			params->rate_den = 1;
-		}
-	}
-
 	hw = &substream->runtime->hw;
 	if (!params->info) {
 		params->info = hw->info & ~(SNDRV_PCM_INFO_FIFO_IN_FRAMES |
@@ -491,16 +510,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 			params->info &= ~(SNDRV_PCM_INFO_MMAP |
 					  SNDRV_PCM_INFO_MMAP_VALID);
 	}
-	if (!params->fifo_size) {
-		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
-		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS);
-		if (snd_mask_single(m) && snd_interval_single(i)) {
-			err = substream->ops->ioctl(substream,
-					SNDRV_PCM_IOCTL1_FIFO_SIZE, params);
-			if (err < 0)
-				return err;
-		}
-	}
+
 	params->rmask = 0;
 	return 0;
 }
@@ -517,6 +527,8 @@ static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
 		return PTR_ERR(params);
 
 	err = snd_pcm_hw_refine(substream, params);
+	if (err >= 0)
+		err = fixup_unreferenced_params(substream, params);
 	if (copy_to_user(_params, params, sizeof(*params))) {
 		if (!err)
 			err = -EFAULT;
@@ -596,6 +608,10 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 	if (err < 0)
 		goto _error;
 
+	err = fixup_unreferenced_params(substream, params);
+	if (err < 0)
+		goto _error;
+
 	if (substream->ops->hw_params != NULL) {
 		err = substream->ops->hw_params(substream, params);
 		if (err < 0)
@@ -3621,6 +3637,8 @@ static int snd_pcm_hw_refine_old_user(struct snd_pcm_substream *substream,
 	}
 	snd_pcm_hw_convert_from_old_params(params, oparams);
 	err = snd_pcm_hw_refine(substream, params);
+	if (err >= 0)
+		err = fixup_unreferenced_params(substream, params);
 	snd_pcm_hw_convert_to_old_params(oparams, params);
 	if (copy_to_user(_oparams, oparams, sizeof(*oparams))) {
 		if (!err)
-- 
2.11.0

[PATCH 3/3] ALSA: pcm: move fixup of info flag after selecting single parameters

[Takashi Sakamoto]

When drivers register no flags about information of PCM hardware, ALSA
PCM core fixups it roughly. Currently, this operation places in a
function snd_pcm_hw_refine(). It can be moved to a function
fixup_unreferenced_params() because it doesn't affects operations
between these two functions.

This idea is better to bundle codes with similar purposes and this commit
achieves it.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/pcm_native.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index dfe6113a6a60..3293db0172db 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -472,13 +472,21 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
 		}
 	}
 
+	if (!params->info) {
+		params->info = substream->runtime->hw.info;
+		params->info &= ~(SNDRV_PCM_INFO_FIFO_IN_FRAMES |
+				  SNDRV_PCM_INFO_DRAIN_TRIGGER);
+		if (!hw_support_mmap(substream))
+			params->info &= ~(SNDRV_PCM_INFO_MMAP |
+					  SNDRV_PCM_INFO_MMAP_VALID);
+	}
+
 	return 0;
 }
 
 int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 		      struct snd_pcm_hw_params *params)
 {
-	struct snd_pcm_hardware *hw;
 	int err;
 
 	params->info = 0;
@@ -502,16 +510,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 	if (err < 0)
 		return err;
 
-	hw = &substream->runtime->hw;
-	if (!params->info) {
-		params->info = hw->info & ~(SNDRV_PCM_INFO_FIFO_IN_FRAMES |
-					    SNDRV_PCM_INFO_DRAIN_TRIGGER);
-		if (!hw_support_mmap(substream))
-			params->info &= ~(SNDRV_PCM_INFO_MMAP |
-					  SNDRV_PCM_INFO_MMAP_VALID);
-	}
-
 	params->rmask = 0;
+
 	return 0;
 }
 EXPORT_SYMBOL(snd_pcm_hw_refine);
-- 
2.11.0

Re: [PATCH 0/3] ALSA: pcm:

[Takashi Sakamoto]

Oops, I sent this patchset with cover-letter without good subject line, 
sorry...

On Jun 9 2017 09:34, Takashi Sakamoto wrote:
> This is a part of layout of the structure.
> 
> struct snd_pcm_hw_params {
>      ...
>      unsigned int info;
>      unsigned int msbits;
>      unsigned int rate_num;
>      unsigned int rate_den;
>      snd_pcm_uframes_t fifo_size;
>      ...
> };
> 
> For example, although the 'msbits', 'rate_num' and 'rate_den' fields are
> filled in a result of HW_REFINE command conditionally, they're never
> filled in a result of HW_PARAMS command. This seems a bug.

I note that there's a case for HW_PARAMS that the result includes proper 
values to these parameters. When drivers register constrain rules 
relevant to them, the members are filled.


Regards

Takashi Sakamoto

Re: [PATCH 0/3] ALSA: pcm:

[Takashi Iwai]

On Fri, 09 Jun 2017 02:34:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> This patchset is a revised version of my former RFC.
> 
> 
> [alsa-devel] [PATCH RFC 00/21] ALSA: pcm: add tracepoints for PCM params operation
> http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120548.html
> 
> In ALSA PCM interface, applications can get hardware capability as
> 'struct snd_pcm_hw_params' type of data returned by a call of ioctl(2)
> with SNDRV_PCM_IOCTL_HW_REFINE/SNDRV_PCM_IOCTL_HW_PARAMS' commands.
> Results of these commands should be the same in a certain conditions but
> actually it's not.
> 
> This is a part of layout of the structure.
> 
> struct snd_pcm_hw_params {
>     ...
>     unsigned int info;
>     unsigned int msbits;
>     unsigned int rate_num;
>     unsigned int rate_den;
>     snd_pcm_uframes_t fifo_size;
>     ...
> };
> 
> For example, although the 'msbits', 'rate_num' and 'rate_den' fields are
> filled in a result of HW_REFINE command conditionally, they're never
> filled in a result of HW_PARAMS command. This seems a bug.
> 
> This patchset attempts to fix the bug. This affects userspace applications
> in cotents of the result, but it's acceptrable with a merit for the
> applications to get enough parameters.
> 
> Takashi Sakamoto (3):
>   ALSA: pcm: use helper functions to refer parameters as constants
>   ALSA: pcm: calculate non-mask/non-interval parameters always when
>     possible
>   ALSA: pcm: move fixup of info flag after selecting single parameters

Applied all three patches now.  Thanks.


Takashi