[alsa-devel] [PATCH 0/3] PCM cleanups
Hi,
this is a patch series for minor cleanups of PCM core codes. Mostly to unify the direction-separated PCM callbacks to unified ones.
Takashi
===
Takashi Iwai (3): ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers ALSA: pcm: Unify playback and capture poll callbacks ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()
sound/core/pcm_compat.c | 10 +-- sound/core/pcm_lib.c | 15 +---- sound/core/pcm_local.h | 18 +++++ sound/core/pcm_native.c | 175 ++++++++++++------------------------------------ 4 files changed, 67 insertions(+), 151 deletions(-)
Introduce two new direction-neutral helpers to calculate the avail and hw_avail values, and clean up the code with them.
The two separated forward and rewind functions are gathered to the unified functions.
No functional change but only code reductions.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_compat.c | 10 ++------ sound/core/pcm_lib.c | 15 +++--------- sound/core/pcm_local.h | 18 ++++++++++++++ sound/core/pcm_native.c | 65 ++++++++----------------------------------------- 4 files changed, 33 insertions(+), 75 deletions(-)
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index b719d0bd833e..0be248543f1e 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -44,10 +44,7 @@ static int snd_pcm_ioctl_rewind_compat(struct snd_pcm_substream *substream,
if (get_user(frames, src)) return -EFAULT; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - err = snd_pcm_playback_rewind(substream, frames); - else - err = snd_pcm_capture_rewind(substream, frames); + err = snd_pcm_rewind(substream, frames); if (put_user(err, src)) return -EFAULT; return err < 0 ? err : 0; @@ -61,10 +58,7 @@ static int snd_pcm_ioctl_forward_compat(struct snd_pcm_substream *substream,
if (get_user(frames, src)) return -EFAULT; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - err = snd_pcm_playback_forward(substream, frames); - else - err = snd_pcm_capture_forward(substream, frames); + err = snd_pcm_forward(substream, frames); if (put_user(err, src)) return -EFAULT; return err < 0 ? err : 0; diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index f4a19509cccf..44b5ae833082 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -191,10 +191,7 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, { snd_pcm_uframes_t avail;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - avail = snd_pcm_playback_avail(runtime); - else - avail = snd_pcm_capture_avail(runtime); + avail = snd_pcm_avail(substream); if (avail > runtime->avail_max) runtime->avail_max = avail; if (runtime->status->state == SNDRV_PCM_STATE_DRAINING) { @@ -1856,10 +1853,7 @@ static int wait_for_avail(struct snd_pcm_substream *substream, * This check must happen after been added to the waitqueue * and having current state be INTERRUPTIBLE. */ - if (is_playback) - avail = snd_pcm_playback_avail(runtime); - else - avail = snd_pcm_capture_avail(runtime); + avail = snd_pcm_avail(substream); if (avail >= runtime->twake) break; snd_pcm_stream_unlock_irq(substream); @@ -2175,10 +2169,7 @@ snd_pcm_sframes_t __snd_pcm_lib_xfer(struct snd_pcm_substream *substream, runtime->twake = runtime->control->avail_min ? : 1; if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) snd_pcm_update_hw_ptr(substream); - if (is_playback) - avail = snd_pcm_playback_avail(runtime); - else - avail = snd_pcm_capture_avail(runtime); + avail = snd_pcm_avail(substream); while (size > 0) { snd_pcm_uframes_t frames, appl_ptr, appl_ofs; snd_pcm_uframes_t cont; diff --git a/sound/core/pcm_local.h b/sound/core/pcm_local.h index 16f254732b2a..7a499d02df6c 100644 --- a/sound/core/pcm_local.h +++ b/sound/core/pcm_local.h @@ -36,6 +36,24 @@ int snd_pcm_update_hw_ptr(struct snd_pcm_substream *substream); void snd_pcm_playback_silence(struct snd_pcm_substream *substream, snd_pcm_uframes_t new_hw_ptr);
+static inline snd_pcm_uframes_t +snd_pcm_avail(struct snd_pcm_substream *substream) +{ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + return snd_pcm_playback_avail(substream->runtime); + else + return snd_pcm_capture_avail(substream->runtime); +} + +static inline snd_pcm_uframes_t +snd_pcm_hw_avail(struct snd_pcm_substream *substream) +{ + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + return snd_pcm_playback_hw_avail(substream->runtime); + else + return snd_pcm_capture_hw_avail(substream->runtime); +} + #ifdef CONFIG_SND_PCM_TIMER void snd_pcm_timer_resolution_change(struct snd_pcm_substream *substream); void snd_pcm_timer_init(struct snd_pcm_substream *substream); diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 35ffccea94c3..f69d89c907b9 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -908,8 +908,8 @@ int snd_pcm_status(struct snd_pcm_substream *substream, _tstamp_end: status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; + status->avail = snd_pcm_avail(substream); if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - status->avail = snd_pcm_playback_avail(runtime); if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || runtime->status->state == SNDRV_PCM_STATE_DRAINING) { status->delay = runtime->buffer_size - status->avail; @@ -917,7 +917,6 @@ int snd_pcm_status(struct snd_pcm_substream *substream, } else status->delay = 0; } else { - status->avail = snd_pcm_capture_avail(runtime); if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) status->delay = status->avail + runtime->delay; else @@ -2610,28 +2609,9 @@ static snd_pcm_sframes_t rewind_appl_ptr(struct snd_pcm_substream *substream, return ret < 0 ? 0 : frames; }
-static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream, - snd_pcm_uframes_t frames) +static snd_pcm_sframes_t snd_pcm_rewind(struct snd_pcm_substream *substream, + snd_pcm_uframes_t frames) { - struct snd_pcm_runtime *runtime = substream->runtime; - snd_pcm_sframes_t ret; - - if (frames == 0) - return 0; - - snd_pcm_stream_lock_irq(substream); - ret = do_pcm_hwsync(substream); - if (!ret) - ret = rewind_appl_ptr(substream, frames, - snd_pcm_playback_hw_avail(runtime)); - snd_pcm_stream_unlock_irq(substream); - return ret; -} - -static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substream, - snd_pcm_uframes_t frames) -{ - struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_sframes_t ret;
if (frames == 0) @@ -2641,33 +2621,14 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr ret = do_pcm_hwsync(substream); if (!ret) ret = rewind_appl_ptr(substream, frames, - snd_pcm_capture_hw_avail(runtime)); - snd_pcm_stream_unlock_irq(substream); - return ret; -} - -static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *substream, - snd_pcm_uframes_t frames) -{ - struct snd_pcm_runtime *runtime = substream->runtime; - snd_pcm_sframes_t ret; - - if (frames == 0) - return 0; - - snd_pcm_stream_lock_irq(substream); - ret = do_pcm_hwsync(substream); - if (!ret) - ret = forward_appl_ptr(substream, frames, - snd_pcm_playback_avail(runtime)); + snd_pcm_hw_avail(substream)); snd_pcm_stream_unlock_irq(substream); return ret; }
-static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *substream, - snd_pcm_uframes_t frames) +static snd_pcm_sframes_t snd_pcm_forward(struct snd_pcm_substream *substream, + snd_pcm_uframes_t frames) { - struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_sframes_t ret;
if (frames == 0) @@ -2677,7 +2638,7 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst ret = do_pcm_hwsync(substream); if (!ret) ret = forward_appl_ptr(substream, frames, - snd_pcm_capture_avail(runtime)); + snd_pcm_avail(substream)); snd_pcm_stream_unlock_irq(substream); return ret; } @@ -2830,10 +2791,7 @@ static int snd_pcm_rewind_ioctl(struct snd_pcm_substream *substream, return -EFAULT; if (put_user(0, _frames)) return -EFAULT; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - result = snd_pcm_playback_rewind(substream, frames); - else - result = snd_pcm_capture_rewind(substream, frames); + result = snd_pcm_rewind(substream, frames); __put_user(result, _frames); return result < 0 ? result : 0; } @@ -2848,10 +2806,7 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream, return -EFAULT; if (put_user(0, _frames)) return -EFAULT; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - result = snd_pcm_playback_forward(substream, frames); - else - result = snd_pcm_capture_forward(substream, frames); + result = snd_pcm_forward(substream, frames); __put_user(result, _frames); return result < 0 ? result : 0; } @@ -2992,7 +2947,7 @@ int snd_pcm_kernel_ioctl(struct snd_pcm_substream *substream, /* provided only for OSS; capture-only and no value returned */ if (substream->stream != SNDRV_PCM_STREAM_CAPTURE) return -EINVAL; - result = snd_pcm_capture_forward(substream, *frames); + result = snd_pcm_forward(substream, *frames); return result < 0 ? result : 0; } case SNDRV_PCM_IOCTL_HW_PARAMS:
The poll callbacks for playback and capture directions are doing fairly similar but with a slight difference. This patch unifies the two functions into a single callback. The advantage of this refactoring is that the direction-specific procedures become clearer.
There should be no functional change but only the code cleanup.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 74 +++++++++++++------------------------------------ 1 file changed, 19 insertions(+), 55 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index f69d89c907b9..eddb0cd6d1eb 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3095,82 +3095,46 @@ static ssize_t snd_pcm_writev(struct kiocb *iocb, struct iov_iter *from) return result; }
-static __poll_t snd_pcm_playback_poll(struct file *file, poll_table * wait) +static __poll_t snd_pcm_poll(struct file *file, poll_table *wait) { struct snd_pcm_file *pcm_file; struct snd_pcm_substream *substream; struct snd_pcm_runtime *runtime; - __poll_t mask; + __poll_t mask, ok; snd_pcm_uframes_t avail;
pcm_file = file->private_data;
substream = pcm_file->substream; + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + ok = EPOLLOUT | EPOLLWRNORM; + else + ok = EPOLLIN | EPOLLRDNORM; if (PCM_RUNTIME_CHECK(substream)) - return EPOLLOUT | EPOLLWRNORM | EPOLLERR; - runtime = substream->runtime; - - poll_wait(file, &runtime->sleep, wait); - - snd_pcm_stream_lock_irq(substream); - avail = snd_pcm_playback_avail(runtime); - switch (runtime->status->state) { - case SNDRV_PCM_STATE_RUNNING: - case SNDRV_PCM_STATE_PREPARED: - case SNDRV_PCM_STATE_PAUSED: - if (avail >= runtime->control->avail_min) { - mask = EPOLLOUT | EPOLLWRNORM; - break; - } - /* Fall through */ - case SNDRV_PCM_STATE_DRAINING: - mask = 0; - break; - default: - mask = EPOLLOUT | EPOLLWRNORM | EPOLLERR; - break; - } - snd_pcm_stream_unlock_irq(substream); - return mask; -} - -static __poll_t snd_pcm_capture_poll(struct file *file, poll_table * wait) -{ - struct snd_pcm_file *pcm_file; - struct snd_pcm_substream *substream; - struct snd_pcm_runtime *runtime; - __poll_t mask; - snd_pcm_uframes_t avail; + return ok | EPOLLERR;
- pcm_file = file->private_data; - - substream = pcm_file->substream; - if (PCM_RUNTIME_CHECK(substream)) - return EPOLLIN | EPOLLRDNORM | EPOLLERR; runtime = substream->runtime; - poll_wait(file, &runtime->sleep, wait);
+ mask = 0; snd_pcm_stream_lock_irq(substream); - avail = snd_pcm_capture_avail(runtime); + avail = snd_pcm_avail(substream); switch (runtime->status->state) { case SNDRV_PCM_STATE_RUNNING: case SNDRV_PCM_STATE_PREPARED: case SNDRV_PCM_STATE_PAUSED: - if (avail >= runtime->control->avail_min) { - mask = EPOLLIN | EPOLLRDNORM; - break; - } - mask = 0; + if (avail >= runtime->control->avail_min) + mask = ok; break; case SNDRV_PCM_STATE_DRAINING: - if (avail > 0) { - mask = EPOLLIN | EPOLLRDNORM; - break; + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) { + mask = ok; + if (!avail) + mask |= EPOLLERR; } - /* Fall through */ + break; default: - mask = EPOLLIN | EPOLLRDNORM | EPOLLERR; + mask = ok | EPOLLERR; break; } snd_pcm_stream_unlock_irq(substream); @@ -3662,7 +3626,7 @@ const struct file_operations snd_pcm_f_ops[2] = { .open = snd_pcm_playback_open, .release = snd_pcm_release, .llseek = no_llseek, - .poll = snd_pcm_playback_poll, + .poll = snd_pcm_poll, .unlocked_ioctl = snd_pcm_ioctl, .compat_ioctl = snd_pcm_ioctl_compat, .mmap = snd_pcm_mmap, @@ -3676,7 +3640,7 @@ const struct file_operations snd_pcm_f_ops[2] = { .open = snd_pcm_capture_open, .release = snd_pcm_release, .llseek = no_llseek, - .poll = snd_pcm_capture_poll, + .poll = snd_pcm_poll, .unlocked_ioctl = snd_pcm_ioctl, .compat_ioctl = snd_pcm_ioctl_compat, .mmap = snd_pcm_mmap,
Yet another slight code cleanup: there are two places where calculating the PCM delay, and they can be unified in a single helper. It reduces the multiple open codes.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..81bbe0b3284b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream, return err; }
+static inline snd_pcm_uframes_t +snd_pcm_calc_delay(struct snd_pcm_substream *substream) +{ + snd_pcm_uframes_t delay; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + delay = snd_pcm_playback_hw_avail(substream->runtime); + else + delay = snd_pcm_capture_avail(substream->runtime); + return delay + substream->runtime->delay; +} + int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status) { @@ -909,19 +921,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; status->avail = snd_pcm_avail(substream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || - runtime->status->state == SNDRV_PCM_STATE_DRAINING) { - status->delay = runtime->buffer_size - status->avail; - status->delay += runtime->delay; - } else - status->delay = 0; - } else { - if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) - status->delay = status->avail + runtime->delay; - else - status->delay = 0; - } + status->delay = snd_pcm_calc_delay(substream); status->avail_max = runtime->avail_max; status->overrange = runtime->overrange; runtime->avail_max = 0; @@ -2655,19 +2655,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream) static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; int err; snd_pcm_sframes_t n = 0;
snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream); - if (!err) { - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - n = snd_pcm_playback_hw_avail(runtime); - else - n = snd_pcm_capture_avail(runtime); - n += runtime->delay; - } + if (!err) + n = snd_pcm_calc_delay(substream); snd_pcm_stream_unlock_irq(substream); return err < 0 ? err : n; }
Hi,
On Apr 16 2018 21:14, Takashi Iwai wrote:
Yet another slight code cleanup: there are two places where calculating the PCM delay, and they can be unified in a single helper. It reduces the multiple open codes.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_native.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..81bbe0b3284b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream, return err; }
+static inline snd_pcm_uframes_t +snd_pcm_calc_delay(struct snd_pcm_substream *substream) +{
- snd_pcm_uframes_t delay;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
delay = snd_pcm_playback_hw_avail(substream->runtime);- else
delay = snd_pcm_capture_avail(substream->runtime);- return delay + substream->runtime->delay;
+}
- int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status) {
@@ -909,19 +921,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; status->avail = snd_pcm_avail(substream);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||runtime->status->state == SNDRV_PCM_STATE_DRAINING) {status->delay = runtime->buffer_size - status->avail;status->delay += runtime->delay;} elsestatus->delay = 0;- } else {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)status->delay = status->avail + runtime->delay;elsestatus->delay = 0;- }
- status->delay = snd_pcm_calc_delay(substream); status->avail_max = runtime->avail_max; status->overrange = runtime->overrange; runtime->avail_max = 0;
@@ -2655,19 +2655,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream) static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) {
struct snd_pcm_runtime *runtime = substream->runtime; int err; snd_pcm_sframes_t n = 0;
snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream);
if (!err) {
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)n = snd_pcm_playback_hw_avail(runtime);elsen = snd_pcm_capture_avail(runtime);n += runtime->delay;}
- if (!err)
n = snd_pcm_calc_delay(substream);
When any PCM substream is under running state, this patchset is good. Below is short descriptions of calculation in each case.
snd_pcm_status() for playback on running * delay = runtime->buffer_size - status->avail (=snd_pcm_playback_hw_avail()) * delay += runtime->delay
snd_pcm_status() for capture on running * delay = status->avail (= snd_pcm_avail() = snd_pcm_capture_avail()) * delay += runtime->delay
snd_pcm_delay() for playback * delay = snd_pcm_playback_hw_avail() * delay += rutime_delay
snd_pcm_delay() for capture * delay = snd_pcm_capture_avail(runtime) * delay += runtime->delay
However, under non-running states, I have some suspicion, because originally 'snd_pcm_status()' take 0 in the states while your code manage to calculate it.
(sound/core/pcm_native.c) 911 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { 912 status->avail = snd_pcm_playback_avail(runtime); 913 if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || 914 runtime->status->state == SNDRV_PCM_STATE_DRAINING) { ... 917 } else 918 status->delay = 0; 919 } else { 920 status->avail = snd_pcm_capture_avail(runtime); 921 if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) ... 923 else 924 status->delay = 0; 925 }
I think this patch is based on an assumption that hw_ptr is zeroed after stopping PCM substream. On the other hand, I cannot find codes which reset hw_ptr to zero during lifetime of PCM runtime. Any calculation of delay with hw_ptr could return non-zero value in non-running states. This patch can change behaviour of PCM core in a view of userspace applications, in my understanding.
Regards
Takashi Sakamoto
On Tue, 17 Apr 2018 05:01:13 +0200, Takashi Sakamoto wrote:
Hi,
On Apr 16 2018 21:14, Takashi Iwai wrote:
Yet another slight code cleanup: there are two places where calculating the PCM delay, and they can be unified in a single helper. It reduces the multiple open codes.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_native.c | 36 +++++++++++++++--------------------- 1 file changed, 15 insertions(+), 21 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..81bbe0b3284b 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream, return err; } +static inline snd_pcm_uframes_t +snd_pcm_calc_delay(struct snd_pcm_substream *substream) +{
- snd_pcm_uframes_t delay;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
delay = snd_pcm_playback_hw_avail(substream->runtime);- else
delay = snd_pcm_capture_avail(substream->runtime);- return delay + substream->runtime->delay;
+}
- int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status) {
@@ -909,19 +921,7 @@ int snd_pcm_status(struct snd_pcm_substream *substream, status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; status->avail = snd_pcm_avail(substream);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||runtime->status->state == SNDRV_PCM_STATE_DRAINING) {status->delay = runtime->buffer_size - status->avail;status->delay += runtime->delay;} elsestatus->delay = 0;- } else {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)status->delay = status->avail + runtime->delay;elsestatus->delay = 0;- }
- status->delay = snd_pcm_calc_delay(substream); status->avail_max = runtime->avail_max; status->overrange = runtime->overrange; runtime->avail_max = 0;
@@ -2655,19 +2655,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream) static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) {
- struct snd_pcm_runtime *runtime = substream->runtime; int err; snd_pcm_sframes_t n = 0; snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream);
- if (!err) {
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)n = snd_pcm_playback_hw_avail(runtime);elsen = snd_pcm_capture_avail(runtime);n += runtime->delay;- }
- if (!err)
n = snd_pcm_calc_delay(substream);When any PCM substream is under running state, this patchset is good. Below is short descriptions of calculation in each case.
snd_pcm_status() for playback on running
- delay = runtime->buffer_size - status->avail
(=snd_pcm_playback_hw_avail())
- delay += runtime->delay
snd_pcm_status() for capture on running
- delay = status->avail (= snd_pcm_avail() = snd_pcm_capture_avail())
- delay += runtime->delay
snd_pcm_delay() for playback
- delay = snd_pcm_playback_hw_avail()
- delay += rutime_delay
snd_pcm_delay() for capture
- delay = snd_pcm_capture_avail(runtime)
- delay += runtime->delay
However, under non-running states, I have some suspicion, because originally 'snd_pcm_status()' take 0 in the states while your code manage to calculate it.
(sound/core/pcm_native.c) 911 if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { 912 status->avail = snd_pcm_playback_avail(runtime); 913 if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || 914 runtime->status->state == SNDRV_PCM_STATE_DRAINING) { ... 917 } else 918 status->delay = 0; 919 } else { 920 status->avail = snd_pcm_capture_avail(runtime); 921 if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) ... 923 else 924 status->delay = 0; 925 }
I think this patch is based on an assumption that hw_ptr is zeroed after stopping PCM substream. On the other hand, I cannot find codes which reset hw_ptr to zero during lifetime of PCM runtime. Any calculation of delay with hw_ptr could return non-zero value in non-running states. This patch can change behaviour of PCM core in a view of userspace applications, in my understanding.
Right, snd_pcm_status() needs an additional check of snd_pcm_running(). The revised patch is below.
thanks,
Takashi
-- 8< --
Subject: [PATCH v2 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()
Yet another slight code cleanup: there are two places where calculating the PCM delay, and they can be unified in a single helper. It reduces the multiple open codes.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_native.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..7585444352df 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream, return err; }
+static inline snd_pcm_uframes_t +snd_pcm_calc_delay(struct snd_pcm_substream *substream) +{ + snd_pcm_uframes_t delay; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + delay = snd_pcm_playback_hw_avail(substream->runtime); + else + delay = snd_pcm_capture_avail(substream->runtime); + return delay + substream->runtime->delay; +} + int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status) { @@ -909,19 +921,8 @@ int snd_pcm_status(struct snd_pcm_substream *substream, status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; status->avail = snd_pcm_avail(substream); - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - if (runtime->status->state == SNDRV_PCM_STATE_RUNNING || - runtime->status->state == SNDRV_PCM_STATE_DRAINING) { - status->delay = runtime->buffer_size - status->avail; - status->delay += runtime->delay; - } else - status->delay = 0; - } else { - if (runtime->status->state == SNDRV_PCM_STATE_RUNNING) - status->delay = status->avail + runtime->delay; - else - status->delay = 0; - } + status->delay = snd_pcm_running(substream) ? + snd_pcm_calc_delay(substream) : 0; status->avail_max = runtime->avail_max; status->overrange = runtime->overrange; runtime->avail_max = 0; @@ -2655,19 +2656,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream) static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) { - struct snd_pcm_runtime *runtime = substream->runtime; int err; snd_pcm_sframes_t n = 0;
snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream); - if (!err) { - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - n = snd_pcm_playback_hw_avail(runtime); - else - n = snd_pcm_capture_avail(runtime); - n += runtime->delay; - } + if (!err) + n = snd_pcm_calc_delay(substream); snd_pcm_stream_unlock_irq(substream); return err < 0 ? err : n; }
Hi,
On Apr 17 2018 14:20, Takashi Iwai wrote:
Subject: [PATCH v2 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()
Yet another slight code cleanup: there are two places where calculating the PCM delay, and they can be unified in a single helper. It reduces the multiple open codes.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/pcm_native.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index eddb0cd6d1eb..7585444352df 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -857,6 +857,18 @@ static int snd_pcm_sw_params_user(struct snd_pcm_substream *substream, return err; }
+static inline snd_pcm_uframes_t +snd_pcm_calc_delay(struct snd_pcm_substream *substream) +{
- snd_pcm_uframes_t delay;
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
delay = snd_pcm_playback_hw_avail(substream->runtime);- else
delay = snd_pcm_capture_avail(substream->runtime);- return delay + substream->runtime->delay;
+}
- int snd_pcm_status(struct snd_pcm_substream *substream, struct snd_pcm_status *status) {
@@ -909,19 +921,8 @@ int snd_pcm_status(struct snd_pcm_substream *substream, status->appl_ptr = runtime->control->appl_ptr; status->hw_ptr = runtime->status->hw_ptr; status->avail = snd_pcm_avail(substream);
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||runtime->status->state == SNDRV_PCM_STATE_DRAINING) {status->delay = runtime->buffer_size - status->avail;status->delay += runtime->delay;} elsestatus->delay = 0;- } else {
if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)status->delay = status->avail + runtime->delay;elsestatus->delay = 0;- }
- status->delay = snd_pcm_running(substream) ?
snd_pcm_calc_delay(substream) : 0;@@ -2655,19 +2656,13 @@ static int snd_pcm_hwsync(struct snd_pcm_substream *substream) static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) {
struct snd_pcm_runtime *runtime = substream->runtime; int err; snd_pcm_sframes_t n = 0;
snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream);
if (!err) {
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)n = snd_pcm_playback_hw_avail(runtime);elsen = snd_pcm_capture_avail(runtime);n += runtime->delay;}
- if (!err)
n = snd_pcm_calc_delay(substream);
Oh, 'snd_pcm_running()' is exactly suitable for this case ;)
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
Thanks
Takashi Sakamoto
Hi,
On Apr 16 2018 21:14, Takashi Iwai wrote:
Hi,
this is a patch series for minor cleanups of PCM core codes. Mostly to unify the direction-separated PCM callbacks to unified ones.
Takashi
===
Takashi Iwai (3): ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers ALSA: pcm: Unify playback and capture poll callbacks
For the above two patches, I find no behaviour changes of PCM core.
Reviewed-by: Takashi Sakamoto o-takashi@sakamocchi.jp
ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()
sound/core/pcm_compat.c | 10 +-- sound/core/pcm_lib.c | 15 +---- sound/core/pcm_local.h | 18 +++++ sound/core/pcm_native.c | 175 ++++++++++++------------------------------------ 4 files changed, 67 insertions(+), 151 deletions(-)
Regards
Takashi Sakamoto
participants (2)
-
Takashi Iwai -
Takashi Sakamoto