[PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Mark Brown]

We intentionally do not return an error if we get a permanent failure
from dma_request_chan() in order to support systems which have TX only
or RX only channels. Add a comment documenting this.

Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-generic-dmaengine-pcm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
index fb95c1464e66..9ef80a48707e 100644
--- a/sound/soc/soc-generic-dmaengine-pcm.c
+++ b/sound/soc/soc-generic-dmaengine-pcm.c
@@ -386,6 +386,11 @@ static int dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm,
 			name = config->chan_names[i];
 		chan = dma_request_chan(dev, name);
 		if (IS_ERR(chan)) {
+			/*
+			 * Only report probe deferral errors, channels
+			 * might not be present for devices that
+			 * support only TX or only RX.
+			 */
 			if (PTR_ERR(chan) == -EPROBE_DEFER)
 				return -EPROBE_DEFER;
 			pcm->chan[i] = NULL;
-- 
2.20.1

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Andy Shevchenko]

On Thu, Oct 08, 2020 at 05:11:05PM +0100, Mark Brown wrote:
> We intentionally do not return an error if we get a permanent failure
> from dma_request_chan() in order to support systems which have TX only
> or RX only channels. Add a comment documenting this.

Thanks, makes sense!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

> Reported-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  sound/soc/soc-generic-dmaengine-pcm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/soc/soc-generic-dmaengine-pcm.c b/sound/soc/soc-generic-dmaengine-pcm.c
> index fb95c1464e66..9ef80a48707e 100644
> --- a/sound/soc/soc-generic-dmaengine-pcm.c
> +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> @@ -386,6 +386,11 @@ static int dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm,
>  			name = config->chan_names[i];
>  		chan = dma_request_chan(dev, name);
>  		if (IS_ERR(chan)) {
> +			/*
> +			 * Only report probe deferral errors, channels
> +			 * might not be present for devices that
> +			 * support only TX or only RX.
> +			 */
>  			if (PTR_ERR(chan) == -EPROBE_DEFER)
>  				return -EPROBE_DEFER;
>  			pcm->chan[i] = NULL;
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Andy Shevchenko]

On Fri, Oct 09, 2020 at 01:27:51PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 08, 2020 at 05:11:05PM +0100, Mark Brown wrote:
> > We intentionally do not return an error if we get a permanent failure
> > from dma_request_chan() in order to support systems which have TX only
> > or RX only channels. Add a comment documenting this.

> > --- a/sound/soc/soc-generic-dmaengine-pcm.c
> > +++ b/sound/soc/soc-generic-dmaengine-pcm.c
> > @@ -386,6 +386,11 @@ static int dmaengine_pcm_request_chan_of(struct dmaengine_pcm *pcm,
> >  			name = config->chan_names[i];
> >  		chan = dma_request_chan(dev, name);
> >  		if (IS_ERR(chan)) {
> > +			/*
> > +			 * Only report probe deferral errors, channels
> > +			 * might not be present for devices that
> > +			 * support only TX or only RX.
> > +			 */
> >  			if (PTR_ERR(chan) == -EPROBE_DEFER)
> >  				return -EPROBE_DEFER;
> >  			pcm->chan[i] = NULL;

Now I would like to continue discussion from this point.

What is the best way for individual ASoC drivers to be sure that at load time
they have or have not DMA resources available?

Now, seems the approach is to check dma-names property present and thus, try to
switch to DMA mode, otherwise PIO. But this seems to me a bit fragile. Why ASoC
core can't simple recognize DMA resources as optional (for the drivers that
want to know if they available or not)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Mark Brown]

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

On Fri, Oct 09, 2020 at 01:31:24PM +0300, Andy Shevchenko wrote:

> What is the best way for individual ASoC drivers to be sure that at load time
> they have or have not DMA resources available?

> Now, seems the approach is to check dma-names property present and thus, try to
> switch to DMA mode, otherwise PIO. But this seems to me a bit fragile. Why ASoC
> core can't simple recognize DMA resources as optional (for the drivers that
> want to know if they available or not)?

I'm not sure what you mean by "recognize DMA resources as optional"
here?  At present drivers that think something might not have appeared
should go through the resources and check them individually, anything
that hard errored won't be there.

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

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Andy Shevchenko]

On Mon, Oct 12, 2020 at 02:37:45PM +0100, Mark Brown wrote:
> On Fri, Oct 09, 2020 at 01:31:24PM +0300, Andy Shevchenko wrote:
> 
> > What is the best way for individual ASoC drivers to be sure that at load time
> > they have or have not DMA resources available?
> 
> > Now, seems the approach is to check dma-names property present and thus, try to
> > switch to DMA mode, otherwise PIO. But this seems to me a bit fragile. Why ASoC
> > core can't simple recognize DMA resources as optional (for the drivers that
> > want to know if they available or not)?
> 
> I'm not sure what you mean by "recognize DMA resources as optional"
> here?  At present drivers that think something might not have appeared
> should go through the resources and check them individually, anything
> that hard errored won't be there.

For example, when the board supports PIO and DMA mode and during the probe time
it wants to check which mode is desired (by means of DT references or alike).

Currently those drivers need to do something like:

	if (of_property_is_present("dma-names"))
		ret = try DMA mode;
	else
		ret = try PIO mode;

but this seems to me a bit stricter than needed. What if DMA mode fails, shall
we fail the probe of the driver?

If ASoC supports optional DMA resources, above can be simplified to something
like:

	ret = try DMA mode;
	if (ret != DMA mode ok)
		ret = try PIO mode;

which makes OF dependent parts gone along with relying on the properties rather
than real resource availability.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Mark Brown]

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

On Mon, Oct 12, 2020 at 04:55:27PM +0300, Andy Shevchenko wrote:

> Currently those drivers need to do something like:

