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 | 60 ++++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index e5135e6..b828e94 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -308,31 +308,12 @@ static int constrain_mask_params(struct snd_pcm_hw_constraints *constrs, return 0; }

-int snd_pcm_hw_refine(struct snd_pcm_substream *substream, - struct snd_pcm_hw_params *params) +static int constrain_interval_params(struct snd_pcm_hw_constraints *constrs, + struct snd_pcm_hw_params *params) { + struct snd_interval *i; unsigned int k; - struct snd_pcm_hardware *hw; - struct snd_mask *m = NULL; - struct snd_interval *i = 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; - - 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; - } - - changed = constrain_mask_params(constrs, params); - if (changed < 0) - return changed; + int changed;

for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { i = hw_param_interval(params, k); @@ -365,6 +346,39 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, return changed; }

+ return 0; +} + +int snd_pcm_hw_refine(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + unsigned int k; + struct snd_pcm_hardware *hw; + struct snd_mask *m = NULL; + struct snd_interval *i = 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; + + 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; + } + + changed = constrain_mask_params(constrs, params); + if (changed < 0) + return changed; + + changed = constrain_interval_params(constrs, params); + if (changed < 0) + return changed; + for (k = 0; k < constrs->rules_num; k++) rstamps[k] = 0; for (k = 0; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++)

When working for devices which have complicated mode for its data transmission or which consists of several components, developers are likely to use rules of parameters of PCM substream. However, there's no infrastructure to assist their work.

In old days, ALSA PCM core got a local 'RULES_DEBUG' macro to debug refinement of parameters for PCM substream. Although this is merely a makeshift, with some modifications, we get the infrastructure.

This commit is for the purpose. Refinement of mask/interval type of PCM parameters is recorded as tracepoint events as 'hw_params_mask' and 'hw_params_interval' on existent 'snd_pcm' subsystem.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/Makefile | 1 + sound/core/pcm_native.c | 146 ++++++++++++++++-------------------------- sound/core/pcm_params_trace.h | 142 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 199 insertions(+), 90 deletions(-) create mode 100644 sound/core/pcm_params_trace.h

diff --git a/sound/core/Makefile b/sound/core/Makefile index e85d9dd..a8514b3 100644 --- a/sound/core/Makefile +++ b/sound/core/Makefile @@ -22,6 +22,7 @@ snd-pcm-$(CONFIG_SND_PCM_IEC958) += pcm_iec958.o

# for trace-points CFLAGS_pcm_lib.o := -I$(src) +CFLAGS_pcm_native.o := -I$(src)

snd-pcm-dmaengine-objs := pcm_dmaengine.o

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 78afdf1..5a2703e 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -37,6 +37,9 @@ #include <sound/minors.h> #include <linux/uio.h>

+#define CREATE_TRACE_POINTS +#include "pcm_params_trace.h" + /* * Compatibility */ @@ -255,50 +258,31 @@ static bool hw_support_mmap(struct snd_pcm_substream *substream) return true; }

-#undef RULES_DEBUG - -#ifdef RULES_DEBUG -#define HW_PARAM(v) [SNDRV_PCM_HW_PARAM_##v] = #v -static const char * const snd_pcm_hw_param_names[] = { - HW_PARAM(ACCESS), - HW_PARAM(FORMAT), - HW_PARAM(SUBFORMAT), - HW_PARAM(SAMPLE_BITS), - HW_PARAM(FRAME_BITS), - HW_PARAM(CHANNELS), - HW_PARAM(RATE), - HW_PARAM(PERIOD_TIME), - HW_PARAM(PERIOD_SIZE), - HW_PARAM(PERIOD_BYTES), - HW_PARAM(PERIODS), - HW_PARAM(BUFFER_TIME), - HW_PARAM(BUFFER_SIZE), - HW_PARAM(BUFFER_BYTES), - HW_PARAM(TICK_TIME), -}; -#endif - -static int constrain_mask_params(struct snd_pcm_hw_constraints *constrs, +static int constrain_mask_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_constraints *constrs, struct snd_pcm_hw_params *params) { struct snd_mask *m; unsigned int k; int changed;

