[PATCH 0/3] ALSA: Add rewind disable support

[PATCH 0/3] ALSA: Add rewind disable support

[Subhransu S. Prusty]

Rewinds can be disabled when data written in ring buffer will never be
validated. This allows for new HDaudio SPIB DMA functionality(allow fetch
up to the application pointer, no rewind supported). Skylake driver
changes using SPIB will be posted once the core changes are merged.

Based on discussion on community, .ack callback is extended with new
attribute argument, to allow fetch up to the application pointer. Also
drop the mmap of status and control if SPIB functionality is reported by
driver.  There may be some slight performance downside, but it's likely
very small.

Verified the changes with tinyalsa and alsa-lib, with both mmap and
without mmap support and it works fine.

Pierre-Louis Bossart (3):
  ALSA: core: let low-level driver or userspace disable rewinds
  ALSA: core: modify .ack callback to take arguments for updating appl
    ptr
  ALSA: pcm: conditionally avoid mmap of control data

 include/sound/pcm-indirect.h  |  4 ++--
 include/sound/pcm.h           |  9 +++++++-
 include/uapi/sound/asound.h   |  2 ++
 sound/core/pcm_lib.c          |  6 ++++--
 sound/core/pcm_native.c       | 49 ++++++++++++++++++++++++++++++++++++++++++-
 sound/mips/hal2.c             | 14 ++++++++++---
 sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++----
 sound/pci/emu10k1/emupcm.c    |  8 +++++--
 sound/pci/rme32.c             | 15 ++++++++++---
 9 files changed, 107 insertions(+), 18 deletions(-)

-- 
1.9.1

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

[Subhransu S. Prusty]

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

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

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

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>
---
 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 361749e60799..a2682c5f5b72 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -368,6 +368,7 @@ struct snd_pcm_runtime {
 	unsigned int rate_num;
 	unsigned int rate_den;
 	unsigned int no_period_wakeup: 1;
+	unsigned int no_rewinds:1;
 
 	/* -- SW params -- */
 	int tstamp_mode;		/* mmap timestamp is updated */
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index fd41697cb4d3..c697ff90450d 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -365,6 +365,7 @@ struct snd_pcm_info {
 #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 13dec5ec93f2..35dd4ca93f84 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -566,6 +566,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;
@@ -2439,6 +2441,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 	if (frames == 0)
 		return 0;
 
+	if (runtime->no_rewinds)
+		return 0;
+
 	snd_pcm_stream_lock_irq(substream);
 	switch (runtime->status->state) {
 	case SNDRV_PCM_STATE_PREPARED:
@@ -2487,6 +2492,9 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 	if (frames == 0)
 		return 0;
 
+	if (runtime->no_rewinds)
+		return 0;
+
 	snd_pcm_stream_lock_irq(substream);
 	switch (runtime->status->state) {
 	case SNDRV_PCM_STATE_PREPARED:
-- 
1.9.1

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

[Takashi Iwai]

On Tue, 16 May 2017 03:01:56 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> Add new hw_params flag to explicitly tell driver that rewinds will never
> be used. This can be used by low-level driver to optimize DMA operations
> and reduce power consumption. Use this flag only when data written in
> ring buffer will never be invalidated, e.g. any update of appl_ptr is
> final.
> 
> Note that the update of appl_ptr include both a read/write data
> operation as well as snd_pcm_forward() whose behavior is not modified.
> 
> 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>
> ---
>  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 361749e60799..a2682c5f5b72 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -368,6 +368,7 @@ struct snd_pcm_runtime {
>  	unsigned int rate_num;
>  	unsigned int rate_den;
>  	unsigned int no_period_wakeup: 1;
> +	unsigned int no_rewinds:1;
>  
>  	/* -- SW params -- */
>  	int tstamp_mode;		/* mmap timestamp is updated */
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index fd41697cb4d3..c697ff90450d 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -365,6 +365,7 @@ struct snd_pcm_info {
>  #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 13dec5ec93f2..35dd4ca93f84 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -566,6 +566,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;
> @@ -2439,6 +2441,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  	if (frames == 0)
>  		return 0;
>  
> +	if (runtime->no_rewinds)
> +		return 0;

I'd return an error here, because it is a clear error -- you declared
that you won't do it but you did.

Also, I wonder whether we should add FORWARD disablement flag as
well.  It's not needed in your case, yes, but FORWARD and REWIND
operations are usually paired.


thanks,

Takashi

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

[Subhransu S. Prusty]

On Tue, May 16, 2017 at 07:53:38AM +0200, Takashi Iwai wrote:
> On Tue, 16 May 2017 03:01:56 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > Add new hw_params flag to explicitly tell driver that rewinds will never
> > be used. This can be used by low-level driver to optimize DMA operations
> > and reduce power consumption. Use this flag only when data written in
> > ring buffer will never be invalidated, e.g. any update of appl_ptr is
> > final.
> > 
> > Note that the update of appl_ptr include both a read/write data
> > operation as well as snd_pcm_forward() whose behavior is not modified.
> > 
> > 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>
> > ---
> >  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 361749e60799..a2682c5f5b72 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -368,6 +368,7 @@ struct snd_pcm_runtime {
> >  	unsigned int rate_num;
> >  	unsigned int rate_den;
> >  	unsigned int no_period_wakeup: 1;
> > +	unsigned int no_rewinds:1;
> >  
> >  	/* -- SW params -- */
> >  	int tstamp_mode;		/* mmap timestamp is updated */
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index fd41697cb4d3..c697ff90450d 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -365,6 +365,7 @@ struct snd_pcm_info {
> >  #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 13dec5ec93f2..35dd4ca93f84 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -566,6 +566,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;
> > @@ -2439,6 +2441,9 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> >  	if (frames == 0)
> >  		return 0;
> >  
> > +	if (runtime->no_rewinds)
> > +		return 0;
> 
> I'd return an error here, because it is a clear error -- you declared
> that you won't do it but you did.

This is done based on the suggestion from Pierre:

"This forces the application to both set the NOREWIND flag 
and change the error handling code on a rewind, when the latter point is 
unnecessary. if no rewind is performed then there is no harm."

> 
> Also, I wonder whether we should add FORWARD disablement flag as
> well.  It's not needed in your case, yes, but FORWARD and REWIND
> operations are usually paired.

Either ways is ok for us. Please suggest the approach.

Regards,
Subhransu
> 
> 
> thanks,
> 
> Takashi

-- 

[PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Subhransu S. Prusty]

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

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

The existing .ack callback is extended with new attribute argument, to
support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
process the application ptr update in the driver like in the skylake
driver which can use this to inform position of appl pointer to host DMA
controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
submitted with a separate patch set.

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

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/sound/pcm-indirect.h  |  4 ++--
 include/sound/pcm.h           |  8 +++++++-
 sound/core/pcm_lib.c          |  6 ++++--
 sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
 sound/mips/hal2.c             | 14 +++++++++++---
 sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
 sound/pci/emu10k1/emupcm.c    |  8 ++++++--
 sound/pci/rme32.c             | 15 ++++++++++++---
 8 files changed, 79 insertions(+), 18 deletions(-)

diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
index 1df7acaaa535..2f647ff970fb 100644
--- a/include/sound/pcm-indirect.h
+++ b/include/sound/pcm-indirect.h
@@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
 	if (rec->sw_io >= rec->sw_buffer_size)
 		rec->sw_io -= rec->sw_buffer_size;
 	if (substream->ops->ack)
-		substream->ops->ack(substream);
+		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
 	return bytes_to_frames(substream->runtime, rec->sw_io);
 }
 
@@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
 	if (rec->sw_io >= rec->sw_buffer_size)
 		rec->sw_io -= rec->sw_buffer_size;
 	if (substream->ops->ack)
-		substream->ops->ack(substream);
+		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
 	return bytes_to_frames(substream->runtime, rec->sw_io);
 }
 
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index a2682c5f5b72..0151552342f9 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -63,6 +63,12 @@ struct snd_pcm_hardware {
 struct snd_pcm_audio_tstamp_config; /* definitions further down */
 struct snd_pcm_audio_tstamp_report;
 
+/*
+ * Attibute to distinguish the ack for legacy code and pointer update.
+ */
+#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
+#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
+
 struct snd_pcm_ops {
 	int (*open)(struct snd_pcm_substream *substream);
 	int (*close)(struct snd_pcm_substream *substream);
@@ -86,7 +92,7 @@ struct snd_pcm_ops {
 	struct page *(*page)(struct snd_pcm_substream *substream,
 			     unsigned long offset);
 	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
-	int (*ack)(struct snd_pcm_substream *substream);
+	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
 };
 
 /*
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 5088d4b8db22..b25af69a67da 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
 			appl_ptr -= runtime->boundary;
 		runtime->control->appl_ptr = appl_ptr;
 		if (substream->ops->ack)
-			substream->ops->ack(substream);
+			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
+						SND_PCM_ACK_APP_PTR);
 
 		offset += frames;
 		size -= frames;
@@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
 			appl_ptr -= runtime->boundary;
 		runtime->control->appl_ptr = appl_ptr;
 		if (substream->ops->ack)
-			substream->ops->ack(substream);
+			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
+						SND_PCM_ACK_APP_PTR);
 
 		offset += frames;
 		size -= frames;
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 35dd4ca93f84..c14cdbd9ff86 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->ack)
+		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 		appl_ptr += runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->ack)
+		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 		appl_ptr -= runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->ack)
+		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 		appl_ptr -= runtime->boundary;
 	runtime->control->appl_ptr = appl_ptr;
 	ret = frames;
+
+	if (substream->ops->ack)
+		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
+
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			return err;
 	}
 	snd_pcm_stream_lock_irq(substream);
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		/* boundary wrap-around is assumed to be handled in userspace */
 		control->appl_ptr = sync_ptr.c.control.appl_ptr;
+
+		/* let low-level driver know about appl_ptr change */
+		if (substream->ops->ack)
+			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
+	}
 	else
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
index 00fc9241d266..3740381b188a 100644
--- a/sound/mips/hal2.c
+++ b/sound/mips/hal2.c
@@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
 	case SNDRV_PCM_TRIGGER_START:
 		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
 		hal2->dac.pcm_indirect.hw_data = 0;
-		substream->ops->ack(substream);
+		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
 		hal2_start_dac(hal2);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
@@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
 
 }
 
-static int hal2_playback_ack(struct snd_pcm_substream *substream)
+static int hal2_playback_ack(struct snd_pcm_substream *substream,
+			     unsigned int ack_attr)
 {
 	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
 	struct hal2_codec *dac = &hal2->dac;
 
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
 	snd_pcm_indirect_playback_transfer(substream,
 					   &dac->pcm_indirect,
@@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
 	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
 }
 
-static int hal2_capture_ack(struct snd_pcm_substream *substream)
+static int hal2_capture_ack(struct snd_pcm_substream *substream,
+			    unsigned int ack_attr)
 {
 	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
 	struct hal2_codec *adc = &hal2->adc;
 
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	snd_pcm_indirect_capture_transfer(substream,
 					  &adc->pcm_indirect,
 					  hal2_capture_transfer);
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index e4cf3187b4dd..877edda61e45 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
 	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
 }
 
-static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
+static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
+					unsigned int ack_attr)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
+
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
 	return 0;
 }
@@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
 	       chip->capt.hw_buf.area + rec->hw_data, bytes);
 }
 
-static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
+static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
+					       unsigned int ack_attr)
 {
 	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
+
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
 	return 0;
 }
@@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
 			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
 
 		if (substream->runtime->periods != CS46XX_FRAGS)
-			snd_cs46xx_playback_transfer(substream);
+			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
 #else
 		spin_lock(&chip->reg_lock);
 		if (substream->runtime->periods != CS46XX_FRAGS)
-			snd_cs46xx_playback_transfer(substream);
+			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
 		{ unsigned int tmp;
 		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
 		tmp &= 0x0000ffff;
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index ef1cf530c929..74c78df6e5a8 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
 	pcm->tram_shift = tram_shift;
 }
 
-static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
+static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
+						unsigned int ack_attr)
 {
 	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
 	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
 
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
 	return 0;
 }
@@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
 		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
 		if (result < 0)
 			goto __err;
-		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
+		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
 		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
index 96d15db65dfd..3fd8de5bab26 100644
--- a/sound/pci/rme32.c
+++ b/sound/pci/rme32.c
@@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
 	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
 		snd_pcm_group_for_each_entry(s, substream) {
 			if (s == rme32->playback_substream) {
-				s->ops->ack(s);
+				s->ops->ack(s, SND_PCM_ACK_LEGACY);
 				break;
 			}
 		}
@@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
 		    substream->runtime->dma_area + rec->sw_data, bytes);
 }
 
-static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
+static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
+				     unsigned int ack_attr)
 {
 	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
 	struct snd_pcm_indirect *rec, *cprec;
 
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	rec = &rme32->playback_pcm;
 	cprec = &rme32->capture_pcm;
 	spin_lock(&rme32->lock);
@@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
 		      bytes);
 }
 
-static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
+static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
+					unsigned int ack_attr)
 {
 	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
+
+	if (!(ack_attr & SND_PCM_ACK_LEGACY))
+		return 0;
+
 	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
 					  snd_rme32_cp_trans_copy);
 	return 0;
-- 
1.9.1

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Tue, 16 May 2017 03:01:57 +0200,
Subhransu S. Prusty wrote:
> 
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> When appl_ptr is updated let low-level driver know, e.g.  to let the
> low-level driver/hardware pre-fetch data opportunistically.
> 
> The existing .ack callback is extended with new attribute argument, to
> support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> process the application ptr update in the driver like in the skylake
> driver which can use this to inform position of appl pointer to host DMA
> controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> submitted with a separate patch set.
> 
> In the ALSA core, this capability is independent from the NO_REWIND
> hardware flag. The low-level driver may however tie both options and only
> use the updated appl_ptr when rewinds are disabled due to hardware
> limitations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>

It might be me who suggested the extension of the ack ops, but now
looking at the result, I reconsider whether it'd be a better choice if
we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
rewrite the patch in that way for comparison?

Besides that, the flag name SND_PCM_ACK_LEGACY sounds too ambiguous to
me...


thanks,

Takashi

