drm/nouveau: tegra: Detach from ARM DMA/IOMMU mapping

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Given that no one else implements it making it a generic API seems rather confusing. For now I'd rename it to arm_dma_iommu_detach_device() and only implement it in arm.

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Quoted 3 lines

Given that no one else implements it making it a generic API seems rather confusing. For now I'd rename it to arm_dma_iommu_detach_device() and only implement it in arm.

That'd be suboptimal because this code is used on both 32-bit and 64-bit ARM. If we make the function 32-bit ARM specific then the driver code would need to use an #ifdef to make sure compilation doesn't break on 64-bit ARM.

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Quoted 8 lines

Quoted 3 lines

Given that no one else implements it making it a generic API seems rather confusing. For now I'd rename it to arm_dma_iommu_detach_device() and only implement it in arm.

That'd be suboptimal because this code is used on both 32-bit and 64-bit ARM. If we make the function 32-bit ARM specific then the driver code would need to use an #ifdef to make sure compilation doesn't break on 64-bit ARM.

Do you still want me to make this ARM specific? While I haven't encountered this issue on 64-bit ARM yet, I think it would happen there as well, under the right circumstances. I could take a shot at implementing the equivalent there (which means essentially implementing it for drivers/iommu/dma-iommu.c and calling that from 64-bit ARM code).

Quoted 19 lines · Thierry Reding

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Quoted 8 lines

Quoted 3 lines

Given that no one else implements it making it a generic API seems rather confusing. For now I'd rename it to arm_dma_iommu_detach_device() and only implement it in arm.

That'd be suboptimal because this code is used on both 32-bit and 64-bit ARM. If we make the function 32-bit ARM specific then the driver code would need to use an #ifdef to make sure compilation doesn't break on 64-bit ARM.

Quoted 5 lines

Do you still want me to make this ARM specific? While I haven't encountered this issue on 64-bit ARM yet, I think it would happen there as well, under the right circumstances. I could take a shot at implementing the equivalent there (which means essentially implementing it for drivers/iommu/dma-iommu.c and calling that from 64-bit ARM code).

It sounds like things are getting a bit backwards here: iommu-dma should have nothing to do with this, since if you've explicitly attached the device to your own IOMMU domain then you're already bypassing everything it knows about and has control over. Arch code calling into iommu-dma to do something which makes arch code not use iommu-dma makes very little sense.

Quoted 26 lines · Robin Murphy

Quoted 19 lines · Thierry Reding

Quoted 9 lines · Thierry Reding

Quoted 8 lines · Christoph Hellwig

Quoted 6 lines · Thierry Reding

From: Thierry Reding <[email protected]>

The dma_iommu_detach_device() API can be used by drivers to forcibly detach a device from an IOMMU that architecture code might have attached to. This is useful for drivers that need explicit control over the IOMMU using the IOMMU API directly.

Quoted 8 lines

Quoted 3 lines

Given that no one else implements it making it a generic API seems rather confusing. For now I'd rename it to arm_dma_iommu_detach_device() and only implement it in arm.

That'd be suboptimal because this code is used on both 32-bit and 64-bit ARM. If we make the function 32-bit ARM specific then the driver code would need to use an #ifdef to make sure compilation doesn't break on 64-bit ARM.

Quoted 5 lines

Do you still want me to make this ARM specific? While I haven't encountered this issue on 64-bit ARM yet, I think it would happen there as well, under the right circumstances. I could take a shot at implementing the equivalent there (which means essentially implementing it for drivers/iommu/dma-iommu.c and calling that from 64-bit ARM code).

Quoted 5 lines

It sounds like things are getting a bit backwards here: iommu-dma should have nothing to do with this, since if you've explicitly attached the device to your own IOMMU domain then you're already bypassing everything it knows about and has control over. Arch code calling into iommu-dma to do something which makes arch code not use iommu-dma makes very little sense.

My understanding is that iommu-dma will set up an IOMMU domain at device probe time anyway. So even if attaching to an own IOMMU domain will end up bypassing iommu-dma, we'd still want to clear up the IOMMU domain and any associated resources, right?

