Cyclic DMA - callback properties and tx_status residue

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

Dan, Vinod,

What is the desired behaviour of the callback for cyclic DMA?

My understanding is:
1. Such callbacks will be made from tasklet context.
2. Exactly one callback will be issued for every 'period' of the transfer
   which has completed.

My main reason for asking is point (2) - because tasklets have a property
which means that they are guaranteed to run exactly once after a
tasklet_schedule() call even if there are further tasklet_schedule()
calls before the tasklet is actually run.  This property can lead to
dropped callbacks unless DMA engine authors have thought about this.

Alternatively, we could document that for cyclic transfers, one callback
will be made after the expiry of a period or number of periods.  In
other words, if two periods expire before we issue the callback, the
user will only get _one_ callback issued.  In that case, we definitely
need some way to determine the DMA position...

The next issue is this tx_status method, and the residue bytes returned.
There seems to be quite a bit of confusion over what this is, and what
it should be, and its relationship to the passed in cookie.  The drivers
I've seen using it appear to total up the number of pending bytes to
be transferred for every transfer which has been issued.  This seems
wrong to me, I think it should be the number of bytes left until the
transfer with the specified cookie has completed.

Moreover, what's the requirement here when we're using cyclic transfers?
If we take that same definition, then, because cyclic transfers never
complete, effectively they have an infinite number of bytes.  That's not
useful.  What would be more useful is if this was defined as the number
of bytes to the end of the cyclic buffer - or something similar.  That
then would allow the current DMA position to be determined for applications
like ASoC to know how much of the buffer it could refill with new data.

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Wed, 2012-05-02 at 15:45 +0100, Russell King - ARM Linux wrote:
> Dan, Vinod,
> 
> What is the desired behaviour of the callback for cyclic DMA?
Typically cyclic DMA is used in the case of sound. This is where alsa
expects the "period" callback for every DMA period.
Generally, such notification serves the purpose of letting client know
that one "block" has been completed.
> 
> My understanding is:
> 1. Such callbacks will be made from tasklet context.
> 2. Exactly one callback will be issued for every 'period' of the transfer
>    which has completed.
> 
> My main reason for asking is point (2) - because tasklets have a property
> which means that they are guaranteed to run exactly once after a
> tasklet_schedule() call even if there are further tasklet_schedule()
> calls before the tasklet is actually run.  This property can lead to
> dropped callbacks unless DMA engine authors have thought about this.
Yes, that can happen and would make sense to have dmac drivers handle
this.
> 
> Alternatively, we could document that for cyclic transfers, one callback
> will be made after the expiry of a period or number of periods.  In
> other words, if two periods expire before we issue the callback, the
> user will only get _one_ callback issued.  In that case, we definitely
> need some way to determine the DMA position...
I think this would be fine with me. 
> 
> The next issue is this tx_status method, and the residue bytes returned.
> There seems to be quite a bit of confusion over what this is, and what
> it should be, and its relationship to the passed in cookie.  The drivers
> I've seen using it appear to total up the number of pending bytes to
> be transferred for every transfer which has been issued.  This seems
> wrong to me, I think it should be the number of bytes left until the
> transfer with the specified cookie has completed.
>From what I understand (Dan please correct me as you may know this much
better than me), the tx_status tells you wrt passed cookie what is the
status. Further, (as you pointed), it also tells you which is last
completed cookie and what is the current "running". And wrt the
currently "running" cookie it tells you how may bytes are left.
> 
> Moreover, what's the requirement here when we're using cyclic transfers?
> If we take that same definition, then, because cyclic transfers never
> complete, effectively they have an infinite number of bytes.  That's not
> useful.  What would be more useful is if this was defined as the number
> of bytes to the end of the cyclic buffer - or something similar.  That
> then would allow the current DMA position to be determined for applications
> like ASoC to know how much of the buffer it could refill with new data.
IIRC had pointed Lars to this API, but quick check of code reveals that
this is unused. I think returning residue as remaining bytes of current
"block" makes sense. This easily gives knowledge of where DMA pointer is
wrt current ongoing transaction. Was that your meaning of end of cyclic
buffer, not sure what end means here?

-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Wed, May 02, 2012 at 09:31:15PM +0530, Vinod Koul wrote:
> On Wed, 2012-05-02 at 15:45 +0100, Russell King - ARM Linux wrote:
> > Dan, Vinod,
> > 
> > What is the desired behaviour of the callback for cyclic DMA?
>
> Typically cyclic DMA is used in the case of sound. This is where alsa
> expects the "period" callback for every DMA period.
> Generally, such notification serves the purpose of letting client know
> that one "block" has been completed.

Yes, though as we have this interface as something generic I didn't want
to call it "the alsa dma interface".

> > My understanding is:
> > 1. Such callbacks will be made from tasklet context.
> > 2. Exactly one callback will be issued for every 'period' of the transfer
> >    which has completed.
> > 
> > My main reason for asking is point (2) - because tasklets have a property
> > which means that they are guaranteed to run exactly once after a
> > tasklet_schedule() call even if there are further tasklet_schedule()
> > calls before the tasklet is actually run.  This property can lead to
> > dropped callbacks unless DMA engine authors have thought about this.
>
> Yes, that can happen and would make sense to have dmac drivers handle
> this.

So we need something that documents this behaviour.

> > Alternatively, we could document that for cyclic transfers, one callback
> > will be made after the expiry of a period or number of periods.  In
> > other words, if two periods expire before we issue the callback, the
> > user will only get _one_ callback issued.  In that case, we definitely
> > need some way to determine the DMA position...
>
> I think this would be fine with me. 

This is also fine by ALSA, because in response to snd_pcm_period_elapsed()
ALSA calls back to request the current DMA position, where that is defined
as the offset from the starting address of the buffer.  (Think,
next_dma_transfer_address - buf_addr passed into device_prep_dma_cyclic()).

> > The next issue is this tx_status method, and the residue bytes returned.
> > There seems to be quite a bit of confusion over what this is, and what
> > it should be, and its relationship to the passed in cookie.  The drivers
> > I've seen using it appear to total up the number of pending bytes to
> > be transferred for every transfer which has been issued.  This seems
> > wrong to me, I think it should be the number of bytes left until the
> > transfer with the specified cookie has completed.
>
> From what I understand (Dan please correct me as you may know this much
> better than me), the tx_status tells you wrt passed cookie what is the
> status. Further, (as you pointed), it also tells you which is last
> completed cookie and what is the current "running". And wrt the
> currently "running" cookie it tells you how may bytes are left.

That means we have a number of buggy drivers on both sides of the API...
PL08x for example always total up all the pending bytes from the position
in the current descriptor to the last issued descriptor.  Many other
implementations simply always return zero.