+ struct snd_mask __maybe_unused old_mask; + 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; -#ifdef RULES_DEBUG - pr_debug("%s = ", snd_pcm_hw_param_names[k]); - pr_cont("%04x%04x%04x%04x -> ", m->bits[3], m->bits[2], m->bits[1], m->bits[0]); -#endif + + /* Keep the parameter to trace. */ + if (trace_hw_params_mask_enabled()) + old_mask = *m; + changed = snd_mask_refine(m, constrs_mask(constrs, k)); -#ifdef RULES_DEBUG - pr_cont("%04x%04x%04x%04x\n", m->bits[3], m->bits[2], m->bits[1], m->bits[0]); -#endif + + trace_hw_params_mask(substream, k, -1, &old_mask, m); + if (changed) params->cmask |= 1 << k; if (changed < 0) @@ -308,38 +292,31 @@ static int constrain_mask_params(struct snd_pcm_hw_constraints *constrs, return 0; }

-static int constrain_interval_params(struct snd_pcm_hw_constraints *constrs, +static int constrain_interval_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_constraints *constrs, struct snd_pcm_hw_params *params) { struct snd_interval *i; unsigned int k; int changed;

+ struct snd_interval __maybe_unused old_interval; + 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; -#ifdef RULES_DEBUG - pr_debug("%s = ", snd_pcm_hw_param_names[k]); - if (i->empty) - pr_cont("empty"); - else - pr_cont("%c%u %u%c", - i->openmin ? '(' : '[', i->min, - i->max, i->openmax ? ')' : ']'); - pr_cont(" -> "); -#endif + + /* Keep the parameter to trace. */ + if (trace_hw_params_interval_enabled()) + old_interval = *i; + changed = snd_interval_refine(i, constrs_interval(constrs, k)); -#ifdef RULES_DEBUG - if (i->empty) - pr_cont("empty\n"); - else - pr_cont("%c%u %u%c\n", - i->openmin ? '(' : '[', i->min, - i->max, i->openmax ? ')' : ']'); -#endif + + trace_hw_params_interval(substream, k, -1, &old_interval, i); + if (changed) params->cmask |= 1 << k; if (changed < 0) @@ -349,7 +326,8 @@ static int constrain_interval_params(struct snd_pcm_hw_constraints *constrs, return 0; }

-static int constrain_params_by_rules(struct snd_pcm_hw_constraints *constrs, +static int constrain_params_by_rules(struct snd_pcm_substream *substream, + struct snd_pcm_hw_constraints *constrs, struct snd_pcm_hw_params *params) { unsigned int k; @@ -358,10 +336,8 @@ static int constrain_params_by_rules(struct snd_pcm_hw_constraints *constrs, unsigned int stamp = 2; int changed, again;

-#ifdef RULES_DEBUG - struct snd_mask *m = NULL; - struct snd_interval *i = NULL; -#endif + struct snd_interval __maybe_unused old_interval; + struct snd_mask __maybe_unused old_mask;

for (k = 0; k < constrs->rules_num; k++) rstamps[k] = 0; @@ -383,41 +359,31 @@ static int constrain_params_by_rules(struct snd_pcm_hw_constraints *constrs, } if (!doit) continue; -#ifdef RULES_DEBUG - pr_debug("Rule %d [%p]: ", k, r->func); - if (r->var >= 0) { - pr_cont("%s = ", snd_pcm_hw_param_names[r->var]); - if (hw_is_mask(r->var)) { - m = hw_param_mask(params, r->var); - pr_cont("%x", *m->bits); - } else { - i = hw_param_interval(params, r->var); - if (i->empty) - pr_cont("empty"); - else - pr_cont("%c%u %u%c", - i->openmin ? '(' : '[', i->min, - i->max, i->openmax ? ')' : ']'); - } + + /* Keep old parameter to trace. */ + if (trace_hw_params_mask_enabled()) { + if (hw_is_mask(r->var)) + old_mask = *hw_param_mask(params, r->var); } -#endif + if (trace_hw_params_interval_enabled()) { + if (hw_is_interval(r->var)) + old_interval = *hw_param_interval(params, r->var); + } + changed = r->func(params, r); -#ifdef RULES_DEBUG - if (r->var >= 0) { - pr_cont(" -> "); - if (hw_is_mask(r->var)) - pr_cont("%x", *m->bits); - else { - if (i->empty) - pr_cont("empty"); - else - pr_cont("%c%u %u%c", - i->openmin ? '(' : '[', i->min, - i->max, i->openmax ? ')' : ']'); - } + + /* Trace the parameter. */ + if (hw_is_mask(r->var)) { + trace_hw_params_mask(substream, r->var, k, + &old_mask, + hw_param_mask(params, r->var)); } - pr_cont("\n"); -#endif + if (hw_is_interval(r->var)) { + trace_hw_params_interval(substream, r->var, k, + &old_interval, + hw_param_interval(params, r->var)); + } + rstamps[k] = stamp; if (changed && r->var >= 0) { params->cmask |= (1 << r->var); @@ -451,15 +417,15 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rate_den = 0; }

- changed = constrain_mask_params(constrs, params); + changed = constrain_mask_params(substream, constrs, params); if (changed < 0) return changed;

- changed = constrain_interval_params(constrs, params); + changed = constrain_interval_params(substream, constrs, params); if (changed < 0) return changed;

- changed = constrain_params_by_rules(constrs, params); + changed = constrain_params_by_rules(substream, constrs, params); if (changed < 0) return changed;

diff --git a/sound/core/pcm_params_trace.h b/sound/core/pcm_params_trace.h new file mode 100644 index 0000000..53a2c0f --- /dev/null +++ b/sound/core/pcm_params_trace.h @@ -0,0 +1,142 @@ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM snd_pcm + +#if !defined(_PCM_PARAMS_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) +#define _PCM_PARAMS_TRACE_H + +#include <linux/tracepoint.h> + +#define HW_PARAM_ENTRY(param) {SNDRV_PCM_HW_PARAM_##param, #param} +#define hw_param_labels \ + HW_PARAM_ENTRY(ACCESS), \ + HW_PARAM_ENTRY(FORMAT), \ + HW_PARAM_ENTRY(SUBFORMAT), \ + HW_PARAM_ENTRY(SAMPLE_BITS), \ + HW_PARAM_ENTRY(FRAME_BITS), \ + HW_PARAM_ENTRY(CHANNELS), \ + HW_PARAM_ENTRY(RATE), \ + HW_PARAM_ENTRY(PERIOD_TIME), \ + HW_PARAM_ENTRY(PERIOD_SIZE), \ + HW_PARAM_ENTRY(PERIOD_BYTES), \ + HW_PARAM_ENTRY(PERIODS), \ + HW_PARAM_ENTRY(BUFFER_TIME), \ + HW_PARAM_ENTRY(BUFFER_SIZE), \ + HW_PARAM_ENTRY(BUFFER_BYTES), \ + HW_PARAM_ENTRY(TICK_TIME) + +TRACE_EVENT(hw_params_mask, + TP_PROTO(struct snd_pcm_substream *substream, snd_pcm_hw_param_t type, int index, const struct snd_mask *prev, const struct snd_mask *curr), + TP_ARGS(substream, type, index, prev, curr), + TP_STRUCT__entry( + __field(int, card) + __field(int, device) + __field(int, subdevice) + __field(int, direction) + __field(snd_pcm_hw_param_t, type) + __field(int, index) + __field(int, total) + __array(__u32, prev_bits, 8) + __array(__u32, curr_bits, 8) + ), + TP_fast_assign( + __entry->card = substream->pcm->card->number; + __entry->device = substream->pcm->device; + __entry->subdevice = substream->number; + __entry->direction = substream->stream; + __entry->type = type; + __entry->index = index; + __entry->total = substream->runtime->hw_constraints.rules_num; + memcpy(__entry->prev_bits, prev->bits, sizeof(__u32) * 8); + memcpy(__entry->curr_bits, curr->bits, sizeof(__u32) * 8); + ), + TP_printk("%d,%d,%d,%d: %03d/%03d: %08x%08x%08x%08x: %08x%08x%08x%08x: %s", + __entry->card, + __entry->device, + __entry->subdevice, + __entry->direction, + __entry->index, + __entry->total, + __entry->prev_bits[3], __entry->prev_bits[2], + __entry->prev_bits[1], __entry->prev_bits[0], + __entry->curr_bits[3], __entry->curr_bits[2], + __entry->curr_bits[1], __entry->curr_bits[0], + __print_symbolic(__entry->type, hw_param_labels) + ) +); + +TRACE_EVENT(hw_params_interval, + TP_PROTO(struct snd_pcm_substream *substream, snd_pcm_hw_param_t type, int index, const struct snd_interval *prev, const struct snd_interval *curr), + TP_ARGS(substream, type, index, prev, curr), + TP_STRUCT__entry( + __field(int, card) + __field(int, device) + __field(int, subdevice) + __field(int, direction) + __field(snd_pcm_hw_param_t, type) + __field(int, index) + __field(int, total) + __field(unsigned int, prev_min) + __field(unsigned int, prev_max) + __field(unsigned int, prev_openmin) + __field(unsigned int, prev_openmax) + __field(unsigned int, prev_integer) + __field(unsigned int, prev_empty) + __field(unsigned int, curr_min) + __field(unsigned int, curr_max) + __field(unsigned int, curr_openmin) + __field(unsigned int, curr_openmax) + __field(unsigned int, curr_integer) + __field(unsigned int, curr_empty) + ), + TP_fast_assign( + __entry->card = substream->pcm->card->number; + __entry->device = substream->pcm->device; + __entry->subdevice = substream->number; + __entry->direction = substream->stream; + __entry->type = type; + __entry->index = index; + __entry->total = substream->runtime->hw_constraints.rules_num; + __entry->prev_min = prev->min; + __entry->prev_max = prev->max; + __entry->prev_openmin = prev->openmin; + __entry->prev_openmax = prev->openmax; + __entry->prev_integer = prev->integer; + __entry->prev_empty = prev->empty; + __entry->curr_min = curr->min; + __entry->curr_max = curr->max; + __entry->curr_openmin = curr->openmin; + __entry->curr_openmax = curr->openmax; + __entry->curr_integer = curr->integer; + __entry->curr_empty = curr->empty; + ), + TP_printk("%d,%d,%d,%d: %03d/%03d: %d %s%u, %u%s %d: %d %s%u, %u%s %d: %s", + __entry->card, + __entry->device, + __entry->subdevice, + __entry->direction, + __entry->index, + __entry->total, + __entry->prev_empty, + __entry->prev_openmin ? "(" : "[", + __entry->prev_min, + __entry->prev_max, + __entry->prev_openmax ? ")" : "]", + __entry->prev_integer, + __entry->curr_empty, + __entry->curr_openmin ? "(" : "[", + __entry->curr_min, + __entry->curr_max, + __entry->curr_openmax ? ")" : "]", + __entry->curr_integer, + __print_symbolic(__entry->type, hw_param_labels) + ) +); + +#endif /* _PCM_PARAMS_TRACE_H */ + +/* This part must be outside protection */ +#undef TRACE_INCLUDE_PATH +#define TRACE_INCLUDE_PATH . +#undef TRACE_INCLUDE_FILE +#define TRACE_INCLUDE_FILE pcm_params_trace +#include <trace/define_trace.h>

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 | 95 +++++++++++++++++++++++++------------------------ 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 3fd2a63..4de9498 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -345,58 +345,61 @@ 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; - } - } - if (!doit) - continue;

- /* Keep old parameter to trace. */ - if (trace_hw_params_mask_enabled()) { - if (hw_is_mask(r->var)) - old_mask = *hw_param_mask(params, r->var); - } - if (trace_hw_params_interval_enabled()) { - if (hw_is_interval(r->var)) - old_interval = *hw_param_interval(params, r->var); +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;

- changed = r->func(params, r); + /* Keep old parameter to trace. */ + if (trace_hw_params_mask_enabled()) { + if (hw_is_mask(r->var)) + old_mask = *hw_param_mask(params, r->var); + } + if (trace_hw_params_interval_enabled()) { + if (hw_is_interval(r->var)) + old_interval = *hw_param_interval(params, r->var); + }

- /* Trace the parameter. */ - if (hw_is_mask(r->var)) { - trace_hw_params_mask(substream, r->var, k, - &old_mask, - hw_param_mask(params, r->var)); - } - if (hw_is_interval(r->var)) { - trace_hw_params_interval(substream, r->var, k, - &old_interval, - hw_param_interval(params, r->var)); - } + changed = r->func(params, r);

- 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++; + /* Trace the parameter. */ + if (hw_is_mask(r->var)) { + trace_hw_params_mask(substream, r->var, k, + &old_mask, + hw_param_mask(params, r->var)); + } + if (hw_is_interval(r->var)) { + trace_hw_params_interval(substream, r->var, k, + &old_interval, + hw_param_interval(params, r->var)); } - } while (again); + + 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++; + } + + if (again) + goto retry;

return 0; }

