[RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms

[RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms

[Sriram Periyasamy]

Skylake audio controller supports SPIB (Software Position in buffer)
capability, which can be used to inform position of application pointer
to host DMA controller. When SPIB mode is enabled, driver could write
the application pointer position in SPIB register. Host DMA will make
sure it won't read/write beyond bytes specified in SPIB register.

SPIB mode will be useful in low power use cases, where DSP could
pre-fetch large buffers to avoid frequent wakes caused due to interrupts.

To support SPIB in the driver, save the spib values in stream context
which can be restored during resume from S3. Add new hw_params flag to
explicitly tell driver that rewinds will never be used.

Please find the references for the previous discussions at [1][2]

[1]
http://mailman.alsa-project.org/pipermail/alsa-devel/2018-January/131329.
html

[2]
https://patchwork.kernel.org/patch/9795233/

and the patches of previous versions at [3][4][5]

[3]
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121967.html

[4]
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-June/121683.html

[5]
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-May/120676.html

Pierre-Louis Bossart (1):
  ALSA: core: let low-level driver or userspace disable rewinds

Ramesh Babu (2):
  ALSA: hda: ext: add spib to stream context
  ASoC: Intel: Skylake: Add support for spib mode

 include/sound/hdaudio_ext.h       |  1 +
 include/sound/pcm.h               |  1 +
 include/uapi/sound/asound.h       |  1 +
 sound/core/pcm_native.c           |  8 ++++++++
 sound/hda/ext/hdac_ext_stream.c   |  2 ++
 sound/soc/intel/skylake/skl-pcm.c | 43 ++++++++++++++++++++++++++++++++++++++-
 6 files changed, 55 insertions(+), 1 deletion(-)

-- 
2.7.4

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

[Sriram Periyasamy]

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.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@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 e054c583d3b3..f60397eb4aab 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -379,6 +379,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 ed0a120d4f08..ff57e4c89de4 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -377,6 +377,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 77ba50ddcf9e..4616420cdbf7 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -692,6 +692,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;
@@ -2614,6 +2616,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 -ENODEV;
+
 	snd_pcm_stream_lock_irq(substream);
 	ret = do_pcm_hwsync(substream);
 	if (!ret)
@@ -2632,6 +2637,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 -ENODEV;
+
 	snd_pcm_stream_lock_irq(substream);
 	ret = do_pcm_hwsync(substream);
 	if (!ret)
-- 
2.7.4

[RESEND][PATCH v4 2/3] ALSA: hda: ext: add spib to stream context

[Sriram Periyasamy]

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

Platforms like skylake support SPIB (software position index in
Buffer) capability, through which application pointer can be
programmed in DMA. This helps DMA stop rendering stale data.

This patch saves spib values in stream context which can be
restored during resume from S3.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
---
 include/sound/hdaudio_ext.h     | 1 +
 sound/hda/ext/hdac_ext_stream.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/include/sound/hdaudio_ext.h b/include/sound/hdaudio_ext.h
index 9c14e21dda85..34c41496fbc7 100644
--- a/include/sound/hdaudio_ext.h
+++ b/include/sound/hdaudio_ext.h
@@ -89,6 +89,7 @@ struct hdac_ext_stream {
 
 	u32 dpib;
 	u32 lpib;
+	u32 spib;
 	bool decoupled:1;
 	bool link_locked:1;
 	bool link_prepared;
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c
index c96d7a7a36af..c5212709bdb7 100644
--- a/sound/hda/ext/hdac_ext_stream.c
+++ b/sound/hda/ext/hdac_ext_stream.c
@@ -461,6 +461,8 @@ int snd_hdac_ext_stream_set_spib(struct hdac_ext_bus *ebus,
 	}
 
 	writel(value, stream->spib_addr);
+	/* save the spib value in stream context */
+	stream->spib = value;
 
 	return 0;
 }
-- 
2.7.4

[RESEND][PATCH v4 3/3] ASoC: Intel: Skylake: Add support for spib mode

[Sriram Periyasamy]

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

Skylake audio controller supports SPIB (Software Position in buffer)
capability, which can be used to inform position of application pointer
to host DMA controller.
When SPIB mode is enabled, driver could write the application pointer
position in SPIB register. Host DMA will make sure it won't
read/write beyond bytes specified in SPIB register.

SPIB mode will be useful in low power use cases, where DSP could
pre-fetch large buffers to avoid frequent wakes caused due to
interrupts.

Skylake driver makes use of no_rewind flag and appl_ptr_update
callback to enable and update SPIB register respectively.

Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 43 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 15cb8ac3e374..01780a50a322 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -43,7 +43,8 @@ static const struct snd_pcm_hardware azx_pcm_hw = {
 				 SNDRV_PCM_INFO_SYNC_START |
 				 SNDRV_PCM_INFO_HAS_WALL_CLOCK | /* legacy */
 				 SNDRV_PCM_INFO_HAS_LINK_ATIME |
-				 SNDRV_PCM_INFO_NO_PERIOD_WAKEUP),
+				 SNDRV_PCM_INFO_NO_PERIOD_WAKEUP |
+				 SNDRV_PCM_INFO_SYNC_APPLPTR),
 	.formats =		SNDRV_PCM_FMTBIT_S16_LE |
 				SNDRV_PCM_FMTBIT_S32_LE |
 				SNDRV_PCM_FMTBIT_S24_LE,
@@ -145,6 +146,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	unsigned int format_val;
 	struct hdac_stream *hstream;
 	struct hdac_ext_stream *stream;
+	struct snd_pcm_runtime *runtime;
 	int err;
 
 	hstream = snd_hdac_get_stream(bus, params->stream,
@@ -170,6 +172,11 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	if (err < 0)
 		return err;
 
+	/* Enable SPIB if no_rewinds flag is set */
+	runtime = hdac_stream(stream)->substream->runtime;
+	if (runtime->no_rewinds)
+		snd_hdac_ext_stream_spbcap_enable(ebus, true, hstream->index);
+
 	hdac_stream(stream)->prepared = 1;
 
 	return 0;
@@ -366,9 +373,14 @@ static int skl_pcm_hw_free(struct snd_pcm_substream *substream,
 {
 	struct hdac_ext_bus *ebus = dev_get_drvdata(dai->dev);
 	struct hdac_ext_stream *stream = get_hdac_ext_stream(substream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hdac_stream *hstream = hdac_stream(stream);
 
 	dev_dbg(dai->dev, "%s: %s\n", __func__, dai->name);
 
+	if (runtime->no_rewinds)
+		snd_hdac_ext_stream_spbcap_enable(ebus, false, hstream->index);
+
 	snd_hdac_stream_cleanup(hdac_stream(stream));
 	hdac_stream(stream)->prepared = 0;
 
@@ -444,6 +456,7 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd,
 	struct skl_module_cfg *mconfig;
 	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
 	struct hdac_ext_stream *stream = get_hdac_ext_stream(substream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_soc_dapm_widget *w;
 	int ret;
 
@@ -469,6 +482,10 @@ static int skl_pcm_trigger(struct snd_pcm_substream *substream, int cmd,
 			snd_hdac_ext_stream_set_dpibr(ebus, stream,
 							stream->lpib);
 			snd_hdac_ext_stream_set_lpib(stream, stream->lpib);
+
+			if (runtime->no_rewinds)
+				snd_hdac_ext_stream_set_spib(ebus,
+						stream, stream->spib);
 		}
 
 	case SNDRV_PCM_TRIGGER_START:
@@ -1095,6 +1112,29 @@ static int skl_platform_pcm_trigger(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+/* Update SPIB register with application position */
+static int skl_platform_ack(struct snd_pcm_substream *substream)
+{
+	struct hdac_ext_stream *estream = get_hdac_ext_stream(substream);
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct hdac_ext_bus *ebus = get_bus_ctx(substream);
+	ssize_t appl_pos, buf_size;
+	u32 spib;
+
+	/* Use spib mode only if no_rewind mode is set */
+	if (!runtime->no_rewinds)
+		return 0;
+
+	appl_pos = frames_to_bytes(runtime, runtime->control->appl_ptr);
+	buf_size = frames_to_bytes(runtime, runtime->buffer_size);
+
+	spib = appl_pos % buf_size;
+
+	/* Allowable value for SPIB is 1 byte to max buffer size */
+	spib = (spib == 0) ? buf_size : spib;
+	return snd_hdac_ext_stream_set_spib(ebus, estream, spib);
+}
+
 static snd_pcm_uframes_t skl_platform_pcm_pointer
 			(struct snd_pcm_substream *substream)
 {
@@ -1202,6 +1242,7 @@ static const struct snd_pcm_ops skl_platform_ops = {
 	.get_time_info =  skl_get_time_info,
 	.mmap = snd_pcm_lib_default_mmap,
 	.page = snd_pcm_sgbuf_ops_page,
+	.ack = skl_platform_ack,
 };
 
 static void skl_pcm_free(struct snd_pcm *pcm)
-- 
2.7.4

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

[Takashi Iwai]

On Tue, 20 Mar 2018 17:01:06 +0100,
Sriram Periyasamy 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.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>

Well, I'm still not convinced with this flag.

First off, does it really need to be per PCM stream?  The introducing
something to hw_parms implies that it varies per application.  But I
can't imagine that a system requires different behavior per stream
regarding such a thing.

Second, the driver can implement a check in PCM ack callback to
prevent the rewind, too.  Then there is no need to touch the PCM
core.


thanks,

Takashi

> ---
>  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 e054c583d3b3..f60397eb4aab 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -379,6 +379,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 ed0a120d4f08..ff57e4c89de4 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -377,6 +377,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 77ba50ddcf9e..4616420cdbf7 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -692,6 +692,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;
> @@ -2614,6 +2616,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 -ENODEV;
> +
>  	snd_pcm_stream_lock_irq(substream);
>  	ret = do_pcm_hwsync(substream);
>  	if (!ret)
> @@ -2632,6 +2637,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 -ENODEV;
> +
>  	snd_pcm_stream_lock_irq(substream);
>  	ret = do_pcm_hwsync(substream);
>  	if (!ret)
> -- 
> 2.7.4
> 

Re: [RESEND][PATCH v4 0/3] Add SPIB Support for Intel Skylake platforms

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 618 bytes --]

On Tue, Mar 20, 2018 at 09:31:05PM +0530, Sriram Periyasamy wrote:
> Skylake audio controller supports SPIB (Software Position in buffer)
> capability, which can be used to inform position of application pointer

You probably want to cut down on the number of tags you're adding in
your subject lines - what I see when I look at this mail in my inbox is:

-> 1061   T 03/20 Sriram Periyasa (1.7K) [alsa-devel][RESEND][PATCH v4 0/3] Add

so I can't see *anything* about what the patch is about!  The
[alsa-devel] bit is probably the main thing that can go, I know the
mailing list software adds it which is unfortuate.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Sriram Periyasamy]

On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
> On Tue, 20 Mar 2018 17:01:06 +0100,
> Sriram Periyasamy 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.
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> 
> Well, I'm still not convinced with this flag.
> 
> First off, does it really need to be per PCM stream?  The introducing

Flag per PCM stream helps where each stream in given system may have
different requirement such as low power or low latency based on the
use case. For example in case of low power stream, driver can perform
required optimizations at hardware level based on the no_rewind flag.

> something to hw_parms implies that it varies per application.  But I
> can't imagine that a system requires different behavior per stream
> regarding such a thing.
> 
> Second, the driver can implement a check in PCM ack callback to
> prevent the rewind, too.  Then there is no need to touch the PCM
> core.
> 

As per the previous discussion at [1],

[1]
https://patchwork.kernel.org/patch/9795233/

from Pierre,

"The application (which is in most cases an audio server) *knows* if it
requires rewinds or not. It's part of its design, with rewinds typically
disabled if period interrupts are required. It's been that way for a
number of years now. The use of rewinds is typically associated with the
combination of a large buffer and no interrupts (having either of the
two would not require rewinds).

So the idea is that the application makes a statement that rewinds will
not be used, and the low-level driver makes use of the information to
enable whatever optimizations are available at the hardware level.

Exposing more information to userspace would quickly lead to a confusing
decision-making and would require more than just a flag."

Thanks,
Sriram.

> 
> thanks,
> 
> Takashi
> 
> > -- 
> > 2.7.4
> > 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

-- 

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

[Takashi Iwai]

On Sun, 25 Mar 2018 12:46:43 +0200,
Sriram Periyasamy wrote:
> 
> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
> > On Tue, 20 Mar 2018 17:01:06 +0100,
> > Sriram Periyasamy 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.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> > 
> > Well, I'm still not convinced with this flag.
> > 
> > First off, does it really need to be per PCM stream?  The introducing
> 
> Flag per PCM stream helps where each stream in given system may have
> different requirement such as low power or low latency based on the
> use case. For example in case of low power stream, driver can perform
> required optimizations at hardware level based on the no_rewind flag.

Yes, but does it really need to be PCM stream, i.e. per application?
Certainly the system will be using some sound backend (like PA).  In
which scenario can such behavior change -- some application uses a
different backend on a phone or a tablet?


> > something to hw_parms implies that it varies per application.  But I
> > can't imagine that a system requires different behavior per stream
> > regarding such a thing.
> > 
> > Second, the driver can implement a check in PCM ack callback to
> > prevent the rewind, too.  Then there is no need to touch the PCM
> > core.
> > 
> 
> As per the previous discussion at [1],
> 
> [1]
> https://patchwork.kernel.org/patch/9795233/
> 
> from Pierre,
> 
> "The application (which is in most cases an audio server) *knows* if it
> requires rewinds or not. It's part of its design, with rewinds typically
> disabled if period interrupts are required. It's been that way for a
> number of years now. The use of rewinds is typically associated with the
> combination of a large buffer and no interrupts (having either of the
> two would not require rewinds).
> 
> So the idea is that the application makes a statement that rewinds will
> not be used, and the low-level driver makes use of the information to
> enable whatever optimizations are available at the hardware level.
> 
> Exposing more information to userspace would quickly lead to a confusing
> decision-making and would require more than just a flag."

And, requiring *that* information is already confusing, IMO.
Think from the application writer POV: what is the relation between
the power-saving and no_rewind behavior in application level at all?
If you have no idea about the hardware details, they are totally
irrelevant.

Or, think like this way: imagine a hardware that requires a different
constraint, e.g. the power-of-two buffer size, for power-efficient
operation.  What would you do?  Adding a new power_of_two bit flag
into hw_params?  Likely not.

In such a case, I would expect some operation mode switch
(e.g. power-saving vs low latency or whatever) instead of a very
specific hw_parmas flag.  It might be a module option, via ALSA
control, or something else.  But it's clearer for which purpose it's
present, at least, and it can be implemented well without changing the
existing API.


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 3/25/18 9:58 AM, Takashi Iwai wrote:
> On Sun, 25 Mar 2018 12:46:43 +0200,
> Sriram Periyasamy wrote:
>>
>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
>>> On Tue, 20 Mar 2018 17:01:06 +0100,
>>> Sriram Periyasamy 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.
>>>>
>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>
>>> Well, I'm still not convinced with this flag.
>>>
>>> First off, does it really need to be per PCM stream?  The introducing
>>
>> Flag per PCM stream helps where each stream in given system may have
>> different requirement such as low power or low latency based on the
>> use case. For example in case of low power stream, driver can perform
>> required optimizations at hardware level based on the no_rewind flag.
> 
> Yes, but does it really need to be PCM stream, i.e. per application?
> Certainly the system will be using some sound backend (like PA).  In
> which scenario can such behavior change -- some application uses a
> different backend on a phone or a tablet?

This is intended for the case where the system server exposes a 
'deep-buffer' PCM device for music playback in low-power mode and a 
separate one for system sounds or anything that requires interactivity.
The need for rewinding is really for the case where the interactive 
system sounds are mixed with music, when you have separation between 
types of sounds and hardware/firmware mixing then the rewinds are 
unnecessary.

If there are multiple applications using different PCM devices each 
(which is a bit hypothetical to me) there is no way to know ahead of 
time when the modules are loaded if the application will perform rewinds 
due to its interactive nature or will just stream without ever 
invalidating the ring buffer. So yes it's per stream.

> 
> 
>>> something to hw_parms implies that it varies per application.  But I
>>> can't imagine that a system requires different behavior per stream
>>> regarding such a thing.
>>>
>>> Second, the driver can implement a check in PCM ack callback to
>>> prevent the rewind, too.  Then there is no need to touch the PCM
>>> core.
>>>
>>
>> As per the previous discussion at [1],
>>
>> [1]
>> https://patchwork.kernel.org/patch/9795233/
>>
>> from Pierre,
>>
>> "The application (which is in most cases an audio server) *knows* if it
>> requires rewinds or not. It's part of its design, with rewinds typically
>> disabled if period interrupts are required. It's been that way for a
>> number of years now. The use of rewinds is typically associated with the
>> combination of a large buffer and no interrupts (having either of the
>> two would not require rewinds).
>>
>> So the idea is that the application makes a statement that rewinds will
>> not be used, and the low-level driver makes use of the information to
>> enable whatever optimizations are available at the hardware level.
>>
>> Exposing more information to userspace would quickly lead to a confusing
>> decision-making and would require more than just a flag."
> 
> And, requiring *that* information is already confusing, IMO.
> Think from the application writer POV: what is the relation between
> the power-saving and no_rewind behavior in application level at all?
> If you have no idea about the hardware details, they are totally
> irrelevant.

I feel like disabling IRQs or disabling rewinds is the same level of 
information, you set the flags without necessary knowing all the power 
savings down to the mW level. But it provides an opportunity to save 
power with additional degrees of freedom for implementations.

An additional benefit of using the underlying SPIB register on Intel 
hardware is that the DMA hardware will not wrap-around, which can lead 
to better detection of real-time issues and a guarantee that stale data 
will not be played.

> 
> Or, think like this way: imagine a hardware that requires a different
> constraint, e.g. the power-of-two buffer size, for power-efficient
> operation.  What would you do?  Adding a new power_of_two bit flag
> into hw_params?  Likely not.

we've added the noIRQ mode in the past using flags, if now you are 
saying that flags is a bad idea then fine, but let's be consistent...

> 
> In such a case, I would expect some operation mode switch
> (e.g. power-saving vs low latency or whatever) instead of a very
> specific hw_parmas flag.  It might be a module option, via ALSA
> control, or something else.  But it's clearer for which purpose it's
> present, at least, and it can be implemented well without changing the
> existing API.

We have no way of predicting what the application will do so the module 
option is not possible.

Using an ALSA control is possible, but it's odd to me.

I really don't see what's so problematic about adding flags. I uses an 
existing capability of the API, it's consistent with the previous 
usages. There is no change in behavior for existing apps, only newer can 
benefit for better use of the hardware. There is no complicated decision 
making, you set the flags if you don't use IRQ or rewinds.
And it's not like we will have new flags every week, we've been talking 
about this SPIB capability since Skylake which is 3 years old already.

> 
> 
> thanks,
> 
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 

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

[Takashi Iwai]

On Wed, 28 Mar 2018 16:30:09 +0200,
Pierre-Louis Bossart wrote:
> 
> On 3/25/18 9:58 AM, Takashi Iwai wrote:
> > On Sun, 25 Mar 2018 12:46:43 +0200,
> > Sriram Periyasamy wrote:
> >>
> >> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
> >>> On Tue, 20 Mar 2018 17:01:06 +0100,
> >>> Sriram Periyasamy 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.
> >>>>
> >>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> >>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> >>>
> >>> Well, I'm still not convinced with this flag.
> >>>
> >>> First off, does it really need to be per PCM stream?  The introducing
> >>
> >> Flag per PCM stream helps where each stream in given system may have
> >> different requirement such as low power or low latency based on the
> >> use case. For example in case of low power stream, driver can perform
> >> required optimizations at hardware level based on the no_rewind flag.
> >
> > Yes, but does it really need to be PCM stream, i.e. per application?
> > Certainly the system will be using some sound backend (like PA).  In
> > which scenario can such behavior change -- some application uses a
> > different backend on a phone or a tablet?
> 
> This is intended for the case where the system server exposes a
> 'deep-buffer' PCM device for music playback in low-power mode and a
> separate one for system sounds or anything that requires
> interactivity.
> The need for rewinding is really for the case where the interactive
> system sounds are mixed with music, when you have separation between
> types of sounds and hardware/firmware mixing then the rewinds are
> unnecessary.

Yes, but why application must tell no-rewind flag if it wants to
save a bit of power?  IOW, how each application can know it needs to
set no-rewind flag *for saving power*?

Or, put in another way: you want to make all applications running in
lower power generically.  What would you do?  Adding no-rewind flag to
all calls?  It makes no sense if the application runs on non-Intel
chips, so can't be hard-coded.

> If there are multiple applications using different PCM devices each
> (which is a bit hypothetical to me) there is no way to know ahead of
> time when the modules are loaded if the application will perform
> rewinds due to its interactive nature or will just stream without ever
> invalidating the ring buffer. So yes it's per stream.

Fair enough, per stream is a requirement.

But still my argument below applies: what you really want to set
is to make the stream low-power.  It's not about to make the stream
non-rewindable.  And this makes me feel uneasy.


> >>> something to hw_parms implies that it varies per application.  But I
> >>> can't imagine that a system requires different behavior per stream
> >>> regarding such a thing.
> >>>
> >>> Second, the driver can implement a check in PCM ack callback to
> >>> prevent the rewind, too.  Then there is no need to touch the PCM
> >>> core.
> >>>
> >>
> >> As per the previous discussion at [1],
> >>
> >> [1]
> >> https://patchwork.kernel.org/patch/9795233/
> >>
> >> from Pierre,
> >>
> >> "The application (which is in most cases an audio server) *knows* if it
> >> requires rewinds or not. It's part of its design, with rewinds typically
> >> disabled if period interrupts are required. It's been that way for a
> >> number of years now. The use of rewinds is typically associated with the
> >> combination of a large buffer and no interrupts (having either of the
> >> two would not require rewinds).
> >>
> >> So the idea is that the application makes a statement that rewinds will
> >> not be used, and the low-level driver makes use of the information to
> >> enable whatever optimizations are available at the hardware level.
> >>
> >> Exposing more information to userspace would quickly lead to a confusing
> >> decision-making and would require more than just a flag."
> >
> > And, requiring *that* information is already confusing, IMO.
> > Think from the application writer POV: what is the relation between
> > the power-saving and no_rewind behavior in application level at all?
> > If you have no idea about the hardware details, they are totally
> > irrelevant.
> 
> I feel like disabling IRQs or disabling rewinds is the same level of
> information, you set the flags without necessary knowing all the power
> savings down to the mW level. But it provides an opportunity to save
> power with additional degrees of freedom for implementations.
 
Sure, I do understand this will bring the merit.  But the question is
the API design.

> An additional benefit of using the underlying SPIB register on Intel
> hardware is that the DMA hardware will not wrap-around, which can lead
> to better detection of real-time issues and a guarantee that stale
> data will not be played.

So, again, the purpose of no-rewind isn't the rewind thing itself.
It's set for obtaining other benefits.

> > Or, think like this way: imagine a hardware that requires a different
> > constraint, e.g. the power-of-two buffer size, for power-efficient
> > operation.  What would you do?  Adding a new power_of_two bit flag
> > into hw_params?  Likely not.
> 
> we've added the noIRQ mode in the past using flags, if now you are
> saying that flags is a bad idea then fine, but let's be consistent...

The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
mandated the period update per definition, and setting this flag
really switches to a different mode, hence it deserves for an API
extension.  And, the flag itself is self-explaining: the less IRQ is
less power.  But no-rewind is...?

> > In such a case, I would expect some operation mode switch
> > (e.g. power-saving vs low latency or whatever) instead of a very
> > specific hw_parmas flag.  It might be a module option, via ALSA
> > control, or something else.  But it's clearer for which purpose it's
> > present, at least, and it can be implemented well without changing the
> > existing API.
> 
> We have no way of predicting what the application will do so the
> module option is not possible.
> 
> Using an ALSA control is possible, but it's odd to me.
> 
> I really don't see what's so problematic about adding flags. I uses an
> existing capability of the API, it's consistent with the previous
> usages. There is no change in behavior for existing apps, only newer
> can benefit for better use of the hardware. There is no complicated
> decision making, you set the flags if you don't use IRQ or rewinds.
> And it's not like we will have new flags every week, we've been
> talking about this SPIB capability since Skylake which is 3 years old
> already.

Again, my concern is that you swapped between the purpose and the
method.  The no-irq isn't any purpose, per se.  It's just a
requirement some hardware casually applies for power saving.

The real need isn't about each detailed hardware-specific flag, but
rather some API to give a hint for the preferred operation mode.


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 3/28/18 10:20 AM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 16:30:09 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 3/25/18 9:58 AM, Takashi Iwai wrote:
>>> On Sun, 25 Mar 2018 12:46:43 +0200,
>>> Sriram Periyasamy wrote:
>>>>
>>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
>>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
>>>>> Sriram Periyasamy 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.
>>>>>>
>>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
>>>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>>>
>>>>> Well, I'm still not convinced with this flag.
>>>>>
>>>>> First off, does it really need to be per PCM stream?  The introducing
>>>>
>>>> Flag per PCM stream helps where each stream in given system may have
>>>> different requirement such as low power or low latency based on the
>>>> use case. For example in case of low power stream, driver can perform
>>>> required optimizations at hardware level based on the no_rewind flag.
>>>
>>> Yes, but does it really need to be PCM stream, i.e. per application?
>>> Certainly the system will be using some sound backend (like PA).  In
>>> which scenario can such behavior change -- some application uses a
>>> different backend on a phone or a tablet?
>>
>> This is intended for the case where the system server exposes a
>> 'deep-buffer' PCM device for music playback in low-power mode and a
>> separate one for system sounds or anything that requires
>> interactivity.
>> The need for rewinding is really for the case where the interactive
>> system sounds are mixed with music, when you have separation between
>> types of sounds and hardware/firmware mixing then the rewinds are
>> unnecessary.
> 
> Yes, but why application must tell no-rewind flag if it wants to
> save a bit of power?  IOW, how each application can know it needs to
> set no-rewind flag *for saving power*?
> 
> Or, put in another way: you want to make all applications running in
> lower power generically.  What would you do?  Adding no-rewind flag to
> all calls?  It makes no sense if the application runs on non-Intel
> chips, so can't be hard-coded.
> 
>> If there are multiple applications using different PCM devices each
>> (which is a bit hypothetical to me) there is no way to know ahead of
>> time when the modules are loaded if the application will perform
>> rewinds due to its interactive nature or will just stream without ever
>> invalidating the ring buffer. So yes it's per stream.
> 
> Fair enough, per stream is a requirement.
> 
> But still my argument below applies: what you really want to set
> is to make the stream low-power.  It's not about to make the stream
> non-rewindable.  And this makes me feel uneasy.
> 
> 
>>>>> something to hw_parms implies that it varies per application.  But I
>>>>> can't imagine that a system requires different behavior per stream
>>>>> regarding such a thing.
>>>>>
>>>>> Second, the driver can implement a check in PCM ack callback to
>>>>> prevent the rewind, too.  Then there is no need to touch the PCM
>>>>> core.
>>>>>
>>>>
>>>> As per the previous discussion at [1],
>>>>
>>>> [1]
>>>> https://patchwork.kernel.org/patch/9795233/
>>>>
>>>> from Pierre,
>>>>
>>>> "The application (which is in most cases an audio server) *knows* if it
>>>> requires rewinds or not. It's part of its design, with rewinds typically
>>>> disabled if period interrupts are required. It's been that way for a
>>>> number of years now. The use of rewinds is typically associated with the
>>>> combination of a large buffer and no interrupts (having either of the
>>>> two would not require rewinds).
>>>>
>>>> So the idea is that the application makes a statement that rewinds will
>>>> not be used, and the low-level driver makes use of the information to
>>>> enable whatever optimizations are available at the hardware level.
>>>>
>>>> Exposing more information to userspace would quickly lead to a confusing
>>>> decision-making and would require more than just a flag."
>>>
>>> And, requiring *that* information is already confusing, IMO.
>>> Think from the application writer POV: what is the relation between
>>> the power-saving and no_rewind behavior in application level at all?
>>> If you have no idea about the hardware details, they are totally
>>> irrelevant.
>>
>> I feel like disabling IRQs or disabling rewinds is the same level of
>> information, you set the flags without necessary knowing all the power
>> savings down to the mW level. But it provides an opportunity to save
>> power with additional degrees of freedom for implementations.
>   
> Sure, I do understand this will bring the merit.  But the question is
> the API design.
> 
>> An additional benefit of using the underlying SPIB register on Intel
>> hardware is that the DMA hardware will not wrap-around, which can lead
>> to better detection of real-time issues and a guarantee that stale
>> data will not be played.
> 
> So, again, the purpose of no-rewind isn't the rewind thing itself.
> It's set for obtaining other benefits.
> 
>>> Or, think like this way: imagine a hardware that requires a different
>>> constraint, e.g. the power-of-two buffer size, for power-efficient
>>> operation.  What would you do?  Adding a new power_of_two bit flag
>>> into hw_params?  Likely not.
>>
>> we've added the noIRQ mode in the past using flags, if now you are
>> saying that flags is a bad idea then fine, but let's be consistent...
> 
> The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
> mandated the period update per definition, and setting this flag
> really switches to a different mode, hence it deserves for an API
> extension.  And, the flag itself is self-explaining: the less IRQ is
> less power.  But no-rewind is...?
> 
>>> In such a case, I would expect some operation mode switch
>>> (e.g. power-saving vs low latency or whatever) instead of a very
>>> specific hw_parmas flag.  It might be a module option, via ALSA
>>> control, or something else.  But it's clearer for which purpose it's
>>> present, at least, and it can be implemented well without changing the
>>> existing API.
>>
>> We have no way of predicting what the application will do so the
>> module option is not possible.
>>
>> Using an ALSA control is possible, but it's odd to me.
>>
>> I really don't see what's so problematic about adding flags. I uses an
>> existing capability of the API, it's consistent with the previous
>> usages. There is no change in behavior for existing apps, only newer
>> can benefit for better use of the hardware. There is no complicated
>> decision making, you set the flags if you don't use IRQ or rewinds.
>> And it's not like we will have new flags every week, we've been
>> talking about this SPIB capability since Skylake which is 3 years old
>> already.
> 
> Again, my concern is that you swapped between the purpose and the
> method.  The no-irq isn't any purpose, per se.  It's just a
> requirement some hardware casually applies for power saving.
> 
> The real need isn't about each detailed hardware-specific flag, but
> rather some API to give a hint for the preferred operation mode.

let me try a different explanation (don't want to be pedantic but try to 
explain the line of thought).

There are two needs in terms of application/driver interaction.

The first need is to let the application know about hardware 
capabilities or restrictions. This is handled today with the .info 
fields. We have quite a few of them that provide information to 
userspace and let the application use the ALSA API in different ways 
(e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the 
example above, if a specific hardware could handle optimizations for 
powers of two, it could add a flag in the .info field and let the 
application make that decision.

The second need is to establish a contract between application and 
driver, set in stone and non modifiable dynamically while the stream is 
open. We are using hw_params for this, and the flags are one way to 
extend the API to new capabilities. the no-irq flag is one example of 
this contract which fundamentally changes the way the application is 
written. It's not limited to power savings but can also be used to 
reduce the latency as done by Android/AAudio, and it's not a 'casual' 
way of doing things but a fundamental design decision.

The proposal in this patchset is to restrict the use of rewinds which 
can have two known benefits
1. better DMA handling with opportunistic/bursty transfers
2. no wrap-around and handling of stale data.
This capability may be used for low-power and low-latency, in a similar 
way to the no-irq mode. Whether it makes sense for a system is not the 
debate here, we want to leave the decision making to system integrators. 
Since we use the hw_params flags in the past, we chose the same 
solution. We also did not add any flag in the .info field since the 
application doesn't really need to know what hardware will do or not. If 
it doesn't use rewinds, it just states it and lets the hardware enable 
new capabilities.

I am really struggling to see how the proposal is viewed as different 
from previous ones and where we confused 'purpose and method'. We used 
the same hooks for the same purposes.

I also don't think that either the no-irq and no-rewinds can be assigned 
to low-power or low-latency usages. I don't think we are in a position 
to make that call and I don't think we want to add a 'low-latency' or 
'low-power' flag to the ALSA API - this would be extremely confusing to 
applications.

If your position is that we can no longer report or enable new 
capabilities with the .info and hw_params fields, then what are the options?
a) do nothing and leave ALSA handle hardware designed prior to 2013
b) define a new API and transition both the info and hw_params to 
something else

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

[Takashi Iwai]

On Wed, 28 Mar 2018 19:58:54 +0200,
Pierre-Louis Bossart wrote:
> 
> On 3/28/18 10:20 AM, Takashi Iwai wrote:
> > On Wed, 28 Mar 2018 16:30:09 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >> On 3/25/18 9:58 AM, Takashi Iwai wrote:
> >>> On Sun, 25 Mar 2018 12:46:43 +0200,
> >>> Sriram Periyasamy wrote:
> >>>>
> >>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
> >>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
> >>>>> Sriram Periyasamy 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.
> >>>>>>
> >>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> >>>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> >>>>>
> >>>>> Well, I'm still not convinced with this flag.
> >>>>>
> >>>>> First off, does it really need to be per PCM stream?  The introducing
> >>>>
> >>>> Flag per PCM stream helps where each stream in given system may have
> >>>> different requirement such as low power or low latency based on the
> >>>> use case. For example in case of low power stream, driver can perform
> >>>> required optimizations at hardware level based on the no_rewind flag.
> >>>
> >>> Yes, but does it really need to be PCM stream, i.e. per application?
> >>> Certainly the system will be using some sound backend (like PA).  In
> >>> which scenario can such behavior change -- some application uses a
> >>> different backend on a phone or a tablet?
> >>
> >> This is intended for the case where the system server exposes a
> >> 'deep-buffer' PCM device for music playback in low-power mode and a
> >> separate one for system sounds or anything that requires
> >> interactivity.
> >> The need for rewinding is really for the case where the interactive
> >> system sounds are mixed with music, when you have separation between
> >> types of sounds and hardware/firmware mixing then the rewinds are
> >> unnecessary.
> >
> > Yes, but why application must tell no-rewind flag if it wants to
> > save a bit of power?  IOW, how each application can know it needs to
> > set no-rewind flag *for saving power*?
> >
> > Or, put in another way: you want to make all applications running in
> > lower power generically.  What would you do?  Adding no-rewind flag to
> > all calls?  It makes no sense if the application runs on non-Intel
> > chips, so can't be hard-coded.
> >
> >> If there are multiple applications using different PCM devices each
> >> (which is a bit hypothetical to me) there is no way to know ahead of
> >> time when the modules are loaded if the application will perform
> >> rewinds due to its interactive nature or will just stream without ever
> >> invalidating the ring buffer. So yes it's per stream.
> >
> > Fair enough, per stream is a requirement.
> >
> > But still my argument below applies: what you really want to set
> > is to make the stream low-power.  It's not about to make the stream
> > non-rewindable.  And this makes me feel uneasy.
> >
> >
> >>>>> something to hw_parms implies that it varies per application.  But I
> >>>>> can't imagine that a system requires different behavior per stream
> >>>>> regarding such a thing.
> >>>>>
> >>>>> Second, the driver can implement a check in PCM ack callback to
> >>>>> prevent the rewind, too.  Then there is no need to touch the PCM
> >>>>> core.
> >>>>>
> >>>>
> >>>> As per the previous discussion at [1],
> >>>>
> >>>> [1]
> >>>> https://patchwork.kernel.org/patch/9795233/
> >>>>
> >>>> from Pierre,
> >>>>
> >>>> "The application (which is in most cases an audio server) *knows* if it
> >>>> requires rewinds or not. It's part of its design, with rewinds typically
> >>>> disabled if period interrupts are required. It's been that way for a
> >>>> number of years now. The use of rewinds is typically associated with the
> >>>> combination of a large buffer and no interrupts (having either of the
> >>>> two would not require rewinds).
> >>>>
> >>>> So the idea is that the application makes a statement that rewinds will
> >>>> not be used, and the low-level driver makes use of the information to
> >>>> enable whatever optimizations are available at the hardware level.
> >>>>
> >>>> Exposing more information to userspace would quickly lead to a confusing
> >>>> decision-making and would require more than just a flag."
> >>>
> >>> And, requiring *that* information is already confusing, IMO.
> >>> Think from the application writer POV: what is the relation between
> >>> the power-saving and no_rewind behavior in application level at all?
> >>> If you have no idea about the hardware details, they are totally
> >>> irrelevant.
> >>
> >> I feel like disabling IRQs or disabling rewinds is the same level of
> >> information, you set the flags without necessary knowing all the power
> >> savings down to the mW level. But it provides an opportunity to save
> >> power with additional degrees of freedom for implementations.
> >   Sure, I do understand this will bring the merit.  But the question
> > is
> > the API design.
> >
> >> An additional benefit of using the underlying SPIB register on Intel
> >> hardware is that the DMA hardware will not wrap-around, which can lead
> >> to better detection of real-time issues and a guarantee that stale
> >> data will not be played.
> >
> > So, again, the purpose of no-rewind isn't the rewind thing itself.
> > It's set for obtaining other benefits.
> >
> >>> Or, think like this way: imagine a hardware that requires a different
> >>> constraint, e.g. the power-of-two buffer size, for power-efficient
> >>> operation.  What would you do?  Adding a new power_of_two bit flag
> >>> into hw_params?  Likely not.
> >>
> >> we've added the noIRQ mode in the past using flags, if now you are
> >> saying that flags is a bad idea then fine, but let's be consistent...
> >
> > The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
> > mandated the period update per definition, and setting this flag
> > really switches to a different mode, hence it deserves for an API
> > extension.  And, the flag itself is self-explaining: the less IRQ is
> > less power.  But no-rewind is...?
> >
> >>> In such a case, I would expect some operation mode switch
> >>> (e.g. power-saving vs low latency or whatever) instead of a very
> >>> specific hw_parmas flag.  It might be a module option, via ALSA
> >>> control, or something else.  But it's clearer for which purpose it's
> >>> present, at least, and it can be implemented well without changing the
> >>> existing API.
> >>
> >> We have no way of predicting what the application will do so the
> >> module option is not possible.
> >>
> >> Using an ALSA control is possible, but it's odd to me.
> >>
> >> I really don't see what's so problematic about adding flags. I uses an
> >> existing capability of the API, it's consistent with the previous
> >> usages. There is no change in behavior for existing apps, only newer
> >> can benefit for better use of the hardware. There is no complicated
> >> decision making, you set the flags if you don't use IRQ or rewinds.
> >> And it's not like we will have new flags every week, we've been
> >> talking about this SPIB capability since Skylake which is 3 years old
> >> already.
> >
> > Again, my concern is that you swapped between the purpose and the
> > method.  The no-irq isn't any purpose, per se.  It's just a
> > requirement some hardware casually applies for power saving.
> >
> > The real need isn't about each detailed hardware-specific flag, but
> > rather some API to give a hint for the preferred operation mode.
> 
> let me try a different explanation (don't want to be pedantic but try
> to explain the line of thought).
> 
> There are two needs in terms of application/driver interaction.
> 
> The first need is to let the application know about hardware
> capabilities or restrictions. This is handled today with the .info
> fields. We have quite a few of them that provide information to
> userspace and let the application use the ALSA API in different ways
> (e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the
> example above, if a specific hardware could handle optimizations for
> powers of two, it could add a flag in the .info field and let the
> application make that decision.
> 
> The second need is to establish a contract between application and
> driver, set in stone and non modifiable dynamically while the stream
> is open. We are using hw_params for this, and the flags are one way to
> extend the API to new capabilities. the no-irq flag is one example of
> this contract which fundamentally changes the way the application is
> written. It's not limited to power savings but can also be used to
> reduce the latency as done by Android/AAudio, and it's not a 'casual'
> way of doing things but a fundamental design decision.
> 
> The proposal in this patchset is to restrict the use of rewinds which
> can have two known benefits
> 1. better DMA handling with opportunistic/bursty transfers
> 2. no wrap-around and handling of stale data.
> This capability may be used for low-power and low-latency, in a
> similar way to the no-irq mode. Whether it makes sense for a system is
> not the debate here, we want to leave the decision making to system
> integrators. Since we use the hw_params flags in the past, we chose
> the same solution. We also did not add any flag in the .info field
> since the application doesn't really need to know what hardware will
> do or not. If it doesn't use rewinds, it just states it and lets the
> hardware enable new capabilities.
>
> I am really struggling to see how the proposal is viewed as different
> from previous ones and where we confused 'purpose and method'. We used
> the same hooks for the same purposes.

I'm fine to change something in the driver internal, and we've
prepared it with the extension of ack callback, so far.  But, again,
if it comes to the question about API, I have to be stubborn. 

The no-IRQ mode was designed for the clear behavior change, thus it's
mandatory to tie with the hw_params.  However, the no-rewind is
different.  It's simple to restrict the rewind from the driver side as
is without changing the current API.  So why it's in hw_params and why
each application must set explicitly?

Yes, it's opt-out.  But it means that it's useless unless application
really knows the very h/w specific feature; i.e. it has to detect the
running system, determine whether this flag is required or not, set it
and stop using rewind.  Doing this for each application?

And, if you think "hey it's overreaction, it's nothing but an opt-out
feature for only specific machines and specific applications", then
why do we have to implement it in the generic hw_parmams API level,
instead of some driver-specific thing like kcontrol or whatever?

(Again, no-irq is a different situation; the no-IRQ is a special mode
 for timer-based scheduling that makes application to skip the
 mandatory period programming model.  But for no-rewind, there is
 nothing required in the application side, since the rewind operation
 is optional from the very beginning.)


> I also don't think that either the no-irq and no-rewinds can be
> assigned to low-power or low-latency usages. I don't think we are in a
> position to make that call and I don't think we want to add a
> 'low-latency' or 'low-power' flag to the ALSA API - this would be
> extremely confusing to applications.
>
> If your position is that we can no longer report or enable new
> capabilities with the .info and hw_params fields, then what are the
> options?
> a) do nothing and leave ALSA handle hardware designed prior to 2013
> b) define a new API and transition both the info and hw_params to
> something else

Extending the hw_params flag itself is fine, but iff it really makes
sense as a generic API usage.  I'm not fully convinced yet that
no_rewind is mandatory, so far.  That's all.


And, I'm really interested in how much impact we'd have if we disable
the rewind completely for compensation of SPIB and other DSP stuff.
AFAIK, it's only PA who really uses the rewind feature.  And, PA uses
it for saving power (more exactly, for low-latency with power-saving
operation using timer scheduling).  Now, practically seen, which mode
should PA use?  Would disabling rewind cost too much even with other
benefits?


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 3/28/18 1:35 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 19:58:54 +0200,
> Pierre-Louis Bossart wrote:
>>
>> On 3/28/18 10:20 AM, Takashi Iwai wrote:
>>> On Wed, 28 Mar 2018 16:30:09 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>> On 3/25/18 9:58 AM, Takashi Iwai wrote:
>>>>> On Sun, 25 Mar 2018 12:46:43 +0200,
>>>>> Sriram Periyasamy wrote:
>>>>>>
>>>>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
>>>>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
>>>>>>> Sriram Periyasamy 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.
>>>>>>>>
>>>>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
>>>>>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>>>>>
>>>>>>> Well, I'm still not convinced with this flag.
>>>>>>>
>>>>>>> First off, does it really need to be per PCM stream?  The introducing
>>>>>>
>>>>>> Flag per PCM stream helps where each stream in given system may have
>>>>>> different requirement such as low power or low latency based on the
>>>>>> use case. For example in case of low power stream, driver can perform
>>>>>> required optimizations at hardware level based on the no_rewind flag.
>>>>>
>>>>> Yes, but does it really need to be PCM stream, i.e. per application?
>>>>> Certainly the system will be using some sound backend (like PA).  In
>>>>> which scenario can such behavior change -- some application uses a
>>>>> different backend on a phone or a tablet?
>>>>
>>>> This is intended for the case where the system server exposes a
>>>> 'deep-buffer' PCM device for music playback in low-power mode and a
>>>> separate one for system sounds or anything that requires
>>>> interactivity.
>>>> The need for rewinding is really for the case where the interactive
>>>> system sounds are mixed with music, when you have separation between
>>>> types of sounds and hardware/firmware mixing then the rewinds are
>>>> unnecessary.
>>>
>>> Yes, but why application must tell no-rewind flag if it wants to
>>> save a bit of power?  IOW, how each application can know it needs to
>>> set no-rewind flag *for saving power*?
>>>
>>> Or, put in another way: you want to make all applications running in
>>> lower power generically.  What would you do?  Adding no-rewind flag to
>>> all calls?  It makes no sense if the application runs on non-Intel
>>> chips, so can't be hard-coded.
>>>
>>>> If there are multiple applications using different PCM devices each
>>>> (which is a bit hypothetical to me) there is no way to know ahead of
>>>> time when the modules are loaded if the application will perform
>>>> rewinds due to its interactive nature or will just stream without ever
>>>> invalidating the ring buffer. So yes it's per stream.
>>>
>>> Fair enough, per stream is a requirement.
>>>
>>> But still my argument below applies: what you really want to set
>>> is to make the stream low-power.  It's not about to make the stream
>>> non-rewindable.  And this makes me feel uneasy.
>>>
>>>
>>>>>>> something to hw_parms implies that it varies per application.  But I
>>>>>>> can't imagine that a system requires different behavior per stream
>>>>>>> regarding such a thing.
>>>>>>>
>>>>>>> Second, the driver can implement a check in PCM ack callback to
>>>>>>> prevent the rewind, too.  Then there is no need to touch the PCM
>>>>>>> core.
>>>>>>>
>>>>>>
>>>>>> As per the previous discussion at [1],
>>>>>>
>>>>>> [1]
>>>>>> https://patchwork.kernel.org/patch/9795233/
>>>>>>
>>>>>> from Pierre,
>>>>>>
>>>>>> "The application (which is in most cases an audio server) *knows* if it
>>>>>> requires rewinds or not. It's part of its design, with rewinds typically
>>>>>> disabled if period interrupts are required. It's been that way for a
>>>>>> number of years now. The use of rewinds is typically associated with the
>>>>>> combination of a large buffer and no interrupts (having either of the
>>>>>> two would not require rewinds).
>>>>>>
>>>>>> So the idea is that the application makes a statement that rewinds will
>>>>>> not be used, and the low-level driver makes use of the information to
>>>>>> enable whatever optimizations are available at the hardware level.
>>>>>>
>>>>>> Exposing more information to userspace would quickly lead to a confusing
>>>>>> decision-making and would require more than just a flag."
>>>>>
>>>>> And, requiring *that* information is already confusing, IMO.
>>>>> Think from the application writer POV: what is the relation between
>>>>> the power-saving and no_rewind behavior in application level at all?
>>>>> If you have no idea about the hardware details, they are totally
>>>>> irrelevant.
>>>>
>>>> I feel like disabling IRQs or disabling rewinds is the same level of
>>>> information, you set the flags without necessary knowing all the power
>>>> savings down to the mW level. But it provides an opportunity to save
>>>> power with additional degrees of freedom for implementations.
>>>    Sure, I do understand this will bring the merit.  But the question
>>> is
>>> the API design.
>>>
>>>> An additional benefit of using the underlying SPIB register on Intel
>>>> hardware is that the DMA hardware will not wrap-around, which can lead
>>>> to better detection of real-time issues and a guarantee that stale
>>>> data will not be played.
>>>
>>> So, again, the purpose of no-rewind isn't the rewind thing itself.
>>> It's set for obtaining other benefits.
>>>
>>>>> Or, think like this way: imagine a hardware that requires a different
>>>>> constraint, e.g. the power-of-two buffer size, for power-efficient
>>>>> operation.  What would you do?  Adding a new power_of_two bit flag
>>>>> into hw_params?  Likely not.
>>>>
>>>> we've added the noIRQ mode in the past using flags, if now you are
>>>> saying that flags is a bad idea then fine, but let's be consistent...
>>>
>>> The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
>>> mandated the period update per definition, and setting this flag
>>> really switches to a different mode, hence it deserves for an API
>>> extension.  And, the flag itself is self-explaining: the less IRQ is
>>> less power.  But no-rewind is...?
>>>
>>>>> In such a case, I would expect some operation mode switch
>>>>> (e.g. power-saving vs low latency or whatever) instead of a very
>>>>> specific hw_parmas flag.  It might be a module option, via ALSA
>>>>> control, or something else.  But it's clearer for which purpose it's
>>>>> present, at least, and it can be implemented well without changing the
>>>>> existing API.
>>>>
>>>> We have no way of predicting what the application will do so the
>>>> module option is not possible.
>>>>
>>>> Using an ALSA control is possible, but it's odd to me.
>>>>
>>>> I really don't see what's so problematic about adding flags. I uses an
>>>> existing capability of the API, it's consistent with the previous
>>>> usages. There is no change in behavior for existing apps, only newer
>>>> can benefit for better use of the hardware. There is no complicated
>>>> decision making, you set the flags if you don't use IRQ or rewinds.
>>>> And it's not like we will have new flags every week, we've been
>>>> talking about this SPIB capability since Skylake which is 3 years old
>>>> already.
>>>
>>> Again, my concern is that you swapped between the purpose and the
>>> method.  The no-irq isn't any purpose, per se.  It's just a
>>> requirement some hardware casually applies for power saving.
>>>
>>> The real need isn't about each detailed hardware-specific flag, but
>>> rather some API to give a hint for the preferred operation mode.
>>
>> let me try a different explanation (don't want to be pedantic but try
>> to explain the line of thought).
>>
>> There are two needs in terms of application/driver interaction.
>>
>> The first need is to let the application know about hardware
>> capabilities or restrictions. This is handled today with the .info
>> fields. We have quite a few of them that provide information to
>> userspace and let the application use the ALSA API in different ways
>> (e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the
>> example above, if a specific hardware could handle optimizations for
>> powers of two, it could add a flag in the .info field and let the
>> application make that decision.
>>
>> The second need is to establish a contract between application and
>> driver, set in stone and non modifiable dynamically while the stream
>> is open. We are using hw_params for this, and the flags are one way to
>> extend the API to new capabilities. the no-irq flag is one example of
>> this contract which fundamentally changes the way the application is
>> written. It's not limited to power savings but can also be used to
>> reduce the latency as done by Android/AAudio, and it's not a 'casual'
>> way of doing things but a fundamental design decision.
>>
>> The proposal in this patchset is to restrict the use of rewinds which
>> can have two known benefits
>> 1. better DMA handling with opportunistic/bursty transfers
>> 2. no wrap-around and handling of stale data.
>> This capability may be used for low-power and low-latency, in a
>> similar way to the no-irq mode. Whether it makes sense for a system is
>> not the debate here, we want to leave the decision making to system
>> integrators. Since we use the hw_params flags in the past, we chose
>> the same solution. We also did not add any flag in the .info field
>> since the application doesn't really need to know what hardware will
>> do or not. If it doesn't use rewinds, it just states it and lets the
>> hardware enable new capabilities.
>>
>> I am really struggling to see how the proposal is viewed as different
>> from previous ones and where we confused 'purpose and method'. We used
>> the same hooks for the same purposes.
> 
> I'm fine to change something in the driver internal, and we've
> prepared it with the extension of ack callback, so far.  But, again,
> if it comes to the question about API, I have to be stubborn.
> 
> The no-IRQ mode was designed for the clear behavior change, thus it's
> mandatory to tie with the hw_params.  However, the no-rewind is
> different.  It's simple to restrict the rewind from the driver side as
> is without changing the current API.  So why it's in hw_params and why
> each application must set explicitly?

I don't see how the driver could make that decision on its own, sorry.
If we restrict the rewinds and then the application calls rewind, we 
would have a run-time issue and stop playback/capture.

> 
> Yes, it's opt-out.  But it means that it's useless unless application
> really knows the very h/w specific feature; i.e. it has to detect the
> running system, determine whether this flag is required or not, set it
> and stop using rewind.  Doing this for each application?
> 
> And, if you think "hey it's overreaction, it's nothing but an opt-out
> feature for only specific machines and specific applications", then
> why do we have to implement it in the generic hw_parmams API level,
> instead of some driver-specific thing like kcontrol or whatever?

It's not just a driver-specific thing, we have to deal with errors in 
the core if rewinds are called. So no matter what signalling mechanism 
is used by the application this capability has to be known by the core.

> 
> (Again, no-irq is a different situation; the no-IRQ is a special mode
>   for timer-based scheduling that makes application to skip the
>   mandatory period programming model.  But for no-rewind, there is
>   nothing required in the application side, since the rewind operation
>   is optional from the very beginning.)

but the driver doesn't know if rewinds will ever be used or not...

> 
> 
>> I also don't think that either the no-irq and no-rewinds can be
>> assigned to low-power or low-latency usages. I don't think we are in a
>> position to make that call and I don't think we want to add a
>> 'low-latency' or 'low-power' flag to the ALSA API - this would be
>> extremely confusing to applications.
>>
>> If your position is that we can no longer report or enable new
>> capabilities with the .info and hw_params fields, then what are the
>> options?
>> a) do nothing and leave ALSA handle hardware designed prior to 2013
>> b) define a new API and transition both the info and hw_params to
>> something else
> 
> Extending the hw_params flag itself is fine, but iff it really makes
> sense as a generic API usage.  I'm not fully convinced yet that
> no_rewind is mandatory, so far.  That's all.
> 
> 
> And, I'm really interested in how much impact we'd have if we disable
> the rewind completely for compensation of SPIB and other DSP stuff.
> AFAIK, it's only PA who really uses the rewind feature.  And, PA uses
> it for saving power (more exactly, for low-latency with power-saving
> operation using timer scheduling).  Now, practically seen, which mode
> should PA use?  Would disabling rewind cost too much even with other
> benefits?

The power savings implemented by PulseAudio with the timer-based 
scheduling are based on 10-year old designs where the core was the 
dominant issue.

Now the power savings have moved to a different level of optimization. 
With more buffering in the audio cluster, it's the path to memory and 
the opportunities to let the audio cluster work in a stand-alone matter 
than really save power. SPIB and friends really address how the memory 
is accessed and if transfers can be scheduled e.g. when the path to 
memory is already active due to some other event.


PulseAudio cannot benefit from proposal with the current implementation. 
It would only benefit when there are two streams, one for deep-buffer 
and one for system/low-latency streams, which is where the transfers can 
be optimized.

Chrome can be benefit immediately since rewinds are never used.
Jack can benefit immediately since rewinds are never used
Android HALs can benefit immediately since rewinds are never used.

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

[Takashi Iwai]

On Wed, 28 Mar 2018 21:50:27 +0200,
Pierre-Louis Bossart wrote:
> 
> On 3/28/18 1:35 PM, Takashi Iwai wrote:
> > On Wed, 28 Mar 2018 19:58:54 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >> On 3/28/18 10:20 AM, Takashi Iwai wrote:
> >>> On Wed, 28 Mar 2018 16:30:09 +0200,
> >>> Pierre-Louis Bossart wrote:
> >>>>
> >>>> On 3/25/18 9:58 AM, Takashi Iwai wrote:
> >>>>> On Sun, 25 Mar 2018 12:46:43 +0200,
> >>>>> Sriram Periyasamy wrote:
> >>>>>>
> >>>>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
> >>>>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
> >>>>>>> Sriram Periyasamy 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.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>>>>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> >>>>>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >>>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
> >>>>>>>
> >>>>>>> Well, I'm still not convinced with this flag.
> >>>>>>>
> >>>>>>> First off, does it really need to be per PCM stream?  The introducing
> >>>>>>
> >>>>>> Flag per PCM stream helps where each stream in given system may have
> >>>>>> different requirement such as low power or low latency based on the
> >>>>>> use case. For example in case of low power stream, driver can perform
> >>>>>> required optimizations at hardware level based on the no_rewind flag.
> >>>>>
> >>>>> Yes, but does it really need to be PCM stream, i.e. per application?
> >>>>> Certainly the system will be using some sound backend (like PA).  In
> >>>>> which scenario can such behavior change -- some application uses a
> >>>>> different backend on a phone or a tablet?
> >>>>
> >>>> This is intended for the case where the system server exposes a
> >>>> 'deep-buffer' PCM device for music playback in low-power mode and a
> >>>> separate one for system sounds or anything that requires
> >>>> interactivity.
> >>>> The need for rewinding is really for the case where the interactive
> >>>> system sounds are mixed with music, when you have separation between
> >>>> types of sounds and hardware/firmware mixing then the rewinds are
> >>>> unnecessary.
> >>>
> >>> Yes, but why application must tell no-rewind flag if it wants to
> >>> save a bit of power?  IOW, how each application can know it needs to
> >>> set no-rewind flag *for saving power*?
> >>>
> >>> Or, put in another way: you want to make all applications running in
> >>> lower power generically.  What would you do?  Adding no-rewind flag to
> >>> all calls?  It makes no sense if the application runs on non-Intel
> >>> chips, so can't be hard-coded.
> >>>
> >>>> If there are multiple applications using different PCM devices each
> >>>> (which is a bit hypothetical to me) there is no way to know ahead of
> >>>> time when the modules are loaded if the application will perform
> >>>> rewinds due to its interactive nature or will just stream without ever
> >>>> invalidating the ring buffer. So yes it's per stream.
> >>>
> >>> Fair enough, per stream is a requirement.
> >>>
> >>> But still my argument below applies: what you really want to set
> >>> is to make the stream low-power.  It's not about to make the stream
> >>> non-rewindable.  And this makes me feel uneasy.
> >>>
> >>>
> >>>>>>> something to hw_parms implies that it varies per application.  But I
> >>>>>>> can't imagine that a system requires different behavior per stream
> >>>>>>> regarding such a thing.
> >>>>>>>
> >>>>>>> Second, the driver can implement a check in PCM ack callback to
> >>>>>>> prevent the rewind, too.  Then there is no need to touch the PCM
> >>>>>>> core.
> >>>>>>>
> >>>>>>
> >>>>>> As per the previous discussion at [1],
> >>>>>>
> >>>>>> [1]
> >>>>>> https://patchwork.kernel.org/patch/9795233/
> >>>>>>
> >>>>>> from Pierre,
> >>>>>>
> >>>>>> "The application (which is in most cases an audio server) *knows* if it
> >>>>>> requires rewinds or not. It's part of its design, with rewinds typically
> >>>>>> disabled if period interrupts are required. It's been that way for a
> >>>>>> number of years now. The use of rewinds is typically associated with the
> >>>>>> combination of a large buffer and no interrupts (having either of the
> >>>>>> two would not require rewinds).
> >>>>>>
> >>>>>> So the idea is that the application makes a statement that rewinds will
> >>>>>> not be used, and the low-level driver makes use of the information to
> >>>>>> enable whatever optimizations are available at the hardware level.
> >>>>>>
> >>>>>> Exposing more information to userspace would quickly lead to a confusing
> >>>>>> decision-making and would require more than just a flag."
> >>>>>
> >>>>> And, requiring *that* information is already confusing, IMO.
> >>>>> Think from the application writer POV: what is the relation between
> >>>>> the power-saving and no_rewind behavior in application level at all?
> >>>>> If you have no idea about the hardware details, they are totally
> >>>>> irrelevant.
> >>>>
> >>>> I feel like disabling IRQs or disabling rewinds is the same level of
> >>>> information, you set the flags without necessary knowing all the power
> >>>> savings down to the mW level. But it provides an opportunity to save
> >>>> power with additional degrees of freedom for implementations.
> >>>    Sure, I do understand this will bring the merit.  But the question
> >>> is
> >>> the API design.
> >>>
> >>>> An additional benefit of using the underlying SPIB register on Intel
> >>>> hardware is that the DMA hardware will not wrap-around, which can lead
> >>>> to better detection of real-time issues and a guarantee that stale
> >>>> data will not be played.
> >>>
> >>> So, again, the purpose of no-rewind isn't the rewind thing itself.
> >>> It's set for obtaining other benefits.
> >>>
> >>>>> Or, think like this way: imagine a hardware that requires a different
> >>>>> constraint, e.g. the power-of-two buffer size, for power-efficient
> >>>>> operation.  What would you do?  Adding a new power_of_two bit flag
> >>>>> into hw_params?  Likely not.
> >>>>
> >>>> we've added the noIRQ mode in the past using flags, if now you are
> >>>> saying that flags is a bad idea then fine, but let's be consistent...
> >>>
> >>> The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
> >>> mandated the period update per definition, and setting this flag
> >>> really switches to a different mode, hence it deserves for an API
> >>> extension.  And, the flag itself is self-explaining: the less IRQ is
> >>> less power.  But no-rewind is...?
> >>>
> >>>>> In such a case, I would expect some operation mode switch
> >>>>> (e.g. power-saving vs low latency or whatever) instead of a very
> >>>>> specific hw_parmas flag.  It might be a module option, via ALSA
> >>>>> control, or something else.  But it's clearer for which purpose it's
> >>>>> present, at least, and it can be implemented well without changing the
> >>>>> existing API.
> >>>>
> >>>> We have no way of predicting what the application will do so the
> >>>> module option is not possible.
> >>>>
> >>>> Using an ALSA control is possible, but it's odd to me.
> >>>>
> >>>> I really don't see what's so problematic about adding flags. I uses an
> >>>> existing capability of the API, it's consistent with the previous
> >>>> usages. There is no change in behavior for existing apps, only newer
> >>>> can benefit for better use of the hardware. There is no complicated
> >>>> decision making, you set the flags if you don't use IRQ or rewinds.
> >>>> And it's not like we will have new flags every week, we've been
> >>>> talking about this SPIB capability since Skylake which is 3 years old
> >>>> already.
> >>>
> >>> Again, my concern is that you swapped between the purpose and the
> >>> method.  The no-irq isn't any purpose, per se.  It's just a
> >>> requirement some hardware casually applies for power saving.
> >>>
> >>> The real need isn't about each detailed hardware-specific flag, but
> >>> rather some API to give a hint for the preferred operation mode.
> >>
> >> let me try a different explanation (don't want to be pedantic but try
> >> to explain the line of thought).
> >>
> >> There are two needs in terms of application/driver interaction.
> >>
> >> The first need is to let the application know about hardware
> >> capabilities or restrictions. This is handled today with the .info
> >> fields. We have quite a few of them that provide information to
> >> userspace and let the application use the ALSA API in different ways
> >> (e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the
> >> example above, if a specific hardware could handle optimizations for
> >> powers of two, it could add a flag in the .info field and let the
> >> application make that decision.
> >>
> >> The second need is to establish a contract between application and
> >> driver, set in stone and non modifiable dynamically while the stream
> >> is open. We are using hw_params for this, and the flags are one way to
> >> extend the API to new capabilities. the no-irq flag is one example of
> >> this contract which fundamentally changes the way the application is
> >> written. It's not limited to power savings but can also be used to
> >> reduce the latency as done by Android/AAudio, and it's not a 'casual'
> >> way of doing things but a fundamental design decision.
> >>
> >> The proposal in this patchset is to restrict the use of rewinds which
> >> can have two known benefits
> >> 1. better DMA handling with opportunistic/bursty transfers
> >> 2. no wrap-around and handling of stale data.
> >> This capability may be used for low-power and low-latency, in a
> >> similar way to the no-irq mode. Whether it makes sense for a system is
> >> not the debate here, we want to leave the decision making to system
> >> integrators. Since we use the hw_params flags in the past, we chose
> >> the same solution. We also did not add any flag in the .info field
> >> since the application doesn't really need to know what hardware will
> >> do or not. If it doesn't use rewinds, it just states it and lets the
> >> hardware enable new capabilities.
> >>
> >> I am really struggling to see how the proposal is viewed as different
> >> from previous ones and where we confused 'purpose and method'. We used
> >> the same hooks for the same purposes.
> >
> > I'm fine to change something in the driver internal, and we've
> > prepared it with the extension of ack callback, so far.  But, again,
> > if it comes to the question about API, I have to be stubborn.
> >
> > The no-IRQ mode was designed for the clear behavior change, thus it's
> > mandatory to tie with the hw_params.  However, the no-rewind is
> > different.  It's simple to restrict the rewind from the driver side as
> > is without changing the current API.  So why it's in hw_params and why
> > each application must set explicitly?
> 
> I don't see how the driver could make that decision on its own, sorry.

The decision should be done by user, of course.  But the question is
how it's told to the driver.  And I hesitate to add no-rewind flag to 
the generic API just for an Intel device-specific quirk.

> If we restrict the rewinds and then the application calls rewind, we
> would have a run-time issue and stop playback/capture.

Yes, the driver needs to know.  It's no doubt there.

> > Yes, it's opt-out.  But it means that it's useless unless application
> > really knows the very h/w specific feature; i.e. it has to detect the
> > running system, determine whether this flag is required or not, set it
> > and stop using rewind.  Doing this for each application?
> >
> > And, if you think "hey it's overreaction, it's nothing but an opt-out
> > feature for only specific machines and specific applications", then
> > why do we have to implement it in the generic hw_parmams API level,
> > instead of some driver-specific thing like kcontrol or whatever?
> 
> It's not just a driver-specific thing, we have to deal with errors in
> the core if rewinds are called. So no matter what signalling mechanism
> is used by the application this capability has to be known by the
> core.

The core already handles it by calling the ack callback.
The driver can simply return an error from there for the unexpected /
unwanted rewind calls.  It's only the matter how to tell to the
driver.

> > (Again, no-irq is a different situation; the no-IRQ is a special mode
> >   for timer-based scheduling that makes application to skip the
> >   mandatory period programming model.  But for no-rewind, there is
> >   nothing required in the application side, since the rewind operation
> >   is optional from the very beginning.)
> 
> but the driver doesn't know if rewinds will ever be used or not...
> 
> >
> >
> >> I also don't think that either the no-irq and no-rewinds can be
> >> assigned to low-power or low-latency usages. I don't think we are in a
> >> position to make that call and I don't think we want to add a
> >> 'low-latency' or 'low-power' flag to the ALSA API - this would be
> >> extremely confusing to applications.
> >>
> >> If your position is that we can no longer report or enable new
> >> capabilities with the .info and hw_params fields, then what are the
> >> options?
> >> a) do nothing and leave ALSA handle hardware designed prior to 2013
> >> b) define a new API and transition both the info and hw_params to
> >> something else
> >
> > Extending the hw_params flag itself is fine, but iff it really makes
> > sense as a generic API usage.  I'm not fully convinced yet that
> > no_rewind is mandatory, so far.  That's all.
> >
> >
> > And, I'm really interested in how much impact we'd have if we disable
> > the rewind completely for compensation of SPIB and other DSP stuff.
> > AFAIK, it's only PA who really uses the rewind feature.  And, PA uses
> > it for saving power (more exactly, for low-latency with power-saving
> > operation using timer scheduling).  Now, practically seen, which mode
> > should PA use?  Would disabling rewind cost too much even with other
> > benefits?
> 
> The power savings implemented by PulseAudio with the timer-based
> scheduling are based on 10-year old designs where the core was the
> dominant issue.
> 
> Now the power savings have moved to a different level of
> optimization. With more buffering in the audio cluster, it's the path
> to memory and the opportunities to let the audio cluster work in a
> stand-alone matter than really save power. SPIB and friends really
> address how the memory is accessed and if transfers can be scheduled
> e.g. when the path to memory is already active due to some other
> event.
> 
> 
> PulseAudio cannot benefit from proposal with the current
> implementation. It would only benefit when there are two streams, one
> for deep-buffer and one for system/low-latency streams, which is where
> the transfers can be optimized.
> 
> Chrome can be benefit immediately since rewinds are never used.
> Jack can benefit immediately since rewinds are never used
> Android HALs can benefit immediately since rewinds are never used.

... if you patch all these.  That's not immediate.
OTOH, if you do introduce a single switch, it's immediately
effective, even with the old user-space without compilation.

And, now back to the question: in which situation you'll have to mix
both no-rewind and rewind operations on a single machine?  In theory,
yes, we may keep rewind working, but from the practical POV, it is
only PA who uses the rewind feature.  And, even if we disable rewind,
PA will still work as is.  (I know it because I broke it previously :)

So, what's wrong with introducing a global switch instead of changing
the existing API and requiring the code change in each application and
rebuild?


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 03/28/2018 04:09 PM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 21:50:27 +0200,
> Pierre-Louis Bossart wrote:
>> On 3/28/18 1:35 PM, Takashi Iwai wrote:
>>> On Wed, 28 Mar 2018 19:58:54 +0200,
>>> Pierre-Louis Bossart wrote:
>>>> On 3/28/18 10:20 AM, Takashi Iwai wrote:
>>>>> On Wed, 28 Mar 2018 16:30:09 +0200,
>>>>> Pierre-Louis Bossart wrote:
>>>>>> On 3/25/18 9:58 AM, Takashi Iwai wrote:
>>>>>>> On Sun, 25 Mar 2018 12:46:43 +0200,
>>>>>>> Sriram Periyasamy wrote:
>>>>>>>> On Tue, Mar 20, 2018 at 05:17:35PM +0100, Takashi Iwai wrote:
>>>>>>>>> On Tue, 20 Mar 2018 17:01:06 +0100,
>>>>>>>>> Sriram Periyasamy 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.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>>>>>>>> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
>>>>>>>>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>>>>>>>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>>>>>>> Well, I'm still not convinced with this flag.
>>>>>>>>>
>>>>>>>>> First off, does it really need to be per PCM stream?  The introducing
>>>>>>>> Flag per PCM stream helps where each stream in given system may have
>>>>>>>> different requirement such as low power or low latency based on the
>>>>>>>> use case. For example in case of low power stream, driver can perform
>>>>>>>> required optimizations at hardware level based on the no_rewind flag.
>>>>>>> Yes, but does it really need to be PCM stream, i.e. per application?
>>>>>>> Certainly the system will be using some sound backend (like PA).  In
>>>>>>> which scenario can such behavior change -- some application uses a
>>>>>>> different backend on a phone or a tablet?
>>>>>> This is intended for the case where the system server exposes a
>>>>>> 'deep-buffer' PCM device for music playback in low-power mode and a
>>>>>> separate one for system sounds or anything that requires
>>>>>> interactivity.
>>>>>> The need for rewinding is really for the case where the interactive
>>>>>> system sounds are mixed with music, when you have separation between
>>>>>> types of sounds and hardware/firmware mixing then the rewinds are
>>>>>> unnecessary.
>>>>> Yes, but why application must tell no-rewind flag if it wants to
>>>>> save a bit of power?  IOW, how each application can know it needs to
>>>>> set no-rewind flag *for saving power*?
>>>>>
>>>>> Or, put in another way: you want to make all applications running in
>>>>> lower power generically.  What would you do?  Adding no-rewind flag to
>>>>> all calls?  It makes no sense if the application runs on non-Intel
>>>>> chips, so can't be hard-coded.
>>>>>
>>>>>> If there are multiple applications using different PCM devices each
>>>>>> (which is a bit hypothetical to me) there is no way to know ahead of
>>>>>> time when the modules are loaded if the application will perform
>>>>>> rewinds due to its interactive nature or will just stream without ever
>>>>>> invalidating the ring buffer. So yes it's per stream.
>>>>> Fair enough, per stream is a requirement.
>>>>>
>>>>> But still my argument below applies: what you really want to set
>>>>> is to make the stream low-power.  It's not about to make the stream
>>>>> non-rewindable.  And this makes me feel uneasy.
>>>>>
>>>>>
>>>>>>>>> something to hw_parms implies that it varies per application.  But I
>>>>>>>>> can't imagine that a system requires different behavior per stream
>>>>>>>>> regarding such a thing.
>>>>>>>>>
>>>>>>>>> Second, the driver can implement a check in PCM ack callback to
>>>>>>>>> prevent the rewind, too.  Then there is no need to touch the PCM
>>>>>>>>> core.
>>>>>>>>>
>>>>>>>> As per the previous discussion at [1],
>>>>>>>>
>>>>>>>> [1]
>>>>>>>> https://patchwork.kernel.org/patch/9795233/
>>>>>>>>
>>>>>>>> from Pierre,
>>>>>>>>
>>>>>>>> "The application (which is in most cases an audio server) *knows* if it
>>>>>>>> requires rewinds or not. It's part of its design, with rewinds typically
>>>>>>>> disabled if period interrupts are required. It's been that way for a
>>>>>>>> number of years now. The use of rewinds is typically associated with the
>>>>>>>> combination of a large buffer and no interrupts (having either of the
>>>>>>>> two would not require rewinds).
>>>>>>>>
>>>>>>>> So the idea is that the application makes a statement that rewinds will
>>>>>>>> not be used, and the low-level driver makes use of the information to
>>>>>>>> enable whatever optimizations are available at the hardware level.
>>>>>>>>
>>>>>>>> Exposing more information to userspace would quickly lead to a confusing
>>>>>>>> decision-making and would require more than just a flag."
>>>>>>> And, requiring *that* information is already confusing, IMO.
>>>>>>> Think from the application writer POV: what is the relation between
>>>>>>> the power-saving and no_rewind behavior in application level at all?
>>>>>>> If you have no idea about the hardware details, they are totally
>>>>>>> irrelevant.
>>>>>> I feel like disabling IRQs or disabling rewinds is the same level of
>>>>>> information, you set the flags without necessary knowing all the power
>>>>>> savings down to the mW level. But it provides an opportunity to save
>>>>>> power with additional degrees of freedom for implementations.
>>>>>     Sure, I do understand this will bring the merit.  But the question
>>>>> is
>>>>> the API design.
>>>>>
>>>>>> An additional benefit of using the underlying SPIB register on Intel
>>>>>> hardware is that the DMA hardware will not wrap-around, which can lead
>>>>>> to better detection of real-time issues and a guarantee that stale
>>>>>> data will not be played.
>>>>> So, again, the purpose of no-rewind isn't the rewind thing itself.
>>>>> It's set for obtaining other benefits.
>>>>>
>>>>>>> Or, think like this way: imagine a hardware that requires a different
>>>>>>> constraint, e.g. the power-of-two buffer size, for power-efficient
>>>>>>> operation.  What would you do?  Adding a new power_of_two bit flag
>>>>>>> into hw_params?  Likely not.
>>>>>> we've added the noIRQ mode in the past using flags, if now you are
>>>>>> saying that flags is a bad idea then fine, but let's be consistent...
>>>>> The no-IRQ is rather a more drastic behavior change.  The ALSA PCM
>>>>> mandated the period update per definition, and setting this flag
>>>>> really switches to a different mode, hence it deserves for an API
>>>>> extension.  And, the flag itself is self-explaining: the less IRQ is
>>>>> less power.  But no-rewind is...?
>>>>>
>>>>>>> In such a case, I would expect some operation mode switch
>>>>>>> (e.g. power-saving vs low latency or whatever) instead of a very
>>>>>>> specific hw_parmas flag.  It might be a module option, via ALSA
>>>>>>> control, or something else.  But it's clearer for which purpose it's
>>>>>>> present, at least, and it can be implemented well without changing the
>>>>>>> existing API.
>>>>>> We have no way of predicting what the application will do so the
>>>>>> module option is not possible.
>>>>>>
>>>>>> Using an ALSA control is possible, but it's odd to me.
>>>>>>
>>>>>> I really don't see what's so problematic about adding flags. I uses an
>>>>>> existing capability of the API, it's consistent with the previous
>>>>>> usages. There is no change in behavior for existing apps, only newer
>>>>>> can benefit for better use of the hardware. There is no complicated
>>>>>> decision making, you set the flags if you don't use IRQ or rewinds.
>>>>>> And it's not like we will have new flags every week, we've been
>>>>>> talking about this SPIB capability since Skylake which is 3 years old
>>>>>> already.
>>>>> Again, my concern is that you swapped between the purpose and the
>>>>> method.  The no-irq isn't any purpose, per se.  It's just a
>>>>> requirement some hardware casually applies for power saving.
>>>>>
>>>>> The real need isn't about each detailed hardware-specific flag, but
>>>>> rather some API to give a hint for the preferred operation mode.
>>>> let me try a different explanation (don't want to be pedantic but try
>>>> to explain the line of thought).
>>>>
>>>> There are two needs in terms of application/driver interaction.
>>>>
>>>> The first need is to let the application know about hardware
>>>> capabilities or restrictions. This is handled today with the .info
>>>> fields. We have quite a few of them that provide information to
>>>> userspace and let the application use the ALSA API in different ways
>>>> (e.g. BATCH, BLOCK_TRANSFER, PAUSE, SYNC_START, WALL_CLK). To take the
>>>> example above, if a specific hardware could handle optimizations for
>>>> powers of two, it could add a flag in the .info field and let the
>>>> application make that decision.
>>>>
>>>> The second need is to establish a contract between application and
>>>> driver, set in stone and non modifiable dynamically while the stream
>>>> is open. We are using hw_params for this, and the flags are one way to
>>>> extend the API to new capabilities. the no-irq flag is one example of
>>>> this contract which fundamentally changes the way the application is
>>>> written. It's not limited to power savings but can also be used to
>>>> reduce the latency as done by Android/AAudio, and it's not a 'casual'
>>>> way of doing things but a fundamental design decision.
>>>>
>>>> The proposal in this patchset is to restrict the use of rewinds which
>>>> can have two known benefits
>>>> 1. better DMA handling with opportunistic/bursty transfers
>>>> 2. no wrap-around and handling of stale data.
>>>> This capability may be used for low-power and low-latency, in a
>>>> similar way to the no-irq mode. Whether it makes sense for a system is
>>>> not the debate here, we want to leave the decision making to system
>>>> integrators. Since we use the hw_params flags in the past, we chose
>>>> the same solution. We also did not add any flag in the .info field
>>>> since the application doesn't really need to know what hardware will
>>>> do or not. If it doesn't use rewinds, it just states it and lets the
>>>> hardware enable new capabilities.
>>>>
>>>> I am really struggling to see how the proposal is viewed as different
>>>> from previous ones and where we confused 'purpose and method'. We used
>>>> the same hooks for the same purposes.
>>> I'm fine to change something in the driver internal, and we've
>>> prepared it with the extension of ack callback, so far.  But, again,
>>> if it comes to the question about API, I have to be stubborn.
>>>
>>> The no-IRQ mode was designed for the clear behavior change, thus it's
>>> mandatory to tie with the hw_params.  However, the no-rewind is
>>> different.  It's simple to restrict the rewind from the driver side as
>>> is without changing the current API.  So why it's in hw_params and why
>>> each application must set explicitly?
>> I don't see how the driver could make that decision on its own, sorry.
> The decision should be done by user, of course.  But the question is
> how it's told to the driver.  And I hesitate to add no-rewind flag to
> the generic API just for an Intel device-specific quirk.
>
>> If we restrict the rewinds and then the application calls rewind, we
>> would have a run-time issue and stop playback/capture.
> Yes, the driver needs to know.  It's no doubt there.
>
>>> Yes, it's opt-out.  But it means that it's useless unless application
>>> really knows the very h/w specific feature; i.e. it has to detect the
>>> running system, determine whether this flag is required or not, set it
>>> and stop using rewind.  Doing this for each application?
>>>
>>> And, if you think "hey it's overreaction, it's nothing but an opt-out
>>> feature for only specific machines and specific applications", then
>>> why do we have to implement it in the generic hw_parmams API level,
>>> instead of some driver-specific thing like kcontrol or whatever?
>> It's not just a driver-specific thing, we have to deal with errors in
>> the core if rewinds are called. So no matter what signalling mechanism
>> is used by the application this capability has to be known by the
>> core.
> The core already handles it by calling the ack callback.
> The driver can simply return an error from there for the unexpected /
> unwanted rewind calls.  It's only the matter how to tell to the
> driver.
>
>>> (Again, no-irq is a different situation; the no-IRQ is a special mode
>>>    for timer-based scheduling that makes application to skip the
>>>    mandatory period programming model.  But for no-rewind, there is
>>>    nothing required in the application side, since the rewind operation
>>>    is optional from the very beginning.)
>> but the driver doesn't know if rewinds will ever be used or not...
>>
>>>
>>>> I also don't think that either the no-irq and no-rewinds can be
>>>> assigned to low-power or low-latency usages. I don't think we are in a
>>>> position to make that call and I don't think we want to add a
>>>> 'low-latency' or 'low-power' flag to the ALSA API - this would be
>>>> extremely confusing to applications.
>>>>
>>>> If your position is that we can no longer report or enable new
>>>> capabilities with the .info and hw_params fields, then what are the
>>>> options?
>>>> a) do nothing and leave ALSA handle hardware designed prior to 2013
>>>> b) define a new API and transition both the info and hw_params to
>>>> something else
>>> Extending the hw_params flag itself is fine, but iff it really makes
>>> sense as a generic API usage.  I'm not fully convinced yet that
>>> no_rewind is mandatory, so far.  That's all.
>>>
>>>
>>> And, I'm really interested in how much impact we'd have if we disable
>>> the rewind completely for compensation of SPIB and other DSP stuff.
>>> AFAIK, it's only PA who really uses the rewind feature.  And, PA uses
>>> it for saving power (more exactly, for low-latency with power-saving
>>> operation using timer scheduling).  Now, practically seen, which mode
>>> should PA use?  Would disabling rewind cost too much even with other
>>> benefits?
>> The power savings implemented by PulseAudio with the timer-based
>> scheduling are based on 10-year old designs where the core was the
>> dominant issue.
>>
>> Now the power savings have moved to a different level of
>> optimization. With more buffering in the audio cluster, it's the path
>> to memory and the opportunities to let the audio cluster work in a
>> stand-alone matter than really save power. SPIB and friends really
>> address how the memory is accessed and if transfers can be scheduled
>> e.g. when the path to memory is already active due to some other
>> event.
>>
>>
>> PulseAudio cannot benefit from proposal with the current
>> implementation. It would only benefit when there are two streams, one
>> for deep-buffer and one for system/low-latency streams, which is where
>> the transfers can be optimized.
>>
>> Chrome can be benefit immediately since rewinds are never used.
>> Jack can benefit immediately since rewinds are never used
>> Android HALs can benefit immediately since rewinds are never used.
> ... if you patch all these.  That's not immediate.
It's a single flag to add to the hw_params. It'll take time to ripple to 
distros but it's not really complex.
> OTOH, if you do introduce a single switch, it's immediately
> effective, even with the old user-space without compilation.
Humm, now you lost me.
In your replies, you stated that the applications need to tell the 
driver - but disagreed that a hw_params flag was the right way. I am not 
religious here, as long as it remains simple enough we can look at other 
options.

I don't get however what 'single switch' are you referring to here?
In all cases, I don't see how we can enable this without rebuilding the 
apps.
Except for ChromeOS, I don't know how a distro would enable a switch 
that would impact all apps using the ALSA API.

>
> And, now back to the question: in which situation you'll have to mix
> both no-rewind and rewind operations on a single machine?  In theory,
> yes, we may keep rewind working, but from the practical POV, it is
> only PA who uses the rewind feature.  And, even if we disable rewind,
> PA will still work as is.  (I know it because I broke it previously :)
define 'work'. If PulseAudio cannot use the rewinds, then the system 
sounds will be heard with a delay and transitions between use cases will 
only happen when the queued audio is finished playing.

If an app wants to play a sound immediately due to user interaction 
requirements, rewinds are an attractive solution. I don't know how we 
could determine ahead of time what userspace will do, certainly the use 
of rewinds is on a per-card basis and likely a per-stream decision.

> So, what's wrong with introducing a global switch instead of changing
> the existing API and requiring the code change in each application and
> rebuild?
I totally agree that pretty much all apps don't do any rewinds, and 
rewinds on capture is quite useless. But I stuck to the 'we don't break 
userspace' mantra with the idea of an opt-in flag requiring an explicit 
code change to take effect. we will break userspace if we add a 
kernel-level switch, and the net result is that no one will ever use 
this option.

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

