| Message ID | 874jy98snd.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Buffers used in ASoC Intel Atom SST driver | expand |
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
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; }
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; > }
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 > >
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; > }
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; }