This commit modifies current code according to fashion in kernel land: - within 80 characters per line - indentation of function arguments with line break - removal of trailing linear white spaces - use int type variable for loop counter

For readability: - 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 | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 39ead67..f644fbe 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -265,12 +265,13 @@ static int constrain_mask_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_mask *m; - unsigned int k; + int k; int changed;

struct snd_mask __maybe_unused old_mask;

- for (k = SNDRV_PCM_HW_PARAM_FIRST_MASK; k <= SNDRV_PCM_HW_PARAM_LAST_MASK; k++) { + 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; @@ -299,12 +300,13 @@ static int constrain_interval_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { struct snd_interval *i; - unsigned int k; + int k; int changed;

struct snd_interval __maybe_unused old_interval;

- for (k = SNDRV_PCM_HW_PARAM_FIRST_INTERVAL; k <= SNDRV_PCM_HW_PARAM_LAST_INTERVAL; k++) { + 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; @@ -332,25 +334,27 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, struct snd_pcm_hw_constraints *constrs, struct snd_pcm_hw_params *params) { - unsigned int k; + int k; unsigned int rstamps[constrs->rules_num]; unsigned int vstamps[SNDRV_PCM_HW_PARAM_LAST_INTERVAL + 1]; - unsigned int stamp = 2; - int changed, again; + unsigned int stamp; + struct snd_pcm_hw_rule *r; + unsigned int d; + bool again; + int changed;

struct snd_interval __maybe_unused old_interval; struct snd_mask __maybe_unused old_mask;

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; - + stamp = 2; 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++) { @@ -388,7 +392,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; @@ -407,7 +411,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, struct snd_pcm_hardware *hw; struct snd_mask *m = NULL; struct snd_interval *i = NULL; - struct snd_pcm_hw_constraints *constrs = &substream->runtime->hw_constraints; + struct snd_pcm_hw_constraints *constrs; int changed;

params->info = 0; @@ -419,6 +423,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, params->rate_den = 0; }

+ constrs = &substream->runtime->hw_constraints; changed = constrain_mask_params(substream, constrs, params); if (changed < 0) return changed;

In a function snd_pcm_hw_params_choose(), target parameters are arranged into a table. Though each entry of this table is read-only, they don't have const qualifier.

This commit adds the qualifier.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_lib.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 5088d4b..0ab6f86 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1733,7 +1733,7 @@ EXPORT_SYMBOL(snd_pcm_hw_param_last); int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, struct snd_pcm_hw_params *params) { - static int vars[] = { + static const int vars[] = { SNDRV_PCM_HW_PARAM_ACCESS, SNDRV_PCM_HW_PARAM_FORMAT, SNDRV_PCM_HW_PARAM_SUBFORMAT, @@ -1744,7 +1744,8 @@ int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, SNDRV_PCM_HW_PARAM_TICK_TIME, -1 }; - int err, *v; + const int *v; + int err;

for (v = vars; *v != -1; v++) { if (*v != SNDRV_PCM_HW_PARAM_BUFFER_SIZE)

Results of ioctl(2) with SNDRV_PCM_IOCTL_HW_REFINE and SNDRV_PCM_IOCTL_HW_PARAMS are different, because the latter has single value for several parameters; e.g. channels of PCM substream. Selection of the single value is done independently of application of constraints and rules. It's helpful for developers to trace the selection process

This commit adds tracepoints to snd_pcm_hw_params_choose() for the purpose.

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

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 2c5a4d9..47af1a8 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -555,11 +555,35 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, const int *v; int err;

+ struct snd_interval __maybe_unused old_interval; + struct snd_mask __maybe_unused old_mask; + for (v = vars; *v != -1; v++) { + /* Keep old parameter to trace. */ + if (trace_hw_params_mask_enabled()) { + if (hw_is_mask(*v)) + old_mask = *hw_param_mask(params, *v); + } + if (trace_hw_params_interval_enabled()) { + if (hw_is_interval(*v)) + old_interval = *hw_param_interval(params, *v); + } + if (*v != SNDRV_PCM_HW_PARAM_BUFFER_SIZE) err = snd_pcm_hw_param_first(pcm, params, *v, NULL); else err = snd_pcm_hw_param_last(pcm, params, *v, NULL); + + /* Trace the parameter. */ + if (hw_is_mask(*v)) { + trace_hw_params_mask(pcm, *v, -1, &old_mask, + hw_param_mask(params, *v)); + } + if (hw_is_interval(*v)) { + trace_hw_params_interval(pcm, *v, -1, &old_interval, + hw_param_interval(params, *v)); + } + if (snd_BUG_ON(err < 0)) return err; }

ALSA pcm core has hw_param_interval_c() to pick up parameter with const qualifier for safe programming.

This commit applies it to the cases.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/oss/pcm_oss.c | 4 ++-- sound/core/pcm_drm_eld.c | 8 ++++---- sound/core/pcm_lib.c | 3 ++- sound/core/pcm_native.c | 3 ++- 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 36baf96..2a47351 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -799,7 +799,7 @@ static int snd_pcm_oss_period_size(struct snd_pcm_substream *substream, static int choose_rate(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, unsigned int best_rate) { - struct snd_interval *it; + const struct snd_interval *it; struct snd_pcm_hw_params *save; unsigned int rate, prev;

@@ -807,7 +807,7 @@ static int choose_rate(struct snd_pcm_substream *substream, if (save == NULL) return -ENOMEM; *save = *params; - it = hw_param_interval(save, SNDRV_PCM_HW_PARAM_RATE); + it = hw_param_interval_c(save, SNDRV_PCM_HW_PARAM_RATE);

/* try multiples of the best rate */ rate = best_rate; diff --git a/sound/core/pcm_drm_eld.c b/sound/core/pcm_drm_eld.c index e70379f..9881d08 100644 --- a/sound/core/pcm_drm_eld.c +++ b/sound/core/pcm_drm_eld.c @@ -29,13 +29,13 @@ static int eld_limit_rates(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { struct snd_interval *r = hw_param_interval(params, rule->var); - struct snd_interval *c; + const struct snd_interval *c; unsigned int rate_mask = 7, i; const u8 *sad, *eld = rule->private;

sad = drm_eld_sad(eld); if (sad) { - c = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); + c = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_CHANNELS);

for (i = drm_eld_sad_count(eld); i > 0; i--, sad += 3) { unsigned max_channels = sad_max_channels(sad); @@ -57,7 +57,7 @@ static int eld_limit_channels(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { struct snd_interval *c = hw_param_interval(params, rule->var); - struct snd_interval *r; + const struct snd_interval *r; struct snd_interval t = { .min = 1, .max = 2, .integer = 1, }; unsigned int i; const u8 *sad, *eld = rule->private; @@ -67,7 +67,7 @@ static int eld_limit_channels(struct snd_pcm_hw_params *params, unsigned int rate_mask = 0;

/* Convert the rate interval to a mask */ - r = hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); + r = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); for (i = 0; i < ARRAY_SIZE(eld_rates); i++) if (r->min <= eld_rates[i] && r->max >= eld_rates[i]) rate_mask |= BIT(i); diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a62f1f4..9659fa6 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1415,7 +1415,8 @@ static int snd_pcm_hw_rule_msbits(struct snd_pcm_hw_params *params, unsigned int l = (unsigned long) rule->private; int width = l & 0xffff; unsigned int msbits = l >> 16; - struct snd_interval *i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS); + const struct snd_interval *i = + hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS);

if (!snd_interval_single(i)) return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 028d6a5..65133c2 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2073,7 +2073,8 @@ static int snd_pcm_hw_rule_format(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { unsigned int k; - struct snd_interval *i = hw_param_interval(params, rule->deps[0]); + const struct snd_interval *i = + hw_param_interval_c(params, rule->deps[0]); struct snd_mask m; struct snd_mask *mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); snd_mask_any(&m);

Some functions in compatibility layer for Open Sound System interface has local variable to copy some parameters in runtime of PCM substream, while this can be replaced with reference of pointers to parameter itself. This brings an advantage to reduce usage of kernel stack.

This commit applies this idea.

Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/oss/pcm_oss.c | 16 ++++++++-------- sound/core/oss/pcm_plugin.c | 5 +++-- sound/core/oss/pcm_plugin.h | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c index 2a47351..e306f05 100644 --- a/sound/core/oss/pcm_oss.c +++ b/sound/core/oss/pcm_oss.c @@ -848,7 +848,7 @@ static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream, int direct; snd_pcm_format_t format, sformat; int n; - struct snd_mask sformat_mask; + const struct snd_mask *sformat_mask; struct snd_mask mask;

if (trylock) { @@ -891,18 +891,18 @@ static int snd_pcm_oss_change_params(struct snd_pcm_substream *substream,

format = snd_pcm_oss_format_from(runtime->oss.format);

- sformat_mask = *hw_param_mask(sparams, SNDRV_PCM_HW_PARAM_FORMAT); + sformat_mask = hw_param_mask_c(sparams, SNDRV_PCM_HW_PARAM_FORMAT); if (direct) sformat = format; else - sformat = snd_pcm_plug_slave_format(format, &sformat_mask); + sformat = snd_pcm_plug_slave_format(format, sformat_mask);

if ((__force int)sformat < 0 || - !snd_mask_test(&sformat_mask, (__force int)sformat)) { + !snd_mask_test(sformat_mask, (__force int)sformat)) { for (sformat = (__force snd_pcm_format_t)0; (__force int)sformat <= (__force int)SNDRV_PCM_FORMAT_LAST; sformat = (__force snd_pcm_format_t)((__force int)sformat + 1)) { - if (snd_mask_test(&sformat_mask, (__force int)sformat) && + if (snd_mask_test(sformat_mask, (__force int)sformat) && snd_pcm_oss_format_to(sformat) >= 0) break; } @@ -1780,7 +1780,7 @@ static int snd_pcm_oss_get_formats(struct snd_pcm_oss_file *pcm_oss_file) int direct; struct snd_pcm_hw_params *params; unsigned int formats = 0; - struct snd_mask format_mask; + const struct snd_mask *format_mask; int fmt;

if ((err = snd_pcm_oss_get_active_substream(pcm_oss_file, &substream)) < 0) @@ -1802,12 +1802,12 @@ static int snd_pcm_oss_get_formats(struct snd_pcm_oss_file *pcm_oss_file) return -ENOMEM; _snd_pcm_hw_params_any(params); err = snd_pcm_hw_refine(substream, params); - format_mask = *hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); + format_mask = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); kfree(params); if (err < 0) return err; for (fmt = 0; fmt < 32; ++fmt) { - if (snd_mask_test(&format_mask, fmt)) { + if (snd_mask_test(format_mask, fmt)) { int f = snd_pcm_oss_format_to(fmt); if (f >= 0) formats |= f; diff --git a/sound/core/oss/pcm_plugin.c b/sound/core/oss/pcm_plugin.c index 727ac44..cadc937 100644 --- a/sound/core/oss/pcm_plugin.c +++ b/sound/core/oss/pcm_plugin.c @@ -266,7 +266,8 @@ snd_pcm_sframes_t snd_pcm_plug_slave_size(struct snd_pcm_substream *plug, snd_pc return frames; }

-static int snd_pcm_plug_formats(struct snd_mask *mask, snd_pcm_format_t format) +static int snd_pcm_plug_formats(const struct snd_mask *mask, + snd_pcm_format_t format) { struct snd_mask formats = *mask; u64 linfmts = (SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S8 | @@ -309,7 +310,7 @@ static snd_pcm_format_t preferred_formats[] = { };

snd_pcm_format_t snd_pcm_plug_slave_format(snd_pcm_format_t format, - struct snd_mask *format_mask) + const struct snd_mask *format_mask) { int i;

diff --git a/sound/core/oss/pcm_plugin.h b/sound/core/oss/pcm_plugin.h index a5035c2..38e2c14 100644 --- a/sound/core/oss/pcm_plugin.h +++ b/sound/core/oss/pcm_plugin.h @@ -126,7 +126,7 @@ int snd_pcm_plug_format_plugins(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *slave_params);

snd_pcm_format_t snd_pcm_plug_slave_format(snd_pcm_format_t format, - struct snd_mask *format_mask); + const struct snd_mask *format_mask);

int snd_pcm_plugin_append(struct snd_pcm_plugin *plugin);

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 | 71 ++++++++++++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 00d5aff..2875895 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -456,12 +456,45 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, return 0; }

-int snd_pcm_hw_refine(struct snd_pcm_substream *substream, +static int fixup_unreferenced_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + const struct snd_mask *m; + const struct snd_interval *i; + 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; - struct snd_mask *m = NULL; - struct snd_interval *i = NULL; struct snd_pcm_hw_constraints *constrs; int changed;

@@ -487,20 +520,6 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream, if (changed < 0) return changed;

- if (!params->msbits) { - i = hw_param_interval(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); - 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 | @@ -509,17 +528,9 @@ 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(params, SNDRV_PCM_HW_PARAM_FORMAT); - i = hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); - if (snd_mask_single(m) && snd_interval_single(i)) { - changed = substream->ops->ioctl(substream, - SNDRV_PCM_IOCTL1_FIFO_SIZE, params); - if (changed < 0) - return changed; - } - } + params->rmask = 0; + return 0; }

@@ -600,6 +611,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; @@ -679,6 +692,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)

When refining mask/interval parameters, helper functions can return error code. This error is not handled immediately, thus there're useless processing.

This commit handles the error immediately.

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

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 92ab8b2..05230f4 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -288,11 +288,13 @@ static int constrain_mask_params(struct snd_pcm_substream *substream,

trace_hw_params_mask(substream, k, -1, &old_mask, m);

+ if (changed < 0) + return changed; + + /* Set corresponding flag so that applications get it. */ if (changed) params->cmask |= 1 << k; - if (changed < 0) - return changed; }

return 0; @@ -326,11 +328,12 @@ static int constrain_interval_params(struct snd_pcm_substream *substream,

trace_hw_params_interval(substream, k, -1, &old_interval, i);

+ if (changed < 0) + return changed; + /* Set corresponding flag so that applications get it. */ if (changed) params->cmask |= 1 << k; - if (changed < 0) - return changed; }

return 0; @@ -431,6 +434,9 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, hw_param_interval(params, r->var)); }

+ if (changed < 0) + return changed; + rstamps[k] = stamp;

/* @@ -443,8 +449,6 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, vstamps[r->var] = stamp; again = true; } - if (changed < 0) - return changed;

stamp++; }