[alsa-devel] [PATCH v3 0/3] introduce device_buffer
Old wine in a new bottle with added flavour now. Based on our discussions in LPC, now we will report the combined delay value as sum of the buffering done in driver and hardware. This is used to report delay Additionally we tell pcm what is buffer in device/driver, this is used to check for error calulcations
Vinod Koul (3): ALSA: pcm - introduce device_buffer ASoC: add device_buffer in asoc ASoC: mid-x86 - implement buffer query in sst_platform driver
include/sound/pcm.h | 1 + include/sound/soc-dai.h | 6 ++++++ include/sound/soc.h | 6 ++++++ sound/core/pcm_lib.c | 10 +++++++++- sound/soc/mid-x86/sst_platform.c | 23 +++++++++++++++++++++++ sound/soc/mid-x86/sst_platform.h | 4 +++- sound/soc/soc-pcm.c | 13 ++++++++++++- 7 files changed, 60 insertions(+), 3 deletions(-)
In many modern SoCs the audio DSP can buffer the PCM ring buffer data, typically in SRAMs or any other memory available. Today we have no means to represent this buffering and ALSA in some cases when device still has to render this buffer but app_ptr is equal to hw_ptr alsa wrongly detects an overrun
This patch tries to add a new field "device_buffer" to represent buffering done in DSPs. This value is also used for the xrun calculations in ALSA
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com --- include/sound/pcm.h | 1 + sound/core/pcm_lib.c | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index c75c0d1..58e669a 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -281,6 +281,7 @@ struct snd_pcm_runtime { unsigned long hw_ptr_jiffies; /* Time when hw_ptr is updated */ unsigned long hw_ptr_buffer_jiffies; /* buffer time in jiffies */ snd_pcm_sframes_t delay; /* extra delay; typically FIFO size */ + snd_pcm_sframes_t device_buffer; /* buffered data in device/driver */
/* -- HW params -- */ snd_pcm_access_t access; /* access mode */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 7ae6719..9b0127f 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -292,7 +292,15 @@ int snd_pcm_update_state(struct snd_pcm_substream *substream, return -EPIPE; } } else { - if (avail >= runtime->stop_threshold) { + snd_pcm_uframes_t actual_avail; + if (avail < runtime->device_buffer) + actual_avail = avail; + else + actual_avail = avail - runtime->device_buffer; + if (actual_avail >= runtime->stop_threshold) { + snd_printd(KERN_ERR "avail > stop_threshold!!\n"); + snd_printd(KERN_ERR "actual_avail %ld, avail %ld, device_buffer %ld!!\n", + actual_avail, avail, runtime->device_buffer); xrun(substream); return -EPIPE; }
2012-8-31 上午9:08 於 "Vinod Koul" vinod.koul@linux.intel.com 寫道：
In many modern SoCs the audio DSP can buffer the PCM ring buffer data,
typically
in SRAMs or any other memory available. Today we have no means to represent this buffering and ALSA in some cases
when
device still has to render this buffer but app_ptr is equal to hw_ptr alsa wrongly detects an overrun
This patch tries to add a new field "device_buffer" to represent
buffering done in
DSPs. This value is also used for the xrun calculations in ALSA
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
are these buffer similar to hda 's FIFO ?
can the alsa application use snd_pcm_rewind to rewind the app_ptr ?
On Fri, 2012-08-31 at 10:06 +0800, Raymond Yau wrote:
2012-8-31 上午9:08 於 "Vinod Koul" vinod.koul@linux.intel.com 寫道：
In many modern SoCs the audio DSP can buffer the PCM ring buffer
data, typically
in SRAMs or any other memory available. Today we have no means to represent this buffering and ALSA in some
cases when
device still has to render this buffer but app_ptr is equal to
hw_ptr alsa
wrongly detects an overrun
This patch tries to add a new field "device_buffer" to represent
buffering done in
DSPs. This value is also used for the xrun calculations in ALSA
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com
are these buffer similar to hda 's FIFO ?
No FIFO should only be represented by "delay" not by this one.
can the alsa application use snd_pcm_rewind to rewind the app_ptr ?
Well yes, but if device is deep buffering that it can cause issues. We discussed this and perhaps it would be good to tell device about app pointer and also rewind/fwd, so that it can control this buffering
Date 31.8.2012 03:14, Vinod Koul wrote:
if (avail >= runtime->stop_threshold) {
snd_pcm_uframes_t actual_avail;if (avail < runtime->device_buffer)actual_avail = avail;elseactual_avail = avail - runtime->device_buffer;if (actual_avail >= runtime->stop_threshold) {
Perhaps this may be simplified to: if (avail >= runtime->stop_threshold + runtime->device_buffer)
snd_printd(KERN_ERR "avail > stop_threshold!!\n");snd_printd(KERN_ERR "actual_avail %ld, avail %ld, device_buffer %ld!!\n",actual_avail, avail, runtime->device_buffer);
I would propose to enhance xrun_log() to show something about device_buffer and remove these printd calls from this location.
Jaroslav
On Fri, 2012-08-31 at 12:48 +0200, Jaroslav Kysela wrote:
Date 31.8.2012 03:14, Vinod Koul wrote:
if (avail >= runtime->stop_threshold) {
snd_pcm_uframes_t actual_avail;if (avail < runtime->device_buffer)actual_avail = avail;elseactual_avail = avail - runtime->device_buffer;if (actual_avail >= runtime->stop_threshold) {Perhaps this may be simplified to: if (avail >= runtime->stop_threshold + runtime->device_buffer)
I see no reason why this wouldnt work, and dont know why i didnt take this more intuitive approach. Let me grab some coffee for my jet lagged mind and rethink :)
snd_printd(KERN_ERR "avail > stop_threshold!!\n");snd_printd(KERN_ERR "actual_avail %ld, avail %ld, device_buffer %ld!!\n",actual_avail, avail, runtime->device_buffer);I would propose to enhance xrun_log() to show something about device_buffer and remove these printd calls from this location.
Yup
Jaroslav
This adds a new callback "buffer" in dai_ops and platform. The runtime->delay is reported to sound core as sum of delay and buffer value. The buffer value is also reported independently to pcm core for error checks
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com --- include/sound/soc-dai.h | 6 ++++++ include/sound/soc.h | 6 ++++++ sound/soc/soc-pcm.c | 13 ++++++++++++- 3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 628db7b..b0c84df 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -182,6 +182,12 @@ struct snd_soc_dai_ops { */ snd_pcm_sframes_t (*delay)(struct snd_pcm_substream *, struct snd_soc_dai *); + /* + * For hardware based buffer reporting + * Optional. + */ + snd_pcm_sframes_t (*buffer)(struct snd_pcm_substream *, + struct snd_soc_dai *); };
/* diff --git a/include/sound/soc.h b/include/sound/soc.h index 313b766..f50bdf9 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -795,6 +795,12 @@ struct snd_soc_platform_driver { */ snd_pcm_sframes_t (*delay)(struct snd_pcm_substream *, struct snd_soc_dai *); + /* + * For hardware based buffer reporting + * Optional. + */ + snd_pcm_sframes_t (*buffer)(struct snd_pcm_substream *, + struct snd_soc_dai *);
/* platform stream pcm ops */ struct snd_pcm_ops *ops; diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ef22d0b..605337f 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -674,6 +674,7 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; snd_pcm_uframes_t offset = 0; snd_pcm_sframes_t delay = 0; + snd_pcm_sframes_t buffer = 0;
if (platform->driver->ops && platform->driver->ops->pointer) offset = platform->driver->ops->pointer(substream); @@ -687,7 +688,17 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream) if (platform->driver->delay) delay += platform->driver->delay(substream, codec_dai);
- runtime->delay = delay; + if (cpu_dai->driver->ops->buffer) + buffer += cpu_dai->driver->ops->buffer(substream, cpu_dai); + + if (codec_dai->driver->ops->buffer) + buffer += codec_dai->driver->ops->buffer(substream, codec_dai); + + if (platform->driver->buffer) + buffer += platform->driver->buffer(substream, codec_dai); + + runtime->delay = delay + buffer; + runtime->device_buffer = buffer;
return offset; }
Signed-off-by: Vinod Koul vinod.koul@linux.intel.com --- sound/soc/mid-x86/sst_platform.c | 23 +++++++++++++++++++++++ sound/soc/mid-x86/sst_platform.h | 4 +++- 2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/sound/soc/mid-x86/sst_platform.c b/sound/soc/mid-x86/sst_platform.c index a263cbe..ec8773f 100644 --- a/sound/soc/mid-x86/sst_platform.c +++ b/sound/soc/mid-x86/sst_platform.c @@ -424,6 +424,28 @@ static snd_pcm_uframes_t sst_platform_pcm_pointer return stream->stream_info.buffer_ptr; }
+static snd_pcm_sframes_t sst_platform_pcm_buffer( + struct snd_pcm_substream *substream, + struct snd_soc_dai *dai) +{ + struct sst_runtime_stream *stream; + int ret_val, status; + struct pcm_stream_info *str_info; + + stream = substream->runtime->private_data; + status = sst_get_stream_status(stream); + if (status == SST_PLATFORM_INIT) + return 0; + str_info = &stream->stream_info; + ret_val = stream->ops->device_control( + SST_SND_BUFFER_DELTA, str_info); + if (ret_val) { + pr_err("sst: error code = %d\n", ret_val); + return ret_val; + } + return stream->stream_info.buffer_delta; +} + static int sst_platform_pcm_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params) { @@ -669,6 +691,7 @@ static struct snd_soc_platform_driver sst_soc_platform_drv = { .compr_ops = &sst_platform_compr_ops, .pcm_new = sst_pcm_new, .pcm_free = sst_pcm_free, + .buffer = sst_platform_pcm_buffer, };
static int sst_platform_probe(struct platform_device *pdev) diff --git a/sound/soc/mid-x86/sst_platform.h b/sound/soc/mid-x86/sst_platform.h index d61c5d5..300b45f 100644 --- a/sound/soc/mid-x86/sst_platform.h +++ b/sound/soc/mid-x86/sst_platform.h @@ -50,6 +50,7 @@ struct pcm_stream_info { void *mad_substream; void (*period_elapsed) (void *mad_substream); unsigned long long buffer_ptr; + unsigned long long buffer_delta; int sfreq; };
@@ -70,7 +71,8 @@ enum sst_controls { SST_SND_BUFFER_POINTER = 0x05, SST_SND_STREAM_INIT = 0x06, SST_SND_START = 0x07, - SST_MAX_CONTROLS = 0x07, + SST_SND_BUFFER_DELTA = 0x08, + SST_MAX_CONTROLS = 0x08, };
enum sst_stream_ops {
Date 31.8.2012 03:14, Vinod Koul wrote:
Old wine in a new bottle with added flavour now. Based on our discussions in LPC, now we will report the combined delay value as sum of the buffering done in driver and hardware. This is used to report delay
Additionally we tell pcm what is buffer in device/driver, this is used to check for error calulcations
I'm afraid, but this won't work for the mmaped appl_ptr / hw_ptr, because the checks are done outside the kernel - in alsa-lib. I believe that this API should be enhanced, too. But the question is, how to pass two values (hwptr, device_buffer) atomically to the user space without locks for x86.
Jaroslav
Vinod Koul (3): ALSA: pcm - introduce device_buffer ASoC: add device_buffer in asoc ASoC: mid-x86 - implement buffer query in sst_platform driver
include/sound/pcm.h | 1 + include/sound/soc-dai.h | 6 ++++++ include/sound/soc.h | 6 ++++++ sound/core/pcm_lib.c | 10 +++++++++- sound/soc/mid-x86/sst_platform.c | 23 +++++++++++++++++++++++ sound/soc/mid-x86/sst_platform.h | 4 +++- sound/soc/soc-pcm.c | 13 ++++++++++++- 7 files changed, 60 insertions(+), 3 deletions(-)
On Fri, 2012-08-31 at 12:41 +0200, Jaroslav Kysela wrote:
Date 31.8.2012 03:14, Vinod Koul wrote:
Old wine in a new bottle with added flavour now. Based on our discussions in LPC, now we will report the combined delay value as sum of the buffering done in driver and hardware. This is used to report delay
Additionally we tell pcm what is buffer in device/driver, this is used to check for error calulcations
I'm afraid, but this won't work for the mmaped appl_ptr / hw_ptr, because the checks are done outside the kernel - in alsa-lib. I believe that this API should be enhanced, too. But the question is, how to pass two values (hwptr, device_buffer) atomically to the user space without locks for x86.
Yes for mmap case, tis needs to be done in lib and that can't be done without ABI/API changes?? Is there a reserved field we can perhaps use for this
At Fri, 31 Aug 2012 20:23:18 +0530, Vinod Koul wrote:
On Fri, 2012-08-31 at 12:41 +0200, Jaroslav Kysela wrote:
Date 31.8.2012 03:14, Vinod Koul wrote:
Old wine in a new bottle with added flavour now. Based on our discussions in LPC, now we will report the combined delay value as sum of the buffering done in driver and hardware. This is used to report delay
Additionally we tell pcm what is buffer in device/driver, this is used to check for error calulcations
I'm afraid, but this won't work for the mmaped appl_ptr / hw_ptr, because the checks are done outside the kernel - in alsa-lib. I believe that this API should be enhanced, too. But the question is, how to pass two values (hwptr, device_buffer) atomically to the user space without locks for x86.
Yes for mmap case, tis needs to be done in lib and that can't be done without ABI/API changes?? Is there a reserved field we can perhaps use for this
We've discussed about this issue at Plumbers in the last week, and found out that yet different solutions would be needed:
- The problem is found in two places, the XRUN check and the DRAINING check. We need to handle both cases.
- In XRUN check, when hw_ptr == appl_ptr, it's really an XRUN _unless_ the PCM state is DRAINING. This is no matter whether the device has a device DMA buffer. The reason is that the hardware is supposed to fetch the new data at this point. Thus, it would fetch the wrong data if we check hw_ptr + device_buffer as proposed.
- If it's in DRAINING state, the trigger STOP should be issued not at the point hw_ptr == appl_ptr but at the adjusted position like this proposal.
Then yet another question came up: what is the exact definition of runtime->delay. If this is supposed to be the time of the on-flight samples in the hardware device buffer (but not FIFO that can't be controlled by trigger on/off), we'd just need to apply runtime->delay for DRAINING position check. No need for the new additional field.
OTOH, if runtime->delay is the total delay including the hardware device buffer _and_ the hardware FIFO, a new field would be needed for the check above. Note that FIFO in this context is a kind of FIFO things that can't be stopped by the DMA controller. This is supposed to be small in usual cases, but not guaranteed to be so. For example, in ASoC abstraction, FIFO can be meant to be the delay in the codec side. This might be miliseconds long.
Takashi
participants (4)
-
Jaroslav Kysela -
Raymond Yau -
Takashi Iwai -
Vinod Koul