From: Robin Murphy <robin.murphy@arm.com> To: Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org Cc: Rob Clark <robdclark@chromium.org>, freedreno@lists.freedesktop.org, Will Deacon <will@kernel.org>, David Airlie <airlied@linux.ie>, linux-arm-msm@vger.kernel.org, Abhinav Kumar <quic_abhinavk@quicinc.com>, open list <linux-kernel@vger.kernel.org>, "iommu@lists.linux.dev" <iommu@lists.linux.dev>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Sean Paul <sean@poorly.run> Subject: Re: [PATCH 4/5] drm/msm: Use separate ASID for each set of pgtables Date: Mon, 22 Aug 2022 15:38:02 +0100 [thread overview] Message-ID: <4650e8ff-86e1-d65e-bf87-c785013f3e08@arm.com> (raw) In-Reply-To: <c8543ace-65cd-b8a8-499c-1b051867366b@arm.com> On 2022-08-22 14:52, Robin Murphy wrote: > On 2022-08-21 19:19, Rob Clark wrote: >> From: Rob Clark <robdclark@chromium.org> >> >> Optimize TLB invalidation by using different ASID for each set of >> pgtables. There can be scenarios where multiple processes end up >> with the same ASID (such as >256 processes using the GPU), but this >> is harmless, it will only result in some over-invalidation (but >> less over-invalidation compared to using ASID=0 for all processes) > > Um, if you're still relying on the GPU doing an invalidate-all-by-ASID > whenever it switches a TTBR, then there's only ever one ASID live in the > TLBs at once, so it really doesn't matter whether its value stays the > same or changes. This seems like a whole chunk of complexity to achieve > nothing :/ > > If you could actually use multiple ASIDs in a meaningful way to avoid > any invalidations, you'd need to do considerably more work to keep track > of reuse, and those races would probably be a lot less benign. Oh, and on top of that, if you did want to go down that route then chances are you'd then also want to start looking at using global mappings in TTBR1 to avoid increased TLB pressure from kernel buffers, and then we'd run up against some pretty horrid MMU-500 errata which so far I've been happy to ignore on the basis that Linux doesn't use global mappings. Spoiler alert: unless you can additionally convert everything to invalidate by VA, the workaround for #752357 most likely makes the whole idea moot. Robin. >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> drivers/gpu/drm/msm/msm_iommu.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >> b/drivers/gpu/drm/msm/msm_iommu.c >> index a54ed354578b..94c8c09980d1 100644 >> --- a/drivers/gpu/drm/msm/msm_iommu.c >> +++ b/drivers/gpu/drm/msm/msm_iommu.c >> @@ -33,6 +33,8 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu >> *mmu, u64 iova, >> size_t size) >> { >> struct msm_iommu_pagetable *pagetable = to_pagetable(mmu); >> + struct adreno_smmu_priv *adreno_smmu = >> + dev_get_drvdata(pagetable->parent->dev); >> struct io_pgtable_ops *ops = pagetable->pgtbl_ops; >> size_t unmapped = 0; >> @@ -43,7 +45,7 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu >> *mmu, u64 iova, >> size -= 4096; >> } >> - iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain); >> + adreno_smmu->tlb_inv_by_id(adreno_smmu->cookie, pagetable->asid); >> return (unmapped == size) ? 0 : -EINVAL; >> } >> @@ -147,6 +149,7 @@ static int msm_fault_handler(struct iommu_domain >> *domain, struct device *dev, >> struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent) >> { >> + static atomic_t asid = ATOMIC_INIT(1); >> struct adreno_smmu_priv *adreno_smmu = >> dev_get_drvdata(parent->dev); >> struct msm_iommu *iommu = to_msm_iommu(parent); >> struct msm_iommu_pagetable *pagetable; >> @@ -210,12 +213,14 @@ struct msm_mmu >> *msm_iommu_pagetable_create(struct msm_mmu *parent) >> pagetable->ttbr = ttbr0_cfg.arm_lpae_s1_cfg.ttbr; >> /* >> - * TODO we would like each set of page tables to have a unique ASID >> - * to optimize TLB invalidation. But iommu_flush_iotlb_all() will >> - * end up flushing the ASID used for TTBR1 pagetables, which is not >> - * what we want. So for now just use the same ASID as TTBR1. >> + * ASID 0 is used for kernel mapped buffers in TTBR1, which we >> + * do not need to invalidate when unmapping from TTBR0 pgtables. >> + * The hw ASID is at *least* 8b, but can be 16b. We just assume >> + * the worst: >> */ >> pagetable->asid = 0; >> + while (!pagetable->asid) >> + pagetable->asid = atomic_inc_return(&asid) & 0xff; >> return &pagetable->base; >> }
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Rob Clark <robdclark@gmail.com>, dri-devel@lists.freedesktop.org Cc: Rob Clark <robdclark@chromium.org>, David Airlie <airlied@linux.ie>, freedreno@lists.freedesktop.org, Abhinav Kumar <quic_abhinavk@quicinc.com>, open list <linux-kernel@vger.kernel.org>, Sean Paul <sean@poorly.run>, "iommu@lists.linux.dev" <iommu@lists.linux.dev>, linux-arm-msm@vger.kernel.org, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Will Deacon <will@kernel.org> Subject: Re: [PATCH 4/5] drm/msm: Use separate ASID for each set of pgtables Date: Mon, 22 Aug 2022 15:38:02 +0100 [thread overview] Message-ID: <4650e8ff-86e1-d65e-bf87-c785013f3e08@arm.com> (raw) In-Reply-To: <c8543ace-65cd-b8a8-499c-1b051867366b@arm.com> On 2022-08-22 14:52, Robin Murphy wrote: > On 2022-08-21 19:19, Rob Clark wrote: >> From: Rob Clark <robdclark@chromium.org> >> >> Optimize TLB invalidation by using different ASID for each set of >> pgtables. There can be scenarios where multiple processes end up >> with the same ASID (such as >256 processes using the GPU), but this >> is harmless, it will only result in some over-invalidation (but >> less over-invalidation compared to using ASID=0 for all processes) > > Um, if you're still relying on the GPU doing an invalidate-all-by-ASID > whenever it switches a TTBR, then there's only ever one ASID live in the > TLBs at once, so it really doesn't matter whether its value stays the > same or changes. This seems like a whole chunk of complexity to achieve > nothing :/ > > If you could actually use multiple ASIDs in a meaningful way to avoid > any invalidations, you'd need to do considerably more work to keep track > of reuse, and those races would probably be a lot less benign. Oh, and on top of that, if you did want to go down that route then chances are you'd then also want to start looking at using global mappings in TTBR1 to avoid increased TLB pressure from kernel buffers, and then we'd run up against some pretty horrid MMU-500 errata which so far I've been happy to ignore on the basis that Linux doesn't use global mappings. Spoiler alert: unless you can additionally convert everything to invalidate by VA, the workaround for #752357 most likely makes the whole idea moot. Robin. >> Signed-off-by: Rob Clark <robdclark@chromium.org> >> --- >> drivers/gpu/drm/msm/msm_iommu.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/msm_iommu.c >> b/drivers/gpu/drm/msm/msm_iommu.c >> index a54ed354578b..94c8c09980d1 100644 >> --- a/drivers/gpu/drm/msm/msm_iommu.c >> +++ b/drivers/gpu/drm/msm/msm_iommu.c >> @@ -33,6 +33,8 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu >> *mmu, u64 iova, >> size_t size) >> { >> struct msm_iommu_pagetable *pagetable = to_pagetable(mmu); >> + struct adreno_smmu_priv *adreno_smmu = >> + dev_get_drvdata(pagetable->parent->dev); >> struct io_pgtable_ops *ops = pagetable->pgtbl_ops; >> size_t unmapped = 0; >> @@ -43,7 +45,7 @@ static int msm_iommu_pagetable_unmap(struct msm_mmu >> *mmu, u64 iova, >> size -= 4096; >> } >> - iommu_flush_iotlb_all(to_msm_iommu(pagetable->parent)->domain); >> + adreno_smmu->tlb_inv_by_id(adreno_smmu->cookie, pagetable->asid); >> return (unmapped == size) ? 0 : -EINVAL; >> } >> @@ -147,6 +149,7 @@ static int msm_fault_handler(struct iommu_domain >> *domain, struct device *dev, >> struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent) >> { >> + static atomic_t asid = ATOMIC_INIT(1); >> struct adreno_smmu_priv *adreno_smmu = >> dev_get_drvdata(parent->dev); >> struct msm_iommu *iommu = to_msm_iommu(parent); >> struct msm_iommu_pagetable *pagetable; >> @@ -210,12 +213,14 @@ struct msm_mmu >> *msm_iommu_pagetable_create(struct msm_mmu *parent) >> pagetable->ttbr = ttbr0_cfg.arm_lpae_s1_cfg.ttbr; >> /* >> - * TODO we would like each set of page tables to have a unique ASID >> - * to optimize TLB invalidation. But iommu_flush_iotlb_all() will >> - * end up flushing the ASID used for TTBR1 pagetables, which is not >> - * what we want. So for now just use the same ASID as TTBR1. >> + * ASID 0 is used for kernel mapped buffers in TTBR1, which we >> + * do not need to invalidate when unmapping from TTBR0 pgtables. >> + * The hw ASID is at *least* 8b, but can be 16b. We just assume >> + * the worst: >> */ >> pagetable->asid = 0; >> + while (!pagetable->asid) >> + pagetable->asid = atomic_inc_return(&asid) & 0xff; >> return &pagetable->base; >> }
next prev parent reply other threads:[~2022-08-22 14:38 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-21 18:19 [PATCH 0/5] drm/msm+iommu/arm-smmu-qcom: tlbinv optimizations Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` [PATCH 1/5] iommu/arm-smmu-qcom: Fix indentation Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` [PATCH 2/5] iommu/arm-smmu-qcom: Provide way to access current TTBR0 Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` [PATCH 3/5] iommu/arm-smmu-qcom: Add private interface to tlbinv by ASID Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-21 18:19 ` [PATCH 4/5] drm/msm: Use separate ASID for each set of pgtables Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-22 13:52 ` Robin Murphy 2022-08-22 13:52 ` Robin Murphy 2022-08-22 14:38 ` Robin Murphy [this message] 2022-08-22 14:38 ` Robin Murphy 2022-08-21 18:19 ` [PATCH 5/5] drm/msm: Skip tlbinv on unmap from non-current pgtables Rob Clark 2022-08-21 18:19 ` Rob Clark 2022-08-24 17:46 ` Akhil P Oommen 2022-08-24 17:46 ` Akhil P Oommen 2022-08-24 19:02 ` Rob Clark 2022-08-24 19:02 ` Rob Clark 2022-08-25 18:12 ` Akhil P Oommen 2022-08-25 18:12 ` Akhil P Oommen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4650e8ff-86e1-d65e-bf87-c785013f3e08@arm.com \ --to=robin.murphy@arm.com \ --cc=airlied@linux.ie \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=iommu@lists.linux.dev \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=robdclark@chromium.org \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.