All of lore.kernel.org
 help / color / mirror / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org
Subject: Re: [PATCH 3/3] ALSA: pcm: Unify delay calculation in snd_pcm_status() and snd_pcm_delay()
Date: Tue, 17 Apr 2018 07:20:15 +0200	[thread overview]
Message-ID: <s5hin8qs92o.wl-tiwai@suse.de> (raw)
In-Reply-To: <c188c58d-3737-22d3-d994-25f2e79e7856@sakamocchi.jp>

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
> 

  reply	other threads:[~2018-04-17  5:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2018-04-17 10:05       ` Takashi Sakamoto
2018-04-17  3:05 ` [PATCH 0/3] PCM cleanups Takashi Sakamoto

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5hin8qs92o.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=o-takashi@sakamocchi.jp \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.