> ---
>  include/sound/pcm-indirect.h  |  4 ++--
>  include/sound/pcm.h           |  8 +++++++-
>  sound/core/pcm_lib.c          |  6 ++++--
>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
>  sound/mips/hal2.c             | 14 +++++++++++---
>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
>  sound/pci/rme32.c             | 15 ++++++++++++---
>  8 files changed, 79 insertions(+), 18 deletions(-)
> 
> diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
> index 1df7acaaa535..2f647ff970fb 100644
> --- a/include/sound/pcm-indirect.h
> +++ b/include/sound/pcm-indirect.h
> @@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>  	if (rec->sw_io >= rec->sw_buffer_size)
>  		rec->sw_io -= rec->sw_buffer_size;
>  	if (substream->ops->ack)
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>  	return bytes_to_frames(substream->runtime, rec->sw_io);
>  }
>  
> @@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>  	if (rec->sw_io >= rec->sw_buffer_size)
>  		rec->sw_io -= rec->sw_buffer_size;
>  	if (substream->ops->ack)
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>  	return bytes_to_frames(substream->runtime, rec->sw_io);
>  }
>  
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index a2682c5f5b72..0151552342f9 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -63,6 +63,12 @@ struct snd_pcm_hardware {
>  struct snd_pcm_audio_tstamp_config; /* definitions further down */
>  struct snd_pcm_audio_tstamp_report;
>  
> +/*
> + * Attibute to distinguish the ack for legacy code and pointer update.
> + */
> +#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
> +#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
> +
>  struct snd_pcm_ops {
>  	int (*open)(struct snd_pcm_substream *substream);
>  	int (*close)(struct snd_pcm_substream *substream);
> @@ -86,7 +92,7 @@ struct snd_pcm_ops {
>  	struct page *(*page)(struct snd_pcm_substream *substream,
>  			     unsigned long offset);
>  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
> -	int (*ack)(struct snd_pcm_substream *substream);
> +	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
>  };
>  
>  /*
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 5088d4b8db22..b25af69a67da 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  			appl_ptr -= runtime->boundary;
>  		runtime->control->appl_ptr = appl_ptr;
>  		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> +						SND_PCM_ACK_APP_PTR);
>  
>  		offset += frames;
>  		size -= frames;
> @@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  			appl_ptr -= runtime->boundary;
>  		runtime->control->appl_ptr = appl_ptr;
>  		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> +						SND_PCM_ACK_APP_PTR);
>  
>  		offset += frames;
>  		size -= frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 35dd4ca93f84..c14cdbd9ff86 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>  			return err;
>  	}
>  	snd_pcm_stream_lock_irq(substream);
> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> +		/* boundary wrap-around is assumed to be handled in userspace */
>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +
> +		/* let low-level driver know about appl_ptr change */
> +		if (substream->ops->ack)
> +			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +	}
>  	else
>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> index 00fc9241d266..3740381b188a 100644
> --- a/sound/mips/hal2.c
> +++ b/sound/mips/hal2.c
> @@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
>  	case SNDRV_PCM_TRIGGER_START:
>  		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
>  		hal2->dac.pcm_indirect.hw_data = 0;
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>  		hal2_start_dac(hal2);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> @@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
>  
>  }
>  
> -static int hal2_playback_ack(struct snd_pcm_substream *substream)
> +static int hal2_playback_ack(struct snd_pcm_substream *substream,
> +			     unsigned int ack_attr)
>  {
>  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>  	struct hal2_codec *dac = &hal2->dac;
>  
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
>  	snd_pcm_indirect_playback_transfer(substream,
>  					   &dac->pcm_indirect,
> @@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
>  	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
>  }
>  
> -static int hal2_capture_ack(struct snd_pcm_substream *substream)
> +static int hal2_capture_ack(struct snd_pcm_substream *substream,
> +			    unsigned int ack_attr)
>  {
>  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>  	struct hal2_codec *adc = &hal2->adc;
>  
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	snd_pcm_indirect_capture_transfer(substream,
>  					  &adc->pcm_indirect,
>  					  hal2_capture_transfer);
> diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
> index e4cf3187b4dd..877edda61e45 100644
> --- a/sound/pci/cs46xx/cs46xx_lib.c
> +++ b/sound/pci/cs46xx/cs46xx_lib.c
> @@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
>  	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
>  }
>  
> -static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
> +static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
> +					unsigned int ack_attr)
>  {
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
>  	return 0;
>  }
> @@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
>  	       chip->capt.hw_buf.area + rec->hw_data, bytes);
>  }
>  
> -static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
> +static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
> +					       unsigned int ack_attr)
>  {
>  	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
>  	return 0;
>  }
> @@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
>  			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
>  
>  		if (substream->runtime->periods != CS46XX_FRAGS)
> -			snd_cs46xx_playback_transfer(substream);
> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>  #else
>  		spin_lock(&chip->reg_lock);
>  		if (substream->runtime->periods != CS46XX_FRAGS)
> -			snd_cs46xx_playback_transfer(substream);
> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>  		{ unsigned int tmp;
>  		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
>  		tmp &= 0x0000ffff;
> diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
> index ef1cf530c929..74c78df6e5a8 100644
> --- a/sound/pci/emu10k1/emupcm.c
> +++ b/sound/pci/emu10k1/emupcm.c
> @@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
>  	pcm->tram_shift = tram_shift;
>  }
>  
> -static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
> +static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
> +						unsigned int ack_attr)
>  {
>  	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
>  	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
>  
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
>  	return 0;
>  }
> @@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
>  		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
>  		if (result < 0)
>  			goto __err;
> -		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
> +		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
>  		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
>  		break;
>  	case SNDRV_PCM_TRIGGER_STOP:
> diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
> index 96d15db65dfd..3fd8de5bab26 100644
> --- a/sound/pci/rme32.c
> +++ b/sound/pci/rme32.c
> @@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
>  	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
>  		snd_pcm_group_for_each_entry(s, substream) {
>  			if (s == rme32->playback_substream) {
> -				s->ops->ack(s);
> +				s->ops->ack(s, SND_PCM_ACK_LEGACY);
>  				break;
>  			}
>  		}
> @@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
>  		    substream->runtime->dma_area + rec->sw_data, bytes);
>  }
>  
> -static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
> +static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
> +				     unsigned int ack_attr)
>  {
>  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
>  	struct snd_pcm_indirect *rec, *cprec;
>  
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	rec = &rme32->playback_pcm;
>  	cprec = &rme32->capture_pcm;
>  	spin_lock(&rme32->lock);
> @@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
>  		      bytes);
>  }
>  
> -static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
> +static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
> +					unsigned int ack_attr)
>  {
>  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>  	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
>  					  snd_rme32_cp_trans_copy);
>  	return 0;
> -- 
> 1.9.1
> 

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Subhransu S. Prusty]

On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> On Tue, 16 May 2017 03:01:57 +0200,
> Subhransu S. Prusty wrote:
> > 
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > 
> > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > low-level driver/hardware pre-fetch data opportunistically.
> > 
> > The existing .ack callback is extended with new attribute argument, to
> > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > process the application ptr update in the driver like in the skylake
> > driver which can use this to inform position of appl pointer to host DMA
> > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > submitted with a separate patch set.
> > 
> > In the ALSA core, this capability is independent from the NO_REWIND
> > hardware flag. The low-level driver may however tie both options and only
> > use the updated appl_ptr when rewinds are disabled due to hardware
> > limitations.
> > 
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> 
> It might be me who suggested the extension of the ack ops, but now
> looking at the result, I reconsider whether it'd be a better choice if
> we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> rewrite the patch in that way for comparison?

Here is the version using update_appl_ptr.

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


Regards,
Subhransu
> 
> Besides that, the flag name SND_PCM_ACK_LEGACY sounds too ambiguous to
> me...
> 
> 
> thanks,
> 
> Takashi
> 
> > ---
> >  include/sound/pcm-indirect.h  |  4 ++--
> >  include/sound/pcm.h           |  8 +++++++-
> >  sound/core/pcm_lib.c          |  6 ++++--
> >  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> >  sound/mips/hal2.c             | 14 +++++++++++---
> >  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> >  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> >  sound/pci/rme32.c             | 15 ++++++++++++---
> >  8 files changed, 79 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
> > index 1df7acaaa535..2f647ff970fb 100644
> > --- a/include/sound/pcm-indirect.h
> > +++ b/include/sound/pcm-indirect.h
> > @@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
> >  	if (rec->sw_io >= rec->sw_buffer_size)
> >  		rec->sw_io -= rec->sw_buffer_size;
> >  	if (substream->ops->ack)
> > -		substream->ops->ack(substream);
> > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> >  	return bytes_to_frames(substream->runtime, rec->sw_io);
> >  }
> >  
> > @@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
> >  	if (rec->sw_io >= rec->sw_buffer_size)
> >  		rec->sw_io -= rec->sw_buffer_size;
> >  	if (substream->ops->ack)
> > -		substream->ops->ack(substream);
> > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> >  	return bytes_to_frames(substream->runtime, rec->sw_io);
> >  }
> >  
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index a2682c5f5b72..0151552342f9 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -63,6 +63,12 @@ struct snd_pcm_hardware {
> >  struct snd_pcm_audio_tstamp_config; /* definitions further down */
> >  struct snd_pcm_audio_tstamp_report;
> >  
> > +/*
> > + * Attibute to distinguish the ack for legacy code and pointer update.
> > + */
> > +#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
> > +#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
> > +
> >  struct snd_pcm_ops {
> >  	int (*open)(struct snd_pcm_substream *substream);
> >  	int (*close)(struct snd_pcm_substream *substream);
> > @@ -86,7 +92,7 @@ struct snd_pcm_ops {
> >  	struct page *(*page)(struct snd_pcm_substream *substream,
> >  			     unsigned long offset);
> >  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
> > -	int (*ack)(struct snd_pcm_substream *substream);
> > +	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
> >  };
> >  
> >  /*
> > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > index 5088d4b8db22..b25af69a67da 100644
> > --- a/sound/core/pcm_lib.c
> > +++ b/sound/core/pcm_lib.c
> > @@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
> >  			appl_ptr -= runtime->boundary;
> >  		runtime->control->appl_ptr = appl_ptr;
> >  		if (substream->ops->ack)
> > -			substream->ops->ack(substream);
> > +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> > +						SND_PCM_ACK_APP_PTR);
> >  
> >  		offset += frames;
> >  		size -= frames;
> > @@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
> >  			appl_ptr -= runtime->boundary;
> >  		runtime->control->appl_ptr = appl_ptr;
> >  		if (substream->ops->ack)
> > -			substream->ops->ack(substream);
> > +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> > +						SND_PCM_ACK_APP_PTR);
> >  
> >  		offset += frames;
> >  		size -= frames;
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index 35dd4ca93f84..c14cdbd9ff86 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> >  		appl_ptr += runtime->boundary;
> >  	runtime->control->appl_ptr = appl_ptr;
> >  	ret = frames;
> > +
> > +	if (substream->ops->ack)
> > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > +
> >   __end:
> >  	snd_pcm_stream_unlock_irq(substream);
> >  	return ret;
> > @@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
> >  		appl_ptr += runtime->boundary;
> >  	runtime->control->appl_ptr = appl_ptr;
> >  	ret = frames;
> > +
> > +	if (substream->ops->ack)
> > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > +
> >   __end:
> >  	snd_pcm_stream_unlock_irq(substream);
> >  	return ret;
> > @@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
> >  		appl_ptr -= runtime->boundary;
> >  	runtime->control->appl_ptr = appl_ptr;
> >  	ret = frames;
> > +
> > +	if (substream->ops->ack)
> > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > +
> >   __end:
> >  	snd_pcm_stream_unlock_irq(substream);
> >  	return ret;
> > @@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
> >  		appl_ptr -= runtime->boundary;
> >  	runtime->control->appl_ptr = appl_ptr;
> >  	ret = frames;
> > +
> > +	if (substream->ops->ack)
> > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > +
> >   __end:
> >  	snd_pcm_stream_unlock_irq(substream);
> >  	return ret;
> > @@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
> >  			return err;
> >  	}
> >  	snd_pcm_stream_lock_irq(substream);
> > -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> > +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> > +		/* boundary wrap-around is assumed to be handled in userspace */
> >  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> > +
> > +		/* let low-level driver know about appl_ptr change */
> > +		if (substream->ops->ack)
> > +			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > +	}
> >  	else
> >  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
> >  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> > diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> > index 00fc9241d266..3740381b188a 100644
> > --- a/sound/mips/hal2.c
> > +++ b/sound/mips/hal2.c
> > @@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
> >  	case SNDRV_PCM_TRIGGER_START:
> >  		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
> >  		hal2->dac.pcm_indirect.hw_data = 0;
> > -		substream->ops->ack(substream);
> > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> >  		hal2_start_dac(hal2);
> >  		break;
> >  	case SNDRV_PCM_TRIGGER_STOP:
> > @@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
> >  
> >  }
> >  
> > -static int hal2_playback_ack(struct snd_pcm_substream *substream)
> > +static int hal2_playback_ack(struct snd_pcm_substream *substream,
> > +			     unsigned int ack_attr)
> >  {
> >  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
> >  	struct hal2_codec *dac = &hal2->dac;
> >  
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
> >  	snd_pcm_indirect_playback_transfer(substream,
> >  					   &dac->pcm_indirect,
> > @@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
> >  	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
> >  }
> >  
> > -static int hal2_capture_ack(struct snd_pcm_substream *substream)
> > +static int hal2_capture_ack(struct snd_pcm_substream *substream,
> > +			    unsigned int ack_attr)
> >  {
> >  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
> >  	struct hal2_codec *adc = &hal2->adc;
> >  
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	snd_pcm_indirect_capture_transfer(substream,
> >  					  &adc->pcm_indirect,
> >  					  hal2_capture_transfer);
> > diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
> > index e4cf3187b4dd..877edda61e45 100644
> > --- a/sound/pci/cs46xx/cs46xx_lib.c
> > +++ b/sound/pci/cs46xx/cs46xx_lib.c
> > @@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
> >  	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
> >  }
> >  
> > -static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
> > +static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
> > +					unsigned int ack_attr)
> >  {
> >  	struct snd_pcm_runtime *runtime = substream->runtime;
> >  	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
> > +
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
> >  	return 0;
> >  }
> > @@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
> >  	       chip->capt.hw_buf.area + rec->hw_data, bytes);
> >  }
> >  
> > -static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
> > +static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
> > +					       unsigned int ack_attr)
> >  {
> >  	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
> > +
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
> >  	return 0;
> >  }
> > @@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
> >  			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
> >  
> >  		if (substream->runtime->periods != CS46XX_FRAGS)
> > -			snd_cs46xx_playback_transfer(substream);
> > +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
> >  #else
> >  		spin_lock(&chip->reg_lock);
> >  		if (substream->runtime->periods != CS46XX_FRAGS)
> > -			snd_cs46xx_playback_transfer(substream);
> > +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
> >  		{ unsigned int tmp;
> >  		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
> >  		tmp &= 0x0000ffff;
> > diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
> > index ef1cf530c929..74c78df6e5a8 100644
> > --- a/sound/pci/emu10k1/emupcm.c
> > +++ b/sound/pci/emu10k1/emupcm.c
> > @@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
> >  	pcm->tram_shift = tram_shift;
> >  }
> >  
> > -static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
> > +static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
> > +						unsigned int ack_attr)
> >  {
> >  	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
> >  	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
> >  
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
> >  	return 0;
> >  }
> > @@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
> >  		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
> >  		if (result < 0)
> >  			goto __err;
> > -		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
> > +		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
> >  		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
> >  		break;
> >  	case SNDRV_PCM_TRIGGER_STOP:
> > diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
> > index 96d15db65dfd..3fd8de5bab26 100644
> > --- a/sound/pci/rme32.c
> > +++ b/sound/pci/rme32.c
> > @@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
> >  	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
> >  		snd_pcm_group_for_each_entry(s, substream) {
> >  			if (s == rme32->playback_substream) {
> > -				s->ops->ack(s);
> > +				s->ops->ack(s, SND_PCM_ACK_LEGACY);
> >  				break;
> >  			}
> >  		}
> > @@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
> >  		    substream->runtime->dma_area + rec->sw_data, bytes);
> >  }
> >  
> > -static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
> > +static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
> > +				     unsigned int ack_attr)
> >  {
> >  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> >  	struct snd_pcm_indirect *rec, *cprec;
> >  
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	rec = &rme32->playback_pcm;
> >  	cprec = &rme32->capture_pcm;
> >  	spin_lock(&rme32->lock);
> > @@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
> >  		      bytes);
> >  }
> >  
> > -static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
> > +static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
> > +					unsigned int ack_attr)
> >  {
> >  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> > +
> > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > +		return 0;
> > +
> >  	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
> >  					  snd_rme32_cp_trans_copy);
> >  	return 0;
> > -- 
> > 1.9.1
> > 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

-- 

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Subhransu S. Prusty]

On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > On Tue, 16 May 2017 03:01:57 +0200,
> > Subhransu S. Prusty wrote:
> > > 
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > low-level driver/hardware pre-fetch data opportunistically.
> > > 
> > > The existing .ack callback is extended with new attribute argument, to
> > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > process the application ptr update in the driver like in the skylake
> > > driver which can use this to inform position of appl pointer to host DMA
> > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > submitted with a separate patch set.
> > > 
> > > In the ALSA core, this capability is independent from the NO_REWIND
> > > hardware flag. The low-level driver may however tie both options and only
> > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > limitations.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > 
> > It might be me who suggested the extension of the ack ops, but now
> > looking at the result, I reconsider whether it'd be a better choice if
> > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > rewrite the patch in that way for comparison?
> 
> Here is the version using update_appl_ptr.

Hi Takashi,

Did you get a chance to look at the update_appl_ptr changes?
Please let us know which one will be preferable, will submit the patches
accordingly.

Regards,
Subhransu

> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 5344c16854d3..1accb8b522d3 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -87,6 +87,7 @@ struct snd_pcm_ops {
>  			     unsigned long offset);
>  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
>  	int (*ack)(struct snd_pcm_substream *substream);
> +	int (*appl_ptr_update)(struct snd_pcm_substream *substream);
>  };
>  
>  /*
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index bb1261591a1f..1656ca903c08 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2090,6 +2090,9 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> @@ -2322,6 +2325,9 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>  		if (substream->ops->ack)
>  			substream->ops->ack(substream);
>  
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +
>  		offset += frames;
>  		size -= frames;
>  		xfer += frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index be8617be12e9..c56d4ed17497 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2475,6 +2475,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2526,6 +2530,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>  		appl_ptr += runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2575,6 +2583,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2624,6 +2636,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>  		appl_ptr -= runtime->boundary;
>  	runtime->control->appl_ptr = appl_ptr;
>  	ret = frames;
> +
> +	if (substream->ops->appl_ptr_update)
> +		substream->ops->appl_ptr_update(substream);
> +
>   __end:
>  	snd_pcm_stream_unlock_irq(substream);
>  	return ret;
> @@ -2723,8 +2739,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>  			return err;
>  	}
>  	snd_pcm_stream_lock_irq(substream);
> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> +		/* boundary wrap-around is assumed to be handled in userspace */
>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +
> +		/* let low-level driver know about appl_ptr change */
> +		if (substream->ops->appl_ptr_update)
> +			substream->ops->appl_ptr_update(substream);
> +	}
>  	else
>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> 
> 
> Regards,
> Subhransu
> > 
> > Besides that, the flag name SND_PCM_ACK_LEGACY sounds too ambiguous to
> > me...
> > 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > > ---
> > >  include/sound/pcm-indirect.h  |  4 ++--
> > >  include/sound/pcm.h           |  8 +++++++-
> > >  sound/core/pcm_lib.c          |  6 ++++--
> > >  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > >  sound/mips/hal2.c             | 14 +++++++++++---
> > >  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > >  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > >  sound/pci/rme32.c             | 15 ++++++++++++---
> > >  8 files changed, 79 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
> > > index 1df7acaaa535..2f647ff970fb 100644
> > > --- a/include/sound/pcm-indirect.h
> > > +++ b/include/sound/pcm-indirect.h
> > > @@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
> > >  	if (rec->sw_io >= rec->sw_buffer_size)
> > >  		rec->sw_io -= rec->sw_buffer_size;
> > >  	if (substream->ops->ack)
> > > -		substream->ops->ack(substream);
> > > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> > >  	return bytes_to_frames(substream->runtime, rec->sw_io);
> > >  }
> > >  
> > > @@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
> > >  	if (rec->sw_io >= rec->sw_buffer_size)
> > >  		rec->sw_io -= rec->sw_buffer_size;
> > >  	if (substream->ops->ack)
> > > -		substream->ops->ack(substream);
> > > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> > >  	return bytes_to_frames(substream->runtime, rec->sw_io);
> > >  }
> > >  
> > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > > index a2682c5f5b72..0151552342f9 100644
> > > --- a/include/sound/pcm.h
> > > +++ b/include/sound/pcm.h
> > > @@ -63,6 +63,12 @@ struct snd_pcm_hardware {
> > >  struct snd_pcm_audio_tstamp_config; /* definitions further down */
> > >  struct snd_pcm_audio_tstamp_report;
> > >  
> > > +/*
> > > + * Attibute to distinguish the ack for legacy code and pointer update.
> > > + */
> > > +#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
> > > +#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
> > > +
> > >  struct snd_pcm_ops {
> > >  	int (*open)(struct snd_pcm_substream *substream);
> > >  	int (*close)(struct snd_pcm_substream *substream);
> > > @@ -86,7 +92,7 @@ struct snd_pcm_ops {
> > >  	struct page *(*page)(struct snd_pcm_substream *substream,
> > >  			     unsigned long offset);
> > >  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
> > > -	int (*ack)(struct snd_pcm_substream *substream);
> > > +	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
> > >  };
> > >  
> > >  /*
> > > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> > > index 5088d4b8db22..b25af69a67da 100644
> > > --- a/sound/core/pcm_lib.c
> > > +++ b/sound/core/pcm_lib.c
> > > @@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
> > >  			appl_ptr -= runtime->boundary;
> > >  		runtime->control->appl_ptr = appl_ptr;
> > >  		if (substream->ops->ack)
> > > -			substream->ops->ack(substream);
> > > +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> > > +						SND_PCM_ACK_APP_PTR);
> > >  
> > >  		offset += frames;
> > >  		size -= frames;
> > > @@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
> > >  			appl_ptr -= runtime->boundary;
> > >  		runtime->control->appl_ptr = appl_ptr;
> > >  		if (substream->ops->ack)
> > > -			substream->ops->ack(substream);
> > > +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> > > +						SND_PCM_ACK_APP_PTR);
> > >  
> > >  		offset += frames;
> > >  		size -= frames;
> > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > > index 35dd4ca93f84..c14cdbd9ff86 100644
> > > --- a/sound/core/pcm_native.c
> > > +++ b/sound/core/pcm_native.c
> > > @@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
> > >  		appl_ptr += runtime->boundary;
> > >  	runtime->control->appl_ptr = appl_ptr;
> > >  	ret = frames;
> > > +
> > > +	if (substream->ops->ack)
> > > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > > +
> > >   __end:
> > >  	snd_pcm_stream_unlock_irq(substream);
> > >  	return ret;
> > > @@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
> > >  		appl_ptr += runtime->boundary;
> > >  	runtime->control->appl_ptr = appl_ptr;
> > >  	ret = frames;
> > > +
> > > +	if (substream->ops->ack)
> > > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > > +
> > >   __end:
> > >  	snd_pcm_stream_unlock_irq(substream);
> > >  	return ret;
> > > @@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
> > >  		appl_ptr -= runtime->boundary;
> > >  	runtime->control->appl_ptr = appl_ptr;
> > >  	ret = frames;
> > > +
> > > +	if (substream->ops->ack)
> > > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > > +
> > >   __end:
> > >  	snd_pcm_stream_unlock_irq(substream);
> > >  	return ret;
> > > @@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
> > >  		appl_ptr -= runtime->boundary;
> > >  	runtime->control->appl_ptr = appl_ptr;
> > >  	ret = frames;
> > > +
> > > +	if (substream->ops->ack)
> > > +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > > +
> > >   __end:
> > >  	snd_pcm_stream_unlock_irq(substream);
> > >  	return ret;
> > > @@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
> > >  			return err;
> > >  	}
> > >  	snd_pcm_stream_lock_irq(substream);
> > > -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> > > +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> > > +		/* boundary wrap-around is assumed to be handled in userspace */
> > >  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> > > +
> > > +		/* let low-level driver know about appl_ptr change */
> > > +		if (substream->ops->ack)
> > > +			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> > > +	}
> > >  	else
> > >  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
> > >  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> > > diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> > > index 00fc9241d266..3740381b188a 100644
> > > --- a/sound/mips/hal2.c
> > > +++ b/sound/mips/hal2.c
> > > @@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
> > >  	case SNDRV_PCM_TRIGGER_START:
> > >  		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
> > >  		hal2->dac.pcm_indirect.hw_data = 0;
> > > -		substream->ops->ack(substream);
> > > +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
> > >  		hal2_start_dac(hal2);
> > >  		break;
> > >  	case SNDRV_PCM_TRIGGER_STOP:
> > > @@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
> > >  
> > >  }
> > >  
> > > -static int hal2_playback_ack(struct snd_pcm_substream *substream)
> > > +static int hal2_playback_ack(struct snd_pcm_substream *substream,
> > > +			     unsigned int ack_attr)
> > >  {
> > >  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
> > >  	struct hal2_codec *dac = &hal2->dac;
> > >  
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
> > >  	snd_pcm_indirect_playback_transfer(substream,
> > >  					   &dac->pcm_indirect,
> > > @@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
> > >  	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
> > >  }
> > >  
> > > -static int hal2_capture_ack(struct snd_pcm_substream *substream)
> > > +static int hal2_capture_ack(struct snd_pcm_substream *substream,
> > > +			    unsigned int ack_attr)
> > >  {
> > >  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
> > >  	struct hal2_codec *adc = &hal2->adc;
> > >  
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	snd_pcm_indirect_capture_transfer(substream,
> > >  					  &adc->pcm_indirect,
> > >  					  hal2_capture_transfer);
> > > diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
> > > index e4cf3187b4dd..877edda61e45 100644
> > > --- a/sound/pci/cs46xx/cs46xx_lib.c
> > > +++ b/sound/pci/cs46xx/cs46xx_lib.c
> > > @@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
> > >  	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
> > >  }
> > >  
> > > -static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
> > > +static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
> > > +					unsigned int ack_attr)
> > >  {
> > >  	struct snd_pcm_runtime *runtime = substream->runtime;
> > >  	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
> > > +
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
> > >  	return 0;
> > >  }
> > > @@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
> > >  	       chip->capt.hw_buf.area + rec->hw_data, bytes);
> > >  }
> > >  
> > > -static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
> > > +static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
> > > +					       unsigned int ack_attr)
> > >  {
> > >  	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
> > > +
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
> > >  	return 0;
> > >  }
> > > @@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
> > >  			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
> > >  
> > >  		if (substream->runtime->periods != CS46XX_FRAGS)
> > > -			snd_cs46xx_playback_transfer(substream);
> > > +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
> > >  #else
> > >  		spin_lock(&chip->reg_lock);
> > >  		if (substream->runtime->periods != CS46XX_FRAGS)
> > > -			snd_cs46xx_playback_transfer(substream);
> > > +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
> > >  		{ unsigned int tmp;
> > >  		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
> > >  		tmp &= 0x0000ffff;
> > > diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
> > > index ef1cf530c929..74c78df6e5a8 100644
> > > --- a/sound/pci/emu10k1/emupcm.c
> > > +++ b/sound/pci/emu10k1/emupcm.c
> > > @@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
> > >  	pcm->tram_shift = tram_shift;
> > >  }
> > >  
> > > -static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
> > > +static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
> > > +						unsigned int ack_attr)
> > >  {
> > >  	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
> > >  	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
> > >  
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
> > >  	return 0;
> > >  }
> > > @@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
> > >  		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
> > >  		if (result < 0)
> > >  			goto __err;
> > > -		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
> > > +		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
> > >  		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
> > >  		break;
> > >  	case SNDRV_PCM_TRIGGER_STOP:
> > > diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
> > > index 96d15db65dfd..3fd8de5bab26 100644
> > > --- a/sound/pci/rme32.c
> > > +++ b/sound/pci/rme32.c
> > > @@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
> > >  	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
> > >  		snd_pcm_group_for_each_entry(s, substream) {
> > >  			if (s == rme32->playback_substream) {
> > > -				s->ops->ack(s);
> > > +				s->ops->ack(s, SND_PCM_ACK_LEGACY);
> > >  				break;
> > >  			}
> > >  		}
> > > @@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
> > >  		    substream->runtime->dma_area + rec->sw_data, bytes);
> > >  }
> > >  
> > > -static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
> > > +static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
> > > +				     unsigned int ack_attr)
> > >  {
> > >  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> > >  	struct snd_pcm_indirect *rec, *cprec;
> > >  
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	rec = &rme32->playback_pcm;
> > >  	cprec = &rme32->capture_pcm;
> > >  	spin_lock(&rme32->lock);
> > > @@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
> > >  		      bytes);
> > >  }
> > >  
> > > -static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
> > > +static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
> > > +					unsigned int ack_attr)
> > >  {
> > >  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> > > +
> > > +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> > > +		return 0;
> > > +
> > >  	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
> > >  					  snd_rme32_cp_trans_copy);
> > >  	return 0;
> > > -- 
> > > 1.9.1
> > > 
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> -- 

-- 

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Thu, 18 May 2017 08:18:21 +0200,
Subhransu S. Prusty wrote:
> 
> On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> > On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > > On Tue, 16 May 2017 03:01:57 +0200,
> > > Subhransu S. Prusty wrote:
> > > > 
> > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > 
> > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > 
> > > > The existing .ack callback is extended with new attribute argument, to
> > > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > process the application ptr update in the driver like in the skylake
> > > > driver which can use this to inform position of appl pointer to host DMA
> > > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > submitted with a separate patch set.
> > > > 
> > > > In the ALSA core, this capability is independent from the NO_REWIND
> > > > hardware flag. The low-level driver may however tie both options and only
> > > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > > limitations.
> > > > 
> > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > 
> > > It might be me who suggested the extension of the ack ops, but now
> > > looking at the result, I reconsider whether it'd be a better choice if
> > > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > > rewrite the patch in that way for comparison?
> > 
> > Here is the version using update_appl_ptr.
> 
> Hi Takashi,
> 
> Did you get a chance to look at the update_appl_ptr changes?
> Please let us know which one will be preferable, will submit the patches
> accordingly.

Now I have a mixed feeling.  Using ack() is basically "the right
thing".  The update of appl_ptr in forward/rewind and sync_ptr should
be notified to ack() in general.  It's the purpose of ack(), after
all.  In that sense, we may call ack() without any argument from any
places.

The only problem is that the rewind is broken on some drivers, and
calling ack() may lead to unexpected results.

That is, we should look at these existing drivers and handle the
rewind case (negative appl_ptr diff) appropriately -- or maybe we
should add a flag to disallow the rewind on such drivers.
After that, ack() can be called safely from all places that update
appl_ptr.

... this is one way.  Another way is to allow a quick hack and doubly
call a new callback.

I prefer the former, but obviously it'll take longer.  So it depends
on urgency.


Takashi

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Thu, 18 May 2017 10:09:23 +0200,
Takashi Iwai wrote:
> 
> On Thu, 18 May 2017 08:18:21 +0200,
> Subhransu S. Prusty wrote:
> > 
> > On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> > > On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > > > On Tue, 16 May 2017 03:01:57 +0200,
> > > > Subhransu S. Prusty wrote:
> > > > > 
> > > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > 
> > > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > > 
> > > > > The existing .ack callback is extended with new attribute argument, to
> > > > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > > process the application ptr update in the driver like in the skylake
> > > > > driver which can use this to inform position of appl pointer to host DMA
> > > > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > > submitted with a separate patch set.
> > > > > 
> > > > > In the ALSA core, this capability is independent from the NO_REWIND
> > > > > hardware flag. The low-level driver may however tie both options and only
> > > > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > > > limitations.
> > > > > 
> > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > 
> > > > It might be me who suggested the extension of the ack ops, but now
> > > > looking at the result, I reconsider whether it'd be a better choice if
> > > > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > > > rewrite the patch in that way for comparison?
> > > 
> > > Here is the version using update_appl_ptr.
> > 
> > Hi Takashi,
> > 
> > Did you get a chance to look at the update_appl_ptr changes?
> > Please let us know which one will be preferable, will submit the patches
> > accordingly.
> 
> Now I have a mixed feeling.  Using ack() is basically "the right
> thing".  The update of appl_ptr in forward/rewind and sync_ptr should
> be notified to ack() in general.  It's the purpose of ack(), after
> all.  In that sense, we may call ack() without any argument from any
> places.
> 
> The only problem is that the rewind is broken on some drivers, and
> calling ack() may lead to unexpected results.
> 
> That is, we should look at these existing drivers and handle the
> rewind case (negative appl_ptr diff) appropriately -- or maybe we
> should add a flag to disallow the rewind on such drivers.
> After that, ack() can be called safely from all places that update
> appl_ptr.
> 
> ... this is one way.  Another way is to allow a quick hack and doubly
> call a new callback.
> 
> I prefer the former, but obviously it'll take longer.  So it depends
> on urgency.

Now I'm thinking of changes like the following: namely, it allows
return an error from ack (it should be!), and does the proper error
handling in forward / rewind / sync_ptr ioctls.  In addition, the
indirect PCM helper checks the negative appl_ptr movement, and returns
an error appropriately.

The total change is attached below.  It'll be split to patches, but
you get an idea.

The merit by this approach is that we "fix" ack callback and its
usages properly without changing its API.  Whenever appl_ptr is moved,
it should have been called for tracking the update.  Now it behaves
so.  And, it also fixes the forgotten error for rewinds, too.

Comments?


thanks,

Takashi

---
diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
index e8cf0b97bf02..3637ddf909a4 100644
--- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
+++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c
@@ -353,9 +353,8 @@ static int snd_bcm2835_pcm_ack(struct snd_pcm_substream *substream)
 	struct snd_pcm_indirect *pcm_indirect = &alsa_stream->pcm_indirect;
 
 	pcm_indirect->hw_queue_size = runtime->hw.buffer_bytes_max;