> > Moreover, what's the requirement here when we're using cyclic transfers?
> > If we take that same definition, then, because cyclic transfers never
> > complete, effectively they have an infinite number of bytes.  That's not
> > useful.  What would be more useful is if this was defined as the number
> > of bytes to the end of the cyclic buffer - or something similar.  That
> > then would allow the current DMA position to be determined for applications
> > like ASoC to know how much of the buffer it could refill with new data.
>
> IIRC had pointed Lars to this API, but quick check of code reveals that
> this is unused. I think returning residue as remaining bytes of current
> "block" makes sense. This easily gives knowledge of where DMA pointer is
> wrt current ongoing transaction. Was that your meaning of end of cyclic
> buffer, not sure what end means here?

I think the residue thing was something Linus created, which is why he's
included in this thread...

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Wed, May 02, 2012 at 05:27:02PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 02, 2012 at 09:31:15PM +0530, Vinod Koul wrote:
> > From what I understand (Dan please correct me as you may know this much
> > better than me), the tx_status tells you wrt passed cookie what is the
> > status. Further, (as you pointed), it also tells you which is last
> > completed cookie and what is the current "running". And wrt the
> > currently "running" cookie it tells you how may bytes are left.
> 
> That means we have a number of buggy drivers on both sides of the API...
> PL08x for example always total up all the pending bytes from the position
> in the current descriptor to the last issued descriptor.  Many other
> implementations simply always return zero.

Right, as there's been no reply on this, I'll see about changing PL08x
and SA11x0 to do exactly this.  I'll also see about implementing it in
the new OMAP DMA engine driver.

The semantics I intend to implement are:

If the cookie has completed successfully, the residue will be zero.  If
it is queued, it will be the number of bytes in the descriptor identified
by the cookie.  If it is in progress, it will be the number of bytes yet
to be transferred.

Now, there is a bit of ambiguity over "number of bytes yet to be transferred"
with DMA hardware which reads from memory into a FIFO, and then transfers
to the peripheral when the peripheral requests DMA service.  Do we define
it as the number of bytes read from memory, or the number of bytes to be
transferred to the peripheral.

The latter may not be easy for DMA hardware to report to us... so my
initial implementation will be number of bytes remaining calculated using
the DMA address pointer into memory.

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Fri, 2012-05-04 at 13:26 +0100, Russell King - ARM Linux wrote:
> On Wed, May 02, 2012 at 05:27:02PM +0100, Russell King - ARM Linux wrote:
> > On Wed, May 02, 2012 at 09:31:15PM +0530, Vinod Koul wrote:
> > > From what I understand (Dan please correct me as you may know this much
> > > better than me), the tx_status tells you wrt passed cookie what is the
> > > status. Further, (as you pointed), it also tells you which is last
> > > completed cookie and what is the current "running". And wrt the
> > > currently "running" cookie it tells you how may bytes are left.
> > 
> > That means we have a number of buggy drivers on both sides of the API...
> > PL08x for example always total up all the pending bytes from the position
> > in the current descriptor to the last issued descriptor.  Many other
> > implementations simply always return zero.
> 
> Right, as there's been no reply on this, I'll see about changing PL08x
> and SA11x0 to do exactly this.  I'll also see about implementing it in
> the new OMAP DMA engine driver.
> 
> The semantics I intend to implement are:
> 
> If the cookie has completed successfully, the residue will be zero.  If
> it is queued, it will be the number of bytes in the descriptor identified
> by the cookie.  If it is in progress, it will be the number of bytes yet
> to be transferred.
Today is is supposed to be defined as:
* @residue: the remaining number of bytes left to transmit
*      on the selected transfer for states DMA_IN_PROGRESS and
*      DMA_PAUSED if this is implemented in the driver, else 0
So what you are saying is correct and already documented.

> 
> Now, there is a bit of ambiguity over "number of bytes yet to be transferred"
> with DMA hardware which reads from memory into a FIFO, and then transfers
> to the peripheral when the peripheral requests DMA service.  Do we define
> it as the number of bytes read from memory, or the number of bytes to be
> transferred to the peripheral.
> 
> The latter may not be easy for DMA hardware to report to us... so my
> initial implementation will be number of bytes remaining calculated using
> the DMA address pointer into memory.
yes, it may not be possible to get this information, so lets take the
dma pointer in memory and do the math.

-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Wed, May 02, 2012 at 03:45:55PM +0100, Russell King - ARM Linux wrote:

> Alternatively, we could document that for cyclic transfers, one callback
> will be made after the expiry of a period or number of periods.  In
> other words, if two periods expire before we issue the callback, the
> user will only get _one_ callback issued.  In that case, we definitely
> need some way to determine the DMA position...

One callback is fine for audio providing it's possible to read the
position to at least some extent.  If we've been delayed long enough to
miss delivering the callback then we've already lost anyway and things
should recover as well as they're going to by just looking at the
postition.

> useful.  What would be more useful is if this was defined as the number
> of bytes to the end of the cyclic buffer - or something similar.  That
> then would allow the current DMA position to be determined for applications
> like ASoC to know how much of the buffer it could refill with new data.

Yes, ideally we'd want to know where we are in the buffer.  In practice
all some hardware can give you is the last buffer processed but that
should be adequate providing the cycle can be split into multiple
buffers but that's going to be required anyway for audio.

Cyclic DMA - callback properties and tx_status residue

[Linus Walleij]

On Wed, May 2, 2012 at 6:27 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

>> > What is the desired behaviour of the callback for cyclic DMA?
>>
>> Typically cyclic DMA is used in the case of sound. This is where alsa
>> expects the "period" callback for every DMA period.
>> Generally, such notification serves the purpose of letting client know
>> that one "block" has been completed.
>
> Yes, though as we have this interface as something generic I didn't want
> to call it "the alsa dma interface".

There is a DMAengine helper in ALSA SoC these days,
sound/soc/soc-dmaengine-pcm.c which does not yet include
get position calls, but I expect that's where the ALSA glue will
end up.

>> From what I understand (Dan please correct me as you may know this much
>> better than me), the tx_status tells you wrt passed cookie what is the
>> status. Further, (as you pointed), it also tells you which is last
>> completed cookie and what is the current "running". And wrt the
>> currently "running" cookie it tells you how may bytes are left.
>
> That means we have a number of buggy drivers on both sides of the API...
> PL08x for example always total up all the pending bytes from the position
> in the current descriptor to the last issued descriptor. ?Many other
> implementations simply always return zero.

My bad, when I implemented it, I imagined the semantic to be
the residue on the *channel* which is not smart, it's better to use
the cookie as reference, so patch at will.

>> IIRC had pointed Lars to this API, but quick check of code reveals that
>> this is unused. I think returning residue as remaining bytes of current
>> "block" makes sense. This easily gives knowledge of where DMA pointer is
>> wrt current ongoing transaction. Was that your meaning of end of cyclic
>> buffer, not sure what end means here?
>
> I think the residue thing was something Linus created, which is why he's
> included in this thread...

Lars-Peter Clausen is the person writing the ALSA SoC wrapper.
Lars-Peter: do you have plans to implement residue calls for
getting buffer position?

It was originally created to be able to use DMA for ALSA SoC on
the U300. I still don't know if there is some other viable usecase
but I expect that things like multimedia codecs would use a DMA
engine much the same way.

