ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe

Message ID 1589881301-4143-1-git-send-email-shengjiu.wang@nxp.com
State New
Headers show
Series
  • ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Related show

Commit Message

Shengjiu Wang May 19, 2020, 9:41 a.m.
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(-)

-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Shengjiu Wang May 20, 2020, 8:20 a.m. | #1
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);

> >

>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Lucas Stach May 20, 2020, 9:42 a.m. | #2
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);

> > > 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shengjiu Wang May 20, 2020, 11:22 a.m. | #3
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
Mark Brown May 20, 2020, 12:38 p.m. | #4
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shengjiu Wang May 21, 2020, 11:30 a.m. | #5
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
Mark Brown May 22, 2020, 1:12 p.m. | #6
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 :/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

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