[PATCH] [RFC PATCH] dmaengine: pl330: residual - do not reset on last descriptor.

[PATCH] [RFC PATCH] dmaengine: pl330: residual - do not reset on last descriptor.

[Alban Browaeys]

Removed the residual rest to zero on last descriptor.

Also set residue granularity to BURST instead of SEGMENT.
This last item needs more investigation before I could give a rationale
for it.

Signed-off-by: Alban Browaeys <prahal@yahoo.com>
Reported-by: gabriel@unseen.is
Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status()
function")

---

My older local version of the residue :
https://github.com/prahal/linux/blob/odroid-3.19/drivers/dma/pl330.c#L2230

I took a second look at both and noticed that in the upstream version,
I was not able to make sense of the "if desc->last then reset residual
to zero" in the loop. Early tests gives a proper sound ouput with it
removed.

More tests are welcome, and it could be the flag change is wrong.

There is also a bad interaction with pulseaudio when the buffer size is
small : https://bugs.freedesktop.org/show_bug.cgi?id=84878. This
might affect testing.
---
 drivers/dma/pl330.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index f513f77..0851f41 100644
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -2275,8 +2275,6 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
 			}
 			break;
 		}
-		if (desc->last)
-			residual = 0;
 	}
 	spin_unlock_irqrestore(&pch->lock, flags);
 
@@ -2890,7 +2888,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
 	pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
 	pd->dst_addr_widths = PL330_DMA_BUSWIDTHS;
 	pd->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
