Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
@ 2020-05-19  9:41 Shengjiu Wang
  2020-05-19 10:01 ` Lucas Stach
  0 siblings, 1 reply; 8+ messages in thread
From: Shengjiu Wang @ 2020-05-19  9:41 UTC (permalink / raw)
  To: timur, nicoleotsuka, Xiubo.Lee, festevam, lgirdwood, broonie,
	perex, tiwai, shawnguo, s.hauer, kernel, linux-imx, sumit.semwal,
	alsa-devel, linuxppc-dev, linux-arm-kernel, linux-kernel,
	linux-media, dri-devel, linaro-mm-sig

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(-)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-19  9:41 [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe Shengjiu Wang
@ 2020-05-19 10:01 ` Lucas Stach
  2020-05-20  8:20   ` Shengjiu Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Lucas Stach @ 2020-05-19 10:01 UTC (permalink / raw)
  To: Shengjiu Wang, timur, nicoleotsuka, Xiubo.Lee, festevam,
	lgirdwood, broonie, perex, tiwai, shawnguo, s.hauer, kernel,
	linux-imx, sumit.semwal, alsa-devel, linuxppc-dev,
	linux-arm-kernel, linux-kernel, linux-media, dri-devel,
	linaro-mm-sig

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-19 10:01 ` Lucas Stach
@ 2020-05-20  8:20   ` Shengjiu Wang
  2020-05-20  9:42     ` Lucas Stach
  0 siblings, 1 reply; 8+ messages in thread
From: Shengjiu Wang @ 2020-05-20  8:20 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam,
	Liam Girdwood, Mark Brown, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, sumit.semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-20  8:20   ` Shengjiu Wang
@ 2020-05-20  9:42     ` Lucas Stach
  2020-05-20 11:22       ` Shengjiu Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Lucas Stach @ 2020-05-20  9:42 UTC (permalink / raw)
  To: Shengjiu Wang
  Cc: Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam,
	Liam Girdwood, Mark Brown, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, sumit.semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-20  9:42     ` Lucas Stach
@ 2020-05-20 11:22       ` Shengjiu Wang
  2020-05-20 12:38         ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Shengjiu Wang @ 2020-05-20 11:22 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li, Fabio Estevam,
	Liam Girdwood, Mark Brown, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, sumit.semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-20 11:22       ` Shengjiu Wang
@ 2020-05-20 12:38         ` Mark Brown
  2020-05-21 11:30           ` Shengjiu Wang
  0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2020-05-20 12:38 UTC (permalink / raw)
  To: Shengjiu Wang
  Cc: Lucas Stach, Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li,
	Fabio Estevam, Liam Girdwood, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, sumit.semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig


[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-20 12:38         ` Mark Brown
@ 2020-05-21 11:30           ` Shengjiu Wang
  2020-05-22 13:12             ` Mark Brown
  0 siblings, 1 reply; 8+ messages in thread
From: Shengjiu Wang @ 2020-05-21 11:30 UTC (permalink / raw)
  To: Mark Brown
  Cc: Lucas Stach, Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li,
	Fabio Estevam, Liam Girdwood, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, Sumit Semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
  2020-05-21 11:30           ` Shengjiu Wang
@ 2020-05-22 13:12             ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2020-05-22 13:12 UTC (permalink / raw)
  To: Shengjiu Wang
  Cc: Lucas Stach, Shengjiu Wang, Timur Tabi, Nicolin Chen, Xiubo Li,
	Fabio Estevam, Liam Girdwood, perex, Takashi Iwai, shawnguo,
	s.hauer, kernel, linux-imx, Sumit Semwal, Linux-ALSA,
	linuxppc-dev, linux-arm-kernel, linux-kernel, linux-media,
	dri-devel, linaro-mm-sig


[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

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 :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19  9:41 [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe Shengjiu Wang
2020-05-19 10:01 ` Lucas Stach
2020-05-20  8:20   ` Shengjiu Wang
2020-05-20  9:42     ` Lucas Stach
2020-05-20 11:22       ` Shengjiu Wang
2020-05-20 12:38         ` Mark Brown
2020-05-21 11:30           ` Shengjiu Wang
2020-05-22 13:12             ` Mark Brown

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git