Buffers used in ASoC Intel Atom SST driver

Buffers used in ASoC Intel Atom SST driver

[Takashi Iwai]

Hi,

while cleaning up / fixing the memory allocation code, I noticed that
Intel SST driver is the only driver that currently uses the
SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.

Is there any reason not to use the standard device buffer
(SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.


thanks,

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ASoC: Intel: sst: Switch to standard device pages

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index a56dd48c045f..c75616a5fd0a 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
 
 	if (dai->driver->playback.channels_min ||
 			dai->driver->capture.channels_min) {
-		snd_pcm_set_managed_buffer_all(pcm,
-			SNDRV_DMA_TYPE_CONTINUOUS,
-			snd_dma_continuous_data(GFP_DMA),
-			SST_MIN_BUFFER, SST_MAX_BUFFER);
+		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
+					       pcm->card->dev,
+					       SST_MIN_BUFFER, SST_MAX_BUFFER);
 	}
 	return 0;
 }
-- 
2.35.3

Re: Buffers used in ASoC Intel Atom SST driver

[Cezary Rojewski]

On 2022-08-18 5:42 PM, Takashi Iwai wrote:
> Hi,
> 
> while cleaning up / fixing the memory allocation code, I noticed that
> Intel SST driver is the only driver that currently uses the
> SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.
> 
> Is there any reason not to use the standard device buffer
> (SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.


Hello,

Does not hurt to check the change out. I'm not aware of such limitation 
for any of Intel's AudioDSP solutions. I cannot test this change though 
as ATOM devices are not part of my CI. Pierre, do you have any devices 
where this change could be tested?

Adding Hans as he is the key intel/atom maintainer too and I'm pretty 
sure he has few devices on his own.

Regards,
Czarek

Re: Buffers used in ASoC Intel Atom SST driver

[Takashi Iwai]

On Thu, 18 Aug 2022 17:59:36 +0200,
Cezary Rojewski wrote:
> 
> On 2022-08-18 5:42 PM, Takashi Iwai wrote:
> > Hi,
> > 
> > while cleaning up / fixing the memory allocation code, I noticed that
> > Intel SST driver is the only driver that currently uses the
> > SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.
> > 
> > Is there any reason not to use the standard device buffer
> > (SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.
> 
> 
> Hello,
> 
> Does not hurt to check the change out. I'm not aware of such
> limitation for any of Intel's AudioDSP solutions. I cannot test this
> change though as ATOM devices are not part of my CI. Pierre, do you
> have any devices where this change could be tested?
> 
> Adding Hans as he is the key intel/atom maintainer too and I'm pretty
> sure he has few devices on his own.

Oh yeah, thanks, Hans might be able to check.
FWIW, the patch I asked is like below.


Takashi

-- 8< --

From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ASoC: Intel: sst: Switch to standard device pages

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index a56dd48c045f..c75616a5fd0a 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
 
 	if (dai->driver->playback.channels_min ||
 			dai->driver->capture.channels_min) {
-		snd_pcm_set_managed_buffer_all(pcm,
-			SNDRV_DMA_TYPE_CONTINUOUS,
-			snd_dma_continuous_data(GFP_DMA),
-			SST_MIN_BUFFER, SST_MAX_BUFFER);
+		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
+					       pcm->card->dev,
+					       SST_MIN_BUFFER, SST_MAX_BUFFER);
 	}
 	return 0;
 }
-- 
2.35.3

Re: Buffers used in ASoC Intel Atom SST driver

[Pierre-Louis Bossart]

On 8/18/22 18:03, Takashi Iwai wrote:
> On Thu, 18 Aug 2022 17:59:36 +0200,
> Cezary Rojewski wrote:
>>
>> On 2022-08-18 5:42 PM, Takashi Iwai wrote:
>>> Hi,
>>>
>>> while cleaning up / fixing the memory allocation code, I noticed that
>>> Intel SST driver is the only driver that currently uses the
>>> SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.
>>>
>>> Is there any reason not to use the standard device buffer
>>> (SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.
>>
>>
>> Hello,
>>
>> Does not hurt to check the change out. I'm not aware of such
>> limitation for any of Intel's AudioDSP solutions. I cannot test this
>> change though as ATOM devices are not part of my CI. Pierre, do you
>> have any devices where this change could be tested?
>>
>> Adding Hans as he is the key intel/atom maintainer too and I'm pretty
>> sure he has few devices on his own.
> 
> Oh yeah, thanks, Hans might be able to check.
> FWIW, the patch I asked is like below.


It's a 2011 contribution, commit d927fdae5cb2ca, probably from the
Medfield days. I doubt anyone in the Intel CC: list has context on this,
I was the only one on the Intel payroll at the time.

I vaguely remember than in early versions the PCM data was copied over
to the DSP SRAM with the .ack callback, it wasn't a straightforward DMA
transfer, and that created all kinds of issues for PulseAudio integration.

This was changed at some point IIRC, I don't see any reason why a
bog-standard DMA buffer wouldn't work with the released firmware for
Baytrail which is two generations newer and does use the DesignWare DMA.

The best would be to try, I can't volunteer since I don't have any
hardware available at the  moment.


> 
> 
> Takashi
> 
> -- 8< --
> 
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ASoC: Intel: sst: Switch to standard device pages
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index a56dd48c045f..c75616a5fd0a 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
>  
>  	if (dai->driver->playback.channels_min ||
>  			dai->driver->capture.channels_min) {
> -		snd_pcm_set_managed_buffer_all(pcm,
> -			SNDRV_DMA_TYPE_CONTINUOUS,
> -			snd_dma_continuous_data(GFP_DMA),
> -			SST_MIN_BUFFER, SST_MAX_BUFFER);
> +		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
> +					       pcm->card->dev,
> +					       SST_MIN_BUFFER, SST_MAX_BUFFER);
>  	}
>  	return 0;
>  }

