[PATCH 0/7] Add capture support to compress API

[PATCH 0/7] Add capture support to compress API

[Charles Keepax]

This chain of patches does some clearing up and add capture
support to the compressed API.

Missing is support for capture streams that don't use the
copy callback. Also I have included two patches at the end
of the chain that remove the buffer pointers and just use
the cumulative values however as our setup uses the copy
callback these are untested past build so should be taken
with caution.

Thanks,
Charles

Charles Keepax (7):
  ALSA: compress_core: Update calc_avail to use cumulative values
  ALSA: compress_core: Calculate avail correctly for capture streams
  ALSA: compress_core: Deconstify copy callback buffer
  ALSA: compress_core: Add support for capture streams
  ASoC: soc-compress: Deduce stream direction
  ALSA: compress_core: Remove unused hw_pointer
  ALSA: compress_core: Rework writes to use cumulative values

 include/sound/compress_driver.h |    4 +-
 sound/core/compress_offload.c   |  117 +++++++++++++++++++++++++-------------
 sound/soc/soc-compress.c        |   11 +++-
 3 files changed, 87 insertions(+), 45 deletions(-)

-- 
1.7.2.5

[PATCH 1/7] ALSA: compress_core: Update calc_avail to use cumulative values

[Charles Keepax]

The app_pointer is managed locally by the compress core for memory
mapped DSPs but for DSPs that are not memory mapped this would have to
be manually updated from within the DSP driver itself, which is hardly
very idiomatic.

This patch switches to using the cumulative values to calculate the
available buffer space because these are already gracefully passed out
of the DSP driver to the compress core and otherwise should be
functionally equivalent.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c |   23 +++++------------------
 1 files changed, 5 insertions(+), 18 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index c84abc8..27bd81a 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -160,8 +160,6 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 		struct snd_compr_avail *avail)
 {
-	long avail_calc; /*this needs to be signed variable */
-
 	memset(avail, 0, sizeof(*avail));
 	snd_compr_update_tstamp(stream, &avail->tstamp);
 	/* Still need to return avail even if tstamp can't be filled in */
@@ -184,22 +182,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 		return stream->runtime->buffer_size;
 	}
 
-	/* FIXME: this routine isn't consistent, in one test we use
-	 * cumulative values and in the other byte offsets. Do we
-	 * really need the byte offsets if the cumulative values have
-	 * been updated? In the PCM interface app_ptr and hw_ptr are
-	 * already cumulative */
-
-	avail_calc = stream->runtime->buffer_size -
-		(stream->runtime->app_pointer - stream->runtime->hw_pointer);
-	pr_debug("calc avail as %ld, app_ptr %lld, hw+ptr %lld\n", avail_calc,
-			stream->runtime->app_pointer,
-			stream->runtime->hw_pointer);
-	if (avail_calc >= stream->runtime->buffer_size)
-		avail_calc -= stream->runtime->buffer_size;
-	pr_debug("ret avail as %ld\n", avail_calc);
-	avail->avail = avail_calc;
-	return avail_calc;
+	avail->avail = stream->runtime->buffer_size -
+		(stream->runtime->total_bytes_available -
+		 stream->runtime->total_bytes_transferred);
+	pr_debug("ret avail as %lld\n", avail->avail);
+	return avail->avail;
 }
 
 static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
-- 
1.7.2.5

Re: [PATCH 1/7] ALSA: compress_core: Update calc_avail to use cumulative values

[Vinod Koul]

On Thu, Apr 11, 2013 at 07:00:39PM +0100, Charles Keepax wrote:
> The app_pointer is managed locally by the compress core for memory
> mapped DSPs but for DSPs that are not memory mapped this would have to
> be manually updated from within the DSP driver itself, which is hardly
> very idiomatic.
> 
> This patch switches to using the cumulative values to calculate the
> available buffer space because these are already gracefully passed out
> of the DSP driver to the compress core and otherwise should be
> functionally equivalent.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  sound/core/compress_offload.c |   23 +++++------------------
>  1 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index c84abc8..27bd81a 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -160,8 +160,6 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
>  static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		struct snd_compr_avail *avail)
>  {
> -	long avail_calc; /*this needs to be signed variable */
> -
>  	memset(avail, 0, sizeof(*avail));
>  	snd_compr_update_tstamp(stream, &avail->tstamp);
>  	/* Still need to return avail even if tstamp can't be filled in */
> @@ -184,22 +182,11 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
>  		return stream->runtime->buffer_size;
>  	}
>  
> -	/* FIXME: this routine isn't consistent, in one test we use
> -	 * cumulative values and in the other byte offsets. Do we
> -	 * really need the byte offsets if the cumulative values have
> -	 * been updated? In the PCM interface app_ptr and hw_ptr are
> -	 * already cumulative */
As I said last time these doesn't seem to generated against either Takashi's or
Mark's public tree. This comment is as below on all the public trees I have seen
	/* FIXME: This needs to be different for capture stream,
	   available is # of compressed data, for playback it's remainder of buffer */

