* [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
@ 2017-06-12 5:27 Subhransu S. Prusty
2017-06-12 6:35 ` Takashi Sakamoto
2017-06-12 6:50 ` Clemens Ladisch
0 siblings, 2 replies; 8+ messages in thread
From: Subhransu S. Prusty @ 2017-06-12 5:27 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>
---
v1->v2: No change
Since the ack callback change is now dropped and "ALSA: pcm: conditionally
avoid mmap of control data" will be dropped with recent change "ALSA: pcm:
Suppress status/control mmap when ack ops is present", I think this patch
can be merged independently.
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 79fedf517070..c1e2b87cd409 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 bf5d0f2acfb9..0e8b7ea0d38d 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -554,6 +554,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;
@@ -2521,6 +2523,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);
ret = do_pcm_hwsync(substream);
if (!ret)
@@ -2539,6 +2544,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);
ret = do_pcm_hwsync(substream);
if (!ret)
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 5:27 [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty
@ 2017-06-12 6:35 ` Takashi Sakamoto
2017-06-12 12:09 ` Pierre-Louis Bossart
2017-06-12 6:50 ` Clemens Ladisch
1 sibling, 1 reply; 8+ messages in thread
From: Takashi Sakamoto @ 2017-06-12 6:35 UTC (permalink / raw)
To: Subhransu S. Prusty, alsa-devel
Cc: tiwai, lgirdwood, Ramesh Babu, Pierre-Louis Bossart,
patches.audio, broonie
Hi,
I have a question to your patch, in a point of the relationship between
queued PCM frames and position of appl_ptr.
On Jun 12 2017 14:27, 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>
> ---
>
> v1->v2: No change
>
> Since the ack callback change is now dropped and "ALSA: pcm: conditionally
> avoid mmap of control data" will be dropped with recent change "ALSA: pcm:
> Suppress status/control mmap when ack ops is present", I think this patch
> can be merged independently.
>
> 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 79fedf517070..c1e2b87cd409 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 bf5d0f2acfb9..0e8b7ea0d38d 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -554,6 +554,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;
> @@ -2521,6 +2523,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);
> ret = do_pcm_hwsync(substream);
> if (!ret)
> @@ -2539,6 +2544,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);
> ret = do_pcm_hwsync(substream);
> if (!ret)
In my understanding, your intention for this patch is to prevent queue
PCM frames in PCM buffer from being dropped. You have a plan to use
'struct snd_pcm_ops.ack' to notify the number of available PCM frames in
the buffer to your hardware. I don't know exactly about design of your
hardware because Intel developers never declare it.
Well, For playback PCM substream, the patch satisfies your intension. On
the other hand, for capture PCM substream, it doesn't.
See below diagram. In this case, one PCM buffer consists of three
period. The 'equal' sign represents queued PCM frames. You can see the
relationship between hwptr and applptr is inverse for playback/capture
substream.
For playback:
0 1p 2p 3p
|--======|========|==------|
^ ^
hwptr applptr
For capture:
0 1p 2p 3p
|-----===|========|=====---|
^ ^
applptr hwptr
When operate rewinding, for playback substream, some queued PCM frames
are dropped.
For playback:
0 1p 2p 3p
|--======|===_____|__------|
^ ^
hwptr applptr<-
The underscore sign represents the dropped PCM frames. But for capture
substream, no queued PCM frames are dropped.
For capture:
0 1p 2p 3p
|#####===|========|=====---|
^ ^
applptr<- hwptr
The sharp sign represent PCM frames which are available again for the
application.
In your case, for capture substream, 'forward' operation should be
avoided because it can drop queued PCM frames.
For capture:
0 1p 2p 3p
|-----___|__======|=====---|
^ ^
->applptr hwptr
If data transmission in your hardware somehow depends on a callback of
'struct snd_pcm_ops.ack()'. I can imagine a unit of data transmission as
an disadvantage of the forward operation.
If describing the detail design of your hardware, you might get more helps.
* I intentionally ignore discussion about burstness of data
transmission and accuracy of hwptr.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 5:27 [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty
2017-06-12 6:35 ` Takashi Sakamoto
@ 2017-06-12 6:50 ` Clemens Ladisch
2017-06-12 11:57 ` Pierre-Louis Bossart
1 sibling, 1 reply; 8+ messages in thread
From: Clemens Ladisch @ 2017-06-12 6:50 UTC (permalink / raw)
To: Subhransu S. Prusty
Cc: alsa-devel, tiwai, lgirdwood, Ramesh Babu, Pierre-Louis Bossart,
patches.audio, broonie
Subhransu S. Prusty wrote:
> Add new hw_params flag to explicitly tell driver that rewinds will never
> be used. [...]
> +++ b/sound/core/pcm_native.c
> @@ -2521,6 +2523,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;
Doing a rewind when you've promised not to is an error. So why doesn't
this return an error code?
Regards,
Clemens
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 6:50 ` Clemens Ladisch
@ 2017-06-12 11:57 ` Pierre-Louis Bossart
2017-06-12 18:44 ` Clemens Ladisch
0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2017-06-12 11:57 UTC (permalink / raw)
To: Clemens Ladisch, Subhransu S. Prusty
Cc: alsa-devel, tiwai, lgirdwood, Ramesh Babu, patches.audio, broonie
On 6/12/17 1:50 AM, Clemens Ladisch wrote:
> Subhransu S. Prusty wrote:
>> Add new hw_params flag to explicitly tell driver that rewinds will never
>> be used. [...]
>
>> +++ b/sound/core/pcm_native.c
>> @@ -2521,6 +2523,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;
>
> Doing a rewind when you've promised not to is an error. So why doesn't
> this return an error code?
there were earlier patches where we switched from 0 to an error code
based on Takashi's feedback. I am not that convinced that applications
actually interpret signed frames as error codes and might interpret
negative values literally, but I am not going to lay on the tracks on
this one. If we do move to error codes then we might want to update the
documentation to make the behavior self-explanatory.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 6:35 ` Takashi Sakamoto
@ 2017-06-12 12:09 ` Pierre-Louis Bossart
2017-06-13 11:09 ` Takashi Sakamoto
0 siblings, 1 reply; 8+ messages in thread
From: Pierre-Louis Bossart @ 2017-06-12 12:09 UTC (permalink / raw)
To: Takashi Sakamoto, Subhransu S. Prusty, alsa-devel
Cc: tiwai, patches.audio, broonie, lgirdwood, Ramesh Babu
On 6/12/17 1:35 AM, Takashi Sakamoto wrote:
> Hi,
>
> I have a question to your patch, in a point of the relationship between
> queued PCM frames and position of appl_ptr.
>
> On Jun 12 2017 14:27, 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>
>> ---
>>
>> v1->v2: No change
>>
>> Since the ack callback change is now dropped and "ALSA: pcm:
>> conditionally
>> avoid mmap of control data" will be dropped with recent change "ALSA:
>> pcm:
>> Suppress status/control mmap when ack ops is present", I think this patch
>> can be merged independently.
>>
>> 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 79fedf517070..c1e2b87cd409 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 bf5d0f2acfb9..0e8b7ea0d38d 100644
>> --- a/sound/core/pcm_native.c
>> +++ b/sound/core/pcm_native.c
>> @@ -554,6 +554,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;
>> @@ -2521,6 +2523,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);
>> ret = do_pcm_hwsync(substream);
>> if (!ret)
>> @@ -2539,6 +2544,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);
>> ret = do_pcm_hwsync(substream);
>> if (!ret)
>
> In my understanding, your intention for this patch is to prevent queue
> PCM frames in PCM buffer from being dropped. You have a plan to use
> 'struct snd_pcm_ops.ack' to notify the number of available PCM frames in
> the buffer to your hardware. I don't know exactly about design of your
> hardware because Intel developers never declare it.
>
> Well, For playback PCM substream, the patch satisfies your intension. On
> the other hand, for capture PCM substream, it doesn't.
>
> See below diagram. In this case, one PCM buffer consists of three
> period. The 'equal' sign represents queued PCM frames. You can see the
> relationship between hwptr and applptr is inverse for playback/capture
> substream.
>
> For playback:
> 0 1p 2p 3p
> |--======|========|==------|
> ^ ^
> hwptr applptr
>
> For capture:
> 0 1p 2p 3p
> |-----===|========|=====---|
> ^ ^
> applptr hwptr
>
> When operate rewinding, for playback substream, some queued PCM frames
> are dropped.
>
> For playback:
> 0 1p 2p 3p
> |--======|===_____|__------|
> ^ ^
> hwptr applptr<-
>
> The underscore sign represents the dropped PCM frames. But for capture
> substream, no queued PCM frames are dropped.
>
> For capture:
> 0 1p 2p 3p
> |#####===|========|=====---|
> ^ ^
> applptr<- hwptr
>
> The sharp sign represent PCM frames which are available again for the
> application.
>
> In your case, for capture substream, 'forward' operation should be
> avoided because it can drop queued PCM frames.
>
> For capture:
> 0 1p 2p 3p
> |-----___|__======|=====---|
> ^ ^
> ->applptr hwptr
>
> If data transmission in your hardware somehow depends on a callback of
> 'struct snd_pcm_ops.ack()'. I can imagine a unit of data transmission as
> an disadvantage of the forward operation.
>
> If describing the detail design of your hardware, you might get more helps.
>
> * I intentionally ignore discussion about burstness of data
> transmission and accuracy of hwptr.
You are probably reading too much into this.
The intent is to optimize DMA operations where the hardware now knows
both hw_ptr and appl_ptr. there is no use of the .ack() for copies into
intermediate buffers and the code for rewind on capture is largely for
consistency - we've never seen anyone using rewind on capture on
DSP-based platforms.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 11:57 ` Pierre-Louis Bossart
@ 2017-06-12 18:44 ` Clemens Ladisch
0 siblings, 0 replies; 8+ messages in thread
From: Clemens Ladisch @ 2017-06-12 18:44 UTC (permalink / raw)
To: alsa-devel
Pierre-Louis Bossart wrote:
> On 6/12/17 1:50 AM, Clemens Ladisch wrote:
>> Subhransu S. Prusty wrote:
>>> Add new hw_params flag to explicitly tell driver that rewinds will never
>>> be used. [...]
>>
>>> +++ b/sound/core/pcm_native.c
>>> @@ -2521,6 +2523,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;
>>
>> Doing a rewind when you've promised not to is an error. So why doesn't
>> this return an error code?
>
> there were earlier patches where we switched from 0 to an error code
> based on Takashi's feedback. I am not that convinced that applications
> actually interpret signed frames as error codes and might interpret
> negative values literally
When the application has set SND_PCM_HW_PARAMS_NO_REWINDS, calling
snd_pcm_rewind() already is an error to begin with. There's no point
in trying to make it work, you can just choose how it breaks.
Regards,
Clemens
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-12 12:09 ` Pierre-Louis Bossart
@ 2017-06-13 11:09 ` Takashi Sakamoto
2017-06-13 13:13 ` Pierre-Louis Bossart
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Sakamoto @ 2017-06-13 11:09 UTC (permalink / raw)
To: Pierre-Louis Bossart, Subhransu S. Prusty, alsa-devel
Cc: tiwai, patches.audio, broonie, lgirdwood, Ramesh Babu
Pierre-Louis,
On Jun 12 2017 21:09, Pierre-Louis Bossart wrote:
>> If describing the detail design of your hardware, you might get more
>> helps.
>>
>> * I intentionally ignore discussion about burstness of data
>> transmission and accuracy of hwptr.
>
> You are probably reading too much into this.
> The intent is to optimize DMA operations where the hardware now knows
> both hw_ptr and appl_ptr. there is no use of the .ack() for copies into
> intermediate buffers
If this patch included enough explanation about the disadvantage of
rewind operation on your driver and hardware in a point of optimization
for DMA data transmission and power consumption, I would not have driven
my imagination so hard for the reviewing.
> and the code for rewind on capture is largely for
> consistency - we've never seen anyone using rewind on capture on
> DSP-based platforms.
I'm not a prophet, and you too. It's not better to change interfaces
with any forecast. Furthermore, once changing interfaces, it affects to
all of applications. Please have enough care of it.
# I can imagine a disadvantage of rewind operation for PCM capture
# substream. When rewinding, available space on PCM buffer for data
# transmission from hardware is reduced. Then, even if hardware already
# done sampling, it can't transfer the PCM frames. There's a delay and
# hardware needs extra buffering.
Regards
Takashi Sakamoto
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds
2017-06-13 11:09 ` Takashi Sakamoto
@ 2017-06-13 13:13 ` Pierre-Louis Bossart
0 siblings, 0 replies; 8+ messages in thread
From: Pierre-Louis Bossart @ 2017-06-13 13:13 UTC (permalink / raw)
To: Takashi Sakamoto, Subhransu S. Prusty, alsa-devel
Cc: tiwai, patches.audio, broonie, lgirdwood, Ramesh Babu
On 6/13/17 7:09 AM, Takashi Sakamoto wrote:
> Pierre-Louis,
>
> On Jun 12 2017 21:09, Pierre-Louis Bossart wrote:
>>> If describing the detail design of your hardware, you might get more
>>> helps.
>>>
>>> * I intentionally ignore discussion about burstness of data
>>> transmission and accuracy of hwptr.
>>
>> You are probably reading too much into this.
>> The intent is to optimize DMA operations where the hardware now knows
>> both hw_ptr and appl_ptr. there is no use of the .ack() for copies into
>> intermediate buffers
>
> If this patch included enough explanation about the disadvantage of
> rewind operation on your driver and hardware in a point of optimization
> for DMA data transmission and power consumption, I would not have driven
> my imagination so hard for the reviewing.
Maybe you missed what we wrote in the commit message:
"this can be used by low-level driver to optimize DMA operations
and reduce power consumption."
There will be follow-up patches that make use of this capability on
Intel platforms but for now this is hardware-agnostic.
>
>> and the code for rewind on capture is largely for
>> consistency - we've never seen anyone using rewind on capture on
>> DSP-based platforms.
>
> I'm not a prophet, and you too. It's not better to change interfaces
> with any forecast. Furthermore, once changing interfaces, it affects to
> all of applications. Please have enough care of it.
I don't get what you are saying. There is no interface or behavior
change, disabling rewinds is strictly opt-in and controlled from the
application. I was just stating that we have not seen anyone use rewinds
on capture and at this point we have no need to disable non-existent
usages...
>
> # I can imagine a disadvantage of rewind operation for PCM capture
> # substream. When rewinding, available space on PCM buffer for data
> # transmission from hardware is reduced. Then, even if hardware already
> # done sampling, it can't transfer the PCM frames. There's a delay and
> # hardware needs extra buffering.
>
>
> Regards
>
> Takashi Sakamoto
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-13 13:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-12 5:27 [PATCH v2] ALSA: core: let low-level driver or userspace disable rewinds Subhransu S. Prusty
2017-06-12 6:35 ` Takashi Sakamoto
2017-06-12 12:09 ` Pierre-Louis Bossart
2017-06-13 11:09 ` Takashi Sakamoto
2017-06-13 13:13 ` Pierre-Louis Bossart
2017-06-12 6:50 ` Clemens Ladisch
2017-06-12 11:57 ` Pierre-Louis Bossart
2017-06-12 18:44 ` Clemens Ladisch
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.