On 07.04.2023 01:37, Jason Gunthorpe wrote: > On Mon, Feb 20, 2023 at 01:58:40PM +0000, Robin Murphy wrote: >> On 2023-02-17 12:35, Jason Gunthorpe wrote: >>> On Fri, Feb 17, 2023 at 12:08:42PM +0100, Marek Szyprowski wrote: >>>> I'm sorry for a delay in replying, but I was busy with other stuff. >>>> >>>> On 23.01.2023 22:00, Jason Gunthorpe wrote: >>>>> On Mon, Jan 23, 2023 at 10:31:01AM +0100, Marek Szyprowski wrote: >>>>>> Add set_platform_dma_ops() required for proper driver operation on ARM >>>>>> 32bit arch after recent changes in the IOMMU framework (detach ops >>>>>> removal). >>>>> Thanks for looking into this! >>>>> >>>>> Can you explain more about how this actually solves the problem in the >>>>> commit message? I don't get it. >>>> Exynos DRM driver calls arm_iommu_detach_device(), then >>>> arm_iommu_attach_device() with a difrent 'mapping', see >>>> drivers/gpu/drm/exynos/exynos_drm_dma.c Lack of set_platform_dma_ops >>>> leads to a warning in iommu_group_do_set_platform_dma(). The other case >>>> of calling arm_iommu_detach_device() is after unsuccessful probe of the >>>> platform device. >>> Why can't this just use the normal iommu path in all cases? >>> >>> It looks like it is trying to copy the DMA API domain from a parent >>> device to a sub device. >>> >>> Even when using arm_iommu an iommu_domain is still present, so the >>> copy code should work? >> The ARM DMA domain is a regular unmanaged domain owned by the ARM DMA code - >> in order to use any *other* domain, a user has to detach from that first >> (wrapped up in arm_iommu_detach_device() which also swizzles the DMA ops at >> the same time). Without set_platform_dma, that detach is now impossible >> (because no IOMMU API default domain exists either). > I was looking at this again, and for completeness, the reason it > doesn't have a default_domain is a bit subtle. > > The driver does support IOMMU_DOMAIN_DMA, and it will go through all > the iommu_group_alloc_default_domain() stuff with that.. > > But... __iommu_domain_alloc() calls iommu_get_dma_cookie() which will > be wired to fail on ARM32, so the core code nixes the IOMMU_DOMAIN_DMA > after the driver successfully created it. > > I wonder if that is actually what we want? As it seems like it would > be OK for ARM32 to have, effectively, a permanently empty unmanaged > domain as the default_domain? > > If instead of failing due to iommu_get_dma_cookie() we set the type to > UNMANAGED and make that the default domain it could fix all of this, > even for all the other ARM32 drivers that haven't reported this yet? > > Alternatively since we taught these drivers to support IDENTITY, we > should be able to remove the set_platform_dma_ops and instead > implement for ARM32 def_domain_type fixed to return IDENTITY? Maybe it would be easier to simply switch ARM32 to generic IOMMU-DMA layer? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
