[PATCH v3 0/3] introduce device_buffer

[PATCH v3 0/3] introduce device_buffer

[Vinod Koul]

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(-)

-- 
1.7.9.5

[PATCH 1/3] ALSA: pcm - introduce device_buffer

[Vinod Koul]

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;
 		}
-- 
1.7.9.5

Re: [PATCH 1/3] ALSA: pcm - introduce device_buffer

[Raymond Yau]

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 ?
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] ALSA: pcm - introduce device_buffer

[Vinod Koul]

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

-- 
~Vinod

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] ALSA: pcm - introduce device_buffer

[Jaroslav Kysela]

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;
> +		else
> +			actual_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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH 1/3] ALSA: pcm - introduce device_buffer

[Vinod Koul]

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;
> > +		else
> > +			actual_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
> 


-- 
~Vinod

[PATCH 2/3] ASoC: add device_buffer in asoc

[Vinod Koul]

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;
 }
-- 
1.7.9.5

[PATCH 3/3] ASoC: mid-x86 - implement buffer query in sst_platform driver

[Vinod Koul]

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 {
-- 
1.7.9.5

Re: [PATCH v3 0/3] introduce device_buffer

[Jaroslav Kysela]

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(-)
> 


-- 
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project; Red Hat, Inc.

Re: [PATCH v3 0/3] introduce device_buffer

[Vinod Koul]

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

-- 
~Vinod

Re: [PATCH v3 0/3] introduce device_buffer

[Takashi Iwai]

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