From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Takashi Iwai <tiwai@suse.de>,
"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Cc: patches.audio@intel.com, alsa-devel@alsa-project.org,
broonie@kernel.org, lgirdwood@gmail.com,
Jaikrishna Nemallapudi <jaikrishnax.nemallapudi@intel.com>
Subject: Re: [PATCH 2/3] ALSA: core: modify .ack callback to take arguments for updating appl ptr
Date: Tue, 16 May 2017 11:11:53 -0500 [thread overview]
Message-ID: <78f5b0c3-a745-7ab6-065d-cb7cf3fd5334@linux.intel.com> (raw)
In-Reply-To: <s5hinl11h0j.wl-tiwai@suse.de>
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
>>
next prev parent reply other threads:[~2017-05-16 16:12 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=78f5b0c3-a745-7ab6-065d-cb7cf3fd5334@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=jaikrishnax.nemallapudi@intel.com \
--cc=lgirdwood@gmail.com \
--cc=patches.audio@intel.com \
--cc=subhransu.s.prusty@intel.com \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).