[PATCH] ASoC: dmaengine: Document support for TX only or RX only streams
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;
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
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)?
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.
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.
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?
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.
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.
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.
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
participants (2)
-
Andy Shevchenko -
Mark Brown