[alsa-devel] [PATCH 1/2] ALSA: pcm: Use a common helper for PCM state check and hwsync
Takashi Sakamoto
o-takashi at sakamocchi.jp
Sun May 21 06:54:28 CEST 2017
Hi,
On May 20 2017 03:30, Takashi Iwai wrote:
> The mostly same codes for checking the current PCM state and calling
> hwsync are found in a few places. This patch simplifies them by
> creating a common helper function.
>
> It also fixes a couple of cases where we missed the proper state check
> (e.g. PAUSED state wasn't handled in rewind and snd_pcm_hwsync()),
> too.
>
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
> sound/core/pcm_native.c | 153 +++++++++++-------------------------------------
> 1 file changed, 35 insertions(+), 118 deletions(-)
>
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index f3a3580eb44c..93bd2c662c1d 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2431,6 +2431,30 @@ static int snd_pcm_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +/* check and update PCM state; return 0 or a negative error
> + * call this inside PCM lock
> + */
> +static int do_pcm_hwsync(struct snd_pcm_substream *substream)
> +{
> + switch (substream->runtime->status->state) {
> + case SNDRV_PCM_STATE_DRAINING:
> + if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> + return -EBADFD;
> + /* Fall through */
> + case SNDRV_PCM_STATE_RUNNING:
> + return snd_pcm_update_hw_ptr(substream);
> + case SNDRV_PCM_STATE_PREPARED:
> + case SNDRV_PCM_STATE_PAUSED:
> + return 0;
> + case SNDRV_PCM_STATE_SUSPENDED:
> + return -ESTRPIPE;
> + case SNDRV_PCM_STATE_XRUN:
> + return -EPIPE;
> + default:
> + return -EBADFD;
> + }
> +}
> +
> static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream,
> snd_pcm_uframes_t frames)
> {
> @@ -2443,25 +2467,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> return 0;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_PREPARED:
> - break;
> - case SNDRV_PCM_STATE_DRAINING:
> - case SNDRV_PCM_STATE_RUNNING:
> - if (snd_pcm_update_hw_ptr(substream) >= 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_XRUN:
> - ret = -EPIPE;
> - goto __end;
> - case SNDRV_PCM_STATE_SUSPENDED:
> - ret = -ESTRPIPE;
> - goto __end;
> - default:
> - ret = -EBADFD;
> + ret = do_pcm_hwsync(substream);
> + if (ret < 0)
> goto __end;
> - }
> -
> hw_avail = snd_pcm_playback_hw_avail(runtime);
> if (hw_avail <= 0) {
> ret = 0;
> @@ -2491,25 +2499,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
> return 0;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_PREPARED:
> - case SNDRV_PCM_STATE_DRAINING:
> - break;
> - case SNDRV_PCM_STATE_RUNNING:
> - if (snd_pcm_update_hw_ptr(substream) >= 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_XRUN:
> - ret = -EPIPE;
> + ret = do_pcm_hwsync(substream);
> + if (ret < 0)
> goto __end;
> - case SNDRV_PCM_STATE_SUSPENDED:
> - ret = -ESTRPIPE;
> - goto __end;
> - default:
> - ret = -EBADFD;
> - goto __end;
> - }
> -
> hw_avail = snd_pcm_capture_hw_avail(runtime);
> if (hw_avail <= 0) {
> ret = 0;
> @@ -2539,26 +2531,9 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
> return 0;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_PREPARED:
> - case SNDRV_PCM_STATE_PAUSED:
> - break;
> - case SNDRV_PCM_STATE_DRAINING:
> - case SNDRV_PCM_STATE_RUNNING:
> - if (snd_pcm_update_hw_ptr(substream) >= 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_XRUN:
> - ret = -EPIPE;
> - goto __end;
> - case SNDRV_PCM_STATE_SUSPENDED:
> - ret = -ESTRPIPE;
> + ret = do_pcm_hwsync(substream);
> + if (ret < 0)
> goto __end;
> - default:
> - ret = -EBADFD;
> - goto __end;
> - }
> -
> avail = snd_pcm_playback_avail(runtime);
> if (avail <= 0) {
> ret = 0;
> @@ -2588,26 +2563,9 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
> return 0;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_PREPARED:
> - case SNDRV_PCM_STATE_DRAINING:
> - case SNDRV_PCM_STATE_PAUSED:
> - break;
> - case SNDRV_PCM_STATE_RUNNING:
> - if (snd_pcm_update_hw_ptr(substream) >= 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_XRUN:
> - ret = -EPIPE;
> - goto __end;
> - case SNDRV_PCM_STATE_SUSPENDED:
> - ret = -ESTRPIPE;
> + ret = do_pcm_hwsync(substream);
> + if (ret < 0)
> goto __end;
> - default:
> - ret = -EBADFD;
> - goto __end;
> - }
> -
> avail = snd_pcm_capture_avail(runtime);
> if (avail <= 0) {
> ret = 0;
> @@ -2627,33 +2585,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>
> static int snd_pcm_hwsync(struct snd_pcm_substream *substream)
> {
> - struct snd_pcm_runtime *runtime = substream->runtime;
> int err;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_DRAINING:
> - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> - goto __badfd;
> - /* Fall through */
> - case SNDRV_PCM_STATE_RUNNING:
> - if ((err = snd_pcm_update_hw_ptr(substream)) < 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_PREPARED:
> - err = 0;
> - break;
> - case SNDRV_PCM_STATE_SUSPENDED:
> - err = -ESTRPIPE;
> - break;
> - case SNDRV_PCM_STATE_XRUN:
> - err = -EPIPE;
> - break;
> - default:
> - __badfd:
> - err = -EBADFD;
> - break;
> - }
> + err = do_pcm_hwsync(substream);
> snd_pcm_stream_unlock_irq(substream);
> return err;
> }
> @@ -2666,31 +2601,13 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
> snd_pcm_sframes_t n = 0;
>
> snd_pcm_stream_lock_irq(substream);
> - switch (runtime->status->state) {
> - case SNDRV_PCM_STATE_DRAINING:
> - if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
> - goto __badfd;
> - /* Fall through */
> - case SNDRV_PCM_STATE_RUNNING:
> - if ((err = snd_pcm_update_hw_ptr(substream)) < 0)
> - break;
> - /* Fall through */
> - case SNDRV_PCM_STATE_PREPARED:
> - case SNDRV_PCM_STATE_SUSPENDED:
> - err = 0;
> + 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;
> - break;
> - case SNDRV_PCM_STATE_XRUN:
> - err = -EPIPE;
> - break;
> - default:
> - __badfd:
> - err = -EBADFD;
> - break;
> }
> snd_pcm_stream_unlock_irq(substream);
> if (!err)
In this patch, the added function, do_pcm_hwsync(), judges from current
status of runtime of PCM substream. The cases are illustrated in a below
table.
any operation:
state capture playback
----------+---------+---------
PREPARED 0 0
RUNNING hw_ptr() hw_ptr()
XRUN EPIPE EPIPE
DRAINING EBADFD hw_ptr()
PAUSED 0 0
SUSPENDED ESTRPIPE ESTRPIPE
others EBADFD EBADFD
Here, 'any' means relevant operations; delay, hwsync, forward and
rewind. Columns with 0 represents they're cases to continue requested
operation. At RUNNING state, snd_pcm_update_hw_ptr() is executed at
first, then continue processing as requested according to its return
value. When getting EPIPE, operation is canceled.
As Iwai-san described, this brings changes to current
implementation. The current cases are illustrated in below tables.
Columns with asterisks get the change.
delay operation:
state capture playback
----------+---------+---------+
PREPARED 0 0
RUNNING hw_ptr() hw_ptr()
XRUN EPIPE EPIPE
DRAINING EBADFD hw_ptr()
PAUSED *EBADFD *EBADFD
SUSPENDED *0 *0
others EBADFD EBADFD
hwsync operation:
state capture playback
----------+---------+---------
PREPARED 0 0
RUNNING hw_ptr() hw_ptr()
XRUN EPIPE EPIPE
DRAINING EBADFD hw_ptr()
PAUSED *EBADFD *EBADFD
SUSPENDED ESTRPIPE ESTRPIPE
others EBADFD EBADFD
forward operation:
state capture playback
----------+---------+---------
PREPARED 0 0
RUNNING hw_ptr() hw_ptr()
XRUN EPIPE EPIPE
DRAINING *0 hw_ptr()
PAUSED 0 0
SUSPENDED ESTRPIPE ESTRPIPE
others EBADFD EBADFD
rewind operation:
state capture playback
----------+---------+---------
PREPARED 0 0
RUNNING hw_ptr() hw_ptr()
XRUN EPIPE EPIPE
DRAINING *0 hw_ptr()
PAUSED *EBADFD *EBADFD
SUSPENDED ESTRPIPE ESTRPIPE
others EBADFD EBADFD
Here, columns with 'others' are for a set of DISCONNECTED, OPEN and
SETUP.
When the PCM substream is under PAUSED state, transmission should be
stopped and PCM buffer is kept as is. In the buffer, hwptr doesn't move
but applptr can be safely moved by applications' requests for
forwarding/rewinding. The change is enough reasonable.
As well, under SUSPENDED state, transmission is stopped. But in this
status, PCM buffer is released. Calculation of PCM frames on the buffer
makes no sense and ESTRPIPE should be returned. Documentation in
alsa-lib also described it[1].
The DRAINING state of PCM substream is valid only in a case that the
substream is for playbacking. Returning EBADFD is reasonable before
synchronizing hwptr for capture PCM substream.
Overall, this change is good to me, against its influences to user
land, from a semantics of states and operations for PCM substream.
Reviewed-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
[1] Error codes
http://www.alsa-project.org/alsa-doc/alsa-lib/pcm.html#pcm_errors
Regards
Takashi Sakamoto
More information about the Alsa-devel
mailing list