[PATCH 0/3] introduce device_buffer

[PATCH 0/3] introduce device_buffer

[Vinod Koul]

Old wine ina  new bottle, so here is the second attempt for adding the notion of
how devices (in my case a DSP) can buffer data and that can be used by sound
core

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              |    7 ++++++-
 sound/core/pcm_lib.c             |   14 +++++++++++---
 sound/core/pcm_native.c          |    6 +++---
 sound/soc/mid-x86/sst_platform.c |   23 +++++++++++++++++++++++
 sound/soc/mid-x86/sst_platform.h |    4 +++-
 sound/soc/soc-pcm.c              |   13 +++++++++++++
 8 files changed, 66 insertions(+), 8 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 memry availble.
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    |   14 +++++++++++---
 sound/core/pcm_native.c |    6 +++---
 3 files changed, 15 insertions(+), 6 deletions(-)

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..1d15431 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;
 		}
@@ -440,9 +448,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream,
 	if (runtime->hw.info & SNDRV_PCM_INFO_BATCH)
 		goto no_jiffies_check;
 	hdelta = delta;
-	if (hdelta < runtime->delay)
+	if (hdelta < (runtime->delay + runtime->device_buffer))
 		goto no_jiffies_check;
-	hdelta -= runtime->delay;
+	hdelta -= (runtime->delay + runtime->device_buffer);
 	jdelta = curr_jiffies - runtime->hw_ptr_jiffies;
 	if (((hdelta * HZ) / runtime->rate) > jdelta + HZ/100) {
 		delta = jdelta /
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 53b5ada..33f1afa 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -606,13 +606,13 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
 		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
 			status->delay = runtime->buffer_size - status->avail;
-			status->delay += runtime->delay;
+			status->delay += runtime->delay + runtime->device_buffer;
 		} else
 			status->delay = 0;
 	} else {
 		status->avail = snd_pcm_capture_avail(runtime);
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
-			status->delay = status->avail + runtime->delay;
+			status->delay = status->avail + runtime->delay + runtime->device_buffer;
 		else
 			status->delay = 0;
 	}
@@ -2442,7 +2442,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
 			n = snd_pcm_playback_hw_avail(runtime);
 		else
 			n = snd_pcm_capture_avail(runtime);
-		n += runtime->delay;
+		n += runtime->delay + runtime->device_buffer;
 		break;
 	case SNDRV_PCM_STATE_XRUN:
 		err = -EPIPE;
-- 
1.7.9.5

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

[Vinod Koul]

Signed-off-by: Vinod Koul <vinod.koul@linux.intel.com>
---
 include/sound/soc-dai.h |    6 ++++++
 include/sound/soc.h     |    7 ++++++-
 sound/soc/soc-pcm.c     |   13 +++++++++++++
 3 files changed, 25 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..0d5046b 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -795,7 +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..cd7b10f 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,19 @@ static snd_pcm_uframes_t soc_pcm_pointer(struct snd_pcm_substream *substream)
 	if (platform->driver->delay)
 		delay += platform->driver->delay(substream, codec_dai);
 
+	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;
+	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 0/3] introduce device_buffer

[Mark Brown]

On Fri, Aug 31, 2012 at 12:42:35AM +0530, Vinod Koul wrote:
> Old wine ina  new bottle, so here is the second attempt for adding the notion of
> how devices (in my case a DSP) can buffer data and that can be used by sound
> core

This all looks good for me.

[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