[PATCH 1/2] dmaengine: Add wrapper for device_tx_status callback

[PATCH 1/2] dmaengine: Add wrapper for device_tx_status callback

[Lars-Peter Clausen]

This patch adds a small inline wrapper for the devivce_tx_status callback of a
dma device. This makes the source code of users of this function a bit more
compact and a bit more legible.

E.g.:
-status = chan->device->device_tx_status(chan, cookie, &state)
+status = dmaengine_tx_status(chan, cookie, &state)

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 include/linux/dmaengine.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 56bbc4d..1731628 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -650,6 +650,12 @@ static inline int dmaengine_resume(struct dma_chan *chan)
 	return dmaengine_device_control(chan, DMA_RESUME, 0);
 }
 
+static inline enum dma_status dmaengine_tx_status(struct dma_chan *chan,
+	dma_cookie_t cookie, struct dma_tx_state *state)
+{
+	return chan->device->device_tx_status(chan, cookie, state);
+}
+
 static inline dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc)
 {
 	return desc->tx_submit(desc);
-- 
1.7.10

[PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Lars-Peter Clausen]

Currently the sound dmaengine pcm helper functions implement the pcm_pointer
callback by trying to count the number of elapsed periods. This is done by
advancing the stream position in the dmaengine callback by one period.
Unfortunately there is no guarantee that the callback will be called for each
elapsed period. It may be possible that under high system load it is only called
once for multiple elapsed periods. This patch addresses the issue by
implementing support for querying the current stream position directly from the
dmaengine device. Since not all dmaengine drivers support reporting the stream
position yet the old period counting mechanism is kept as a fallback.

Furthermore the new mechanism allows to report the stream position with a
sub-period granularity, given that the dmaengine driver supports this.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/soc-dmaengine-pcm.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-dmaengine-pcm.c b/sound/soc/soc-dmaengine-pcm.c
index 643147e..1c754a7 100644
--- a/sound/soc/soc-dmaengine-pcm.c
+++ b/sound/soc/soc-dmaengine-pcm.c
@@ -30,6 +30,7 @@
 
 struct dmaengine_pcm_runtime_data {
 	struct dma_chan *dma_chan;
+	dma_cookie_t cookie;
 
 	unsigned int pos;
 
@@ -146,7 +147,7 @@ static int dmaengine_pcm_prepare_and_submit(struct snd_pcm_substream *substream)
 
 	desc->callback = dmaengine_pcm_dma_complete;
 	desc->callback_param = substream;
-	dmaengine_submit(desc);
+	prtd->cookie = dmaengine_submit(desc);
 
 	return 0;
 }
@@ -202,7 +203,27 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
 snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
 {
 	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
-	return bytes_to_frames(substream->runtime, prtd->pos);
+	struct dma_tx_state state;
+	enum dma_status status;
+	unsigned int pos;
+
+	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
+	if (status != DMA_IN_PROGRESS && status != DMA_PAUSED) {
+		pos = 0;
+	} else if (state.residue == 0) {
+		/* This should never happen with cyclic transfers, so assume
+		 * that the dmaengine driver does not support reporting residue
+		 * and fall back to counting periods. */
+		pos = prtd->pos;
+	} else {
+		pos = snd_pcm_lib_buffer_bytes(substream);
+		if (state.residue <= pos)
+			pos -= state.residue;
+		else
+			pos = 0;
+	}
+
+	return bytes_to_frames(substream->runtime, pos);
 }
 EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_pointer);
 
-- 
1.7.10