-	snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
-					   snd_bcm2835_pcm_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, pcm_indirect,
+						  snd_bcm2835_pcm_transfer);
 }
 
 /* trigger callback */
diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
index 1df7acaaa535..7ade285328cf 100644
--- a/include/sound/pcm-indirect.h
+++ b/include/sound/pcm-indirect.h
@@ -43,7 +43,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
 /*
  * helper function for playback ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 				   struct snd_pcm_indirect *rec,
 				   snd_pcm_indirect_copy_t copy)
@@ -56,6 +56,8 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready += (int)frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -82,6 +84,7 @@ snd_pcm_indirect_playback_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready += bytes;
 		rec->sw_ready -= bytes;
 	}
+	return 0;
 }
 
 /*
@@ -109,7 +112,7 @@ snd_pcm_indirect_playback_pointer(struct snd_pcm_substream *substream,
 /*
  * helper function for capture ack callback
  */
-static inline void
+static inline int
 snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 				  struct snd_pcm_indirect *rec,
 				  snd_pcm_indirect_copy_t copy)
@@ -121,6 +124,8 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if (diff < 0)
+			return -EINVAL;
 		rec->sw_ready -= frames_to_bytes(runtime, diff);
 		rec->appl_ptr = appl_ptr;
 	}
@@ -147,6 +152,7 @@ snd_pcm_indirect_capture_transfer(struct snd_pcm_substream *substream,
 		rec->hw_ready -= bytes;
 		rec->sw_ready += bytes;
 	}
+	return 0;
 }
 
 /*
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f3a3580eb44c..a1d1b4540fbb 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2431,13 +2431,68 @@ static int snd_pcm_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static int apply_appl_ptr(struct snd_pcm_substream *substream,
+			  snd_pcm_uframes_t appl_ptr)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_uframes_t old_appl_ptr = runtime->control->appl_ptr;
+	int ret;
+
+	runtime->control->appl_ptr = appl_ptr;
+	if (substream->ops->ack) {
+		ret = substream->ops->ack(substream);
+		if (ret < 0) {
+			runtime->control->appl_ptr = old_appl_ptr;
+			return ret;
+		}
+	}
+	return 0;
+}
+
+static snd_pcm_sframes_t increase_appl_ptr(struct snd_pcm_substream *substream,
+					   snd_pcm_uframes_t frames,
+					   snd_pcm_sframes_t avail)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_sframes_t appl_ptr;
+	int ret;
+
+	if (avail <= 0)
+		return 0;
+	if (frames > (snd_pcm_uframes_t)avail)
+		frames = avail;
+	appl_ptr = runtime->control->appl_ptr + frames;
+	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
+		appl_ptr -= runtime->boundary;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
+}
+
+static snd_pcm_sframes_t decrease_appl_ptr(struct snd_pcm_substream *substream,
+					   snd_pcm_uframes_t frames,
+					   snd_pcm_sframes_t avail)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	snd_pcm_sframes_t appl_ptr;
+	int ret;
+
+	if (avail <= 0)
+		return 0;
+	if (frames > (snd_pcm_uframes_t)avail)
+		frames = avail;
+	appl_ptr = runtime->control->appl_ptr - frames;
+	if (appl_ptr < 0)
+		appl_ptr += runtime->boundary;
+	runtime->control->appl_ptr = appl_ptr;
+	ret = apply_appl_ptr(substream, appl_ptr);
+	return ret < 0 ? ret : frames;
+}
+
 static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *substream,
 						 snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t hw_avail;
 
 	if (frames == 0)
 		return 0;
@@ -2462,18 +2517,8 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
 		goto __end;
 	}
 
-	hw_avail = snd_pcm_playback_hw_avail(runtime);
-	if (hw_avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)hw_avail)
-		frames = hw_avail;
-	appl_ptr = runtime->control->appl_ptr - frames;
-	if (appl_ptr < 0)
-		appl_ptr += runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = decrease_appl_ptr(substream, frames,
+				snd_pcm_playback_hw_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2483,9 +2528,7 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 						snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t hw_avail;
 
 	if (frames == 0)
 		return 0;
@@ -2510,18 +2553,8 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
 		goto __end;
 	}
 
-	hw_avail = snd_pcm_capture_hw_avail(runtime);
-	if (hw_avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)hw_avail)
-		frames = hw_avail;
-	appl_ptr = runtime->control->appl_ptr - frames;
-	if (appl_ptr < 0)
-		appl_ptr += runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = decrease_appl_ptr(substream, frames,
+				snd_pcm_capture_hw_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2531,9 +2564,7 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 						  snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t avail;
 
 	if (frames == 0)
 		return 0;
@@ -2559,18 +2590,8 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
 		goto __end;
 	}
 
-	avail = snd_pcm_playback_avail(runtime);
-	if (avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)avail)
-		frames = avail;
-	appl_ptr = runtime->control->appl_ptr + frames;
-	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
-		appl_ptr -= runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = increase_appl_ptr(substream, frames,
+				snd_pcm_playback_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2580,9 +2601,7 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 						 snd_pcm_uframes_t frames)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
-	snd_pcm_sframes_t appl_ptr;
 	snd_pcm_sframes_t ret;
-	snd_pcm_sframes_t avail;
 
 	if (frames == 0)
 		return 0;
@@ -2608,18 +2627,8 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
 		goto __end;
 	}
 
-	avail = snd_pcm_capture_avail(runtime);
-	if (avail <= 0) {
-		ret = 0;
-		goto __end;
-	}
-	if (frames > (snd_pcm_uframes_t)avail)
-		frames = avail;
-	appl_ptr = runtime->control->appl_ptr + frames;
-	if (appl_ptr >= (snd_pcm_sframes_t)runtime->boundary)
-		appl_ptr -= runtime->boundary;
-	runtime->control->appl_ptr = appl_ptr;
-	ret = frames;
+	ret = increase_appl_ptr(substream, frames,
+				snd_pcm_capture_avail(runtime));
  __end:
 	snd_pcm_stream_unlock_irq(substream);
 	return ret;
@@ -2721,10 +2730,15 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
 			return err;
 	}
 	snd_pcm_stream_lock_irq(substream);
-	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
-		control->appl_ptr = sync_ptr.c.control.appl_ptr;
-	else
+	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
+		err = apply_appl_ptr(substream, sync_ptr.c.control.appl_ptr);
+		if (err < 0) {
+			snd_pcm_stream_unlock_irq(substream);
+			return err;
+		}
+	} else {
 		sync_ptr.c.control.appl_ptr = control->appl_ptr;
+	}
 	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
 		control->avail_min = sync_ptr.c.control.avail_min;
 	else
diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
index 00fc9241d266..684dc4ddef41 100644
--- a/sound/mips/hal2.c
+++ b/sound/mips/hal2.c
@@ -616,10 +616,9 @@ static int hal2_playback_ack(struct snd_pcm_substream *substream)
 	struct hal2_codec *dac = &hal2->dac;
 
 	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
-	snd_pcm_indirect_playback_transfer(substream,
-					   &dac->pcm_indirect,
-					   hal2_playback_transfer);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream,
+						  &dac->pcm_indirect,
+						  hal2_playback_transfer);
 }
 
 static int hal2_capture_open(struct snd_pcm_substream *substream)
@@ -707,10 +706,9 @@ static int hal2_capture_ack(struct snd_pcm_substream *substream)
 	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
 	struct hal2_codec *adc = &hal2->adc;
 
-	snd_pcm_indirect_capture_transfer(substream,
-					  &adc->pcm_indirect,
-					  hal2_capture_transfer);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream,
+						 &adc->pcm_indirect,
+						 hal2_capture_transfer);
 }
 
 static struct snd_pcm_ops hal2_playback_ops = {
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
index e4cf3187b4dd..429c0fbe3570 100644
--- a/sound/pci/cs46xx/cs46xx_lib.c
+++ b/sound/pci/cs46xx/cs46xx_lib.c
@@ -887,8 +887,8 @@ static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
-	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec,
+						  snd_cs46xx_pb_trans_copy);
 }
 
 static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -903,8 +903,8 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
 {
 	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec,
+						 snd_cs46xx_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t snd_cs46xx_playback_direct_pointer(struct snd_pcm_substream *substream)
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
index ef1cf530c929..bdda29f335f6 100644
--- a/sound/pci/emu10k1/emupcm.c
+++ b/sound/pci/emu10k1/emupcm.c
@@ -1632,8 +1632,8 @@ static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substr
 	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
 	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
 
-	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec,
+						  fx8010_pb_trans_copy);
 }
 
 static int snd_emu10k1_fx8010_playback_hw_params(struct snd_pcm_substream *substream,
diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
index 96d15db65dfd..f9b424056d0f 100644
--- a/sound/pci/rme32.c
+++ b/sound/pci/rme32.c
@@ -1157,9 +1157,8 @@ static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
 	if (rme32->running & (1 << SNDRV_PCM_STREAM_CAPTURE))
 		rec->hw_queue_size -= cprec->hw_ready;
 	spin_unlock(&rme32->lock);
-	snd_pcm_indirect_playback_transfer(substream, rec,
-					   snd_rme32_pb_trans_copy);
-	return 0;
+	return snd_pcm_indirect_playback_transfer(substream, rec,
+						  snd_rme32_pb_trans_copy);
 }
 
 static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
@@ -1174,9 +1173,8 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
 static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
 {
 	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
-	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
-					  snd_rme32_cp_trans_copy);
-	return 0;
+	return snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
+						 snd_rme32_cp_trans_copy);
 }
 
 static snd_pcm_uframes_t

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Vinod Koul]

On Thu, May 18, 2017 at 10:09:23AM +0200, Takashi Iwai wrote:
> On Thu, 18 May 2017 08:18:21 +0200,
> Subhransu S. Prusty wrote:
> > 
> > On Tue, May 16, 2017 at 01:06:57PM +0530, Subhransu S. Prusty wrote:
> > > On Tue, May 16, 2017 at 07:56:44AM +0200, Takashi Iwai wrote:
> > > > On Tue, 16 May 2017 03:01:57 +0200,
> > > > Subhransu S. Prusty wrote:
> > > > > 
> > > > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > 
> > > > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > > low-level driver/hardware pre-fetch data opportunistically.
> > > > > 
> > > > > The existing .ack callback is extended with new attribute argument, to
> > > > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > > process the application ptr update in the driver like in the skylake
> > > > > driver which can use this to inform position of appl pointer to host DMA
> > > > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > > submitted with a separate patch set.
> > > > > 
> > > > > In the ALSA core, this capability is independent from the NO_REWIND
> > > > > hardware flag. The low-level driver may however tie both options and only
> > > > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > > > limitations.
> > > > > 
> > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > 
> > > > It might be me who suggested the extension of the ack ops, but now
> > > > looking at the result, I reconsider whether it'd be a better choice if
> > > > we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> > > > rewrite the patch in that way for comparison?
> > > 
> > > Here is the version using update_appl_ptr.
> > 
> > Hi Takashi,
> > 
> > Did you get a chance to look at the update_appl_ptr changes?
> > Please let us know which one will be preferable, will submit the patches
> > accordingly.
> 
> Now I have a mixed feeling.  Using ack() is basically "the right
> thing".  The update of appl_ptr in forward/rewind and sync_ptr should
> be notified to ack() in general.  It's the purpose of ack(), after
> all.  In that sense, we may call ack() without any argument from any
> places.
> 
> The only problem is that the rewind is broken on some drivers, and
> calling ack() may lead to unexpected results.

Precisely the reason we went with an arg to make it opt-in and ensure
existing drivers don't break

> That is, we should look at these existing drivers and handle the
> rewind case (negative appl_ptr diff) appropriately -- or maybe we
> should add a flag to disallow the rewind on such drivers.
> After that, ack() can be called safely from all places that update
> appl_ptr.

Testing a large set of those drivers will be an issue :(

> ... this is one way.  Another way is to allow a quick hack and doubly
> call a new callback.

Sorry but how would invoking twice help here?

> 
> I prefer the former, but obviously it'll take longer.  So it depends
> on urgency.

We have been going back and forth on this for at least couple of years now,
so I would really love this to get merged before next window :)

-- 
~Vinod

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Pierre-Louis Bossart]

On 5/16/17 12:56 AM, Takashi Iwai wrote:
> On Tue, 16 May 2017 03:01:57 +0200,
> Subhransu S. Prusty wrote:
>>
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> When appl_ptr is updated let low-level driver know, e.g.  to let the
>> low-level driver/hardware pre-fetch data opportunistically.
>>
>> The existing .ack callback is extended with new attribute argument, to
>> support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
>> doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
>> process the application ptr update in the driver like in the skylake
>> driver which can use this to inform position of appl pointer to host DMA
>> controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
>> submitted with a separate patch set.
>>
>> In the ALSA core, this capability is independent from the NO_REWIND
>> hardware flag. The low-level driver may however tie both options and only
>> use the updated appl_ptr when rewinds are disabled due to hardware
>> limitations.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>
> It might be me who suggested the extension of the ack ops, but now
> looking at the result, I reconsider whether it'd be a better choice if
> we add another ops (e.g. update_appl_ptr()) instead.  Could you try to
> rewrite the patch in that way for comparison?

Yes indeed, we had initially a separate appl_ptr_update() to avoid 
touching legacy code using ack(). After the initial feedback we merged 
the two with a legacy flag, at the end of the day there is no difference 
in functionality so we'll let you pick one of the two solutions...

>
> Besides that, the flag name SND_PCM_ACK_LEGACY sounds too ambiguous to
> me...
>
>
> thanks,
>
> Takashi
>
>> ---
>>  include/sound/pcm-indirect.h  |  4 ++--
>>  include/sound/pcm.h           |  8 +++++++-
>>  sound/core/pcm_lib.c          |  6 ++++--
>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
>>  sound/mips/hal2.c             | 14 +++++++++++---
>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
>>  sound/pci/rme32.c             | 15 ++++++++++++---
>>  8 files changed, 79 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
>> index 1df7acaaa535..2f647ff970fb 100644
>> --- a/include/sound/pcm-indirect.h
>> +++ b/include/sound/pcm-indirect.h
>> @@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>>  	if (rec->sw_io >= rec->sw_buffer_size)
>>  		rec->sw_io -= rec->sw_buffer_size;
>>  	if (substream->ops->ack)
>> -		substream->ops->ack(substream);
>> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>>  	return bytes_to_frames(substream->runtime, rec->sw_io);
>>  }
>>
>> @@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>>  	if (rec->sw_io >= rec->sw_buffer_size)
>>  		rec->sw_io -= rec->sw_buffer_size;
>>  	if (substream->ops->ack)
>> -		substream->ops->ack(substream);
>> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>>  	return bytes_to_frames(substream->runtime, rec->sw_io);
>>  }
>>
>> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
>> index a2682c5f5b72..0151552342f9 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -63,6 +63,12 @@ struct snd_pcm_hardware {
>>  struct snd_pcm_audio_tstamp_config; /* definitions further down */
>>  struct snd_pcm_audio_tstamp_report;
>>
>> +/*
>> + * Attibute to distinguish the ack for legacy code and pointer update.
>> + */
>> +#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
>> +#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
>> +
>>  struct snd_pcm_ops {
>>  	int (*open)(struct snd_pcm_substream *substream);
>>  	int (*close)(struct snd_pcm_substream *substream);
>> @@ -86,7 +92,7 @@ struct snd_pcm_ops {
>>  	struct page *(*page)(struct snd_pcm_substream *substream,
>>  			     unsigned long offset);
>>  	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
>> -	int (*ack)(struct snd_pcm_substream *substream);
>> +	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
>>  };
>>
>>  /*
>> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
>> index 5088d4b8db22..b25af69a67da 100644
>> --- a/sound/core/pcm_lib.c
>> +++ b/sound/core/pcm_lib.c
>> @@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>>  			appl_ptr -= runtime->boundary;
>>  		runtime->control->appl_ptr = appl_ptr;
>>  		if (substream->ops->ack)
>> -			substream->ops->ack(substream);
>> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
>> +						SND_PCM_ACK_APP_PTR);
>>
>>  		offset += frames;
>>  		size -= frames;
>> @@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>>  			appl_ptr -= runtime->boundary;
>>  		runtime->control->appl_ptr = appl_ptr;
>>  		if (substream->ops->ack)
>> -			substream->ops->ack(substream);
>> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
>> +						SND_PCM_ACK_APP_PTR);
>>
>>  		offset += frames;
>>  		size -= frames;
>> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
>> index 35dd4ca93f84..c14cdbd9ff86 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>>  		appl_ptr += runtime->boundary;
>>  	runtime->control->appl_ptr = appl_ptr;
>>  	ret = frames;
>> +
>> +	if (substream->ops->ack)
>> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
>> +
>>   __end:
>>  	snd_pcm_stream_unlock_irq(substream);
>>  	return ret;
>> @@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>>  		appl_ptr += runtime->boundary;
>>  	runtime->control->appl_ptr = appl_ptr;
>>  	ret = frames;
>> +
>> +	if (substream->ops->ack)
>> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
>> +
>>   __end:
>>  	snd_pcm_stream_unlock_irq(substream);
>>  	return ret;
>> @@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>>  		appl_ptr -= runtime->boundary;
>>  	runtime->control->appl_ptr = appl_ptr;
>>  	ret = frames;
>> +
>> +	if (substream->ops->ack)
>> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
>> +
>>   __end:
>>  	snd_pcm_stream_unlock_irq(substream);
>>  	return ret;
>> @@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>>  		appl_ptr -= runtime->boundary;
>>  	runtime->control->appl_ptr = appl_ptr;
>>  	ret = frames;
>> +
>> +	if (substream->ops->ack)
>> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
>> +
>>   __end:
>>  	snd_pcm_stream_unlock_irq(substream);
>>  	return ret;
>> @@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>>  			return err;
>>  	}
>>  	snd_pcm_stream_lock_irq(substream);
>> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
>> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
>> +		/* boundary wrap-around is assumed to be handled in userspace */
>>  		control->appl_ptr = sync_ptr.c.control.appl_ptr;
>> +
>> +		/* let low-level driver know about appl_ptr change */
>> +		if (substream->ops->ack)
>> +			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
>> +	}
>>  	else
>>  		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>>  	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
>> diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
>> index 00fc9241d266..3740381b188a 100644
>> --- a/sound/mips/hal2.c
>> +++ b/sound/mips/hal2.c
>> @@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
>>  	case SNDRV_PCM_TRIGGER_START:
>>  		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
>>  		hal2->dac.pcm_indirect.hw_data = 0;
>> -		substream->ops->ack(substream);
>> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>>  		hal2_start_dac(hal2);
>>  		break;
>>  	case SNDRV_PCM_TRIGGER_STOP:
>> @@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
>>
>>  }
>>
>> -static int hal2_playback_ack(struct snd_pcm_substream *substream)
>> +static int hal2_playback_ack(struct snd_pcm_substream *substream,
>> +			     unsigned int ack_attr)
>>  {
>>  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>>  	struct hal2_codec *dac = &hal2->dac;
>>
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
>>  	snd_pcm_indirect_playback_transfer(substream,
>>  					   &dac->pcm_indirect,
>> @@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
>>  	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
>>  }
>>
>> -static int hal2_capture_ack(struct snd_pcm_substream *substream)
>> +static int hal2_capture_ack(struct snd_pcm_substream *substream,
>> +			    unsigned int ack_attr)
>>  {
>>  	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>>  	struct hal2_codec *adc = &hal2->adc;
>>
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	snd_pcm_indirect_capture_transfer(substream,
>>  					  &adc->pcm_indirect,
>>  					  hal2_capture_transfer);
>> diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
>> index e4cf3187b4dd..877edda61e45 100644
>> --- a/sound/pci/cs46xx/cs46xx_lib.c
>> +++ b/sound/pci/cs46xx/cs46xx_lib.c
>> @@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
>>  	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
>>  }
>>
>> -static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
>> +static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
>> +					unsigned int ack_attr)
>>  {
>>  	struct snd_pcm_runtime *runtime = substream->runtime;
>>  	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
>> +
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
>>  	return 0;
>>  }
>> @@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
>>  	       chip->capt.hw_buf.area + rec->hw_data, bytes);
>>  }
>>
>> -static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
>> +static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
>> +					       unsigned int ack_attr)
>>  {
>>  	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
>> +
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
>>  	return 0;
>>  }
>> @@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
>>  			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
>>
>>  		if (substream->runtime->periods != CS46XX_FRAGS)
>> -			snd_cs46xx_playback_transfer(substream);
>> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>>  #else
>>  		spin_lock(&chip->reg_lock);
>>  		if (substream->runtime->periods != CS46XX_FRAGS)
>> -			snd_cs46xx_playback_transfer(substream);
>> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>>  		{ unsigned int tmp;
>>  		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
>>  		tmp &= 0x0000ffff;
>> diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
>> index ef1cf530c929..74c78df6e5a8 100644
>> --- a/sound/pci/emu10k1/emupcm.c
>> +++ b/sound/pci/emu10k1/emupcm.c
>> @@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
>>  	pcm->tram_shift = tram_shift;
>>  }
>>
>> -static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
>> +static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
>> +						unsigned int ack_attr)
>>  {
>>  	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
>>  	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
>>
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
>>  	return 0;
>>  }
>> @@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
>>  		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
>>  		if (result < 0)
>>  			goto __err;
>> -		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
>> +		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
>>  		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
>>  		break;
>>  	case SNDRV_PCM_TRIGGER_STOP:
>> diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
>> index 96d15db65dfd..3fd8de5bab26 100644
>> --- a/sound/pci/rme32.c
>> +++ b/sound/pci/rme32.c
>> @@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
>>  	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
>>  		snd_pcm_group_for_each_entry(s, substream) {
>>  			if (s == rme32->playback_substream) {
>> -				s->ops->ack(s);
>> +				s->ops->ack(s, SND_PCM_ACK_LEGACY);
>>  				break;
>>  			}
>>  		}
>> @@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
>>  		    substream->runtime->dma_area + rec->sw_data, bytes);
>>  }
>>
>> -static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
>> +static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
>> +				     unsigned int ack_attr)
>>  {
>>  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
>>  	struct snd_pcm_indirect *rec, *cprec;
>>
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	rec = &rme32->playback_pcm;
>>  	cprec = &rme32->capture_pcm;
>>  	spin_lock(&rme32->lock);
>> @@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
>>  		      bytes);
>>  }
>>
>> -static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
>> +static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
>> +					unsigned int ack_attr)
>>  {
>>  	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
>> +
>> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
>> +		return 0;
>> +
>>  	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
>>  					  snd_rme32_cp_trans_copy);
>>  	return 0;
>> --
>> 1.9.1
>>

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Sakamoto]

