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

--

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

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

--

--

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

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

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