As a result I am unable to apply & test this on any of the trees I work with :(
Pls clone either of these and regenerate..

> -
> -	avail_calc = stream->runtime->buffer_size -
> -		(stream->runtime->app_pointer - stream->runtime->hw_pointer);
> -	pr_debug("calc avail as %ld, app_ptr %lld, hw+ptr %lld\n", avail_calc,
> -			stream->runtime->app_pointer,
> -			stream->runtime->hw_pointer);
> -	if (avail_calc >= stream->runtime->buffer_size)
> -		avail_calc -= stream->runtime->buffer_size;
> -	pr_debug("ret avail as %ld\n", avail_calc);
> -	avail->avail = avail_calc;
> -	return avail_calc;
> +	avail->avail = stream->runtime->buffer_size -
> +		(stream->runtime->total_bytes_available -
> +		 stream->runtime->total_bytes_transferred);
> +	pr_debug("ret avail as %lld\n", avail->avail);
> +	return avail->avail;
>  }
>  
>  static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
> -- 
> 1.7.2.5
> 

Re: [PATCH 1/7] ALSA: compress_core: Update calc_avail to use cumulative values

[Richard Fitzgerald]

>As I said last time these doesn't seem to generated against either Takashi's or
>Mark's public tree.
>
>As a result I am unable to apply & test this on any of the trees I work with :(
>Pls clone either of these and regenerate..

I've just git-am'd the patch no problem onto Takashi's for-next tree.
What repo & branch are you trying to apply to?

Re: [PATCH 1/7] ALSA: compress_core: Update calc_avail to use cumulative values

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 825 bytes --]

On Mon, Apr 15, 2013 at 09:43:54PM +0530, Vinod Koul wrote:

> As I said last time these doesn't seem to generated against either Takashi's or
> Mark's public tree. This comment is as below on all the public trees I have seen
> 	/* FIXME: This needs to be different for capture stream,
> 	   available is # of compressed data, for playback it's remainder of buffer */
> 
> As a result I am unable to apply & test this on any of the trees I work with :(
> Pls clone either of these and regenerate..

Richard did say (dropping CCs, grumble) that these applied to Takashi's
tree but as he's now merged in the ASoC stuff for this release cycle
they'll need to be respun against his for-next or the asoc-v3.10 tag.
For the avoidance of confusion I'd suggest reposting explicitly
specifying what they're  based off.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

[PATCH 2/7] ALSA: compress_core: Calculate avail correctly for capture streams

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c |   29 ++++++++++++++++++-----------
 1 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 27bd81a..1f69863 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -153,7 +153,10 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 	pr_debug("dsp consumed till %d total %d bytes\n",
 		tstamp->byte_offset, tstamp->copied_total);
 	stream->runtime->hw_pointer = tstamp->byte_offset;
-	stream->runtime->total_bytes_transferred = tstamp->copied_total;
+	if (stream->direction == SND_COMPRESS_PLAYBACK)
+		stream->runtime->total_bytes_transferred = tstamp->copied_total;
+	else
+		stream->runtime->total_bytes_available = tstamp->copied_total;
 	return 0;
 }
 
@@ -164,12 +167,9 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 	snd_compr_update_tstamp(stream, &avail->tstamp);
 	/* Still need to return avail even if tstamp can't be filled in */
 
-	/* FIXME: This needs to be different for capture stream,
-	   available is # of compressed data, for playback it's
-	   remainder of buffer */
-
 	if (stream->runtime->total_bytes_available == 0 &&
-			stream->runtime->state == SNDRV_PCM_STATE_SETUP) {
+			stream->runtime->state == SNDRV_PCM_STATE_SETUP &&
+			stream->direction == SND_COMPRESS_PLAYBACK) {
 		pr_debug("detected init and someone forgot to do a write\n");
 		return stream->runtime->buffer_size;
 	}
@@ -178,13 +178,20 @@ static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
 			stream->runtime->total_bytes_transferred);
 	if (stream->runtime->total_bytes_available ==
 				stream->runtime->total_bytes_transferred) {
-		pr_debug("both pointers are same, returning full avail\n");
-		return stream->runtime->buffer_size;
+		if (stream->direction == SND_COMPRESS_PLAYBACK) {
+			pr_debug("both pointers are same, returning full avail\n");
+			return stream->runtime->buffer_size;
+		} else {
+			pr_debug("both pointers are same, returning no avail\n");
+			return 0;
+		}
 	}
 