Re: Buffers used in ASoC Intel Atom SST driver

[Takashi Iwai]

On Thu, 18 Aug 2022 18:03:15 +0200,
Takashi Iwai wrote:
> 
> On Thu, 18 Aug 2022 17:59:36 +0200,
> Cezary Rojewski wrote:
> > 
> > On 2022-08-18 5:42 PM, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > while cleaning up / fixing the memory allocation code, I noticed that
> > > Intel SST driver is the only driver that currently uses the
> > > SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.
> > > 
> > > Is there any reason not to use the standard device buffer
> > > (SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.
> > 
> > 
> > Hello,
> > 
> > Does not hurt to check the change out. I'm not aware of such
> > limitation for any of Intel's AudioDSP solutions. I cannot test this
> > change though as ATOM devices are not part of my CI. Pierre, do you
> > have any devices where this change could be tested?
> > 
> > Adding Hans as he is the key intel/atom maintainer too and I'm pretty
> > sure he has few devices on his own.
> 
> Oh yeah, thanks, Hans might be able to check.
> FWIW, the patch I asked is like below.

I guess we can merge and go forward.  The changes are very likely
safe, judging from the code, as there is no evidence of DMA address
restriction there.  So I'm going to submit the full patchset for
cleanups.


thanks,

Takashi

> 
> 
> Takashi
> 
> -- 8< --
> 
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ASoC: Intel: sst: Switch to standard device pages
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index a56dd48c045f..c75616a5fd0a 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
>  
>  	if (dai->driver->playback.channels_min ||
>  			dai->driver->capture.channels_min) {
> -		snd_pcm_set_managed_buffer_all(pcm,
> -			SNDRV_DMA_TYPE_CONTINUOUS,
> -			snd_dma_continuous_data(GFP_DMA),
> -			SST_MIN_BUFFER, SST_MAX_BUFFER);
> +		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
> +					       pcm->card->dev,
> +					       SST_MIN_BUFFER, SST_MAX_BUFFER);
>  	}
>  	return 0;
>  }
> -- 
> 2.35.3
> 
> 

Re: Buffers used in ASoC Intel Atom SST driver

[Hans de Goede]

Hi,

On 8/18/22 18:03, Takashi Iwai wrote:
> On Thu, 18 Aug 2022 17:59:36 +0200,
> Cezary Rojewski wrote:
>>
>> On 2022-08-18 5:42 PM, Takashi Iwai wrote:
>>> Hi,
>>>
>>> while cleaning up / fixing the memory allocation code, I noticed that
>>> Intel SST driver is the only driver that currently uses the
>>> SNDRV_DMA_TYPE_CONTINUOUS pages for its PCM buffer.
>>>
>>> Is there any reason not to use the standard device buffer
>>> (SNDRV_DMA_TYPE_DEV)?  i.e. the change like below.
>>
>>
>> Hello,
>>
>> Does not hurt to check the change out. I'm not aware of such
>> limitation for any of Intel's AudioDSP solutions. I cannot test this
>> change though as ATOM devices are not part of my CI. Pierre, do you
>> have any devices where this change could be tested?
>>
>> Adding Hans as he is the key intel/atom maintainer too and I'm pretty
>> sure he has few devices on his own.
> 
> Oh yeah, thanks, Hans might be able to check.
> FWIW, the patch I asked is like below.

I know this has already been merged, but still I have just given this
a quick test-spin and it works ok for me.

Regards,

Hans


> 
> 
> Takashi
> 
> -- 8< --
> 
> From: Takashi Iwai <tiwai@suse.de>
> Subject: [PATCH] ASoC: Intel: sst: Switch to standard device pages
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index a56dd48c045f..c75616a5fd0a 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
>  
>  	if (dai->driver->playback.channels_min ||
>  			dai->driver->capture.channels_min) {
> -		snd_pcm_set_managed_buffer_all(pcm,
> -			SNDRV_DMA_TYPE_CONTINUOUS,
> -			snd_dma_continuous_data(GFP_DMA),
> -			SST_MIN_BUFFER, SST_MAX_BUFFER);
> +		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
> +					       pcm->card->dev,
> +					       SST_MIN_BUFFER, SST_MAX_BUFFER);
>  	}
>  	return 0;
>  }