[5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[Hans de Goede]

Hi All,

After rebasing a set of bytcr_rt5640 patches, on top of asoc/for-next
I noticed that playing back audio would only generate random-noise / buzzing
(I did not try recording any audio). 

After poking at this for a while I've found the culprit:

2e6b836312a4 ("ASoC: intel: atom: Fix reference to PCM buffer address")

If I revert that single commit then audio on Intel Bay Trail and
Cherry Trail devices works fine again with 5.14.

This is with a Fedora 34 userspace using pipewire as audiodaemon

I'm not sure what is going on here, but since the old code
changed by the broken commit has worked fine for ages and
given where we are in the devel-cycle I think it might be best
to just revert 2e6b836312a4 again.

Regards,

Hans

Re: [5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[Hans de Goede]

Hi,

On 8/19/21 4:42 PM, Hans de Goede wrote:
> Hi All,
> 
> After rebasing a set of bytcr_rt5640 patches, on top of asoc/for-next
> I noticed that playing back audio would only generate random-noise / buzzing
> (I did not try recording any audio). 
> 
> After poking at this for a while I've found the culprit:
> 
> 2e6b836312a4 ("ASoC: intel: atom: Fix reference to PCM buffer address")
> 
> If I revert that single commit then audio on Intel Bay Trail and
> Cherry Trail devices works fine again with 5.14.
> 
> This is with a Fedora 34 userspace using pipewire as audiodaemon
> 
> I'm not sure what is going on here, but since the old code
> changed by the broken commit has worked fine for ages and
> given where we are in the devel-cycle I think it might be best
> to just revert 2e6b836312a4 again.

p.s. to be clear I noticed this after rebasing on top of asoc/for-next
but the troublesome commit is actually in Linus tree now and thus
will hit end users once 5.14 is released.

(the troublesome commit landed in 5.14-rc6 and my previous tests
were with 5.14-rc5)

Regards,

Hans

Re: [5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[Takashi Iwai]

On Thu, 19 Aug 2021 16:45:19 +0200,
Hans de Goede wrote:
> 
> Hi,
> 
> On 8/19/21 4:42 PM, Hans de Goede wrote:
> > Hi All,
> > 
> > After rebasing a set of bytcr_rt5640 patches, on top of asoc/for-next
> > I noticed that playing back audio would only generate random-noise / buzzing
> > (I did not try recording any audio). 
> > 
> > After poking at this for a while I've found the culprit:
> > 
> > 2e6b836312a4 ("ASoC: intel: atom: Fix reference to PCM buffer address")
> > 
> > If I revert that single commit then audio on Intel Bay Trail and
> > Cherry Trail devices works fine again with 5.14.
> > 
> > This is with a Fedora 34 userspace using pipewire as audiodaemon
> > 
> > I'm not sure what is going on here, but since the old code
> > changed by the broken commit has worked fine for ages and
> > given where we are in the devel-cycle I think it might be best
> > to just revert 2e6b836312a4 again.
> 
> p.s. to be clear I noticed this after rebasing on top of asoc/for-next
> but the troublesome commit is actually in Linus tree now and thus
> will hit end users once 5.14 is released.
> 
> (the troublesome commit landed in 5.14-rc6 and my previous tests
> were with 5.14-rc5)

Ah, the commit might be problematic on 5.14 where dma_addr isn't set
yet for the CONTINUOUS buffer type (which was fixed in sound.git
for-next branch).

Could you try the patch below?


thanks,

Takashi

---
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -127,7 +127,7 @@ static void sst_fill_alloc_params(struct snd_pcm_substream *substream,
 	snd_pcm_uframes_t period_size;
 	ssize_t periodbytes;
 	ssize_t buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
-	u32 buffer_addr = substream->runtime->dma_addr;
+	u32 buffer_addr = virt_to_phys(substream->runtime->dma_area);
 
 	channels = substream->runtime->channels;
 	period_size = substream->runtime->period_size;

Re: [5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[Hans de Goede]

Hi,

On 8/19/21 4:52 PM, Takashi Iwai wrote:
> On Thu, 19 Aug 2021 16:45:19 +0200,
> Hans de Goede wrote:
>>
>> Hi,
>>
>> On 8/19/21 4:42 PM, Hans de Goede wrote:
>>> Hi All,
>>>
>>> After rebasing a set of bytcr_rt5640 patches, on top of asoc/for-next
>>> I noticed that playing back audio would only generate random-noise / buzzing
>>> (I did not try recording any audio). 
>>>
>>> After poking at this for a while I've found the culprit:
>>>
>>> 2e6b836312a4 ("ASoC: intel: atom: Fix reference to PCM buffer address")
>>>
>>> If I revert that single commit then audio on Intel Bay Trail and
>>> Cherry Trail devices works fine again with 5.14.
>>>
>>> This is with a Fedora 34 userspace using pipewire as audiodaemon
>>>
>>> I'm not sure what is going on here, but since the old code
>>> changed by the broken commit has worked fine for ages and
>>> given where we are in the devel-cycle I think it might be best
>>> to just revert 2e6b836312a4 again.
>>
>> p.s. to be clear I noticed this after rebasing on top of asoc/for-next
>> but the troublesome commit is actually in Linus tree now and thus
>> will hit end users once 5.14 is released.
>>
>> (the troublesome commit landed in 5.14-rc6 and my previous tests
>> were with 5.14-rc5)
> 
> Ah, the commit might be problematic on 5.14 where dma_addr isn't set
> yet for the CONTINUOUS buffer type (which was fixed in sound.git
> for-next branch).
> 
> Could you try the patch below?

I can confirm that this fixes things.

Note though that I'm running 5.14-rc6 with asoc/for-next merged in,
so this suggest that the "dma_addr isn't set yet for the CONTINUOUS
buffer type" problem also exists in asoc/for-next which means that
asoc/for-next as a standalone tree also has broken Intel SST audio
atm (I did not verify this).

Either way I guess that even with the dma_addr being set doing
virt_to_phys(substream->runtime->dma_area) will still be fine,
so this probably is the right thing to do for now regardless.

Regards,

Hans








> 
> 
> thanks,
> 
> Takashi
> 
> ---
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -127,7 +127,7 @@ static void sst_fill_alloc_params(struct snd_pcm_substream *substream,
>  	snd_pcm_uframes_t period_size;
>  	ssize_t periodbytes;
>  	ssize_t buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
> -	u32 buffer_addr = substream->runtime->dma_addr;
> +	u32 buffer_addr = virt_to_phys(substream->runtime->dma_area);
>  
>  	channels = substream->runtime->channels;
>  	period_size = substream->runtime->period_size;
> 

Re: [5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio

[Takashi Iwai]

On Thu, 19 Aug 2021 17:11:25 +0200,
Hans de Goede wrote:
> 
> Hi,
> 
> On 8/19/21 4:52 PM, Takashi Iwai wrote:
> > On Thu, 19 Aug 2021 16:45:19 +0200,
> > Hans de Goede wrote:
> >>
> >> Hi,
> >>
> >> On 8/19/21 4:42 PM, Hans de Goede wrote:
> >>> Hi All,
> >>>
> >>> After rebasing a set of bytcr_rt5640 patches, on top of asoc/for-next
> >>> I noticed that playing back audio would only generate random-noise / buzzing
> >>> (I did not try recording any audio). 
> >>>
> >>> After poking at this for a while I've found the culprit:
> >>>
> >>> 2e6b836312a4 ("ASoC: intel: atom: Fix reference to PCM buffer address")
> >>>
> >>> If I revert that single commit then audio on Intel Bay Trail and
> >>> Cherry Trail devices works fine again with 5.14.
> >>>
> >>> This is with a Fedora 34 userspace using pipewire as audiodaemon
> >>>
> >>> I'm not sure what is going on here, but since the old code
> >>> changed by the broken commit has worked fine for ages and
> >>> given where we are in the devel-cycle I think it might be best
> >>> to just revert 2e6b836312a4 again.
> >>
> >> p.s. to be clear I noticed this after rebasing on top of asoc/for-next
> >> but the troublesome commit is actually in Linus tree now and thus
> >> will hit end users once 5.14 is released.
> >>
> >> (the troublesome commit landed in 5.14-rc6 and my previous tests
> >> were with 5.14-rc5)
> > 
> > Ah, the commit might be problematic on 5.14 where dma_addr isn't set
> > yet for the CONTINUOUS buffer type (which was fixed in sound.git
> > for-next branch).
> > 
> > Could you try the patch below?
> 
> I can confirm that this fixes things.
> 
> Note though that I'm running 5.14-rc6 with asoc/for-next merged in,
> so this suggest that the "dma_addr isn't set yet for the CONTINUOUS
> buffer type" problem also exists in asoc/for-next which means that
> asoc/for-next as a standalone tree also has broken Intel SST audio
> atm (I did not verify this).

Yes, asoc/for-next is behind my for-next branch, so it doesn't contain
the necessary commit yet.

> Either way I guess that even with the dma_addr being set doing
> virt_to_phys(substream->runtime->dma_area) will still be fine,
> so this probably is the right thing to do for now regardless.

I'll submit the proper patch with this fix.  After the merge for 5.14,
it'll be reverted again for 5.15.


thanks,

Takashi