[alsa-devel] [PATCH 0/7] ALSA: Add rewinds disabled, delays, max_inflight_bytes
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(-)
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:
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
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 --
@@ -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.
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
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))
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 *//* 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
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 :)
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
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
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.
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);
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;/** we want to force sync pointer,* if driver implements appl_ptr_update*/if (substream->ops->appl_ptr_update)return -ENXIO;-- 1.9.1
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
--
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; }
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
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)
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
--
--- 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.
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); }
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
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
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?
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
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
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.
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
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
- 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)
- Report max in-flight bytes to avoid problems with stale data (like late
wake-ups, rewinds)
- 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
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
- 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)
- Report max in-flight bytes to avoid problems with stale data (like late
wake-ups, rewinds)
- 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
--
participants (6)
-
Hardik Shah -
Pierre-Louis Bossart -
Subhransu S. Prusty -
Takashi Iwai -
Takashi Sakamoto -
Vinod Koul