[alsa-devel] [PATCH 0/3] ALSA: pcm:
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(-)
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);
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)
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);
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
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto