[PATCH] ASoC: amd: Changing Audio Format does not reflect.

[PATCH] ASoC: amd: Changing Audio Format does not reflect.

[Ravulapati Vishnu vardhan rao]

When you run aplay subsequently as below by changing the stream format:

aplay -Dhw:2,0 -c2 -fS16_LE -r48000 /dev/zero -vv -d 5;aplay -Dhw:2,0
-c2 -fS24_LE -r48000 /dev/zero -vv
as a single command, the format gets corrupted and audio does not play.

So clear the ACP_(I2S/BT)TDM_ITER/IRER register when dma stops.

Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
---
 sound/soc/amd/raven/acp3x-i2s.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/amd/raven/acp3x-i2s.c b/sound/soc/amd/raven/acp3x-i2s.c
index 3a3c47e..b07c50a 100644
--- a/sound/soc/amd/raven/acp3x-i2s.c
+++ b/sound/soc/amd/raven/acp3x-i2s.c
@@ -240,9 +240,7 @@ static int acp3x_i2s_trigger(struct snd_pcm_substream *substream,
 				reg_val = mmACP_I2STDM_IRER;
 			}
 		}
-		val = rv_readl(rtd->acp3x_base + reg_val);
-		val = val & ~BIT(0);
-		rv_writel(val, rtd->acp3x_base + reg_val);
+		rv_writel(0, rtd->acp3x_base + reg_val);
 
 		if (!(rv_readl(rtd->acp3x_base + mmACP_BTTDM_ITER) & BIT(0)) &&
 		     !(rv_readl(rtd->acp3x_base + mmACP_BTTDM_IRER) & BIT(0)))
-- 
2.7.4

Re: [PATCH] ASoC: amd: Changing Audio Format does not reflect.

[Agrawal, Akshu]

On 3/19/2020 4:52 PM, Ravulapati Vishnu vardhan rao wrote:
> When you run aplay subsequently as below by changing the stream format:
>
> aplay -Dhw:2,0 -c2 -fS16_LE -r48000 /dev/zero -vv -d 5;aplay -Dhw:2,0
> -c2 -fS24_LE -r48000 /dev/zero -vv
> as a single command, the format gets corrupted and audio does not play.
>
> So clear the ACP_(I2S/BT)TDM_ITER/IRER register when dma stops.
>
> Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com>
> ---
>   sound/soc/amd/raven/acp3x-i2s.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/sound/soc/amd/raven/acp3x-i2s.c b/sound/soc/amd/raven/acp3x-i2s.c
> index 3a3c47e..b07c50a 100644
> --- a/sound/soc/amd/raven/acp3x-i2s.c
> +++ b/sound/soc/amd/raven/acp3x-i2s.c
> @@ -240,9 +240,7 @@ static int acp3x_i2s_trigger(struct snd_pcm_substream *substream,
>   				reg_val = mmACP_I2STDM_IRER;
>   			}
>   		}
> -		val = rv_readl(rtd->acp3x_base + reg_val);
> -		val = val & ~BIT(0);
> -		rv_writel(val, rtd->acp3x_base + reg_val);
> +		rv_writel(0, rtd->acp3x_base + reg_val);

Clearing the entire register might result in issues.

IMO better fix is to have a sample_len mask for ITER and IRER register.

Use it to clear sample length bits in acp3x_i2s_hwparams function before 
setting the new format resolution.


Thanks,

Akshu

RE: [PATCH] ASoC: amd: Changing Audio Format does not reflect.

[RAVULAPATI, VISHNU VARDHAN RAO]

[AMD Official Use Only - Internal Distribution Only]



-----Original Message-----
From: Agrawal, Akshu <Akshu.Agrawal@amd.com> 
Sent: Friday, March 20, 2020 9:39 AM
To: RAVULAPATI, VISHNU VARDHAN RAO <Vishnuvardhanrao.Ravulapati@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; broonie@kernel.org; Liam Girdwood <lgirdwood@gmail.com>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Agrawal, Akshu <Akshu.Agrawal@amd.com>; Wei Yongjun <weiyongjun1@huawei.com>; moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ASoC: amd: Changing Audio Format does not reflect.


On 3/19/2020 4:52 PM, Ravulapati Vishnu vardhan rao wrote:
> When you run aplay subsequently as below by changing the stream format:
>
> aplay -Dhw:2,0 -c2 -fS16_LE -r48000 /dev/zero -vv -d 5;aplay -Dhw:2,0
> -c2 -fS24_LE -r48000 /dev/zero -vv
> as a single command, the format gets corrupted and audio does not play.
>
> So clear the ACP_(I2S/BT)TDM_ITER/IRER register when dma stops.
>
> Signed-off-by: Ravulapati Vishnu vardhan rao 
> <Vishnuvardhanrao.Ravulapati@amd.com>
> ---
>   sound/soc/amd/raven/acp3x-i2s.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/sound/soc/amd/raven/acp3x-i2s.c 
> b/sound/soc/amd/raven/acp3x-i2s.c index 3a3c47e..b07c50a 100644
> --- a/sound/soc/amd/raven/acp3x-i2s.c
> +++ b/sound/soc/amd/raven/acp3x-i2s.c
> @@ -240,9 +240,7 @@ static int acp3x_i2s_trigger(struct snd_pcm_substream *substream,
>   				reg_val = mmACP_I2STDM_IRER;
>   			}
>   		}
> -		val = rv_readl(rtd->acp3x_base + reg_val);
> -		val = val & ~BIT(0);
> -		rv_writel(val, rtd->acp3x_base + reg_val);
> +		rv_writel(0, rtd->acp3x_base + reg_val);

Clearing the entire register might result in issues.

IMO better fix is to have a sample_len mask for ITER and IRER register.

Use it to clear sample length bits in acp3x_i2s_hwparams function before setting the new format resolution.


Thanks,

Akshu


When stream is open once again all the values will be set.
So with clearing ITER/IRER to 0 when device closes there will be no issues.

-Vishnu

Re: [PATCH] ASoC: amd: Changing Audio Format does not reflect.

[Agrawal, Akshu]

On 3/24/2020 3:46 PM, RAVULAPATI, VISHNU VARDHAN RAO wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
>
>
> -----Original Message-----
> From: Agrawal, Akshu <Akshu.Agrawal@amd.com>
> Sent: Friday, March 20, 2020 9:39 AM
> To: RAVULAPATI, VISHNU VARDHAN RAO <Vishnuvardhanrao.Ravulapati@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; broonie@kernel.org; Liam Girdwood <lgirdwood@gmail.com>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Agrawal, Akshu <Akshu.Agrawal@amd.com>; Wei Yongjun <weiyongjun1@huawei.com>; moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; open list <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH] ASoC: amd: Changing Audio Format does not reflect.
>
>
> On 3/19/2020 4:52 PM, Ravulapati Vishnu vardhan rao wrote:
>> When you run aplay subsequently as below by changing the stream format:
>>
>> aplay -Dhw:2,0 -c2 -fS16_LE -r48000 /dev/zero -vv -d 5;aplay -Dhw:2,0
>> -c2 -fS24_LE -r48000 /dev/zero -vv
>> as a single command, the format gets corrupted and audio does not play.
>>
>> So clear the ACP_(I2S/BT)TDM_ITER/IRER register when dma stops.
>>
>> Signed-off-by: Ravulapati Vishnu vardhan rao
>> <Vishnuvardhanrao.Ravulapati@amd.com>
>> ---
>>    sound/soc/amd/raven/acp3x-i2s.c | 4 +---
>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/amd/raven/acp3x-i2s.c
>> b/sound/soc/amd/raven/acp3x-i2s.c index 3a3c47e..b07c50a 100644
>> --- a/sound/soc/amd/raven/acp3x-i2s.c
>> +++ b/sound/soc/amd/raven/acp3x-i2s.c
>> @@ -240,9 +240,7 @@ static int acp3x_i2s_trigger(struct snd_pcm_substream *substream,
>>    				reg_val = mmACP_I2STDM_IRER;
>>    			}
>>    		}
>> -		val = rv_readl(rtd->acp3x_base + reg_val);
>> -		val = val & ~BIT(0);
>> -		rv_writel(val, rtd->acp3x_base + reg_val);
>> +		rv_writel(0, rtd->acp3x_base + reg_val);
> Clearing the entire register might result in issues.
>
> IMO better fix is to have a sample_len mask for ITER and IRER register.
>
> Use it to clear sample length bits in acp3x_i2s_hwparams function before setting the new format resolution.
>
>
> Thanks,
>
> Akshu
>
>
> When stream is open once again all the values will be set.
> So with clearing ITER/IRER to 0 when device closes there will be no issues.
>
> -Vishnu

The issue is in below code:

         val = rv_readl(rtd->acp3x_base + reg_val);
         val = val | (rtd->xfer_resolution  << 3);
         rv_writel(val, rtd->acp3x_base + reg_val);

where you are setting bits without first clearing them. If initial value 
was 10 and you want to set 01, you will end up with 11.

The correct way is to use mask and clear the bits before using | to 
avoid previous residual values.