On Thu, Apr 13, 2023 at 12:03:55AM +0200, Marek Szyprowski wrote: > 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? It is the long term goal, yes In the short term it would be nice if we could at least keep the hacks out of the drivers Jason