Hi,

On May 16 2017 10:01, Subhransu S. Prusty wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> When appl_ptr is updated let low-level driver know, e.g.  to let the
> low-level driver/hardware pre-fetch data opportunistically.
> 
> The existing .ack callback is extended with new attribute argument, to
> support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> process the application ptr update in the driver like in the skylake
> driver which can use this to inform position of appl pointer to host DMA
> controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> submitted with a separate patch set.
> 
> In the ALSA core, this capability is independent from the NO_REWIND
> hardware flag. The low-level driver may however tie both options and only
> use the updated appl_ptr when rewinds are disabled due to hardware
> limitations.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>   include/sound/pcm-indirect.h  |  4 ++--
>   include/sound/pcm.h           |  8 +++++++-
>   sound/core/pcm_lib.c          |  6 ++++--
>   sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
>   sound/mips/hal2.c             | 14 +++++++++++---
>   sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
>   sound/pci/emu10k1/emupcm.c    |  8 ++++++--
>   sound/pci/rme32.c             | 15 ++++++++++++---
>   8 files changed, 79 insertions(+), 18 deletions(-)

I think it better to take care of 
'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as well. 
This is a driver for sound functionality of Raspberry PI 1/zero and some 
people may still get influences from this work.

$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
...

> diff --git a/include/sound/pcm-indirect.h b/include/sound/pcm-indirect.h
> index 1df7acaaa535..2f647ff970fb 100644
> --- a/include/sound/pcm-indirect.h
> +++ b/include/sound/pcm-indirect.h
> @@ -101,7 +101,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>   	if (rec->sw_io >= rec->sw_buffer_size)
>   		rec->sw_io -= rec->sw_buffer_size;
>   	if (substream->ops->ack)
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>   	return bytes_to_frames(substream->runtime, rec->sw_io);
>   }
>   
> @@ -170,7 +170,7 @@ typedef void (*snd_pcm_indirect_copy_t)(struct snd_pcm_substream *substream,
>   	if (rec->sw_io >= rec->sw_buffer_size)
>   		rec->sw_io -= rec->sw_buffer_size;
>   	if (substream->ops->ack)
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>   	return bytes_to_frames(substream->runtime, rec->sw_io);
>   }
>   
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index a2682c5f5b72..0151552342f9 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -63,6 +63,12 @@ struct snd_pcm_hardware {
>   struct snd_pcm_audio_tstamp_config; /* definitions further down */
>   struct snd_pcm_audio_tstamp_report;
>   
> +/*
> + * Attibute to distinguish the ack for legacy code and pointer update.
> + */
> +#define SND_PCM_ACK_LEGACY		BIT(0) /* Legacy callback */
> +#define SND_PCM_ACK_APP_PTR		BIT(1) /* Update pointer callback */
> +
>   struct snd_pcm_ops {
>   	int (*open)(struct snd_pcm_substream *substream);
>   	int (*close)(struct snd_pcm_substream *substream);
> @@ -86,7 +92,7 @@ struct snd_pcm_ops {
>   	struct page *(*page)(struct snd_pcm_substream *substream,
>   			     unsigned long offset);
>   	int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma);
> -	int (*ack)(struct snd_pcm_substream *substream);
> +	int (*ack)(struct snd_pcm_substream *substream, unsigned int ack_attr);
>   };
>   
>   /*
> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index 5088d4b8db22..b25af69a67da 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -2089,7 +2089,8 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
>   			appl_ptr -= runtime->boundary;
>   		runtime->control->appl_ptr = appl_ptr;
>   		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> +						SND_PCM_ACK_APP_PTR);
>   
>   		offset += frames;
>   		size -= frames;
> @@ -2321,7 +2322,8 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
>   			appl_ptr -= runtime->boundary;
>   		runtime->control->appl_ptr = appl_ptr;
>   		if (substream->ops->ack)
> -			substream->ops->ack(substream);
> +			substream->ops->ack(substream, SND_PCM_ACK_LEGACY |
> +						SND_PCM_ACK_APP_PTR);
>   
>   		offset += frames;
>   		size -= frames;
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 35dd4ca93f84..c14cdbd9ff86 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -2476,6 +2476,10 @@ static snd_pcm_sframes_t snd_pcm_playback_rewind(struct snd_pcm_substream *subst
>   		appl_ptr += runtime->boundary;
>   	runtime->control->appl_ptr = appl_ptr;
>   	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>    __end:
>   	snd_pcm_stream_unlock_irq(substream);
>   	return ret;
> @@ -2527,6 +2531,10 @@ static snd_pcm_sframes_t snd_pcm_capture_rewind(struct snd_pcm_substream *substr
>   		appl_ptr += runtime->boundary;
>   	runtime->control->appl_ptr = appl_ptr;
>   	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>    __end:
>   	snd_pcm_stream_unlock_irq(substream);
>   	return ret;
> @@ -2576,6 +2584,10 @@ static snd_pcm_sframes_t snd_pcm_playback_forward(struct snd_pcm_substream *subs
>   		appl_ptr -= runtime->boundary;
>   	runtime->control->appl_ptr = appl_ptr;
>   	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>    __end:
>   	snd_pcm_stream_unlock_irq(substream);
>   	return ret;
> @@ -2625,6 +2637,10 @@ static snd_pcm_sframes_t snd_pcm_capture_forward(struct snd_pcm_substream *subst
>   		appl_ptr -= runtime->boundary;
>   	runtime->control->appl_ptr = appl_ptr;
>   	ret = frames;
> +
> +	if (substream->ops->ack)
> +		substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +
>    __end:
>   	snd_pcm_stream_unlock_irq(substream);
>   	return ret;
> @@ -2726,8 +2742,14 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
>   			return err;
>   	}
>   	snd_pcm_stream_lock_irq(substream);
> -	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL))
> +	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) {
> +		/* boundary wrap-around is assumed to be handled in userspace */
>   		control->appl_ptr = sync_ptr.c.control.appl_ptr;
> +
> +		/* let low-level driver know about appl_ptr change */
> +		if (substream->ops->ack)
> +			substream->ops->ack(substream, SND_PCM_ACK_APP_PTR);
> +	}
>   	else
>   		sync_ptr.c.control.appl_ptr = control->appl_ptr;
>   	if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN))
> diff --git a/sound/mips/hal2.c b/sound/mips/hal2.c
> index 00fc9241d266..3740381b188a 100644
> --- a/sound/mips/hal2.c
> +++ b/sound/mips/hal2.c
> @@ -577,7 +577,7 @@ static int hal2_playback_trigger(struct snd_pcm_substream *substream, int cmd)
>   	case SNDRV_PCM_TRIGGER_START:
>   		hal2->dac.pcm_indirect.hw_io = hal2->dac.buffer_dma;
>   		hal2->dac.pcm_indirect.hw_data = 0;
> -		substream->ops->ack(substream);
> +		substream->ops->ack(substream, SND_PCM_ACK_LEGACY);
>   		hal2_start_dac(hal2);
>   		break;
>   	case SNDRV_PCM_TRIGGER_STOP:
> @@ -610,11 +610,15 @@ static void hal2_playback_transfer(struct snd_pcm_substream *substream,
>   
>   }
>   
> -static int hal2_playback_ack(struct snd_pcm_substream *substream)
> +static int hal2_playback_ack(struct snd_pcm_substream *substream,
> +			     unsigned int ack_attr)
>   {
>   	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>   	struct hal2_codec *dac = &hal2->dac;
>   
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	dac->pcm_indirect.hw_queue_size = H2_BUF_SIZE / 2;
>   	snd_pcm_indirect_playback_transfer(substream,
>   					   &dac->pcm_indirect,
> @@ -702,11 +706,15 @@ static void hal2_capture_transfer(struct snd_pcm_substream *substream,
>   	memcpy(substream->runtime->dma_area + rec->sw_data, buf, bytes);
>   }
>   
> -static int hal2_capture_ack(struct snd_pcm_substream *substream)
> +static int hal2_capture_ack(struct snd_pcm_substream *substream,
> +			    unsigned int ack_attr)
>   {
>   	struct snd_hal2 *hal2 = snd_pcm_substream_chip(substream);
>   	struct hal2_codec *adc = &hal2->adc;
>   
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	snd_pcm_indirect_capture_transfer(substream,
>   					  &adc->pcm_indirect,
>   					  hal2_capture_transfer);
> diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c
> index e4cf3187b4dd..877edda61e45 100644
> --- a/sound/pci/cs46xx/cs46xx_lib.c
> +++ b/sound/pci/cs46xx/cs46xx_lib.c
> @@ -883,10 +883,15 @@ static void snd_cs46xx_pb_trans_copy(struct snd_pcm_substream *substream,
>   	memcpy(cpcm->hw_buf.area + rec->hw_data, runtime->dma_area + rec->sw_data, bytes);
>   }
>   
> -static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream)
> +static int snd_cs46xx_playback_transfer(struct snd_pcm_substream *substream,
> +					unsigned int ack_attr)
>   {
>   	struct snd_pcm_runtime *runtime = substream->runtime;
>   	struct snd_cs46xx_pcm * cpcm = runtime->private_data;
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	snd_pcm_indirect_playback_transfer(substream, &cpcm->pcm_rec, snd_cs46xx_pb_trans_copy);
>   	return 0;
>   }
> @@ -900,9 +905,14 @@ static void snd_cs46xx_cp_trans_copy(struct snd_pcm_substream *substream,
>   	       chip->capt.hw_buf.area + rec->hw_data, bytes);
>   }
>   
> -static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream)
> +static int snd_cs46xx_capture_transfer(struct snd_pcm_substream *substream,
> +					       unsigned int ack_attr)
>   {
>   	struct snd_cs46xx *chip = snd_pcm_substream_chip(substream);
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	snd_pcm_indirect_capture_transfer(substream, &chip->capt.pcm_rec, snd_cs46xx_cp_trans_copy);
>   	return 0;
>   }
> @@ -981,11 +991,11 @@ static int snd_cs46xx_playback_trigger(struct snd_pcm_substream *substream,
>   			cs46xx_dsp_pcm_link(chip,cpcm->pcm_channel);
>   
>   		if (substream->runtime->periods != CS46XX_FRAGS)
> -			snd_cs46xx_playback_transfer(substream);
> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>   #else
>   		spin_lock(&chip->reg_lock);
>   		if (substream->runtime->periods != CS46XX_FRAGS)
> -			snd_cs46xx_playback_transfer(substream);
> +			snd_cs46xx_playback_transfer(substream, SND_PCM_ACK_LEGACY);
>   		{ unsigned int tmp;
>   		tmp = snd_cs46xx_peek(chip, BA1_PCTL);
>   		tmp &= 0x0000ffff;
> diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c
> index ef1cf530c929..74c78df6e5a8 100644
> --- a/sound/pci/emu10k1/emupcm.c
> +++ b/sound/pci/emu10k1/emupcm.c
> @@ -1627,11 +1627,15 @@ static void fx8010_pb_trans_copy(struct snd_pcm_substream *substream,
>   	pcm->tram_shift = tram_shift;
>   }
>   
> -static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream)
> +static int snd_emu10k1_fx8010_playback_transfer(struct snd_pcm_substream *substream,
> +						unsigned int ack_attr)
>   {
>   	struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream);
>   	struct snd_emu10k1_fx8010_pcm *pcm = &emu->fx8010.pcm[substream->number];
>   
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	snd_pcm_indirect_playback_transfer(substream, &pcm->pcm_rec, fx8010_pb_trans_copy);
>   	return 0;
>   }
> @@ -1710,7 +1714,7 @@ static int snd_emu10k1_fx8010_playback_trigger(struct snd_pcm_substream *substre
>   		result = snd_emu10k1_fx8010_register_irq_handler(emu, snd_emu10k1_fx8010_playback_irq, pcm->gpr_running, substream, &pcm->irq);
>   		if (result < 0)
>   			goto __err;
> -		snd_emu10k1_fx8010_playback_transfer(substream);	/* roll the ball */
> +		snd_emu10k1_fx8010_playback_transfer(substream, SND_PCM_ACK_LEGACY);	/* roll the ball */
>   		snd_emu10k1_ptr_write(emu, emu->gpr_base + pcm->gpr_trigger, 0, 1);
>   		break;
>   	case SNDRV_PCM_TRIGGER_STOP:
> diff --git a/sound/pci/rme32.c b/sound/pci/rme32.c
> index 96d15db65dfd..3fd8de5bab26 100644
> --- a/sound/pci/rme32.c
> +++ b/sound/pci/rme32.c
> @@ -1092,7 +1092,7 @@ static int snd_rme32_capture_prepare(struct snd_pcm_substream *substream)
>   	if (cmd == SNDRV_PCM_TRIGGER_START && rme32->fullduplex_mode) {
>   		snd_pcm_group_for_each_entry(s, substream) {
>   			if (s == rme32->playback_substream) {
> -				s->ops->ack(s);
> +				s->ops->ack(s, SND_PCM_ACK_LEGACY);
>   				break;
>   			}
>   		}
> @@ -1145,11 +1145,15 @@ static void snd_rme32_pb_trans_copy(struct snd_pcm_substream *substream,
>   		    substream->runtime->dma_area + rec->sw_data, bytes);
>   }
>   
> -static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream)
> +static int snd_rme32_playback_fd_ack(struct snd_pcm_substream *substream,
> +				     unsigned int ack_attr)
>   {
>   	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
>   	struct snd_pcm_indirect *rec, *cprec;
>   
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	rec = &rme32->playback_pcm;
>   	cprec = &rme32->capture_pcm;
>   	spin_lock(&rme32->lock);
> @@ -1171,9 +1175,14 @@ static void snd_rme32_cp_trans_copy(struct snd_pcm_substream *substream,
>   		      bytes);
>   }
>   
> -static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream)
> +static int snd_rme32_capture_fd_ack(struct snd_pcm_substream *substream,
> +					unsigned int ack_attr)
>   {
>   	struct rme32 *rme32 = snd_pcm_substream_chip(substream);
> +
> +	if (!(ack_attr & SND_PCM_ACK_LEGACY))
> +		return 0;
> +
>   	snd_pcm_indirect_capture_transfer(substream, &rme32->capture_pcm,
>   					  snd_rme32_cp_trans_copy);
>   	return 0;

Regards

Takashi Sakamoto

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Fri, 19 May 2017 05:57:05 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > low-level driver/hardware pre-fetch data opportunistically.
> >
> > The existing .ack callback is extended with new attribute argument, to
> > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > process the application ptr update in the driver like in the skylake
> > driver which can use this to inform position of appl pointer to host DMA
> > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > submitted with a separate patch set.
> >
> > In the ALSA core, this capability is independent from the NO_REWIND
> > hardware flag. The low-level driver may however tie both options and only
> > use the updated appl_ptr when rewinds are disabled due to hardware
> > limitations.
> >
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > ---
> >   include/sound/pcm-indirect.h  |  4 ++--
> >   include/sound/pcm.h           |  8 +++++++-
> >   sound/core/pcm_lib.c          |  6 ++++--
> >   sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> >   sound/mips/hal2.c             | 14 +++++++++++---
> >   sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> >   sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> >   sound/pci/rme32.c             | 15 ++++++++++++---
> >   8 files changed, 79 insertions(+), 18 deletions(-)
> 
> I think it better to take care of
> 'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> well. This is a driver for sound functionality of Raspberry PI 1/zero
> and some people may still get influences from this work.
> 
> $ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> ...

Yep, we need to cover all codes if we really change the PCM ops.


Takashi

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Pierre-Louis Bossart]

On 5/19/17 1:27 AM, Takashi Iwai wrote:
> On Fri, 19 May 2017 05:57:05 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On May 16 2017 10:01, Subhransu S. Prusty wrote:
>>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>>
>>> When appl_ptr is updated let low-level driver know, e.g.  to let the
>>> low-level driver/hardware pre-fetch data opportunistically.
>>>
>>> The existing .ack callback is extended with new attribute argument, to
>>> support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
>>> doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
>>> process the application ptr update in the driver like in the skylake
>>> driver which can use this to inform position of appl pointer to host DMA
>>> controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
>>> submitted with a separate patch set.
>>>
>>> In the ALSA core, this capability is independent from the NO_REWIND
>>> hardware flag. The low-level driver may however tie both options and only
>>> use the updated appl_ptr when rewinds are disabled due to hardware
>>> limitations.
>>>
>>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>> ---
>>>   include/sound/pcm-indirect.h  |  4 ++--
>>>   include/sound/pcm.h           |  8 +++++++-
>>>   sound/core/pcm_lib.c          |  6 ++++--
>>>   sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
>>>   sound/mips/hal2.c             | 14 +++++++++++---
>>>   sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
>>>   sound/pci/emu10k1/emupcm.c    |  8 ++++++--
>>>   sound/pci/rme32.c             | 15 ++++++++++++---
>>>   8 files changed, 79 insertions(+), 18 deletions(-)
>>
>> I think it better to take care of
>> 'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
>> well. This is a driver for sound functionality of Raspberry PI 1/zero
>> and some people may still get influences from this work.
>>
>> $ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
>> v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
>> ...
>
> Yep, we need to cover all codes if we really change the PCM ops.

The reason precisely why I preferred to avoid mucking with the existing 
.ack(). we have a risk of introducing problems for legacy and staging, 
not to mention out-of-tree.

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

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Vinod Koul]

On Fri, May 19, 2017 at 10:01:04AM -0500, Pierre-Louis Bossart wrote:
> On 5/19/17 1:27 AM, Takashi Iwai wrote:
> >On Fri, 19 May 2017 05:57:05 +0200,
> >Takashi Sakamoto wrote:
> >>
> >>Hi,
> >>
> >>On May 16 2017 10:01, Subhransu S. Prusty wrote:
> >>>From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>>
> >>>When appl_ptr is updated let low-level driver know, e.g.  to let the
> >>>low-level driver/hardware pre-fetch data opportunistically.
> >>>
> >>>The existing .ack callback is extended with new attribute argument, to
> >>>support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> >>>doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> >>>process the application ptr update in the driver like in the skylake
> >>>driver which can use this to inform position of appl pointer to host DMA
> >>>controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> >>>submitted with a separate patch set.
> >>>
> >>>In the ALSA core, this capability is independent from the NO_REWIND
> >>>hardware flag. The low-level driver may however tie both options and only
> >>>use the updated appl_ptr when rewinds are disabled due to hardware
> >>>limitations.
> >>>
> >>>Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >>>Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> >>>Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> >>>---
> >>>  include/sound/pcm-indirect.h  |  4 ++--
> >>>  include/sound/pcm.h           |  8 +++++++-
> >>>  sound/core/pcm_lib.c          |  6 ++++--
> >>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> >>>  sound/mips/hal2.c             | 14 +++++++++++---
> >>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> >>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> >>>  sound/pci/rme32.c             | 15 ++++++++++++---
> >>>  8 files changed, 79 insertions(+), 18 deletions(-)
> >>
> >>I think it better to take care of
> >>'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> >>well. This is a driver for sound functionality of Raspberry PI 1/zero
> >>and some people may still get influences from this work.
> >>
> >>$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> >>v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> >>...
> >
> >Yep, we need to cover all codes if we really change the PCM ops.
> 
> The reason precisely why I preferred to avoid mucking with the
> existing .ack(). we have a risk of introducing problems for legacy
> and staging, not to mention out-of-tree.

Patching them should be fine, and I wont mind breaking out-of tree ones,
isn't that a good thing :)

-- 
~Vinod

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Mon, 22 May 2017 07:22:19 +0200,
Vinod Koul wrote:
> 
> On Fri, May 19, 2017 at 10:01:04AM -0500, Pierre-Louis Bossart wrote:
> > On 5/19/17 1:27 AM, Takashi Iwai wrote:
> > >On Fri, 19 May 2017 05:57:05 +0200,
> > >Takashi Sakamoto wrote:
> > >>
> > >>Hi,
> > >>
> > >>On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > >>>From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >>>
> > >>>When appl_ptr is updated let low-level driver know, e.g.  to let the
> > >>>low-level driver/hardware pre-fetch data opportunistically.
> > >>>
> > >>>The existing .ack callback is extended with new attribute argument, to
> > >>>support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > >>>doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > >>>process the application ptr update in the driver like in the skylake
> > >>>driver which can use this to inform position of appl pointer to host DMA
> > >>>controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > >>>submitted with a separate patch set.
> > >>>
> > >>>In the ALSA core, this capability is independent from the NO_REWIND
> > >>>hardware flag. The low-level driver may however tie both options and only
> > >>>use the updated appl_ptr when rewinds are disabled due to hardware
> > >>>limitations.
> > >>>
> > >>>Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >>>Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > >>>Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > >>>---
> > >>>  include/sound/pcm-indirect.h  |  4 ++--
> > >>>  include/sound/pcm.h           |  8 +++++++-
> > >>>  sound/core/pcm_lib.c          |  6 ++++--
> > >>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > >>>  sound/mips/hal2.c             | 14 +++++++++++---
> > >>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > >>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > >>>  sound/pci/rme32.c             | 15 ++++++++++++---
> > >>>  8 files changed, 79 insertions(+), 18 deletions(-)
> > >>
> > >>I think it better to take care of
> > >>'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> > >>well. This is a driver for sound functionality of Raspberry PI 1/zero
> > >>and some people may still get influences from this work.
> > >>
> > >>$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> > >>v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> > >>...
> > >
> > >Yep, we need to cover all codes if we really change the PCM ops.
> > 
> > The reason precisely why I preferred to avoid mucking with the
> > existing .ack(). we have a risk of introducing problems for legacy
> > and staging, not to mention out-of-tree.
> 
> Patching them should be fine, and I wont mind breaking out-of tree ones,
> isn't that a good thing :)

Heh, that's a fun as always :)

Actually it's a difficult judgment when to change the API and when
not.  This case is also in the gray zone.

If it were only additions of some new features, I'd think of avoiding
to change the existing API.  But, when it eventually fixes the bugs in
the existing drivers, we should not be too afraid of changing it.


Takashi

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Vinod Koul]

On Mon, May 22, 2017 at 09:16:34AM +0200, Takashi Iwai wrote:
> On Mon, 22 May 2017 07:22:19 +0200,
> Vinod Koul wrote:
> > 
> > On Fri, May 19, 2017 at 10:01:04AM -0500, Pierre-Louis Bossart wrote:
> > > On 5/19/17 1:27 AM, Takashi Iwai wrote:
> > > >On Fri, 19 May 2017 05:57:05 +0200,
> > > >Takashi Sakamoto wrote:
> > > >>
> > > >>Hi,
> > > >>
> > > >>On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > > >>>From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > >>>
> > > >>>When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > >>>low-level driver/hardware pre-fetch data opportunistically.
> > > >>>
> > > >>>The existing .ack callback is extended with new attribute argument, to
> > > >>>support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > >>>doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > >>>process the application ptr update in the driver like in the skylake
> > > >>>driver which can use this to inform position of appl pointer to host DMA
> > > >>>controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > >>>submitted with a separate patch set.
> > > >>>
> > > >>>In the ALSA core, this capability is independent from the NO_REWIND
> > > >>>hardware flag. The low-level driver may however tie both options and only
> > > >>>use the updated appl_ptr when rewinds are disabled due to hardware
> > > >>>limitations.
> > > >>>
> > > >>>Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > >>>Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > >>>Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > >>>---
> > > >>>  include/sound/pcm-indirect.h  |  4 ++--
> > > >>>  include/sound/pcm.h           |  8 +++++++-
> > > >>>  sound/core/pcm_lib.c          |  6 ++++--
> > > >>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > > >>>  sound/mips/hal2.c             | 14 +++++++++++---
> > > >>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > > >>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > > >>>  sound/pci/rme32.c             | 15 ++++++++++++---
> > > >>>  8 files changed, 79 insertions(+), 18 deletions(-)
> > > >>
> > > >>I think it better to take care of
> > > >>'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> > > >>well. This is a driver for sound functionality of Raspberry PI 1/zero
> > > >>and some people may still get influences from this work.
> > > >>
> > > >>$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> > > >>v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> > > >>...
> > > >
> > > >Yep, we need to cover all codes if we really change the PCM ops.
> > > 
> > > The reason precisely why I preferred to avoid mucking with the
> > > existing .ack(). we have a risk of introducing problems for legacy
> > > and staging, not to mention out-of-tree.
> > 
> > Patching them should be fine, and I wont mind breaking out-of tree ones,
> > isn't that a good thing :)
> 
> Heh, that's a fun as always :)
> 
> Actually it's a difficult judgment when to change the API and when
> not.  This case is also in the gray zone.
> 
> If it were only additions of some new features, I'd think of avoiding
> to change the existing API.  But, when it eventually fixes the bugs in
> the existing drivers, we should not be too afraid of changing it.

Then this case falls in the case where we are not changing API, right. If
you agree we can post the other approach.

-- 
~Vinod

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Takashi Iwai]