Re: [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 02:04:13PM +0200, Lars-Peter Clausen wrote:
> @@ -202,7 +203,27 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
>  {
>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> -	return bytes_to_frames(substream->runtime, prtd->pos);
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	unsigned int pos;
> +
> +	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
> +	if (status != DMA_IN_PROGRESS && status != DMA_PAUSED) {
> +		pos = 0;
> +	} else if (state.residue == 0) {
> +		/* This should never happen with cyclic transfers, so assume
> +		 * that the dmaengine driver does not support reporting residue
> +		 * and fall back to counting periods. */
> +		pos = prtd->pos;

That is an incorrect assumption.

The current DMA position is defined by the DMA pointer.  When the DMA
engine has transferred the last few bytes of the DMA, the DMA pointer
will be pointing to the byte _after_ the last byte transferred in the
buffer.

Therefore, when you calculate the residue as (buf_start + buf_len -
current_pointer) you end up with zero.

What we need to do is to get rid of this idea that reporting the residue
is optional for DMA engine drivers.  Let's make it absolutely required
in order to support cyclic transfers.

Re: [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Lars-Peter Clausen]

On 06/11/2012 03:24 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 11, 2012 at 02:04:13PM +0200, Lars-Peter Clausen wrote:
>> @@ -202,7 +203,27 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
>>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
>>  {
>>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
>> -	return bytes_to_frames(substream->runtime, prtd->pos);
>> +	struct dma_tx_state state;
>> +	enum dma_status status;
>> +	unsigned int pos;
>> +
>> +	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
>> +	if (status != DMA_IN_PROGRESS && status != DMA_PAUSED) {
>> +		pos = 0;
>> +	} else if (state.residue == 0) {
>> +		/* This should never happen with cyclic transfers, so assume
>> +		 * that the dmaengine driver does not support reporting residue
>> +		 * and fall back to counting periods. */
>> +		pos = prtd->pos;
> 
> That is an incorrect assumption.
> 
> The current DMA position is defined by the DMA pointer.  When the DMA
> engine has transferred the last few bytes of the DMA, the DMA pointer
> will be pointing to the byte _after_ the last byte transferred in the
> buffer.
> 
> Therefore, when you calculate the residue as (buf_start + buf_len -
> current_pointer) you end up with zero.

My assumption is that for cyclic DMA it will be pointing to the first byte
again after the last byte has been transferred. Something like this:

offset = (offset + 1) % buf_len
and current_pointer = buf_start + offset

If this is not true for a particular DMA controller I think we should still
make sure in the driver for this controller that residue will never be zero
for cyclic transfers, but instead will return the buffer size. This is the
only thing that really makes sense. The buffer only has buf_len bytes, if
residue can be a value in the interval of [0,buf_len] this means that it has
buf_len+1 possible values. Which again means there must be two values which
map to the same state. In this case it would be 0 and buf_len.

> 
> What we need to do is to get rid of this idea that reporting the residue
> is optional for DMA engine drivers.  Let's make it absolutely required
> in order to support cyclic transfers.

While I agree, I don't think we can just rip out the old mechanism until the
platforms which are currently using this have fixed their dmaengine drivers
to provide residue.

- Lars

Re: [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Russell King - ARM Linux]

On Mon, Jun 11, 2012 at 04:02:42PM +0200, Lars-Peter Clausen wrote:
> On 06/11/2012 03:24 PM, Russell King - ARM Linux wrote:
> > On Mon, Jun 11, 2012 at 02:04:13PM +0200, Lars-Peter Clausen wrote:
> >> @@ -202,7 +203,27 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
> >>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
> >>  {
> >>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
> >> -	return bytes_to_frames(substream->runtime, prtd->pos);
> >> +	struct dma_tx_state state;
> >> +	enum dma_status status;
> >> +	unsigned int pos;
> >> +
> >> +	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
> >> +	if (status != DMA_IN_PROGRESS && status != DMA_PAUSED) {
> >> +		pos = 0;
> >> +	} else if (state.residue == 0) {
> >> +		/* This should never happen with cyclic transfers, so assume
> >> +		 * that the dmaengine driver does not support reporting residue
> >> +		 * and fall back to counting periods. */
> >> +		pos = prtd->pos;
> > 
> > That is an incorrect assumption.
> > 
> > The current DMA position is defined by the DMA pointer.  When the DMA
> > engine has transferred the last few bytes of the DMA, the DMA pointer
> > will be pointing to the byte _after_ the last byte transferred in the
> > buffer.
> > 
> > Therefore, when you calculate the residue as (buf_start + buf_len -
> > current_pointer) you end up with zero.
> 
> My assumption is that for cyclic DMA it will be pointing to the first byte
> again after the last byte has been transferred. Something like this:
> 
> offset = (offset + 1) % buf_len
> and current_pointer = buf_start + offset
> 
> If this is not true for a particular DMA controller I think we should still
> make sure in the driver for this controller that residue will never be zero
> for cyclic transfers, but instead will return the buffer size. This is the
> only thing that really makes sense. The buffer only has buf_len bytes, if
> residue can be a value in the interval of [0,buf_len] this means that it has
> buf_len+1 possible values. Which again means there must be two values which
> map to the same state. In this case it would be 0 and buf_len.

No.  Think about it.  Think about the race conditions which can occur.
You take a spin lock with IRQs disabled.  The DMA engine then completes
the final transfer, and stops with the DMA pointer pointing at the byte
after the last one transferred.  You read the DMA pointer.  Because you're
holding a spinlock, the interrupt to tell you that the DMA has completed
can't be delivered.  You perform the calculation I gave, and the value
will be zero.

That's because you need the interrupt to happen in order to restart the
cyclic DMA.

This _does_ happen with existing drivers.  You can't ignore it.  The whole
"return 0 if you don't implement it" has always been an absurd idea.  You
can't reliably use it to detect lack of implementation.

The only way to detect it is to note whether the residue is ever non-zero.
If it becomes non-zero, always assume it is valid.  Otherwise, if it always
seems to be zero, only then can you make such an assumption.

Re: [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Lars-Peter Clausen]

On 06/11/2012 04:09 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 11, 2012 at 04:02:42PM +0200, Lars-Peter Clausen wrote:
>> On 06/11/2012 03:24 PM, Russell King - ARM Linux wrote:
>>> On Mon, Jun 11, 2012 at 02:04:13PM +0200, Lars-Peter Clausen wrote:
>>>> @@ -202,7 +203,27 @@ EXPORT_SYMBOL_GPL(snd_dmaengine_pcm_trigger);
>>>>  snd_pcm_uframes_t snd_dmaengine_pcm_pointer(struct snd_pcm_substream *substream)
>>>>  {
>>>>  	struct dmaengine_pcm_runtime_data *prtd = substream_to_prtd(substream);
>>>> -	return bytes_to_frames(substream->runtime, prtd->pos);
>>>> +	struct dma_tx_state state;
>>>> +	enum dma_status status;
>>>> +	unsigned int pos;
>>>> +
>>>> +	status = dmaengine_tx_status(prtd->dma_chan, prtd->cookie, &state);
>>>> +	if (status != DMA_IN_PROGRESS && status != DMA_PAUSED) {
>>>> +		pos = 0;
>>>> +	} else if (state.residue == 0) {
>>>> +		/* This should never happen with cyclic transfers, so assume
>>>> +		 * that the dmaengine driver does not support reporting residue
>>>> +		 * and fall back to counting periods. */
>>>> +		pos = prtd->pos;
>>>
>>> That is an incorrect assumption.
>>>
>>> The current DMA position is defined by the DMA pointer.  When the DMA
>>> engine has transferred the last few bytes of the DMA, the DMA pointer
>>> will be pointing to the byte _after_ the last byte transferred in the
>>> buffer.
>>>
>>> Therefore, when you calculate the residue as (buf_start + buf_len -
>>> current_pointer) you end up with zero.
>>
>> My assumption is that for cyclic DMA it will be pointing to the first byte
>> again after the last byte has been transferred. Something like this:
>>
>> offset = (offset + 1) % buf_len
>> and current_pointer = buf_start + offset
>>
>> If this is not true for a particular DMA controller I think we should still
>> make sure in the driver for this controller that residue will never be zero
>> for cyclic transfers, but instead will return the buffer size. This is the
>> only thing that really makes sense. The buffer only has buf_len bytes, if
>> residue can be a value in the interval of [0,buf_len] this means that it has
>> buf_len+1 possible values. Which again means there must be two values which
>> map to the same state. In this case it would be 0 and buf_len.
> 
> No.  Think about it.  Think about the race conditions which can occur.
> You take a spin lock with IRQs disabled.  The DMA engine then completes
> the final transfer, and stops with the DMA pointer pointing at the byte
> after the last one transferred.  You read the DMA pointer.  Because you're
> holding a spinlock, the interrupt to tell you that the DMA has completed
> can't be delivered.  You perform the calculation I gave, and the value
> will be zero.
> 

My initial approach to this problem was to set residue to ~((u32)0) for
drivers which don't implement it. But as I said I think that residue of 0
should be reserved as a special state, which means that the transfer is
done. Since a cyclic transfer is never done (unless aborted) it should never
return a residue of zero.

Otherwise you'll have a ambiguity between a residue of 0 and a residue of
buffer_size, which in my opinion is an undesirable feature of the dmaengine API.

> That's because you need the interrupt to happen in order to restart the
> cyclic DMA.
> 
> This _does_ happen with existing drivers.  You can't ignore it.  The whole
> "return 0 if you don't implement it" has always been an absurd idea.  You
> can't reliably use it to detect lack of implementation.

In my opinion the driver should hide the fact that it emulates cyclic
transfers by the means of normal transfers and return buffer_size instead of
0 in the special case you just described.

- Lars

Re: [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Mon, Jun 11, 2012 at 02:24:09PM +0100, Russell King - ARM Linux wrote:

> What we need to do is to get rid of this idea that reporting the residue
> is optional for DMA engine drivers.  Let's make it absolutely required
> in order to support cyclic transfers.

I tend to agree, if we are going to let things not implement this we
need to provide an out of bounds way for them to signal that they don't
support it and make it an error to use the interface at all.  Otherwise
the interface complexity increases as you get into special cases and so
on.  We only need to fix the drivers that are used with ASoC immediately
and there's not so many of them which is easier than being forced to get
every driver upgraded at once.

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

Re: [alsa-devel] [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Lars-Peter Clausen]

On 06/11/2012 04:57 PM, Mark Brown wrote:
> On Mon, Jun 11, 2012 at 02:24:09PM +0100, Russell King - ARM Linux wrote:
> 
>> What we need to do is to get rid of this idea that reporting the residue
>> is optional for DMA engine drivers.  Let's make it absolutely required
>> in order to support cyclic transfers.
> 
> I tend to agree, if we are going to let things not implement this we
> need to provide an out of bounds way for them to signal that they don't
> support it and make it an error to use the interface at all.  Otherwise
> the interface complexity increases as you get into special cases and so
> on.  We only need to fix the drivers that are used with ASoC immediately
> and there's not so many of them which is easier than being forced to get
> every driver upgraded at once.

I think the previous discussions have made it clear that we don't want to
make it optional for drivers to implement residue reporting for cyclic
transfers.

Another option is to provide the current implementation of the pcm_pointer
as a standalone legacy function, which can be used by the old platforms
until their dmaengine drivers have cached up. And add a new residue-only
implementation which will be mandatory for new drivers. That would make it
more explicit that those platforms are sort of broken and need to be fixed.
After all of them have been fixed the legacy pcm_pointer implementation can
be removed.

I still think though that residue should never be reported as 0 for active
cyclic transfers.

- Lars

Re: [alsa-devel] [PATCH 2/2] ASoC: dmaengine-pcm: Add support for querying stream position from DMA device

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Mon, Jun 11, 2012 at 05:20:09PM +0200, Lars-Peter Clausen wrote:

> Another option is to provide the current implementation of the pcm_pointer
> as a standalone legacy function, which can be used by the old platforms
> until their dmaengine drivers have cached up. And add a new residue-only
> implementation which will be mandatory for new drivers. That would make it
> more explicit that those platforms are sort of broken and need to be fixed.
> After all of them have been fixed the legacy pcm_pointer implementation can
> be removed.

This seems like a good approach, yes.

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