| Message ID | 1487003909-11710-5-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 13, 2017 at 05:38:26PM +0100, Arnaud Pouliquen wrote: > From: olivier moysan <olivier.moysan@st.com> > > Add copy support in pcm damengine operations. > As example, this allows to: > - process data before rendering (IEC status insertion), > - process captured sample ( sample mask and shift). dmaengine with copy... so not DMA at all? I'm a bit confused about why we're extending the dmaengine code to have a copy operation rather than just writing driver code.
Hello Mark, On 02/14/2017 06:16 PM, Mark Brown wrote: > On Mon, Feb 13, 2017 at 05:38:26PM +0100, Arnaud Pouliquen wrote: >> From: olivier moysan <olivier.moysan@st.com> >> >> Add copy support in pcm damengine operations. As example, this >> allows to: - process data before rendering (IEC status >> insertion), - process captured sample ( sample mask and shift). > > dmaengine with copy... so not DMA at all? I'm a bit confused > about why we're extending the dmaengine code to have a copy > operation rather than just writing driver code. > For DFSDM we need to re-arrange samples after DMA transfer, before sending them to application. For this, a copy ops(called in snd_pcm_lib_read_transfer) alreday exists in snd_pcm_ops struct. This ops is not available at soc dma engine level, so at ASoC driver level. Aim of this patch is to expose the copy in driver. DMA is still used, copy ops just allows to handle the copy_to_user in driver. Advantage of this add-on is that we still able to take benefice of the generic DMA in DFSDM driver. Now if you estimate that it is too stm32 SOC specific, the other solution is to handle directly the PCM platform device in DFSDM driver. Regards Arnaud
On Wed, Feb 15, 2017 at 02:59:18PM +0100, Arnaud Pouliquen wrote: > For DFSDM we need to re-arrange samples after DMA transfer, before > sending them to application. > For this, a copy ops(called in snd_pcm_lib_read_transfer) alreday exists > in snd_pcm_ops struct. No, copy() is an alternative to doing DMA rather than something that works with DMA. If you want to have some sort of post processing operation it shoudn't be called copy(). It's probably worth considering just making a new format and letting userspace convert things here.
On 02/15/2017 03:53 PM, Mark Brown wrote: > On Wed, Feb 15, 2017 at 02:59:18PM +0100, Arnaud Pouliquen wrote: > >> For DFSDM we need to re-arrange samples after DMA transfer, >> before sending them to application. For this, a copy ops(called >> in snd_pcm_lib_read_transfer) alreday exists in snd_pcm_ops >> struct. > > No, copy() is an alternative to doing DMA rather than something > that works with DMA. If you want to have some sort of post > processing operation it shoudn't be called copy(). It's probably > worth considering just making a new format and letting userspace > convert things here. > You mean using a plug-in alsa to do it? this seems not possible with tiny-alsa... Without a plug-in, driver will not be compatible with standard audio application. I'm newbies in alsa plugin But it seems quite tricky to do it. This solution means that userspace has to be aware on hardware constraint. Processing to do is a mask on 8 LSB + a left shift on bits. T shift value depends on several parameters: the SPI frequency, the sampling rate and the filter parameters. That means an alsa control to get scaling, handled by the plugin. So i would prefer to find a way to do it in driver... I saw that blackfin driver implements this kind of mechanism (bf5xx_pcm_copy) to support the TDM mode. if i well understood the code they process the TDM buffer filled by DMA. Samples are extracted from the good TDM slots and copied in buffer that will be sent to userland. Do you think this would be reasonable if i implement something similar in my driver, without using the soc generic DMA engine? Regards Arnaud
On Wed, Feb 15, 2017 at 04:46:44PM +0100, Arnaud Pouliquen wrote: > On 02/15/2017 03:53 PM, Mark Brown wrote: > > On Wed, Feb 15, 2017 at 02:59:18PM +0100, Arnaud Pouliquen wrote: > > No, copy() is an alternative to doing DMA rather than something > > that works with DMA. If you want to have some sort of post > > processing operation it shoudn't be called copy(). It's probably > > worth considering just making a new format and letting userspace > > convert things here. > You mean using a plug-in alsa to do it? this seems not possible with > tiny-alsa... > Without a plug-in, driver will not be compatible with standard audio > application. Right. But if you're using tinyalsa you've got some open coded system specific stuff anyway! > Do you think this would be reasonable if i implement something similar > in my driver, without using the soc generic DMA engine? Probably. It might even be possible to fit it elegently into the generic dmaengine code, just don't say it's a copy() operation since that's a specific thing in ALSA.
Hello Mark, Sorry for the late answer, i was OoO... On 02/16/2017 09:14 PM, Mark Brown wrote: > On Wed, Feb 15, 2017 at 04:46:44PM +0100, Arnaud Pouliquen wrote: >> On 02/15/2017 03:53 PM, Mark Brown wrote: >>> On Wed, Feb 15, 2017 at 02:59:18PM +0100, Arnaud Pouliquen >>> wrote: > >>> No, copy() is an alternative to doing DMA rather than >>> something that works with DMA. If you want to have some sort >>> of post processing operation it shoudn't be called copy(). >>> It's probably worth considering just making a new format and >>> letting userspace convert things here. > >> You mean using a plug-in alsa to do it? this seems not possible >> with tiny-alsa... Without a plug-in, driver will not be >> compatible with standard audio application. > > Right. But if you're using tinyalsa you've got some open coded > system specific stuff anyway! > >> Do you think this would be reasonable if i implement something >> similar in my driver, without using the soc generic DMA engine? > > Probably. It might even be possible to fit it elegently into the > generic dmaengine code, just don't say it's a copy() operation > since that's a specific thing in ALSA. > Ok I will have a look in generic dmaengine and try to propose something in this way in the V2. Regards, Arnaud
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index 67be244..9d7bce8 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -137,6 +137,9 @@ struct snd_dmaengine_pcm_config { int (*prepare_slave_config)(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct dma_slave_config *slave_config); + int (*copy)(struct snd_pcm_substream *substream, int channel, + snd_pcm_uframes_t pos, + void __user *buf, snd_pcm_uframes_t count); struct dma_chan *(*compat_request_channel)( struct snd_soc_pcm_runtime *rtd, struct snd_pcm_substream *substream); diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c index 6cef397..bd8332ce 100644 --- a/sound/soc/soc-generic-dmaengine-pcm.c +++ b/sound/soc/soc-generic-dmaengine-pcm.c @@ -329,6 +329,16 @@ static snd_pcm_uframes_t dmaengine_pcm_pointer( return snd_dmaengine_pcm_pointer(substream); } +int dmaengine_pcm_copy(struct snd_pcm_substream *substream, int channel, + snd_pcm_uframes_t pos, void __user *buf, + snd_pcm_uframes_t count) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct dmaengine_pcm *pcm = soc_platform_to_pcm(rtd->platform); + + return pcm->config->copy(substream, channel, pos, buf, count); +} + static const struct snd_pcm_ops dmaengine_pcm_ops = { .open = dmaengine_pcm_open, .close = snd_dmaengine_pcm_close, @@ -339,6 +349,17 @@ static snd_pcm_uframes_t dmaengine_pcm_pointer( .pointer = dmaengine_pcm_pointer, }; +static const struct snd_pcm_ops dmaengine_pcm_ops_with_cpy = { + .open = dmaengine_pcm_open, + .close = snd_dmaengine_pcm_close, + .ioctl = snd_pcm_lib_ioctl, + .hw_params = dmaengine_pcm_hw_params, + .hw_free = snd_pcm_lib_free_pages, + .trigger = snd_dmaengine_pcm_trigger, + .pointer = dmaengine_pcm_pointer, + .copy = dmaengine_pcm_copy, +}; + static const struct snd_soc_platform_driver dmaengine_pcm_platform = { .component_driver = { .probe_order = SND_SOC_COMP_ORDER_LATE, @@ -347,6 +368,14 @@ static snd_pcm_uframes_t dmaengine_pcm_pointer( .pcm_new = dmaengine_pcm_new, }; +static const struct snd_soc_platform_driver dmaengine_pcm_platform_with_cpy = { + .component_driver = { + .probe_order = SND_SOC_COMP_ORDER_LATE, + }, + .ops = &dmaengine_pcm_ops_with_cpy, + .pcm_new = dmaengine_pcm_new, +}; + static const char * const dmaengine_pcm_dma_channel_names[] = { [SNDRV_PCM_STREAM_PLAYBACK] = "tx", [SNDRV_PCM_STREAM_CAPTURE] = "rx", @@ -439,8 +468,12 @@ int snd_dmaengine_pcm_register(struct device *dev, if (ret) goto err_free_dma; - ret = snd_soc_add_platform(dev, &pcm->platform, - &dmaengine_pcm_platform); + if (config && config->copy) + ret = snd_soc_add_platform(dev, &pcm->platform, + &dmaengine_pcm_platform_with_cpy); + else + ret = snd_soc_add_platform(dev, &pcm->platform, + &dmaengine_pcm_platform); if (ret) goto err_free_dma;