From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Sakamoto Subject: Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr Date: Fri, 19 May 2017 12:57:05 +0900 Message-ID: <0be78e8b-ebc0-9f8b-5bed-df01a791f940@sakamocchi.jp> References: <1494896518-23399-1-git-send-email-subhransu.s.prusty@intel.com> <1494896518-23399-3-git-send-email-subhransu.s.prusty@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-proxy004.phy.lolipop.jp (smtp-proxy004.phy.lolipop.jp [157.7.104.45]) by alsa0.perex.cz (Postfix) with ESMTP id 486F3266A0D for ; Fri, 19 May 2017 05:57:07 +0200 (CEST) In-Reply-To: <1494896518-23399-3-git-send-email-subhransu.s.prusty@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: "Subhransu S. Prusty" , alsa-devel@alsa-project.org Cc: tiwai@suse.de, lgirdwood@gmail.com, Jaikrishna Nemallapudi , Pierre-Louis Bossart , patches.audio@intel.com, broonie@kernel.org List-Id: alsa-devel@alsa-project.org Hi, On May 16 2017 10:01, Subhransu S. Prusty wrote: > From: Pierre-Louis Bossart > > 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 > Signed-off-by: Jaikrishna Nemallapudi > Signed-off-by: Subhransu S. Prusty > --- > 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