-	pd->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
+	pd->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 
 	ret = dma_async_device_register(pd);
 	if (ret) {
-- 
2.1.4

Re: [PATCH] [RFC PATCH] dmaengine: pl330: residual - do not reset on last descriptor.

[Krzysztof Kozlowski]

2015-07-07 20:05 GMT+09:00 Alban Browaeys <alban.browaeys@gmail.com>:
> Removed the residual rest to zero on last descriptor.
>
> Also set residue granularity to BURST instead of SEGMENT.
> This last item needs more investigation before I could give a rationale
> for it.
>
> Signed-off-by: Alban Browaeys <prahal@yahoo.com>
> Reported-by: gabriel@unseen.is
> Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status()
> function")
>

Hi,

I am not quite sure what bug you are trying to fix here. It isn't
described in commit message. Also I don't get why the residue=0 is
being removed. This will break the residue calculation...

Some time ago I posted a patch fixing sound issues on Odroid XU3.
Today Vinod applied it:
https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-dma.git/commit/?h=fixes&id=5dd90e5b91e0f5c925b12b132c7cd27538870256

I don't know if this is the same issue... can you describe exactly
what you are trying to fix, why and how?

Best regards,
Krzysztof

> ---
>
> My older local version of the residue :
> https://github.com/prahal/linux/blob/odroid-3.19/drivers/dma/pl330.c#L2230
>
> I took a second look at both and noticed that in the upstream version,
> I was not able to make sense of the "if desc->last then reset residual
> to zero" in the loop. Early tests gives a proper sound ouput with it
> removed.
>
> More tests are welcome, and it could be the flag change is wrong.
>
> There is also a bad interaction with pulseaudio when the buffer size is
> small : https://bugs.freedesktop.org/show_bug.cgi?id=84878. This
> might affect testing.
> ---
>  drivers/dma/pl330.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index f513f77..0851f41 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2275,8 +2275,6 @@ pl330_tx_status(struct dma_chan *chan, dma_cookie_t cookie,
>                         }
>                         break;
>                 }
> -               if (desc->last)
> -                       residual = 0;
>         }
>         spin_unlock_irqrestore(&pch->lock, flags);
>
> @@ -2890,7 +2888,7 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>         pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
>         pd->dst_addr_widths = PL330_DMA_BUSWIDTHS;
>         pd->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> -       pd->residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> +       pd->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
>
>         ret = dma_async_device_register(pd);
>         if (ret) {
> --
> 2.1.4
>
> --
> 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

Re: [PATCH] [RFC PATCH] dmaengine: pl330: residual - do not reset on last descriptor.

[Alban Browaeys]

Le mardi 07 juillet 2015 à 20:53 +0900, Krzysztof Kozlowski a écrit :
> 2015-07-07 20:05 GMT+09:00 Alban Browaeys <alban.browaeys@gmail.com>:
> > Removed the residual rest to zero on last descriptor.
> > 
> > Also set residue granularity to BURST instead of SEGMENT.
> > This last item needs more investigation before I could give a 
> > rationale
> > for it.
> > 
> > Signed-off-by: Alban Browaeys <prahal@yahoo.com>
> > Reported-by: gabriel@unseen.is
> > Reported-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Fixes: aee4d1fac887 ("dmaengine: pl330: improve pl330_tx_status()
> > function")
> > 
> 
> Hi,
> 
> I am not quite sure what bug you are trying to fix here. It isn't
> described in commit message. Also I don't get why the residue=0 is
> being removed. This will break the residue calculation...
> 
> Some time ago I posted a patch fixing sound issues on Odroid XU3.
> Today Vinod applied it:
> https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave
> -dma.git/commit/?h=fixes&id=5dd90e5b91e0f5c925b12b132c7cd27538870256
> 
> I don't know if this is the same issue... can you describe exactly
> what you are trying to fix, why and how?
> 
> 

This was about "choppy sound" issue left after the dma pause fix:
https://www.mail-archive.com/linux-samsung
-soc%40vger.kernel.org/msg44567.html

From a glance at the commit you mention I would say it is the same
issue. Will try this commit instead in a few hours and report.


Thanks
Alban


> choppy sound) on Odroid XU3-Lite. On current linux-next
> (next-20150612) the sound is awful (choppy) after few seconds of 
> play.
> For example first four of:
> $ aplay /usr/share/sounds/alsa/Front_Right.wav
> work fine. But then it just gets worse and underruns are reported:
> $ Playing WAVE '/usr/share/sounds/alsa/Front_Right.wav' : Signed 16
> bit Little Endian, Rate 48000 Hz, Mono
> $ underrun!!! (at least 0.095 ms long)

Re: [PATCH] [RFC PATCH] dmaengine: pl330: residual - do not reset on last descriptor.

[Alban Browaeys]

Le mardi 07 juillet 2015 à 14:13 +0200, Alban Browaeys a écrit :
> Le mardi 07 juillet 2015 à 20:53 +0900, Krzysztof Kozlowski a écrit 

> > > 
> > I am not quite sure what bug you are trying to fix here. It isn't
> > described in commit message. Also I don't get why the residue=0 is
> > being removed. This will break the residue calculation...
> > 
> > Some time ago I posted a patch fixing sound issues on Odroid XU3.
> > Today Vinod applied it:
> > https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave
> > -dma.git/commit/?h=fixes&id=5dd90e5b91e0f5c925b12b132c7cd2753887025
> > 6
> > 
> > I don't know if this is the same issue... can you describe exactly
> > what you are trying to fix, why and how?
> > 
> > 
> 
> This was about "choppy sound" issue left after the dma pause fix:
> https://www.mail-archive.com/linux-samsung
> -soc%40vger.kernel.org/msg44567.html
> 
> From a glance at the commit you mention I would say it is the same
> issue. Will try this commit instead in a few hours and report.

The commit of yours, that is 
https://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-dma.git/commit
/?h=fixes&id=5dd90e5b91e0f5c925b12b132c7cd2753887025 ,
fixes  Odroid U2/U3 "choppy sound" too.


Thanks.
Alban 

> 
> > choppy sound) on Odroid XU3-Lite. On current linux-next
> > (next-20150612) the sound is awful (choppy) after few seconds of 
> > play.
> > For example first four of:
> > $ aplay /usr/share/sounds/alsa/Front_Right.wav
> > work fine. But then it just gets worse and underruns are reported:
> > $ Playing WAVE '/usr/share/sounds/alsa/Front_Right.wav' : Signed 16
> > bit Little Endian, Rate 48000 Hz, Mono