[Takashi Iwai]

On Wed, 28 Mar 2018 23:51:46 +0200,
Pierre-Louis Bossart wrote:
> >> Chrome can be benefit immediately since rewinds are never used.
> >> Jack can benefit immediately since rewinds are never used
> >> Android HALs can benefit immediately since rewinds are never used.
> > ... if you patch all these.  That's not immediate.
> It's a single flag to add to the hw_params. It'll take time to ripple
> to distros but it's not really complex.

Yes, but you have to modify *each* of them.  That's flaky.
Sure, it's safer for not enabling it as default, but is it your goal?

> > OTOH, if you do introduce a single switch, it's immediately
> > effective, even with the old user-space without compilation.
> Humm, now you lost me.
> In your replies, you stated that the applications need to tell the
> driver - but disagreed that a hw_params flag was the right way. I am
> not religious here, as long as it remains simple enough we can look at
> other options.
> 
> I don't get however what 'single switch' are you referring to here?

Just a simple module option or a kctl, for example.
That is, if we can forget about the ability for adjustment per stream,
the operation mode can be flipped by a switch on the fly.  This makes
things a lot easier.

> In all cases, I don't see how we can enable this without rebuilding
> the apps.
> Except for ChromeOS, I don't know how a distro would enable a switch
> that would impact all apps using the ALSA API.

