On Fri, Jul 03, 2020 at 09:04:49AM -0700, Rob Clark wrote:
On Fri, Jul 3, 2020 at 7:53 AM Sai Prakash Ranjan saiprakash.ranjan@codeaurora.org wrote:
Hi Will,
On 2020-07-03 19:07, Will Deacon wrote:
On Mon, Jun 29, 2020 at 09:22:50PM +0530, Sai Prakash Ranjan wrote:
diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index f455c597f76d..bd1d58229cc2 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -218,6 +218,9 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, iova |= GENMASK_ULL(63, 49);
- if (mmu->features & MMU_FEATURE_USE_SYSTEM_CACHE)
prot |= IOMMU_SYS_CACHE_ONLY;Given that I think this is the only user of IOMMU_SYS_CACHE_ONLY, then it looks like it should actually be a property on the domain because we never need to configure it on a per-mapping basis within a domain, and therefore it shouldn't be exposed by the IOMMU API as a prot flag.
Do you agree?
GPU being the only user is for now, but there are other clients which can use this. Plus how do we set the memory attributes if we do not expose this as prot flag?
It does appear that the downstream kgsl driver sets this for basically all mappings.. well there is some conditional stuff around DOMAIN_ATTR_USE_LLC_NWA but it seems based on the property of the domain. (Jordan may know more about what that is about.) But looks like there are a lot of different paths into iommu_map in kgsl so I might have missed something.
Downstream does set it universally. There are some theoretical use cases where it might be beneficial to set it on a per-mapping basis with a bunch of hinting from userspace and nobody has tried to characterize this on real hardware so it is not clear to me if it is worth it.
I think a domain wide attribute works for now but if a compelling per-mapping use case does comes down the pipeline we need to have a backup in mind - possibly a prot flag to disable NWA?
Jordan
Assuming there isn't some case where we specifically don't want to use the system cache for some mapping, I think it could be a domain attribute that sets an io_pgtable_cfg::quirks flag
BR, -R