arch/arm/mm/dma-mapping.c +4 -0

+void arch_iommu_detach_device(struct device *dev)
+{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);

Quoted 4 lines

arch/arm/mm/dma-mapping.c +4 -0

+void arch_iommu_detach_device(struct device *dev)
+{
+#ifdef CONFIG_ARM_DMA_USE_IOMMU
+	struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);

arch/arm/mm/dma-mapping.c void arch_teardown_dma_ops(struct device *dev) · line 2440

Quoted 10 lines

arch/arm/mm/dma-mapping.c +10 -0

+	const struct dma_map_ops *dma_ops;
+
+	if (!mapping)
+		return;
+
+	arm_iommu_release_mapping(mapping);
+	arm_iommu_detach_device(dev);
+
+	dma_ops = arm_get_dma_map_ops(dev->archdata.dma_coherent);
+	set_dma_ops(dev, dma_ops);

Why not simply:

	set_dma_ops(dev, arm_get_dma_map_ops(dev->archdata.dma_coherent));

From: Thierry Reding <[email protected]>

The ARM_DMA_USE_IOMMU Kconfig option has side-effects that drivers can not opt into but have to explicitly opt out of. This can lead to subtle bugs that are difficult to track down and not immediately obvious to be related to this Kconfig option.

To avoid this confusion, always enable the option to expose any lurking bugs once and allow any regressions introduced by the DMA/IOMMU code to be caught more quickly in the future.

Note that some drivers still use the Kconfig symbol to provide different code paths depending on what architecture the code runs on (e.g. 32-bit ARM vs. 64-bit ARM which have different and incompatible implementations of the DMA/IOMMU integration code), so leave the symbol in place to keep those drivers working.

For the long term, it is preferable to transition everyone to the generic DMA/IOMMU integration code in drivers/iommu/dma-iommu.c.

Signed-off-by Thierry Reding <[email protected]>

---
Changes in v2:
- new patch

 arch/arm/Kconfig               |  2 +-
 arch/arm/include/asm/device.h  |  6 ------
 arch/arm/mm/dma-mapping.c      | 18 ------------------
 drivers/iommu/Kconfig          |  7 -------
 drivers/media/platform/Kconfig |  1 -
 5 files changed, 1 insertion(+), 33 deletions(-)

arch/arm/Kconfig +0 -0

index fa0b190f8a38..3c91de78535a 100644

Quoted 4 lines

config ARM_DMA_USE_IOMMU

arch/arm/Kconfig +1 -1

-	bool
+	def_bool y
 	select ARM_HAS_SG_CHAIN

Quoted 1 line

select NEED_SG_DMA_LENGTH

This doesn't work - as has recently been discussed with hch, we can't globally enable NEED_SG_DMA_LENGTH on ARM - the ARM architecture pre-dates the addition of the DMA length member in the scatterlist, and not every machine supports the splitting of the DMA length from the non-DMA length member. Hence, this will cause a regression, sorry.

The series seems to miss a cover letter.

Also I really think this patch original patch shouldn't be in the proper series.

From: Thierry Reding <[email protected]>