On Fri, 26 May 2017 09:42:43 +0200,
Vinod Koul wrote:
> 
> On Mon, May 22, 2017 at 09:16:34AM +0200, Takashi Iwai wrote:
> > On Mon, 22 May 2017 07:22:19 +0200,
> > Vinod Koul wrote:
> > > 
> > > On Fri, May 19, 2017 at 10:01:04AM -0500, Pierre-Louis Bossart wrote:
> > > > On 5/19/17 1:27 AM, Takashi Iwai wrote:
> > > > >On Fri, 19 May 2017 05:57:05 +0200,
> > > > >Takashi Sakamoto wrote:
> > > > >>
> > > > >>Hi,
> > > > >>
> > > > >>On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > > > >>>From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > >>>
> > > > >>>When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > >>>low-level driver/hardware pre-fetch data opportunistically.
> > > > >>>
> > > > >>>The existing .ack callback is extended with new attribute argument, to
> > > > >>>support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > >>>doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > >>>process the application ptr update in the driver like in the skylake
> > > > >>>driver which can use this to inform position of appl pointer to host DMA
> > > > >>>controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > >>>submitted with a separate patch set.
> > > > >>>
> > > > >>>In the ALSA core, this capability is independent from the NO_REWIND
> > > > >>>hardware flag. The low-level driver may however tie both options and only
> > > > >>>use the updated appl_ptr when rewinds are disabled due to hardware
> > > > >>>limitations.
> > > > >>>
> > > > >>>Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > >>>Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > >>>Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > >>>---
> > > > >>>  include/sound/pcm-indirect.h  |  4 ++--
> > > > >>>  include/sound/pcm.h           |  8 +++++++-
> > > > >>>  sound/core/pcm_lib.c          |  6 ++++--
> > > > >>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > > > >>>  sound/mips/hal2.c             | 14 +++++++++++---
> > > > >>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > > > >>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > > > >>>  sound/pci/rme32.c             | 15 ++++++++++++---
> > > > >>>  8 files changed, 79 insertions(+), 18 deletions(-)
> > > > >>
> > > > >>I think it better to take care of
> > > > >>'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> > > > >>well. This is a driver for sound functionality of Raspberry PI 1/zero
> > > > >>and some people may still get influences from this work.
> > > > >>
> > > > >>$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> > > > >>v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> > > > >>...
> > > > >
> > > > >Yep, we need to cover all codes if we really change the PCM ops.
> > > > 
> > > > The reason precisely why I preferred to avoid mucking with the
> > > > existing .ack(). we have a risk of introducing problems for legacy
> > > > and staging, not to mention out-of-tree.
> > > 
> > > Patching them should be fine, and I wont mind breaking out-of tree ones,
> > > isn't that a good thing :)
> > 
> > Heh, that's a fun as always :)
> > 
> > Actually it's a difficult judgment when to change the API and when
> > not.  This case is also in the gray zone.
> > 
> > If it were only additions of some new features, I'd think of avoiding
> > to change the existing API.  But, when it eventually fixes the bugs in
> > the existing drivers, we should not be too afraid of changing it.
> 
> Then this case falls in the case where we are not changing API, right. If
> you agree we can post the other approach.

Actually the changes for ack have been merged yesterday; the ack gets
called whenever appl_ptr changes.  This *is* the right behavior, after
all.


thanks,

Takashi

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Vinod Koul]

On Fri, May 26, 2017 at 09:47:32AM +0200, Takashi Iwai wrote:
> On Fri, 26 May 2017 09:42:43 +0200,
> Vinod Koul wrote:
> > 
> > On Mon, May 22, 2017 at 09:16:34AM +0200, Takashi Iwai wrote:
> > > On Mon, 22 May 2017 07:22:19 +0200,
> > > Vinod Koul wrote:
> > > > 
> > > > On Fri, May 19, 2017 at 10:01:04AM -0500, Pierre-Louis Bossart wrote:
> > > > > On 5/19/17 1:27 AM, Takashi Iwai wrote:
> > > > > >On Fri, 19 May 2017 05:57:05 +0200,
> > > > > >Takashi Sakamoto wrote:
> > > > > >>
> > > > > >>Hi,
> > > > > >>
> > > > > >>On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > > > > >>>From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > >>>
> > > > > >>>When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > > > >>>low-level driver/hardware pre-fetch data opportunistically.
> > > > > >>>
> > > > > >>>The existing .ack callback is extended with new attribute argument, to
> > > > > >>>support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > > > >>>doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > > > >>>process the application ptr update in the driver like in the skylake
> > > > > >>>driver which can use this to inform position of appl pointer to host DMA
> > > > > >>>controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > > > >>>submitted with a separate patch set.
> > > > > >>>
> > > > > >>>In the ALSA core, this capability is independent from the NO_REWIND
> > > > > >>>hardware flag. The low-level driver may however tie both options and only
> > > > > >>>use the updated appl_ptr when rewinds are disabled due to hardware
> > > > > >>>limitations.
> > > > > >>>
> > > > > >>>Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > > > >>>Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > > > >>>Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > > > >>>---
> > > > > >>>  include/sound/pcm-indirect.h  |  4 ++--
> > > > > >>>  include/sound/pcm.h           |  8 +++++++-
> > > > > >>>  sound/core/pcm_lib.c          |  6 ++++--
> > > > > >>>  sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > > > > >>>  sound/mips/hal2.c             | 14 +++++++++++---
> > > > > >>>  sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > > > > >>>  sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > > > > >>>  sound/pci/rme32.c             | 15 ++++++++++++---
> > > > > >>>  8 files changed, 79 insertions(+), 18 deletions(-)
> > > > > >>
> > > > > >>I think it better to take care of
> > > > > >>'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> > > > > >>well. This is a driver for sound functionality of Raspberry PI 1/zero
> > > > > >>and some people may still get influences from this work.
> > > > > >>
> > > > > >>$ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> > > > > >>v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> > > > > >>...
> > > > > >
> > > > > >Yep, we need to cover all codes if we really change the PCM ops.
> > > > > 
> > > > > The reason precisely why I preferred to avoid mucking with the
> > > > > existing .ack(). we have a risk of introducing problems for legacy
> > > > > and staging, not to mention out-of-tree.
> > > > 
> > > > Patching them should be fine, and I wont mind breaking out-of tree ones,
> > > > isn't that a good thing :)
> > > 
> > > Heh, that's a fun as always :)
> > > 
> > > Actually it's a difficult judgment when to change the API and when
> > > not.  This case is also in the gray zone.
> > > 
> > > If it were only additions of some new features, I'd think of avoiding
> > > to change the existing API.  But, when it eventually fixes the bugs in
> > > the existing drivers, we should not be too afraid of changing it.
> > 
> > Then this case falls in the case where we are not changing API, right. If
> > you agree we can post the other approach.
> 
> Actually the changes for ack have been merged yesterday; the ack gets
> called whenever appl_ptr changes.  This *is* the right behavior, after
> all.


Thats right, so we don't need additonal arg anymore, thanks for pointing out.
I had missed the patches.

Will respin this on top of those changes

-- 
~Vinod

Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr

[Vinod Koul]

On Fri, May 19, 2017 at 08:27:40AM +0200, Takashi Iwai wrote:
> On Fri, 19 May 2017 05:57:05 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >
> > > When appl_ptr is updated let low-level driver know, e.g.  to let the
> > > low-level driver/hardware pre-fetch data opportunistically.
> > >
> > > The existing .ack callback is extended with new attribute argument, to
> > > support this capability. Legacy driver subscribe to SND_PCM_ACK_LEGACY and
> > > doesn't process ack if it is not set. SND_PCM_ACK_APP_PTR can be used to
> > > process the application ptr update in the driver like in the skylake
> > > driver which can use this to inform position of appl pointer to host DMA
> > > controller. The skylake driver to process the SND_PCM_ACK_APP_PTR will be
> > > submitted with a separate patch set.
> > >
> > > In the ALSA core, this capability is independent from the NO_REWIND
> > > hardware flag. The low-level driver may however tie both options and only
> > > use the updated appl_ptr when rewinds are disabled due to hardware
> > > limitations.
> > >
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > > ---
> > >   include/sound/pcm-indirect.h  |  4 ++--
> > >   include/sound/pcm.h           |  8 +++++++-
> > >   sound/core/pcm_lib.c          |  6 ++++--
> > >   sound/core/pcm_native.c       | 24 +++++++++++++++++++++++-
> > >   sound/mips/hal2.c             | 14 +++++++++++---
> > >   sound/pci/cs46xx/cs46xx_lib.c | 18 ++++++++++++++----
> > >   sound/pci/emu10k1/emupcm.c    |  8 ++++++--
> > >   sound/pci/rme32.c             | 15 ++++++++++++---
> > >   8 files changed, 79 insertions(+), 18 deletions(-)
> > 
> > I think it better to take care of
> > 'drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c' as
> > well. This is a driver for sound functionality of Raspberry PI 1/zero
> > and some people may still get influences from this work.
> > 
> > $ git grep -A20 'struct snd_pcm_ops' v4.12-rc1 | grep \\.ack
> > v4.12-rc1:drivers/staging/vc04_services/bcm2835-audio/bcm2835-pcm.c-...
> > ...
> 
> Yep, we need to cover all codes if we really change the PCM ops.

Yes and fortunately there are a large set, so conversion is not a big issue
here, converging to best approach is :)

-- 
~Vinod

[PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Subhransu S. Prusty]

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

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

If driver subscribes for application pointer update, driver needs to get
notification whenever appl ptr changes. With the above case driver won't
get appl ptr notifications.

This patch check on a hw info flag and returns error when user land asks
for mmaping control & status data, thus forcing user to issue
IOCTL_SYNC_PTR.

