[PATCH 0/7] ALSA: Add rewinds disabled, delays, max_inflight_bytes

[PATCH 0/7] ALSA: Add rewinds disabled, delays, max_inflight_bytes

[Subhransu S. Prusty]

Set of patches to fix issues with delays, hw_ptr fuzziness [1] and
increased buffering w/ DSPs

1. Rewinds can be disabled  when data written in ring buffer will never be
validated. This allow for new HDaudio SPIB DMA functionality(allow fetch up to the
application pointer, no rewind supported)

2. Report max in-flight bytes to avoid problems with stale data (like late
wake-ups, rewinds)

3. add new estimate for USB startup delay.

[1]
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093646.html


Pierre-Louis Bossart (6):
  ALSA: core: let low-level driver or userspace disable rewinds
  ALSA: core: add .update_appl_ptr callback for pcm ops
  ALSA: core: add report of max inflight bytes
  ALSA: hda: add default value for max_inflight_bytes
  ALSA: usb: no_period_wake and max_inflight_bytes report
  ALSA: usb: take startup delay into account

Ramesh Babu (1):
  ALSA: pcm: avoid mmap of control data if .update_appl_ptr is
    implemented

 include/sound/pcm.h            |  4 +++
 include/uapi/sound/asound.h    |  6 ++--
 sound/core/pcm_lib.c           | 26 ++++++++++++++++++
 sound/core/pcm_native.c        | 51 +++++++++++++++++++++++++++++++++-
 sound/pci/hda/hda_controller.c |  1 +
 sound/usb/card.h               |  1 +
 sound/usb/pcm.c                | 62 +++++++++++++++++++++++++++++++++++++-----
 7 files changed, 141 insertions(+), 10 deletions(-)

-- 
1.9.1

[PATCH 1/7] ALSA: core: let low-level driver or userspace disable rewinds

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add new hw_params flag to explicitly tell driver that rewinds will never
be used. This can be used by low-level driver to optimize DMA operations
and reduce power consumption. Use this flag only when data written in
ring buffer will never be invalidated, e.g. any update of appl_ptr is
final.

Note that the update of appl_ptr include both a read/write data
operation as well as snd_pcm_forward() whose behavior is not modified.