Yours,
Linus Walleij

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 11:27:17AM +0200, Linus Walleij wrote:
> On Wed, May 2, 2012 at 6:27 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> 
> >> > What is the desired behaviour of the callback for cyclic DMA?
> >>
> >> Typically cyclic DMA is used in the case of sound. This is where alsa
> >> expects the "period" callback for every DMA period.
> >> Generally, such notification serves the purpose of letting client know
> >> that one "block" has been completed.
> >
> > Yes, though as we have this interface as something generic I didn't want
> > to call it "the alsa dma interface".
> 
> There is a DMAengine helper in ALSA SoC these days,
> sound/soc/soc-dmaengine-pcm.c which does not yet include
> get position calls, but I expect that's where the ALSA glue will
> end up.

I'm more or less ignoring that because with current DMA engine stuff, it's
buggy.  The presence of such a user doesn't really define how this
interface supposed to work either.

> >> IIRC had pointed Lars to this API, but quick check of code reveals that
> >> this is unused. I think returning residue as remaining bytes of current
> >> "block" makes sense. This easily gives knowledge of where DMA pointer is
> >> wrt current ongoing transaction. Was that your meaning of end of cyclic
> >> buffer, not sure what end means here?
> >
> > I think the residue thing was something Linus created, which is why he's
> > included in this thread...
> 
> Lars-Peter Clausen is the person writing the ALSA SoC wrapper.
> Lars-Peter: do you have plans to implement residue calls for
> getting buffer position?
> 
> It was originally created to be able to use DMA for ALSA SoC on
> the U300. I still don't know if there is some other viable usecase
> but I expect that things like multimedia codecs would use a DMA
> engine much the same way.

Given that counting the number of period callbacks to determine the
position is buggy, soc-dmaengine-pcm.c is buggy as it currently stands.
I'm willing to bet that if ALSA requests a small period size, and you
load a system up with lots of activity, you'll eventually hear
corrupted audio.

It's difficult to test because the corruption will probably only be
occasional and it'll be purely audio based - it'll basically play the
bit of the buffer that's currently being loaded with new data from time
to time because ALSAs idea of where the DMA is vs where the hardware
is will gradually slip over time.

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Wed, May 09, 2012 at 10:33:34AM +0100, Russell King - ARM Linux wrote:
> On Wed, May 09, 2012 at 11:27:17AM +0200, Linus Walleij wrote:

> > There is a DMAengine helper in ALSA SoC these days,
> > sound/soc/soc-dmaengine-pcm.c which does not yet include
> > get position calls, but I expect that's where the ALSA glue will
> > end up.

> I'm more or less ignoring that because with current DMA engine stuff, it's
> buggy.  The presence of such a user doesn't really define how this
> interface supposed to work either.

We need to at least include it in the fixes; I'm currently insisting
that any dmaengine based device uses it.  If we can't come up with a
standard library to map the dmaengine based DMA controllers to the ALSA
userspace API something is seriously wrong.  We definitely shouldn't be
treating it as an API reference but we should be ensuring that it does
the best possible job with the APIs that do exist and enhancing the
underlying APIs if that's needed.

Looking at the code the current usage doesn't seem obviously wrong so
there's some work to do - we get a callback assigned on each descriptor
we submit so it's a bit surprising that this might not get delivered,
though as has been discussed further up the thread that's something that
might actually happen and perhaps we need to clarify the interfaces
here.

> Given that counting the number of period callbacks to determine the
> position is buggy, soc-dmaengine-pcm.c is buggy as it currently stands.
> I'm willing to bet that if ALSA requests a small period size, and you
> load a system up with lots of activity, you'll eventually hear
> corrupted audio.

> It's difficult to test because the corruption will probably only be
> occasional and it'll be purely audio based - it'll basically play the
> bit of the buffer that's currently being loaded with new data from time
> to time because ALSAs idea of where the DMA is vs where the hardware
> is will gradually slip over time.