> 	if (of_property_is_present("dma-names"))
> 		ret = try DMA mode;
> 	else
> 		ret = try PIO mode;

> but this seems to me a bit stricter than needed. What if DMA mode fails, shall
> we fail the probe of the driver?

They can also just try registering DMA and fall back to PIO.

> If ASoC supports optional DMA resources, above can be simplified to something
> like:

> 	ret = try DMA mode;
> 	if (ret != DMA mode ok)
> 		ret = try PIO mode;

> which makes OF dependent parts gone along with relying on the properties rather
> than real resource availability.

I don't understand the blocker to writing that code at the minute?

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

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Andy Shevchenko]

On Mon, Oct 12, 2020 at 04:48:03PM +0100, Mark Brown wrote:
> On Mon, Oct 12, 2020 at 04:55:27PM +0300, Andy Shevchenko wrote:
> 
> > Currently those drivers need to do something like:
> 
> > 	if (of_property_is_present("dma-names"))
> > 		ret = try DMA mode;
> > 	else
> > 		ret = try PIO mode;
> 
> > but this seems to me a bit stricter than needed. What if DMA mode fails, shall
> > we fail the probe of the driver?
> 
> They can also just try registering DMA and fall back to PIO.

There is no possibility to do like this right now.

> > If ASoC supports optional DMA resources, above can be simplified to something
> > like:
> 
> > 	ret = try DMA mode;
> > 	if (ret != DMA mode ok)
> > 		ret = try PIO mode;
> 
> > which makes OF dependent parts gone along with relying on the properties rather
> > than real resource availability.
> 
> I don't understand the blocker to writing that code at the minute?

Return code in both cases DMA okay, DMA is not okay is 0.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Mark Brown]

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

On Mon, Oct 12, 2020 at 07:31:47PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 12, 2020 at 04:48:03PM +0100, Mark Brown wrote:
> > On Mon, Oct 12, 2020 at 04:55:27PM +0300, Andy Shevchenko wrote:

> > > 	if (ret != DMA mode ok)
> > > 		ret = try PIO mode;

> > > which makes OF dependent parts gone along with relying on the properties rather
> > > than real resource availability.

> > I don't understand the blocker to writing that code at the minute?

> Return code in both cases DMA okay, DMA is not okay is 0.

Ah, right - we don't really expose the resulting component to the
drivers.  Although we don't appear to have any drivers doing the open
coding you suggest?  The active use case we have is for drivers
(currently only the STM32 SAI AFAICT) that always do DMA but only do one
direction (not half duplex, a single direction on a given DAI).  They
don't want to fall back to PIO, they want to know which channel is
valid.  It's not just a DMA/no DMA question, it's also which of the DMA
channels are valid.

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

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Andy Shevchenko]

On Mon, Oct 12, 2020 at 07:26:04PM +0100, Mark Brown wrote:
> On Mon, Oct 12, 2020 at 07:31:47PM +0300, Andy Shevchenko wrote:
> > On Mon, Oct 12, 2020 at 04:48:03PM +0100, Mark Brown wrote:
> > > On Mon, Oct 12, 2020 at 04:55:27PM +0300, Andy Shevchenko wrote:
> 
> > > > 	if (ret != DMA mode ok)
> > > > 		ret = try PIO mode;
> 
> > > > which makes OF dependent parts gone along with relying on the properties rather
> > > > than real resource availability.
> 
> > > I don't understand the blocker to writing that code at the minute?
> 
> > Return code in both cases DMA okay, DMA is not okay is 0.
> 
> Ah, right - we don't really expose the resulting component to the
> drivers.  Although we don't appear to have any drivers doing the open
> coding you suggest?  The active use case we have is for drivers
> (currently only the STM32 SAI AFAICT) that always do DMA but only do one
> direction (not half duplex, a single direction on a given DAI).  They
> don't want to fall back to PIO, they want to know which channel is
> valid.  It's not just a DMA/no DMA question, it's also which of the DMA
> channels are valid.

Looking into them I think all of the cases are requiring DMA to work.
At least one channel. Seems no one is designed for optional DMA performance.

The problem here is they are checking for properties (meta-data) rather than
resources (data) to be available. But since they will fail sooner or later it
doesn't make big difference.


% git grep -n dma-names -- sound/soc/ | cut -f1 -d: | sort -u
sound/soc/adi/axi-i2s.c
sound/soc/atmel/atmel-i2s.c
sound/soc/stm/stm32_sai_sub.c
sound/soc/tegra/tegra210_admaif.c
sound/soc/ti/davinci-mcasp.c

axi-i2s: Checks for channels to see if capture / playback are supposed to be
	 working, but AFAICS tries without actually checking the resources
	 availability.

	snd_soc_dai_init_dma_data(dai,
		i2s->has_playback ? &i2s->playback_dma_data : NULL,
		i2s->has_capture  ? &i2s->capture_dma_data  : NULL);

atmel-i2s: Checks for half-duplex channel, but registers unconditionally.

        snd_soc_dai_init_dma_data(dai, &dev->playback, &dev->capture);

tegra210_admaif: Checks for Tx to be always present.

	Custom DAI probe that simply assigns data structure pointers.

davinchi-mcasp: Checks for names to be present

	Custom DAI probe that simply assigns data structure pointers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] ASoC: dmaengine: Document support for TX only or RX only streams

[Mark Brown]

On Thu, 8 Oct 2020 17:11:05 +0100, Mark Brown wrote:
> We intentionally do not return an error if we get a permanent failure
> from dma_request_chan() in order to support systems which have TX only
> or RX only channels. Add a comment documenting this.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: dmaengine: Document support for TX only or RX only streams
      commit: 86f29c7442ac4ba5fe19fc2ada457f76c0080dd6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark