diff mbox

dmaengine: pl330: Really fix choppy sound because of wrong residue calculation

Message ID 1434376809-8029-1-git-send-email-k.kozlowski.k@gmail.com (mailing list archive)
State Accepted
Headers show

Commit Message

Krzysztof Kozlowski June 15, 2015, 2 p.m. UTC
When pl330 driver was used during sound playback, after some time or
after a number of plays the sound became choppy or totally noisy. For
example on Odroid XU3 board the first four executions of aplay with
small WAVE worked fine, but fifth was unrecognizable with errors:
	$ aplay /usr/share/sounds/alsa/Front_Right.wava
	underrun!!! (at least 0.095 ms long)

Issue was caused by wrong residue reported by pl330 driver to
pcm_dmaengine for its cyclic dma transfers.

The pl330_tx_status(), residue reporting function, used a "last" flag in
a descriptor to indicate that there is no more data to send.

The pl330_tx_submit() iterated over descriptors trying to remove this
flag from them and then mark last descriptor as "last".  However when
iterating it actually removed the flag not from descriptors but always
from last of it (and then reset it). Thus effectively once some
descriptor was marked as last, then it stayed like this forever causing
residue to be reported too low.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")
Cc: <stable@vger.kernel.org>
Reported-by: gabriel@unseen.is
Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>

---

This fixes sound on my Odroid XU3. Tests on other boards are welcomed!
---
 drivers/dma/pl330.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Krzysztof Kozlowski June 22, 2015, 2:14 a.m. UTC | #1
2015-06-15 23:00 GMT+09:00 Krzysztof Kozlowski <k.kozlowski.k@gmail.com>:
> When pl330 driver was used during sound playback, after some time or
> after a number of plays the sound became choppy or totally noisy. For
> example on Odroid XU3 board the first four executions of aplay with
> small WAVE worked fine, but fifth was unrecognizable with errors:
>         $ aplay /usr/share/sounds/alsa/Front_Right.wava
>         underrun!!! (at least 0.095 ms long)
>
> Issue was caused by wrong residue reported by pl330 driver to
> pcm_dmaengine for its cyclic dma transfers.
>
> The pl330_tx_status(), residue reporting function, used a "last" flag in
> a descriptor to indicate that there is no more data to send.
>
> The pl330_tx_submit() iterated over descriptors trying to remove this
> flag from them and then mark last descriptor as "last".  However when
> iterating it actually removed the flag not from descriptors but always
> from last of it (and then reset it). Thus effectively once some
> descriptor was marked as last, then it stayed like this forever causing
> residue to be reported too low.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
> Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")
> Cc: <stable@vger.kernel.org>
> Reported-by: gabriel@unseen.is
> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Gabriel, could you give this patch a try? It helped for my Odroid XU3,
which has the same audio codec. Nevertheless it would be great to hear
that it solves the initial bug report.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
Vinod Koul June 30, 2015, 4:14 a.m. UTC | #2
On Mon, Jun 22, 2015 at 11:14:42AM +0900, Krzysztof Koz?owski wrote:
> 2015-06-15 23:00 GMT+09:00 Krzysztof Kozlowski <k.kozlowski.k@gmail.com>:
> > When pl330 driver was used during sound playback, after some time or
> > after a number of plays the sound became choppy or totally noisy. For
> > example on Odroid XU3 board the first four executions of aplay with
> > small WAVE worked fine, but fifth was unrecognizable with errors:
> >         $ aplay /usr/share/sounds/alsa/Front_Right.wava
> >         underrun!!! (at least 0.095 ms long)
> >
> > Issue was caused by wrong residue reported by pl330 driver to
> > pcm_dmaengine for its cyclic dma transfers.
> >
> > The pl330_tx_status(), residue reporting function, used a "last" flag in
> > a descriptor to indicate that there is no more data to send.
> >
> > The pl330_tx_submit() iterated over descriptors trying to remove this
> > flag from them and then mark last descriptor as "last".  However when
> > iterating it actually removed the flag not from descriptors but always
> > from last of it (and then reset it). Thus effectively once some
> > descriptor was marked as last, then it stayed like this forever causing
> > residue to be reported too low.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
> > Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")
> > Cc: <stable@vger.kernel.org>
> > Reported-by: gabriel@unseen.is
> > Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Gabriel, could you give this patch a try? It helped for my Odroid XU3,
> which has the same audio codec. Nevertheless it would be great to hear
> that it solves the initial bug report.

So is there a word on  what finally fixes this
Krzysztof Kozlowski June 30, 2015, 4:51 a.m. UTC | #3
2015-06-30 13:14 GMT+09:00 Vinod Koul <vinod.koul@intel.com>:
> On Mon, Jun 22, 2015 at 11:14:42AM +0900, Krzysztof Koz?owski wrote:
>> 2015-06-15 23:00 GMT+09:00 Krzysztof Kozlowski <k.kozlowski.k@gmail.com>:
>> > When pl330 driver was used during sound playback, after some time or
>> > after a number of plays the sound became choppy or totally noisy. For
>> > example on Odroid XU3 board the first four executions of aplay with
>> > small WAVE worked fine, but fifth was unrecognizable with errors:
>> >         $ aplay /usr/share/sounds/alsa/Front_Right.wava
>> >         underrun!!! (at least 0.095 ms long)
>> >
>> > Issue was caused by wrong residue reported by pl330 driver to
>> > pcm_dmaengine for its cyclic dma transfers.
>> >
>> > The pl330_tx_status(), residue reporting function, used a "last" flag in
>> > a descriptor to indicate that there is no more data to send.
>> >
>> > The pl330_tx_submit() iterated over descriptors trying to remove this
>> > flag from them and then mark last descriptor as "last".  However when
>> > iterating it actually removed the flag not from descriptors but always
>> > from last of it (and then reset it). Thus effectively once some
>> > descriptor was marked as last, then it stayed like this forever causing
>> > residue to be reported too low.
>> >
>> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
>> > Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")
>> > Cc: <stable@vger.kernel.org>
>> > Reported-by: gabriel@unseen.is
>> > Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> Gabriel, could you give this patch a try? It helped for my Odroid XU3,
>> which has the same audio codec. Nevertheless it would be great to hear
>> that it solves the initial bug report.
>
> So is there a word on  what finally fixes this

This fixes sound issue on my Odroid XU3 board, which is good. My board
has the same audio codec and DMA driver as the board used by Gabriel
so there is high chance that his issue is also fixed.

However it is sad that I did not receive any reviews or tests from
anyone, apparently no one cares enough about it :) .

Anyway the patch was also merged to Tizen kernel tree:
https://review.tizen.org/git/?p=platform/kernel/linux-exynos.git;a=shortlog;h=refs/heads/tizen
https://review.tizen.org/git/?p=platform/kernel/linux-exynos.git;a=commit;h=41dec9884a82443da1b7814ae42efe65895ff3af

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lars-Peter Clausen June 30, 2015, 7:01 a.m. UTC | #4
On 06/30/2015 06:51 AM, Krzysztof Koz?owski wrote:
> 2015-06-30 13:14 GMT+09:00 Vinod Koul <vinod.koul@intel.com>:
>> On Mon, Jun 22, 2015 at 11:14:42AM +0900, Krzysztof Koz?owski wrote:
>>> 2015-06-15 23:00 GMT+09:00 Krzysztof Kozlowski <k.kozlowski.k@gmail.com>:
>>>> When pl330 driver was used during sound playback, after some time or
>>>> after a number of plays the sound became choppy or totally noisy. For
>>>> example on Odroid XU3 board the first four executions of aplay with
>>>> small WAVE worked fine, but fifth was unrecognizable with errors:
>>>>          $ aplay /usr/share/sounds/alsa/Front_Right.wava
>>>>          underrun!!! (at least 0.095 ms long)
>>>>
>>>> Issue was caused by wrong residue reported by pl330 driver to
>>>> pcm_dmaengine for its cyclic dma transfers.
>>>>
>>>> The pl330_tx_status(), residue reporting function, used a "last" flag in
>>>> a descriptor to indicate that there is no more data to send.
>>>>
>>>> The pl330_tx_submit() iterated over descriptors trying to remove this
>>>> flag from them and then mark last descriptor as "last".  However when
>>>> iterating it actually removed the flag not from descriptors but always
>>>> from last of it (and then reset it). Thus effectively once some
>>>> descriptor was marked as last, then it stayed like this forever causing
>>>> residue to be reported too low.
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
>>>> Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status() function")
>>>> Cc: <stable@vger.kernel.org>
>>>> Reported-by: gabriel@unseen.is
>>>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>> Gabriel, could you give this patch a try? It helped for my Odroid XU3,
>>> which has the same audio codec. Nevertheless it would be great to hear
>>> that it solves the initial bug report.
>>
>> So is there a word on  what finally fixes this
>
> This fixes sound issue on my Odroid XU3 board, which is good. My board
> has the same audio codec and DMA driver as the board used by Gabriel
> so there is high chance that his issue is also fixed.
>
> However it is sad that I did not receive any reviews or tests from
> anyone, apparently no one cares enough about it :) .