-	avail->avail = stream->runtime->buffer_size -
-		(stream->runtime->total_bytes_available -
-		 stream->runtime->total_bytes_transferred);
+	avail->avail = stream->runtime->total_bytes_available -
+			stream->runtime->total_bytes_transferred;
+	if (stream->direction == SND_COMPRESS_PLAYBACK)
+		avail->avail = stream->runtime->buffer_size - avail->avail;
+
 	pr_debug("ret avail as %lld\n", avail->avail);
 	return avail->avail;
 }
-- 
1.7.2.5

[PATCH 3/7] ALSA: compress_core: Deconstify copy callback buffer

[Charles Keepax]

The buffer passed to the copy callback should not be const because the
copy callback can be used for capture and playback.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 include/sound/compress_driver.h |    2 +-
 sound/core/compress_offload.c   |    8 +++++---
 sound/soc/soc-compress.c        |    2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index ff6c741..db8273a 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -121,7 +121,7 @@ struct snd_compr_ops {
 	int (*trigger)(struct snd_compr_stream *stream, int cmd);
 	int (*pointer)(struct snd_compr_stream *stream,
 			struct snd_compr_tstamp *tstamp);
-	int (*copy)(struct snd_compr_stream *stream, const char __user *buf,
+	int (*copy)(struct snd_compr_stream *stream, char __user *buf,
 		       size_t count);
 	int (*mmap)(struct snd_compr_stream *stream,
 			struct vm_area_struct *vma);
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 1f69863..52ca4cc 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -272,10 +272,12 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 	if (avail > count)
 		avail = count;
 
-	if (stream->ops->copy)
-		retval = stream->ops->copy(stream, buf, avail);
-	else
+	if (stream->ops->copy) {
+		char __user* cbuf = (char __user*)buf;
+		retval = stream->ops->copy(stream, cbuf, avail);
+	} else {
 		retval = snd_compr_write_data(stream, buf, avail);
+	}
 	if (retval > 0)
 		stream->runtime->total_bytes_available += retval;
 
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index f9b2197..d3f03f6 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -307,7 +307,7 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 }
 
 static int soc_compr_copy(struct snd_compr_stream *cstream,
-			  const char __user *buf, size_t count)
+			  char __user *buf, size_t count)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
-- 
1.7.2.5

[PATCH 4/7] ALSA: compress_core: Add support for capture streams

[Charles Keepax]

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/core/compress_offload.c |   43 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 52ca4cc..d0fc9a1 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -296,7 +296,41 @@ static ssize_t snd_compr_write(struct file *f, const char __user *buf,
 static ssize_t snd_compr_read(struct file *f, char __user *buf,
 		size_t count, loff_t *offset)
 {
-	return -ENXIO;
+	struct snd_compr_file *data = f->private_data;
+	struct snd_compr_stream *stream;
+	size_t avail;
+	int retval;
+
+	if (snd_BUG_ON(!data))
+		return -EFAULT;
+
+	stream = &data->stream;
+	mutex_lock(&stream->device->lock);
+
+	/* read is allowed when stream is running */
+	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING) {
+		retval = -EBADFD;
+		goto out;
+	}
+
+	avail = snd_compr_get_avail(stream);
+	pr_debug("avail returned %ld\n", (unsigned long)avail);
+	/* calculate how much we can read from buffer */
+	if (avail > count)
+		avail = count;
+
+	if (stream->ops->copy) {
+		retval = stream->ops->copy(stream, buf, avail);
+	} else {
+		retval = -ENXIO;
+		goto out;
+	}
+	if (retval > 0)
+		stream->runtime->total_bytes_transferred += retval;
+
+out:
+	mutex_unlock(&stream->device->lock);
+	return retval;
 }
 
 static int snd_compr_mmap(struct file *f, struct vm_area_struct *vma)
@@ -481,7 +515,12 @@ snd_compr_set_params(struct snd_compr_stream *stream, unsigned long arg)
 		retval = stream->ops->set_params(stream, params);
 		if (retval)
 			goto out;
