Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On Fri, Sep 29, 2023 at 06:25:21PM +0100, Robin Murphy wrote: > On 29/09/2023 4:45 pm, Will Deacon wrote: > > On Mon, Sep 25, 2023 at 06:54:42PM +0100, Robin Murphy wrote: > > > On 2023-04-10 19:52, Dmitry Baryshkov wrote: > > > > If the Adreno SMMU is dma-coherent, allocation will fail unless we > > > > disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the > > > > coherent SMMUs (like we have on sm8350 platform). > > > > > > Hmm, but is it right that it should fail in the first place? The fact is > > > that if the SMMU is coherent then walks *will* be outer-WBWA, so I > > > honestly > > > can't see why the io-pgtable code is going out of its way to explicitly > > > reject a request to give them the same attribute it's already giving then > > > anyway :/ > > > > > > Even if the original intent was for the quirk to have an over-specific > > > implication of representing inner-NC as well, that hardly seems useful if > > > what we've ended up with in practice is a nonsensical-looking check in one > > > place and then a weird hacky bodge in another purely to work around it. > > > > > > Does anyone know a good reason why this is the way it is? > > > > I think it was mainly because the quick doesn't make sense for a coherent > > page-table walker and we could in theory use that bit for something else > > in that case. > > Yuck, even if we did want some horrible notion of quirks being conditional > on parts of the config rather than just the format, then the users would > need to be testing for the same condition as the pagetable code itself (i.e. > cfg->coherent_walk), rather than hoping some other property of something > else indirectly reflects the right information - e.g. there'd be no hope of > backporting this particular bodge before 5.19 where the old > iommu_capable(IOMMU_CAP_CACHE_COHERENCY) always returned true, and in future > we could conceivably support coherent SMMUs being configured for > non-coherent walks on a per-domain basis. That doesn't sound like an insurmountable problem to me. Either a bunch of other stuff has to be backported as well, or the msm_iommu driver can fish the pgtable configuration out of the SMMU, like it already does elsewhere. > Furthermore, if we did overload a flag to have multiple meanings, then we'd > have no way of knowing which one the caller was actually expecting, thus the > illusion of being able to validate calls in the meantime isn't necessarily > as helpful as it seems, particularly in a case where the "wrong" > interpretation would be to have no effect anyway. Mostly though I'd hope > that if we ever got anywhere near the point of running out of quirk bits > we'd have already realised that it's time for a better interface :( Although I agree that practically I can't see us reusing quirk bits, I do much prefer that we reject quirks that don't make sense. Yes, in this case it happens that the quirk is expressing something which is already true for the coherent walker, but that feels like a special case to me rather than something which is likely to be true in general, for example, the system cache quirk proposed by Qualcomm to expose the unused inner-NC-outer-WBWRA MAIR configuration. Implicitly accepting quirks also makes it more difficult if we wanted to change the default configuration in future; for example if we wanted to adjust the default allocation hints. So I'd prefer to leave the page-table code as-is. Will
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On 29/09/2023 4:45 pm, Will Deacon wrote: On Mon, Sep 25, 2023 at 06:54:42PM +0100, Robin Murphy wrote: On 2023-04-10 19:52, Dmitry Baryshkov wrote: If the Adreno SMMU is dma-coherent, allocation will fail unless we disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the coherent SMMUs (like we have on sm8350 platform). Hmm, but is it right that it should fail in the first place? The fact is that if the SMMU is coherent then walks *will* be outer-WBWA, so I honestly can't see why the io-pgtable code is going out of its way to explicitly reject a request to give them the same attribute it's already giving then anyway :/ Even if the original intent was for the quirk to have an over-specific implication of representing inner-NC as well, that hardly seems useful if what we've ended up with in practice is a nonsensical-looking check in one place and then a weird hacky bodge in another purely to work around it. Does anyone know a good reason why this is the way it is? I think it was mainly because the quick doesn't make sense for a coherent page-table walker and we could in theory use that bit for something else in that case. Yuck, even if we did want some horrible notion of quirks being conditional on parts of the config rather than just the format, then the users would need to be testing for the same condition as the pagetable code itself (i.e. cfg->coherent_walk), rather than hoping some other property of something else indirectly reflects the right information - e.g. there'd be no hope of backporting this particular bodge before 5.19 where the old iommu_capable(IOMMU_CAP_CACHE_COHERENCY) always returned true, and in future we could conceivably support coherent SMMUs being configured for non-coherent walks on a per-domain basis. Furthermore, if we did overload a flag to have multiple meanings, then we'd have no way of knowing which one the caller was actually expecting, thus the illusion of being able to validate calls in the meantime isn't necessarily as helpful as it seems, particularly in a case where the "wrong" interpretation would be to have no effect anyway. Mostly though I'd hope that if we ever got anywhere near the point of running out of quirk bits we'd have already realised that it's time for a better interface :( Based on that, I think that when I do get round to needing to touch this code, I'll propose just streamlining the whole quirk. Cheers, Robin.
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On Mon, Sep 25, 2023 at 06:54:42PM +0100, Robin Murphy wrote: > On 2023-04-10 19:52, Dmitry Baryshkov wrote: > > If the Adreno SMMU is dma-coherent, allocation will fail unless we > > disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the > > coherent SMMUs (like we have on sm8350 platform). > > Hmm, but is it right that it should fail in the first place? The fact is > that if the SMMU is coherent then walks *will* be outer-WBWA, so I honestly > can't see why the io-pgtable code is going out of its way to explicitly > reject a request to give them the same attribute it's already giving then > anyway :/ > > Even if the original intent was for the quirk to have an over-specific > implication of representing inner-NC as well, that hardly seems useful if > what we've ended up with in practice is a nonsensical-looking check in one > place and then a weird hacky bodge in another purely to work around it. > > Does anyone know a good reason why this is the way it is? I think it was mainly because the quick doesn't make sense for a coherent page-table walker and we could in theory use that bit for something else in that case. Will
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On 2023-04-10 19:52, Dmitry Baryshkov wrote:
If the Adreno SMMU is dma-coherent, allocation will fail unless we
disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the
coherent SMMUs (like we have on sm8350 platform).
Hmm, but is it right that it should fail in the first place? The fact is
that if the SMMU is coherent then walks *will* be outer-WBWA, so I
honestly can't see why the io-pgtable code is going out of its way to
explicitly reject a request to give them the same attribute it's already
giving then anyway :/
Even if the original intent was for the quirk to have an over-specific
implication of representing inner-NC as well, that hardly seems useful
if what we've ended up with in practice is a nonsensical-looking check
in one place and then a weird hacky bodge in another purely to work
around it.
Does anyone know a good reason why this is the way it is?
[ just came across this code in the tree while trying to figure out what
to do with iommu_set_pgtable_quirks()... ]
Thanks,
Robin.
Fixes: 54af0ceb7595 ("arm64: dts: qcom: sm8350: add GPU, GMU, GPU CC and SMMU
nodes")
Reported-by: David Heidelberg
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 2942d2548ce6..f74495dcbd96 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1793,7 +1793,8 @@ a6xx_create_address_space(struct msm_gpu *gpu, struct
platform_device *pdev)
* This allows GPU to set the bus attributes required to use system
* cache on behalf of the iommu page table walker.
*/
- if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice))
+ if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice) &&
+ !device_iommu_capable(>dev, IOMMU_CAP_CACHE_COHERENCY))
quirks |= IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
return adreno_iommu_create_address_space(gpu, pdev, quirks);
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On Mon, Apr 10, 2023 at 09:52:26PM +0300, Dmitry Baryshkov wrote:
> If the Adreno SMMU is dma-coherent, allocation will fail unless we
> disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the
> coherent SMMUs (like we have on sm8350 platform).
>
> Fixes: 54af0ceb7595 ("arm64: dts: qcom: sm8350: add GPU, GMU, GPU CC and SMMU
> nodes")
> Reported-by: David Heidelberg
I don't see this patch in linux-next yet, would it be possible to get it
picked up? I ran into this on sc8280xp as well.
Regards,
Bjorn
> Signed-off-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 2942d2548ce6..f74495dcbd96 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1793,7 +1793,8 @@ a6xx_create_address_space(struct msm_gpu *gpu, struct
> platform_device *pdev)
>* This allows GPU to set the bus attributes required to use system
>* cache on behalf of the iommu page table walker.
>*/
> - if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice))
> + if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice) &&
> + !device_iommu_capable(>dev, IOMMU_CAP_CACHE_COHERENCY))
> quirks |= IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
>
> return adreno_iommu_create_address_space(gpu, pdev, quirks);
> --
> 2.39.2
>
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
On 10.04.2023 20:52, Dmitry Baryshkov wrote:
> If the Adreno SMMU is dma-coherent, allocation will fail unless we
> disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the
> coherent SMMUs (like we have on sm8350 platform).
>
> Fixes: 54af0ceb7595 ("arm64: dts: qcom: sm8350: add GPU, GMU, GPU CC and SMMU
> nodes")
> Reported-by: David Heidelberg
> Signed-off-by: Dmitry Baryshkov
> Tested-by: David Heidelberg
> ---
Also required for SM8450 (and others)
Reviewed-by: Konrad Dybcio
Tested-by: Konrad Dybcio # SM8450 HDK
Cc:
Konrad
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 2942d2548ce6..f74495dcbd96 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -1793,7 +1793,8 @@ a6xx_create_address_space(struct msm_gpu *gpu, struct
> platform_device *pdev)
>* This allows GPU to set the bus attributes required to use system
>* cache on behalf of the iommu page table walker.
>*/
> - if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice))
> + if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice) &&
> + !device_iommu_capable(>dev, IOMMU_CAP_CACHE_COHERENCY))
> quirks |= IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
>
> return adreno_iommu_create_address_space(gpu, pdev, quirks);
Re: [PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
Thank you!
Tested-by: David Heidelberg
On 10/04/2023 20:52, Dmitry Baryshkov wrote:
If the Adreno SMMU is dma-coherent, allocation will fail unless we
disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the
coherent SMMUs (like we have on sm8350 platform).
Fixes: 54af0ceb7595 ("arm64: dts: qcom: sm8350: add GPU, GMU, GPU CC and SMMU
nodes")
Reported-by: David Heidelberg
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 2942d2548ce6..f74495dcbd96 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1793,7 +1793,8 @@ a6xx_create_address_space(struct msm_gpu *gpu, struct
platform_device *pdev)
* This allows GPU to set the bus attributes required to use system
* cache on behalf of the iommu page table walker.
*/
- if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice))
+ if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice) &&
+ !device_iommu_capable(>dev, IOMMU_CAP_CACHE_COHERENCY))
quirks |= IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
return adreno_iommu_create_address_space(gpu, pdev, quirks);
--
David Heidelberg
Consultant Software Engineer
[PATCH] drm/msm/a6xx: don't set IO_PGTABLE_QUIRK_ARM_OUTER_WBWA with coherent SMMU
If the Adreno SMMU is dma-coherent, allocation will fail unless we
disable IO_PGTABLE_QUIRK_ARM_OUTER_WBWA. Skip setting this quirk for the
coherent SMMUs (like we have on sm8350 platform).
Fixes: 54af0ceb7595 ("arm64: dts: qcom: sm8350: add GPU, GMU, GPU CC and SMMU
nodes")
Reported-by: David Heidelberg
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 2942d2548ce6..f74495dcbd96 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1793,7 +1793,8 @@ a6xx_create_address_space(struct msm_gpu *gpu, struct
platform_device *pdev)
* This allows GPU to set the bus attributes required to use system
* cache on behalf of the iommu page table walker.
*/
- if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice))
+ if (!IS_ERR_OR_NULL(a6xx_gpu->htw_llc_slice) &&
+ !device_iommu_capable(>dev, IOMMU_CAP_CACHE_COHERENCY))
quirks |= IO_PGTABLE_QUIRK_ARM_OUTER_WBWA;
return adreno_iommu_create_address_space(gpu, pdev, quirks);
--
2.39.2
