[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[Jassi]

S3C2410_DMAF_CIRCULAR requires that we don't free buffer
resources after TC-IRQ because we will revisit the buffer.
During simple long term playback/capture, like movie-play,
this may block those resources(kmem/dma_pool) unncessarily.
This is especially a problem since we allocate them in IRQ
context.
For now, we disable the option and free buff resources after
it's TC-IRQ. Also, chan->curr will point to the active buffer
while chan->next is rendered useless.

Signed-Off-by: Jassi <jassi.brar@samsung.com>
---
 arch/arm/plat-s3c64xx/dma.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-s3c64xx/dma.c b/arch/arm/plat-s3c64xx/dma.c
index 266a107..02bc82b 100644
--- a/arch/arm/plat-s3c64xx/dma.c
+++ b/arch/arm/plat-s3c64xx/dma.c
@@ -375,10 +375,12 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id,
 		end->next = buff;
 		endlli->next_lli = buff->lli_dma;
 
+#if 0
 		if (chan->flags & S3C2410_DMAF_CIRCULAR) {
 			struct s3c64xx_dma_buff *curr = chan->curr;
 			lli->next_lli = curr->lli_dma;
 		}
+#endif
 
 		if (next == chan->curr) {
 			writel(buff->lli_dma, chan->regs + PL080_CH_LLI);
-- 
1.6.2.5

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[Mark Brown]

On Tue, Sep 15, 2009 at 07:01:22PM +0900, Jassi wrote:

> +#if 0
>  		if (chan->flags & S3C2410_DMAF_CIRCULAR) {
>  			struct s3c64xx_dma_buff *curr = chan->curr;
>  			lli->next_lli = curr->lli_dma;
>  		}
> +#endif

If you're doing something like this you should remove the code rather
than if 0 it out, or at least insert a comment explaining why the code
is sitting around.  Though I'm a bit concerned that if something has
actually tried to set up circular DMA it might get upset if that's
ignored (not that I've looked in detail).

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[jassi brar]

On Tue, Sep 15, 2009 at 7:07 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 15, 2009 at 07:01:22PM +0900, Jassi wrote:
>
>> +#if 0
>> ? ? ? ? ? ? ? if (chan->flags & S3C2410_DMAF_CIRCULAR) {
>> ? ? ? ? ? ? ? ? ? ? ? struct s3c64xx_dma_buff *curr = chan->curr;
>> ? ? ? ? ? ? ? ? ? ? ? lli->next_lli = curr->lli_dma;
>> ? ? ? ? ? ? ? }
>> +#endif
>
> If you're doing something like this you should remove the code rather
> than if 0 it out, or at least insert a comment explaining why the code
> is sitting around.
Yes, infact the comment in changelog was moved from here.
I wanted to remove it altogether, but ....

> ?Though I'm a bit concerned that if something has
> actually tried to set up circular DMA it might get upset if that's
> ignored (not that I've looked in detail).
That shudn't be an issue.
S3C64xx dma isn't used by any code other than I2S which doesn't
have any machine driver.

If the decision to disable CIRCULAR flag is acceptable, i will resend the patch.

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[Mark Brown]

On Tue, Sep 15, 2009 at 08:01:39PM +0900, jassi brar wrote:
> On Tue, Sep 15, 2009 at 7:07 PM, Mark Brown

> > ?Though I'm a bit concerned that if something has
> > actually tried to set up circular DMA it might get upset if that's
> > ignored (not that I've looked in detail).

> That shudn't be an issue.
> S3C64xx dma isn't used by any code other than I2S which doesn't
> have any machine driver.

I'd expect that to change very soon, though?

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[jassi brar]

On Tue, Sep 15, 2009 at 8:52 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Sep 15, 2009 at 08:01:39PM +0900, jassi brar wrote:
>> On Tue, Sep 15, 2009 at 7:07 PM, Mark Brown
>
>> > ?Though I'm a bit concerned that if something has
>> > actually tried to set up circular DMA it might get upset if that's
>> > ignored (not that I've looked in detail).
>
>> That shudn't be an issue.
>> S3C64xx dma isn't used by any code other than I2S which doesn't
>> have any machine driver.
>
> I'd expect that to change very soon, though?
Yes, tomorrow i plan to submit a basic SMDK6410-WM8580 I2S-v4
*2channels* driver.
Later my spi driver too, that makes use of DMA.

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[Ben Dooks]

On Tue, Sep 15, 2009 at 07:01:22PM +0900, Jassi wrote:
> S3C2410_DMAF_CIRCULAR requires that we don't free buffer
> resources after TC-IRQ because we will revisit the buffer.
> During simple long term playback/capture, like movie-play,
> this may block those resources(kmem/dma_pool) unncessarily.
> This is especially a problem since we allocate them in IRQ
> context.
> For now, we disable the option and free buff resources after
> it's TC-IRQ. Also, chan->curr will point to the active buffer
> while chan->next is rendered useless.

This would have been better changing the sound driver and ensuring
it does not allocate as much memory for buffering audio.

The whole point of the circular buffer is it is allocated once and
then used until the end of playback. If what you are saying is right,
and resources are running out then somewhere either the SoC layer is
losing them or between the DMA and the I2S driver we are losing
buffers. This points to a bug in the resource management of one of
them.
 
> Signed-Off-by: Jassi <jassi.brar@samsung.com>
> ---
>  arch/arm/plat-s3c64xx/dma.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-s3c64xx/dma.c b/arch/arm/plat-s3c64xx/dma.c
> index 266a107..02bc82b 100644
> --- a/arch/arm/plat-s3c64xx/dma.c
> +++ b/arch/arm/plat-s3c64xx/dma.c
> @@ -375,10 +375,12 @@ int s3c2410_dma_enqueue(unsigned int channel, void *id,
>  		end->next = buff;
>  		endlli->next_lli = buff->lli_dma;
>  
> +#if 0
>  		if (chan->flags & S3C2410_DMAF_CIRCULAR) {
>  			struct s3c64xx_dma_buff *curr = chan->curr;
>  			lli->next_lli = curr->lli_dma;
>  		}
> +#endif
>  
>  		if (next == chan->curr) {
>  			writel(buff->lli_dma, chan->regs + PL080_CH_LLI);
> -- 
> 1.6.2.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[jassi brar]

On Wed, Sep 16, 2009 at 9:28 AM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Tue, Sep 15, 2009 at 07:01:22PM +0900, Jassi wrote:
>> S3C2410_DMAF_CIRCULAR requires that we don't free buffer
>> resources after TC-IRQ because we will revisit the buffer.
>> During simple long term playback/capture, like movie-play,
>> this may block those resources(kmem/dma_pool) unncessarily.
>> This is especially a problem since we allocate them in IRQ
>> context.
>> For now, we disable the option and free buff resources after
>> it's TC-IRQ. Also, chan->curr will point to the active buffer
>> while chan->next is rendered useless.
>
> This would have been better changing the sound driver and ensuring
> it does not allocate as much memory for buffering audio.
>
> The whole point of the circular buffer is it is allocated once and
> then used until the end of playback. If what you are saying is right,
> and resources are running out then somewhere either the SoC layer is
> losing them or between the DMA and the I2S driver we are losing
> buffers. This points to a bug in the resource management of one of
> them.
Please correct if any of the following is wrong as per the existing code:-
In DMA driver:-
   1) A 'struct s3c64xx_dma_buff' and a 'struct pl080s_lli' is allocated
 during every call to s3c2410_dma_enqueue.
    2) These resources are freed only when s3c64xx_dma_ctrl(FLUSH)
 is called by the dma client.

 In Dma Client Driver(I2S):-
   1) DMA is aquired and setup once before active continuous use
       and s3c2410_dma_enqueue is called as many times as the client
has some data to transfer, without any other call to dma driver.
   2) Unless there is some suspend/resume or pause/play event,
s3c64xx_dma_ctrl(FLUSH) may only be called after playback/capture ends.

Now consider the situation when we didn't set the CIRCULAR flag.
During this time the buffer resources are simply piling up without being
ever reused. Note that the playback maybe of many hours and at
some point dma_pool_alloc(and later kzalloc) will start returning NULL.

Besides,
a) The dma driver doesn't free those resource even for
Non-CIRCULAR flagged requests.
b) Only I2S driver makes use of the DMA driver(curently). To make
use of the feature(which is available only in 64xx dma driver) the 64xx
i2s driver will have to break from 24xx drivers(which we want to unify
ultimately?) and ALSA already provides a buffer looping mechanism.

[PATCH 4/7] S3C64XX DMA: S3C2410_DMAF_CIRCULAR disable

[Ben Dooks]

On Wed, Sep 16, 2009 at 11:24:47AM +0900, jassi brar wrote:
> On Wed, Sep 16, 2009 at 9:28 AM, Ben Dooks <ben-linux@fluff.org> wrote:
> > On Tue, Sep 15, 2009 at 07:01:22PM +0900, Jassi wrote:
> >> S3C2410_DMAF_CIRCULAR requires that we don't free buffer
> >> resources after TC-IRQ because we will revisit the buffer.
> >> During simple long term playback/capture, like movie-play,
> >> this may block those resources(kmem/dma_pool) unncessarily.
> >> This is especially a problem since we allocate them in IRQ
> >> context.
> >> For now, we disable the option and free buff resources after
> >> it's TC-IRQ. Also, chan->curr will point to the active buffer
> >> while chan->next is rendered useless.
> >
> > This would have been better changing the sound driver and ensuring
> > it does not allocate as much memory for buffering audio.
> >
> > The whole point of the circular buffer is it is allocated once and
> > then used until the end of playback. If what you are saying is right,
> > and resources are running out then somewhere either the SoC layer is
> > losing them or between the DMA and the I2S driver we are losing
> > buffers. This points to a bug in the resource management of one of
> > them.
> Please correct if any of the following is wrong as per the existing code:-
> In DMA driver:-
>    1) A 'struct s3c64xx_dma_buff' and a 'struct pl080s_lli' is allocated
>  during every call to s3c2410_dma_enqueue.
>     2) These resources are freed only when s3c64xx_dma_ctrl(FLUSH)
>  is called by the dma client.
> 
>  In Dma Client Driver(I2S):-
>    1) DMA is aquired and setup once before active continuous use
>        and s3c2410_dma_enqueue is called as many times as the client
> has some data to transfer, without any other call to dma driver.
>    2) Unless there is some suspend/resume or pause/play event,
> s3c64xx_dma_ctrl(FLUSH) may only be called after playback/capture ends.
> 
> Now consider the situation when we didn't set the CIRCULAR flag.
> During this time the buffer resources are simply piling up without being
> ever reused. Note that the playback maybe of many hours and at
> some point dma_pool_alloc(and later kzalloc) will start returning NULL.
> 
> Besides,
> a) The dma driver doesn't free those resource even for
> Non-CIRCULAR flagged requests.
> b) Only I2S driver makes use of the DMA driver(curently). To make
> use of the feature(which is available only in 64xx dma driver) the 64xx
> i2s driver will have to break from 24xx drivers(which we want to unify
> ultimately?) and ALSA already provides a buffer looping mechanism.

Yes, you are right, having gone back over the original code tree that
was produced I've found the final patch in the series that never got
sent which defines the correct use of the DMAF_CIRCUALR and ensures
that the ASoC driver uses it correctly.

In this case, the ASoC driver uses the S3C2410_DMAF_CIRCULAR to
enqueue the buffers once when the stream is initialised and then
does not call enqueue again.

I will post a patch to the lists with this functionality back in so
that it can be tested and merged.

-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.