From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: "Alexander E. Patrakov" <patrakov@gmail.com>,
alsa-devel@alsa-project.org
Cc: Takashi Iwai <tiwai@suse.de>
Subject: Re: [RFC PATCH 1/4] ALSA: core: let low-level driver or userspace disable rewinds
Date: Tue, 28 Jul 2015 09:19:32 -0500 [thread overview]
Message-ID: <55B78F74.1020601@linux.intel.com> (raw)
In-Reply-To: <55A14D23.3070600@gmail.com>
On 7/11/15 12:06 PM, Alexander E. Patrakov wrote:
> 08.07.2015 15:10, Pierre-Louis Bossart wrote:
>> 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.
>>
>> Caveat: there is currently no way to query capabilities without
>> opening a pcm stream, so applications might need to serially
>> open all exposed devices, check what they support by looking at
>> hw_params->info and close them (this is what PulseAudio does so
>> might not be an issue)
>
> Thanks for the patch. I was also about to add this flag, for the use in
> userspace. It's good to know that it is also useful in the kernel space.
>
> I usually don't comment on kernel patches, but this one provokes some
> questions and thoughts.
>
> 1. What amount of power-saving are we talking about, for Intel chips?
> (ideally, this should be in the commit message)
The power savings will depend on non-audio parameters, so it's
impossible to state a value. And it's probably not public information
anyway.
>
> 2. Shouldn't SND_PCM_APPEND (the mode used by dmix and friends) be also
> made incompatible with this flag, because, when mixing, it essentially
> overwrites incompletely-mixed data, i.e. rewinds without saying the word
> "rewind"? Shouldn't all other kinds of freewheeling be made incompatible
> with this flag, because the card, essentially, is never told about the
> application pointer? Or is this a userspace-only concern?
Good point, I wasn't aware of this flag. willl look into it.
> 3. I have not seen any justification for the drastic measure of making a
> DMA-based device completely unrewindable. Maybe a more polite "please
> make this a batch/blocktransfer card" request, thus disallowing only
> sub-period rewinds, would still be useful for powersaving, without
> killing dmix.
>
> 4. If this "no rewinds" mode is not made the default, then exactly
> nobody will use it. Everyone except sound servers opens the default
> device with the default flags. I understand the potential to break
> existing userspace, especially PulseAudio, but we really need to think
> more here.
Not sure I understand the issue. when a new functionality is added it
takes time to be adopted. If we can push it in sound servers first then
it creates a wide pool of users from day1.
>
> So, here is an alternative (or maybe additional - your flag does make
> sense, after all) hackish idea that should be compatible with the
> existing userspace.
>
> Please make it possible to "allow", "put in auto mode" or "disallow" the
> device to DMA up to one period in advance. Please use the following
> logic when in "auto mode":
This is a separate discussion. the patches I wrote are only to make sure
the hardware can safely fetch the data that is provided by userspace. I
think the open is more how we configure the DMA burst, something I
haven't looked at yet.
>
> If the card is batch or blocktransfer "by itself", let it be. We can't
> do anything here, the DMA operations are already "optimized" whether we
> want it or not.
>
> If the card does not support disabling the period wakeup, then FIXME - I
> don't know what to do. For compatibility, I think we can't optimize DMA
> operations here.
>
> If the card supports disabling the period wakeup, and it is disabled,
> then don't allow it to optimize DMA operations.
>
> If the card supports disabling the period wakeup, but it is not
> disabled, then allow it to optimize DMA operations.
>
> As you have probably guessed, the idea here is to detect typical setup
> made by PulseAudio. I suspect that this also affects CRAS.
>
> Also maybe this idea by David Henningsson is good:
>
> """
> E g, if the application sets a low period size
> but also sets the "disable period interrupts" flag, that could be an
> indicator that it wants lots of interrupts just to update the pointer,
> but nothing else. Maybe that's even the behaviour today (haven't checked).
> """
>
> (taken from
> http://mailman.alsa-project.org/pipermail/alsa-devel/2014-September/080885.html
> )
>
>>
>> Signed-off-by: Pierre-Louis Bossart
>> <pierre-louis.bossart@linux.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 691e7ee..25310b7 100644
>> --- a/include/sound/pcm.h
>> +++ b/include/sound/pcm.h
>> @@ -370,6 +370,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 a45be6b..b62b162 100644
>> --- a/include/uapi/sound/asound.h
>> +++ b/include/uapi/sound/asound.h
>> @@ -356,6 +356,7 @@ typedef int snd_pcm_hw_param_t;
>> #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 d126c03..a70e52d 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -543,6 +543,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;
>>
>> bits = snd_pcm_format_physical_width(runtime->format);
>> runtime->sample_bits = bits;
>> @@ -2428,6 +2430,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:
>> @@ -2476,6 +2481,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:
>>
>
>
next prev parent reply other threads:[~2015-07-28 14:19 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-08 10:10 [RFC PATCH 0/4] better support for bursty DMA usages Pierre-Louis Bossart
2015-07-08 10:10 ` [RFC PATCH 1/4] ALSA: core: let low-level driver or userspace disable rewinds Pierre-Louis Bossart
2015-07-08 14:21 ` Takashi Iwai
2015-07-08 16:58 ` Pierre-Louis Bossart
2015-07-11 17:06 ` Alexander E. Patrakov
2015-07-14 3:32 ` Raymond Yau
2015-07-14 17:39 ` Alexander E. Patrakov
2015-07-15 1:23 ` Raymond Yau
2015-07-15 4:51 ` Alexander E. Patrakov
2015-07-28 14:19 ` Pierre-Louis Bossart [this message]
2015-07-28 15:43 ` Alexander E. Patrakov
2015-07-29 17:46 ` Pierre-Louis Bossart
2015-07-30 6:11 ` Alexander E. Patrakov
2015-07-30 13:46 ` Pierre-Louis Bossart
2015-07-08 10:10 ` [RFC PATCH 2/4] ALSA: core: add .notify callback for pcm ops Pierre-Louis Bossart
2015-07-08 14:27 ` Takashi Iwai
2015-07-08 17:10 ` Pierre-Louis Bossart
2015-07-09 14:44 ` Takashi Iwai
2015-07-09 7:25 ` Raymond Yau
2015-07-09 7:35 ` Pierre-Louis Bossart
2015-07-08 10:10 ` [RFC PATCH 3/4] ALSA: core: add report of max dma burst Pierre-Louis Bossart
2015-07-08 14:37 ` Takashi Iwai
2015-07-08 17:46 ` Pierre-Louis Bossart
2015-07-10 2:35 ` Raymond Yau
2015-07-10 17:13 ` Lars-Peter Clausen
2015-07-11 17:46 ` Alexander E. Patrakov
2015-07-11 18:28 ` Jaroslav Kysela
2015-07-16 2:11 ` Raymond Yau
2015-07-08 10:10 ` [RFC PATCH 4/4] ALSA: hda: add default value for max_dma_burst Pierre-Louis Bossart
2015-07-08 14:31 ` [RFC PATCH 0/4] better support for bursty DMA usages Takashi Iwai
2015-07-08 17:50 ` Pierre-Louis Bossart
2015-07-15 10:14 ` Raymond Yau
2015-07-15 10:38 ` Alexander E. Patrakov
2015-07-22 14:44 ` Raymond Yau
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=55B78F74.1020601@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=patrakov@gmail.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 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.