Re: [PATCH 02/20] iommu/terga-gart: Replace set_platform_dma_ops() with IOMMU_DOMAIN_PLATFORM

[Dmitry Osipenko <digetx@gmail.com>]

03.05.2023 20:20, Jason Gunthorpe пишет:
> On Wed, May 03, 2023 at 04:43:53PM +0200, Thierry Reding wrote:
> 
>>> The only thing it does is cause dma-iommu.c in ARM64 to use the
>>> dma-ranges from OF instead of the domain aperture. sprd has no
>>> dma-ranges in arch/arm64/boot/dts/sprd.
>>>
>>> Further, sprd hard fails any map attempt outside the aperture, so it
>>> looks like a bug if the OF somehow chooses a wider aperture as
>>> dma-iommu.c will start failing maps.
>>
>> That all sounds odd. of_dma_configure_id() already sets up the DMA mask
>> based on dma-ranges and the DMA API uses that to restrict what IOVA any
>> buffers can get mapped to for a given device.
> 
> Yes, and after it sets up the mask it also passes that range down like this:
> 
>  of_dma_configure_id():
> 	end = dma_start + size - 1;
> 	mask = DMA_BIT_MASK(ilog2(end) + 1);
> 	dev->coherent_dma_mask &= mask;
> 
> 	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
> 
> Which eventually goes to:
> 
>  iommu_setup_dma_ops()
>  iommu_dma_init_domain()
> 
> Which then does:
> 
> 	if (domain->geometry.force_aperture) {
> 
> And if not set uses the dma_start/size parameter as the actual
> aperture. (!?)
> 
> Since sprd does this in the iommu driver:
> 
> 	dom->domain.geometry.aperture_start = 0;
> 	dom->domain.geometry.aperture_end = SZ_256M - 1;
> 
> And it is serious about enforcing it during map:
> 
> 	unsigned long start = domain->geometry.aperture_start;
> 	unsigned long end = domain->geometry.aperture_end;
> 
> 	if (iova < start || (iova + size) > (end + 1)) {
> 			return -EINVAL;
> 
> We must see the dma_start/size parameter be a subset of the aperture
> or eventually dma-iommu.c will see map failures.
> 
> I can't see how this is can happen, so it looks like omitting
> force_aperture is a mistake not a deliberate choice. I'll make a patch
> and see if the SPRD folks have any comment. If there is no reason I
> can go ahead and purge force_aperture and all the dma_start/size
> passing through arch_setup_dma_ops().
> 
>>> Thus, I propose we just remove the whole thing. All drivers must set
>>> an aperture and the aperture is the pure HW capability to map an
>>> IOPTE at that address. ie it reflects the design of the page table
>>> itself and nothing else.
>>
>> Yeah, that sounds reasonable. If the aperture represents what the IOMMU
>> supports. Together with each device's DMA mask we should have everything
>> we need.
> 
> Arguably we should respect the dma-ranges as well, but I think that
> should come up from the iommu driver via the get_resv_regions() which
> is the usual place we return FW originated information.
> 
>> For Tegra GART I think there's indeed no use-cases at the moment. Dmitry
>> had at one point tried to make use of it because it can be helpful on
>> some of the older devices that were very memory-constrained. That
>> support never made it upstream because it required significant changes
>> in various places, if I recall correctly. For anything with a decent
>> enough amount of RAM, CMA is usually a better option.
> 
> So the actual use case of this HW is to linearize buffers? ie it is a
> general scatter/gather engine?
> 
>> This has occasionally come up in the past and I seem to remember that it
>> had once been proposed to simply remove tegra-gart and there had been no
>> objections. Adding Dmitry, if he doesn't have objections to remaving it,
>> neither do I.
> 
> Dmitry, please say yes and I will remove it instead of trying to carry
> it. The driver is almost 10 years old at this point, I'm skeptical
> anyone will need it on a 6.2 era kernel..

You probably missed that support for many of 10 years old Tegra2/3
devices was added to kernel during last years.

This GART isn't used by upstream DRM driver, but it's used by downstream
kernel which uses alternative Tegra DRM driver that works better for
older hardware.

If it's too much burden to maintain this driver, then feel free to
remove it and I'll continue maintaining it in downstream myself.
Otherwise I can test your changes if needed.