So here came the question how it would impact on PA.
If it doesn't work for PA, it means that essentially all traditional
Linux distros will turn it off as default.  It's only for Android and
Chrome OS, and they have own setup.  They can turn it on as default.

If anyone wants to turn it on for JACK, they can do it.  But I don't
think people would mix up JACK and PA on the same system at the very
same time.  (One can hook up as a client, but then it doesn't matter
about this hardware feature.)

> > And, now back to the question: in which situation you'll have to mix
> > both no-rewind and rewind operations on a single machine?  In theory,
> > yes, we may keep rewind working, but from the practical POV, it is
> > only PA who uses the rewind feature.  And, even if we disable rewind,
> > PA will still work as is.  (I know it because I broke it previously :)
> define 'work'. If PulseAudio cannot use the rewinds, then the system
> sounds will be heard with a delay and transitions between use cases
> will only happen when the queued audio is finished playing.

I just tried music players with PA, and disabled the rewind forcibly
(by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
couldn't hear any audible difference in its response for changing the
stream position, etc, interactively.  There was no audible latency
increase by that change.

It was the test weeks ago, so I refreshed testing now.  And the
result is same, I saw no response difference.
Any specific workload to make the clear difference in your mind?

> If an app wants to play a sound immediately due to user interaction
> requirements, rewinds are an attractive solution. I don't know how we
> could determine ahead of time what userspace will do, certainly the
> use of rewinds is on a per-card basis and likely a per-stream
> decision.
> 
> > So, what's wrong with introducing a global switch instead of changing
> > the existing API and requiring the code change in each application and
> > rebuild?
> I totally agree that pretty much all apps don't do any rewinds, and
> rewinds on capture is quite useless. But I stuck to the 'we don't
> break userspace' mantra with the idea of an opt-in flag requiring an
> explicit code change to take effect. we will break userspace if we add
> a kernel-level switch, and the net result is that no one will ever use
> this option.

Sure, I agree with the golden "no regression" rule, too.  So I thought
of some switch that is default off for the systems that may use
rewind, while turning it on as default on the known systems like
Android.  As PA is the sole user of rewind, here we won't see any
regression, in theory.

And, such a system-level switch is preferable from the system
management POV.  It's easier than a hard-coded API extension flag, and
you can toggle it at any time for debugging if a problem happens.

*IFF* the no-rewind feature is required by other multiple systems,
adding some API would make sense.  But, currently, no-rewind is needed
only for enabling some feature that is indirectly involved with the
rewind on Intel chips.  To my eyes, it's too far away from the purpose
of the PCM hw_params API.

(Again, no-irq was in a different situation; it's a flag for a mode
 that didn't exist (zero period) in the hw_params space.  But
 no-rewind is irrelevant with the hw_params configuration itself.)


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 03/29/2018 10:42 AM, Takashi Iwai wrote:
> On Wed, 28 Mar 2018 23:51:46 +0200,
> Pierre-Louis Bossart wrote:
>>>> Chrome can be benefit immediately since rewinds are never used.
>>>> Jack can benefit immediately since rewinds are never used
>>>> Android HALs can benefit immediately since rewinds are never used.
>>> ... if you patch all these.  That's not immediate.
>> It's a single flag to add to the hw_params. It'll take time to ripple
>> to distros but it's not really complex.
> Yes, but you have to modify *each* of them.  That's flaky.
> Sure, it's safer for not enabling it as default, but is it your goal?
>
>>> OTOH, if you do introduce a single switch, it's immediately
>>> effective, even with the old user-space without compilation.
>> Humm, now you lost me.
>> In your replies, you stated that the applications need to tell the
>> driver - but disagreed that a hw_params flag was the right way. I am
>> not religious here, as long as it remains simple enough we can look at
>> other options.
>>
>> I don't get however what 'single switch' are you referring to here?
> Just a simple module option or a kctl, for example.
> That is, if we can forget about the ability for adjustment per stream,
> the operation mode can be flipped by a switch on the fly.  This makes
> things a lot easier.
>
>> In all cases, I don't see how we can enable this without rebuilding
>> the apps.
>> Except for ChromeOS, I don't know how a distro would enable a switch
>> that would impact all apps using the ALSA API.
> So here came the question how it would impact on PA.
> If it doesn't work for PA, it means that essentially all traditional
> Linux distros will turn it off as default.  It's only for Android and
> Chrome OS, and they have own setup.  They can turn it on as default.
>
> If anyone wants to turn it on for JACK, they can do it.  But I don't
> think people would mix up JACK and PA on the same system at the very
> same time.  (One can hook up as a client, but then it doesn't matter
> about this hardware feature.)
I do recall Lennart implementing a mechanism to hand-over the ALSA 
resources between PulseAudio and Jack so there is a precedent here.
>
>>> And, now back to the question: in which situation you'll have to mix
>>> both no-rewind and rewind operations on a single machine?  In theory,
>>> yes, we may keep rewind working, but from the practical POV, it is
>>> only PA who uses the rewind feature.  And, even if we disable rewind,
>>> PA will still work as is.  (I know it because I broke it previously :)
>> define 'work'. If PulseAudio cannot use the rewinds, then the system
>> sounds will be heard with a delay and transitions between use cases
>> will only happen when the queued audio is finished playing.
> I just tried music players with PA, and disabled the rewind forcibly
> (by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
> couldn't hear any audible difference in its response for changing the
> stream position, etc, interactively.  There was no audible latency
> increase by that change.
>
> It was the test weeks ago, so I refreshed testing now.  And the
> result is same, I saw no response difference.
> Any specific workload to make the clear difference in your mind?
It depends on how the app is written and what the configuration of the 
server is. Back in the Meego days the plan was to use very long buffers 
and not rewinding did impact user experience. It's the same issue on 
Windows and Android with deep buffers, source transitions, volume 
changes can be painful when the committed samples cannot be invalidated.
>
>> If an app wants to play a sound immediately due to user interaction
>> requirements, rewinds are an attractive solution. I don't know how we
>> could determine ahead of time what userspace will do, certainly the
>> use of rewinds is on a per-card basis and likely a per-stream
>> decision.
>>
>>> So, what's wrong with introducing a global switch instead of changing
>>> the existing API and requiring the code change in each application and
>>> rebuild?
>> I totally agree that pretty much all apps don't do any rewinds, and
>> rewinds on capture is quite useless. But I stuck to the 'we don't
>> break userspace' mantra with the idea of an opt-in flag requiring an
>> explicit code change to take effect. we will break userspace if we add
>> a kernel-level switch, and the net result is that no one will ever use
>> this option.
> Sure, I agree with the golden "no regression" rule, too.  So I thought
> of some switch that is default off for the systems that may use
> rewind, while turning it on as default on the known systems like
> Android.  As PA is the sole user of rewind, here we won't see any
> regression, in theory.
The regression will depend on the depth of the buffer and whether apps 
are ok to indicate the max latency when connecting with PulseAudio. 
we've done crazy things with PulseAudio in the past and things will go 
wrong.

>
> And, such a system-level switch is preferable from the system
> management POV.  It's easier than a hard-coded API extension flag, and
> you can toggle it at any time for debugging if a problem happens.
>
> *IFF* the no-rewind feature is required by other multiple systems,
> adding some API would make sense.  But, currently, no-rewind is needed
> only for enabling some feature that is indirectly involved with the
> rewind on Intel chips.  To my eyes, it's too far away from the purpose
> of the PCM hw_params API.
>
> (Again, no-irq was in a different situation; it's a flag for a mode
>   that didn't exist (zero period) in the hw_params space.  But
>   no-rewind is irrelevant with the hw_params configuration itself.)

I hope we didn't give you the wrong impression that we are abusing the 
API for nefarious or Intel-specific views.
In the initial patchset some 2+ years ago, there was this change that 
disabled rewinds but also an additional functionality that let drivers 
expose the granularity of the DMA bursts to userspace (I think Jaroslav 
wanted it to be called in-flight-bytes), and possibly set them depending 
on the application need. The latter part is of interest to others, and I 
don't think removing the rewinds is useful only to Intel, as soon as 
people use a second-level buffer rewinds are problematic for everyone.

So maybe a way to progress is to leave this flag set as a module 
parameter as you suggested for now, but with the information known to 
the core, which lets Intel enable functionality on Google OSes 
(Android/Chrome), and in a second step (when we are done with SOF 
upstream) suggest a more generic API enhancement related to DMA handling 
which is not Intel specific.

Would that be agreeable?

Thanks
-Pierre

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

[Takashi Iwai]

On Thu, 29 Mar 2018 21:16:58 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 03/29/2018 10:42 AM, Takashi Iwai wrote:
> > On Wed, 28 Mar 2018 23:51:46 +0200,
> > Pierre-Louis Bossart wrote:
> >>>> Chrome can be benefit immediately since rewinds are never used.
> >>>> Jack can benefit immediately since rewinds are never used
> >>>> Android HALs can benefit immediately since rewinds are never used.
> >>> ... if you patch all these.  That's not immediate.
> >> It's a single flag to add to the hw_params. It'll take time to ripple
> >> to distros but it's not really complex.
> > Yes, but you have to modify *each* of them.  That's flaky.
> > Sure, it's safer for not enabling it as default, but is it your goal?
> >
> >>> OTOH, if you do introduce a single switch, it's immediately
> >>> effective, even with the old user-space without compilation.
> >> Humm, now you lost me.
> >> In your replies, you stated that the applications need to tell the
> >> driver - but disagreed that a hw_params flag was the right way. I am
> >> not religious here, as long as it remains simple enough we can look at
> >> other options.
> >>
> >> I don't get however what 'single switch' are you referring to here?
> > Just a simple module option or a kctl, for example.
> > That is, if we can forget about the ability for adjustment per stream,
> > the operation mode can be flipped by a switch on the fly.  This makes
> > things a lot easier.
> >
> >> In all cases, I don't see how we can enable this without rebuilding
> >> the apps.
> >> Except for ChromeOS, I don't know how a distro would enable a switch
> >> that would impact all apps using the ALSA API.
> > So here came the question how it would impact on PA.
> > If it doesn't work for PA, it means that essentially all traditional
> > Linux distros will turn it off as default.  It's only for Android and
> > Chrome OS, and they have own setup.  They can turn it on as default.
> >
> > If anyone wants to turn it on for JACK, they can do it.  But I don't
> > think people would mix up JACK and PA on the same system at the very
> > same time.  (One can hook up as a client, but then it doesn't matter
> > about this hardware feature.)
> I do recall Lennart implementing a mechanism to hand-over the ALSA
> resources between PulseAudio and Jack so there is a precedent here.
> >
> >>> And, now back to the question: in which situation you'll have to mix
> >>> both no-rewind and rewind operations on a single machine?  In theory,
> >>> yes, we may keep rewind working, but from the practical POV, it is
> >>> only PA who uses the rewind feature.  And, even if we disable rewind,
> >>> PA will still work as is.  (I know it because I broke it previously :)
> >> define 'work'. If PulseAudio cannot use the rewinds, then the system
> >> sounds will be heard with a delay and transitions between use cases
> >> will only happen when the queued audio is finished playing.
> > I just tried music players with PA, and disabled the rewind forcibly
> > (by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
> > couldn't hear any audible difference in its response for changing the
> > stream position, etc, interactively.  There was no audible latency
> > increase by that change.
> >
> > It was the test weeks ago, so I refreshed testing now.  And the
> > result is same, I saw no response difference.
> > Any specific workload to make the clear difference in your mind?
> It depends on how the app is written and what the configuration of the
> server is.

Isn't it about what PA does, not apps?

> Back in the Meego days the plan was to use very long
> buffers and not rewinding did impact user experience. It's the same
> issue on Windows and Android with deep buffers, source transitions,
> volume changes can be painful when the committed samples cannot be
> invalidated.

Yes, but we're talking specifically about the Intel HD-audio, and it
apparently works as is without rewind.  So an API change looks
overcommitting (even though it's one bit flag addition).

If the feature is really generic in wider hardware ranges, it's a
convincing factor for changing the API.  Let's see.

> >> If an app wants to play a sound immediately due to user interaction
> >> requirements, rewinds are an attractive solution. I don't know how we
> >> could determine ahead of time what userspace will do, certainly the
> >> use of rewinds is on a per-card basis and likely a per-stream
> >> decision.
> >>
> >>> So, what's wrong with introducing a global switch instead of changing
> >>> the existing API and requiring the code change in each application and
> >>> rebuild?
> >> I totally agree that pretty much all apps don't do any rewinds, and
> >> rewinds on capture is quite useless. But I stuck to the 'we don't
> >> break userspace' mantra with the idea of an opt-in flag requiring an
> >> explicit code change to take effect. we will break userspace if we add
> >> a kernel-level switch, and the net result is that no one will ever use
> >> this option.
> > Sure, I agree with the golden "no regression" rule, too.  So I thought
> > of some switch that is default off for the systems that may use
> > rewind, while turning it on as default on the known systems like
> > Android.  As PA is the sole user of rewind, here we won't see any
> > regression, in theory.
> The regression will depend on the depth of the buffer and whether apps
> are ok to indicate the max latency when connecting with
> PulseAudio. we've done crazy things with PulseAudio in the past and
> things will go wrong.

Yes, obviously we need testing.  But, in this case, the target
hardware is *very* limited.  So covering this shouldn't be a big
matter, I hope.  (And we'll keep a "big red button" to turn it off in
emergency for some time.)


> > And, such a system-level switch is preferable from the system
> > management POV.  It's easier than a hard-coded API extension flag, and
> > you can toggle it at any time for debugging if a problem happens.
> >
> > *IFF* the no-rewind feature is required by other multiple systems,
> > adding some API would make sense.  But, currently, no-rewind is needed
> > only for enabling some feature that is indirectly involved with the
> > rewind on Intel chips.  To my eyes, it's too far away from the purpose
> > of the PCM hw_params API.
> >
> > (Again, no-irq was in a different situation; it's a flag for a mode
> >   that didn't exist (zero period) in the hw_params space.  But
> >   no-rewind is irrelevant with the hw_params configuration itself.)
> 
> I hope we didn't give you the wrong impression that we are abusing the
> API for nefarious or Intel-specific views.
> In the initial patchset some 2+ years ago, there was this change that
> disabled rewinds but also an additional functionality that let drivers
> expose the granularity of the DMA bursts to userspace (I think
> Jaroslav wanted it to be called in-flight-bytes), and possibly set
> them depending on the application need. The latter part is of interest
> to others, and I don't think removing the rewinds is useful only to
> Intel, as soon as people use a second-level buffer rewinds are
> problematic for everyone.

I can think of some interface, too, but using hw_params flag is still
questionable.  It's no worst choice, sure, but it doesn't look like
the best fit, either, IMO -- especially because the rewind is no
target of hw_params config space.

We need to be extremely careful if it comes to API.  After all, API is
the only fixed definition we can never ever change.  Once set, it must
be rock-solid for the next decades...

> So maybe a way to progress is to leave this flag set as a module
> parameter as you suggested for now, but with the information known to
> the core, which lets Intel enable functionality on Google OSes
> (Android/Chrome), and in a second step (when we are done with SOF
> upstream) suggest a more generic API enhancement related to DMA
> handling which is not Intel specific.
> 
> Would that be agreeable?

Yes, that's a compromise.  I'd really love to have the support for
this kind of feature, while I hesitate to add such a spontaneous bit
flag in the global API level.  So let's begin with the driver-specific
implementation, and extend to API level once when we see what are the
real demands in wide range of hardware.


thanks,

Takashi

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

[Pierre-Louis Bossart]

On 03/29/2018 03:10 PM, Takashi Iwai wrote:
> On Thu, 29 Mar 2018 21:16:58 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>> On 03/29/2018 10:42 AM, Takashi Iwai wrote:
>>> On Wed, 28 Mar 2018 23:51:46 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>>> Chrome can be benefit immediately since rewinds are never used.
>>>>>> Jack can benefit immediately since rewinds are never used
>>>>>> Android HALs can benefit immediately since rewinds are never used.
>>>>> ... if you patch all these.  That's not immediate.
>>>> It's a single flag to add to the hw_params. It'll take time to ripple
>>>> to distros but it's not really complex.
>>> Yes, but you have to modify *each* of them.  That's flaky.
>>> Sure, it's safer for not enabling it as default, but is it your goal?
>>>
>>>>> OTOH, if you do introduce a single switch, it's immediately
>>>>> effective, even with the old user-space without compilation.
>>>> Humm, now you lost me.
>>>> In your replies, you stated that the applications need to tell the
>>>> driver - but disagreed that a hw_params flag was the right way. I am
>>>> not religious here, as long as it remains simple enough we can look at
>>>> other options.
>>>>
>>>> I don't get however what 'single switch' are you referring to here?
>>> Just a simple module option or a kctl, for example.
>>> That is, if we can forget about the ability for adjustment per stream,
>>> the operation mode can be flipped by a switch on the fly.  This makes
>>> things a lot easier.
>>>
>>>> In all cases, I don't see how we can enable this without rebuilding
>>>> the apps.
>>>> Except for ChromeOS, I don't know how a distro would enable a switch
>>>> that would impact all apps using the ALSA API.
>>> So here came the question how it would impact on PA.
>>> If it doesn't work for PA, it means that essentially all traditional
>>> Linux distros will turn it off as default.  It's only for Android and
>>> Chrome OS, and they have own setup.  They can turn it on as default.
>>>
>>> If anyone wants to turn it on for JACK, they can do it.  But I don't
>>> think people would mix up JACK and PA on the same system at the very
>>> same time.  (One can hook up as a client, but then it doesn't matter
>>> about this hardware feature.)
>> I do recall Lennart implementing a mechanism to hand-over the ALSA
>> resources between PulseAudio and Jack so there is a precedent here.
>>>>> And, now back to the question: in which situation you'll have to mix
>>>>> both no-rewind and rewind operations on a single machine?  In theory,
>>>>> yes, we may keep rewind working, but from the practical POV, it is
>>>>> only PA who uses the rewind feature.  And, even if we disable rewind,
>>>>> PA will still work as is.  (I know it because I broke it previously :)
>>>> define 'work'. If PulseAudio cannot use the rewinds, then the system
>>>> sounds will be heard with a delay and transitions between use cases
>>>> will only happen when the queued audio is finished playing.
>>> I just tried music players with PA, and disabled the rewind forcibly
>>> (by putting return 0 in rewind_appl_ptr() in pcm_native.c).  I
>>> couldn't hear any audible difference in its response for changing the
>>> stream position, etc, interactively.  There was no audible latency
>>> increase by that change.
>>>
>>> It was the test weeks ago, so I refreshed testing now.  And the
>>> result is same, I saw no response difference.
>>> Any specific workload to make the clear difference in your mind?
>> It depends on how the app is written and what the configuration of the
>> server is.
> Isn't it about what PA does, not apps?
The PulseAudio server has parameters, but the app can ask for a 
different behavior depending on its needs by passing a pa_buffer_attr 
structure, with a -1 value meaning "highest possible latency". This is 
only possible btw with the regular API, apps using the simple API will 
not control this behavior and can't ask for a specific latency.

https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/Developer/Clients/LatencyControl/

>
>> Back in the Meego days the plan was to use very long
>> buffers and not rewinding did impact user experience. It's the same
>> issue on Windows and Android with deep buffers, source transitions,
>> volume changes can be painful when the committed samples cannot be
>> invalidated.
> Yes, but we're talking specifically about the Intel HD-audio, and it
> apparently works as is without rewind.  So an API change looks
> overcommitting (even though it's one bit flag addition).
>
> If the feature is really generic in wider hardware ranges, it's a
> convincing factor for changing the API.  Let's see.
>
>>>> If an app wants to play a sound immediately due to user interaction
>>>> requirements, rewinds are an attractive solution. I don't know how we
>>>> could determine ahead of time what userspace will do, certainly the
>>>> use of rewinds is on a per-card basis and likely a per-stream
>>>> decision.
>>>>
>>>>> So, what's wrong with introducing a global switch instead of changing
>>>>> the existing API and requiring the code change in each application and
>>>>> rebuild?
>>>> I totally agree that pretty much all apps don't do any rewinds, and
>>>> rewinds on capture is quite useless. But I stuck to the 'we don't
>>>> break userspace' mantra with the idea of an opt-in flag requiring an
>>>> explicit code change to take effect. we will break userspace if we add
>>>> a kernel-level switch, and the net result is that no one will ever use
>>>> this option.
>>> Sure, I agree with the golden "no regression" rule, too.  So I thought
>>> of some switch that is default off for the systems that may use
>>> rewind, while turning it on as default on the known systems like
>>> Android.  As PA is the sole user of rewind, here we won't see any
>>> regression, in theory.
>> The regression will depend on the depth of the buffer and whether apps
>> are ok to indicate the max latency when connecting with
>> PulseAudio. we've done crazy things with PulseAudio in the past and
>> things will go wrong.
> Yes, obviously we need testing.  But, in this case, the target
> hardware is *very* limited.  So covering this shouldn't be a big
> matter, I hope.  (And we'll keep a "big red button" to turn it off in
> emergency for some time.)
>
>
>>> And, such a system-level switch is preferable from the system
>>> management POV.  It's easier than a hard-coded API extension flag, and
>>> you can toggle it at any time for debugging if a problem happens.
>>>
>>> *IFF* the no-rewind feature is required by other multiple systems,
>>> adding some API would make sense.  But, currently, no-rewind is needed
>>> only for enabling some feature that is indirectly involved with the
>>> rewind on Intel chips.  To my eyes, it's too far away from the purpose
>>> of the PCM hw_params API.
>>>
>>> (Again, no-irq was in a different situation; it's a flag for a mode
>>>    that didn't exist (zero period) in the hw_params space.  But
>>>    no-rewind is irrelevant with the hw_params configuration itself.)
>> I hope we didn't give you the wrong impression that we are abusing the
>> API for nefarious or Intel-specific views.
>> In the initial patchset some 2+ years ago, there was this change that
>> disabled rewinds but also an additional functionality that let drivers
>> expose the granularity of the DMA bursts to userspace (I think
>> Jaroslav wanted it to be called in-flight-bytes), and possibly set
>> them depending on the application need. The latter part is of interest
>> to others, and I don't think removing the rewinds is useful only to
>> Intel, as soon as people use a second-level buffer rewinds are
>> problematic for everyone.
> I can think of some interface, too, but using hw_params flag is still
> questionable.  It's no worst choice, sure, but it doesn't look like
> the best fit, either, IMO -- especially because the rewind is no
> target of hw_params config space.
>
> We need to be extremely careful if it comes to API.  After all, API is
> the only fixed definition we can never ever change.  Once set, it must
> be rock-solid for the next decades...
>
>> So maybe a way to progress is to leave this flag set as a module
>> parameter as you suggested for now, but with the information known to
>> the core, which lets Intel enable functionality on Google OSes
>> (Android/Chrome), and in a second step (when we are done with SOF
>> upstream) suggest a more generic API enhancement related to DMA
>> handling which is not Intel specific.
>>
>> Would that be agreeable?
> Yes, that's a compromise.  I'd really love to have the support for
> this kind of feature, while I hesitate to add such a spontaneous bit
> flag in the global API level.  So let's begin with the driver-specific
> implementation, and extend to API level once when we see what are the
> real demands in wide range of hardware.

ok, makes sense.
Thanks for your feedback and enjoy your 4 day weekend.
I'll bug you next week with UCM stuff.
-Pierre