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

[PATCH 1/3] 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

[PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Charles Keepax]

The compress API for non-memory mapped DSPs shares a copy callback for
both read and write, however the file operation of write passes a const
buffer. Thus we can't maintain const correctness for the copy callback
and support both read and write.

This patch seperates the read and write callbacks in the compressed API.

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

diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index ff6c741..4c64dd1 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -99,7 +99,9 @@ struct snd_compr_stream {
  * @trigger: Trigger operations like start, pause, resume, drain, stop.
  * This callback is mandatory
  * @pointer: Retrieve current h/w pointer information. Mandatory
- * @copy: Copy the compressed data to/from userspace, Optional
+ * @write: Copy the compressed data from userspace, Optional
+ * Can't be implemented if DSP supports mmap
+ * @read: Copy the compressed data to userspace, Optional
  * Can't be implemented if DSP supports mmap
  * @mmap: DSP mmap method to mmap DSP memory
  * @ack: Ack for DSP when data is written to audio buffer, Optional
@@ -121,7 +123,9 @@ 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 (*write)(struct snd_compr_stream *stream, const char __user *buf,
+		       size_t count);
+	int (*read)(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 27bd81a..78c6f97 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -265,8 +265,8 @@ 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);
+	if (stream->ops->write)
+		retval = stream->ops->write(stream, buf, avail);
 	else
 		retval = snd_compr_write_data(stream, buf, avail);
 	if (retval > 0)
@@ -403,7 +403,7 @@ static int snd_compr_allocate_buffer(struct snd_compr_stream *stream,
 	void *buffer;
 
 	buffer_size = params->buffer.fragment_size * params->buffer.fragments;
-	if (stream->ops->copy) {
+	if (stream->ops->write) {
 		buffer = NULL;
 		/* if copy is defined the driver will be required to copy
 		 * the data from core
-- 
1.7.2.5

[PATCH 3/3] ASoC: soc-compress: Split copy into seperate read and write callbacks

[Charles Keepax]

The compress API for non-memory mapped DSPs shares a copy callback for
both read and write, however the file operation of write passes a const
buffer. Thus we can't maintain const correctness for the copy callback
and support both read and write.

This patch seperates the read and write callbacks in the ASoC compressed
API.

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

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index f9b2197..0148d82 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -306,8 +306,8 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
 	return 0;
 }
 
-static int soc_compr_copy(struct snd_compr_stream *cstream,
-			  const char __user *buf, size_t count)
+static int soc_compr_write(struct snd_compr_stream *cstream,
+			   const char __user *buf, size_t count)
 {
 	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
 	struct snd_soc_platform *platform = rtd->platform;
@@ -315,8 +315,24 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
 
 	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
 
-	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
-		ret = platform->driver->compr_ops->copy(cstream, buf, count);
+	if (platform->driver->compr_ops && platform->driver->compr_ops->write)
+		ret = platform->driver->compr_ops->write(cstream, buf, count);
+
+	mutex_unlock(&rtd->pcm_mutex);
+	return ret;
+}
+
+static int soc_compr_read(struct snd_compr_stream *cstream,
+			  char __user *buf, size_t count)
+{
+	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
+	struct snd_soc_platform *platform = rtd->platform;
+	int ret = 0;
+
+	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
+
+	if (platform->driver->compr_ops && platform->driver->compr_ops->read)
+		ret = platform->driver->compr_ops->read(cstream, buf, count);
 
 	mutex_unlock(&rtd->pcm_mutex);
 	return ret;
@@ -392,9 +408,13 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	}
 	memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
 
-	/* Add copy callback for not memory mapped DSPs */
-	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
-		compr->ops->copy = soc_compr_copy;
+	/* Add write/read callback for not memory mapped DSPs */
+	if (platform->driver->compr_ops) {
+		if (platform->driver->compr_ops->write)
+			compr->ops->write = soc_compr_write;
+		if (platform->driver->compr_ops->read)
+			compr->ops->read = soc_compr_read;
+	}
 
 	mutex_init(&compr->lock);
 	ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
-- 
1.7.2.5

Re: [PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Pierre-Louis Bossart]

On 04/03/2013 07:13 AM, Charles Keepax wrote:
> The compress API for non-memory mapped DSPs shares a copy callback for
> both read and write, however the file operation of write passes a const
> buffer. Thus we can't maintain const correctness for the copy callback
> and support both read and write.
>
> This patch seperates the read and write callbacks in the compressed API.

Why not remove the const to make this consistent with the PCM interface?
-Pierre

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

[Pierre-Louis Bossart]

On 04/03/2013 07:13 AM, 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.

This isn't very elegant. In your implementation you bypass app_ptr and 
hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr 
and hw_ptr everywhere else. This patch seems to make things more 
confused when they could be simpler without all these redundant fields? 
I am probably partly responsible for the introduction of these 
cumulative values, now I think the time has come to simplify things.
-Pierre

Re: [PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Charles Keepax]

On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
> Why not remove the const to make this consistent with the PCM interface?

I had been trying to avoid loosing the const from the write file
operation, however I am happy to just loose this through casting
instead. It is certainly a simpler change I will respin these to
do so.

Charles

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

[Charles Keepax]

On Thu, Apr 04, 2013 at 01:22:27PM -0500, Pierre-Louis Bossart wrote:
> This isn't very elegant. In your implementation you bypass app_ptr and  
> hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr  
> and hw_ptr everywhere else. This patch seems to make things more  
> confused when they could be simpler without all these redundant fields?  
> I am probably partly responsible for the introduction of these  
> cumulative values, now I think the time has come to simplify things.

I am not sure I agree this is less elegant it greatly simplifies
the calculation of the available data for one, also half of the
avail function was using them anyway. The cumulative values make
less assumptions about the underlying representation (although
admittedly it is rather unlikely this will be anything other than
a ring buffer) and contain more information (ie. how much data
has been transferred so far).

You say we use app_ptr and hw_ptr everywhere else but only in
places relating to situations where compress_offload is managing
the buffer (ie. memory mapped DSPs). They feel more like internal
buffer state, where the cumulative values surely reflect the
stream state better.

If anything if we were looking to simplify I would be inclined to
keep the cumulative values?

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

[Pierre-Louis Bossart]

On 4/5/13 3:36 AM, Charles Keepax wrote:
> On Thu, Apr 04, 2013 at 01:22:27PM -0500, Pierre-Louis Bossart wrote:
>> This isn't very elegant. In your implementation you bypass app_ptr and
>> hw_ptr to use cumulative values, for 'memory-mapped' DSPs we use app_ptr
>> and hw_ptr everywhere else. This patch seems to make things more
>> confused when they could be simpler without all these redundant fields?
>> I am probably partly responsible for the introduction of these
>> cumulative values, now I think the time has come to simplify things.
>
> I am not sure I agree this is less elegant it greatly simplifies
> the calculation of the available data for one, also half of the
> avail function was using them anyway. The cumulative values make
> less assumptions about the underlying representation (although
> admittedly it is rather unlikely this will be anything other than
> a ring buffer) and contain more information (ie. how much data
> has been transferred so far).
>
> You say we use app_ptr and hw_ptr everywhere else but only in
> places relating to situations where compress_offload is managing
> the buffer (ie. memory mapped DSPs). They feel more like internal
> buffer state, where the cumulative values surely reflect the
> stream state better.
>
> If anything if we were looking to simplify I would be inclined to
> keep the cumulative values?

That is my proposal as well, app_ptr and hw_ptr are defined as offsets 
but can't really be used to make the difference between buffer full and 
buffer empty and won't work for your implementation. I believe in the 
pcm case only cumulative values are used in the core.
Vinod, please chime in...
-Pierre

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

[Charles Keepax]

On Fri, Apr 05, 2013 at 09:51:08AM -0500, Pierre-Louis Bossart wrote:
> On 4/5/13 3:36 AM, Charles Keepax wrote:
>> If anything if we were looking to simplify I would be inclined to
>> keep the cumulative values?
>
> That is my proposal as well, app_ptr and hw_ptr are defined as offsets  
> but can't really be used to make the difference between buffer full and  
> buffer empty and won't work for your implementation. I believe in the  
> pcm case only cumulative values are used in the core.
> Vinod, please chime in...

Ah ok, I misunderstood. I will start having a look at what this
would take but wait for Vinod to give some feedback before I
upstream a new version.

Thanks,
Charles

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

[Vinod Koul]

On Fri, Apr 05, 2013 at 04:18:45PM +0100, Charles Keepax wrote:
> On Fri, Apr 05, 2013 at 09:51:08AM -0500, Pierre-Louis Bossart wrote:
> > On 4/5/13 3:36 AM, Charles Keepax wrote:
> >> If anything if we were looking to simplify I would be inclined to
> >> keep the cumulative values?
> >
> > That is my proposal as well, app_ptr and hw_ptr are defined as offsets  
> > but can't really be used to make the difference between buffer full and  
> > buffer empty and won't work for your implementation. I believe in the  
> > pcm case only cumulative values are used in the core.
> > Vinod, please chime in...
> 
> Ah ok, I misunderstood. I will start having a look at what this
> would take but wait for Vinod to give some feedback before I
> upstream a new version.
Okay, historical note :)

we can normally use offsets to work and do all the calculation in the core. But
this check fails when app_ptr = hw_ptr. Here we dont know if the buffer is full
or empty :) So we need to look at cummulative values for those checks, hence the
core maintains both of these and IIRC looks at cummulative values when
calucating avail only when we hit the above condition or probably in case when
we wrap over the ring buffer.

In both memory mapped and non mapped these cases are true so I dont see why this
caluclation is specfic for memory maped DSPs. There is nothing in this case
which relies. We only use the values reported by DSP and internal counters...

--
~Vinod

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

[Vinod Koul]

On Wed, Apr 03, 2013 at 01:13:27PM +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.
app pointer means what has been written into DSP buffer, no matter if its memory
mapped or not. This is updated after .copy returns so if you have been asked to
copy 100bytes, and return of .copy, app pointer is incremented by 100, so why is
that not true for you?

> 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 */
What is this patch based off< certainly not asoc/sound-next as I dont remember
this comment. IIRC comment was to test this for capture case!
> -
> -	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);
Neverthless, I agree that is simpler calculation...

> +	return avail->avail;
>  }
>  
>  static inline size_t snd_compr_get_avail(struct snd_compr_stream *stream)
> -- 
> 1.7.2.5
> 