I care, I've had it in applied to my local tree for a while now. Without it 
audio does not work.

Tested-by: Lars-Peter Clausen <lars@metafoo.de>
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski June 30, 2015, 7:57 a.m. UTC | #5
2015-06-30 16:01 GMT+09:00 Lars-Peter Clausen <lars@metafoo.de>:
> On 06/30/2015 06:51 AM, Krzysztof Koz?owski wrote:
>>
>> 2015-06-30 13:14 GMT+09:00 Vinod Koul <vinod.koul@intel.com>:
>>>
>>> On Mon, Jun 22, 2015 at 11:14:42AM +0900, Krzysztof Koz?owski wrote:
>>>>
>>>> 2015-06-15 23:00 GMT+09:00 Krzysztof Kozlowski
>>>> <k.kozlowski.k@gmail.com>:
>>>>>
>>>>> When pl330 driver was used during sound playback, after some time or
>>>>> after a number of plays the sound became choppy or totally noisy. For
>>>>> example on Odroid XU3 board the first four executions of aplay with
>>>>> small WAVE worked fine, but fifth was unrecognizable with errors:
>>>>>          $ aplay /usr/share/sounds/alsa/Front_Right.wava
>>>>>          underrun!!! (at least 0.095 ms long)
>>>>>
>>>>> Issue was caused by wrong residue reported by pl330 driver to
>>>>> pcm_dmaengine for its cyclic dma transfers.
>>>>>
>>>>> The pl330_tx_status(), residue reporting function, used a "last" flag
>>>>> in
>>>>> a descriptor to indicate that there is no more data to send.
>>>>>
>>>>> The pl330_tx_submit() iterated over descriptors trying to remove this
>>>>> flag from them and then mark last descriptor as "last".  However when
>>>>> iterating it actually removed the flag not from descriptors but always
>>>>> from last of it (and then reset it). Thus effectively once some
>>>>> descriptor was marked as last, then it stayed like this forever causing
>>>>> residue to be reported too low.
>>>>>
>>>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski.k@gmail.com>
>>>>> Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status()
>>>>> function")
>>>>> Cc: <stable@vger.kernel.org>
>>>>> Reported-by: gabriel@unseen.is
>>>>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>
>>>>
>>>> Gabriel, could you give this patch a try? It helped for my Odroid XU3,
>>>> which has the same audio codec. Nevertheless it would be great to hear
>>>> that it solves the initial bug report.
>>>
>>>
>>> So is there a word on  what finally fixes this
>>
>>
>> This fixes sound issue on my Odroid XU3 board, which is good. My board
>> has the same audio codec and DMA driver as the board used by Gabriel
>> so there is high chance that his issue is also fixed.
>>
>> However it is sad that I did not receive any reviews or tests from
>> anyone, apparently no one cares enough about it :) .
>
>
> I care, I've had it in applied to my local tree for a while now. Without it
> audio does not work.
>
> Tested-by: Lars-Peter Clausen <lars@metafoo.de>

Thanks, I appreciate this!

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul July 7, 2015, 4 a.m. UTC | #6
On Mon, Jun 15, 2015 at 11:00:09PM +0900, Krzysztof Kozlowski wrote:
> When pl330 driver was used during sound playback, after some time or
> after a number of plays the sound became choppy or totally noisy. For
> example on Odroid XU3 board the first four executions of aplay with
> small WAVE worked fine, but fifth was unrecognizable with errors:
> 	$ aplay /usr/share/sounds/alsa/Front_Right.wava
> 	underrun!!! (at least 0.095 ms long)
> 
> Issue was caused by wrong residue reported by pl330 driver to
> pcm_dmaengine for its cyclic dma transfers.
> 
> The pl330_tx_status(), residue reporting function, used a "last" flag in
> a descriptor to indicate that there is no more data to send.
> 
> The pl330_tx_submit() iterated over descriptors trying to remove this
> flag from them and then mark last descriptor as "last".  However when
> iterating it actually removed the flag not from descriptors but always
> from last of it (and then reset it). Thus effectively once some
> descriptor was marked as last, then it stayed like this forever causing
> residue to be reported too low.

Applied, thanks
diff mbox

Patch

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f513f77b1d85..4feb65a6b3b0 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2328,7 +2328,7 @@  static dma_cookie_t pl330_tx_submit(struct dma_async_tx_descriptor *tx)
 			desc->txd.callback = last->txd.callback;
 			desc->txd.callback_param = last->txd.callback_param;
 		}
-		last->last = false;
+		desc->last = false;
 
 		dma_cookie_assign(&desc->txd);