[alsa-devel] [PATCH 0/2] ALSA: pcm: probe tracepoint events only when parameter is changed
Hi,
This patchset includes a part 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
The purpose of this patchset is to limit added events of tracepoint for PCM parameters only when corresponding parameter is actually changed. Developers get the events without events for unchanged parameters.
Additionally, this simplifies error handling of PCM parameters processing. This is intrusive somehow for userspace applications but just for error path.
Takashi Sakamoto (2): ALSA: pcm: return error immediately for parameters handling ALSA: pcm: probe events when parameters are changed actually
sound/core/pcm_native.c | 122 ++++++++++++++++++++++++++---------------------- 1 file changed, 67 insertions(+), 55 deletions(-)
When refining mask/interval parameters, helper functions can return error code. This error is not handled immediately. This seems to return parameters to userspace applications in its meddle of processing.
However, in general, when receiving error from system calls, the application might not handle argument buffer. It's reasonable to judge the above design as superfluity.
This commit handles the error immediately.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 66 ++++++++++++++++++++++++++++--------------------- 1 file changed, 38 insertions(+), 28 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 076187ae8859..425f54827e78 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -276,14 +276,14 @@ static int constrain_mask_params(struct snd_pcm_substream *substream, old_mask = *m;
changed = snd_mask_refine(m, constrs_mask(constrs, k)); + if (changed < 0) + return changed;
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) - return changed; }
return 0; @@ -312,14 +312,14 @@ static int constrain_interval_params(struct snd_pcm_substream *substream, old_interval = *i;
changed = snd_interval_refine(i, constrs_interval(constrs, k)); + if (changed < 0) + return changed;
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) - return changed; }
return 0; @@ -406,6 +406,8 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, }
changed = r->func(params, r); + if (changed < 0) + return changed;
if (hw_is_mask(r->var)) { trace_hw_mask_param(substream, r->var, k + 1, @@ -428,8 +430,7 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, vstamps[r->var] = stamp; again = true; } - if (changed < 0) - return changed; + stamp++; }
@@ -527,13 +528,16 @@ 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; - } + if (err < 0) + goto end;
+ err = fixup_unreferenced_params(substream, params); + if (err < 0) + goto end; + + if (copy_to_user(_params, params, sizeof(*params))) + err = -EFAULT; +end: kfree(params); return err; } @@ -749,11 +753,12 @@ static int snd_pcm_hw_params_user(struct snd_pcm_substream *substream, return PTR_ERR(params);
err = snd_pcm_hw_params(substream, params); - if (copy_to_user(_params, params, sizeof(*params))) { - if (!err) - err = -EFAULT; - } + if (err < 0) + goto end;
+ if (copy_to_user(_params, params, sizeof(*params))) + err = -EFAULT; +end: kfree(params); return err; } @@ -3699,14 +3704,17 @@ 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) - err = -EFAULT; - } + if (err < 0) + goto out_old; + + err = fixup_unreferenced_params(substream, params); + if (err < 0) + goto out_old;
+ snd_pcm_hw_convert_to_old_params(oparams, params); + if (copy_to_user(_oparams, oparams, sizeof(*oparams))) + err = -EFAULT; +out_old: kfree(oparams); out: kfree(params); @@ -3729,14 +3737,16 @@ static int snd_pcm_hw_params_old_user(struct snd_pcm_substream *substream, err = PTR_ERR(oparams); goto out; } + snd_pcm_hw_convert_from_old_params(params, oparams); err = snd_pcm_hw_params(substream, params); - snd_pcm_hw_convert_to_old_params(oparams, params); - if (copy_to_user(_oparams, oparams, sizeof(*oparams))) { - if (!err) - err = -EFAULT; - } + if (err < 0) + goto out_old;
+ snd_pcm_hw_convert_to_old_params(oparams, params); + if (copy_to_user(_oparams, oparams, sizeof(*oparams))) + err = -EFAULT; +out_old: kfree(oparams); out: kfree(params);
At present, trace events are probed even if corresponding parameter is not actually changed. This is inconvenient.
This commit improves the behaviour.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/pcm_native.c | 56 +++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 425f54827e78..5099078dde93 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -278,12 +278,12 @@ static int constrain_mask_params(struct snd_pcm_substream *substream, changed = snd_mask_refine(m, constrs_mask(constrs, k)); if (changed < 0) return changed; - - trace_hw_mask_param(substream, k, 0, &old_mask, m); + if (changed == 0) + continue;
/* Set corresponding flag so that the caller gets it. */ - if (changed) - params->cmask |= 1 << k; + trace_hw_mask_param(substream, k, 0, &old_mask, m); + params->cmask |= 1 << k; }
return 0; @@ -314,12 +314,12 @@ static int constrain_interval_params(struct snd_pcm_substream *substream, changed = snd_interval_refine(i, constrs_interval(constrs, k)); if (changed < 0) return changed; - - trace_hw_interval_param(substream, k, 0, &old_interval, i); + if (changed == 0) + continue;
/* Set corresponding flag so that the caller gets it. */ - if (changed) - params->cmask |= 1 << k; + trace_hw_interval_param(substream, k, 0, &old_interval, i); + params->cmask |= 1 << k; }
return 0; @@ -409,29 +409,29 @@ static int constrain_params_by_rules(struct snd_pcm_substream *substream, if (changed < 0) return changed;
- 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; - /* - * When the parameters is changed, notify it to the caller + * When the parameter is changed, notify it to the caller * by corresponding returned bit, then preparing for next * iteration. */ if (changed && r->var >= 0) { + 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)); + } + params->cmask |= (1 << r->var); vstamps[r->var] = stamp; again = true; }
- stamp++; + rstamps[k] = stamp++; }
/* Iterate to evaluate all rules till no parameters are changed. */ @@ -604,7 +604,7 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, const int *v; struct snd_mask old_mask; struct snd_interval old_interval; - int err; + int changed;
for (v = vars; *v != -1; v++) { /* Keep old parameter to trace. */ @@ -617,13 +617,15 @@ static int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm, 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); + changed = snd_pcm_hw_param_first(pcm, params, *v, NULL); else - err = snd_pcm_hw_param_last(pcm, params, *v, NULL); - if (snd_BUG_ON(err < 0)) - return err; + changed = snd_pcm_hw_param_last(pcm, params, *v, NULL); + if (snd_BUG_ON(changed < 0)) + return changed; + if (changed == 0) + continue;
- /* Trace the parameter. */ + /* Trace the changed parameter. */ if (hw_is_mask(*v)) { trace_hw_mask_param(pcm, *v, 0, &old_mask, hw_param_mask(params, *v));
On Sun, 11 Jun 2017 16:56:11 +0200, Takashi Sakamoto wrote:
Hi,
This patchset includes a part 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
The purpose of this patchset is to limit added events of tracepoint for PCM parameters only when corresponding parameter is actually changed. Developers get the events without events for unchanged parameters.
Additionally, this simplifies error handling of PCM parameters processing. This is intrusive somehow for userspace applications but just for error path.
Takashi Sakamoto (2): ALSA: pcm: return error immediately for parameters handling ALSA: pcm: probe events when parameters are changed actually
Applied both patches now. Thanks.
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto