* [PATCH 0/3] ALSA: Add rewind disable support @ 2017-05-16 1:01 Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty ` (2 more replies) 0 siblings, 3 replies; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 1:01 UTC (permalink / raw) To: alsa-devel; +Cc: tiwai, patches.audio, broonie, Subhransu S. Prusty, lgirdwood 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds 2017-05-16 1:01 [PATCH 0/3] ALSA: Add rewind disable support Subhransu S. Prusty @ 2017-05-16 1:01 ` Subhransu S. Prusty 2017-05-16 5:53 ` Takashi Iwai 2017-05-16 1:01 ` [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data Subhransu S. Prusty 2 siblings, 1 reply; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 1:01 UTC (permalink / raw) To: alsa-devel Cc: tiwai, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, patches.audio, broonie, 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds 2017-05-16 1:01 ` [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty @ 2017-05-16 5:53 ` Takashi Iwai 2017-05-16 7:40 ` Subhransu S. Prusty 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 5:53 UTC (permalink / raw) To: Subhransu S. Prusty Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds 2017-05-16 5:53 ` Takashi Iwai @ 2017-05-16 7:40 ` Subhransu S. Prusty 0 siblings, 0 replies; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 7:40 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie 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 -- ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 1:01 [PATCH 0/3] ALSA: Add rewind disable support Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty @ 2017-05-16 1:01 ` Subhransu S. Prusty 2017-05-16 5:56 ` Takashi Iwai 2017-05-19 3:57 ` Takashi Sakamoto 2017-05-16 1:01 ` [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data Subhransu S. Prusty 2 siblings, 2 replies; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 1:01 UTC (permalink / raw) To: alsa-devel Cc: tiwai, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, patches.audio, broonie, 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 1:01 ` [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Subhransu S. Prusty @ 2017-05-16 5:56 ` Takashi Iwai 2017-05-16 7:36 ` Subhransu S. Prusty 2017-05-16 16:11 ` Pierre-Louis Bossart 2017-05-19 3:57 ` Takashi Sakamoto 1 sibling, 2 replies; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 5:56 UTC (permalink / raw) To: Subhransu S. Prusty Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie 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 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 5:56 ` Takashi Iwai @ 2017-05-16 7:36 ` Subhransu S. Prusty 2017-05-18 6:18 ` Subhransu S. Prusty 2017-05-16 16:11 ` Pierre-Louis Bossart 1 sibling, 1 reply; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 7:36 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie 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 -- ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 7:36 ` Subhransu S. Prusty @ 2017-05-18 6:18 ` Subhransu S. Prusty 2017-05-18 8:09 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-18 6:18 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie 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 > > -- -- ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-18 6:18 ` Subhransu S. Prusty @ 2017-05-18 8:09 ` Takashi Iwai 2017-05-19 13:11 ` Takashi Iwai 2017-05-22 5:41 ` Vinod Koul 0 siblings, 2 replies; 31+ messages in thread From: Takashi Iwai @ 2017-05-18 8:09 UTC (permalink / raw) To: Subhransu S. Prusty Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-18 8:09 ` Takashi Iwai @ 2017-05-19 13:11 ` Takashi Iwai 2017-05-22 5:41 ` Vinod Koul 1 sibling, 0 replies; 31+ messages in thread From: Takashi Iwai @ 2017-05-19 13:11 UTC (permalink / raw) To: Subhransu S. Prusty Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-18 8:09 ` Takashi Iwai 2017-05-19 13:11 ` Takashi Iwai @ 2017-05-22 5:41 ` Vinod Koul 1 sibling, 0 replies; 31+ messages in thread From: Vinod Koul @ 2017-05-22 5:41 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 5:56 ` Takashi Iwai 2017-05-16 7:36 ` Subhransu S. Prusty @ 2017-05-16 16:11 ` Pierre-Louis Bossart 1 sibling, 0 replies; 31+ messages in thread From: Pierre-Louis Bossart @ 2017-05-16 16:11 UTC (permalink / raw) To: Takashi Iwai, Subhransu S. Prusty Cc: patches.audio, alsa-devel, broonie, lgirdwood, Jaikrishna Nemallapudi 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 >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-16 1:01 ` [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Subhransu S. Prusty 2017-05-16 5:56 ` Takashi Iwai @ 2017-05-19 3:57 ` Takashi Sakamoto 2017-05-19 6:27 ` Takashi Iwai 1 sibling, 1 reply; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-19 3:57 UTC (permalink / raw) To: Subhransu S. Prusty, alsa-devel Cc: tiwai, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, patches.audio, broonie 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-19 3:57 ` Takashi Sakamoto @ 2017-05-19 6:27 ` Takashi Iwai 2017-05-19 15:01 ` Pierre-Louis Bossart 2017-05-22 5:21 ` Vinod Koul 0 siblings, 2 replies; 31+ messages in thread From: Takashi Iwai @ 2017-05-19 6:27 UTC (permalink / raw) To: Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-19 6:27 ` Takashi Iwai @ 2017-05-19 15:01 ` Pierre-Louis Bossart 2017-05-22 5:22 ` Vinod Koul 2017-05-22 5:21 ` Vinod Koul 1 sibling, 1 reply; 31+ messages in thread From: Pierre-Louis Bossart @ 2017-05-19 15:01 UTC (permalink / raw) To: Takashi Iwai, Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, broonie, Subhransu S. Prusty 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 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-19 15:01 ` Pierre-Louis Bossart @ 2017-05-22 5:22 ` Vinod Koul 2017-05-22 7:16 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Vinod Koul @ 2017-05-22 5:22 UTC (permalink / raw) To: Pierre-Louis Bossart Cc: alsa-devel, Takashi Iwai, lgirdwood, Jaikrishna Nemallapudi, Takashi Sakamoto, patches.audio, broonie, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-22 5:22 ` Vinod Koul @ 2017-05-22 7:16 ` Takashi Iwai 2017-05-26 7:42 ` Vinod Koul 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-22 7:16 UTC (permalink / raw) To: Vinod Koul Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Takashi Sakamoto, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-22 7:16 ` Takashi Iwai @ 2017-05-26 7:42 ` Vinod Koul 2017-05-26 7:47 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Vinod Koul @ 2017-05-26 7:42 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Takashi Sakamoto, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-26 7:42 ` Vinod Koul @ 2017-05-26 7:47 ` Takashi Iwai 2017-05-26 8:01 ` Vinod Koul 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-26 7:47 UTC (permalink / raw) To: Vinod Koul Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Takashi Sakamoto, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-26 7:47 ` Takashi Iwai @ 2017-05-26 8:01 ` Vinod Koul 0 siblings, 0 replies; 31+ messages in thread From: Vinod Koul @ 2017-05-26 8:01 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Jaikrishna Nemallapudi, Pierre-Louis Bossart, broonie, Takashi Sakamoto, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr 2017-05-19 6:27 ` Takashi Iwai 2017-05-19 15:01 ` Pierre-Louis Bossart @ 2017-05-22 5:21 ` Vinod Koul 1 sibling, 0 replies; 31+ messages in thread From: Vinod Koul @ 2017-05-22 5:21 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, Pierre-Louis Bossart, Jaikrishna Nemallapudi, lgirdwood, broonie, Takashi Sakamoto, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 1:01 [PATCH 0/3] ALSA: Add rewind disable support Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Subhransu S. Prusty @ 2017-05-16 1:01 ` Subhransu S. Prusty 2017-05-16 5:38 ` Takashi Sakamoto 2 siblings, 1 reply; 31+ messages in thread From: Subhransu S. Prusty @ 2017-05-16 1:01 UTC (permalink / raw) To: alsa-devel Cc: tiwai, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, patches.audio, broonie, Jaikrishna Nemallapudi, 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 1:01 ` [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data Subhransu S. Prusty @ 2017-05-16 5:38 ` Takashi Sakamoto 2017-05-16 5:46 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-16 5:38 UTC (permalink / raw) To: Subhransu S. Prusty, alsa-devel Cc: tiwai, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, patches.audio, broonie, Jaikrishna Nemallapudi 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 5:38 ` Takashi Sakamoto @ 2017-05-16 5:46 ` Takashi Iwai 2017-05-16 5:57 ` Takashi Sakamoto 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 5:46 UTC (permalink / raw) To: Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 5:46 ` Takashi Iwai @ 2017-05-16 5:57 ` Takashi Sakamoto 2017-05-16 6:18 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-16 5:57 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 5:57 ` Takashi Sakamoto @ 2017-05-16 6:18 ` Takashi Iwai 2017-05-16 6:24 ` Takashi Sakamoto 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 6:18 UTC (permalink / raw) To: Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 6:18 ` Takashi Iwai @ 2017-05-16 6:24 ` Takashi Sakamoto 2017-05-16 6:34 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-16 6:24 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 6:24 ` Takashi Sakamoto @ 2017-05-16 6:34 ` Takashi Iwai 2017-05-16 6:54 ` Takashi Sakamoto 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 6:34 UTC (permalink / raw) To: Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 6:34 ` Takashi Iwai @ 2017-05-16 6:54 ` Takashi Sakamoto 2017-05-16 7:02 ` Takashi Iwai 0 siblings, 1 reply; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-16 6:54 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 6:54 ` Takashi Sakamoto @ 2017-05-16 7:02 ` Takashi Iwai 2017-05-16 10:55 ` Takashi Sakamoto 0 siblings, 1 reply; 31+ messages in thread From: Takashi Iwai @ 2017-05-16 7:02 UTC (permalink / raw) To: Takashi Sakamoto Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data 2017-05-16 7:02 ` Takashi Iwai @ 2017-05-16 10:55 ` Takashi Sakamoto 0 siblings, 0 replies; 31+ messages in thread From: Takashi Sakamoto @ 2017-05-16 10:55 UTC (permalink / raw) To: Takashi Iwai Cc: alsa-devel, patches.audio, lgirdwood, Ramesh Babu, Pierre-Louis Bossart, broonie, Jaikrishna Nemallapudi, Subhransu S. Prusty 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-05-26 7:59 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-16 1:01 [PATCH 0/3] ALSA: Add rewind disable support Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 1/3] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty 2017-05-16 5:53 ` Takashi Iwai 2017-05-16 7:40 ` Subhransu S. Prusty 2017-05-16 1:01 ` [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Subhransu S. Prusty 2017-05-16 5:56 ` Takashi Iwai 2017-05-16 7:36 ` Subhransu S. Prusty 2017-05-18 6:18 ` Subhransu S. Prusty 2017-05-18 8:09 ` Takashi Iwai 2017-05-19 13:11 ` Takashi Iwai 2017-05-22 5:41 ` Vinod Koul 2017-05-16 16:11 ` Pierre-Louis Bossart 2017-05-19 3:57 ` Takashi Sakamoto 2017-05-19 6:27 ` Takashi Iwai 2017-05-19 15:01 ` Pierre-Louis Bossart 2017-05-22 5:22 ` Vinod Koul 2017-05-22 7:16 ` Takashi Iwai 2017-05-26 7:42 ` Vinod Koul 2017-05-26 7:47 ` Takashi Iwai 2017-05-26 8:01 ` Vinod Koul 2017-05-22 5:21 ` Vinod Koul 2017-05-16 1:01 ` [PATCH 3/3] ALSA: pcm: conditionally avoid mmap of control data Subhransu S. Prusty 2017-05-16 5:38 ` Takashi Sakamoto 2017-05-16 5:46 ` Takashi Iwai 2017-05-16 5:57 ` Takashi Sakamoto 2017-05-16 6:18 ` Takashi Iwai 2017-05-16 6:24 ` Takashi Sakamoto 2017-05-16 6:34 ` Takashi Iwai 2017-05-16 6:54 ` Takashi Sakamoto 2017-05-16 7:02 ` Takashi Iwai 2017-05-16 10:55 ` Takashi Sakamoto
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.