Right, any driver relying on this code must currently be confident that the
completion callbacks can actually be delivered before the next period
expires (for example, by requiring a large enough period size and
ensuring that the completion gets delivered in hard IRQ context on the
DMA side so there's less impact from load).  This might break down under
extreme loads, though if it does I rather suspect that we'd underflow
anyway.

If we can resolve the issues with reading the current position in
dmaengine then this should be fairly simple to address more
comprehensively of course, though there will be some hardware imposed
limits on what we can do.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/1bc521af/attachment-0001.sig>

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Wed, May 09, 2012 at 12:16:15PM +0100, Mark Brown wrote:
> Looking at the code the current usage doesn't seem obviously wrong so
> there's some work to do - we get a callback assigned on each descriptor
> we submit so it's a bit surprising that this might not get delivered,
> though as has been discussed further up the thread that's something that
> might actually happen and perhaps we need to clarify the interfaces
> here.

Go back and look at my opening email in this thread.  Think about
the comments in include/linux/interrupt.h about the tasklet guarantees.
Then look at (eg) the IMX DMA engine driver and ascertain how the
callback is called.  Then look at soc-dmaengine-pcm's callback
method.

Put all this together and what you get is that there's absolutely no
guarantee that the callback will be called for each period.

You really won't know if it slips just one or two periods, because your
audio output won't be affected by that.  It _may_ only become apparant
if DMA catches up with the part of the buffer you're writing to.

Amd I'm also willing to bet that the IMX DMA engine with ASoC has only
been tested with an unloaded system.

> If we can resolve the issues with reading the current position in
> dmaengine then this should be fairly simple to address more
> comprehensively of course, though there will be some hardware imposed
> limits on what we can do.

The issue with doing that is it will break existing setups - have a look
at the various DMA engine implementations of the tx_status method, and
how many actually set the 'residue' bytes correctly (the answer at the
moment is none of them do, according to the definition that we've now
come up with in this thread.)

This is *exactly* the kind of issues we have with DMA engine at present;
the implementations are at best poor quality, partly because the API is
poorly documented and no one has particularly thought about these issues.
Every DMA engine implementation implements the entire API using its own
private code, so each one has its own differing behaviour.

So, really we're not at the point where ASoC maintainers can insist on
anything of DMA engine at present; we need to get DMA engine stuff sorted
out so that we have guaranteed API conditions which can be relied upon by
all users of the API across each DMA engine driver for any particular
kernel version.

Until we have that, we're going to hit problems.  See the recent
brokenness over whether the DMA engine callback is called with spinlocks
held or not, which then caused drivers to spring up their own tasklets
which they schedule from the DMA engine callback.

DMA engine is slowly getting better (mainly through my efforts at trying
to extract common code, and fix down the API issues) but it needs folk
with strong and good review skills to ensure that we stop the influx of
non-conforming code and get that non-conforming code fixed.

Cyclic DMA - callback properties and tx_status residue

[Lars-Peter Clausen]

On 05/09/2012 11:27 AM, Linus Walleij wrote:
> On Wed, May 2, 2012 at 6:27 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> [...]
>>> IIRC had pointed Lars to this API, but quick check of code reveals that
>>> this is unused. I think returning residue as remaining bytes of current
>>> "block" makes sense. This easily gives knowledge of where DMA pointer is
>>> wrt current ongoing transaction. Was that your meaning of end of cyclic
>>> buffer, not sure what end means here?
>>
>> I think the residue thing was something Linus created, which is why he's
>> included in this thread...
> 
> Lars-Peter Clausen is the person writing the ALSA SoC wrapper.
> Lars-Peter: do you have plans to implement residue calls for
> getting buffer position?
> 

At the time the ALSA wrapper was written there were no DMA engine
implementations which actually provided residue information, so that's why I
left it out. But it is really straight forward to add it, given that we can
agree on a common definition of what the residue should be for cyclic
transfers. Russel's suggestion to define it as the number of bytes until the
end of the buffer is in my opinion the best option.

- Lars

Cyclic DMA - callback properties and tx_status residue

[Lars-Peter Clausen]

On 05/09/2012 02:19 PM, Russell King - ARM Linux wrote:
> 
>> If we can resolve the issues with reading the current position in
>> dmaengine then this should be fairly simple to address more
>> comprehensively of course, though there will be some hardware imposed
>> limits on what we can do.
> 
> The issue with doing that is it will break existing setups - have a look
> at the various DMA engine implementations of the tx_status method, and
> how many actually set the 'residue' bytes correctly (the answer at the
> moment is none of them do, according to the definition that we've now
> come up with in this thread.)

Due to the modular design of the ALSA dmaengine wrapper we can use two
different pcm_pointer callback implementations for now. One which does the
callback counting and the other which uses tx_status. And than we can switch
the ASoC drivers over, which are known to only use DMA engine
implementations which have a tx_status method with proper residue information.

> 
> This is *exactly* the kind of issues we have with DMA engine at present;
> the implementations are at best poor quality, partly because the API is
> poorly documented and no one has particularly thought about these issues.
> Every DMA engine implementation implements the entire API using its own
> private code, so each one has its own differing behaviour.
> 
> So, really we're not at the point where ASoC maintainers can insist on
> anything of DMA engine at present; we need to get DMA engine stuff sorted
> out so that we have guaranteed API conditions which can be relied upon by
> all users of the API across each DMA engine driver for any particular
> kernel version.

I think having the common ASoC wrapper here really helps to sort this out,
because it forces us to come up with DMA engine drivers which behave in the
same way in regards to their API. Before basically every DMA engine user was
in a SoC specific driver and people would get away with API variations in
their DMA engine drivers, because the user was only expecting to ever see
that one DMA engine with it's specific API.

> 
> [...]
> 
> DMA engine is slowly getting better (mainly through my efforts at trying
> to extract common code, and fix down the API issues) but it needs folk
> with strong and good review skills to ensure that we stop the influx of
> non-conforming code and get that non-conforming code fixed.

I totally agree and quite thankful for the cleanups you've done over the
past few months.

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Wed, May 09, 2012 at 01:19:45PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 09, 2012 at 12:16:15PM +0100, Mark Brown wrote:

> > Looking at the code the current usage doesn't seem obviously wrong so
> > there's some work to do - we get a callback assigned on each descriptor
> > we submit so it's a bit surprising that this might not get delivered,
> > though as has been discussed further up the thread that's something that
> > might actually happen and perhaps we need to clarify the interfaces
> > here.

> Go back and look at my opening email in this thread.  Think about
> the comments in include/linux/interrupt.h about the tasklet guarantees.
> Then look at (eg) the IMX DMA engine driver and ascertain how the
> callback is called.  Then look at soc-dmaengine-pcm's callback
> method.

> Put all this together and what you get is that there's absolutely no
> guarantee that the callback will be called for each period.

I'm not saying that the current implementation works perfectly and that
there are no bugs anywhere (indeed, I did say "that [dropped callbacks
are] something that might actually happen"), I'm saying that the way the
interface has been written it's entirely reasonable for someone to
expect that their completion callbacks will be delivered; it's highly
unusual to have a callback like this that's unreliably called and it's
going to be error prone.  As part of fixing this we should look at
trying to ensure that the interface doesn't mislead people.

> You really won't know if it slips just one or two periods, because your
> audio output won't be affected by that.  It _may_ only become apparant
> if DMA catches up with the part of the buffer you're writing to.

> Amd I'm also willing to bet that the IMX DMA engine with ASoC has only
> been tested with an unloaded system.

You're probably going to be OK even under reasonable load for most
applications except VoIP - the buffers tend to end up being reasonably
deep in the context of the scheduler.

> > If we can resolve the issues with reading the current position in
> > dmaengine then this should be fairly simple to address more
> > comprehensively of course, though there will be some hardware imposed
> > limits on what we can do.

> The issue with doing that is it will break existing setups - have a look
> at the various DMA engine implementations of the tx_status method, and
> how many actually set the 'residue' bytes correctly (the answer at the
> moment is none of them do, according to the definition that we've now
> come up with in this thread.)

So as a transition measure can we quirk this in the soc-dmaengine stuff?
It seems like we should be able to, possibly based on capability
information from dmaengine (ISTR some DMA controllers floating around
which weren't able to provide any useful information anyway so we may
need to keep some level of workaround anyway).

> So, really we're not at the point where ASoC maintainers can insist on
> anything of DMA engine at present; we need to get DMA engine stuff sorted
> out so that we have guaranteed API conditions which can be relied upon by
> all users of the API across each DMA engine driver for any particular
> kernel version.

I'm afraid this is trying to bolt the stable door after the horse has
bolted - we've already got most of the drivers using the same code base,
the only ones that aren't are the ones that need to emulate cyclic DMA.

> DMA engine is slowly getting better (mainly through my efforts at trying
> to extract common code, and fix down the API issues) but it needs folk
> with strong and good review skills to ensure that we stop the influx of
> non-conforming code and get that non-conforming code fixed.

For this I think actual cross-platform users are going to be enormously
helpful - part of the reason dmaengine is the way that it is is that
many of the drivers would only ever be used with a fixed set of clients
which only ever used that driver so it was very easy for them to evolve
private conventions without anyone noticing.  Generic users don't allow
this sort of stuff and it seems more sensible to get people to fix their
systems at the dmaengine level than to duplicate the higher level code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120509/8e66b86d/attachment.sig>

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Wed, 2012-05-09 at 13:19 +0100, Russell King - ARM Linux wrote:
> On Wed, May 09, 2012 at 12:16:15PM +0100, Mark Brown wrote:
> > Looking at the code the current usage doesn't seem obviously wrong so
> > there's some work to do - we get a callback assigned on each descriptor
> > we submit so it's a bit surprising that this might not get delivered,
> > though as has been discussed further up the thread that's something that
> > might actually happen and perhaps we need to clarify the interfaces
> > here.
> 
> Go back and look at my opening email in this thread.  Think about
> the comments in include/linux/interrupt.h about the tasklet guarantees.
> Then look at (eg) the IMX DMA engine driver and ascertain how the
> callback is called.  Then look at soc-dmaengine-pcm's callback
> method.
> 
> Put all this together and what you get is that there's absolutely no
> guarantee that the callback will be called for each period.
> 
> You really won't know if it slips just one or two periods, because your
> audio output won't be affected by that.  It _may_ only become apparant
> if DMA catches up with the part of the buffer you're writing to.
> 
> Amd I'm also willing to bet that the IMX DMA engine with ASoC has only
> been tested with an unloaded system. 

DMAengine mandates the callback be called from tasklet. Yes under a very
heavy loaded systems there can be a certain case where callback maybe
delayed. Or the tasklet gets scheduled _once_ for couple of dma
completions, thus missed

But tell me what prevents the dmac driver for fixing this. In ISR they
can easily find which descriptor is completed and mark them so.
In tasklet, it can ensure that all callback are generated including
previous missed ones, if that is something the usage of dmaengine needs.

For audio needs I think it is fine if we miss, as long as subsequent
comes untill we get underrun/overrun.

Quite a few ASoC drivers use dmaengine today (cyclic was added keeping
audio in mind), so adding library helps to avoid each ASoC driver having
its own implementation. It indirectly ensures that APIs are properly
implemented as well and any bugs due to deviation form std behavior gets
identified and fixed!

> DMA engine is slowly getting better (mainly through my efforts at trying
> to extract common code, and fix down the API issues) but it needs folk
> with strong and good review skills to ensure that we stop the influx of
> non-conforming code and get that non-conforming code fixed.
Yes and a big Thanks for this. Russell, your contributions are very appreciated.

-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Thu, May 10, 2012 at 09:14:18AM +0530, Vinod Koul wrote:
> On Wed, 2012-05-09 at 13:19 +0100, Russell King - ARM Linux wrote:
> > On Wed, May 09, 2012 at 12:16:15PM +0100, Mark Brown wrote:
> > > Looking at the code the current usage doesn't seem obviously wrong so
> > > there's some work to do - we get a callback assigned on each descriptor
> > > we submit so it's a bit surprising that this might not get delivered,
> > > though as has been discussed further up the thread that's something that
> > > might actually happen and perhaps we need to clarify the interfaces
> > > here.
> > 
> > Go back and look at my opening email in this thread.  Think about
> > the comments in include/linux/interrupt.h about the tasklet guarantees.
> > Then look at (eg) the IMX DMA engine driver and ascertain how the
> > callback is called.  Then look at soc-dmaengine-pcm's callback
> > method.
> > 
> > Put all this together and what you get is that there's absolutely no
> > guarantee that the callback will be called for each period.
> > 
> > You really won't know if it slips just one or two periods, because your
> > audio output won't be affected by that.  It _may_ only become apparant
> > if DMA catches up with the part of the buffer you're writing to.
> > 
> > Amd I'm also willing to bet that the IMX DMA engine with ASoC has only
> > been tested with an unloaded system. 
> 
> DMAengine mandates the callback be called from tasklet. Yes under a very
> heavy loaded systems there can be a certain case where callback maybe
> delayed. Or the tasklet gets scheduled _once_ for couple of dma
> completions, thus missed
> 
> But tell me what prevents the dmac driver for fixing this. In ISR they
> can easily find which descriptor is completed and mark them so.
> In tasklet, it can ensure that all callback are generated including
> previous missed ones, if that is something the usage of dmaengine needs.
> 
> For audio needs I think it is fine if we miss, as long as subsequent
> comes untill we get underrun/overrun.
> 
> Quite a few ASoC drivers use dmaengine today (cyclic was added keeping
> audio in mind), so adding library helps to avoid each ASoC driver having
> its own implementation. It indirectly ensures that APIs are properly
> implemented as well and any bugs due to deviation form std behavior gets
> identified and fixed!

I'm not going to describe the problem for a third time.  I've said what it
is.  I've pointed at the drivers which are potentially a problem.  Please
look over them and you'll see the problem.

In fact, please look at both imx-dma.c _and_ imx-sdma.c - search for
"callback" and have a look at the code around there.

You will find that imx-dma.c processes one descriptor per tasklet run,
and calls the callback under a spinlock.

You'll find that imx-sdma.c always calls callbacks from IRQ context.

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Thu, May 10, 2012 at 09:14:18AM +0530, Vinod Koul wrote:

> For audio needs I think it is fine if we miss, as long as subsequent
> comes untill we get underrun/overrun.

No, - this is what Russell has been reporting.  Due to the lack of
position callbacks the ASoC dmaengine stuff is counting completions to
work out where it is in the buffer.  If we miss completions then the
drivers will start to loose track of where they are in the buffer.  If
we had usable position reporting this would work OK, or if the ASoC code
were changed to remember where each buffer ends and get that information
passed back in the completion argument then it could recover that way
(but that's fairly painful).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120510/4ade4fa9/attachment.sig>

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Thu, 2012-05-10 at 08:44 +0100, Russell King - ARM Linux wrote:
> In fact, please look at both imx-dma.c _and_ imx-sdma.c - search for
> "callback" and have a look at the code around there.
> 
> You will find that imx-dma.c processes one descriptor per tasklet run,
> and calls the callback under a spinlock.
> 
> You'll find that imx-sdma.c always calls callbacks from IRQ context.
Yup, let me try to fix them!

-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Thu, 2012-05-10 at 10:42 +0100, Mark Brown wrote:
> On Thu, May 10, 2012 at 09:14:18AM +0530, Vinod Koul wrote:
> 
> > For audio needs I think it is fine if we miss, as long as subsequent
> > comes untill we get underrun/overrun.
> 
> No, - this is what Russell has been reporting.  Due to the lack of
> position callbacks the ASoC dmaengine stuff is counting completions to
> work out where it is in the buffer.  If we miss completions then the
> drivers will start to loose track of where they are in the buffer.  If
> we had usable position reporting this would work OK, or if the ASoC code
> were changed to remember where each buffer ends and get that information
> passed back in the completion argument then it could recover that way
> (but that's fairly painful).

This should not be in ASoC library. The driver should be able to detect
if a tasklet was called for many dma completions and notify the client
(soc-lib) accordingly.

One way to do this would be to ensure that the descriptor is marked in
irq_handler and then all descriptor completed notified in tasklet.


-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Huang Shijie]

On Thu, May 10, 2012 at 6:58 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> On Thu, 2012-05-10 at 08:44 +0100, Russell King - ARM Linux wrote:
>> In fact, please look at both imx-dma.c _and_ imx-sdma.c - search for
>> "callback" and have a look at the code around there.
>>
>> You will find that imx-dma.c processes one descriptor per tasklet run,
>> and calls the callback under a spinlock.
>>
>> You'll find that imx-sdma.c always calls callbacks from IRQ context.
> Yup, let me try to fix them!
>
I ever sent you a patch to fix it.

Best Regards
Huang Shijie

> --
> ~Vinod
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Thu, 2012-05-10 at 21:19 +0800, Huang Shijie wrote:
> On Thu, May 10, 2012 at 6:58 PM, Vinod Koul <vinod.koul@linux.intel.com> wrote:
> > On Thu, 2012-05-10 at 08:44 +0100, Russell King - ARM Linux wrote:
> >> In fact, please look at both imx-dma.c _and_ imx-sdma.c - search for
> >> "callback" and have a look at the code around there.
> >>
> >> You will find that imx-dma.c processes one descriptor per tasklet run,
> >> and calls the callback under a spinlock.
> >>
> >> You'll find that imx-sdma.c always calls callbacks from IRQ context.
> > Yup, let me try to fix them!
> >
> I ever sent you a patch to fix it.
Yes :-), Thank You

Looks fine, should show up in -next in a day or so...


-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Fri, May 04, 2012 at 06:15:50PM +0530, Vinod Koul wrote:
> On Fri, 2012-05-04 at 13:26 +0100, Russell King - ARM Linux wrote:
> > On Wed, May 02, 2012 at 05:27:02PM +0100, Russell King - ARM Linux wrote:
> > > On Wed, May 02, 2012 at 09:31:15PM +0530, Vinod Koul wrote:
> > > > From what I understand (Dan please correct me as you may know this much
> > > > better than me), the tx_status tells you wrt passed cookie what is the
> > > > status. Further, (as you pointed), it also tells you which is last
> > > > completed cookie and what is the current "running". And wrt the
> > > > currently "running" cookie it tells you how may bytes are left.
> > > 
> > > That means we have a number of buggy drivers on both sides of the API...
> > > PL08x for example always total up all the pending bytes from the position
> > > in the current descriptor to the last issued descriptor.  Many other
> > > implementations simply always return zero.
> > 
> > Right, as there's been no reply on this, I'll see about changing PL08x
> > and SA11x0 to do exactly this.  I'll also see about implementing it in
> > the new OMAP DMA engine driver.
> > 
> > The semantics I intend to implement are:
> > 
> > If the cookie has completed successfully, the residue will be zero.  If
> > it is queued, it will be the number of bytes in the descriptor identified
> > by the cookie.  If it is in progress, it will be the number of bytes yet
> > to be transferred.
> Today is is supposed to be defined as:
> * @residue: the remaining number of bytes left to transmit
> *      on the selected transfer for states DMA_IN_PROGRESS and
> *      DMA_PAUSED if this is implemented in the driver, else 0
> So what you are saying is correct and already documented.

BTW.  Not quite - it's ambiguous, and we really must get rid of that
'else 0' clause there and require it to be implemented if we're going
to end up with users of the DMA engine API using it.

The statement also implies that a pending transfer should return a
residue of zero.  Is that really consistent with what we want this
API to do?

It also talks about 'bytes left to transmit'.  That could be interpreted
strictly as the number of bytes to be transferred from the DMA engine
to the peripheral, or from the DMA engine to memory.  That's not quite
the same as calculating the residue off the DMA address pointer.

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Thu, 2012-05-10 at 23:54 +0100, Russell King - ARM Linux wrote:
> On Fri, May 04, 2012 at 06:15:50PM +0530, Vinod Koul wrote:
> > On Fri, 2012-05-04 at 13:26 +0100, Russell King - ARM Linux wrote:
> > > On Wed, May 02, 2012 at 05:27:02PM +0100, Russell King - ARM Linux wrote:
> > > > On Wed, May 02, 2012 at 09:31:15PM +0530, Vinod Koul wrote:
> > > > > From what I understand (Dan please correct me as you may know this much
> > > > > better than me), the tx_status tells you wrt passed cookie what is the
> > > > > status. Further, (as you pointed), it also tells you which is last
> > > > > completed cookie and what is the current "running". And wrt the
> > > > > currently "running" cookie it tells you how may bytes are left.
> > > > 
> > > > That means we have a number of buggy drivers on both sides of the API...
> > > > PL08x for example always total up all the pending bytes from the position
> > > > in the current descriptor to the last issued descriptor.  Many other
> > > > implementations simply always return zero.
> > > 
> > > Right, as there's been no reply on this, I'll see about changing PL08x
> > > and SA11x0 to do exactly this.  I'll also see about implementing it in
> > > the new OMAP DMA engine driver.
> > > 
> > > The semantics I intend to implement are:
> > > 
> > > If the cookie has completed successfully, the residue will be zero.  If
> > > it is queued, it will be the number of bytes in the descriptor identified
> > > by the cookie.  If it is in progress, it will be the number of bytes yet
> > > to be transferred.
> > Today is is supposed to be defined as:
> > * @residue: the remaining number of bytes left to transmit
> > *      on the selected transfer for states DMA_IN_PROGRESS and
> > *      DMA_PAUSED if this is implemented in the driver, else 0
> > So what you are saying is correct and already documented.
> 
> BTW.  Not quite - it's ambiguous, and we really must get rid of that
> 'else 0' clause there and require it to be implemented if we're going
> to end up with users of the DMA engine API using it.
> 
> The statement also implies that a pending transfer should return a
> residue of zero.  Is that really consistent with what we want this
> API to do?
> 
> It also talks about 'bytes left to transmit'.  That could be interpreted
> strictly as the number of bytes to be transferred from the DMA engine
> to the peripheral, or from the DMA engine to memory.  That's not quite
> the same as calculating the residue off the DMA address pointer.
Yes this needs to be mandatory. Let me add that explicitly.

  * @last: last completed DMA cookie
  * @used: last issued DMA cookie (i.e. the one in progress)
- * @residue: the remaining number of bytes left to transmit
- *     on the selected transfer for states DMA_IN_PROGRESS and
- *     DMA_PAUSED if this is implemented in the driver, else 0
+ * @residue: is defined as:
+ *   - for completed descriptor: 0
+ *   - for descriptors which are in DMA_PAUSED, DMA_IN_PROGRESS:
+ *     length - (current_addr - start_addr), where:
+ *             length = descriptor dma length
+ *             current_addr = current dma pointer queried for dma channel
+ *             start_addr: starting address of this descriptor
+ *   This is mandatory and needs to be correctly filled by driver
  */
Does this make it clear?


-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Linus Walleij]