Suggested-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
---
 include/uapi/sound/asound.h |  1 +
 sound/core/pcm_native.c     | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c697ff90450d..dea7d89b41ca 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -284,6 +284,7 @@ enum {
 #define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME     0x02000000  /* report absolute hardware link audio time, not reset on startup */
 #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
 #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
+#define SNDRV_PCM_INFO_NO_STATUS_MMAP	0x10000000	/* status and control mmap not supported */
 
 #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
 #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index c14cdbd9ff86..2635a7d4d1fa 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3524,21 +3524,38 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
 	struct snd_pcm_file * pcm_file;
 	struct snd_pcm_substream *substream;	
 	unsigned long offset;
+	unsigned int info;
 	
 	pcm_file = file->private_data;
 	substream = pcm_file->substream;
 	if (PCM_RUNTIME_CHECK(substream))
 		return -ENXIO;
+	info = substream->runtime->hw.info;
 
 	offset = area->vm_pgoff << PAGE_SHIFT;
 	switch (offset) {
 	case SNDRV_PCM_MMAP_OFFSET_STATUS:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+		/*
+		 * force fallback to ioctl if driver doesn't support status
+		 * and control mmap.
+		 */
+		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
+			return -ENXIO;
+
 		return snd_pcm_mmap_status(substream, file, area);
 	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
 		if (pcm_file->no_compat_mmap)
 			return -ENXIO;
+
+		/*
+		 * force fallback to ioctl if driver doesn't support status
+		 * and control mmap.
+		 */
+		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
+			return -ENXIO;
+
 		return snd_pcm_mmap_control(substream, file, area);
 	default:
 		return snd_pcm_mmap_data(substream, file, area);
-- 
1.9.1

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

Hi,

On May 16 2017 10:01, Subhransu S. Prusty wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>
> In case of mmap, by default alsa-lib mmaps both control and status data.
>
> If driver subscribes for application pointer update, driver needs to get
> notification whenever appl ptr changes. With the above case driver won't
> get appl ptr notifications.
>
> This patch check on a hw info flag and returns error when user land asks
> for mmaping control & status data, thus forcing user to issue
> IOCTL_SYNC_PTR.
>
> Suggested-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> ---
>  include/uapi/sound/asound.h |  1 +
>  sound/core/pcm_native.c     | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)

Would you please explain about the case that drivers can't support page 
frame mapping, please?

As long as I know, it depends on kernel and platform architecture 
whether mapping is available or not, because the data of 'struct 
snd_pcm_mmap_status' and 'struct snd_pcm_mmap_control' is not directly 
related to data transmission and independent of target device design.

Of course, I can understand your intension for previous patches. But the 
code comment is quite misleading and worthless.

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index c697ff90450d..dea7d89b41ca 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -284,6 +284,7 @@ enum {
>  #define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME     0x02000000  /* report absolute hardware link audio time, not reset on startup */
>  #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
>  #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
> +#define SNDRV_PCM_INFO_NO_STATUS_MMAP	0x10000000	/* status and control mmap not supported */
>
>  #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
>  #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index c14cdbd9ff86..2635a7d4d1fa 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -3524,21 +3524,38 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
>  	struct snd_pcm_file * pcm_file;
>  	struct snd_pcm_substream *substream;	
>  	unsigned long offset;
> +	unsigned int info;
>  	
>  	pcm_file = file->private_data;
>  	substream = pcm_file->substream;
>  	if (PCM_RUNTIME_CHECK(substream))
>  		return -ENXIO;
> +	info = substream->runtime->hw.info;
>
>  	offset = area->vm_pgoff << PAGE_SHIFT;
>  	switch (offset) {
>  	case SNDRV_PCM_MMAP_OFFSET_STATUS:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +		/*
> +		 * force fallback to ioctl if driver doesn't support status
> +		 * and control mmap.
> +		 */
> +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> +			return -ENXIO;
> +
>  		return snd_pcm_mmap_status(substream, file, area);
>  	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
>  		if (pcm_file->no_compat_mmap)
>  			return -ENXIO;
> +
> +		/*
> +		 * force fallback to ioctl if driver doesn't support status
> +		 * and control mmap.
> +		 */
> +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> +			return -ENXIO;
> +
>  		return snd_pcm_mmap_control(substream, file, area);
>  	default:
>  		return snd_pcm_mmap_data(substream, file, area);

Regards

Takashi Sakamoto

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

On Tue, 16 May 2017 07:38:40 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On May 16 2017 10:01, Subhransu S. Prusty wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > In case of mmap, by default alsa-lib mmaps both control and status data.
> >
> > If driver subscribes for application pointer update, driver needs to get
> > notification whenever appl ptr changes. With the above case driver won't
> > get appl ptr notifications.
> >
> > This patch check on a hw info flag and returns error when user land asks
> > for mmaping control & status data, thus forcing user to issue
> > IOCTL_SYNC_PTR.
> >
> > Suggested-by: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > Signed-off-by: Ramesh Babu <ramesh.babu@intel.com>
> > Signed-off-by: Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
> > Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > ---
> >  include/uapi/sound/asound.h |  1 +
> >  sound/core/pcm_native.c     | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> 
> Would you please explain about the case that drivers can't support
> page frame mapping, please?
> 
> As long as I know, it depends on kernel and platform architecture
> whether mapping is available or not, because the data of 'struct
> snd_pcm_mmap_status' and 'struct snd_pcm_mmap_control' is not directly
> related to data transmission and independent of target device design.

It's only a part of the condition.

In this case, the problem is that the mmap control allows the appl_ptr
being changed silently without interaction with the driver.  If the
driver requires some explicit action for changing the appl_ptr, it
won't work.

> Of course, I can understand your intension for previous patches. But
> the code comment is quite misleading and worthless.

Worthless is a too strong word, but I agree that more clarification
would be more helpful.


thanks,

Takashi

> 
> > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> > index c697ff90450d..dea7d89b41ca 100644
> > --- a/include/uapi/sound/asound.h
> > +++ b/include/uapi/sound/asound.h
> > @@ -284,6 +284,7 @@ enum {
> >  #define SNDRV_PCM_INFO_HAS_LINK_ABSOLUTE_ATIME     0x02000000  /* report absolute hardware link audio time, not reset on startup */
> >  #define SNDRV_PCM_INFO_HAS_LINK_ESTIMATED_ATIME    0x04000000  /* report estimated link audio time */
> >  #define SNDRV_PCM_INFO_HAS_LINK_SYNCHRONIZED_ATIME 0x08000000  /* report synchronized audio/system time */
> > +#define SNDRV_PCM_INFO_NO_STATUS_MMAP	0x10000000	/* status and control mmap not supported */
> >
> >  #define SNDRV_PCM_INFO_DRAIN_TRIGGER	0x40000000		/* internal kernel flag - trigger in drain */
> >  #define SNDRV_PCM_INFO_FIFO_IN_FRAMES	0x80000000	/* internal kernel flag - FIFO size is in frames */
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index c14cdbd9ff86..2635a7d4d1fa 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -3524,21 +3524,38 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
> >  	struct snd_pcm_file * pcm_file;
> >  	struct snd_pcm_substream *substream;	
> >  	unsigned long offset;
> > +	unsigned int info;
> >  	
> >  	pcm_file = file->private_data;
> >  	substream = pcm_file->substream;
> >  	if (PCM_RUNTIME_CHECK(substream))
> >  		return -ENXIO;
> > +	info = substream->runtime->hw.info;
> >
> >  	offset = area->vm_pgoff << PAGE_SHIFT;
> >  	switch (offset) {
> >  	case SNDRV_PCM_MMAP_OFFSET_STATUS:
> >  		if (pcm_file->no_compat_mmap)
> >  			return -ENXIO;
> > +		/*
> > +		 * force fallback to ioctl if driver doesn't support status
> > +		 * and control mmap.
> > +		 */
> > +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> > +			return -ENXIO;
> > +
> >  		return snd_pcm_mmap_status(substream, file, area);
> >  	case SNDRV_PCM_MMAP_OFFSET_CONTROL:
> >  		if (pcm_file->no_compat_mmap)
> >  			return -ENXIO;
> > +
> > +		/*
> > +		 * force fallback to ioctl if driver doesn't support status
> > +		 * and control mmap.
> > +		 */
> > +		if (info & SNDRV_PCM_INFO_NO_STATUS_MMAP)
> > +			return -ENXIO;
> > +
> >  		return snd_pcm_mmap_control(substream, file, area);
> >  	default:
> >  		return snd_pcm_mmap_data(substream, file, area);
> 
> Regards
> 
> Takashi Sakamoto
> 

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

On May 16 2017 14:46, Takashi Iwai wrote:
> In this case, the problem is that the mmap control allows the appl_ptr
> being changed silently without interaction with the driver.  If the
> driver requires some explicit action for changing the appl_ptr, it
> won't work.

I think 'struct snd_pcm_ops.pointer' is available for this purpose, too.

static snd_pcm_sframes_t snd_pcm_playback_rewind(...)
{
     ...
     hw_avail = snd_pcm_playback_hw_avail(runtime);
     (->struct snd_pcm_ops.pointer())
     ...
     (rewind PCM frames)
     ...
+   substream->ops->pointer(...);
     (->struct snd_pcm_ops.pointer())
     ...
}

If drivers need to handle event to update the appl_ptr, it records value 
of the appl_ptr, then compare it to current value to get the updates.

I note that the callback is done in both of process/irq contexts.

On May 16 2017 14:46, Takashi Iwai wrote:
 > Worthless is a too strong word, but I agree that more clarification
 > would be more helpful.

Sorry to use such strong expression. I'll care of it.


Regards

Takashi Sakamoto

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

On Tue, 16 May 2017 07:57:30 +0200,
Takashi Sakamoto wrote:
> 
> On May 16 2017 14:46, Takashi Iwai wrote:
> > In this case, the problem is that the mmap control allows the appl_ptr
> > being changed silently without interaction with the driver.  If the
> > driver requires some explicit action for changing the appl_ptr, it
> > won't work.
> 
> I think 'struct snd_pcm_ops.pointer' is available for this purpose, too.
> 
> static snd_pcm_sframes_t snd_pcm_playback_rewind(...)
> {
>     ...
>     hw_avail = snd_pcm_playback_hw_avail(runtime);
>     (->struct snd_pcm_ops.pointer())
>     ...
>     (rewind PCM frames)
>     ...
> +   substream->ops->pointer(...);
>     (->struct snd_pcm_ops.pointer())
>     ...
> }
> 
> If drivers need to handle event to update the appl_ptr, it records
> value of the appl_ptr, then compare it to current value to get the
> updates.

IIRC, the problem isn't about the forward / rewind but about the
normal data transfer.  In mmap mode, we transfer data on the mmap
buffer, and update appl_ptr via mmap control.  Both are done without
notification to the driver (which is intentional for avoiding the
context switching).

So we want to disable this optimization and always notify to the
driver.  Disabling mmap status/control is the straight hack as it
falls back to ioctl and then the driver can know the change.


Takashi

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

On May 16 2017 15:18, Takashi Iwai wrote:
> On Tue, 16 May 2017 07:57:30 +0200,
> Takashi Sakamoto wrote:
>>
>> On May 16 2017 14:46, Takashi Iwai wrote:
>>> In this case, the problem is that the mmap control allows the appl_ptr
>>> being changed silently without interaction with the driver.  If the
>>> driver requires some explicit action for changing the appl_ptr, it
>>> won't work.
>>
>> I think 'struct snd_pcm_ops.pointer' is available for this purpose, too.
>>
>> static snd_pcm_sframes_t snd_pcm_playback_rewind(...)
>> {
>>     ...
>>     hw_avail = snd_pcm_playback_hw_avail(runtime);
>>     (->struct snd_pcm_ops.pointer())
>>     ...
>>     (rewind PCM frames)
>>     ...
>> +   substream->ops->pointer(...);
>>     (->struct snd_pcm_ops.pointer())
>>     ...
>> }
>>
>> If drivers need to handle event to update the appl_ptr, it records
>> value of the appl_ptr, then compare it to current value to get the
>> updates.
>
> IIRC, the problem isn't about the forward / rewind but about the
> normal data transfer.  In mmap mode, we transfer data on the mmap
> buffer, and update appl_ptr via mmap control.  Both are done without
> notification to the driver (which is intentional for avoiding the
> context switching).
>
> So we want to disable this optimization and always notify to the
> driver.  Disabling mmap status/control is the straight hack as it
> falls back to ioctl and then the driver can know the change.

There's SNDRV_PCM_IOCTL_HW_SYNC command. In kernel land implementation, 
this command is handled with a call of 'struct snd_pcm_ops.pointer'.

In alsa-lib, this command is often executed in most cases to handle PCM 
frames.


Regards

Takashi Sakamoto

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

On Tue, 16 May 2017 08:24:39 +0200,
Takashi Sakamoto wrote:
> 
> On May 16 2017 15:18, Takashi Iwai wrote:
> > On Tue, 16 May 2017 07:57:30 +0200,
> > Takashi Sakamoto wrote:
> >>
> >> On May 16 2017 14:46, Takashi Iwai wrote:
> >>> In this case, the problem is that the mmap control allows the appl_ptr
> >>> being changed silently without interaction with the driver.  If the
> >>> driver requires some explicit action for changing the appl_ptr, it
> >>> won't work.
> >>
> >> I think 'struct snd_pcm_ops.pointer' is available for this purpose, too.
> >>
> >> static snd_pcm_sframes_t snd_pcm_playback_rewind(...)
> >> {
> >>     ...
> >>     hw_avail = snd_pcm_playback_hw_avail(runtime);
> >>     (->struct snd_pcm_ops.pointer())
> >>     ...
> >>     (rewind PCM frames)
> >>     ...
> >> +   substream->ops->pointer(...);
> >>     (->struct snd_pcm_ops.pointer())
> >>     ...
> >> }
> >>
> >> If drivers need to handle event to update the appl_ptr, it records
> >> value of the appl_ptr, then compare it to current value to get the
> >> updates.
> >
> > IIRC, the problem isn't about the forward / rewind but about the
> > normal data transfer.  In mmap mode, we transfer data on the mmap
> > buffer, and update appl_ptr via mmap control.  Both are done without
> > notification to the driver (which is intentional for avoiding the
> > context switching).
> >
> > So we want to disable this optimization and always notify to the
> > driver.  Disabling mmap status/control is the straight hack as it
> > falls back to ioctl and then the driver can know the change.
> 
> There's SNDRV_PCM_IOCTL_HW_SYNC command. In kernel land
> implementation, this command is handled with a call of 'struct
> snd_pcm_ops.pointer'.
> 
> In alsa-lib, this command is often executed in most cases to handle
> PCM frames.

The HWSYNC or SYNC_PTR ioctls are used only as the fallback when mmap
failed.  It's the exact goal of this patch :)


Takashi

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

On May 16 2017 15:34, Takashi Iwai wrote:
>>> IIRC, the problem isn't about the forward / rewind but about the
>>> normal data transfer.  In mmap mode, we transfer data on the mmap
>>> buffer, and update appl_ptr via mmap control.  Both are done without
>>> notification to the driver (which is intentional for avoiding the
>>> context switching).
>>>
>>> So we want to disable this optimization and always notify to the
>>> driver.  Disabling mmap status/control is the straight hack as it
>>> falls back to ioctl and then the driver can know the change.
>>
>> There's SNDRV_PCM_IOCTL_HW_SYNC command. In kernel land
>> implementation, this command is handled with a call of 'struct
>> snd_pcm_ops.pointer'.
>>
>> In alsa-lib, this command is often executed in most cases to handle
>> PCM frames.
>
> The HWSYNC or SYNC_PTR ioctls are used only as the fallback when mmap
> failed.  It's the exact goal of this patch :)

Although SYNC_PTR is in the fallback mechanism of alsa-lib, HWSYNC is 
not in.

(alsa-lib)
ioctl(SNDRV_PCM_IOCTL_HWSYNC)
<-snd_pcm_hw_hw_sync()
= struct snd_pcm_fast_ops.hwsync
   <-__snd_pcm_hw_hwsync()
     <-snd_pcm_hwsync()
     <-snd_pcm_avail()
       <-snd_pcm_read_areas()
         <-snd_pcm_mmap_readi()
         <-snd_pcm_mmap_readn()
     <-snd_pcm_avail_delay()
     <-snd_pcm_write_areas()
       <-snd_pcm_mmap_writei()
       <-snd_pcm_mmap_writen()

For a case that applications execute ioctl(2) with 
SNDRV_PCM_IOCTL_READN/READI/WRITEN/WRITEI, in kernel land 
snd_pcm_lib_read1()/snd_pcm_lib_write1() should have calls of 'struct 
snd_pcm_ops.pointer', I think.


Regards

Takashi Sakamoto

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Iwai]

On Tue, 16 May 2017 08:54:17 +0200,
Takashi Sakamoto wrote:
> 
> On May 16 2017 15:34, Takashi Iwai wrote:
> >>> IIRC, the problem isn't about the forward / rewind but about the
> >>> normal data transfer.  In mmap mode, we transfer data on the mmap
> >>> buffer, and update appl_ptr via mmap control.  Both are done without
> >>> notification to the driver (which is intentional for avoiding the
> >>> context switching).
> >>>
> >>> So we want to disable this optimization and always notify to the
> >>> driver.  Disabling mmap status/control is the straight hack as it
> >>> falls back to ioctl and then the driver can know the change.
> >>
> >> There's SNDRV_PCM_IOCTL_HW_SYNC command. In kernel land
> >> implementation, this command is handled with a call of 'struct
> >> snd_pcm_ops.pointer'.
> >>
> >> In alsa-lib, this command is often executed in most cases to handle
> >> PCM frames.
> >
> > The HWSYNC or SYNC_PTR ioctls are used only as the fallback when mmap
> > failed.  It's the exact goal of this patch :)
> 
> Although SYNC_PTR is in the fallback mechanism of alsa-lib, HWSYNC is
> not in.
> 
> (alsa-lib)
> ioctl(SNDRV_PCM_IOCTL_HWSYNC)
> <-snd_pcm_hw_hw_sync()
> = struct snd_pcm_fast_ops.hwsync
>   <-__snd_pcm_hw_hwsync()
>     <-snd_pcm_hwsync()
>     <-snd_pcm_avail()
>       <-snd_pcm_read_areas()
>         <-snd_pcm_mmap_readi()
>         <-snd_pcm_mmap_readn()
>     <-snd_pcm_avail_delay()
>     <-snd_pcm_write_areas()
>       <-snd_pcm_mmap_writei()
>       <-snd_pcm_mmap_writen()
> 
> For a case that applications execute ioctl(2) with
> SNDRV_PCM_IOCTL_READN/READI/WRITEN/WRITEI, in kernel land
> snd_pcm_lib_read1()/snd_pcm_lib_write1() should have calls of 'struct
> snd_pcm_ops.pointer', I think.

It's about the less-optimized code path with snd_pcm_mmap_read/write.
For the code path calling snd_pcm_mmap_begin() /
snd_pcm_mmap_commit(), there is no hwsync ioctl call.


Takashi

Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data

[Takashi Sakamoto]

On May 16 2017 16:02, Takashi Iwai wrote:
> On Tue, 16 May 2017 08:54:17 +0200,
> Takashi Sakamoto wrote:
>>
>> On May 16 2017 15:34, Takashi Iwai wrote:
>>>>> IIRC, the problem isn't about the forward / rewind but about the
>>>>> normal data transfer.  In mmap mode, we transfer data on the mmap
>>>>> buffer, and update appl_ptr via mmap control.  Both are done without
>>>>> notification to the driver (which is intentional for avoiding the
>>>>> context switching).
>>>>>
>>>>> So we want to disable this optimization and always notify to the
>>>>> driver.  Disabling mmap status/control is the straight hack as it
>>>>> falls back to ioctl and then the driver can know the change.
>>>>
>>>> There's SNDRV_PCM_IOCTL_HW_SYNC command. In kernel land
>>>> implementation, this command is handled with a call of 'struct
>>>> snd_pcm_ops.pointer'.
>>>>
>>>> In alsa-lib, this command is often executed in most cases to handle
>>>> PCM frames.
>>>
>>> The HWSYNC or SYNC_PTR ioctls are used only as the fallback when mmap
>>> failed.  It's the exact goal of this patch :)
>>
>> Although SYNC_PTR is in the fallback mechanism of alsa-lib, HWSYNC is
>> not in.
>>
>> (alsa-lib)
>> ioctl(SNDRV_PCM_IOCTL_HWSYNC)
>> <-snd_pcm_hw_hw_sync()
>> = struct snd_pcm_fast_ops.hwsync
>>   <-__snd_pcm_hw_hwsync()
>>     <-snd_pcm_hwsync()
>>     <-snd_pcm_avail()
>>       <-snd_pcm_read_areas()
>>         <-snd_pcm_mmap_readi()
>>         <-snd_pcm_mmap_readn()
>>     <-snd_pcm_avail_delay()
>>     <-snd_pcm_write_areas()
>>       <-snd_pcm_mmap_writei()
>>       <-snd_pcm_mmap_writen()
>>
>> For a case that applications execute ioctl(2) with
>> SNDRV_PCM_IOCTL_READN/READI/WRITEN/WRITEI, in kernel land
>> snd_pcm_lib_read1()/snd_pcm_lib_write1() should have calls of 'struct
>> snd_pcm_ops.pointer', I think.
>
> It's about the less-optimized code path with snd_pcm_mmap_read/write.
> For the code path calling snd_pcm_mmap_begin() /
> snd_pcm_mmap_commit(), there is no hwsync ioctl call.

Indeed. I recalled it from my memory just after posting the previous 
message. In a case of the fallback, ioctl(2) with SYNC_PTR is surely 
executed:

ioctl(SNDRV_PCM_IOCTL_SYNC_PTR)
<-snd_pcm_hw_hw_mmap_commit)
= struct snd_pcm_fast_ops.mmap_commit()
   <-__snd_pcm_mmap_commit()
     <-snd_pcm_mmap_commit()

Now I agreed with the aim of this patch. In alsa-lib, 0 is passed as a 
'flags' option to ioctl(2) with SYNC_PTR in the call of 
snd_pcm_mmap_commit(). In kernel land, overhead to handle the ioctl is 
lighter than HWSYNC because hwptr is not synchronized.

However, in my opinion, disabling mmap the status and control data is 
exaggerated to the aim. SYNC_PTR ioctl is still available when page 
frame of these data is mapped. I can assume an idea to take alsa-lib to 
execute this command in snd_pcm_mmap_commit() when the NO_REWIND flag is 
enabled. Here, I assume that the flag and SYNC_PTR are tight coupling. 
(but we should concern about backward compatibility of relevant stuffs...)


By the way, at present, I think that this patchset may also be 
convenient to IEC 611883-1/6 engine and drivers in ALSA firewire stack. 
The engine is programmed to execute packetization in both of irq context 
of OHCI1394 isochronous context and process context of HWSYNC, to reduce 
packetization delay for PCM data substream. However, to the process 
context, this design expects applications to call ioctl(2) periodically 
with some commands; e.g. HWSYNC. As Iwai-san said, this ioctl command is 
not executed in snd_pcm_mmap_begin()/snd_pcm_mmap_commit() loop and I 
had concerns about it. (For example, PulseAudio is programmed to execute 
snd_pcm_avail() in the loop, but jackd isn't.) But I forgot it, oops :p


Thanks

Takashi Sakamoto