Re: [PATCHv3 7/7] drm/msm/a6xx: Add support for using system cache(LLC)

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
[off-list ref] wrote:

quoted

Hi Will,

On 2020-07-03 19:07, Will Deacon wrote:

quoted

On Mon, Jun 29, 2020 at 09:22:50PM +0530, Sai Prakash Ranjan wrote:

quoted

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

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel