Re: [PATCH 16/18] staging/media/tegra-vde: Clean up IOMMU workaround

[Dmitry Osipenko]

24.08.2020 17:01, Robin Murphy пишет:
...
>> Robin, thank you very much for the clarifications!
>>
>> In accordance to yours comments, this patch can't be applied until Tegra
>> SMMU will support IOMMU_DOMAIN_IDENTITY and implement def_domain_type()
>> callback that returns IOMMU_DOMAIN_IDENTITY for the VDE device.
>>
>> Otherwise you're breaking the VDE driver because
>> dma_buf_map_attachment() [1] returns the IOMMU SGT of the implicit
>> domain which is then mapped into the VDE's explicit domain [2], and this
>> is a nonsense.
> 
> It's true that iommu_dma_ops will do some work in the unattached default
> domain, but non-coherent cache maintenance will still be performed
> correctly on the underlying memory, which is really all that you care
> about for this case. As for tegra_vde_iommu_map(), that seems to do the
> right thing in only referencing the physical side of the scatterlist
> (via iommu_map_sg()) and ignoring the DMA side, so things ought to work
> out OK even if it is a little non-obvious.

I'll need to double-check this, it's indeed not clear to me right now.

I see that if Tegra DRM driver uses implicit IOMMU domain, then when VDE
driver imports DMA-buf from Terga DRM and the imported buffer will be
auto-mapped to the implicit VDE IOVA [1].

[1]
https://elixir.bootlin.com/linux/v5.9-rc2/source/drivers/gpu/drm/tegra/gem.c#L574

>> Hence, either VDE driver should bypass iommu_dma_ops from the start or
>> it needs a way to kick out the ops, like it does this using ARM's
>> arm_iommu_detach_device().
>>
>>
>> The same applies to the Tegra GPU devices, otherwise you're breaking
>> them as well because Tegra DRM is sensible to implicit vs explicit
>> domain.
> 
> Note that Tegra DRM will only be as broken as its current state on
> arm64, and I was under the impression that that was OK now - at least I
> don't recall seeing any complaints since 43c5bf11a610. Although that
> commit and the one before it are resolving the scalability issue that
> they describe, it was very much in my mind at the time that they also
> have the happy side-effect described above - the default domain isn't
> *completely* out of the way, but it's far enough that sensible cases
> should be able to work as expected.

The Tegra DRM has a very special quirk for ARM32 that was added in this
commit [2] and driver relies on checking of whether explicit or implicit
IOMMU is used in order to activate the quirk.

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=273da5a046965ccf0ec79eb63f2d5173467e20fa

Once the implicit IOMMU is used for the DRM driver, the quirk no longer
works (if I'm not missing something). This problem needs to be resolved
before implicit IOMMU could be used by the Tegra DRM on ARM32.

>> BTW, I tried to apply this series and T30 doesn't boot anymore. I don't
>> have more info for now.
> 
> Yeah, I'm still trying to get to the bottom of whether it's actually
> working as intended at all, even on my RK3288. So far my debugging
> instrumentation has been confusingly inconclusive :/

Surely it will take some time to resolve all the problems and it's great
that you're pushing this work!

I'll try to help with fixing the ARM32 Tegra side of the problems. I
added this to my "TODO" list and should be able to take a closer look
during of this/next weeks!