Re: [PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Vinod Koul]

On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
> On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
> > Why not remove the const to make this consistent with the PCM interface?
> 
> I had been trying to avoid loosing the const from the write file
> operation, however I am happy to just loose this through casting
> instead. It is certainly a simpler change I will respin these to
> do so.
Keeping const of write seems to be a better idea... Ah i cant decide :(
I will asker wiser folks... Takashi, Mark?

--
~Vinod

Re: [PATCH 3/3] ASoC: soc-compress: Split copy into seperate read and write callbacks

[Vinod Koul]

On Wed, Apr 03, 2013 at 01:13:29PM +0100, Charles Keepax wrote:
> The compress API for non-memory mapped DSPs shares a copy callback for
> both read and write, however the file operation of write passes a const
> buffer. Thus we can't maintain const correctness for the copy callback
> and support both read and write.
This should be part of 2nd patch, you break bisect by not doing so...

> 
> This patch seperates the read and write callbacks in the ASoC compressed
> API.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  sound/soc/soc-compress.c |   34 +++++++++++++++++++++++++++-------
>  1 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
> index f9b2197..0148d82 100644
> --- a/sound/soc/soc-compress.c
> +++ b/sound/soc/soc-compress.c
> @@ -306,8 +306,8 @@ static int soc_compr_pointer(struct snd_compr_stream *cstream,
>  	return 0;
>  }
>  
> -static int soc_compr_copy(struct snd_compr_stream *cstream,
> -			  const char __user *buf, size_t count)
> +static int soc_compr_write(struct snd_compr_stream *cstream,
> +			   const char __user *buf, size_t count)
>  {
>  	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
>  	struct snd_soc_platform *platform = rtd->platform;
> @@ -315,8 +315,24 @@ static int soc_compr_copy(struct snd_compr_stream *cstream,
>  
>  	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
>  
> -	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
> -		ret = platform->driver->compr_ops->copy(cstream, buf, count);
> +	if (platform->driver->compr_ops && platform->driver->compr_ops->write)
> +		ret = platform->driver->compr_ops->write(cstream, buf, count);
> +
> +	mutex_unlock(&rtd->pcm_mutex);
> +	return ret;
> +}
> +
> +static int soc_compr_read(struct snd_compr_stream *cstream,
> +			  char __user *buf, size_t count)
> +{
> +	struct snd_soc_pcm_runtime *rtd = cstream->private_data;
> +	struct snd_soc_platform *platform = rtd->platform;
> +	int ret = 0;
> +
> +	mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
> +
> +	if (platform->driver->compr_ops && platform->driver->compr_ops->read)
> +		ret = platform->driver->compr_ops->read(cstream, buf, count);
>  
>  	mutex_unlock(&rtd->pcm_mutex);
>  	return ret;
> @@ -392,9 +408,13 @@ int soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
>  	}
>  	memcpy(compr->ops, &soc_compr_ops, sizeof(soc_compr_ops));
>  
> -	/* Add copy callback for not memory mapped DSPs */
> -	if (platform->driver->compr_ops && platform->driver->compr_ops->copy)
> -		compr->ops->copy = soc_compr_copy;
> +	/* Add write/read callback for not memory mapped DSPs */
> +	if (platform->driver->compr_ops) {
> +		if (platform->driver->compr_ops->write)
> +			compr->ops->write = soc_compr_write;
> +		if (platform->driver->compr_ops->read)
> +			compr->ops->read = soc_compr_read;
> +	}
>  
>  	mutex_init(&compr->lock);
>  	ret = snd_compress_new(rtd->card->snd_card, num, direction, compr);
> -- 
> 1.7.2.5
> 

Re: [PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Mark Brown]

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

On Tue, Apr 09, 2013 at 04:45:07PM +0530, Vinod Koul wrote:
> On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
> > On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:

> > > Why not remove the const to make this consistent with the PCM interface?

> > I had been trying to avoid loosing the const from the write file
> > operation, however I am happy to just loose this through casting
> > instead. It is certainly a simpler change I will respin these to
> > do so.

> Keeping const of write seems to be a better idea... Ah i cant decide :(
> I will asker wiser folks... Takashi, Mark?

I generally prefer the const correctness, but on the other hand Pierre
is right about the PCM interface and it's probably more trouble than
it's worth to change that.  So meh, dunno :/

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

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

Re: [PATCH 2/3] ALSA: compress_core: Split copy into seperate read and write callbacks

[Takashi Iwai]

At Tue, 9 Apr 2013 12:47:28 +0100,
Mark Brown wrote:
> 
> [1  <text/plain; us-ascii (7bit)>]
> On Tue, Apr 09, 2013 at 04:45:07PM +0530, Vinod Koul wrote:
> > On Fri, Apr 05, 2013 at 09:23:45AM +0100, Charles Keepax wrote:
> > > On Thu, Apr 04, 2013 at 01:01:57PM -0500, Pierre-Louis Bossart wrote:
> 
> > > > Why not remove the const to make this consistent with the PCM interface?
> 
> > > I had been trying to avoid loosing the const from the write file
> > > operation, however I am happy to just loose this through casting
> > > instead. It is certainly a simpler change I will respin these to
> > > do so.
> 
> > Keeping const of write seems to be a better idea... Ah i cant decide :(
> > I will asker wiser folks... Takashi, Mark?
> 
> I generally prefer the const correctness, but on the other hand Pierre
> is right about the PCM interface and it's probably more trouble than
> it's worth to change that.  So meh, dunno :/

I'm inclined to take an easier approach, just removing const, in such
a case.

If we do split to read & write, it should be done for PCM as well.
There are only handful users, so it shouldn't be too hard.


Takashi

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

[Pierre-Louis Bossart]

> we can normally use offsets to work and do all the calculation in the core. But
> this check fails when app_ptr = hw_ptr. Here we dont know if the buffer is full
> or empty :) So we need to look at cummulative values for those checks, hence the
> core maintains both of these and IIRC looks at cummulative values when
> calucating avail only when we hit the above condition or probably in case when
> we wrap over the ring buffer.
>
> In both memory mapped and non mapped these cases are true so I dont see why this
> caluclation is specfic for memory maped DSPs. There is nothing in this case
> which relies. We only use the values reported by DSP and internal counters...

the question was whether we need these offsets at all. Can't we just use 
cumulative values everywhere?
-Pierre

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

[Charles Keepax]

On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
> On Wed, Apr 03, 2013 at 01:13:27PM +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.
> app pointer means what has been written into DSP buffer, no matter if its memory
> mapped or not. This is updated after .copy returns so if you have been asked to
> copy 100bytes, and return of .copy, app pointer is incremented by 100, so why is
> that not true for you?

Currently though app_pointer is not updated after a copy callback
the cumulative values are updated, but not app_pointer.
app_pointer is only updated in the case were we don't have a copy
callback and the buffer is managed by the compressed core.

I am quite happy to change that or remove this comment from the change
log if we prefer but as it currently stands app_pointer will not
be updated for systems that use a copy callback by the compress
core.

Charles

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

[Charles Keepax]

On Tue, Apr 09, 2013 at 08:59:48PM -0700, Pierre-Louis Bossart wrote:
> the question was whether we need these offsets at all. Can't we just use  
> cumulative values everywhere?
> -Pierre
>

Removing them does introduce a modulus operation which is not
very nice given we are dealing with all 64-bit values. Although
limiting the buffer size to 32-bit would make that slightly
neater.

Aside from that the change is trivial to make and does match how
the PCM interface is handled.

Charles

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

[Charles Keepax]

On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
> What is this patch based off< certainly not asoc/sound-next as I dont remember
> this comment. IIRC comment was to test this for capture case!

This patch is based off Mark's asoc/for-next tree, apologies I
guess I should have based this of Takashi's for-next tree.
Although I wouldn't have thought they would have differed here as
there haven't been any updates recently appart from the meta-data
stuff.

Charles

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

[Mark Brown]

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

On Wed, Apr 10, 2013 at 11:07:57AM +0100, Charles Keepax wrote:
> On Tue, Apr 09, 2013 at 04:32:30PM +0530, Vinod Koul wrote:
> > What is this patch based off< certainly not asoc/sound-next as I dont remember
> > this comment. IIRC comment was to test this for capture case!

> This patch is based off Mark's asoc/for-next tree, apologies I
> guess I should have based this of Takashi's for-next tree.
> Although I wouldn't have thought they would have differed here as
> there haven't been any updates recently appart from the meta-data
> stuff.

Thie FIXMEs appear to come from your initial commit, must've been dealt
with since.

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

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