Depending on the kernel configuration, early ARM architecture setup code may have attached the GPU to a DMA/IOMMU mapping that transparently uses the IOMMU to back the DMA API. Tegra requires special handling for IOMMU backed buffers (a special bit in the GPU's MMU page tables indicates the memory path to take: via the SMMU or directly to the memory controller). Transparently backing DMA memory with an IOMMU prevents Nouveau from properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory don't need to be physically contiguous from the GPU's perspective since the GPU can map them into contiguous buffers using its own MMU. Mapping these buffers through the IOMMU is unnecessary and will even lead to performance degradation because of the additional translation.

Signed-off-by Thierry Reding <[email protected]>

---
I had already sent this out independently to fix a regression that was introduced in v4.16, but then Christoph pointed out that it should've been sent to a wider audience and should use a core API rather than calling into architecture code directly.

I've added it to this series for easier reference and to show the need for the new API.

Quoted 17 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Depending on the kernel configuration, early ARM architecture setup code may have attached the GPU to a DMA/IOMMU mapping that transparently uses the IOMMU to back the DMA API. Tegra requires special handling for IOMMU backed buffers (a special bit in the GPU's MMU page tables indicates the memory path to take: via the SMMU or directly to the memory controller). Transparently backing DMA memory with an IOMMU prevents Nouveau from properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory don't need to be physically contiguous from the GPU's perspective since the GPU can map them into contiguous buffers using its own MMU. Mapping these buffers through the IOMMU is unnecessary and will even lead to performance degradation because of the additional translation.

Signed-off-by Thierry Reding <[email protected]>

Quoted 8 lines

---
I had already sent this out independently to fix a regression that was introduced in v4.16, but then Christoph pointed out that it should've been sent to a wider audience and should use a core API rather than calling into architecture code directly.

I've added it to this series for easier reference and to show the need for the new API.

This is good stuff, I am struggling with something similar on ARM64. One problem that I wasn't able to fully solve cleanly was that for arm-smmu the SMMU HW resources are not released until the domain itself is destroyed and I never quite figured out a way to swap the default domain cleanly.

This is a problem for the MSM GPU because not only do we run our own IOMMU as you do we also have a hardware dependency to use context bank 0 to asynchronously switch the pagetable during rendering.

I'm not sure if this is a problem you have encountered.

In any event, this code
gets us a little bit further down the path and at least there is somebody else out there in the cold dark world that understands my pain. :)

For your interest, here was my half-hearted attempt to avoid creating DMA domains in the first place based on a blacklist to try to spur a bit of
discussion: https://patchwork.freedesktop.org/series/41573/

Quoted 36 lines · Jordan Crouse

Quoted 17 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Depending on the kernel configuration, early ARM architecture setup code may have attached the GPU to a DMA/IOMMU mapping that transparently uses the IOMMU to back the DMA API. Tegra requires special handling for IOMMU backed buffers (a special bit in the GPU's MMU page tables indicates the memory path to take: via the SMMU or directly to the memory controller). Transparently backing DMA memory with an IOMMU prevents Nouveau from properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory don't need to be physically contiguous from the GPU's perspective since the GPU can map them into contiguous buffers using its own MMU. Mapping these buffers through the IOMMU is unnecessary and will even lead to performance degradation because of the additional translation.

Signed-off-by Thierry Reding <[email protected]>

Quoted 8 lines

---
I had already sent this out independently to fix a regression that was introduced in v4.16, but then Christoph pointed out that it should've been sent to a wider audience and should use a core API rather than calling into architecture code directly.

I've added it to this series for easier reference and to show the need for the new API.

This is good stuff, I am struggling with something similar on ARM64. One problem that I wasn't able to fully solve cleanly was that for arm-smmu the SMMU HW resources are not released until the domain itself is destroyed and I never quite figured out a way to swap the default domain cleanly.

This is a problem for the MSM GPU because not only do we run our own IOMMU as you do we also have a hardware dependency to use context bank 0 to asynchronously switch the pagetable during rendering.

Quoted 1 line

I'm not sure if this is a problem you have encountered.

I don't think I have. Recent chips have similar capabilities, but they don't have the restriction to a context bank, as far as I understand. Adding Mikko who's had more exposure to this.

Quoted 4 lines

In any event, this code
gets us a little bit further down the path and at least there is somebody else out there in the cold dark world that understands my pain. :)

This doesn't actually fix anything on 64-bit ARM, and oddly enough I haven't run into this issue myself on 64-bit ARM either. I think the reason is that I haven't tested Nouveau on Tegra186 yet, which is the first SoC which has an ARM SMMU. On prior 64-bit ARM chips we've used the custom Tegra SMMU and that driver simply forbids creating any DMA domains, so it will allow only explicit usage of the IOMMU API. There is code in the generic DMA/IOMMU integration layer to not use the DMA API with non-DMA IOMMU domains, but that's not true on 32-bit ARM, unfortunately. It's entirely possible that Tegra186 will show exactly the same problem that you are describing.

We do use the IOMMU API explicitly in the Tegra DRM driver as well, and that is something that I've tested on Tegra186 and that I know to be working. However, the reason why it works there is that the IOMMU group will contain multiple display controllers, which will again trigger a special case that will prevent the DMA/IOMMU integration from setting up a DMA domain for use with those devices.

Quoted 3 lines

For your interest, here was my half-hearted attempt to avoid creating DMA domains in the first place based on a blacklist to try to spur a bit of
discussion: https://patchwork.freedesktop.org/series/41573/

This looks very interesting and simple, but I can imagine that it will see significant pushback from the ARM SMMU maintainers (if not complete silence), because it adds SoC-specific knowledge to an otherwise fully generic driver. Having the GPU driver explicitly detach from the IOMMU domain sounds like a better option, but I don't see how that would help with the context bank issue that you're seeing.

One other possibility that I can imagine is to add something to struct device that could be used by arch_setup_dma_ops() to not attach any of the IOMMU-backed DMA operations to the device. Unfortunately that code is called before the driver's ->probe() implementation is called, so a driver doesn't have an opportunity to set it. Something like of_dma_configure() could still set that up, perhaps based on some DT property, though I can already hear the NAK from DT maintainers because this is, after all, policy, not hardware description.

The last solution that I can think of that might allow us to do this is to add a flag to struct device_driver (bool explicit_iommu?) that will be used by the DMA/IOMMU setup code to decide whether or not to attach to the IOMMU automatically.

Though, again, I'm not sure that would actually solve your bank problem. That's really something I don't see any other solution than to fix it in the ARM SMMU driver. Perhaps context bank 0 can always be reserved for non-DMA domains?

Thierry

Quoted 37 lines · Thierry Reding

Quoted 36 lines · Jordan Crouse

Quoted 17 lines · Thierry Reding

From: Thierry Reding <[email protected]>

Depending on the kernel configuration, early ARM architecture setup code may have attached the GPU to a DMA/IOMMU mapping that transparently uses the IOMMU to back the DMA API. Tegra requires special handling for IOMMU backed buffers (a special bit in the GPU's MMU page tables indicates the memory path to take: via the SMMU or directly to the memory controller). Transparently backing DMA memory with an IOMMU prevents Nouveau from properly handling such memory accesses and causes memory access faults.

As a side-note: buffers other than those allocated in instance memory don't need to be physically contiguous from the GPU's perspective since the GPU can map them into contiguous buffers using its own MMU. Mapping these buffers through the IOMMU is unnecessary and will even lead to performance degradation because of the additional translation.

Signed-off-by Thierry Reding <[email protected]>

Quoted 8 lines

---
I had already sent this out independently to fix a regression that was introduced in v4.16, but then Christoph pointed out that it should've been sent to a wider audience and should use a core API rather than calling into architecture code directly.

I've added it to this series for easier reference and to show the need for the new API.

This is good stuff, I am struggling with something similar on ARM64. One problem that I wasn't able to fully solve cleanly was that for arm-smmu the SMMU HW resources are not released until the domain itself is destroyed and I never quite figured out a way to swap the default domain cleanly.

This is a problem for the MSM GPU because not only do we run our own IOMMU as you do we also have a hardware dependency to use context bank 0 to asynchronously switch the pagetable during rendering.

Quoted 5 lines

Quoted 1 line

I'm not sure if this is a problem you have encountered.

I don't think I have. Recent chips have similar capabilities, but they don't have the restriction to a context bank, as far as I understand. Adding Mikko who's had more exposure to this.

IIRC the only way I've gotten Host1x to work on Tegra186 with IOMMU enabled is doing the equivalent of this patch (or actually using the DMA API, which may be possible but has some potential issues).

As you said, we don't have a limitation regarding the context bank or similar - Host1x handles context switching by changing the sent stream ID on the fly (which is quite difficult to deal with from kernel point of view as well), and the actual GPU has its own MMU.