[PATCH 0/1] Fix ADI axi-i2s + Xilinx AXI-DMA capture

[PATCH 0/1] Fix ADI axi-i2s + Xilinx AXI-DMA capture

[Luca Ceresoli]

Hi,

here is a fix for a nasty audio capture problem when the axi-i2s
output stream is fed to a Xilinx AXI-DMA.

The commit is simple and hopefully well described, but I am not 100%
sure the solution is the correct one. If it is not, I'll be glad to
know which one is the best. It solved my problem anyway.

Luca


Luca Ceresoli (1):
  axi-i2s: set period size register

 sound/soc/adi/axi-i2s.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH 1/1] axi-i2s: set period size register

[Luca Ceresoli]

The default value of the PERIOD_LEN register is 0 and results in
axi-i2s keeping TLAST always asserted in its AXI Stream output.

When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
DMA generating an interrupt flood and ALSA produce a corrupted
recording. This is because AXI-DMA raises an interrupt whenever TLAST
is active.

Fix by setting the PERIOD_LEN register as soon as the period is
known. This way TLAST is emitted once per period, and the DMA raises
interrupts correctly.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 sound/soc/adi/axi-i2s.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/sound/soc/adi/axi-i2s.c b/sound/soc/adi/axi-i2s.c
index 4c23381727a1..af581a313a40 100644
--- a/sound/soc/adi/axi-i2s.c
+++ b/sound/soc/adi/axi-i2s.c
@@ -24,6 +24,7 @@
 #define AXI_I2S_REG_CTRL	0x04
 #define AXI_I2S_REG_CLK_CTRL	0x08
 #define AXI_I2S_REG_STATUS	0x10
+#define AXI_I2S_REG_PERIOD_LEN	0x18
 
 #define AXI_I2S_REG_RX_FIFO	0x28
 #define AXI_I2S_REG_TX_FIFO	0x2C
@@ -101,6 +102,17 @@ static int axi_i2s_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int axi_i2s_prepare(struct snd_pcm_substream *substream,
+			   struct snd_soc_dai *dai)
+{
+	struct axi_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+	unsigned int period_bytes = snd_pcm_lib_period_bytes(substream);
+
+	/* adi_i2s counts 32-bit words, thus divide bytes by 4 */
+	return regmap_write(i2s->regmap, AXI_I2S_REG_PERIOD_LEN,
+			    (period_bytes / 4) - 1);
+}
+
 static int axi_i2s_startup(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *dai)
 {
@@ -147,6 +159,7 @@ static const struct snd_soc_dai_ops axi_i2s_dai_ops = {
 	.shutdown = axi_i2s_shutdown,
 	.trigger = axi_i2s_trigger,
 	.hw_params = axi_i2s_hw_params,
+	.prepare = axi_i2s_prepare,
 };
 
 static struct snd_soc_dai_driver axi_i2s_dai = {
@@ -175,7 +188,7 @@ static const struct regmap_config axi_i2s_regmap_config = {
 	.reg_bits = 32,
 	.reg_stride = 4,
 	.val_bits = 32,
-	.max_register = AXI_I2S_REG_STATUS,
+	.max_register = AXI_I2S_REG_PERIOD_LEN,
 };
 
 static int axi_i2s_probe(struct platform_device *pdev)
-- 
2.17.1

Re: [PATCH 1/1] axi-i2s: set period size register

[Lars-Peter Clausen]

On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
> The default value of the PERIOD_LEN register is 0 and results in
> axi-i2s keeping TLAST always asserted in its AXI Stream output.
> 
> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
> DMA generating an interrupt flood and ALSA produce a corrupted
> recording. This is because AXI-DMA raises an interrupt whenever TLAST
> is active.
> 
> Fix by setting the PERIOD_LEN register as soon as the period is
> known. This way TLAST is emitted once per period, and the DMA raises
> interrupts correctly.

The patch looks OK. But I'd prefer not to merge it if possible.

We've done some early experiments with the Xilinx AXI-DMA, but it turned out
to be to unreliable and we've abandoned support for it. One of the more
critical issues was that you can't abort a DMA transfer. That means when
audio capture is stopped the DMA will halt, but not complete the current
transfer. Then when the next audio capture start the DMA will continue with
the previous transfer. The observed effect of this was that the system would
just crash randomly (Presumably due to memory corruption).

Have you considered using the ADI AXI-DMAC? That should work just fine.

> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  sound/soc/adi/axi-i2s.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/adi/axi-i2s.c b/sound/soc/adi/axi-i2s.c
> index 4c23381727a1..af581a313a40 100644
> --- a/sound/soc/adi/axi-i2s.c
> +++ b/sound/soc/adi/axi-i2s.c
> @@ -24,6 +24,7 @@
>  #define AXI_I2S_REG_CTRL	0x04
>  #define AXI_I2S_REG_CLK_CTRL	0x08
>  #define AXI_I2S_REG_STATUS	0x10
> +#define AXI_I2S_REG_PERIOD_LEN	0x18
>  
>  #define AXI_I2S_REG_RX_FIFO	0x28
>  #define AXI_I2S_REG_TX_FIFO	0x2C
> @@ -101,6 +102,17 @@ static int axi_i2s_hw_params(struct snd_pcm_substream *substream,
>  	return 0;
>  }
>  
> +static int axi_i2s_prepare(struct snd_pcm_substream *substream,
> +			   struct snd_soc_dai *dai)
> +{
> +	struct axi_i2s *i2s = snd_soc_dai_get_drvdata(dai);
> +	unsigned int period_bytes = snd_pcm_lib_period_bytes(substream);
> +
> +	/* adi_i2s counts 32-bit words, thus divide bytes by 4 */
> +	return regmap_write(i2s->regmap, AXI_I2S_REG_PERIOD_LEN,
> +			    (period_bytes / 4) - 1);
> +}
> +
>  static int axi_i2s_startup(struct snd_pcm_substream *substream,
>  	struct snd_soc_dai *dai)
>  {
> @@ -147,6 +159,7 @@ static const struct snd_soc_dai_ops axi_i2s_dai_ops = {
>  	.shutdown = axi_i2s_shutdown,
>  	.trigger = axi_i2s_trigger,
>  	.hw_params = axi_i2s_hw_params,
> +	.prepare = axi_i2s_prepare,
>  };
>  
>  static struct snd_soc_dai_driver axi_i2s_dai = {
> @@ -175,7 +188,7 @@ static const struct regmap_config axi_i2s_regmap_config = {
>  	.reg_bits = 32,
>  	.reg_stride = 4,
>  	.val_bits = 32,
> -	.max_register = AXI_I2S_REG_STATUS,
> +	.max_register = AXI_I2S_REG_PERIOD_LEN,
>  };
>  
>  static int axi_i2s_probe(struct platform_device *pdev)
> 

Re: [PATCH 1/1] axi-i2s: set period size register

[Luca Ceresoli]

Hi,

thanks for your feedback.

[Adding Michal Simek (Xilinx maintainer) in Cc]

On 27/08/2018 14:27, Lars-Peter Clausen wrote:
> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>> The default value of the PERIOD_LEN register is 0 and results in
>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>
>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>> DMA generating an interrupt flood and ALSA produce a corrupted
>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>> is active.
>>
>> Fix by setting the PERIOD_LEN register as soon as the period is
>> known. This way TLAST is emitted once per period, and the DMA raises
>> interrupts correctly.
> 
> The patch looks OK. But I'd prefer not to merge it if possible.
> 
> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
> to be to unreliable and we've abandoned support for it. One of the more
> critical issues was that you can't abort a DMA transfer. That means when
> audio capture is stopped the DMA will halt, but not complete the current
> transfer. Then when the next audio capture start the DMA will continue with
> the previous transfer. The observed effect of this was that the system would
> just crash randomly (Presumably due to memory corruption).

Strange. I have done many capture experiments with arecord and didn't
run into such bad issues. I only have a much less serious problem
(garbage or old samples in the first few buffers), but no crashes.

Michal, are you aware of these problems?

> Have you considered using the ADI AXI-DMAC? That should work just fine.

Not until today, because AXI-DMA is working here.

I'd like to better understand what's going on before changing an IP that
is working. Do you have additional details about your setup? How do you
run your tests?

Regards,
-- 
Luca

Re: [PATCH 1/1] axi-i2s: set period size register

[Lars-Peter Clausen]

On 08/27/2018 06:22 PM, Luca Ceresoli wrote:
> Hi,
> 
> thanks for your feedback.
> 
> [Adding Michal Simek (Xilinx maintainer) in Cc]
> 
> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>> The default value of the PERIOD_LEN register is 0 and results in
>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>
>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>> is active.
>>>
>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>> known. This way TLAST is emitted once per period, and the DMA raises
>>> interrupts correctly.
>>
>> The patch looks OK. But I'd prefer not to merge it if possible.
>>
>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>> to be to unreliable and we've abandoned support for it. One of the more
>> critical issues was that you can't abort a DMA transfer. That means when
>> audio capture is stopped the DMA will halt, but not complete the current
>> transfer. Then when the next audio capture start the DMA will continue with
>> the previous transfer. The observed effect of this was that the system would
>> just crash randomly (Presumably due to memory corruption).
> 
> Strange. I have done many capture experiments with arecord and didn't
> run into such bad issues. I only have a much less serious problem
> (garbage or old samples in the first few buffers), but no crashes.
> 
> Michal, are you aware of these problems?
> 
>> Have you considered using the ADI AXI-DMAC? That should work just fine.
> 
> Not until today, because AXI-DMA is working here.
> 
> I'd like to better understand what's going on before changing an IP that
> is working. Do you have additional details about your setup? How do you
> run your tests?

This was 4-5 years ago. A AXI-DMA with both TX and RX connected to the
AXI-I2S.

It might be that back then I didn't have buffer prealloc enabled, so a
new DMA buffer gets allocated for each transfer. Then you end up with
use after free and the DMA overwriting freed (and maybe reused) memory.

It was bad enough that it was a lot easier to add PL330 support to the
I2S peripheral. Not using Xilinx DMA for anything anymore has saved me
from a lot of headache.

Re: [PATCH 1/1] axi-i2s: set period size register

[Luca Ceresoli]

Hi,

On 27/08/2018 18:51, Lars-Peter Clausen wrote:
> On 08/27/2018 06:22 PM, Luca Ceresoli wrote:
>> Hi,
>>
>> thanks for your feedback.
>>
>> [Adding Michal Simek (Xilinx maintainer) in Cc]
>>
>> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>>> The default value of the PERIOD_LEN register is 0 and results in
>>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>>
>>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>>> is active.
>>>>
>>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>>> known. This way TLAST is emitted once per period, and the DMA raises
>>>> interrupts correctly.
>>>
>>> The patch looks OK. But I'd prefer not to merge it if possible.
>>>
>>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>>> to be to unreliable and we've abandoned support for it. One of the more
>>> critical issues was that you can't abort a DMA transfer. That means when
>>> audio capture is stopped the DMA will halt, but not complete the current
>>> transfer. Then when the next audio capture start the DMA will continue with
>>> the previous transfer. The observed effect of this was that the system would
>>> just crash randomly (Presumably due to memory corruption).
>>
>> Strange. I have done many capture experiments with arecord and didn't
>> run into such bad issues. I only have a much less serious problem
>> (garbage or old samples in the first few buffers), but no crashes.
>>
>> Michal, are you aware of these problems?
>>
>>> Have you considered using the ADI AXI-DMAC? That should work just fine.
>>
>> Not until today, because AXI-DMA is working here.
>>
>> I'd like to better understand what's going on before changing an IP that
>> is working. Do you have additional details about your setup? How do you
>> run your tests?
> 
> This was 4-5 years ago. A AXI-DMA with both TX and RX connected to the
> AXI-I2S.
> 
> It might be that back then I didn't have buffer prealloc enabled, so a
> new DMA buffer gets allocated for each transfer. Then you end up with
> use after free and the DMA overwriting freed (and maybe reused) memory.
> 
> It was bad enough that it was a lot easier to add PL330 support to the
> I2S peripheral. Not using Xilinx DMA for anything anymore has saved me
> from a lot of headache.

I kind of understand you. However since AXI-DMA it is working for me
with this patch, chances are that bug has been fixed in the meanwhile.

-- 
Luca

Re: [PATCH 1/1] axi-i2s: set period size register

[Michal Simek]

On 27.8.2018 18:22, Luca Ceresoli wrote:
> Hi,
> 
> thanks for your feedback.
> 
> [Adding Michal Simek (Xilinx maintainer) in Cc]
> 
> On 27/08/2018 14:27, Lars-Peter Clausen wrote:
>> On 08/24/2018 06:04 PM, Luca Ceresoli wrote:
>>> The default value of the PERIOD_LEN register is 0 and results in
>>> axi-i2s keeping TLAST always asserted in its AXI Stream output.
>>>
>>> When the AXI Stream is sent to a Xilinx AXI-DMA, this results in the
>>> DMA generating an interrupt flood and ALSA produce a corrupted
>>> recording. This is because AXI-DMA raises an interrupt whenever TLAST
>>> is active.
>>>
>>> Fix by setting the PERIOD_LEN register as soon as the period is
>>> known. This way TLAST is emitted once per period, and the DMA raises
>>> interrupts correctly.
>>
>> The patch looks OK. But I'd prefer not to merge it if possible.
>>
>> We've done some early experiments with the Xilinx AXI-DMA, but it turned out
>> to be to unreliable and we've abandoned support for it. One of the more
>> critical issues was that you can't abort a DMA transfer. That means when
>> audio capture is stopped the DMA will halt, but not complete the current
>> transfer. Then when the next audio capture start the DMA will continue with
>> the previous transfer. The observed effect of this was that the system would
>> just crash randomly (Presumably due to memory corruption).
> 
> Strange. I have done many capture experiments with arecord and didn't
> run into such bad issues. I only have a much less serious problem
> (garbage or old samples in the first few buffers), but no crashes.
> 
> Michal, are you aware of these problems?

I have never played with i2c and axi dma. We wanted to use this solution
for ultra96 but we didn't finish it.
Also there is new i2s IP which is going to xilinx tree and it is more
recent but also I didn't test it but at least IP team/sw guys can look
at issues with it.

Thanks,
Michal

Re: [PATCH 0/1] Fix ADI axi-i2s + Xilinx AXI-DMA capture

[Mark Brown]

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

On Fri, Aug 24, 2018 at 06:04:29PM +0200, Luca Ceresoli wrote:
> Hi,
> 
> here is a fix for a nasty audio capture problem when the axi-i2s
> output stream is fed to a Xilinx AXI-DMA.

Please don't send cover letters for single patches, if there is anything
that needs saying put it in the changelog of the patch or after the ---
if it's administrative stuff.  This reduces mail volume and ensures that 
any important information is recorded in the changelog rather than being
lost. 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/1] Fix ADI axi-i2s + Xilinx AXI-DMA capture

[Luca Ceresoli]

Hi Mark,

On 28/08/2018 20:58, Mark Brown wrote:
> On Fri, Aug 24, 2018 at 06:04:29PM +0200, Luca Ceresoli wrote:
>> Hi,
>>
>> here is a fix for a nasty audio capture problem when the axi-i2s
>> output stream is fed to a Xilinx AXI-DMA.
> 
> Please don't send cover letters for single patches, if there is anything
> that needs saying put it in the changelog of the patch or after the ---
> if it's administrative stuff.  This reduces mail volume and ensures that 
> any important information is recorded in the changelog rather than being
> lost. 

OK, will do. Actually I usually do as you suggest, but in this case I
thought it deserved a little extra visibility. Sorry about that.

-- 
Luca