Caveat: there is currently no way to query capabilities without opening
a pcm stream, so applications might need to serially open all exposed
devices, check what they support by looking at hw_params->info and close
them (this is what PulseAudio does so might not be an issue)

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/sound/pcm.h         | 1 +
 include/uapi/sound/asound.h | 1 +
 sound/core/pcm_native.c     | 8 ++++++++
 3 files changed, 10 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index af1fb37..5344c16 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -368,6 +368,7 @@ struct snd_pcm_runtime {
 	unsigned int rate_num;
 	unsigned int rate_den;
 	unsigned int no_period_wakeup: 1;
+	unsigned int no_rewinds:1;
 
 	/* -- SW params -- */
 	int tstamp_mode;		/* mmap timestamp is updated */
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 609cadb..6828ed2 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -362,6 +362,7 @@ typedef int snd_pcm_hw_param_t;
 #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
 #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
 #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
+#define SNDRV_PCM_HW_PARAMS_NO_REWINDS	        (1<<3)	/* disable rewinds */
 
 struct snd_interval {
 	unsigned int min, max;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c61fd50..be8617b 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -565,6 +565,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
 	runtime->no_period_wakeup =
 			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
 			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
+	runtime->no_rewinds =
+		(params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;
 
 	bits = snd_pcm_format_physical_width(runtime->format);
 	runtime->sample_bits = bits;
@@ -2438,6 +2440,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 	if (frames == 0)
 		return 0;
 
+	if (runtime->no_rewinds)
+		return 0;
+
 	snd_pcm_stream_lock_irq(substream);
 	switch (runtime->status->state) {
 	case SNDRV_PCM_STATE_PREPARED:
@@ -2486,6 +2491,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 	if (frames == 0)
 		return 0;
 
+	if (runtime->no_rewinds)
+		return 0;
+
 	snd_pcm_stream_lock_irq(substream);
 	switch (runtime->status->state) {
 	case SNDRV_PCM_STATE_PREPARED:
-- 
1.9.1

Re: [PATCH 1/7] ALSA: core: let low-level driver or userspace disable rewinds

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:24 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Add new hw_params flag to explicitly tell driver that rewinds will never
> be used. This can be used by low-level driver to optimize DMA operations
> and reduce power consumption. Use this flag only when data written in
> ring buffer will never be invalidated, e.g. any update of appl_ptr is
> final.
> 
> Note that the update of appl_ptr include both a read/write data
> operation as well as snd_pcm_forward() whose behavior is not modified.
> 
> Caveat: there is currently no way to query capabilities without opening
> a pcm stream, so applications might need to serially open all exposed
> devices, check what they support by looking at hw_params->info and close
> them (this is what PulseAudio does so might not be an issue)

This is a general issue in the current PCM API design, and not
specific to this new bit.

> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  include/sound/pcm.h         | 1 +
>  include/uapi/sound/asound.h | 1 +
>  sound/core/pcm_native.c     | 8 ++++++++
>  3 files changed, 10 insertions(+)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index af1fb37..5344c16 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -368,6 +368,7 @@ struct snd_pcm_runtime {
>  	unsigned int rate_num;
>  	unsigned int rate_den;
>  	unsigned int no_period_wakeup: 1;
> +	unsigned int no_rewinds:1;
>  
>  	/* -- SW params -- */
>  	int tstamp_mode;		/* mmap timestamp is updated */
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 609cadb..6828ed2 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -362,6 +362,7 @@ typedef int snd_pcm_hw_param_t;
>  #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
>  #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
>  #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
> +#define SNDRV_PCM_HW_PARAMS_NO_REWINDS	        (1<<3)	/* disable rewinds */
>  
>  struct snd_interval {
>  	unsigned int min, max;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c61fd50..be8617b 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -565,6 +565,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
>  	runtime->no_period_wakeup =
>  			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
>  			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
> +	runtime->no_rewinds =
> +		(params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;
>  
>  	bits = snd_pcm_format_physical_width(runtime->format);
>  	runtime->sample_bits = bits;
> @@ -2438,6 +2440,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  	if (frames == 0)
>  		return 0;
>  
> +	if (runtime->no_rewinds)
> +		return 0;

Better to return an error instead?

>  	snd_pcm_stream_lock_irq(substream);
>  	switch (runtime->status->state) {
>  	case SNDRV_PCM_STATE_PREPARED:
> @@ -2486,6 +2491,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>  	if (frames == 0)
>  		return 0;
>  
> +	if (runtime->no_rewinds)
> +		return 0;

Ditto.


Takashi

Re: [PATCH 1/7] ALSA: core: let low-level driver or userspace disable rewinds

[Subhransu S. Prusty]

On Fri, Sep 30, 2016 at 03:22:40PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:24 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > Add new hw_params flag to explicitly tell driver that rewinds will never
> > be used. This can be used by low-level driver to optimize DMA operations
> > and reduce power consumption. Use this flag only when data written in
> > ring buffer will never be invalidated, e.g. any update of appl_ptr is
> > final.
> > 
> > Note that the update of appl_ptr include both a read/write data
> > operation as well as snd_pcm_forward() whose behavior is not modified.
> > 
> > Caveat: there is currently no way to query capabilities without opening
> > a pcm stream, so applications might need to serially open all exposed
> > devices, check what they support by looking at hw_params->info and close
> > them (this is what PulseAudio does so might not be an issue)
> 
> This is a general issue in the current PCM API design, and not
> specific to this new bit.

Will remove this from commit message.

> 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > ---
> >  include/sound/pcm.h         | 1 +
> >  include/uapi/sound/asound.h | 1 +
> >  sound/core/pcm_native.c     | 8 ++++++++
> >  3 files changed, 10 insertions(+)
> > 
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index af1fb37..5344c16 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -368,6 +368,7 @@ struct snd_pcm_runtime {
> >  	unsigned int rate_num;
> >  	unsigned int rate_den;
> >  	unsigned int no_period_wakeup: 1;
> > +	unsigned int no_rewinds:1;
> >  
> >  	/* -- SW params -- */
> >  	int tstamp_mode;		/* mmap timestamp is updated */
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index 609cadb..6828ed2 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -362,6 +362,7 @@ typedef int snd_pcm_hw_param_t;
> >  #define SNDRV_PCM_HW_PARAMS_NORESAMPLE	(1<<0)	/* avoid rate resampling */
> >  #define SNDRV_PCM_HW_PARAMS_EXPORT_BUFFER	(1<<1)	/* export buffer */
> >  #define SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP	(1<<2)	/* disable period wakeups */
> > +#define SNDRV_PCM_HW_PARAMS_NO_REWINDS	        (1<<3)	/* disable rewinds */
> >  
> >  struct snd_interval {
> >  	unsigned int min, max;
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index c61fd50..be8617b 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -565,6 +565,8 @@ static int snd_pcm_hw_params(struct snd_pcm_substream *substream,
> >  	runtime->no_period_wakeup =
> >  			(params->info & SNDRV_PCM_INFO_NO_PERIOD_WAKEUP) &&
> >  			(params->flags & SNDRV_PCM_HW_PARAMS_NO_PERIOD_WAKEUP);
> > +	runtime->no_rewinds =
> > +		(params->flags & SNDRV_PCM_HW_PARAMS_NO_REWINDS) ? 1 : 0;
> >  
> >  	bits = snd_pcm_format_physical_width(runtime->format);
> >  	runtime->sample_bits = bits;
> > @@ -2438,6 +2440,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> >  	if (frames == 0)
> >  		return 0;
> >  
> > +	if (runtime->no_rewinds)
> > +		return 0;
> 
> Better to return an error instead?

As the number of frames rewinded is zero, it looks appropriate. Any reason
why returning an error code would help?

Regards,
Subhransu
-- 

Re: [PATCH 1/7] ALSA: core: let low-level driver or userspace disable rewinds

[Pierre-Louis Bossart]

>>> @@ -2438,6 +2440,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>>>  	if (frames == 0)
>>>  		return 0;
>>>
>>> +	if (runtime->no_rewinds)
>>> +		return 0;
>>
>> Better to return an error instead?
>
> As the number of frames rewinded is zero, it looks appropriate. Any reason
> why returning an error code would help?

The return type is also snd_sframes_t, not sure how a error code would 
be returned without additional changes.

Re: [PATCH 1/7] ALSA: core: let low-level driver or userspace disable rewinds

[Takashi Iwai]

Hi,

sorry for the late response.  I've been slowly catching up the pending
stuff...

On Mon, 03 Oct 2016 16:39:41 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>> @@ -2438,6 +2440,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> >>>  	if (frames == 0)
> >>>  		return 0;
> >>>
> >>> +	if (runtime->no_rewinds)
> >>> +		return 0;
> >>
> >> Better to return an error instead?
> >
> > As the number of frames rewinded is zero, it looks appropriate. Any reason
> > why returning an error code would help?

Well, this is an operation that is not supposed to work.  The
configuration declares that there shall be no rewind, right?

> The return type is also snd_sframes_t, not sure how a error code would
> be returned without additional changes.

snd_pcm_sframes_t is a signed type, and a negative value is taken as
an error by ioctl.


thanks,

Takashi

[PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

When appl_ptr is updated let low-level driver know, e.g.  to let the
low-level driver/hardware pre-fetch data opportunistically.

The existing .ack callback could be used but it would need to be
extended with new arguments, resulting in multiple changes in legacy
code.

Instead a new .appl_ptr_update callback is added.  The difference
between .ack and .appl_ptr_update is that .ack is only called on read or
write. .appl_ptr_update is called on read, write, rewind, forward or
when updating the appl_ptr from userspace.

In the ALSA core, this capability is independent from the NO_REWIND
hardware flag. The low-level driver may however tie both options and
only use the updated appl_ptr when rewinds are disabled due to hardware
limitations.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Babu, Ramesh <ramesh.babu@intel.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/sound/pcm.h     |  1 +
 sound/core/pcm_lib.c    |  6 ++++++
 sound/core/pcm_native.c | 24 +++++++++++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 5344c16..1accb8b 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -87,6 +87,7 @@ struct snd_pcm_ops {
 			     unsigned long offset);
 	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
 	int (*ack)(struct snd_pcm_substream *substream);
+	int (*appl_ptr_update)(struct snd_pcm_substream *substream);
 };
 
 /*
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index bb12615..1656ca9 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2090,6 +2090,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 		if (substream->ops->ack)
 			substream->ops->ack(substream);
 
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+
 		offset += frames;
 		size -= frames;
 		xfer += frames;
@@ -2322,6 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 		if (substream->ops->ack)
 			substream->ops->ack(substream);
 
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+
 		offset += frames;
 		size -= frames;
 		xfer += frames;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index be8617b..c56d4ed 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2475,6 +2475,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2526,6 +2530,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2575,6 +2583,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 		appl_ptr -= runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2624,6 +2636,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 		appl_ptr -= runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->appl_ptr_update)
+		substream->ops->appl_ptr_update(substream);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2723,8 +2739,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			return err;
 	}
 	snd_pcm_stream_lock_irq(substream);
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		/* boundary wrap-around is assumed to be handled in userspace */
 		control->appl_ptr = sync_ptr.c.control.appl_ptr;
+
+		/* let low-level driver know about appl_ptr change */
+		if (substream->ops->appl_ptr_update)
+			substream->ops->appl_ptr_update(substream);
+	}
 	else
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
-- 
1.9.1

Re: [PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:25 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> When appl_ptr is updated let low-level driver know, e.g.  to let the
> low-level driver/hardware pre-fetch data opportunistically.
> 
> The existing .ack callback could be used but it would need to be
> extended with new arguments, resulting in multiple changes in legacy
> code.

I wouldn't mind changing these callers.  They aren't so many, after
all. 


Takashi

> Instead a new .appl_ptr_update callback is added.  The difference
> between .ack and .appl_ptr_update is that .ack is only called on read or
> write. .appl_ptr_update is called on read, write, rewind, forward or
> when updating the appl_ptr from userspace.
> 
> In the ALSA core, this capability is independent from the NO_REWIND
> hardware flag. The low-level driver may however tie both options and
> only use the updated appl_ptr when rewinds are disabled due to hardware
> limitations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Babu, Ramesh <ramesh.babu@intel.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  include/sound/pcm.h     |  1 +
>  sound/core/pcm_lib.c    |  6 ++++++
>  sound/core/pcm_native.c | 24 +++++++++++++++++++++++-
>  3 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 5344c16..1accb8b 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -87,6 +87,7 @@ struct snd_pcm_ops {
>  			     unsigned long offset);
>  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
>  	int (*ack)(struct snd_pcm_substream *substream);
> +	int (*appl_ptr_update)(struct snd_pcm_substream *substream);
>  };
>  
>  /*
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index bb12615..1656ca9 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2090,6 +2090,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> @@ -2322,6 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index be8617b..c56d4ed 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2475,6 +2475,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2526,6 +2530,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2575,6 +2583,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2624,6 +2636,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2723,8 +2739,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>  			return err;
>  	}
>  	snd_pcm_stream_lock_irq(substream);
> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> +		/* boundary wrap-around is assumed to be handled in userspace */
>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +
> +		/* let low-level driver know about appl_ptr change */
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +	}
>  	else
>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> -- 
> 1.9.1
> 

Re: [PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Vinod Koul]

On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:25 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > low-level driver/hardware pre-fetch data opportunistically.
> > 
> > The existing .ack callback could be used but it would need to be
> > extended with new arguments, resulting in multiple changes in legacy
> > code.
> 
> I wouldn't mind changing these callers.  They aren't so many, after
> all. 

Yes this was one of the discussions we had in the past. I don't recall the
conclusion so had recommened to keep as is and discuss here.

Do you think it's better to do that or use a new one :)
-- 
~Vinod

Re: [PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Takashi Iwai]

On Fri, 30 Sep 2016 19:20:10 +0200,
Vinod Koul wrote:
> 
> On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> > On Fri, 30 Sep 2016 14:43:25 +0200,
> > Subhransu S. Prusty wrote:
> > > 
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > low-level driver/hardware pre-fetch data opportunistically.
> > > 
> > > The existing .ack callback could be used but it would need to be
> > > extended with new arguments, resulting in multiple changes in legacy
> > > code.
> > 
> > I wouldn't mind changing these callers.  They aren't so many, after
> > all. 
> 
> Yes this was one of the discussions we had in the past. I don't recall the
> conclusion so had recommened to keep as is and discuss here.
> 
> Do you think it's better to do that or use a new one :)

It's OK to change ack callback, and actually it'll be cleaner.
But then it'll be a problem in the next patch, I suppose :)


Takashi

Re: [PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Vinod Koul]

On Fri, Sep 30, 2016 at 08:38:38PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 19:20:10 +0200,
> Vinod Koul wrote:
> > 
> > On Fri, Sep 30, 2016 at 03:24:59PM +0200, Takashi Iwai wrote:
> > > On Fri, 30 Sep 2016 14:43:25 +0200,
> > > Subhransu S. Prusty wrote:
> > > > 
> > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > 
> > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > 
> > > > The existing .ack callback could be used but it would need to be
> > > > extended with new arguments, resulting in multiple changes in legacy
> > > > code.
> > > 
> > > I wouldn't mind changing these callers.  They aren't so many, after
> > > all. 
> > 
> > Yes this was one of the discussions we had in the past. I don't recall the
> > conclusion so had recommened to keep as is and discuss here.
> > 
> > Do you think it's better to do that or use a new one :)
> 
> It's OK to change ack callback, and actually it'll be cleaner.
> But then it'll be a problem in the next patch, I suppose :)

Yes and one of the reason is that we are using one flag to advertise two
capabilities, one is no rewind and second is appl_ptr update.

We feel we should deal with them by using two flags so that code can be
made cleaner.

Thanks
-- 
~Vinod

Re: [PATCH 2/7] ALSA: core: add .update_appl_ptr callback for pcm ops

[Pierre-Louis Bossart]

>>>> When appl_ptr is updated let low-level driver know, e.g.  to let the
>>>> low-level driver/hardware pre-fetch data opportunistically.
>>>>
>>>> The existing .ack callback could be used but it would need to be
>>>> extended with new arguments, resulting in multiple changes in legacy
>>>> code.
>>>
>>> I wouldn't mind changing these callers.  They aren't so many, after
>>> all.
>>
>> Yes this was one of the discussions we had in the past. I don't recall the
>> conclusion so had recommened to keep as is and discuss here.
>>
>> Do you think it's better to do that or use a new one :)
>
> It's OK to change ack callback, and actually it'll be cleaner.
> But then it'll be a problem in the next patch, I suppose :)

The main problem is test/coverage of legacy v. alignment of 
functionality. If we change .ack() then others will have to test for the 
changes and there's a risk of a regression on a specific platform that 
wasn't tested - Murphy's law applies.

[PATCH 3/7] ALSA: pcm: avoid mmap of control data if .update_appl_ptr is implemented

[Subhransu S. Prusty]

From: Ramesh Babu <ramesh.babu@intel.com>

In case of mmap, by default alsa-lib mmaps both control & status data.

If driver subscribes for .appl_ptr_update, driver needs to get
notification whenever appl ptr changes. So with control & status mmaped,
driver won't get appl ptr notifications.

In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
conf But this makes driver behavior dependent on a flag in the conf.

This patch conditionally checks for .appl_ptr_update and returns error
when user land asks for mmaping control & status data, thus forcing user
to issue IOCTL_SYNC_PTR.

One drawback with this approach is, if .appl_ptr_update is subscribed by
driver, the user space looses flexibility to mmap the control & status
data.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/core/pcm_native.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c56d4ed..1965d83 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3535,10 +3535,22 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
 	case SNDRV_PCM_MMAP_OFFSET_STATUS:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+		/*
+		 * we want to force sync pointer,
+		 * if driver implements appl_ptr_update
+		 */
+		if (substream->ops->appl_ptr_update)
+			return -ENXIO;
 		return snd_pcm_mmap_status(substream, file, area);
 	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+		/*
+		 * we want to force sync pointer,
+		 * if driver implements appl_ptr_update
+		 */
+		if (substream->ops->appl_ptr_update)
+			return -ENXIO;
 		return snd_pcm_mmap_control(substream, file, area);
 	default:
 		return snd_pcm_mmap_data(substream, file, area);
-- 
1.9.1

Re: [PATCH 3/7] ALSA: pcm: avoid mmap of control data if .update_appl_ptr is implemented

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:26 +0200,
Subhransu S. Prusty wrote:
> 
> From: Ramesh Babu <ramesh.babu@intel.com>
> 
> In case of mmap, by default alsa-lib mmaps both control & status data.
> 
> If driver subscribes for .appl_ptr_update, driver needs to get
> notification whenever appl ptr changes. So with control & status mmaped,
> driver won't get appl ptr notifications.
> 
> In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
> conf But this makes driver behavior dependent on a flag in the conf.
> 
> This patch conditionally checks for .appl_ptr_update and returns error
> when user land asks for mmaping control & status data, thus forcing user
> to issue IOCTL_SYNC_PTR.
> 
> One drawback with this approach is, if .appl_ptr_update is subscribed by
> driver, the user space looses flexibility to mmap the control & status
> data.

Yes, and it can be seen as an obvious regression, so I'm not sure
whether this condition is the best choice.

OTOH, I now understand why you need it in this way -- alsa-lib does
mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as
a fallback.  So, one solution would be to fix alsa-lib side.  Or, if
we fix the kernel in this way, it would work with old alsa-lib, but it
has another penalty.  Hmm...


Takashi

> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/core/pcm_native.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c56d4ed..1965d83 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3535,10 +3535,22 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
>  	case SNDRV_PCM_MMAP_OFFSET_STATUS:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * we want to force sync pointer,
> +		 * if driver implements appl_ptr_update
> +		 */
> +		if (substream->ops->appl_ptr_update)
> +			return -ENXIO;
>  		return snd_pcm_mmap_status(substream, file, area);
>  	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * we want to force sync pointer,
> +		 * if driver implements appl_ptr_update
> +		 */
> +		if (substream->ops->appl_ptr_update)
> +			return -ENXIO;
>  		return snd_pcm_mmap_control(substream, file, area);
>  	default:
>  		return snd_pcm_mmap_data(substream, file, area);
> -- 
> 1.9.1
> 

Re: [PATCH 3/7] ALSA: pcm: avoid mmap of control data if .update_appl_ptr is implemented

[Subhransu S. Prusty]

On Fri, Sep 30, 2016 at 03:40:45PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:26 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Ramesh Babu <ramesh.babu@intel.com>
> > 
> > In case of mmap, by default alsa-lib mmaps both control & status data.
> > 
> > If driver subscribes for .appl_ptr_update, driver needs to get
> > notification whenever appl ptr changes. So with control & status mmaped,
> > driver won't get appl ptr notifications.
> > 
> > In alsa-lib IOCTL_SYNC_PTR can be forced using  sync_ptr_ioctl flag in
> > conf But this makes driver behavior dependent on a flag in the conf.
> > 
> > This patch conditionally checks for .appl_ptr_update and returns error
> > when user land asks for mmaping control & status data, thus forcing user
> > to issue IOCTL_SYNC_PTR.
> > 
> > One drawback with this approach is, if .appl_ptr_update is subscribed by
> > driver, the user space looses flexibility to mmap the control & status
> > data.
> 
> Yes, and it can be seen as an obvious regression, so I'm not sure
> whether this condition is the best choice.
> 
> OTOH, I now understand why you need it in this way -- alsa-lib does
> mmap ctrl/status pages at snd_pcm_open() and sync_ptr is used only as
> a fallback.  So, one solution would be to fix alsa-lib side.  Or, if
> we fix the kernel in this way, it would work with old alsa-lib, but it
> has another penalty.  Hmm...

I will check if it can be fixed in alsa-lib side.

Regards,
Subhransu
> 
> 

-- 

[PATCH 4/7] ALSA: core: add report of max inflight bytes

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Report size of data burst before updating the hw_ptr, e.g. while DMA
operations are on-going.

This can help fix two issues with stale data discussed many times over
on the alsa-devel mailing list (refilling/reading ring buffer too late
during capture or rewinding too close to the hw_ptr during playback)

This patch only reports the maximum burst of data and does not provide
hooks to negotiate its size. This might be useful to lower power or
reduce latency, but isn't typically supported by fixed-function DMA
hardware.

The use of IOCTL1 is not really required but keep for symmetry with
existing code used to retried fifo_size

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/sound/pcm.h         |  2 ++
 include/uapi/sound/asound.h |  5 +++--
 sound/core/pcm_lib.c        | 20 ++++++++++++++++++++
 sound/core/pcm_native.c     |  7 +++++++
 4 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 1accb8b..8c9d80a 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -56,6 +56,7 @@ struct snd_pcm_hardware {
 	unsigned int periods_min;	/* min # of periods */
 	unsigned int periods_max;	/* max # of periods */
 	size_t fifo_size;		/* fifo size in bytes */
+	unsigned int max_inflight_bytes;/* hw_ptr precision/fuzziness, e.g. due to DMA transfers */
 };
 
 struct snd_pcm_substream;
@@ -105,6 +106,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_IOCTL1_CHANNEL_INFO	2
 #define SNDRV_PCM_IOCTL1_GSTATE		3
 #define SNDRV_PCM_IOCTL1_FIFO_SIZE	4
+#define SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES	5
 
 #define SNDRV_PCM_TRIGGER_STOP		0
 #define SNDRV_PCM_TRIGGER_START		1
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 6828ed2..5f539d7 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -392,8 +392,9 @@ struct snd_pcm_hw_params {
 	unsigned int msbits;		/* R: used most significant bits */
 	unsigned int rate_num;		/* R: rate numerator */
 	unsigned int rate_den;		/* R: rate denominator */
-	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames */
-	unsigned char reserved[64];	/* reserved for future */
+	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames, indicates buffering after hw_ptr updates */
+	unsigned int max_inflight_bytes;/* R: typically DMA burst in bytes, indicates buffering before hw_ptr updates */
+	unsigned char reserved[60];	/* reserved for future */
 };
 
 enum {
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 1656ca9..06b44ed 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1827,6 +1827,23 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int snd_pcm_lib_ioctl_max_inflight_bytes(
+	struct snd_pcm_substream *substream,
+	void *arg)
+{
+	struct snd_pcm_hw_params *params = arg;
+
+	params->max_inflight_bytes = substream->runtime->hw.max_inflight_bytes;
+	/*
+	 * Sanity check that max_inflight_bytes isn't larger than buffer_size,
+	 * couldn't think of any other checks
+	 */
+	if (params->max_inflight_bytes > substream->runtime->buffer_size)
+		params->max_inflight_bytes = substream->runtime->buffer_size;
+
+	return 0;
+}
+
 /**
  * snd_pcm_lib_ioctl - a generic PCM ioctl callback
  * @substream: the pcm substream instance
@@ -1850,6 +1867,9 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
 		return snd_pcm_lib_ioctl_channel_info(substream, arg);
 	case SNDRV_PCM_IOCTL1_FIFO_SIZE:
 		return snd_pcm_lib_ioctl_fifo_size(substream, arg);
+	case SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES:
+		return snd_pcm_lib_ioctl_max_inflight_bytes(substream, arg);
+
 	}
 	return -ENXIO;
 }
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 1965d83..a29b5af 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -292,6 +292,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 
 	params->info = 0;
 	params->fifo_size = 0;
+	params->max_inflight_bytes = 0;
 	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS))
 		params->msbits = 0;
 	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) {
@@ -449,6 +450,12 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 				return changed;
 		}
 	}
+	if (!params->max_inflight_bytes) {
+		changed = substream->ops->ioctl(substream,
+				SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES, params);
+		if (changed < 0)
+			return changed;
+	}
 	params->rmask = 0;
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 4/7] ALSA: core: add report of max inflight bytes

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:27 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Report size of data burst before updating the hw_ptr, e.g. while DMA
> operations are on-going.
> 
> This can help fix two issues with stale data discussed many times over
> on the alsa-devel mailing list (refilling/reading ring buffer too late
> during capture or rewinding too close to the hw_ptr during playback)
> 
> This patch only reports the maximum burst of data and does not provide
> hooks to negotiate its size. This might be useful to lower power or
> reduce latency, but isn't typically supported by fixed-function DMA
> hardware.

This needs to be discussed rather from the actual demand.

 From the API change POV, the only (and the biggest) question is
whether hw_params is the best API.  It looks feasible, so far, but
someone else might have a better idea.  Let's see.


Takashi


> 
> The use of IOCTL1 is not really required but keep for symmetry with
> existing code used to retried fifo_size
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  include/sound/pcm.h         |  2 ++
>  include/uapi/sound/asound.h |  5 +++--
>  sound/core/pcm_lib.c        | 20 ++++++++++++++++++++
>  sound/core/pcm_native.c     |  7 +++++++
>  4 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 1accb8b..8c9d80a 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -56,6 +56,7 @@ struct snd_pcm_hardware {
>  	unsigned int periods_min;	/* min # of periods */
>  	unsigned int periods_max;	/* max # of periods */
>  	size_t fifo_size;		/* fifo size in bytes */
> +	unsigned int max_inflight_bytes;/* hw_ptr precision/fuzziness, e.g. due to DMA transfers */
>  };
>  
>  struct snd_pcm_substream;
> @@ -105,6 +106,7 @@ struct snd_pcm_ops {
>  #define SNDRV_PCM_IOCTL1_CHANNEL_INFO	2
>  #define SNDRV_PCM_IOCTL1_GSTATE		3
>  #define SNDRV_PCM_IOCTL1_FIFO_SIZE	4
> +#define SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES	5
>  
>  #define SNDRV_PCM_TRIGGER_STOP		0
>  #define SNDRV_PCM_TRIGGER_START		1
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 6828ed2..5f539d7 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -392,8 +392,9 @@ struct snd_pcm_hw_params {
>  	unsigned int msbits;		/* R: used most significant bits */
>  	unsigned int rate_num;		/* R: rate numerator */
>  	unsigned int rate_den;		/* R: rate denominator */
> -	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames */
> -	unsigned char reserved[64];	/* reserved for future */
> +	snd_pcm_uframes_t fifo_size;	/* R: chip FIFO size in frames, indicates buffering after hw_ptr updates */
> +	unsigned int max_inflight_bytes;/* R: typically DMA burst in bytes, indicates buffering before hw_ptr updates */
> +	unsigned char reserved[60];	/* reserved for future */
>  };
>  
>  enum {
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 1656ca9..06b44ed 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1827,6 +1827,23 @@ static int snd_pcm_lib_ioctl_fifo_size(struct snd_pcm_substream *substream,
>  	return 0;
>  }
>  
> +static int snd_pcm_lib_ioctl_max_inflight_bytes(
> +	struct snd_pcm_substream *substream,
> +	void *arg)
> +{
> +	struct snd_pcm_hw_params *params = arg;
> +
> +	params->max_inflight_bytes = substream->runtime->hw.max_inflight_bytes;
> +	/*
> +	 * Sanity check that max_inflight_bytes isn't larger than buffer_size,
> +	 * couldn't think of any other checks
> +	 */
> +	if (params->max_inflight_bytes > substream->runtime->buffer_size)
> +		params->max_inflight_bytes = substream->runtime->buffer_size;
> +
> +	return 0;
> +}
> +
>  /**
>   * snd_pcm_lib_ioctl - a generic PCM ioctl callback
>   * @substream: the pcm substream instance
> @@ -1850,6 +1867,9 @@ int snd_pcm_lib_ioctl(struct snd_pcm_substream *substream,
>  		return snd_pcm_lib_ioctl_channel_info(substream, arg);
>  	case SNDRV_PCM_IOCTL1_FIFO_SIZE:
>  		return snd_pcm_lib_ioctl_fifo_size(substream, arg);
> +	case SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES:
> +		return snd_pcm_lib_ioctl_max_inflight_bytes(substream, arg);
> +
>  	}
>  	return -ENXIO;
>  }
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 1965d83..a29b5af 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -292,6 +292,7 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  
>  	params->info = 0;
>  	params->fifo_size = 0;
> +	params->max_inflight_bytes = 0;
>  	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_SAMPLE_BITS))
>  		params->msbits = 0;
>  	if (params->rmask & (1 << SNDRV_PCM_HW_PARAM_RATE)) {
> @@ -449,6 +450,12 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
>  				return changed;
>  		}
>  	}
> +	if (!params->max_inflight_bytes) {
> +		changed = substream->ops->ioctl(substream,
> +				SNDRV_PCM_IOCTL1_MAX_INFLIGHT_BYTES, params);
> +		if (changed < 0)
> +			return changed;
> +	}
>  	params->rmask = 0;
>  	return 0;
>  }
> -- 
> 1.9.1
> 

[PATCH 5/7] ALSA: hda: add default value for max_inflight_bytes

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

set value to 128 bytes for legacy HDAudio, can be overridden
as needed (on a per-stream basis) for enhanced hardware with
more buffering capabilities

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/pci/hda/hda_controller.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 2ad3b44..922d7b9 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -587,6 +587,7 @@ static struct snd_pcm_hardware azx_pcm_hw = {
 	.periods_min =		2,
 	.periods_max =		AZX_MAX_FRAG,
 	.fifo_size =		0,
+	.max_inflight_bytes = 128 /* default value for all legacy HDAudio controllers, override as needed */
 };
 
 static int azx_pcm_open(struct snd_pcm_substream *substream)
-- 
1.9.1

Re: [PATCH 5/7] ALSA: hda: add default value for max_inflight_bytes

[Hardik Shah]

On Fri, Sep 30, 2016 at 06:13:28PM +0530, Subhransu S. Prusty wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> set value to 128 bytes for legacy HDAudio, can be overridden
> as needed (on a per-stream basis) for enhanced hardware with
> more buffering capabilities
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/pci/hda/hda_controller.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
> index 2ad3b44..922d7b9 100644
> --- a/sound/pci/hda/hda_controller.c
> +++ b/sound/pci/hda/hda_controller.c
> @@ -587,6 +587,7 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>  	.periods_min =		2,
>  	.periods_max =		AZX_MAX_FRAG,
>  	.fifo_size =		0,
> +	.max_inflight_bytes = 128 /* default value for all legacy HDAudio controllers, override as needed */

This line is going beyond 100 chars

>  };
>  
>  static int azx_pcm_open(struct snd_pcm_substream *substream)
> -- 
> 1.9.1
> 

-- 

Re: [PATCH 5/7] ALSA: hda: add default value for max_inflight_bytes

[Pierre-Louis Bossart]

>> --- a/sound/pci/hda/hda_controller.c
>> +++ b/sound/pci/hda/hda_controller.c
>> @@ -587,6 +587,7 @@ static struct snd_pcm_hardware azx_pcm_hw = {
>>  	.periods_min =		2,
>>  	.periods_max =		AZX_MAX_FRAG,
>>  	.fifo_size =		0,
>> +	.max_inflight_bytes = 128 /* default value for all legacy HDAudio controllers, override as needed */
>
> This line is going beyond 100 chars

introducing multi-line comments makes the structure less readable so 
even if checkpatch complains with a warning I used a single line. If you 
disagree please suggest a format that leaves the readable AND meets the 
80 char/line rule.

[PATCH 6/7] ALSA: usb: no_period_wake and max_inflight_bytes report

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Enable no_period_wake and report max_inflight_bytes.

The estimate is a worst-case, it could be refined based on
actual number of packets per URB and total URBs, however this
information is only available in the prepare step

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/usb/pcm.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 44d178e..5bcc729 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -713,7 +713,8 @@ static int configure_endpoint(struct snd_usb_substream *subs)
 static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 			     struct snd_pcm_hw_params *hw_params)
 {
-	struct snd_usb_substream *subs = substream->runtime->private_data;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_usb_substream *subs = runtime->private_data;
 	struct audioformat *fmt;
 	int ret;
 
@@ -728,7 +729,6 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	subs->buffer_periods = params_periods(hw_params);
 	subs->channels = params_channels(hw_params);
 	subs->cur_rate = params_rate(hw_params);
-
 	fmt = find_format(subs);
 	if (!fmt) {
 		dev_dbg(&subs->dev->dev,
@@ -749,6 +749,26 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream,
 	subs->altset_idx = fmt->altset_idx;
 	subs->need_setup_ep = true;
 
+	/*
+	 * worst-case max_inflight_bytes is one URB (MAX_PACKS ms),
+	 * multiply by size of 1ms packet for this format. The delay
+	 * can be larger since there are up to MAX_URBS submitted,
+	 * we only track the max number of bytes transferred when one
+	 * URB is submitted.
+	 */
+	runtime->hw.max_inflight_bytes = DIV_ROUND_UP(
+		MAX_PACKS * subs->cur_rate * subs->channels *
+		snd_pcm_format_width(subs->pcm_format), 8 * 1000);
+
+	/*
+	 * The number of packets cannot exceed the ring buffer size, reflect
+	 * constraints for low-latency usages
+	 */
+	if (runtime->hw.max_inflight_bytes >
+		subs->period_bytes * subs->buffer_periods)
+		runtime->hw.max_inflight_bytes =
+			subs->period_bytes * subs->buffer_periods;
+
 	return 0;
 }
 
@@ -851,6 +871,7 @@ static struct snd_pcm_hardware snd_usb_hardware =
 	.info =			SNDRV_PCM_INFO_MMAP |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_BATCH |
+				SNDRV_PCM_INFO_NO_PERIOD_WAKEUP |
 				SNDRV_PCM_INFO_INTERLEAVED |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_PAUSE,
@@ -1225,6 +1246,7 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction)
 	subs->interface = -1;
 	subs->altset_idx = 0;
 	runtime->hw = snd_usb_hardware;
+	runtime->hw.max_inflight_bytes = 0; /* will be set in hw_params() */
 	runtime->private_data = subs;
 	subs->pcm_substream = substream;
 	/* runtime PM is also done there */
@@ -1328,7 +1350,7 @@ static void retire_capture_urb(struct snd_usb_substream *subs,
 		}
 	}
 
-	if (period_elapsed)
+	if (period_elapsed && !runtime->no_period_wakeup)
 		snd_pcm_period_elapsed(subs->pcm_substream);
 }
 
@@ -1542,7 +1564,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
-	if (period_elapsed)
+	if (period_elapsed && !runtime->no_period_wakeup)
 		snd_pcm_period_elapsed(subs->pcm_substream);
 }
 
-- 
1.9.1

[PATCH 7/7] ALSA: usb: take startup delay into account

[Subhransu S. Prusty]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

For playback usages, the endpoints are started before the prepare
step, and valid audio data will be rendered with a delay that
cannot be recovered.
Worst-case the initial delay due to buffering of empty URBS can
be up to 12ms

Tested with audio_time:

Timestamps without delay taken into account:
test$ ./audio_time -Dhw:1,0 -p
playback: systime: 120093327 nsec, audio time 125000000 nsec, systime delta -4906673
playback: systime: 245090755 nsec, audio time 250000000 nsec, systime delta -4909245
playback: systime: 370034641 nsec, audio time 375000000 nsec, systime delta -4965359
playback: systime: 495089634 nsec, audio time 500000000 nsec, systime delta -4910366

Timestamps with delay taken into account (12ms delay shown)
test$ ./audio_time -Dhw:1,0 -p -d
playback: systime: 120095090 nsec, audio time 108000000 nsec, systime delta 12095090
playback: systime: 245090932 nsec, audio time 232000000 nsec, systime delta 13090932
playback: systime: 370091792 nsec, audio time 357000000 nsec, systime delta 13091792
playback: systime: 495092309 nsec, audio time 482000000 nsec, systime delta 13092309

Suggested-by: Rakesh Ughreja <rakesh.ughreja@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 sound/usb/card.h |  1 +
 sound/usb/pcm.c  | 32 +++++++++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 71778ca..d128058 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -148,6 +148,7 @@ struct snd_usb_substream {
 
 	int last_frame_number;          /* stored frame number */
 	int last_delay;                 /* stored delay */
+	int start_delay;                /* initial delay due to empty frames */
 
 	struct {
 		int marker;
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 5bcc729..2bf7537 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
 	hwptr_done = subs->hwptr_done;
 	substream->runtime->delay = snd_usb_pcm_delay(subs,
 						substream->runtime->rate);
+	substream->runtime->delay += subs->start_delay;
 	spin_unlock(&subs->lock);
 	return hwptr_done / (substream->runtime->frame_bits >> 3);
 }
@@ -858,9 +859,34 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 
 	/* for playback, submit the URBs now; otherwise, the first hwptr_done
 	 * updates for all URBs would happen at the same time when starting */
-	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
+	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) {
 		ret = start_endpoints(subs, true);
 
+		/*
+		 * Since we submit empty URBS, the constant initial delay will
+		 * not be recovered and will be added to the dynamic delay
+		 * measured with the frame counter. Worst-case the
+		 * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms
+		 */
+		if (ret == 0) {
+			struct snd_usb_endpoint *ep = subs->data_endpoint;
+			int total_packs = 0;
+			int i;
+
+			for (i = 0; i < ep->nurbs - 1; i++) {
+				struct snd_urb_ctx *u = &ep->urb[i];
+
+				total_packs += u->packets;
+			}
+			subs->start_delay = DIV_ROUND_UP(total_packs *
+							 subs->cur_rate, 1000);
+			if (subs->start_delay)
+				dev_dbg(&subs->dev->dev,
+					"Initial delay for EP @%p: %d frames\n",
+					ep, subs->start_delay);
+		}
+	}
+
  unlock:
 	snd_usb_unlock_shutdown(subs->stream->chip);
 	return ret;
@@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
 	runtime->delay = subs->last_delay;
 	runtime->delay += frames;
 	subs->last_delay = runtime->delay;
+	runtime->delay += subs->start_delay;
 
 	/* realign last_frame_number */
 	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
@@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 		subs->last_delay = 0;
 	else
 		subs->last_delay -= processed;
-	runtime->delay = subs->last_delay;
-
+	runtime->delay = subs->last_delay + subs->start_delay;
 	/*
 	 * Report when delay estimate is off by more than 2ms.
 	 * The error should be lower than 2ms since the estimate relies
-- 
1.9.1

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:30 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> For playback usages, the endpoints are started before the prepare
> step, and valid audio data will be rendered with a delay that
> cannot be recovered.
> Worst-case the initial delay due to buffering of empty URBS can
> be up to 12ms
> 
> Tested with audio_time:
> 
> Timestamps without delay taken into account:
> test$ ./audio_time -Dhw:1,0 -p
> playback: systime: 120093327 nsec, audio time 125000000 nsec, systime delta -4906673
> playback: systime: 245090755 nsec, audio time 250000000 nsec, systime delta -4909245
> playback: systime: 370034641 nsec, audio time 375000000 nsec, systime delta -4965359
> playback: systime: 495089634 nsec, audio time 500000000 nsec, systime delta -4910366
> 
> Timestamps with delay taken into account (12ms delay shown)
> test$ ./audio_time -Dhw:1,0 -p -d
> playback: systime: 120095090 nsec, audio time 108000000 nsec, systime delta 12095090
> playback: systime: 245090932 nsec, audio time 232000000 nsec, systime delta 13090932
> playback: systime: 370091792 nsec, audio time 357000000 nsec, systime delta 13091792
> playback: systime: 495092309 nsec, audio time 482000000 nsec, systime delta 13092309

This is really a fix unlike other patches in the series.
Please split it from others, so that we can apply it easily.


Takashi

> 
> Suggested-by: Rakesh Ughreja <rakesh.ughreja@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/usb/card.h |  1 +
>  sound/usb/pcm.c  | 32 +++++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 71778ca..d128058 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -148,6 +148,7 @@ struct snd_usb_substream {
>  
>  	int last_frame_number;          /* stored frame number */
>  	int last_delay;                 /* stored delay */
> +	int start_delay;                /* initial delay due to empty frames */
>  
>  	struct {
>  		int marker;
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 5bcc729..2bf7537 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
>  	hwptr_done = subs->hwptr_done;
>  	substream->runtime->delay = snd_usb_pcm_delay(subs,
>  						substream->runtime->rate);
> +	substream->runtime->delay += subs->start_delay;
>  	spin_unlock(&subs->lock);
>  	return hwptr_done / (substream->runtime->frame_bits >> 3);
>  }
> @@ -858,9 +859,34 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
>  
>  	/* for playback, submit the URBs now; otherwise, the first hwptr_done
>  	 * updates for all URBs would happen at the same time when starting */
> -	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
> +	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) {
>  		ret = start_endpoints(subs, true);
>  
> +		/*
> +		 * Since we submit empty URBS, the constant initial delay will
> +		 * not be recovered and will be added to the dynamic delay
> +		 * measured with the frame counter. Worst-case the
> +		 * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms
> +		 */
> +		if (ret == 0) {
> +			struct snd_usb_endpoint *ep = subs->data_endpoint;
> +			int total_packs = 0;
> +			int i;
> +
> +			for (i = 0; i < ep->nurbs - 1; i++) {
> +				struct snd_urb_ctx *u = &ep->urb[i];
> +
> +				total_packs += u->packets;
> +			}
> +			subs->start_delay = DIV_ROUND_UP(total_packs *
> +							 subs->cur_rate, 1000);
> +			if (subs->start_delay)
> +				dev_dbg(&subs->dev->dev,
> +					"Initial delay for EP @%p: %d frames\n",
> +					ep, subs->start_delay);
> +		}
> +	}
> +
>   unlock:
>  	snd_usb_unlock_shutdown(subs->stream->chip);
>  	return ret;
> @@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
>  	runtime->delay = subs->last_delay;
>  	runtime->delay += frames;
>  	subs->last_delay = runtime->delay;
> +	runtime->delay += subs->start_delay;
>  
>  	/* realign last_frame_number */
>  	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
> @@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
>  		subs->last_delay = 0;
>  	else
>  		subs->last_delay -= processed;
> -	runtime->delay = subs->last_delay;
> -
> +	runtime->delay = subs->last_delay + subs->start_delay;
>  	/*
>  	 * Report when delay estimate is off by more than 2ms.
>  	 * The error should be lower than 2ms since the estimate relies
> -- 
> 1.9.1
> 

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Pierre-Louis Bossart]

>> For playback usages, the endpoints are started before the prepare
>> step, and valid audio data will be rendered with a delay that
>> cannot be recovered.
>> Worst-case the initial delay due to buffering of empty URBS can
>> be up to 12ms

> This is really a fix unlike other patches in the series.
> Please split it from others, so that we can apply it easily.

There was also additional discussions on this topic since I put this 
together, not sure it makes sense to merge this patch at the moment.
In the Windows driver, the URBs can be submitted at a specific time, 
which allows for synchronous starts of all endpoints (limited to the 1ms 
frame resolution). In Linux we can't since the start time is owned by 
the xhci driver and can't be modified by the class driver. I talked with 
Mathias Nyman and Baolu Lu on this before the summer and of course I 
became busy with other things. The short story is that there is a wider 
problem with USB start and linking endpoints that should be addressed as 
a single step. Maybe something to talk about at the miniconference?

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Takashi Iwai]

On Mon, 03 Oct 2016 17:04:23 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> For playback usages, the endpoints are started before the prepare
> >> step, and valid audio data will be rendered with a delay that
> >> cannot be recovered.
> >> Worst-case the initial delay due to buffering of empty URBS can
> >> be up to 12ms
> 
> > This is really a fix unlike other patches in the series.
> > Please split it from others, so that we can apply it easily.
> 
> There was also additional discussions on this topic since I put this
> together, not sure it makes sense to merge this patch at the moment.
> In the Windows driver, the URBs can be submitted at a specific time,
> which allows for synchronous starts of all endpoints (limited to the
> 1ms frame resolution). In Linux we can't since the start time is owned
> by the xhci driver and can't be modified by the class driver. I talked
> with Mathias Nyman and Baolu Lu on this before the summer and of
> course I became busy with other things. The short story is that there
> is a wider problem with USB start and linking endpoints that should be
> addressed as a single step. Maybe something to talk about at the
> miniconference?

Sure, why not.  Now it's close to LPC and we won't reach to the
solution soonish yet.


Takashi

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Takashi Sakamoto]

Hi,

On Sep 30 2016 21:43, Subhransu S. Prusty wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> For playback usages, the endpoints are started before the prepare
> step,

As long as I understand, in ALSA USB audio device class driver, 
submitting URBs starts at .prepare called by PCM core. So not before it 
as long as the 'prepare step' means .prepare call.

> and valid audio data will be rendered with a delay that
> cannot be recovered.
> Worst-case the initial delay due to buffering of empty URBS can
> be up to 12ms

I thinks the aim of this patch is to add proper delay to current 
calculation of estimated delay, due to the URBs accumulated between 
.prepare and .trigger call. If so, it's better to explain about it in 
the comment so that the other developers such as me can follow this 
improvement.

> Tested with audio_time:
>
> Timestamps without delay taken into account:
> test$ ./audio_time -Dhw:1,0 -p
> playback: systime: 120093327 nsec, audio time 125000000 nsec, systime delta -4906673
> playback: systime: 245090755 nsec, audio time 250000000 nsec, systime delta -4909245
> playback: systime: 370034641 nsec, audio time 375000000 nsec, systime delta -4965359
> playback: systime: 495089634 nsec, audio time 500000000 nsec, systime delta -4910366
>
> Timestamps with delay taken into account (12ms delay shown)
> test$ ./audio_time -Dhw:1,0 -p -d
> playback: systime: 120095090 nsec, audio time 108000000 nsec, systime delta 12095090
> playback: systime: 245090932 nsec, audio time 232000000 nsec, systime delta 13090932
> playback: systime: 370091792 nsec, audio time 357000000 nsec, systime delta 13091792
> playback: systime: 495092309 nsec, audio time 482000000 nsec, systime delta 13092309
>
> Suggested-by: Rakesh Ughreja <rakesh.ughreja@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  sound/usb/card.h |  1 +
>  sound/usb/pcm.c  | 32 +++++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
>
> diff --git a/sound/usb/card.h b/sound/usb/card.h
> index 71778ca..d128058 100644
> --- a/sound/usb/card.h
> +++ b/sound/usb/card.h
> @@ -148,6 +148,7 @@ struct snd_usb_substream {
>
>  	int last_frame_number;          /* stored frame number */
>  	int last_delay;                 /* stored delay */
> +	int start_delay;                /* initial delay due to empty frames */
>
>  	struct {
>  		int marker;
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index 5bcc729..2bf7537 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -86,6 +86,7 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
>  	hwptr_done = subs->hwptr_done;
>  	substream->runtime->delay = snd_usb_pcm_delay(subs,
>  						substream->runtime->rate);
> +	substream->runtime->delay += subs->start_delay;
>  	spin_unlock(&subs->lock);
>  	return hwptr_done / (substream->runtime->frame_bits >> 3);
>  }
> @@ -858,9 +859,34 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
>
>  	/* for playback, submit the URBs now; otherwise, the first hwptr_done
>  	 * updates for all URBs would happen at the same time when starting */
> -	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
> +	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) {
>  		ret = start_endpoints(subs, true);
>
> +		/*
> +		 * Since we submit empty URBS, the constant initial delay will
> +		 * not be recovered and will be added to the dynamic delay
> +		 * measured with the frame counter. Worst-case the
> +		 * startup delay can be (MAX_URBS-1) * MAX_PACKS = 12ms
> +		 */
> +		if (ret == 0) {
> +			struct snd_usb_endpoint *ep = subs->data_endpoint;
> +			int total_packs = 0;
> +			int i;
> +
> +			for (i = 0; i < ep->nurbs - 1; i++) {
> +				struct snd_urb_ctx *u = &ep->urb[i];
> +
> +				total_packs += u->packets;
> +			}
> +			subs->start_delay = DIV_ROUND_UP(total_packs *
> +							 subs->cur_rate, 1000);
> +			if (subs->start_delay)
> +				dev_dbg(&subs->dev->dev,
> +					"Initial delay for EP @%p: %d frames\n",
> +					ep, subs->start_delay);
> +		}
> +	}
> +
>   unlock:
>  	snd_usb_unlock_shutdown(subs->stream->chip);
>  	return ret;
> @@ -1549,6 +1575,7 @@ static void prepare_playback_urb(struct snd_usb_substream *subs,
>  	runtime->delay = subs->last_delay;
>  	runtime->delay += frames;
>  	subs->last_delay = runtime->delay;
> +	runtime->delay += subs->start_delay;
>
>  	/* realign last_frame_number */
>  	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
> @@ -1597,8 +1624,7 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
>  		subs->last_delay = 0;
>  	else
>  		subs->last_delay -= processed;
> -	runtime->delay = subs->last_delay;
> -
> +	runtime->delay = subs->last_delay + subs->start_delay;
>  	/*
>  	 * Report when delay estimate is off by more than 2ms.
>  	 * The error should be lower than 2ms since the estimate relies


Regards

Takashi Sakamoto

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Pierre-Louis Bossart]

On 10/3/16 1:46 AM, Takashi Sakamoto wrote:
> Hi,
>
> On Sep 30 2016 21:43, Subhransu S. Prusty wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> For playback usages, the endpoints are started before the prepare
>> step,
>
> As long as I understand, in ALSA USB audio device class driver,
> submitting URBs starts at .prepare called by PCM core. So not before it
> as long as the 'prepare step' means .prepare call.

The comment should have said 'started during the prepare step', you're 
right.

>
>> and valid audio data will be rendered with a delay that
>> cannot be recovered.
>> Worst-case the initial delay due to buffering of empty URBS can
>> be up to 12ms
>
> I thinks the aim of this patch is to add proper delay to current
> calculation of estimated delay, due to the URBs accumulated between
> .prepare and .trigger call. If so, it's better to explain about it in
> the comment so that the other developers such as me can follow this
> improvement.

Yes indeed. will fix but as replied to Takashi Iwai there are wider 
questions on why endpoints can't be linked and why the start times can't 
be set by the class driver.

Re: [PATCH 7/7] ALSA: usb: take startup delay into account

[Takashi Sakamoto]

Hi Louis,

On Oct 4 2016 00:08, Pierre-Louis Bossart wrote:
> On 10/3/16 1:46 AM, Takashi Sakamoto wrote:
>> Hi,
>>
>> On Sep 30 2016 21:43, Subhransu S. Prusty wrote:
>>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>
>>> For playback usages, the endpoints are started before the prepare
>>> step,
>>
>> As long as I understand, in ALSA USB audio device class driver,
>> submitting URBs starts at .prepare called by PCM core. So not before it
>> as long as the 'prepare step' means .prepare call.
> 
> The comment should have said 'started during the prepare step', you're
> right.

OK. Thanks for your confirmation.

We have the similar design in drivers with IEC 61883-1/6 packet
streaming engine for Audio and Music units on IEEE 1394 bus (locates in
sound/firewire/*). This is the reason I have interests in this patchset.

>>> and valid audio data will be rendered with a delay that
>>> cannot be recovered.
>>> Worst-case the initial delay due to buffering of empty URBS can
>>> be up to 12ms
>>
>> I thinks the aim of this patch is to add proper delay to current
>> calculation of estimated delay, due to the URBs accumulated between
>> .prepare and .trigger call. If so, it's better to explain about it in
>> the comment so that the other developers such as me can follow this
>> improvement.
> 
> Yes indeed. will fix but as replied to Takashi Iwai there are wider
> questions on why endpoints can't be linked and why the start times can't
> be set by the class driver.

>From me, an additional question.

There's no guarantee that applications call ioctl() with
SNDRV_PCM_IOCTL_START within 12 msec after ioctl() with
SNDRV_PCM_IOCTL_PREPARE. In this case, more URBs might be sumitted by
usb_submit_urb/snd_complete_urb() cycle, as long as I understand. So
it's quite rough to compute based on MAX_URBS (=12).

I think it better to use actual time (monotonic time or something
suitable) to compute the additional delay between .prepare and .trigger
calls. Or calculate queued packet between the two calls, then compute
the delay.


Thanks

Takashi Sakamoto

Re: [PATCH 0/7] ALSA: Add rewinds disabled, delays, max_inflight_bytes

[Takashi Iwai]

On Fri, 30 Sep 2016 14:43:23 +0200,
Subhransu S. Prusty wrote:
> 
> Set of patches to fix issues with delays, hw_ptr fuzziness [1] and
> increased buffering w/ DSPs
> 
> 1. Rewinds can be disabled  when data written in ring buffer will never be
> validated. This allow for new HDaudio SPIB DMA functionality(allow fetch up to the
> application pointer, no rewind supported)
> 
> 2. Report max in-flight bytes to avoid problems with stale data (like late
> wake-ups, rewinds)
> 
> 3. add new estimate for USB startup delay.

Well, all these three are really individual and independent features.
So, please send three patch sets instead.

For example, I would apply the USB startup delay fix easily, while the
inflight bytes feature needs more discussion.  The rewind disablement
needs a bit more discussion (e.g. can it be disabled from the driver
side as well?).


thanks,

Takashi

> 
> [1]
> http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093646.html
> 
> 
> Pierre-Louis Bossart (6):
>   ALSA: core: let low-level driver or userspace disable rewinds
>   ALSA: core: add .update_appl_ptr callback for pcm ops
>   ALSA: core: add report of max inflight bytes
>   ALSA: hda: add default value for max_inflight_bytes
>   ALSA: usb: no_period_wake and max_inflight_bytes report
>   ALSA: usb: take startup delay into account
> 
> Ramesh Babu (1):
>   ALSA: pcm: avoid mmap of control data if .update_appl_ptr is
>     implemented
> 
>  include/sound/pcm.h            |  4 +++
>  include/uapi/sound/asound.h    |  6 ++--
>  sound/core/pcm_lib.c           | 26 ++++++++++++++++++
>  sound/core/pcm_native.c        | 51 +++++++++++++++++++++++++++++++++-
>  sound/pci/hda/hda_controller.c |  1 +
>  sound/usb/card.h               |  1 +
>  sound/usb/pcm.c                | 62 +++++++++++++++++++++++++++++++++++++-----
>  7 files changed, 141 insertions(+), 10 deletions(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH 0/7] ALSA: Add rewinds disabled, delays, max_inflight_bytes

[Subhransu S. Prusty]

On Fri, Sep 30, 2016 at 03:13:19PM +0200, Takashi Iwai wrote:
> On Fri, 30 Sep 2016 14:43:23 +0200,
> Subhransu S. Prusty wrote:
> > 
> > Set of patches to fix issues with delays, hw_ptr fuzziness [1] and
> > increased buffering w/ DSPs
> > 
> > 1. Rewinds can be disabled  when data written in ring buffer will never be
> > validated. This allow for new HDaudio SPIB DMA functionality(allow fetch up to the
> > application pointer, no rewind supported)
> > 
> > 2. Report max in-flight bytes to avoid problems with stale data (like late
> > wake-ups, rewinds)
> > 
> > 3. add new estimate for USB startup delay.
> 
> Well, all these three are really individual and independent features.
> So, please send three patch sets instead.

Sure. I will send separate patch series for each of them.

> 
> For example, I would apply the USB startup delay fix easily, while the
> inflight bytes feature needs more discussion.  The rewind disablement
> needs a bit more discussion (e.g. can it be disabled from the driver
> side as well?).

No I think. Considering the SPIB usecases, user may decide to use SPIB or not.

Regards,
Subhransu

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > [1]
> > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-June/093646.html
> > 
> > 
> > Pierre-Louis Bossart (6):
> >   ALSA: core: let low-level driver or userspace disable rewinds
> >   ALSA: core: add .update_appl_ptr callback for pcm ops
> >   ALSA: core: add report of max inflight bytes
> >   ALSA: hda: add default value for max_inflight_bytes
> >   ALSA: usb: no_period_wake and max_inflight_bytes report
> >   ALSA: usb: take startup delay into account
> > 
> > Ramesh Babu (1):
> >   ALSA: pcm: avoid mmap of control data if .update_appl_ptr is
> >     implemented
> > 
> >  include/sound/pcm.h            |  4 +++
> >  include/uapi/sound/asound.h    |  6 ++--
> >  sound/core/pcm_lib.c           | 26 ++++++++++++++++++
> >  sound/core/pcm_native.c        | 51 +++++++++++++++++++++++++++++++++-
> >  sound/pci/hda/hda_controller.c |  1 +
> >  sound/usb/card.h               |  1 +
> >  sound/usb/pcm.c                | 62 +++++++++++++++++++++++++++++++++++++-----
> >  7 files changed, 141 insertions(+), 10 deletions(-)
> > 
> > -- 
> > 1.9.1
> > 

--