On Fri, May 11, 2012 at 5:00 AM, Vinod Koul <vinod.koul@linux.intel.com> wrote:

> Yes this needs to be mandatory. Let me add that explicitly.
>
> ?* @last: last completed DMA cookie
> ?* @used: last issued DMA cookie (i.e. the one in progress)
> - * @residue: the remaining number of bytes left to transmit
> - * ? ? on the selected transfer for states DMA_IN_PROGRESS and
> - * ? ? DMA_PAUSED if this is implemented in the driver, else 0
> + * @residue: is defined as:
> + * ? - for completed descriptor: 0
> + * ? - for descriptors which are in DMA_PAUSED, DMA_IN_PROGRESS:
> + * ? ? length - (current_addr - start_addr), where:
> + * ? ? ? ? ? ? length = descriptor dma length
> + * ? ? ? ? ? ? current_addr = current dma pointer queried for dma channel
> + * ? ? ? ? ? ? start_addr: starting address of this descriptor
> + * ? This is mandatory and needs to be correctly filled by driver
> ?*/
> Does this make it clear?

Looks good to me.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Fri, May 11, 2012 at 02:24:10PM +0200, Linus Walleij wrote:
> > + * @residue: is defined as:
> > + * ? - for completed descriptor: 0
> > + * ? - for descriptors which are in DMA_PAUSED, DMA_IN_PROGRESS:
> > + * ? ? length - (current_addr - start_addr), where:
> > + * ? ? ? ? ? ? length = descriptor dma length
> > + * ? ? ? ? ? ? current_addr = current dma pointer queried for dma channel
> > + * ? ? ? ? ? ? start_addr: starting address of this descriptor
> > + * ? This is mandatory and needs to be correctly filled by driver
> > ?*/
> > Does this make it clear?
> 
> Looks good to me.

