* [PATCH 0/3] PCM cleanups
@ 2018-04-16 12:14 Takashi Iwai
2018-04-16 12:14 ` [PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers Takashi Iwai
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Takashi Iwai @ 2018-04-16 12:14 UTC (permalink / raw)
To: alsa-devel
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(-)
--
2.16.3
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers 2018-04-16 12:14 [PATCH 0/3] PCM cleanups Takashi Iwai @ 2018-04-16 12:14 ` Takashi Iwai 2018-04-16 12:14 ` [PATCH 2/3] ALSA: pcm: Unify playback and capture poll callbacks Takashi Iwai ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: Takashi Iwai @ 2018-04-16 12:14 UTC (permalink / raw) To: alsa-devel 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: -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] ALSA: pcm: Unify playback and capture poll callbacks 2018-04-16 12:14 [PATCH 0/3] PCM cleanups Takashi Iwai 2018-04-16 12:14 ` [PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers Takashi Iwai @ 2018-04-16 12:14 ` Takashi Iwai 2018-04-16 12:14 ` [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() Takashi Iwai 2018-04-17 3:05 ` [PATCH 0/3] PCM cleanups Takashi Sakamoto 3 siblings, 0 replies; 8+ messages in thread From: Takashi Iwai @ 2018-04-16 12:14 UTC (permalink / raw) To: alsa-devel 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, -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() 2018-04-16 12:14 [PATCH 0/3] PCM cleanups Takashi Iwai 2018-04-16 12:14 ` [PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers Takashi Iwai 2018-04-16 12:14 ` [PATCH 2/3] ALSA: pcm: Unify playback and capture poll callbacks Takashi Iwai @ 2018-04-16 12:14 ` Takashi Iwai 2018-04-17 3:01 ` Takashi Sakamoto 2018-04-17 3:05 ` [PATCH 0/3] PCM cleanups Takashi Sakamoto 3 siblings, 1 reply; 8+ messages in thread From: Takashi Iwai @ 2018-04-16 12:14 UTC (permalink / raw) To: alsa-devel 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; } -- 2.16.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() 2018-04-16 12:14 ` [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() Takashi Iwai @ 2018-04-17 3:01 ` Takashi Sakamoto 2018-04-17 5:20 ` Takashi Iwai 0 siblings, 1 reply; 8+ messages in thread From: Takashi Sakamoto @ 2018-04-17 3:01 UTC (permalink / raw) To: Takashi Iwai, alsa-devel 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; > - } 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; > } 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() 2018-04-17 3:01 ` Takashi Sakamoto @ 2018-04-17 5:20 ` Takashi Iwai 2018-04-17 10:05 ` Takashi Sakamoto 0 siblings, 1 reply; 8+ messages in thread From: Takashi Iwai @ 2018-04-17 5:20 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel 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; > > - } 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; > > } > > 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; } -- 2.16.3 > > > Regards > > Takashi Sakamoto > ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() 2018-04-17 5:20 ` Takashi Iwai @ 2018-04-17 10:05 ` Takashi Sakamoto 0 siblings, 0 replies; 8+ messages in thread From: Takashi Sakamoto @ 2018-04-17 10:05 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel 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; > - } 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; > } Oh, 'snd_pcm_running()' is exactly suitable for this case ;) Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> Thanks Takashi Sakamoto ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] PCM cleanups 2018-04-16 12:14 [PATCH 0/3] PCM cleanups Takashi Iwai ` (2 preceding siblings ...) 2018-04-16 12:14 ` [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() Takashi Iwai @ 2018-04-17 3:05 ` Takashi Sakamoto 3 siblings, 0 replies; 8+ messages in thread From: Takashi Sakamoto @ 2018-04-17 3:05 UTC (permalink / raw) To: Takashi Iwai, alsa-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-17 10:05 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-16 12:14 [PATCH 0/3] PCM cleanups Takashi Iwai 2018-04-16 12:14 ` [PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers Takashi Iwai 2018-04-16 12:14 ` [PATCH 2/3] ALSA: pcm: Unify playback and capture poll callbacks Takashi Iwai 2018-04-16 12:14 ` [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay() Takashi Iwai 2018-04-17 3:01 ` Takashi Sakamoto 2018-04-17 5:20 ` Takashi Iwai 2018-04-17 10:05 ` Takashi Sakamoto 2018-04-17 3:05 ` [PATCH 0/3] PCM cleanups Takashi Sakamoto
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.