Re: [alsa-devel] of_dma_request_slave_channel() failed ?
Hi Morimoto-san,
CC asoc, dma, iommu
On Tue, Sep 4, 2018 at 8:06 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
Hi Renesas ML Cc Rob
I noticed that Sound driver is using PIO mode on latest kernel.
... rcar_sound ec500000.sound: src[0] : probe error -11 rcar_sound ec500000.sound: ssi[0] fallback to PIO mode rcar_sound ec500000.sound: src[1] : probe error -11 rcar_sound ec500000.sound: ssi[1] fallback to PIO mode rcar_sound ec500000.sound: ssi[2] : probe error -11 rcar_sound ec500000.sound: ssi[2] fallback to PIO mode rcar_sound ec500000.sound: ssi[3] : probe error -11 rcar_sound ec500000.sound: ssi[3] fallback to PIO mode rcar_sound ec500000.sound: probed ...Sound can use DMA mode and PIO mode. It automatically fallbacks to PIO mode if it can't use DMA.
I bisected this issue, and found the 1st bad commit
commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d Author: Rob Herring robh@kernel.org Date: Mon Jul 9 09:41:52 2018 -0600
iommu: Remove IOMMU_OF_DECLARE Now that we use the driver core to stop deferred probe for missing drivers, IOMMU_OF_DECLARE can be removed. This is slightly less optimal than having a list of built-in drivers in that we'll now defer probe twice before giving up. This shouldn't have a significant impact on boot times as past discussions about deferred probe have given no evidence of deferred probe having a substantial impact. ...
In more detail, it seems it can't find DMA controller, and of_dma_request_slave_channel() returns -EPROBE_DEFER from this commit.
struct dma_chan *of_dma_request_slave_channel(struct device_node *np, const char *name) { ... ofdma = of_dma_find_controller(&dma_spec);
if (ofdma) { chan = ofdma->of_dma_xlate(&dma_spec, ofdma); } else {=> ret_no_channel = -EPROBE_DEFER; chan = NULL; } ... => return ERR_PTR(ret_no_channel); }
Is this known issue ? Or sound shouldn't fallbacks to PIO mode ?
I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback to PIO mode when -EPROBE_DEFER") to work around this?
While this got rid of the error messages, and postpones sound initialization until the audio DMAC is available, it does mean that the driver will _never_ fall back to PIO.
I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling back to PIO.
With CONFIG_RCAR_DMAC=y:
rcar-dmac e6700000.dma-controller: ignoring dependency for device, assuming no driver rcar-dmac e7300000.dma-controller: ignoring dependency for device, assuming no driver rcar-dmac e7310000.dma-controller: ignoring dependency for device, assuming no driver rcar-dmac ec700000.dma-controller: ignoring dependency for device, assuming no driver rcar-dmac ec720000.dma-controller: ignoring dependency for device, assuming no driver
So it seems the audio DMAC is deferred a second time, before the iommu driver probed.
subsys_initcall(ipmmu_init); calls platform_driver_register module_platform_driver(rcar_dmac_driver);
Gr{oetje,eeting}s,
Geert
On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:
So it seems the audio DMAC is deferred a second time, before the iommu driver probed.
Shouldn't there be at least one more round of deferred probe triggers after the IOMMU probes?
Hi Mark,
On Wed, Sep 12, 2018 at 5:51 PM Mark Brown broonie@kernel.org wrote:
On Tue, Sep 11, 2018 at 11:43:47AM +0200, Geert Uytterhoeven wrote:
So it seems the audio DMAC is deferred a second time, before the iommu driver probed.
Shouldn't there be at least one more round of deferred probe triggers after the IOMMU probes?
My statement above was incorrect. Adding more debug info allowed to gain more insight:
A. If CONFIG_IPMMU_VMSA=y, everything is fine:
1. ipmmu probes, 2. audio-dmac probes, 3. audio probes, using DMA
B. If CONFIG_IPMMU_VMSA=n, the sound driver falls back to PIO, which Morimoto-san worked around by commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback to PIO mode when -EPROBE_DEFER")[1]
1. ipmmu is not probed: driver not enabled => OK 2. audio-dmac is not probed: iommu not found 3. audio fails to probe (3 times, hence just ignoring the first one is not sufficient): "of_dma_find_controller: can't find DMA controller /soc/dma-controller@ec700000" => fell back to PIO before [1] => -EPROBE_DEFER since [1] 4. audio-dmac probes of_iommu_xlate() calls driver_deferred_probe_check_state(), leading to ("ignoring dependency for device, assuming no driver")
If step 3 returned -EPROBE_DEFER, there's one additional step: 5. audio reprobes successfully, using DMA.
While step 2 (DMA-capable device not probed because iommu not found) is correct for devices with a builtin dmac, it causes issues with external dmacs, as the dmac probe may be postponed after the DMA slave driver probe.
C. Before commit ac6bbf0cdf4206c5 ("iommu: Remove IOMMU_OF_DECLARE"), with CONFIG_IPMMU_VMSA=n:
1. ipmmu is not probed: driver not enabled => OK 2. audio-dmac probes => OK Missing IOMMU ignored, as per !ops && !of_iommu_driver_present(iommu_spec->np) in of_iommu_xlate(), which is really !ops && !of_match_node(&__iommu_of_table, np)) 3. audio probes, using DMA Note: if of_dma_find_controller() would have failed (e.g. missing dmac driver), the audio driver fell back to PIO here.
As per Morimoto-san's recent reply, audio really needs DMA, so the current behavior of B.5. is OK. However, this may not be true for other devices (e.g. SPI, i2c, serial, ...), where the dmac is optional.
The main issue is that if of_dma_find_controller() fails, a DMA slave driver cannot distinguish between dmac not yet probed successfully, and dmac driver not present.
However, as we rely more and more on probe deferral, this will cause more issues in the future.
I guess the proper solution is to take into account dependencies described in DT before probing devices, i.e. devices with phandles should always be probed after the targets of these phandles? Complication: - Circular dependencies, - Optional phandle targets (dmacs, oops).
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 13/09/18 10:00, Geert Uytterhoeven wrote: [...]
The main issue is that if of_dma_find_controller() fails, a DMA slave driver cannot distinguish between dmac not yet probed successfully, and dmac driver not present.
...which is in fact the exact same problem that the IOMMU code has - might it make sense to give of_dma_request_slave_channel() similar (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC driver's not shown up by this point, assume it's not built-in and go on without it". Of course it is somewhat easier for IOMMU drivers as there's zero chance of those popping up as modules later on.
However, as we rely more and more on probe deferral, this will cause more issues in the future.
I guess the proper solution is to take into account dependencies described in DT before probing devices, i.e. devices with phandles should always be probed after the targets of these phandles? Complication:
- Circular dependencies,
- Optional phandle targets (dmacs, oops).
Yeah, a proper dependency graph would be ideal, but it's certainly not trivial. There was an interesting proposal a couple of years ago of a half-way approach where various phandle-chasing routines could actively try to probe the target, but I think that got stymied on the fact that a single OF node may represent multiple different driver-level providers, so it's tricky to tell whether the *right* dependency has been satisfied without detailed knowledge of the individual bindings and driver implementations. That's probably still an issue for other approaches too, sadly.
Robin.
Hi Robin,
On Thu, Sep 13, 2018 at 12:12 PM Robin Murphy robin.murphy@arm.com wrote:
On 13/09/18 10:00, Geert Uytterhoeven wrote: [...]
The main issue is that if of_dma_find_controller() fails, a DMA slave driver cannot distinguish between dmac not yet probed successfully, and dmac driver not present.
...which is in fact the exact same problem that the IOMMU code has - might it make sense to give of_dma_request_slave_channel() similar (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC driver's not shown up by this point, assume it's not built-in and go on without it". Of course it is somewhat easier for IOMMU drivers as there's zero chance of those popping up as modules later on.
It may solve the issue in some cases. But only if the dmac is reprobed before the DMA slave driver, which is not guaranteed.
BTW, it seems e.g. i2c and serial suffer from the same problem, and fall back to PIO. However, these drivers try to obtain the DMA channel when used, not during probe, so they start using DMA after the dmac has been probed successfully.
Gr{oetje,eeting}s,
Geert
Hi Geert
...which is in fact the exact same problem that the IOMMU code has - might it make sense to give of_dma_request_slave_channel() similar (optional?) driver_deferred_probe_check_state() logic, i.e. "if my DMAC driver's not shown up by this point, assume it's not built-in and go on without it". Of course it is somewhat easier for IOMMU drivers as there's zero chance of those popping up as modules later on.
It may solve the issue in some cases. But only if the dmac is reprobed before the DMA slave driver, which is not guaranteed.
BTW, it seems e.g. i2c and serial suffer from the same problem, and fall back to PIO. However, these drivers try to obtain the DMA channel when used, not during probe, so they start using DMA after the dmac has been probed successfully.
Actually sound driver get DMA channel when used. But checking DMA availability when probe timing (= try to get, and release it soon). sound driver side other approach is that it don't check it when probe.
Best regards --- Kuninori Morimoto
Hi Geert
Thank you for your feedback
commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d Author: Rob Herring robh@kernel.org Date: Mon Jul 9 09:41:52 2018 -0600
iommu: Remove IOMMU_OF_DECLARE Now that we use the driver core to stop deferred probe for missing drivers, IOMMU_OF_DECLARE can be removed. This is slightly less optimal than having a list of built-in drivers in that we'll now defer probe twice before giving up. This shouldn't have a significant impact on boot times as past discussions about deferred probe have given no evidence of deferred probe having a substantial impact. ...
(snip)
I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback to PIO mode when -EPROBE_DEFER") to work around this?
Yes, it is work around for it.
While this got rid of the error messages, and postpones sound initialization until the audio DMAC is available, it does mean that the driver will _never_ fall back to PIO.
I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling back to PIO.
If I focus only for sound here, the purpose of PIO mode is checking basic HW connection, clock settings, etc. Without DMAC support, it can't use many sound features. And PIO mode is supporting "SSI" only.
If DT has SRC/CTU/DVC settings for sound, it needs DMA support anyway.
&rcar_sound { ... ports { rsnd_port0: port@0 { rsnd_endpoint0: endpoint { ... => playback = <&ssi0 &src0 &dvc0>; }; }; }; };
Before 6c92d5a2744e2761 patch, driver will forcibly ignore non-SSI modules, and try to use PIO mode. I'm not sure it is "kindly support" or "overkill support".
After this patch, it needs DMA, otherwise, probe will be failed. DT shouldn't have non-SSI modules if you want to use PIO mode.
+ /* use PIO mode */ - playback = <&ssi0 &src0 &dvc0>; + playback = <&ssi0>;
Hi Morimoto-san,
On Thu, Sep 13, 2018 at 7:48 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
commit ac6bbf0cdf4206c517ac9789814c23e372ebce4d Author: Rob Herring robh@kernel.org Date: Mon Jul 9 09:41:52 2018 -0600
iommu: Remove IOMMU_OF_DECLARE Now that we use the driver core to stop deferred probe for missing drivers, IOMMU_OF_DECLARE can be removed. This is slightly less optimal than having a list of built-in drivers in that we'll now defer probe twice before giving up. This shouldn't have a significant impact on boot times as past discussions about deferred probe have given no evidence of deferred probe having a substantial impact. ...
(snip)
I assume you wrote commit 6c92d5a2744e2761 ("ASoC: rsnd: don't fallback to PIO mode when -EPROBE_DEFER") to work around this?
Yes, it is work around for it.
While this got rid of the error messages, and postpones sound initialization until the audio DMAC is available, it does mean that the driver will _never_ fall back to PIO.
I.e. if CONFIG_RCAR_DMAC=n, audio will fail to probe instead of falling back to PIO.
If I focus only for sound here, the purpose of PIO mode is checking basic HW connection, clock settings, etc. Without DMAC support, it can't use many sound features. And PIO mode is supporting "SSI" only.
If DT has SRC/CTU/DVC settings for sound, it needs DMA support anyway.
&rcar_sound { ... ports { rsnd_port0: port@0 { rsnd_endpoint0: endpoint { ... => playback = <&ssi0 &src0 &dvc0>; }; }; }; };
Before 6c92d5a2744e2761 patch, driver will forcibly ignore non-SSI modules, and try to use PIO mode. I'm not sure it is "kindly support" or "overkill support".
After this patch, it needs DMA, otherwise, probe will be failed. DT shouldn't have non-SSI modules if you want to use PIO mode.
+ /* use PIO mode */ - playback = <&ssi0 &src0 &dvc0>; + playback = <&ssi0>;
OK, so falling back to PIO was really a "best effort" fallback, and the user should really enable DMA?
Gr{oetje,eeting}s,
Geert
Hi Geert
Before 6c92d5a2744e2761 patch, driver will forcibly ignore non-SSI modules, and try to use PIO mode. I'm not sure it is "kindly support" or "overkill support".
After this patch, it needs DMA, otherwise, probe will be failed. DT shouldn't have non-SSI modules if you want to use PIO mode.
+ /* use PIO mode */ - playback = <&ssi0 &src0 &dvc0>; + playback = <&ssi0>;OK, so falling back to PIO was really a "best effort" fallback, and the user should really enable DMA?
Yeah, I'm thinking this is better. PIO fallback is of course "nice to have" if possible. But, because of this new iommu patch, keeping this feature needs "big complicated patch", and we can get "small effect" I think. Thus, I think this is the time to remove this feature. Can you agree ?
Best regards --- Kuninori Morimoto
Hi Morimoto-san,
On Fri, Sep 14, 2018 at 2:53 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
Before 6c92d5a2744e2761 patch, driver will forcibly ignore non-SSI modules, and try to use PIO mode. I'm not sure it is "kindly support" or "overkill support".
After this patch, it needs DMA, otherwise, probe will be failed. DT shouldn't have non-SSI modules if you want to use PIO mode.
+ /* use PIO mode */ - playback = <&ssi0 &src0 &dvc0>; + playback = <&ssi0>;OK, so falling back to PIO was really a "best effort" fallback, and the user should really enable DMA?
Yeah, I'm thinking this is better. PIO fallback is of course "nice to have" if possible. But, because of this new iommu patch, keeping this feature needs "big complicated patch", and we can get "small effect" I think. Thus, I think this is the time to remove this feature. Can you agree ?
You're the rcar-sound expert ;-) If you think there's not much value in keeping PIO fallback, it can be removed.
We can change behavior once these dependency issues have been resolved in a generic way.
Gr{oetje,eeting}s,
Geert
participants (4)
-
Geert Uytterhoeven -
Kuninori Morimoto -
Mark Brown -
Robin Murphy