There's a problem with defining it that way.  Consider a scatterlist.
What does 'current address' and 'start address' mean with respect to
those transfers?

Rather than trying to define this in terms of mathematics, which will
always lead to this kind of confusion, use English instead.

- for descriptors which are in progress or paused:
  the number of bytes yet to be transferred to or from the memory for
  MEM2DEV or DEV2MEM transfers.

This leaves MEM2MEM and DEV2DEV transfers ambiguous - both suffer from
the problem of whether you use the source or destination pointers/lengths
to calculate the residue.

For MEM2MEM, I would suggest these should either be reported as complete
or not-complete in their entirety.  (In other words, either their full
transfer size or zero.)  It's racy to try to do anything else with an
in-progress transfer...

DEV2DEV I've not thought about, and I'm rather wishing we didn't have
that transfer type...

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:

> This should not be in ASoC library. The driver should be able to detect
> if a tasklet was called for many dma completions and notify the client
> (soc-lib) accordingly.

Certainly if we're going to keep the current API with completions in the
DMA requests I'd really expect that the completions would get called -
if we changed that around then perhaps it'd make more sense for the
audio to keep track of things.  Obviously if we get reliable position
information and audio uses that then this is definitely an ASoC thing.

> One way to do this would be to ensure that the descriptor is marked in
> irq_handler and then all descriptor completed notified in tasklet.

Yes, ideally this would be in the dmaengine core so individual DMA
drivers didn't need to worry about it either.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120511/c3eeb1c5/attachment.sig>

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> This should not be in ASoC library. The driver should be able to detect
> if a tasklet was called for many dma completions and notify the client
> (soc-lib) accordingly.

No.  Think about the cyclic case.  How do you correctly record how many
'periods' have completed so that when your tasklet eventually gets called,
you call its callback the correct number of times?

I think that's totally pointless and wasteful too from the ALSA core code
point of view - all the ALSA cares about is that "some periods have
completed" and ALSA will then ask the low level driver where the DMA is,
and update things according to the current DMA position.

It is my opinion that having this emulation of the current position in
soc-dmaengine-pcm is not only causing a wrong direction to be taken here,
but is potentially causing inefficiency by forcing conditions into the API
which we really don't need.

> One way to do this would be to ensure that the descriptor is marked in
> irq_handler and then all descriptor completed notified in tasklet.

No.  You're assuming that for each IRQ you have the tasklet run exactly
once.  There is no such guarantee.

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Fri, May 11, 2012 at 03:07:10PM +0100, Russell King - ARM Linux wrote:
> On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> > This should not be in ASoC library. The driver should be able to detect
> > if a tasklet was called for many dma completions and notify the client
> > (soc-lib) accordingly.

> No.  Think about the cyclic case.  How do you correctly record how many
> 'periods' have completed so that when your tasklet eventually gets called,
> you call its callback the correct number of times?

I do think that (separately to what ALSA does) if we're going to have a
callback specified per request we really should make sure that they all
get run as best as we can since the way the API is written people using
non-cyclic transfers may run into the same surprise; for cyclic ones the
issue you identify is very real though.

> It is my opinion that having this emulation of the current position in
> soc-dmaengine-pcm is not only causing a wrong direction to be taken here,
> but is potentially causing inefficiency by forcing conditions into the API
> which we really don't need.

I agree entirely, it's clearly just a bodge for not being able to read
the position and we'd be better off fixing that.

> > One way to do this would be to ensure that the descriptor is marked in
> > irq_handler and then all descriptor completed notified in tasklet.

> No.  You're assuming that for each IRQ you have the tasklet run exactly
> once.  There is no such guarantee.

I think the intention here was that the framework keep a list of all
pending transfers and then in the tasklet it runs through all the
completions up to the most recent one rather than only completing the
most recent one.  Easier said than done for cyclic of course...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120511/03e91aff/attachment.sig>

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Fri, May 11, 2012 at 03:18:35PM +0100, Mark Brown wrote:
> On Fri, May 11, 2012 at 03:07:10PM +0100, Russell King - ARM Linux wrote:
> > On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> > > This should not be in ASoC library. The driver should be able to detect
> > > if a tasklet was called for many dma completions and notify the client
> > > (soc-lib) accordingly.
> 
> > No.  Think about the cyclic case.  How do you correctly record how many
> > 'periods' have completed so that when your tasklet eventually gets called,
> > you call its callback the correct number of times?
> 
> I do think that (separately to what ALSA does) if we're going to have a
> callback specified per request we really should make sure that they all
> get run as best as we can since the way the API is written people using
> non-cyclic transfers may run into the same surprise; for cyclic ones the
> issue you identify is very real though.

Please stop confusing non-cyclic with cyclic transfers.  We know what the
required semantics are for non-cyclic transfers and they really aren't
under discussion here.  I'm focusing 100% on the cyclic case here.

> > It is my opinion that having this emulation of the current position in
> > soc-dmaengine-pcm is not only causing a wrong direction to be taken here,
> > but is potentially causing inefficiency by forcing conditions into the API
> > which we really don't need.
> 
> I agree entirely, it's clearly just a bodge for not being able to read
> the position and we'd be better off fixing that.
> 
> > > One way to do this would be to ensure that the descriptor is marked in
> > > irq_handler and then all descriptor completed notified in tasklet.
> 
> > No.  You're assuming that for each IRQ you have the tasklet run exactly
> > once.  There is no such guarantee.
> 
> I think the intention here was that the framework keep a list of all
> pending transfers and then in the tasklet it runs through all the
> completions up to the most recent one rather than only completing the
> most recent one.  Easier said than done for cyclic of course...

That's my point.  And keeping count of N times to call the callback in
the DMA engine driver is racy at best.

Here's what I think.  I think the best solution for cyclic transfers
is as follows:

1. The callback will be guaranteed to be called at least _once_ when one
   or more periods have expired.

2. There may be fewer callbacks issued than periods have expired.

3. All DMA engine drivers supporting cyclic transfers *must* without
   exception implement the residue in their tx_status function.

4. The residue returned _must_ be the number of bytes between the next
   byte to be read or written by DMA to/from memory and the end of the
   buffer - more explicitly, according to this mathematics:
	buffer_dma_start_address + buffer_size - next_dma_address

5. Users of DMA engine cyclic transfers are expected to use the tx_status
   function to determine how many periods have completed on each callback
   if this information matters to their operation.

Cyclic DMA - callback properties and tx_status residue

[Mark Brown]

On Fri, May 11, 2012 at 03:29:41PM +0100, Russell King - ARM Linux wrote:
> On Fri, May 11, 2012 at 03:18:35PM +0100, Mark Brown wrote:

> > I do think that (separately to what ALSA does) if we're going to have a
> > callback specified per request we really should make sure that they all
> > get run as best as we can since the way the API is written people using
> > non-cyclic transfers may run into the same surprise; for cyclic ones the
> > issue you identify is very real though.

> Please stop confusing non-cyclic with cyclic transfers.  We know what the
> required semantics are for non-cyclic transfers and they really aren't
> under discussion here.  I'm focusing 100% on the cyclic case here.

I'm not confusing them - I'm just pretty sure that all the issues which
you are identifying also apply to the non-cyclic case so while we're
looking at fixing things we (collectively, not particularly including
you as you're focused on the cyclic case) should look at that too.
Right now we're not handling either cyclic or non-cyclic cases well.

> That's my point.  And keeping count of N times to call the callback in
> the DMA engine driver is racy at best.

Yes, it'll always break down at some point - no argument there.

> Here's what I think.  I think the best solution for cyclic transfers
> is as follows:

I agree, for cyclic transfers this is a very good approach.  I'd
probably even set up a separate callback mechanism for cyclic transfers
to make this crystal clear and error out if an attempt was made to use
the linear one.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120511/b42d9312/attachment.sig>

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Fri, 2012-05-11 at 14:03 +0100, Russell King - ARM Linux wrote:
> On Fri, May 11, 2012 at 02:24:10PM +0200, Linus Walleij wrote:
> > > + * @residue: is defined as:
> > > + *   - for completed descriptor: 0
> > > + *   - for descriptors which are in DMA_PAUSED, DMA_IN_PROGRESS:
> > > + *     length - (current_addr - start_addr), where:
> > > + *             length = descriptor dma length
> > > + *             current_addr = current dma pointer queried for dma channel
> > > + *             start_addr: starting address of this descriptor
> > > + *   This is mandatory and needs to be correctly filled by driver
> > >  */
> > > Does this make it clear?
> > 
> > Looks good to me.
> 
> There's a problem with defining it that way.  Consider a scatterlist.
> What does 'current address' and 'start address' mean with respect to
> those transfers?
> 
> Rather than trying to define this in terms of mathematics, which will
> always lead to this kind of confusion, use English instead.
> 
> - for descriptors which are in progress or paused:
>   the number of bytes yet to be transferred to or from the memory for
>   MEM2DEV or DEV2MEM transfers.
> 
> This leaves MEM2MEM and DEV2DEV transfers ambiguous - both suffer from
> the problem of whether you use the source or destination pointers/lengths
> to calculate the residue.
in MEM2MEM both pointers should typically increment/decrement, so we
should consider any one of them. DEV2DEV is problematic unless h/w has a
way to tell you.
Btw some IPs like Synopsis ahb ones will decrement the length value, so
reading the length will effectively tell you where you are.
> 
> For MEM2MEM, I would suggest these should either be reported as complete
> or not-complete in their entirety.  (In other words, either their full
> transfer size or zero.)  It's racy to try to do anything else with an
> in-progress transfer...
Agree, I would also think for mem-copy which is much faster any query
would give not so accurate results. probably by the time you report
value the descriptor might have finished!
> 
> DEV2DEV I've not thought about, and I'm rather wishing we didn't have
> that transfer type...
well everything boils down to hardware capability. I would expect decent
hardware design to be able to report.
But why would someone take the data inside SoC just to send it back.
Would anyone have a practical use of this?

-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Fri, 2012-05-11 at 15:07 +0100, Russell King - ARM Linux wrote:
> On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> > This should not be in ASoC library. The driver should be able to detect
> > if a tasklet was called for many dma completions and notify the client
> > (soc-lib) accordingly.
> 
> No.  Think about the cyclic case.  How do you correctly record how many
> 'periods' have completed so that when your tasklet eventually gets called,
> you call its callback the correct number of times?
In ISR you can find which descriptors the interupt is for and then use
this info in tasklet.

> 
> I think that's totally pointless and wasteful too from the ALSA core code
> point of view - all the ALSA cares about is that "some periods have
> completed" and ALSA will then ask the low level driver where the DMA is,
> and update things according to the current DMA position.
> 
> It is my opinion that having this emulation of the current position in
> soc-dmaengine-pcm is not only causing a wrong direction to be taken here,
> but is potentially causing inefficiency by forcing conditions into the API
> which we really don't need.
> 
> > One way to do this would be to ensure that the descriptor is marked in
> > irq_handler and then all descriptor completed notified in tasklet.
> 
> No.  You're assuming that for each IRQ you have the tasklet run exactly
> once.  There is no such guarantee.
No, I am assuming IRQ is called everytime and takslet sometimes!
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
~Vinod

Cyclic DMA - callback properties and tx_status residue

[Russell King - ARM Linux]

On Tue, May 15, 2012 at 10:37:26AM +0530, Vinod Koul wrote:
> On Fri, 2012-05-11 at 15:07 +0100, Russell King - ARM Linux wrote:
> > On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> > > This should not be in ASoC library. The driver should be able to detect
> > > if a tasklet was called for many dma completions and notify the client
> > > (soc-lib) accordingly.
> > 
> > No.  Think about the cyclic case.  How do you correctly record how many
> > 'periods' have completed so that when your tasklet eventually gets called,
> > you call its callback the correct number of times?
> In ISR you can find which descriptors the interupt is for and then use
> this info in tasklet.

What descriptors?  With cyclic DMA you only have one descriptor.  Your
DMA engine may only need to be programmed once to do this too.

Cyclic DMA - callback properties and tx_status residue

[Vinod Koul]

On Tue, 2012-05-15 at 08:37 +0100, Russell King - ARM Linux wrote:
> On Tue, May 15, 2012 at 10:37:26AM +0530, Vinod Koul wrote:
> > On Fri, 2012-05-11 at 15:07 +0100, Russell King - ARM Linux wrote:
> > > On Thu, May 10, 2012 at 04:31:15PM +0530, Vinod Koul wrote:
> > > > This should not be in ASoC library. The driver should be able to detect
> > > > if a tasklet was called for many dma completions and notify the client
> > > > (soc-lib) accordingly.
> > > 
> > > No.  Think about the cyclic case.  How do you correctly record how many
> > > 'periods' have completed so that when your tasklet eventually gets called,
> > > you call its callback the correct number of times?
> > In ISR you can find which descriptors the interupt is for and then use
> > this info in tasklet.
> 
> What descriptors?  With cyclic DMA you only have one descriptor.  Your
> DMA engine may only need to be programmed once to do this too.
My bad mixed my reply to generic case where you can mark descriptor that
interrupt is triggered and later in tasklet call pending notifications.

Said that, nothing prevents the driver from keeping the count of
interrupts and calling the callbacks. BUT it makes no sense in cyclic
case where ALSA wouldn't need this and would need just a notification
and current position.


-- 
~Vinod