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@xxxxxxx>
> > ---
> > 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@xxxxxxx>
---
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
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
