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

From: Hans de Goede <hdegoede@redhat.com>
To: Takashi Iwai <tiwai@suse.de>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	Mark Brown <broonie@kernel.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: Re: [5.14 regression] "ASoC: intel: atom: Fix reference to PCM buffer address" breaks Intel SST audio
Date: Thu, 19 Aug 2021 17:11:25 +0200	[thread overview]
Message-ID: <aa8f9df0-96a4-4050-454c-dda7104bce3c@redhat.com> (raw)
In-Reply-To: <s5ha6lda3lr.wl-tiwai@suse.de>

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;
> 