| Message ID | 1589881301-4143-1-git-send-email-shengjiu.wang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe | expand |
Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > There are two requirements that we need to move the request > of dma channel from probe to open. How do you handle -EPROBE_DEFER return code from the channel request if you don't do it in probe? > - When dma device binds with power-domains, the power will > be enabled when we request dma channel. If the request of dma > channel happen on probe, then the power-domains will be always > enabled after kernel boot up, which is not good for power > saving, so we need to move the request of dma channel to .open(); This is certainly something which could be fixed in the dmaengine driver. > - With FE-BE case, if the dma channel is requested in probe, > then there will be below issue, which is caused by that the > dma channel will be requested duplicately Why is this requested a second time? Is this just some missing cleanup on a deferred probe path? Regards, Lucas > [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) > [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) > [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) > [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) > [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) > [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) > [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) > [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) > [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) > [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > --- > sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- > 1 file changed, 159 insertions(+), 14 deletions(-) > > diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c > index 04a9bc749016..dae53b384df4 100644 > --- a/sound/soc/fsl/imx-pcm-dma.c > +++ b/sound/soc/fsl/imx-pcm-dma.c > @@ -11,6 +11,7 @@ > #include <linux/dmaengine.h> > #include <linux/types.h> > #include <linux/module.h> > +#include <linux/dma-mapping.h> > > #include <sound/core.h> > #include <sound/pcm.h> > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) > return true; > } > > -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > - .compat_filter_fn = filter, > -}; > +static int imx_pcm_hw_params(struct snd_soc_component *component, > + struct snd_pcm_substream *substream, > + struct snd_pcm_hw_params *params) > +{ > + struct snd_pcm_runtime *runtime = substream->runtime; > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > + struct snd_dmaengine_dai_dma_data *dma_data; > + struct dma_slave_config config; > + struct dma_chan *chan; > + int ret = 0; > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > + runtime->dma_bytes = params_buffer_bytes(params); > + > + chan = snd_dmaengine_pcm_get_chan(substream); > + if (!chan) > + return -EINVAL; > + > + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); > + if (ret) > + return ret; > + > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > + if (!dma_data) > + return -EINVAL; > + > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > + dma_data, > + &config); > + return dmaengine_slave_config(chan, &config); > +} > + > +static int imx_pcm_hw_free(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > { > - struct snd_dmaengine_pcm_config *config; > + snd_pcm_set_runtime_buffer(substream, NULL); > + return 0; > +} > + > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + return snd_dmaengine_pcm_pointer(substream); > +} > + > +static int imx_pcm_trigger(struct snd_soc_component *component, > + struct snd_pcm_substream *substream, int cmd) > +{ > + return snd_dmaengine_pcm_trigger(substream, cmd); > +} > + > +static int imx_pcm_open(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > + struct snd_dmaengine_dai_dma_data *dma_data; > + struct device *dev = component->dev; > + struct snd_pcm_hardware hw; > + struct dma_chan *chan; > + int ret; > + > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > + SNDRV_PCM_HW_PARAM_PERIODS); > + if (ret < 0) { > + dev_err(dev, "failed to set pcm hw params periods\n"); > + return ret; > + } > + > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > + if (!dma_data) > + return -EINVAL; > + > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > + if (!chan) { > + /* Try to request channel using compat_filter_fn */ > + chan = snd_dmaengine_pcm_request_channel(filter, > + dma_data->filter_data); > + if (!chan) > + return -ENXIO; > + } > > - config = devm_kzalloc(&pdev->dev, > - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); > - if (!config) > - return -ENOMEM; > - *config = imx_dmaengine_pcm_config; > + ret = snd_dmaengine_pcm_open(substream, chan); > + if (ret) > + goto pcm_open_fail; > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > - config, > - SND_DMAENGINE_PCM_FLAG_COMPAT); > + memset(&hw, 0, sizeof(hw)); > + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > + SNDRV_PCM_INFO_INTERLEAVED; > + hw.periods_min = 2; > + hw.periods_max = UINT_MAX; > + hw.period_bytes_min = 256; > + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > + hw.fifo_size = dma_data->fifo_size; > + > + /* Refine the hw according to caps of DMA. */ > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > + dma_data, > + &hw, > + chan); > + if (ret < 0) > + goto refine_runtime_hwparams_fail; > + > + snd_soc_set_runtime_hwparams(substream, &hw); > + > + /* Support allocate memory from IRAM */ > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > + chan->device->dev, > + hw.buffer_bytes_max, > + &substream->dma_buffer); > + if (ret < 0) > + goto alloc_pagas_fail; > + > + return 0; > + > +alloc_pagas_fail: > +refine_runtime_hwparams_fail: > + snd_dmaengine_pcm_close(substream); > +pcm_open_fail: > + dma_release_channel(chan); > + > + return ret; > +} > + > +static int imx_pcm_close(struct snd_soc_component *component, > + struct snd_pcm_substream *substream) > +{ > + if (substream) { > + snd_dma_free_pages(&substream->dma_buffer); > + substream->dma_buffer.area = NULL; > + substream->dma_buffer.addr = 0; > + } > + > + return snd_dmaengine_pcm_close_release_chan(substream); > +} > + > +static int imx_pcm_new(struct snd_soc_component *component, > + struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_card *card = rtd->card->snd_card; > + > + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > +} > + > +static const struct snd_soc_component_driver imx_pcm_component = { > + .name = "imx-pcm-dma", > + .pcm_construct = imx_pcm_new, > + .open = imx_pcm_open, > + .close = imx_pcm_close, > + .hw_params = imx_pcm_hw_params, > + .hw_free = imx_pcm_hw_free, > + .trigger = imx_pcm_trigger, > + .pointer = imx_pcm_pointer, > +}; > + > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > +{ > + return devm_snd_soc_register_component(&pdev->dev, > + &imx_pcm_component, NULL, 0); > } > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); >
Hi On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > There are two requirements that we need to move the request > > of dma channel from probe to open. > > How do you handle -EPROBE_DEFER return code from the channel request if > you don't do it in probe? I use the dma_request_slave_channel or dma_request_channel instead of dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER return code. > > > - When dma device binds with power-domains, the power will > > be enabled when we request dma channel. If the request of dma > > channel happen on probe, then the power-domains will be always > > enabled after kernel boot up, which is not good for power > > saving, so we need to move the request of dma channel to .open(); > > This is certainly something which could be fixed in the dmaengine > driver. Dma driver always call the pm_runtime_get_sync in device_alloc_chan_resources, the device_alloc_chan_resources is called when channel is requested. so power is enabled on channel request. > > > - With FE-BE case, if the dma channel is requested in probe, > > then there will be below issue, which is caused by that the > > dma channel will be requested duplicately > > Why is this requested a second time? Is this just some missing cleanup > on a deferred probe path? Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, the DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier probe, DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel is not used in this FE-BE case). When FE-BE startup, DMA2 channel is created, it needs the ESAI's "dma:tx", so below warning comes out. > > Regards, > Lucas > > > [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 > > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) > > [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) > > [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) > > [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) > > [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) > > [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) > > [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) > > [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) > > [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) > > [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > > [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 > > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > --- > > sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- > > 1 file changed, 159 insertions(+), 14 deletions(-) > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c > > index 04a9bc749016..dae53b384df4 100644 > > --- a/sound/soc/fsl/imx-pcm-dma.c > > +++ b/sound/soc/fsl/imx-pcm-dma.c > > @@ -11,6 +11,7 @@ > > #include <linux/dmaengine.h> > > #include <linux/types.h> > > #include <linux/module.h> > > +#include <linux/dma-mapping.h> > > > > #include <sound/core.h> > > #include <sound/pcm.h> > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) > > return true; > > } > > > > -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { > > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > > - .compat_filter_fn = filter, > > -}; > > +static int imx_pcm_hw_params(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream, > > + struct snd_pcm_hw_params *params) > > +{ > > + struct snd_pcm_runtime *runtime = substream->runtime; > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > + struct snd_dmaengine_dai_dma_data *dma_data; > > + struct dma_slave_config config; > > + struct dma_chan *chan; > > + int ret = 0; > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > > + runtime->dma_bytes = params_buffer_bytes(params); > > + > > + chan = snd_dmaengine_pcm_get_chan(substream); > > + if (!chan) > > + return -EINVAL; > > + > > + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); > > + if (ret) > > + return ret; > > + > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > + if (!dma_data) > > + return -EINVAL; > > + > > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > > + dma_data, > > + &config); > > + return dmaengine_slave_config(chan, &config); > > +} > > + > > +static int imx_pcm_hw_free(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream) > > { > > - struct snd_dmaengine_pcm_config *config; > > + snd_pcm_set_runtime_buffer(substream, NULL); > > + return 0; > > +} > > + > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream) > > +{ > > + return snd_dmaengine_pcm_pointer(substream); > > +} > > + > > +static int imx_pcm_trigger(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream, int cmd) > > +{ > > + return snd_dmaengine_pcm_trigger(substream, cmd); > > +} > > + > > +static int imx_pcm_open(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream) > > +{ > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > + struct snd_dmaengine_dai_dma_data *dma_data; > > + struct device *dev = component->dev; > > + struct snd_pcm_hardware hw; > > + struct dma_chan *chan; > > + int ret; > > + > > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > > + SNDRV_PCM_HW_PARAM_PERIODS); > > + if (ret < 0) { > > + dev_err(dev, "failed to set pcm hw params periods\n"); > > + return ret; > > + } > > + > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > + if (!dma_data) > > + return -EINVAL; > > + > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > > + if (!chan) { > > + /* Try to request channel using compat_filter_fn */ > > + chan = snd_dmaengine_pcm_request_channel(filter, > > + dma_data->filter_data); > > + if (!chan) > > + return -ENXIO; > > + } > > > > - config = devm_kzalloc(&pdev->dev, > > - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); > > - if (!config) > > - return -ENOMEM; > > - *config = imx_dmaengine_pcm_config; > > + ret = snd_dmaengine_pcm_open(substream, chan); > > + if (ret) > > + goto pcm_open_fail; > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > > - config, > > - SND_DMAENGINE_PCM_FLAG_COMPAT); > > + memset(&hw, 0, sizeof(hw)); > > + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > > + SNDRV_PCM_INFO_INTERLEAVED; > > + hw.periods_min = 2; > > + hw.periods_max = UINT_MAX; > > + hw.period_bytes_min = 256; > > + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > > + hw.fifo_size = dma_data->fifo_size; > > + > > + /* Refine the hw according to caps of DMA. */ > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > > + dma_data, > > + &hw, > > + chan); > > + if (ret < 0) > > + goto refine_runtime_hwparams_fail; > > + > > + snd_soc_set_runtime_hwparams(substream, &hw); > > + > > + /* Support allocate memory from IRAM */ > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > > + chan->device->dev, > > + hw.buffer_bytes_max, > > + &substream->dma_buffer); > > + if (ret < 0) > > + goto alloc_pagas_fail; > > + > > + return 0; > > + > > +alloc_pagas_fail: > > +refine_runtime_hwparams_fail: > > + snd_dmaengine_pcm_close(substream); > > +pcm_open_fail: > > + dma_release_channel(chan); > > + > > + return ret; > > +} > > + > > +static int imx_pcm_close(struct snd_soc_component *component, > > + struct snd_pcm_substream *substream) > > +{ > > + if (substream) { > > + snd_dma_free_pages(&substream->dma_buffer); > > + substream->dma_buffer.area = NULL; > > + substream->dma_buffer.addr = 0; > > + } > > + > > + return snd_dmaengine_pcm_close_release_chan(substream); > > +} > > + > > +static int imx_pcm_new(struct snd_soc_component *component, > > + struct snd_soc_pcm_runtime *rtd) > > +{ > > + struct snd_card *card = rtd->card->snd_card; > > + > > + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > +} > > + > > +static const struct snd_soc_component_driver imx_pcm_component = { > > + .name = "imx-pcm-dma", > > + .pcm_construct = imx_pcm_new, > > + .open = imx_pcm_open, > > + .close = imx_pcm_close, > > + .hw_params = imx_pcm_hw_params, > > + .hw_free = imx_pcm_hw_free, > > + .trigger = imx_pcm_trigger, > > + .pointer = imx_pcm_pointer, > > +}; > > + > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > +{ > > + return devm_snd_soc_register_component(&pdev->dev, > > + &imx_pcm_component, NULL, 0); > > } > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); > > >
Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > Hi > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > There are two requirements that we need to move the request > > > of dma channel from probe to open. > > > > How do you handle -EPROBE_DEFER return code from the channel request if > > you don't do it in probe? > > I use the dma_request_slave_channel or dma_request_channel instead > of dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > return code. This is a pretty weak argument. The dmaengine device might probe after you try to get the channel. Using a function to request the channel that doesn't allow you to handle probe deferral is IMHO a bug and should be fixed, instead of building even more assumptions on top of it. > > > - When dma device binds with power-domains, the power will > > > be enabled when we request dma channel. If the request of dma > > > channel happen on probe, then the power-domains will be always > > > enabled after kernel boot up, which is not good for power > > > saving, so we need to move the request of dma channel to .open(); > > > > This is certainly something which could be fixed in the dmaengine > > driver. > > Dma driver always call the pm_runtime_get_sync in > device_alloc_chan_resources, the device_alloc_chan_resources is > called when channel is requested. so power is enabled on channel > request. So why can't you fix the dmaengine driver to do that RPM call at a later time when the channel is actually going to be used? This will allow further power savings with other slave devices than the audio PCM. Regards, Lucas > > > - With FE-BE case, if the dma channel is requested in probe, > > > then there will be below issue, which is caused by that the > > > dma channel will be requested duplicately > > > > Why is this requested a second time? Is this just some missing cleanup > > on a deferred probe path? > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, > the DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier > probe, DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel > is not used in this FE-BE case). When FE-BE startup, DMA2 > channel is created, it needs the ESAI's "dma:tx", so below warning > comes out. > > > Regards, > > Lucas > > > > > [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 > > > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > > [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) > > > [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) > > > [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) > > > [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) > > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) > > > [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) > > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) > > > [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) > > > [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) > > > [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) > > > [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) > > > [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) > > > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > > > [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 > > > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > > > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > > --- > > > sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- > > > 1 file changed, 159 insertions(+), 14 deletions(-) > > > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c > > > index 04a9bc749016..dae53b384df4 100644 > > > --- a/sound/soc/fsl/imx-pcm-dma.c > > > +++ b/sound/soc/fsl/imx-pcm-dma.c > > > @@ -11,6 +11,7 @@ > > > #include <linux/dmaengine.h> > > > #include <linux/types.h> > > > #include <linux/module.h> > > > +#include <linux/dma-mapping.h> > > > > > > #include <sound/core.h> > > > #include <sound/pcm.h> > > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) > > > return true; > > > } > > > > > > -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { > > > - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, > > > - .compat_filter_fn = filter, > > > -}; > > > +static int imx_pcm_hw_params(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream, > > > + struct snd_pcm_hw_params *params) > > > +{ > > > + struct snd_pcm_runtime *runtime = substream->runtime; > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > + struct dma_slave_config config; > > > + struct dma_chan *chan; > > > + int ret = 0; > > > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); > > > + runtime->dma_bytes = params_buffer_bytes(params); > > > + > > > + chan = snd_dmaengine_pcm_get_chan(substream); > > > + if (!chan) > > > + return -EINVAL; > > > + > > > + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); > > > + if (ret) > > > + return ret; > > > + > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > + if (!dma_data) > > > + return -EINVAL; > > > + > > > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > > > + dma_data, > > > + &config); > > > + return dmaengine_slave_config(chan, &config); > > > +} > > > + > > > +static int imx_pcm_hw_free(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > { > > > - struct snd_dmaengine_pcm_config *config; > > > + snd_pcm_set_runtime_buffer(substream, NULL); > > > + return 0; > > > +} > > > + > > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + return snd_dmaengine_pcm_pointer(substream); > > > +} > > > + > > > +static int imx_pcm_trigger(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream, int cmd) > > > +{ > > > + return snd_dmaengine_pcm_trigger(substream, cmd); > > > +} > > > + > > > +static int imx_pcm_open(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > + struct device *dev = component->dev; > > > + struct snd_pcm_hardware hw; > > > + struct dma_chan *chan; > > > + int ret; > > > + > > > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > > > + SNDRV_PCM_HW_PARAM_PERIODS); > > > + if (ret < 0) { > > > + dev_err(dev, "failed to set pcm hw params periods\n"); > > > + return ret; > > > + } > > > + > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > + if (!dma_data) > > > + return -EINVAL; > > > + > > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > > > + if (!chan) { > > > + /* Try to request channel using compat_filter_fn */ > > > + chan = snd_dmaengine_pcm_request_channel(filter, > > > + dma_data->filter_data); > > > + if (!chan) > > > + return -ENXIO; > > > + } > > > > > > - config = devm_kzalloc(&pdev->dev, > > > - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); > > > - if (!config) > > > - return -ENOMEM; > > > - *config = imx_dmaengine_pcm_config; > > > + ret = snd_dmaengine_pcm_open(substream, chan); > > > + if (ret) > > > + goto pcm_open_fail; > > > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > > > - config, > > > - SND_DMAENGINE_PCM_FLAG_COMPAT); > > > + memset(&hw, 0, sizeof(hw)); > > > + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | > > > + SNDRV_PCM_INFO_INTERLEAVED; > > > + hw.periods_min = 2; > > > + hw.periods_max = UINT_MAX; > > > + hw.period_bytes_min = 256; > > > + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); > > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > > > + hw.fifo_size = dma_data->fifo_size; > > > + > > > + /* Refine the hw according to caps of DMA. */ > > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > > > + dma_data, > > > + &hw, > > > + chan); > > > + if (ret < 0) > > > + goto refine_runtime_hwparams_fail; > > > + > > > + snd_soc_set_runtime_hwparams(substream, &hw); > > > + > > > + /* Support allocate memory from IRAM */ > > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > > > + chan->device->dev, > > > + hw.buffer_bytes_max, > > > + &substream->dma_buffer); > > > + if (ret < 0) > > > + goto alloc_pagas_fail; > > > + > > > + return 0; > > > + > > > +alloc_pagas_fail: > > > +refine_runtime_hwparams_fail: > > > + snd_dmaengine_pcm_close(substream); > > > +pcm_open_fail: > > > + dma_release_channel(chan); > > > + > > > + return ret; > > > +} > > > + > > > +static int imx_pcm_close(struct snd_soc_component *component, > > > + struct snd_pcm_substream *substream) > > > +{ > > > + if (substream) { > > > + snd_dma_free_pages(&substream->dma_buffer); > > > + substream->dma_buffer.area = NULL; > > > + substream->dma_buffer.addr = 0; > > > + } > > > + > > > + return snd_dmaengine_pcm_close_release_chan(substream); > > > +} > > > + > > > +static int imx_pcm_new(struct snd_soc_component *component, > > > + struct snd_soc_pcm_runtime *rtd) > > > +{ > > > + struct snd_card *card = rtd->card->snd_card; > > > + > > > + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); > > > +} > > > + > > > +static const struct snd_soc_component_driver imx_pcm_component = { > > > + .name = "imx-pcm-dma", > > > + .pcm_construct = imx_pcm_new, > > > + .open = imx_pcm_open, > > > + .close = imx_pcm_close, > > > + .hw_params = imx_pcm_hw_params, > > > + .hw_free = imx_pcm_hw_free, > > > + .trigger = imx_pcm_trigger, > > > + .pointer = imx_pcm_pointer, > > > +}; > > > + > > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > +{ > > > + return devm_snd_soc_register_component(&pdev->dev, > > > + &imx_pcm_component, NULL, 0); > > > } > > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); > > >
Hi On Wed, May 20, 2020 at 5:42 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > Hi > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > There are two requirements that we need to move the request > > > > of dma channel from probe to open. > > > > > > How do you handle -EPROBE_DEFER return code from the channel request if > > > you don't do it in probe? > > > > I use the dma_request_slave_channel or dma_request_channel instead > > of dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > > return code. > > This is a pretty weak argument. The dmaengine device might probe after > you try to get the channel. Using a function to request the channel > that doesn't allow you to handle probe deferral is IMHO a bug and > should be fixed, instead of building even more assumptions on top of > it. > I see some driver also request dma channel in open() or hw_params(). how can they avoid the defer probe issue? for example： sound/arm/pxa2xx-pcm-lib.c sound/soc/sprd/sprd-pcm-dma.c > > > > - When dma device binds with power-domains, the power will > > > > be enabled when we request dma channel. If the request of dma > > > > channel happen on probe, then the power-domains will be always > > > > enabled after kernel boot up, which is not good for power > > > > saving, so we need to move the request of dma channel to .open(); > > > > > > This is certainly something which could be fixed in the dmaengine > > > driver. > > > > Dma driver always call the pm_runtime_get_sync in > > device_alloc_chan_resources, the device_alloc_chan_resources is > > called when channel is requested. so power is enabled on channel > > request. > > So why can't you fix the dmaengine driver to do that RPM call at a > later time when the channel is actually going to be used? This will > allow further power savings with other slave devices than the audio > PCM. > > Regards, > Lucas > It seems the best place for calling pm_runtime_get_sync is the device_alloc_chan_resources, and calling pm_runtime_put_sync in the .device_free_chan_resources For the slave_sg mode, the .device_prep_slave_sg and .device_issue_pending will be called many times after .device_alloc_chan_resources. so it is not good to call pm_runtime_get_sync in .device_prep_slave_sg or .device_issue_pending best regards wang shengjiu
On Wed, May 20, 2020 at 07:22:19PM +0800, Shengjiu Wang wrote: > I see some driver also request dma channel in open() or hw_params(). > how can they avoid the defer probe issue? > for example： > sound/arm/pxa2xx-pcm-lib.c > sound/soc/sprd/sprd-pcm-dma.c Other drivers having problems means those drivers should be fixed, not that we should copy the problems. In the case of the PXA driver that's very old code which predates deferred probe by I'd guess a decade.
On Wed, May 20, 2020 at 8:38 PM Mark Brown <broonie@kernel.org> wrote: > > On Wed, May 20, 2020 at 07:22:19PM +0800, Shengjiu Wang wrote: > > > I see some driver also request dma channel in open() or hw_params(). > > how can they avoid the defer probe issue? > > for example： > > sound/arm/pxa2xx-pcm-lib.c > > sound/soc/sprd/sprd-pcm-dma.c > > Other drivers having problems means those drivers should be fixed, not > that we should copy the problems. In the case of the PXA driver that's > very old code which predates deferred probe by I'd guess a decade. Thanks. For the FE-BE case, do you have any suggestion for how fix it? With DMA1->ASRC->DMA2->ESAI case, the DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier probe, DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel is not used in this FE-BE case). When FE-BE startup, DMA2 channel is created, it needs the ESAI's "dma:tx", so the warning comes out. best regards wang shengjiu
On Thu, May 21, 2020 at 07:30:04PM +0800, Shengjiu Wang wrote: > On Wed, May 20, 2020 at 8:38 PM Mark Brown <broonie@kernel.org> wrote: > > Other drivers having problems means those drivers should be fixed, not > > that we should copy the problems. In the case of the PXA driver that's > > very old code which predates deferred probe by I'd guess a decade. > Thanks. > For the FE-BE case, do you have any suggestion for how fix it? > With DMA1->ASRC->DMA2->ESAI case, the DMA1->ASRC->DMA2 > is in FE, ESAI is in BE. When ESAI drvier probe, DMA3 channel is > created with ESAI's "dma:tx" (DMA3 channel > is not used in this FE-BE case). When FE-BE startup, DMA2 > channel is created, it needs the ESAI's "dma:tx", so the warning > comes out. Not really TBH, this seems like another one of those csaes where DPCM is creaking at the seams :/
On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > Hi > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de> > wrote: > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > There are two requirements that we need to move the request of dma > > > > channel from probe to open. > > > > > > How do you handle -EPROBE_DEFER return code from the channel request > > > if you don't do it in probe? > > > > I use the dma_request_slave_channel or dma_request_channel instead of > > dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > > return code. > > This is a pretty weak argument. The dmaengine device might probe after you > try to get the channel. Using a function to request the channel that doesn't > allow you to handle probe deferral is IMHO a bug and should be fixed, instead > of building even more assumptions on top of it. > > > > > - When dma device binds with power-domains, the power will be > > > > enabled when we request dma channel. If the request of dma channel > > > > happen on probe, then the power-domains will be always enabled > > > > after kernel boot up, which is not good for power saving, so we > > > > need to move the request of dma channel to .open(); > > > > > > This is certainly something which could be fixed in the dmaengine > > > driver. > > > > Dma driver always call the pm_runtime_get_sync in > > device_alloc_chan_resources, the device_alloc_chan_resources is called > > when channel is requested. so power is enabled on channel request. > > So why can't you fix the dmaengine driver to do that RPM call at a later time > when the channel is actually going to be used? This will allow further power > savings with other slave devices than the audio PCM. Hi Lucas, Thanks for your suggestion. I have tried to implement runtime autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) for this feature as below (or you can refer to drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ pm_runtime_put_autosuspend in all dmaengine driver interface like device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cyclic/ device_tx_status... pm_runtime_use_autosuspend(fsl_chan->dev); pm_runtime_set_autosuspend_delay(fsl_chan->dev, 2000); That could resolve this audio case since the autosuspend could suspend runtime after 2 seconds if there is no further dma transfer but only channel request(device_alloc_chan_resources). But unfortunately, it cause another issue. As you know, on our i.mx8qm/qxp, power domain done by scfw (drivers/firmware/imx/scu-pd.c) over mailbox: imx_sc_pd_power()->imx_scu_call_rpc()-> imx_scu_ipc_write()->mbox_send_message() which means have to 'waits for completion', meanwhile, some driver like tty will call dmaengine interfaces in non-atomic case as below, static int uart_write(struct tty_struct *tty, const unsigned char *buf, int count) { ....... port = uart_port_lock(state, flags); ...... __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg... uart_port_unlock(port, flags); return ret; } Thus dma runtime resume may happen in that timing window and cause kernel alarm. I'm not sure whether there are similar limitations on other driver subsystem. But for me, It looks like the only way to resolve the contradiction between tty and scu-pd (hardware limitation on i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync only in device_alloc_chan_resources because request channel is a safe non-atomic phase. Do you have any idea? Thanks in advance. > > > > > - With FE-BE case, if the dma channel is requested in probe, then > > > > there will be below issue, which is caused by that the dma channel > > > > will be requested duplicately > > > > > > Why is this requested a second time? Is this just some missing > > > cleanup on a deferred probe path? > > > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, the > > DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier probe, > > DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel > > is not used in this FE-BE case). When FE-BE startup, DMA2 > > channel is created, it needs the ESAI's "dma:tx", so below warning > > comes out. > > > > > Regards, > > > Lucas > > > > > > > [ 638.906268] sysfs: cannot create duplicate filename > '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted > > > > 5.7.0-rc1-12956-gfc64b2585593 #287 [ 638.927113] Hardware name: > > > > Freescale i.MX6 Quad/DualLite (Device Tree) [ 638.933690] > > > > [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] > > > > (show_stack+0x10/0x14) [ 638.941464] [<c010b8ec>] (show_stack) > > > > from [<c0557fc0>] (dump_stack+0xe4/0x118) [ 638.948808] > > > > [<c0557fc0>] (dump_stack) from [<c032aeb4>] > > > > (sysfs_warn_dup+0x50/0x64) [ 638.956406] [<c032aeb4>] > > > > (sysfs_warn_dup) from [<c032b1a8>] > > > > (sysfs_do_create_link_sd+0xc8/0xd4) > > > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from > > > > [<c05dc668>] (dma_request_chan+0xb0/0x210) [ 638.974120] > > > > [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] > > > > (dma_request_slave_channel+0x8/0x14) > > > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from > > > > [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > > > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from > > > > [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) [ 639.001952] > > > > [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] > > > > (dpcm_fe_dai_hw_params+0x70/0xe4) [ 639.010765] [<c09c39d4>] > > > > (dpcm_fe_dai_hw_params) from [<c099b274>] > > > > (snd_pcm_hw_params+0x158/0x418) [ 639.019750] [<c099b274>] > > > > (snd_pcm_hw_params) from [<c099c5a0>] > (snd_pcm_ioctl+0x734/0x183c) [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) > from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) [ 639.035812] [<c029ff94>] > (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) [ 639.043490] > Exception stack(0xec529fa8 to 0xec529ff0) > > > > [ 639.048565] 9fa0: bee84650 01321870 > 00000004 c25c4111 bee84650 0002000f > > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 > 00001f40 > > > > 00000000 0002c2f8 00000003 [ 639.064964] 9fe0: b6f483fc bee8451c > > > > b6ee2655 b6e1dcf8 [ 639.070339] fsl-esai-dai 2024000.esai: Cannot > > > > create DMA dma:tx symlink > > > > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > > > --- > > > > sound/soc/fsl/imx-pcm-dma.c | 173 > > > > +++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 159 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c > > > > b/sound/soc/fsl/imx-pcm-dma.c index 04a9bc749016..dae53b384df4 > > > > 100644 > > > > --- a/sound/soc/fsl/imx-pcm-dma.c > > > > +++ b/sound/soc/fsl/imx-pcm-dma.c > > > > @@ -11,6 +11,7 @@ > > > > #include <linux/dmaengine.h> > > > > #include <linux/types.h> > > > > #include <linux/module.h> > > > > +#include <linux/dma-mapping.h> > > > > > > > > #include <sound/core.h> > > > > #include <sound/pcm.h> > > > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void > *param) > > > > return true; > > > > } > > > > > > > > -static const struct snd_dmaengine_pcm_config > imx_dmaengine_pcm_config = { > > > > - .prepare_slave_config = > snd_dmaengine_pcm_prepare_slave_config, > > > > - .compat_filter_fn = filter, > > > > -}; > > > > +static int imx_pcm_hw_params(struct snd_soc_component > *component, > > > > + struct snd_pcm_substream *substream, > > > > + struct snd_pcm_hw_params *params) { > > > > + struct snd_pcm_runtime *runtime = substream->runtime; > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > > + struct dma_slave_config config; > > > > + struct dma_chan *chan; > > > > + int ret = 0; > > > > > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > > + snd_pcm_set_runtime_buffer(substream, > &substream->dma_buffer); > > > > + runtime->dma_bytes = params_buffer_bytes(params); > > > > + > > > > + chan = snd_dmaengine_pcm_get_chan(substream); > > > > + if (!chan) > > > > + return -EINVAL; > > > > + > > > > + ret = snd_hwparams_to_dma_slave_config(substream, params, > &config); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > > + if (!dma_data) > > > > + return -EINVAL; > > > > + > > > > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > > > > + dma_data, > > > > + &config); > > > > + return dmaengine_slave_config(chan, &config); } > > > > + > > > > +static int imx_pcm_hw_free(struct snd_soc_component *component, > > > > + struct snd_pcm_substream *substream) > > > > { > > > > - struct snd_dmaengine_pcm_config *config; > > > > + snd_pcm_set_runtime_buffer(substream, NULL); > > > > + return 0; > > > > +} > > > > + > > > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component > *component, > > > > + struct snd_pcm_substream > > > > +*substream) { > > > > + return snd_dmaengine_pcm_pointer(substream); > > > > +} > > > > + > > > > +static int imx_pcm_trigger(struct snd_soc_component *component, > > > > + struct snd_pcm_substream *substream, int > > > > +cmd) { > > > > + return snd_dmaengine_pcm_trigger(substream, cmd); } > > > > + > > > > +static int imx_pcm_open(struct snd_soc_component *component, > > > > + struct snd_pcm_substream *substream) { > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > > + struct device *dev = component->dev; > > > > + struct snd_pcm_hardware hw; > > > > + struct dma_chan *chan; > > > > + int ret; > > > > + > > > > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > > > > + > SNDRV_PCM_HW_PARAM_PERIODS); > > > > + if (ret < 0) { > > > > + dev_err(dev, "failed to set pcm hw params periods\n"); > > > > + return ret; > > > > + } > > > > + > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > > + if (!dma_data) > > > > + return -EINVAL; > > > > + > > > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > > > > + if (!chan) { > > > > + /* Try to request channel using compat_filter_fn */ > > > > + chan = snd_dmaengine_pcm_request_channel(filter, > > > > + > dma_data->filter_data); > > > > + if (!chan) > > > > + return -ENXIO; > > > > + } > > > > > > > > - config = devm_kzalloc(&pdev->dev, > > > > - sizeof(struct snd_dmaengine_pcm_config), > GFP_KERNEL); > > > > - if (!config) > > > > - return -ENOMEM; > > > > - *config = imx_dmaengine_pcm_config; > > > > + ret = snd_dmaengine_pcm_open(substream, chan); > > > > + if (ret) > > > > + goto pcm_open_fail; > > > > > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > > > > - config, > > > > - SND_DMAENGINE_PCM_FLAG_COMPAT); > > > > + memset(&hw, 0, sizeof(hw)); > > > > + hw.info = SNDRV_PCM_INFO_MMAP | > SNDRV_PCM_INFO_MMAP_VALID | > > > > + SNDRV_PCM_INFO_INTERLEAVED; > > > > + hw.periods_min = 2; > > > > + hw.periods_max = UINT_MAX; > > > > + hw.period_bytes_min = 256; > > > > + hw.period_bytes_max = > dma_get_max_seg_size(chan->device->dev); > > > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > > > > + hw.fifo_size = dma_data->fifo_size; > > > > + > > > > + /* Refine the hw according to caps of DMA. */ > > > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > > > > + > dma_data, > > > > + &hw, > > > > + chan); > > > > + if (ret < 0) > > > > + goto refine_runtime_hwparams_fail; > > > > + > > > > + snd_soc_set_runtime_hwparams(substream, &hw); > > > > + > > > > + /* Support allocate memory from IRAM */ > > > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > > > > + chan->device->dev, > > > > + hw.buffer_bytes_max, > > > > + &substream->dma_buffer); > > > > + if (ret < 0) > > > > + goto alloc_pagas_fail; > > > > + > > > > + return 0; > > > > + > > > > +alloc_pagas_fail: > > > > +refine_runtime_hwparams_fail: > > > > + snd_dmaengine_pcm_close(substream); > > > > +pcm_open_fail: > > > > + dma_release_channel(chan); > > > > + > > > > + return ret; > > > > +} > > > > + > > > > +static int imx_pcm_close(struct snd_soc_component *component, > > > > + struct snd_pcm_substream *substream) { > > > > + if (substream) { > > > > + snd_dma_free_pages(&substream->dma_buffer); > > > > + substream->dma_buffer.area = NULL; > > > > + substream->dma_buffer.addr = 0; > > > > + } > > > > + > > > > + return snd_dmaengine_pcm_close_release_chan(substream); > > > > +} > > > > + > > > > +static int imx_pcm_new(struct snd_soc_component *component, > > > > + struct snd_soc_pcm_runtime *rtd) { > > > > + struct snd_card *card = rtd->card->snd_card; > > > > + > > > > + return dma_coerce_mask_and_coherent(card->dev, > > > > +DMA_BIT_MASK(32)); } > > > > + > > > > +static const struct snd_soc_component_driver imx_pcm_component = { > > > > + .name = "imx-pcm-dma", > > > > + .pcm_construct = imx_pcm_new, > > > > + .open = imx_pcm_open, > > > > + .close = imx_pcm_close, > > > > + .hw_params = imx_pcm_hw_params, > > > > + .hw_free = imx_pcm_hw_free, > > > > + .trigger = imx_pcm_trigger, > > > > + .pointer = imx_pcm_pointer, > > > > +}; > > > > + > > > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) { > > > > + return devm_snd_soc_register_component(&pdev->dev, > > > > + > &imx_pcm_component, > > > > +NULL, 0); > > > > } > > > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); > > > >
Hi Robin, Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > Hi > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach <l.stach@pengutronix.de> > > wrote: > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > There are two requirements that we need to move the request of dma > > > > > channel from probe to open. > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel request > > > > if you don't do it in probe? > > > > > > I use the dma_request_slave_channel or dma_request_channel instead of > > > dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > > > return code. > > > > This is a pretty weak argument. The dmaengine device might probe after you > > try to get the channel. Using a function to request the channel that doesn't > > allow you to handle probe deferral is IMHO a bug and should be fixed, instead > > of building even more assumptions on top of it. > > > > > > > - When dma device binds with power-domains, the power will be > > > > > enabled when we request dma channel. If the request of dma channel > > > > > happen on probe, then the power-domains will be always enabled > > > > > after kernel boot up, which is not good for power saving, so we > > > > > need to move the request of dma channel to .open(); > > > > > > > > This is certainly something which could be fixed in the dmaengine > > > > driver. > > > > > > Dma driver always call the pm_runtime_get_sync in > > > device_alloc_chan_resources, the device_alloc_chan_resources is called > > > when channel is requested. so power is enabled on channel request. > > > > So why can't you fix the dmaengine driver to do that RPM call at a later time > > when the channel is actually going to be used? This will allow further power > > savings with other slave devices than the audio PCM. > Hi Lucas, > Thanks for your suggestion. I have tried to implement runtime autosuspend in > fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) for this feature as below > (or you can refer to drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > pm_runtime_put_autosuspend in all dmaengine driver interface like > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cyclic/ > device_tx_status... > > > pm_runtime_use_autosuspend(fsl_chan->dev); > pm_runtime_set_autosuspend_delay(fsl_chan->dev, 2000); > > That could resolve this audio case since the autosuspend could suspend runtime after > 2 seconds if there is no further dma transfer but only channel request(device_alloc_chan_resources). > But unfortunately, it cause another issue. As you know, on our i.mx8qm/qxp, > power domain done by scfw (drivers/firmware/imx/scu-pd.c) over mailbox: > imx_sc_pd_power()->imx_scu_call_rpc()-> imx_scu_ipc_write()->mbox_send_message() > which means have to 'waits for completion', meanwhile, some driver like tty will call dmaengine > interfaces in non-atomic case as below, > > static int uart_write(struct tty_struct *tty, const unsigned char *buf, int count) > { > ....... > port = uart_port_lock(state, flags); > ...... > __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg... > uart_port_unlock(port, flags); > return ret; > } > > Thus dma runtime resume may happen in that timing window and cause kernel alarm. > I'm not sure whether there are similar limitations on other driver subsystem. But for me, > It looks like the only way to resolve the contradiction between tty and scu-pd (hardware > limitation on i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync > only in device_alloc_chan_resources because request channel is a safe non-atomic phase. > Do you have any idea? Thanks in advance. If you look closely at the driver you used as an example (hidma.c) it looks like there is already something in there, which looks very much like what you need here: In hidma_issue_pending() the driver tries to get the device to runtime resume. If this doesn't work, maybe due to the power domain code not being able to be called in atomic context, the actual work of waking up the dma hardware and issuing the descriptor is shunted to a tasklet. If I'm reading this right, this is exactly what you need here to be able to call the dmaengine code from atomic context: try the rpm get and issue immediately when possible, otherwise shunt the work to a non- atomic context where you can deal with the requirements of scu-pd. Also you don't need the runtime resume in all of the functions you mentioned. From a quick look into the edma driver it looks like for example the prep_slave_dma() function only touches data structures in memory, so you don't actually need the device to be awake at that point. Only later in the flow when you write registers in the dma hardware and actually issue the transfer you need to wake the device from sleep. Regards, Lucas > > > > > > > > > > > > - With FE-BE case, if the dma channel is requested in probe, then > > > > > there will be below issue, which is caused by that the dma channel > > > > > will be requested duplicately > > > > > > > > Why is this requested a second time? Is this just some missing > > > > cleanup on a deferred probe path? > > > > > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, the > > > DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier probe, > > > DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel > > > is not used in this FE-BE case). When FE-BE startup, DMA2 > > > channel is created, it needs the ESAI's "dma:tx", so below warning > > > comes out. > > > > > > > Regards, > > > > Lucas > > > > > > > > > [ 638.906268] sysfs: cannot create duplicate filename > > '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' > > > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted > > > > > 5.7.0-rc1-12956-gfc64b2585593 #287 [ 638.927113] Hardware name: > > > > > Freescale i.MX6 Quad/DualLite (Device Tree) [ 638.933690] > > > > > [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] > > > > > (show_stack+0x10/0x14) [ 638.941464] [<c010b8ec>] (show_stack) > > > > > from [<c0557fc0>] (dump_stack+0xe4/0x118) [ 638.948808] > > > > > [<c0557fc0>] (dump_stack) from [<c032aeb4>] > > > > > (sysfs_warn_dup+0x50/0x64) [ 638.956406] [<c032aeb4>] > > > > > (sysfs_warn_dup) from [<c032b1a8>] > > > > > (sysfs_do_create_link_sd+0xc8/0xd4) > > > > > [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from > > > > > [<c05dc668>] (dma_request_chan+0xb0/0x210) [ 638.974120] > > > > > [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] > > > > > (dma_request_slave_channel+0x8/0x14) > > > > > [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from > > > > > [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) > > > > > [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from > > > > > [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) [ 639.001952] > > > > > [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] > > > > > (dpcm_fe_dai_hw_params+0x70/0xe4) [ 639.010765] [<c09c39d4>] > > > > > (dpcm_fe_dai_hw_params) from [<c099b274>] > > > > > (snd_pcm_hw_params+0x158/0x418) [ 639.019750] [<c099b274>] > > > > > (snd_pcm_hw_params) from [<c099c5a0>] > > (snd_pcm_ioctl+0x734/0x183c) [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) > > from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) [ 639.035812] [<c029ff94>] > > (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) [ 639.043490] > > Exception stack(0xec529fa8 to 0xec529ff0) > > > > > [ 639.048565] 9fa0: bee84650 01321870 > > 00000004 c25c4111 bee84650 0002000f > > > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 > > 00001f40 > > > > > 00000000 0002c2f8 00000003 [ 639.064964] 9fe0: b6f483fc bee8451c > > > > > b6ee2655 b6e1dcf8 [ 639.070339] fsl-esai-dai 2024000.esai: Cannot > > > > > create DMA dma:tx symlink > > > > > > > > > > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> > > > > > --- > > > > > sound/soc/fsl/imx-pcm-dma.c | 173 > > > > > +++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 159 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/sound/soc/fsl/imx-pcm-dma.c > > > > > b/sound/soc/fsl/imx-pcm-dma.c index 04a9bc749016..dae53b384df4 > > > > > 100644 > > > > > --- a/sound/soc/fsl/imx-pcm-dma.c > > > > > +++ b/sound/soc/fsl/imx-pcm-dma.c > > > > > @@ -11,6 +11,7 @@ > > > > > #include <linux/dmaengine.h> > > > > > #include <linux/types.h> > > > > > #include <linux/module.h> > > > > > +#include <linux/dma-mapping.h> > > > > > > > > > > #include <sound/core.h> > > > > > #include <sound/pcm.h> > > > > > @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void > > *param) > > > > > return true; > > > > > } > > > > > > > > > > -static const struct snd_dmaengine_pcm_config > > imx_dmaengine_pcm_config = { > > > > > - .prepare_slave_config = > > snd_dmaengine_pcm_prepare_slave_config, > > > > > - .compat_filter_fn = filter, > > > > > -}; > > > > > +static int imx_pcm_hw_params(struct snd_soc_component > > *component, > > > > > + struct snd_pcm_substream *substream, > > > > > + struct snd_pcm_hw_params *params) { > > > > > + struct snd_pcm_runtime *runtime = substream->runtime; > > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > > > + struct dma_slave_config config; > > > > > + struct dma_chan *chan; > > > > > + int ret = 0; > > > > > > > > > > -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) > > > > > + snd_pcm_set_runtime_buffer(substream, > > &substream->dma_buffer); > > > > > + runtime->dma_bytes = params_buffer_bytes(params); > > > > > + > > > > > + chan = snd_dmaengine_pcm_get_chan(substream); > > > > > + if (!chan) > > > > > + return -EINVAL; > > > > > + > > > > > + ret = snd_hwparams_to_dma_slave_config(substream, params, > > &config); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > > > + if (!dma_data) > > > > > + return -EINVAL; > > > > > + > > > > > + snd_dmaengine_pcm_set_config_from_dai_data(substream, > > > > > + dma_data, > > > > > + &config); > > > > > + return dmaengine_slave_config(chan, &config); } > > > > > + > > > > > +static int imx_pcm_hw_free(struct snd_soc_component *component, > > > > > + struct snd_pcm_substream *substream) > > > > > { > > > > > - struct snd_dmaengine_pcm_config *config; > > > > > + snd_pcm_set_runtime_buffer(substream, NULL); > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component > > *component, > > > > > + struct snd_pcm_substream > > > > > +*substream) { > > > > > + return snd_dmaengine_pcm_pointer(substream); > > > > > +} > > > > > + > > > > > +static int imx_pcm_trigger(struct snd_soc_component *component, > > > > > + struct snd_pcm_substream *substream, int > > > > > +cmd) { > > > > > + return snd_dmaengine_pcm_trigger(substream, cmd); } > > > > > + > > > > > +static int imx_pcm_open(struct snd_soc_component *component, > > > > > + struct snd_pcm_substream *substream) { > > > > > + struct snd_soc_pcm_runtime *rtd = substream->private_data; > > > > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; > > > > > + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); > > > > > + struct snd_dmaengine_dai_dma_data *dma_data; > > > > > + struct device *dev = component->dev; > > > > > + struct snd_pcm_hardware hw; > > > > > + struct dma_chan *chan; > > > > > + int ret; > > > > > + > > > > > + ret = snd_pcm_hw_constraint_integer(substream->runtime, > > > > > + > > SNDRV_PCM_HW_PARAM_PERIODS); > > > > > + if (ret < 0) { > > > > > + dev_err(dev, "failed to set pcm hw params periods\n"); > > > > > + return ret; > > > > > + } > > > > > + > > > > > + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); > > > > > + if (!dma_data) > > > > > + return -EINVAL; > > > > > + > > > > > + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); > > > > > + if (!chan) { > > > > > + /* Try to request channel using compat_filter_fn */ > > > > > + chan = snd_dmaengine_pcm_request_channel(filter, > > > > > + > > dma_data->filter_data); > > > > > + if (!chan) > > > > > + return -ENXIO; > > > > > + } > > > > > > > > > > - config = devm_kzalloc(&pdev->dev, > > > > > - sizeof(struct snd_dmaengine_pcm_config), > > GFP_KERNEL); > > > > > - if (!config) > > > > > - return -ENOMEM; > > > > > - *config = imx_dmaengine_pcm_config; > > > > > + ret = snd_dmaengine_pcm_open(substream, chan); > > > > > + if (ret) > > > > > + goto pcm_open_fail; > > > > > > > > > > - return devm_snd_dmaengine_pcm_register(&pdev->dev, > > > > > - config, > > > > > - SND_DMAENGINE_PCM_FLAG_COMPAT); > > > > > + memset(&hw, 0, sizeof(hw)); > > > > > + hw.info = SNDRV_PCM_INFO_MMAP | > > SNDRV_PCM_INFO_MMAP_VALID | > > > > > + SNDRV_PCM_INFO_INTERLEAVED; > > > > > + hw.periods_min = 2; > > > > > + hw.periods_max = UINT_MAX; > > > > > + hw.period_bytes_min = 256; > > > > > + hw.period_bytes_max = > > dma_get_max_seg_size(chan->device->dev); > > > > > + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; > > > > > + hw.fifo_size = dma_data->fifo_size; > > > > > + > > > > > + /* Refine the hw according to caps of DMA. */ > > > > > + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, > > > > > + > > dma_data, > > > > > + &hw, > > > > > + chan); > > > > > + if (ret < 0) > > > > > + goto refine_runtime_hwparams_fail; > > > > > + > > > > > + snd_soc_set_runtime_hwparams(substream, &hw); > > > > > + > > > > > + /* Support allocate memory from IRAM */ > > > > > + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, > > > > > + chan->device->dev, > > > > > + hw.buffer_bytes_max, > > > > > + &substream->dma_buffer); > > > > > + if (ret < 0) > > > > > + goto alloc_pagas_fail; > > > > > + > > > > > + return 0; > > > > > + > > > > > +alloc_pagas_fail: > > > > > +refine_runtime_hwparams_fail: > > > > > + snd_dmaengine_pcm_close(substream); > > > > > +pcm_open_fail: > > > > > + dma_release_channel(chan); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static int imx_pcm_close(struct snd_soc_component *component, > > > > > + struct snd_pcm_substream *substream) { > > > > > + if (substream) { > > > > > + snd_dma_free_pages(&substream->dma_buffer); > > > > > + substream->dma_buffer.area = NULL; > > > > > + substream->dma_buffer.addr = 0; > > > > > + } > > > > > + > > > > > + return snd_dmaengine_pcm_close_release_chan(substream); > > > > > +} > > > > > + > > > > > +static int imx_pcm_new(struct snd_soc_component *component, > > > > > + struct snd_soc_pcm_runtime *rtd) { > > > > > + struct snd_card *card = rtd->card->snd_card; > > > > > + > > > > > + return dma_coerce_mask_and_coherent(card->dev, > > > > > +DMA_BIT_MASK(32)); } > > > > > + > > > > > +static const struct snd_soc_component_driver imx_pcm_component = { > > > > > + .name = "imx-pcm-dma", > > > > > + .pcm_construct = imx_pcm_new, > > > > > + .open = imx_pcm_open, > > > > > + .close = imx_pcm_close, > > > > > + .hw_params = imx_pcm_hw_params, > > > > > + .hw_free = imx_pcm_hw_free, > > > > > + .trigger = imx_pcm_trigger, > > > > > + .pointer = imx_pcm_pointer, > > > > > +}; > > > > > + > > > > > +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) { > > > > > + return devm_snd_soc_register_component(&pdev->dev, > > > > > + > > &imx_pcm_component, > > > > > +NULL, 0); > > > > > } > > > > > EXPORT_SYMBOL_GPL(imx_pcm_dma_init); > > > > > >
Hi Lucas, On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > Hi Robin, > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > > Hi > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > <l.stach@pengutronix.de> > > > wrote: > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > > There are two requirements that we need to move the request of > > > > > > dma channel from probe to open. > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel > > > > > request if you don't do it in probe? > > > > > > > > I use the dma_request_slave_channel or dma_request_channel instead > > > > of dmaengine_pcm_request_chan_of. so there should be not > > > > -EPROBE_DEFER return code. > > > > > > This is a pretty weak argument. The dmaengine device might probe > > > after you try to get the channel. Using a function to request the > > > channel that doesn't allow you to handle probe deferral is IMHO a > > > bug and should be fixed, instead of building even more assumptions on top > of it. > > > > > > > > > - When dma device binds with power-domains, the power will be > > > > > > enabled when we request dma channel. If the request of dma > > > > > > channel happen on probe, then the power-domains will be always > > > > > > enabled after kernel boot up, which is not good for power > > > > > > saving, so we need to move the request of dma channel to > > > > > > .open(); > > > > > > > > > > This is certainly something which could be fixed in the > > > > > dmaengine driver. > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > device_alloc_chan_resources, the device_alloc_chan_resources is > > > > called when channel is requested. so power is enabled on channel > request. > > > > > > So why can't you fix the dmaengine driver to do that RPM call at a > > > later time when the channel is actually going to be used? This will > > > allow further power savings with other slave devices than the audio PCM. > > Hi Lucas, > > Thanks for your suggestion. I have tried to implement runtime > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) > > for this feature as below (or you can refer to > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > pm_runtime_put_autosuspend in all dmaengine driver interface like > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cycli > > c/ > > device_tx_status... > > > > > > pm_runtime_use_autosuspend(fsl_chan->dev); > > pm_runtime_set_autosuspend_delay(fsl_chan->dev, > 2000); > > > > That could resolve this audio case since the autosuspend could suspend > > runtime after > > 2 seconds if there is no further dma transfer but only channel > request(device_alloc_chan_resources). > > But unfortunately, it cause another issue. As you know, on our > > i.mx8qm/qxp, power domain done by scfw (drivers/firmware/imx/scu-pd.c) > over mailbox: > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > imx_scu_ipc_write()->mbox_send_message() > > which means have to 'waits for completion', meanwhile, some driver > > like tty will call dmaengine interfaces in non-atomic case as below, > > > > static int uart_write(struct tty_struct *tty, const unsigned char > > *buf, int count) { > > ....... > > port = uart_port_lock(state, flags); > > ...... > > __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg... > > uart_port_unlock(port, flags); > > return ret; > > } > > > > Thus dma runtime resume may happen in that timing window and cause > kernel alarm. > > I'm not sure whether there are similar limitations on other driver > > subsystem. But for me, It looks like the only way to resolve the > > contradiction between tty and scu-pd (hardware limitation on > > i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync > only in device_alloc_chan_resources because request channel is a safe > non-atomic phase. > > Do you have any idea? Thanks in advance. > > If you look closely at the driver you used as an example (hidma.c) it looks like > there is already something in there, which looks very much like what you need > here: > > In hidma_issue_pending() the driver tries to get the device to runtime resume. > If this doesn't work, maybe due to the power domain code not being able to > be called in atomic context, the actual work of waking up the dma hardware > and issuing the descriptor is shunted to a tasklet. > > If I'm reading this right, this is exactly what you need here to be able to call the > dmaengine code from atomic context: try the rpm get and issue immediately > when possible, otherwise shunt the work to a non- atomic context where you > can deal with the requirements of scu-pd. Yes, I can schedule_work to worker to runtime resume edma channel by calling scu-pd. But that means all dmaengine interfaces should be taken care, not only issue_pending() but also dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ dmaengine_tx_status(). Not sure why hidma only take care issue_pending. Maybe their user case is just for memcpy/memset so that no further complicate case as ALSA or TTY. Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync into interrupt handler as qcom/bam_dma.c. but how could resolve the scu-pd's non-atmoic limitation in interrupt handler? > > Also you don't need the runtime resume in all of the functions you mentioned. > From a quick look into the edma driver it looks like for example the > prep_slave_dma() function only touches data structures in memory, so you > don't actually need the device to be awake at that point. Only later in the flow > when you write registers in the dma hardware and actually issue the transfer > you need to wake the device from sleep. Yes, don't need add pm_runtime_get_sync into prep_slave_dma, only care where HW indeed touched like issue_pending()/terminated_all()/pause()/resume..etc.
Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong: > Hi Lucas, > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Robin, > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > > > Hi > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > <l.stach@pengutronix.de> > > > > wrote: > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > > > There are two requirements that we need to move the request of > > > > > > > dma channel from probe to open. > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel > > > > > > request if you don't do it in probe? > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel instead > > > > > of dmaengine_pcm_request_chan_of. so there should be not > > > > > -EPROBE_DEFER return code. > > > > > > > > This is a pretty weak argument. The dmaengine device might probe > > > > after you try to get the channel. Using a function to request the > > > > channel that doesn't allow you to handle probe deferral is IMHO a > > > > bug and should be fixed, instead of building even more assumptions on top > > of it. > > > > > > > > > > > - When dma device binds with power-domains, the power will be > > > > > > > enabled when we request dma channel. If the request of dma > > > > > > > channel happen on probe, then the power-domains will be always > > > > > > > enabled after kernel boot up, which is not good for power > > > > > > > saving, so we need to move the request of dma channel to > > > > > > > .open(); > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > dmaengine driver. > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > device_alloc_chan_resources, the device_alloc_chan_resources is > > > > > called when channel is requested. so power is enabled on channel > > request. > > > > > > > > So why can't you fix the dmaengine driver to do that RPM call at a > > > > later time when the channel is actually going to be used? This will > > > > allow further power savings with other slave devices than the audio PCM. > > > Hi Lucas, > > > Thanks for your suggestion. I have tried to implement runtime > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) > > > for this feature as below (or you can refer to > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > pm_runtime_put_autosuspend in all dmaengine driver interface like > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cycli > > > c/ > > > device_tx_status... > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->dev); > > > pm_runtime_set_autosuspend_delay(fsl_chan->dev, > > 2000); > > > > > > That could resolve this audio case since the autosuspend could suspend > > > runtime after > > > 2 seconds if there is no further dma transfer but only channel > > request(device_alloc_chan_resources). > > > But unfortunately, it cause another issue. As you know, on our > > > i.mx8qm/qxp, power domain done by scfw (drivers/firmware/imx/scu-pd.c) > > over mailbox: > > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > > imx_scu_ipc_write()->mbox_send_message() > > > which means have to 'waits for completion', meanwhile, some driver > > > like tty will call dmaengine interfaces in non-atomic case as below, > > > > > > static int uart_write(struct tty_struct *tty, const unsigned char > > > *buf, int count) { > > > ....... > > > port = uart_port_lock(state, flags); > > > ...... > > > __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg... > > > uart_port_unlock(port, flags); > > > return ret; > > > } > > > > > > Thus dma runtime resume may happen in that timing window and cause > > kernel alarm. > > > I'm not sure whether there are similar limitations on other driver > > > subsystem. But for me, It looks like the only way to resolve the > > > contradiction between tty and scu-pd (hardware limitation on > > > i.mx8qm/qxp) is to give up autosuspend and keep pm_runtime_get_sync > > only in device_alloc_chan_resources because request channel is a safe > > non-atomic phase. > > > Do you have any idea? Thanks in advance. > > > > If you look closely at the driver you used as an example (hidma.c) it looks like > > there is already something in there, which looks very much like what you need > > here: > > > > In hidma_issue_pending() the driver tries to get the device to runtime resume. > > If this doesn't work, maybe due to the power domain code not being able to > > be called in atomic context, the actual work of waking up the dma hardware > > and issuing the descriptor is shunted to a tasklet. > > > > If I'm reading this right, this is exactly what you need here to be able to call the > > dmaengine code from atomic context: try the rpm get and issue immediately > > when possible, otherwise shunt the work to a non- atomic context where you > > can deal with the requirements of scu-pd. > Yes, I can schedule_work to worker to runtime resume edma channel by calling scu-pd. > But that means all dmaengine interfaces should be taken care, not only issue_pending() > but also dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ > dmaengine_tx_status(). Not sure why hidma only take care issue_pending. Maybe > their user case is just for memcpy/memset so that no further complicate case as ALSA > or TTY. > Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync into interrupt > handler as qcom/bam_dma.c. but how could resolve the scu-pd's non-atmoic limitation > in interrupt handler? Sure, this all needs some careful analysis on how those functions are called and what to do about atomic callers, but it should be doable. I don't see any fundamental issues here. I don't see why you would ever need to wake the hardware in an interrupt handler. Surely the hardware is already awake, as it wouldn't signal an interrupt otherwise. And for the issue with scu-pd you only care about the state transition of suspended->running. If the hardware is already running/awake, the runtime pm state handling is nothing more than bumping a refcount, which is atomic safe. Putting the HW in suspend is already handled asynchronously in a worker, so this is also atomic safe. Regards, Lucas
On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote: > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong: > > Hi Lucas, > > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > > > Hi Robin, > > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > > > > Hi > > > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > > <l.stach@pengutronix.de> > > > > > wrote: > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > > > > There are two requirements that we need to move the > > > > > > > > request of dma channel from probe to open. > > > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel > > > > > > > request if you don't do it in probe? > > > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel > > > > > > instead of dmaengine_pcm_request_chan_of. so there should be > > > > > > not -EPROBE_DEFER return code. > > > > > > > > > > This is a pretty weak argument. The dmaengine device might probe > > > > > after you try to get the channel. Using a function to request > > > > > the channel that doesn't allow you to handle probe deferral is > > > > > IMHO a bug and should be fixed, instead of building even more > > > > > assumptions on top > > > of it. > > > > > > > > > > > > > - When dma device binds with power-domains, the power will > > > > > > > > be enabled when we request dma channel. If the request of > > > > > > > > dma channel happen on probe, then the power-domains will > > > > > > > > be always enabled after kernel boot up, which is not good > > > > > > > > for power saving, so we need to move the request of dma > > > > > > > > channel to .open(); > > > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > > dmaengine driver. > > > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > > device_alloc_chan_resources, the device_alloc_chan_resources > > > > > > is called when channel is requested. so power is enabled on > > > > > > channel > > > request. > > > > > > > > > > So why can't you fix the dmaengine driver to do that RPM call at > > > > > a later time when the channel is actually going to be used? This > > > > > will allow further power savings with other slave devices than the audio > PCM. > > > > Hi Lucas, > > > > Thanks for your suggestion. I have tried to implement runtime > > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 > > > > sec) for this feature as below (or you can refer to > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > > pm_runtime_put_autosuspend in all dmaengine driver interface like > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_c > > > > ycli > > > > c/ > > > > device_tx_status... > > > > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->dev); > > > > pm_runtime_set_autosuspend_delay(fsl_chan-> > dev, > > > 2000); > > > > > > > > That could resolve this audio case since the autosuspend could > > > > suspend runtime after > > > > 2 seconds if there is no further dma transfer but only channel > > > request(device_alloc_chan_resources). > > > > But unfortunately, it cause another issue. As you know, on our > > > > i.mx8qm/qxp, power domain done by scfw > > > > (drivers/firmware/imx/scu-pd.c) > > > over mailbox: > > > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > > > imx_scu_ipc_write()->mbox_send_message() > > > > which means have to 'waits for completion', meanwhile, some driver > > > > like tty will call dmaengine interfaces in non-atomic case as > > > > below, > > > > > > > > static int uart_write(struct tty_struct *tty, const unsigned char > > > > *buf, int count) { > > > > ....... > > > > port = uart_port_lock(state, flags); > > > > ...... > > > > __uart_start(tty); //call > start_tx()->dmaengine_prep_slave_sg... > > > > uart_port_unlock(port, flags); > > > > return ret; > > > > } > > > > > > > > Thus dma runtime resume may happen in that timing window and cause > > > kernel alarm. > > > > I'm not sure whether there are similar limitations on other driver > > > > subsystem. But for me, It looks like the only way to resolve the > > > > contradiction between tty and scu-pd (hardware limitation on > > > > i.mx8qm/qxp) is to give up autosuspend and keep > > > > pm_runtime_get_sync > > > only in device_alloc_chan_resources because request channel is a > > > safe non-atomic phase. > > > > Do you have any idea? Thanks in advance. > > > > > > If you look closely at the driver you used as an example (hidma.c) > > > it looks like there is already something in there, which looks very > > > much like what you need > > > here: > > > > > > In hidma_issue_pending() the driver tries to get the device to runtime > resume. > > > If this doesn't work, maybe due to the power domain code not being > > > able to be called in atomic context, the actual work of waking up > > > the dma hardware and issuing the descriptor is shunted to a tasklet. > > > > > > If I'm reading this right, this is exactly what you need here to be > > > able to call the dmaengine code from atomic context: try the rpm get > > > and issue immediately when possible, otherwise shunt the work to a > > > non- atomic context where you can deal with the requirements of scu-pd. > > Yes, I can schedule_work to worker to runtime resume edma channel by > calling scu-pd. > > But that means all dmaengine interfaces should be taken care, not only > > issue_pending() but also > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ > > dmaengine_tx_status(). Not sure why hidma only take care > > issue_pending. Maybe their user case is just for memcpy/memset so that > > no further complicate case as ALSA or TTY. > > Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync > > into interrupt handler as qcom/bam_dma.c. but how could resolve the > > scu-pd's non-atmoic limitation in interrupt handler? > > Sure, this all needs some careful analysis on how those functions are called > and what to do about atomic callers, but it should be doable. I don't see any > fundamental issues here. > > I don't see why you would ever need to wake the hardware in an interrupt > handler. Surely the hardware is already awake, as it wouldn't signal an > interrupt otherwise. And for the issue with scu-pd you only care about the > state transition of suspended->running. If the hardware is already > running/awake, the runtime pm state handling is nothing more than bumping > a refcount, which is atomic safe. Putting the HW in suspend is already handled > asynchronously in a worker, so this is also atomic safe. But with autosuspend used, in corner case, may runtime suspended before falling Into edma interrupt handler if timeout happen with the delay value of pm_runtime_set_autosuspend_delay(). Thus, can't touch any edma interrupt status register unless runtime resume edma in interrupt handler while runtime resume function based on scu-pd's power domain may block or sleep. I have a simple workaround that disable runtime suspend in issue_pending worker by calling pm_runtime_forbid() and then enable runtime auto suspend in dmaengine_terminate_all so that we could easily regard that edma channel is always in runtime resume between issue_pending and channel terminated and ignore the above interrupt handler/scu-pd limitation.
Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong: > On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong: > > > Hi Lucas, > > > > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Robin, > > > > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > > > > > Hi > > > > > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > > > <l.stach@pengutronix.de> > > > > > > wrote: > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > > > > > There are two requirements that we need to move the > > > > > > > > > request of dma channel from probe to open. > > > > > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel > > > > > > > > request if you don't do it in probe? > > > > > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should be > > > > > > > not -EPROBE_DEFER return code. > > > > > > > > > > > > This is a pretty weak argument. The dmaengine device might probe > > > > > > after you try to get the channel. Using a function to request > > > > > > the channel that doesn't allow you to handle probe deferral is > > > > > > IMHO a bug and should be fixed, instead of building even more > > > > > > assumptions on top > > > > of it. > > > > > > > > > > > > > > > - When dma device binds with power-domains, the power will > > > > > > > > > be enabled when we request dma channel. If the request of > > > > > > > > > dma channel happen on probe, then the power-domains will > > > > > > > > > be always enabled after kernel boot up, which is not good > > > > > > > > > for power saving, so we need to move the request of dma > > > > > > > > > channel to .open(); > > > > > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > > > dmaengine driver. > > > > > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > > > device_alloc_chan_resources, the device_alloc_chan_resources > > > > > > > is called when channel is requested. so power is enabled on > > > > > > > channel > > > > request. > > > > > > > > > > > > So why can't you fix the dmaengine driver to do that RPM call at > > > > > > a later time when the channel is actually going to be used? This > > > > > > will allow further power savings with other slave devices than the audio > > PCM. > > > > > Hi Lucas, > > > > > Thanks for your suggestion. I have tried to implement runtime > > > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 > > > > > sec) for this feature as below (or you can refer to > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface like > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_c > > > > > ycli > > > > > c/ > > > > > device_tx_status... > > > > > > > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->dev); > > > > > pm_runtime_set_autosuspend_delay(fsl_chan-> > > dev, > > > > 2000); > > > > > > > > > > That could resolve this audio case since the autosuspend could > > > > > suspend runtime after > > > > > 2 seconds if there is no further dma transfer but only channel > > > > request(device_alloc_chan_resources). > > > > > But unfortunately, it cause another issue. As you know, on our > > > > > i.mx8qm/qxp, power domain done by scfw > > > > > (drivers/firmware/imx/scu-pd.c) > > > > over mailbox: > > > > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > > > > imx_scu_ipc_write()->mbox_send_message() > > > > > which means have to 'waits for completion', meanwhile, some driver > > > > > like tty will call dmaengine interfaces in non-atomic case as > > > > > below, > > > > > > > > > > static int uart_write(struct tty_struct *tty, const unsigned char > > > > > *buf, int count) { > > > > > ....... > > > > > port = uart_port_lock(state, flags); > > > > > ...... > > > > > __uart_start(tty); //call > > start_tx()->dmaengine_prep_slave_sg... > > > > > uart_port_unlock(port, flags); > > > > > return ret; > > > > > } > > > > > > > > > > Thus dma runtime resume may happen in that timing window and cause > > > > kernel alarm. > > > > > I'm not sure whether there are similar limitations on other driver > > > > > subsystem. But for me, It looks like the only way to resolve the > > > > > contradiction between tty and scu-pd (hardware limitation on > > > > > i.mx8qm/qxp) is to give up autosuspend and keep > > > > > pm_runtime_get_sync > > > > only in device_alloc_chan_resources because request channel is a > > > > safe non-atomic phase. > > > > > Do you have any idea? Thanks in advance. > > > > > > > > If you look closely at the driver you used as an example (hidma.c) > > > > it looks like there is already something in there, which looks very > > > > much like what you need > > > > here: > > > > > > > > In hidma_issue_pending() the driver tries to get the device to runtime > > resume. > > > > If this doesn't work, maybe due to the power domain code not being > > > > able to be called in atomic context, the actual work of waking up > > > > the dma hardware and issuing the descriptor is shunted to a tasklet. > > > > > > > > If I'm reading this right, this is exactly what you need here to be > > > > able to call the dmaengine code from atomic context: try the rpm get > > > > and issue immediately when possible, otherwise shunt the work to a > > > > non- atomic context where you can deal with the requirements of scu-pd. > > > Yes, I can schedule_work to worker to runtime resume edma channel by > > calling scu-pd. > > > But that means all dmaengine interfaces should be taken care, not only > > > issue_pending() but also > > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ > > > dmaengine_tx_status(). Not sure why hidma only take care > > > issue_pending. Maybe their user case is just for memcpy/memset so that > > > no further complicate case as ALSA or TTY. > > > Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync > > > into interrupt handler as qcom/bam_dma.c. but how could resolve the > > > scu-pd's non-atmoic limitation in interrupt handler? > > > > Sure, this all needs some careful analysis on how those functions are called > > and what to do about atomic callers, but it should be doable. I don't see any > > fundamental issues here. > > > > I don't see why you would ever need to wake the hardware in an interrupt > > handler. Surely the hardware is already awake, as it wouldn't signal an > > interrupt otherwise. And for the issue with scu-pd you only care about the > > state transition of suspended->running. If the hardware is already > > running/awake, the runtime pm state handling is nothing more than bumping > > a refcount, which is atomic safe. Putting the HW in suspend is already handled > > asynchronously in a worker, so this is also atomic safe. > But with autosuspend used, in corner case, may runtime suspended before falling > Into edma interrupt handler if timeout happen with the delay value of > pm_runtime_set_autosuspend_delay(). Thus, can't touch any edma interrupt > status register unless runtime resume edma in interrupt handler while runtime > resume function based on scu-pd's power domain may block or sleep. > I have a simple workaround that disable runtime suspend in issue_pending worker > by calling pm_runtime_forbid() and then enable runtime auto suspend in > dmaengine_terminate_all so that we could easily regard that edma channel is always > in runtime resume between issue_pending and channel terminated and ignore the above > interrupt handler/scu-pd limitation. The IRQ handler is the point where you are informed by the hardware that a specific operation is complete. I don't see any use-case where it would be valid to drop the rpm refcount to 0 before the IRQ is handled. Surely the hardware needs to stay awake until the currently queued operations are complete and if the IRQ handler is the completion point the IRQ handler is the first point in time where your autosuspend timer should start to run. There should never be a situation where the timer expiry can get between IRQ signaling and the handler code running. Regards, Lucas
On 20201/04/20 22:01 Lucas Stach <l.stach@pengutronix.de> wrote: > Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong: > > On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote: > > > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong: > > > > Hi Lucas, > > > > > > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > Hi Robin, > > > > > > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu > Wang: > > > > > > > > Hi > > > > > > > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > > > > <l.stach@pengutronix.de> > > > > > > > wrote: > > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu > Wang: > > > > > > > > > > There are two requirements that we need to move the > > > > > > > > > > request of dma channel from probe to open. > > > > > > > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the > > > > > > > > > channel request if you don't do it in probe? > > > > > > > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel > > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should > > > > > > > > be not -EPROBE_DEFER return code. > > > > > > > > > > > > > > This is a pretty weak argument. The dmaengine device might > > > > > > > probe after you try to get the channel. Using a function to > > > > > > > request the channel that doesn't allow you to handle probe > > > > > > > deferral is IMHO a bug and should be fixed, instead of > > > > > > > building even more assumptions on top > > > > > of it. > > > > > > > > > > > > > > > > > - When dma device binds with power-domains, the power > > > > > > > > > > will be enabled when we request dma channel. If the > > > > > > > > > > request of dma channel happen on probe, then the > > > > > > > > > > power-domains will be always enabled after kernel boot > > > > > > > > > > up, which is not good for power saving, so we need > > > > > > > > > > to move the request of dma channel to .open(); > > > > > > > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > > > > dmaengine driver. > > > > > > > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > > > > device_alloc_chan_resources, the > > > > > > > > device_alloc_chan_resources is called when channel is > > > > > > > > requested. so power is enabled on channel > > > > > request. > > > > > > > > > > > > > > So why can't you fix the dmaengine driver to do that RPM > > > > > > > call at a later time when the channel is actually going to > > > > > > > be used? This will allow further power savings with other > > > > > > > slave devices than the audio > > > PCM. > > > > > > Hi Lucas, > > > > > > Thanks for your suggestion. I have tried to implement > > > > > > runtime autosuspend in fsl-edma driver on i.mx8qm/qxp with > > > > > > delay time (2 > > > > > > sec) for this feature as below (or you can refer to > > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface > > > > > > like > > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_d > > > > > > ma_c > > > > > > ycli > > > > > > c/ > > > > > > device_tx_status... > > > > > > > > > > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->de > v); > > > > > > pm_runtime_set_autosuspend_delay(fsl_cha > n-> > > > dev, > > > > > 2000); > > > > > > > > > > > > That could resolve this audio case since the autosuspend could > > > > > > suspend runtime after > > > > > > 2 seconds if there is no further dma transfer but only channel > > > > > request(device_alloc_chan_resources). > > > > > > But unfortunately, it cause another issue. As you know, on our > > > > > > i.mx8qm/qxp, power domain done by scfw > > > > > > (drivers/firmware/imx/scu-pd.c) > > > > > over mailbox: > > > > > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > > > > > imx_scu_ipc_write()->mbox_send_message() > > > > > > which means have to 'waits for completion', meanwhile, some > > > > > > driver like tty will call dmaengine interfaces in non-atomic > > > > > > case as below, > > > > > > > > > > > > static int uart_write(struct tty_struct *tty, const unsigned > > > > > > char *buf, int count) { > > > > > > ....... > > > > > > port = uart_port_lock(state, flags); > > > > > > ...... > > > > > > __uart_start(tty); //call > > > start_tx()->dmaengine_prep_slave_sg... > > > > > > uart_port_unlock(port, flags); > > > > > > return ret; > > > > > > } > > > > > > > > > > > > Thus dma runtime resume may happen in that timing window and > > > > > > cause > > > > > kernel alarm. > > > > > > I'm not sure whether there are similar limitations on other > > > > > > driver subsystem. But for me, It looks like the only way to > > > > > > resolve the contradiction between tty and scu-pd (hardware > > > > > > limitation on > > > > > > i.mx8qm/qxp) is to give up autosuspend and keep > > > > > > pm_runtime_get_sync > > > > > only in device_alloc_chan_resources because request channel is a > > > > > safe non-atomic phase. > > > > > > Do you have any idea? Thanks in advance. > > > > > > > > > > If you look closely at the driver you used as an example > > > > > (hidma.c) it looks like there is already something in there, > > > > > which looks very much like what you need > > > > > here: > > > > > > > > > > In hidma_issue_pending() the driver tries to get the device to > > > > > runtime > > > resume. > > > > > If this doesn't work, maybe due to the power domain code not > > > > > being able to be called in atomic context, the actual work of > > > > > waking up the dma hardware and issuing the descriptor is shunted to a > tasklet. > > > > > > > > > > If I'm reading this right, this is exactly what you need here to > > > > > be able to call the dmaengine code from atomic context: try the > > > > > rpm get and issue immediately when possible, otherwise shunt the > > > > > work to a > > > > > non- atomic context where you can deal with the requirements of > scu-pd. > > > > Yes, I can schedule_work to worker to runtime resume edma channel > > > > by > > > calling scu-pd. > > > > But that means all dmaengine interfaces should be taken care, not > > > > only > > > > issue_pending() but also > > > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ > > > > dmaengine_tx_status(). Not sure why hidma only take care > > > > issue_pending. Maybe their user case is just for memcpy/memset so > > > > that no further complicate case as ALSA or TTY. > > > > Besides, for autosuspend in cyclic, we have to add > > > > pm_runtime_get_sync into interrupt handler as qcom/bam_dma.c. but > > > > how could resolve the scu-pd's non-atmoic limitation in interrupt > handler? > > > > > > Sure, this all needs some careful analysis on how those functions > > > are called and what to do about atomic callers, but it should be > > > doable. I don't see any fundamental issues here. > > > > > > I don't see why you would ever need to wake the hardware in an > > > interrupt handler. Surely the hardware is already awake, as it > > > wouldn't signal an interrupt otherwise. And for the issue with > > > scu-pd you only care about the state transition of > > > suspended->running. If the hardware is already running/awake, the > > > runtime pm state handling is nothing more than bumping a refcount, > > > which is atomic safe. Putting the HW in suspend is already handled > asynchronously in a worker, so this is also atomic safe. > > But with autosuspend used, in corner case, may runtime suspended > > before falling Into edma interrupt handler if timeout happen with the > > delay value of pm_runtime_set_autosuspend_delay(). Thus, can't touch > > any edma interrupt status register unless runtime resume edma in > > interrupt handler while runtime resume function based on scu-pd's power > domain may block or sleep. > > I have a simple workaround that disable runtime suspend in > > issue_pending worker by calling pm_runtime_forbid() and then enable > > runtime auto suspend in dmaengine_terminate_all so that we could > > easily regard that edma channel is always in runtime resume between > > issue_pending and channel terminated and ignore the above interrupt > handler/scu-pd limitation. > > The IRQ handler is the point where you are informed by the hardware that a > specific operation is complete. I don't see any use-case where it would be valid > to drop the rpm refcount to 0 before the IRQ is handled. Surely the hardware > needs to stay awake until the currently queued operations are complete and if > the IRQ handler is the completion point the IRQ handler is the first point in > time where your autosuspend timer should start to run. There should never be > a situation where the timer expiry can get between IRQ signaling and the > handler code running. But the timer of runtime_auto_suspend decide when enter runtime suspend rather than hardware, while transfer data size and transfer rate on IP bus decide when the dma interrupt happen. Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/ pm_runtime_mark_last_busy in interrupt handler to hope the runtime_auto_suspend timer expiry later than interrupt coming, but if the transfer data size is larger in cyclic and transfer rate is very slow like 115200 or lower on uart, the fix autosuspend timer 100ms/200ms maybe not enough, hence, runtime suspend may execute meanwhile the dma interrupt maybe triggered and caught by GIC(but interrupt handler prevent by spin_lock_irqsave in pm_suspend_timer_fn() ), and then interrupt handler start to run after runtime suspend.
Am Mittwoch, dem 21.04.2021 um 14:54 +0000 schrieb Robin Gong: > On 20201/04/20 22:01 Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong: > > > On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong: > > > > > Hi Lucas, > > > > > > > > > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Robin, > > > > > > > > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong: > > > > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu > > Wang: > > > > > > > > > Hi > > > > > > > > > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > > > > > <l.stach@pengutronix.de> > > > > > > > > wrote: > > > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu > > Wang: > > > > > > > > > > > There are two requirements that we need to move the > > > > > > > > > > > request of dma channel from probe to open. > > > > > > > > > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the > > > > > > > > > > channel request if you don't do it in probe? > > > > > > > > > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel > > > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should > > > > > > > > > be not -EPROBE_DEFER return code. > > > > > > > > > > > > > > > > This is a pretty weak argument. The dmaengine device might > > > > > > > > probe after you try to get the channel. Using a function to > > > > > > > > request the channel that doesn't allow you to handle probe > > > > > > > > deferral is IMHO a bug and should be fixed, instead of > > > > > > > > building even more assumptions on top > > > > > > of it. > > > > > > > > > > > > > > > > > > > - When dma device binds with power-domains, the power > > > > > > > > > > > will be enabled when we request dma channel. If the > > > > > > > > > > > request of dma channel happen on probe, then the > > > > > > > > > > > power-domains will be always enabled after kernel boot > > > > > > > > > > > up, which is not good for power saving, so we need > > > > > > > > > > > to move the request of dma channel to .open(); > > > > > > > > > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > > > > > dmaengine driver. > > > > > > > > > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > > > > > device_alloc_chan_resources, the > > > > > > > > > device_alloc_chan_resources is called when channel is > > > > > > > > > requested. so power is enabled on channel > > > > > > request. > > > > > > > > > > > > > > > > So why can't you fix the dmaengine driver to do that RPM > > > > > > > > call at a later time when the channel is actually going to > > > > > > > > be used? This will allow further power savings with other > > > > > > > > slave devices than the audio > > > > PCM. > > > > > > > Hi Lucas, > > > > > > > Thanks for your suggestion. I have tried to implement > > > > > > > runtime autosuspend in fsl-edma driver on i.mx8qm/qxp with > > > > > > > delay time (2 > > > > > > > sec) for this feature as below (or you can refer to > > > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface > > > > > > > like > > > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_d > > > > > > > ma_c > > > > > > > ycli > > > > > > > c/ > > > > > > > device_tx_status... > > > > > > > > > > > > > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->de > > v); > > > > > > > pm_runtime_set_autosuspend_delay(fsl_cha > > n-> > > > > dev, > > > > > > 2000); > > > > > > > > > > > > > > That could resolve this audio case since the autosuspend could > > > > > > > suspend runtime after > > > > > > > 2 seconds if there is no further dma transfer but only channel > > > > > > request(device_alloc_chan_resources). > > > > > > > But unfortunately, it cause another issue. As you know, on our > > > > > > > i.mx8qm/qxp, power domain done by scfw > > > > > > > (drivers/firmware/imx/scu-pd.c) > > > > > > over mailbox: > > > > > > > imx_sc_pd_power()->imx_scu_call_rpc()-> > > > > > > > imx_scu_ipc_write()->mbox_send_message() > > > > > > > which means have to 'waits for completion', meanwhile, some > > > > > > > driver like tty will call dmaengine interfaces in non-atomic > > > > > > > case as below, > > > > > > > > > > > > > > static int uart_write(struct tty_struct *tty, const unsigned > > > > > > > char *buf, int count) { > > > > > > > ....... > > > > > > > port = uart_port_lock(state, flags); > > > > > > > ...... > > > > > > > __uart_start(tty); //call > > > > start_tx()->dmaengine_prep_slave_sg... > > > > > > > uart_port_unlock(port, flags); > > > > > > > return ret; > > > > > > > } > > > > > > > > > > > > > > Thus dma runtime resume may happen in that timing window and > > > > > > > cause > > > > > > kernel alarm. > > > > > > > I'm not sure whether there are similar limitations on other > > > > > > > driver subsystem. But for me, It looks like the only way to > > > > > > > resolve the contradiction between tty and scu-pd (hardware > > > > > > > limitation on > > > > > > > i.mx8qm/qxp) is to give up autosuspend and keep > > > > > > > pm_runtime_get_sync > > > > > > only in device_alloc_chan_resources because request channel is a > > > > > > safe non-atomic phase. > > > > > > > Do you have any idea? Thanks in advance. > > > > > > > > > > > > If you look closely at the driver you used as an example > > > > > > (hidma.c) it looks like there is already something in there, > > > > > > which looks very much like what you need > > > > > > here: > > > > > > > > > > > > In hidma_issue_pending() the driver tries to get the device to > > > > > > runtime > > > > resume. > > > > > > If this doesn't work, maybe due to the power domain code not > > > > > > being able to be called in atomic context, the actual work of > > > > > > waking up the dma hardware and issuing the descriptor is shunted to a > > tasklet. > > > > > > > > > > > > If I'm reading this right, this is exactly what you need here to > > > > > > be able to call the dmaengine code from atomic context: try the > > > > > > rpm get and issue immediately when possible, otherwise shunt the > > > > > > work to a > > > > > > non- atomic context where you can deal with the requirements of > > scu-pd. > > > > > Yes, I can schedule_work to worker to runtime resume edma channel > > > > > by > > > > calling scu-pd. > > > > > But that means all dmaengine interfaces should be taken care, not > > > > > only > > > > > issue_pending() but also > > > > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/ > > > > > dmaengine_tx_status(). Not sure why hidma only take care > > > > > issue_pending. Maybe their user case is just for memcpy/memset so > > > > > that no further complicate case as ALSA or TTY. > > > > > Besides, for autosuspend in cyclic, we have to add > > > > > pm_runtime_get_sync into interrupt handler as qcom/bam_dma.c. but > > > > > how could resolve the scu-pd's non-atmoic limitation in interrupt > > handler? > > > > > > > > Sure, this all needs some careful analysis on how those functions > > > > are called and what to do about atomic callers, but it should be > > > > doable. I don't see any fundamental issues here. > > > > > > > > I don't see why you would ever need to wake the hardware in an > > > > interrupt handler. Surely the hardware is already awake, as it > > > > wouldn't signal an interrupt otherwise. And for the issue with > > > > scu-pd you only care about the state transition of > > > > suspended->running. If the hardware is already running/awake, the > > > > runtime pm state handling is nothing more than bumping a refcount, > > > > which is atomic safe. Putting the HW in suspend is already handled > > asynchronously in a worker, so this is also atomic safe. > > > But with autosuspend used, in corner case, may runtime suspended > > > before falling Into edma interrupt handler if timeout happen with the > > > delay value of pm_runtime_set_autosuspend_delay(). Thus, can't touch > > > any edma interrupt status register unless runtime resume edma in > > > interrupt handler while runtime resume function based on scu-pd's power > > domain may block or sleep. > > > I have a simple workaround that disable runtime suspend in > > > issue_pending worker by calling pm_runtime_forbid() and then enable > > > runtime auto suspend in dmaengine_terminate_all so that we could > > > easily regard that edma channel is always in runtime resume between > > > issue_pending and channel terminated and ignore the above interrupt > > handler/scu-pd limitation. > > > > The IRQ handler is the point where you are informed by the hardware that a > > specific operation is complete. I don't see any use-case where it would be valid > > to drop the rpm refcount to 0 before the IRQ is handled. Surely the hardware > > needs to stay awake until the currently queued operations are complete and if > > the IRQ handler is the completion point the IRQ handler is the first point in > > time where your autosuspend timer should start to run. There should never be > > a situation where the timer expiry can get between IRQ signaling and the > > handler code running. > But the timer of runtime_auto_suspend decide when enter runtime suspend rather > than hardware, while transfer data size and transfer rate on IP bus decide when the > dma interrupt happen. > But it isn't the hardware that decides to drop the rpm refcount to 0 and starting the autosuspend timer, it's the driver. > Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/ > pm_runtime_mark_last_busy in interrupt handler to hope the runtime_auto_suspend > timer expiry later than interrupt coming, but if the transfer data size is larger in cyclic > and transfer rate is very slow like 115200 or lower on uart, the fix autosuspend timer > 100ms/200ms maybe not enough, hence, runtime suspend may execute meanwhile > the dma interrupt maybe triggered and caught by GIC(but interrupt handler prevent > by spin_lock_irqsave in pm_suspend_timer_fn() ), and then interrupt handler start > to run after runtime suspend. If your driver code drops the rpm refcount to 0 and starts the autosuspend timer while a cyclic transfer is still in flight this is clearly a bug. Autosuspend is not there to paper over driver bugs, but to amortize cost of actually suspending and resuming the hardware. Your driver code must still work even if the timeout is 0, i.e. the hardware is immediately suspended after you drop the rpm refcount to 0. If you still have transfers queued/in-flight the driver code must keep a rpm reference. Regards, Lucas
On Wed, Apr 21, 2021 at 07:43:18PM +0200, Lucas Stach wrote: > If your driver code drops the rpm refcount to 0 and starts the > autosuspend timer while a cyclic transfer is still in flight this is > clearly a bug. Autosuspend is not there to paper over driver bugs, but > to amortize cost of actually suspending and resuming the hardware. Your > driver code must still work even if the timeout is 0, i.e. the hardware > is immediately suspended after you drop the rpm refcount to 0. > If you still have transfers queued/in-flight the driver code must keep > a rpm reference. Right, failing to do that is a clear bug. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material.
On 2021/04/22 Lucas Stach <l.stach@pengutronix.de> wrote: > > But the timer of runtime_auto_suspend decide when enter runtime > > suspend rather than hardware, while transfer data size and transfer > > rate on IP bus decide when the dma interrupt happen. > > > But it isn't the hardware that decides to drop the rpm refcount to 0 and > starting the autosuspend timer, it's the driver. Yes, driver should keep rpm refcount never to 0 in such case. But I think that case Is a common case in dma cyclic with runtime_auto_suspend, so some dma driver also add pm_runtime_get_sync/ pm_runtime_put_autosuspend in interrupt handler like qcom/bam_dma.c for safe rather than only pm_runtime_mark_last_busy(). > > > Generally, we can call pm_runtime_get_sync(fsl_chan->dev)/ > > pm_runtime_mark_last_busy in interrupt handler to hope the > > runtime_auto_suspend timer expiry later than interrupt coming, but if > > the transfer data size is larger in cyclic and transfer rate is very > > slow like 115200 or lower on uart, the fix autosuspend timer > > 100ms/200ms maybe not enough, hence, runtime suspend may execute > > meanwhile the dma interrupt maybe triggered and caught by GIC(but > > interrupt handler prevent by spin_lock_irqsave in pm_suspend_timer_fn() ), > and then interrupt handler start to run after runtime suspend. > > If your driver code drops the rpm refcount to 0 and starts the autosuspend > timer while a cyclic transfer is still in flight this is clearly a bug. Autosuspend is > not there to paper over driver bugs, but to amortize cost of actually > suspending and resuming the hardware. Your driver code must still work even > if the timeout is 0, i.e. the hardware is immediately suspended after you drop > the rpm refcount to 0. > > If you still have transfers queued/in-flight the driver code must keep a rpm > reference. Yes, that's what I said for fix before as below. 'I have a simple workaround that disable runtime suspend in issue_pending worker by calling pm_runtime_forbid() and then enable runtime auto suspend in dmaengine_terminate_all so that we could easily regard that edma channel is always in runtime resume between issue_pending and channel terminated and ignore the above interrupt handler/scu-pd limitation'
diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c index 04a9bc749016..dae53b384df4 100644 --- a/sound/soc/fsl/imx-pcm-dma.c +++ b/sound/soc/fsl/imx-pcm-dma.c @@ -11,6 +11,7 @@ #include <linux/dmaengine.h> #include <linux/types.h> #include <linux/module.h> +#include <linux/dma-mapping.h> #include <sound/core.h> #include <sound/pcm.h> @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param) return true; } -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = { - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config, - .compat_filter_fn = filter, -}; +static int imx_pcm_hw_params(struct snd_soc_component *component, + struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_dmaengine_dai_dma_data *dma_data; + struct dma_slave_config config; + struct dma_chan *chan; + int ret = 0; -int imx_pcm_dma_init(struct platform_device *pdev, size_t size) + snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer); + runtime->dma_bytes = params_buffer_bytes(params); + + chan = snd_dmaengine_pcm_get_chan(substream); + if (!chan) + return -EINVAL; + + ret = snd_hwparams_to_dma_slave_config(substream, params, &config); + if (ret) + return ret; + + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); + if (!dma_data) + return -EINVAL; + + snd_dmaengine_pcm_set_config_from_dai_data(substream, + dma_data, + &config); + return dmaengine_slave_config(chan, &config); +} + +static int imx_pcm_hw_free(struct snd_soc_component *component, + struct snd_pcm_substream *substream) { - struct snd_dmaengine_pcm_config *config; + snd_pcm_set_runtime_buffer(substream, NULL); + return 0; +} + +static snd_pcm_uframes_t imx_pcm_pointer(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + return snd_dmaengine_pcm_pointer(substream); +} + +static int imx_pcm_trigger(struct snd_soc_component *component, + struct snd_pcm_substream *substream, int cmd) +{ + return snd_dmaengine_pcm_trigger(substream, cmd); +} + +static int imx_pcm_open(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0); + struct snd_dmaengine_dai_dma_data *dma_data; + struct device *dev = component->dev; + struct snd_pcm_hardware hw; + struct dma_chan *chan; + int ret; + + ret = snd_pcm_hw_constraint_integer(substream->runtime, + SNDRV_PCM_HW_PARAM_PERIODS); + if (ret < 0) { + dev_err(dev, "failed to set pcm hw params periods\n"); + return ret; + } + + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream); + if (!dma_data) + return -EINVAL; + + chan = dma_request_slave_channel(cpu_dai->dev, tx ? "tx" : "rx"); + if (!chan) { + /* Try to request channel using compat_filter_fn */ + chan = snd_dmaengine_pcm_request_channel(filter, + dma_data->filter_data); + if (!chan) + return -ENXIO; + } - config = devm_kzalloc(&pdev->dev, - sizeof(struct snd_dmaengine_pcm_config), GFP_KERNEL); - if (!config) - return -ENOMEM; - *config = imx_dmaengine_pcm_config; + ret = snd_dmaengine_pcm_open(substream, chan); + if (ret) + goto pcm_open_fail; - return devm_snd_dmaengine_pcm_register(&pdev->dev, - config, - SND_DMAENGINE_PCM_FLAG_COMPAT); + memset(&hw, 0, sizeof(hw)); + hw.info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | + SNDRV_PCM_INFO_INTERLEAVED; + hw.periods_min = 2; + hw.periods_max = UINT_MAX; + hw.period_bytes_min = 256; + hw.period_bytes_max = dma_get_max_seg_size(chan->device->dev); + hw.buffer_bytes_max = IMX_DEFAULT_DMABUF_SIZE; + hw.fifo_size = dma_data->fifo_size; + + /* Refine the hw according to caps of DMA. */ + ret = snd_dmaengine_pcm_refine_runtime_hwparams(substream, + dma_data, + &hw, + chan); + if (ret < 0) + goto refine_runtime_hwparams_fail; + + snd_soc_set_runtime_hwparams(substream, &hw); + + /* Support allocate memory from IRAM */ + ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV_IRAM, + chan->device->dev, + hw.buffer_bytes_max, + &substream->dma_buffer); + if (ret < 0) + goto alloc_pagas_fail; + + return 0; + +alloc_pagas_fail: +refine_runtime_hwparams_fail: + snd_dmaengine_pcm_close(substream); +pcm_open_fail: + dma_release_channel(chan); + + return ret; +} + +static int imx_pcm_close(struct snd_soc_component *component, + struct snd_pcm_substream *substream) +{ + if (substream) { + snd_dma_free_pages(&substream->dma_buffer); + substream->dma_buffer.area = NULL; + substream->dma_buffer.addr = 0; + } + + return snd_dmaengine_pcm_close_release_chan(substream); +} + +static int imx_pcm_new(struct snd_soc_component *component, + struct snd_soc_pcm_runtime *rtd) +{ + struct snd_card *card = rtd->card->snd_card; + + return dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32)); +} + +static const struct snd_soc_component_driver imx_pcm_component = { + .name = "imx-pcm-dma", + .pcm_construct = imx_pcm_new, + .open = imx_pcm_open, + .close = imx_pcm_close, + .hw_params = imx_pcm_hw_params, + .hw_free = imx_pcm_hw_free, + .trigger = imx_pcm_trigger, + .pointer = imx_pcm_pointer, +}; + +int imx_pcm_dma_init(struct platform_device *pdev, size_t size) +{ + return devm_snd_soc_register_component(&pdev->dev, + &imx_pcm_component, NULL, 0); } EXPORT_SYMBOL_GPL(imx_pcm_dma_init);
There are two requirements that we need to move the request of dma channel from probe to open. - When dma device binds with power-domains, the power will be enabled when we request dma channel. If the request of dma channel happen on probe, then the power-domains will be always enabled after kernel boot up, which is not good for power saving, so we need to move the request of dma channel to .open(); - With FE-BE case, if the dma channel is requested in probe, then there will be below issue, which is caused by that the dma channel will be requested duplicately [ 638.906268] sysfs: cannot create duplicate filename '/devices/soc0/soc/2000000.bus/2000000.spba-bus/2024000.esai/dma:tx' [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted 5.7.0-rc1-12956-gfc64b2585593 #287 [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 638.933690] [<c0110dd8>] (unwind_backtrace) from [<c010b8ec>] (show_stack+0x10/0x14) [ 638.941464] [<c010b8ec>] (show_stack) from [<c0557fc0>] (dump_stack+0xe4/0x118) [ 638.948808] [<c0557fc0>] (dump_stack) from [<c032aeb4>] (sysfs_warn_dup+0x50/0x64) [ 638.956406] [<c032aeb4>] (sysfs_warn_dup) from [<c032b1a8>] (sysfs_do_create_link_sd+0xc8/0xd4) [ 638.965134] [<c032b1a8>] (sysfs_do_create_link_sd) from [<c05dc668>] (dma_request_chan+0xb0/0x210) [ 638.974120] [<c05dc668>] (dma_request_chan) from [<c05dc7d0>] (dma_request_slave_channel+0x8/0x14) [ 638.983111] [<c05dc7d0>] (dma_request_slave_channel) from [<c09d5548>] (fsl_asrc_dma_hw_params+0x1e0/0x438) [ 638.992881] [<c09d5548>] (fsl_asrc_dma_hw_params) from [<c09c1654>] (soc_pcm_hw_params+0x4a0/0x6a8) [ 639.001952] [<c09c1654>] (soc_pcm_hw_params) from [<c09c39d4>] (dpcm_fe_dai_hw_params+0x70/0xe4) [ 639.010765] [<c09c39d4>] (dpcm_fe_dai_hw_params) from [<c099b274>] (snd_pcm_hw_params+0x158/0x418) [ 639.019750] [<c099b274>] (snd_pcm_hw_params) from [<c099c5a0>] (snd_pcm_ioctl+0x734/0x183c) [ 639.028129] [<c099c5a0>] (snd_pcm_ioctl) from [<c029ff94>] (ksys_ioctl+0x2ac/0xb98) [ 639.035812] [<c029ff94>] (ksys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28) [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) [ 639.048565] 9fa0: bee84650 01321870 00000004 c25c4111 bee84650 0002000f [ 639.056766] 9fc0: bee84650 01321870 01321820 00000036 00001f40 00000000 0002c2f8 00000003 [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com> --- sound/soc/fsl/imx-pcm-dma.c | 173 +++++++++++++++++++++++++++++++++--- 1 file changed, 159 insertions(+), 14 deletions(-)