[alsa-devel] [PATCH v2 0/8] [PATCH 0/8] ALSA: pcm: code refactoring for snd_pcm_hw_refine()
Hi,
This patchset is revised version of my previous one.
[alsa-devel] [PATCH 0/8] ALSA: pcm: code refactoring for snd_pcm_hw_refine() http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121443.html
Changes from v1: - get rid of __may_unused keyword - remove subtle changes
Here, I repeat a description on my former message.
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. In kernel land, the data is prepared in 'snd_pcm_hw_refine()' in 'sound/core/pcm_native.c'. This patchset is a code refactoring to this function.
When applications execute open(2) to any PCM character device, runtime of corresponding PCM substream is created in kernel land. Then, the runtime gets information to configure the parameter data. When applications execute ioctl(2) with the above commands, the given parameter data is actually changed according to the information.
The parameter data structure includes several types of information. Major part of the parameter data is represented by two kinds of shape; mask ('struct snd_mask') and interval ('struct snd_interval'). 15 types of data is pre-defined as 'SNDRV_PCM_HW_PARAM_XXX' macros with the two types.
The runtime has several constraints, which is a kind of information to configure the parameters. There're two types of constraints; independent and dependent. The independent constrain is for each of single parameters. On the other hand, the dependent constrain is for each of combination between parameters, thus has specific structure, 'snd_pcm_hw_rule'. This structure describes relationships between several parameters.
In 'sound/core/pcm_native.c', the above constraints and rules are applied to parameter data for userspace. This processing is done in 'snd_pcm_hw_refine()'. In this patchset, I attempt to clean up the processing so that developers easily understand it.
This is a kind of refactoring, thus behaviour of the processing is not changed. Tracepoints added in my previous patchset is helpful for your test to check it. I wrote an application as low level program and it shall satisfies your interests about the above mechanism[1].
[1] refine-pcm-params.c https://github.com/takaswie/alsa-ioctl-test/
Takashi Sakamoto (8): ALSA: pcm: add a helper function to constrain mask-type parameters ALSA: pcm: add a helper function to constrain interval-type parameters ALSA: pcm: add a helper function to apply parameter rules ALSA: pcm: use goto statement instead of while statement to reduce indentation ALSA: pcm: remove function local variable with alternative evaluation ALSA: pcm: adaption of code formatting ALSA: pcm: use helper functions to check whether parameters are determined ALSA: pcm: add comment about application of rule to PCM parameters
sound/core/pcm_native.c | 241 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 169 insertions(+), 72 deletions(-)
Application of constraints to mask-type parameters for PCM substream is done in a call of snd_pcm_hw_refine(), while the function includes much codes and is not enough friendly to readers.
This commit splits the codes to a separated function so that readers can get it easily. I leave desicion into compilers to merge the function into its callee.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 56 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 19 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2bde07a4a87f..1dee5f960fbe 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -253,6 +253,39 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream) return true; }
+static int constrain_mask_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_pcm_hw_constraints *constrs = + &substream->runtime->hw_constraints; + struct snd_mask *m; + unsigned int k; + struct snd_mask old_mask; + int changed; + + for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { + m = hw_param_mask(params, k); + if (snd_mask_empty(m)) + return -EINVAL; + if (!(params->rmask & (1 << k))) + continue; + + if (trace_hw_mask_param_enabled()) + old_mask = *m; + + changed = snd_mask_refine(m, constrs_mask(constrs, k)); + + trace_hw_mask_param(substream, k, 0, &old_mask, m); + + if (changed) + params->cmask |= 1 << k; + if (changed < 0) + return changed; + } + + return 0; +} + int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -265,6 +298,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; unsigned int stamp = 2; int changed, again; + int err;
struct snd_mask __maybe_unused old_mask; struct snd_interval __maybe_unused old_interval; @@ -278,25 +312,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rate_den = 0; }
- for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { - m = hw_param_mask(params, k); - if (snd_mask_empty(m)) - return -EINVAL; - if (!(params->rmask & (1 << k))) - continue; - - if (trace_hw_mask_param_enabled()) - old_mask = *m; - - changed = snd_mask_refine(m, constrs_mask(constrs, k)); - - trace_hw_mask_param(substream, k, 0, &old_mask, m); - - if (changed) - params->cmask |= 1 << k; - if (changed < 0) - return changed; - } + err = constrain_mask_params(substream, params); + if (err < 0) + return err;
for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { i = hw_param_interval(params, k);
Application of constraints to interval-type parameters for PCM substream is done in a call of snd_pcm_hw_refine(), while the function includes much codes and is not enough friendly to readers.
This commit splits the codes to a separated function so that readers can get it easily. I leave desicion into compilers to merge the function into its callee.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 55 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 19 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 1dee5f960fbe..7e811ace6bf2 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -286,6 +286,39 @@ static int constrain_mask_params(struct snd_pcm_substream *substream, return 0; }
+static int constrain_interval_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_pcm_hw_constraints *constrs = + &substream->runtime->hw_constraints; + struct snd_interval *i; + unsigned int k; + struct snd_interval old_interval; + int changed; + + for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { + i = hw_param_interval(params, k); + if (snd_interval_empty(i)) + return -EINVAL; + if (!(params->rmask & (1 << k))) + continue; + + if (trace_hw_interval_param_enabled()) + old_interval = *i; + + changed = snd_interval_refine(i, constrs_interval(constrs, k)); + + trace_hw_interval_param(substream, k, 0, &old_interval, i); + + if (changed) + params->cmask |= 1 << k; + if (changed < 0) + return changed; + } + + return 0; +} + int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -316,25 +349,9 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, if (err < 0) return err;
- for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { - i = hw_param_interval(params, k); - if (snd_interval_empty(i)) - return -EINVAL; - if (!(params->rmask & (1 << k))) - continue; - - if (trace_hw_interval_param_enabled()) - old_interval = *i; - - changed = snd_interval_refine(i, constrs_interval(constrs, k)); - - trace_hw_interval_param(substream, k, 0, &old_interval, i); - - if (changed) - params->cmask |= 1 << k; - if (changed < 0) - return changed; - } + err = constrain_interval_params(substream, params); + if (err < 0) + return err;
for (k = 0; k < constrs->rules_num; k++) rstamps[k] = 0;
Application of rules to parameters of PCM substream is done in a call of snd_pcm_hw_refine(), while the function includes much codes and is not enough friendly to readers.
This commit splits the codes to a separated function so that readers can get it easily. I leave desicion into compilers to merge the function into its callee.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 77 +++++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7e811ace6bf2..000e6e9a0c2b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -319,43 +319,23 @@ static int constrain_interval_params(struct snd_pcm_substream *substream, return 0; }
-int snd_pcm_hw_refine(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) +static int constrain_params_by_rules(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) { + struct snd_pcm_hw_constraints *constrs = + &substream->runtime->hw_constraints; unsigned int k; - struct snd_pcm_hardware *hw; - struct snd_interval *i = NULL; - struct snd_mask *m = NULL; - struct snd_pcm_hw_constraints *constrs = &substream->runtime->hw_constraints; unsigned int rstamps[constrs->rules_num]; unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; unsigned int stamp = 2; - int changed, again; - int err; - - struct snd_mask __maybe_unused old_mask; - struct snd_interval __maybe_unused old_interval; - - params->info = 0; - params->fifo_size = 0; - if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS)) - params->msbits = 0; - if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) { - params->rate_num = 0; - params->rate_den = 0; - } - - err = constrain_mask_params(substream, params); - if (err < 0) - return err; - - err = constrain_interval_params(substream, params); - if (err < 0) - return err; + struct snd_mask old_mask; + struct snd_interval old_interval; + int again; + int changed;
for (k = 0; k < constrs->rules_num; k++) rstamps[k] = 0; - for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) + for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0; do { again = 0; @@ -405,6 +385,39 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, stamp++; } } while (again); + + return 0; +} + +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; + int err; + + params->info = 0; + params->fifo_size = 0; + if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS)) + params->msbits = 0; + if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) { + params->rate_num = 0; + params->rate_den = 0; + } + + err = constrain_mask_params(substream, params); + if (err < 0) + return err; + + err = constrain_interval_params(substream, params); + if (err < 0) + return err; + + err = constrain_params_by_rules(substream, params); + if (err < 0) + return err; + if (!params->msbits) { i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS); if (snd_interval_single(i)) @@ -432,10 +445,10 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); if (snd_mask_min(m) == snd_mask_max(m) && snd_interval_min(i) == snd_interval_max(i)) { - changed = substream->ops->ioctl(substream, + err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_FIFO_SIZE, params); - if (changed < 0) - return changed; + if (err < 0) + return err; } } params->rmask = 0;
In a process to calculate parameters of PCM substream, application of all rules is iterated several times till parameter dependencies are satisfied. In current implementation, two loops are used for the design, however this brings two-level indentation and decline readability.
This commit attempts to reduce the indentation by using goto statement, instead of outer while loop.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 86 +++++++++++++++++++++++++------------------------ 1 file changed, 44 insertions(+), 42 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 000e6e9a0c2b..41aeb6facdec 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -337,54 +337,56 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, rstamps[k] = 0; for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0; - do { - again = 0; - for (k = 0; k < constrs->rules_num; k++) { - struct snd_pcm_hw_rule *r = &constrs->rules[k]; - unsigned int d; - int doit = 0; - if (r->cond && !(r->cond & params->flags)) - continue; - for (d = 0; r->deps[d] >= 0; d++) { - if (vstamps[r->deps[d]] > rstamps[k]) { - doit = 1; - break; - } +retry: + again = 0; + for (k = 0; k < constrs->rules_num; k++) { + struct snd_pcm_hw_rule *r = &constrs->rules[k]; + unsigned int d; + int doit = 0; + if (r->cond && !(r->cond & params->flags)) + continue; + for (d = 0; r->deps[d] >= 0; d++) { + if (vstamps[r->deps[d]] > rstamps[k]) { + doit = 1; + break; } - if (!doit) - continue; + } + if (!doit) + continue;
- if (trace_hw_mask_param_enabled()) { - if (hw_is_mask(r->var)) - old_mask = *hw_param_mask(params, r->var); - } - if (trace_hw_interval_param_enabled()) { - if (hw_is_interval(r->var)) - old_interval = *hw_param_interval(params, r->var); - } + if (trace_hw_mask_param_enabled()) { + if (hw_is_mask(r->var)) + old_mask = *hw_param_mask(params, r->var); + } + if (trace_hw_interval_param_enabled()) { + if (hw_is_interval(r->var)) + old_interval = *hw_param_interval(params, r->var); + }
- changed = r->func(params, r); + changed = r->func(params, r);
- if (hw_is_mask(r->var)) { - trace_hw_mask_param(substream, r->var, k + 1, - &old_mask, hw_param_mask(params, r->var)); - } - if (hw_is_interval(r->var)) { - trace_hw_interval_param(substream, r->var, k + 1, - &old_interval, hw_param_interval(params, r->var)); - } + if (hw_is_mask(r->var)) { + trace_hw_mask_param(substream, r->var, k + 1, + &old_mask, hw_param_mask(params, r->var)); + } + if (hw_is_interval(r->var)) { + trace_hw_interval_param(substream, r->var, k + 1, + &old_interval, hw_param_interval(params, r->var)); + }
- rstamps[k] = stamp; - if (changed && r->var >= 0) { - params->cmask |= (1 << r->var); - vstamps[r->var] = stamp; - again = 1; - } - if (changed < 0) - return changed; - stamp++; + rstamps[k] = stamp; + if (changed && r->var >= 0) { + params->cmask |= (1 << r->var); + vstamps[r->var] = stamp; + again = 1; } - } while (again); + if (changed < 0) + return changed; + stamp++; + } + + if (again) + goto retry;
return 0; }
A local variable is used to judge whether a parameter should be handled due to reverse dependency of the other rules. However, this can be obsoleted by check of a sentinel in dependency array.
This commit removes the local variable and check the sentinel to reduce stack usage.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 41aeb6facdec..db4cdd114ed4 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -342,16 +342,13 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, for (k = 0; k < constrs->rules_num; k++) { struct snd_pcm_hw_rule *r = &constrs->rules[k]; unsigned int d; - int doit = 0; if (r->cond && !(r->cond & params->flags)) continue; for (d = 0; r->deps[d] >= 0; d++) { - if (vstamps[r->deps[d]] > rstamps[k]) { - doit = 1; + if (vstamps[r->deps[d]] > rstamps[k]) break; - } } - if (!doit) + if (r->deps[d] < 0) continue;
if (trace_hw_mask_param_enabled()) {
This commit modifies current for readability in below aspects: - use bool type variable instead of int type variable assigned to 0/1 - move variable definition from loop to top of the function definition
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 db4cdd114ed4..40560e579d33 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -328,9 +328,11 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, unsigned int rstamps[constrs->rules_num]; unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; unsigned int stamp = 2; + struct snd_pcm_hw_rule *r; + unsigned int d; struct snd_mask old_mask; struct snd_interval old_interval; - int again; + bool again; int changed;
for (k = 0; k < constrs->rules_num; k++) @@ -338,10 +340,9 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0; retry: - again = 0; + again = false; for (k = 0; k < constrs->rules_num; k++) { - struct snd_pcm_hw_rule *r = &constrs->rules[k]; - unsigned int d; + r = &constrs->rules[k]; if (r->cond && !(r->cond & params->flags)) continue; for (d = 0; r->deps[d] >= 0; d++) { @@ -375,7 +376,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, if (changed && r->var >= 0) { params->cmask |= (1 << r->var); vstamps[r->var] = stamp; - again = 1; + again = true; } if (changed < 0) return changed; @@ -453,7 +454,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rmask = 0; return 0; } - EXPORT_SYMBOL(snd_pcm_hw_refine);
static int snd_pcm_hw_refine_user(struct snd_pcm_substream *substream,
A commit 8bea869c5e56 ("ALSA: PCM midlevel: improve fifo_size handling") allows drivers to implement calculation of fifo size in parameter structure. This calculation runs only when two of the other parameters have single value.
In ALSA PCM core, there're some helper functions for the case. This commit applies the functions instead of value comparison.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 40560e579d33..80275aa0bcca 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -443,8 +443,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, if (!params->fifo_size) { m = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - if (snd_mask_min(m) == snd_mask_max(m) && - snd_interval_min(i) == snd_interval_max(i)) { + if (snd_mask_single(m) && snd_interval_single(i)) { err = substream->ops->ioctl(substream, SNDRV_PCM_IOCTL1_FIFO_SIZE, params); if (err < 0)
Drivers add rules of parameters to runtime of PCM substream, when applications open ALSA PCM character device. When applications call ioctl(2) with SNDRV_PCM_IOCTL_HW_REFINE or SNDRV_PCM_IOCTL_HW_PARAMS, the rules are applied to the parameters and return the result to user space.
The rule can have dependency between parameters. Additionally, it can have condition flags about application of rules. Userspace applications can indicate the flags to suppress change of parameters.
This commit attempts to describe the mechanism.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 80275aa0bcca..422ee4629698 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -267,6 +267,8 @@ static int constrain_mask_params(struct snd_pcm_substream *substream, m = hw_param_mask(params, k); if (snd_mask_empty(m)) return -EINVAL; + + /* This parameter is not requested to change by a caller. */ if (!(params->rmask & (1 << k))) continue;
@@ -277,6 +279,7 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,
trace_hw_mask_param(substream, k, 0, &old_mask, m);
+ /* Set corresponding flag so that the caller gets it. */ if (changed) params->cmask |= 1 << k; if (changed < 0) @@ -300,6 +303,8 @@ static int constrain_interval_params(struct snd_pcm_substream *substream, i = hw_param_interval(params, k); if (snd_interval_empty(i)) return -EINVAL; + + /* This parameter is not requested to change by a caller. */ if (!(params->rmask & (1 << k))) continue;
@@ -310,6 +315,7 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,
trace_hw_interval_param(substream, k, 0, &old_interval, i);
+ /* Set corresponding flag so that the caller gets it. */ if (changed) params->cmask |= 1 << k; if (changed < 0) @@ -327,7 +333,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, unsigned int k; unsigned int rstamps[constrs->rules_num]; unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; - unsigned int stamp = 2; + unsigned int stamp; struct snd_pcm_hw_rule *r; unsigned int d; struct snd_mask old_mask; @@ -335,16 +341,54 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, bool again; int changed;
+ /* + * Each application of rule has own sequence number. + * + * Each member of 'rstamps' array represents the sequence number of + * recent application of corresponding rule. + */ for (k = 0; k < constrs->rules_num; k++) rstamps[k] = 0; + + /* + * Each member of 'vstamps' array represents the sequence number of + * recent application of rule in which corresponding parameters were + * changed. + * + * In initial state, elements corresponding to parameters requested by + * a caller is 1. For unrequested parameters, corresponding members + * have 0 so that the parameters are never changed anymore. + */ for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) vstamps[k] = (params->rmask & (1 << k)) ? 1 : 0; + + /* Due to the above design, actual sequence number starts at 2. */ + stamp = 2; retry: + /* Apply all rules in order. */ again = false; for (k = 0; k < constrs->rules_num; k++) { r = &constrs->rules[k]; + + /* + * Check condition bits of this rule. When the rule has + * some condition bits, parameter without the bits is + * never processed. SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP + * is an example of the condition bits. + */ if (r->cond && !(r->cond & params->flags)) continue; + + /* + * The 'deps' array includes maximum three dependencies + * to SNDRV_PCM_HW_PARAM_XXXs for this rule. The fourth + * member of this array is a sentinel and should be + * negative value. + * + * This rule should be processed in this time when dependent + * parameters were changed at former applications of the other + * rules. + */ for (d = 0; r->deps[d] >= 0; d++) { if (vstamps[r->deps[d]] > rstamps[k]) break; @@ -373,6 +417,12 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, }
rstamps[k] = stamp; + + /* + * When the parameters is changed, notify it to the caller + * by corresponding returned bit, then preparing for next + * iteration. + */ if (changed && r->var >= 0) { params->cmask |= (1 << r->var); vstamps[r->var] = stamp; @@ -383,6 +433,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, stamp++; }
+ /* Iterate to evaluate all rules till no parameters are changed. */ if (again) goto retry;
On Thu, 08 Jun 2017 23:36:58 +0200, Takashi Sakamoto wrote:
Hi,
This patchset is revised version of my previous one.
[alsa-devel] [PATCH 0/8] ALSA: pcm: code refactoring for snd_pcm_hw_refine() http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121443.html
Changes from v1:
- get rid of __may_unused keyword
- remove subtle changes
Here, I repeat a description on my former message.
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. In kernel land, the data is prepared in 'snd_pcm_hw_refine()' in 'sound/core/pcm_native.c'. This patchset is a code refactoring to this function.
When applications execute open(2) to any PCM character device, runtime of corresponding PCM substream is created in kernel land. Then, the runtime gets information to configure the parameter data. When applications execute ioctl(2) with the above commands, the given parameter data is actually changed according to the information.
The parameter data structure includes several types of information. Major part of the parameter data is represented by two kinds of shape; mask ('struct snd_mask') and interval ('struct snd_interval'). 15 types of data is pre-defined as 'SNDRV_PCM_HW_PARAM_XXX' macros with the two types.
The runtime has several constraints, which is a kind of information to configure the parameters. There're two types of constraints; independent and dependent. The independent constrain is for each of single parameters. On the other hand, the dependent constrain is for each of combination between parameters, thus has specific structure, 'snd_pcm_hw_rule'. This structure describes relationships between several parameters.
In 'sound/core/pcm_native.c', the above constraints and rules are applied to parameter data for userspace. This processing is done in 'snd_pcm_hw_refine()'. In this patchset, I attempt to clean up the processing so that developers easily understand it.
This is a kind of refactoring, thus behaviour of the processing is not changed. Tracepoints added in my previous patchset is helpful for your test to check it. I wrote an application as low level program and it shall satisfies your interests about the above mechanism[1].
[1] refine-pcm-params.c https://github.com/takaswie/alsa-ioctl-test/
Takashi Sakamoto (8): ALSA: pcm: add a helper function to constrain mask-type parameters ALSA: pcm: add a helper function to constrain interval-type parameters ALSA: pcm: add a helper function to apply parameter rules ALSA: pcm: use goto statement instead of while statement to reduce indentation ALSA: pcm: remove function local variable with alternative evaluation ALSA: pcm: adaption of code formatting ALSA: pcm: use helper functions to check whether parameters are determined ALSA: pcm: add comment about application of rule to PCM parameters
sound/core/pcm_native.c | 241 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 169 insertions(+), 72 deletions(-)
Now looks better, and I merged all eight patches. Thanks!
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto