[PATCH 0/3] PCM cleanups

[PATCH 0/3] PCM cleanups

[Takashi Iwai]

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

[PATCH 1/3] ALSA: pcm: Clean up with snd_pcm_avail() and snd_pcm_hw_avail() helpers

[Takashi Iwai]

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

[PATCH 2/3] ALSA: pcm: Unify playback and capture poll callbacks

[Takashi Iwai]

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

[PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

[Takashi Iwai]

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

Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

[Takashi Sakamoto]

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

Re: [PATCH 0/3] PCM cleanups

[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

Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

[Takashi Iwai]

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
> 

Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()

[Takashi Sakamoto]

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