[alsa-devel] [PATCH 0/7] Add capture support to compress API
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(-)
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)
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
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?
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.
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; }
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;
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 {
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");
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;
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; }
participants (4)
-
Charles Keepax -
Mark Brown -
Richard Fitzgerald -
Vinod Koul