-		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+
+		if (stream->direction == SND_COMPRESS_PLAYBACK)
+			stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+		else
+			stream->runtime->state = SNDRV_PCM_STATE_PREPARED;
+
 		stream->metadata_set = false;
 		stream->next_track = false;
 	} else {
-- 
1.7.2.5

[PATCH 5/7] ASoC: soc-compress: Deduce stream direction

[Charles Keepax]

Previously we just hard coded all streams as playback streams, this
patch checks the DAI to see if it is a capture or playback stream. It is
worth noting that at this time only unidirectional streams are
supported.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 sound/soc/soc-compress.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index d3f03f6..e16ce86 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -376,7 +376,14 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	/* check client and interface hw capabilities */
 	snprintf(new_name, sizeof(new_name), "%s %s-%d",
 			rtd->dai_link->stream_name, codec_dai->name, num);
-	direction = SND_COMPRESS_PLAYBACK;
+
+	if (codec_dai->driver->playback.channels_min)
+		direction = SND_COMPRESS_PLAYBACK;
+	else if (codec_dai->driver->capture.channels_min)
+		direction = SND_COMPRESS_CAPTURE;
+	else
+		return -EINVAL;
+
 	compr = kzalloc(sizeof(*compr), GFP_KERNEL);
 	if (compr == NULL) {
 		snd_printk(KERN_ERR "Cannot allocate compr\n");
-- 
1.7.2.5

[PATCH 6/7] ALSA: compress_core: Remove unused hw_pointer

[Charles Keepax]

Only tested as far as build.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 include/sound/compress_driver.h |    1 -
 sound/core/compress_offload.c   |    2 --
 2 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index db8273a..2d7de96 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -56,7 +56,6 @@ struct snd_compr_runtime {
 	u64 buffer_size;
 	u32 fragment_size;
 	u32 fragments;
-	u64 hw_pointer;
 	u64 app_pointer;
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index d0fc9a1..87f944c 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -152,7 +152,6 @@ static int snd_compr_update_tstamp(struct snd_compr_stream *stream,
 	stream->ops->pointer(stream, tstamp);
 	pr_debug("dsp consumed till %d total %d bytes\n",
 		tstamp->byte_offset, tstamp->copied_total);
-	stream->runtime->hw_pointer = tstamp->byte_offset;
 	if (stream->direction == SND_COMPRESS_PLAYBACK)
 		stream->runtime->total_bytes_transferred = tstamp->copied_total;
 	else
@@ -657,7 +656,6 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
-		stream->runtime->hw_pointer = 0;
 		stream->runtime->app_pointer = 0;
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
-- 
1.7.2.5

[PATCH 7/7] ALSA: compress_core: Rework writes to use cumulative values

[Charles Keepax]

This patch reworks the writes to use cumulative values thus making the
app_pointer unecessary and removing it.

Only tested as far as build.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 include/sound/compress_driver.h |    1 -
 sound/core/compress_offload.c   |   18 +++++++++++-------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 2d7de96..9031a26 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -56,7 +56,6 @@ struct snd_compr_runtime {
 	u64 buffer_size;
 	u32 fragment_size;
 	u32 fragments;
-	u64 app_pointer;
 	u64 total_bytes_available;
 	u64 total_bytes_transferred;
 	wait_queue_head_t sleep;
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 87f944c..38d42af 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -28,11 +28,13 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/list.h>
+#include <linux/math64.h>
 #include <linux/mm.h>
 #include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
+#include <linux/types.h>
 #include <linux/uio.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
@@ -223,21 +225,24 @@ static int snd_compr_write_data(struct snd_compr_stream *stream,
 	void *dstn;
 	size_t copy;
 	struct snd_compr_runtime *runtime = stream->runtime;
+	/* 64-bit Modulus */
+	u64 app_pointer = div64_u64(runtime->total_bytes_available,
+				    runtime->buffer_size);
+	app_pointer = runtime->total_bytes_available -
+		      (app_pointer * runtime->buffer_size);
 
-	dstn = runtime->buffer + runtime->app_pointer;
+	dstn = runtime->buffer + app_pointer;
 	pr_debug("copying %ld at %lld\n",
-			(unsigned long)count, runtime->app_pointer);
-	if (count < runtime->buffer_size - runtime->app_pointer) {
+			(unsigned long)count, app_pointer);
+	if (count < runtime->buffer_size - app_pointer) {
 		if (copy_from_user(dstn, buf, count))
 			return -EFAULT;
-		runtime->app_pointer += count;
 	} else {
-		copy = runtime->buffer_size - runtime->app_pointer;
+		copy = runtime->buffer_size - app_pointer;
 		if (copy_from_user(dstn, buf, copy))
 			return -EFAULT;
 		if (copy_from_user(runtime->buffer, buf + copy, count - copy))
 			return -EFAULT;
-		runtime->app_pointer = count - copy;
 	}
 	/* if DSP cares, let it know data has been written */
 	if (stream->ops->ack)
@@ -656,7 +661,6 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
 	if (!retval) {
 		stream->runtime->state = SNDRV_PCM_STATE_SETUP;
 		wake_up(&stream->runtime->sleep);
-		stream->runtime->app_pointer = 0;
 		stream->runtime->total_bytes_available = 0;
 		stream->runtime->total_bytes_transferred = 0;
 	}
-- 
1.7.2.5