drm: panfrost: Coherency support

Midgard GPUs have ACE-Lite master interfaces which allows systems to integrate them in an I/O-coherent manner. It seems that from the GPU's viewpoint, the rest of the system is its outer shareable domain, and so even when snoop signals are wired up, they are only emitted for outer shareable accesses. As such, setting the TTBR_SHARE_OUTER bit does indeed get coherent pagetable walks working nicely for the coherent T620 in the Arm Juno SoC.

Reviewed-by Steven Price <[email protected]>

Tested-by Neil Armstrong <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable-arm.c +7 -1

index dc7bcf858b6d..b4072a18e45d 100644
@@ -440,7 +440,13 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 
-	if (prot & IOMMU_CACHE)
+	/*
+	 * Also Mali has its own notions of shareability wherein its Inner
+	 * domain covers the cores within the GPU, and its Outer domain is
+	 * "outside the GPU" (i.e. either the Inner or System domain in CPU
+	 * terms, depending on coherency).
+	 */
+	if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE)
 		pte |= ARM_LPAE_PTE_SH_IS;
 	else
 		pte |= ARM_LPAE_PTE_SH_OS;

Midgard GPUs have ACE-Lite master interfaces which allows systems to integrate them in an I/O-coherent manner. It seems that from the GPU's viewpoint, the rest of the system is its outer shareable domain, and so even when snoop signals are wired up, they are only emitted for outer shareable accesses. As such, setting the TTBR_SHARE_OUTER bit does indeed get coherent pagetable walks working nicely for the coherent T620 in the Arm Juno SoC.

Reviewed-by Steven Price <[email protected]>

Tested-by Neil Armstrong <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable-arm.c +0 -1

index dc7bcf858b6d..b4072a18e45d 100644
@@ -440,7 +440,13 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 
-	if (prot & IOMMU_CACHE)

Quoted 23 lines · On Tue, 22 Sep 2020 15:16:48 +0100 Robin Murphy <[email protected]> wrote:

Midgard GPUs have ACE-Lite master interfaces which allows systems to integrate them in an I/O-coherent manner. It seems that from the GPU's viewpoint, the rest of the system is its outer shareable domain, and so even when snoop signals are wired up, they are only emitted for outer shareable accesses. As such, setting the TTBR_SHARE_OUTER bit does indeed get coherent pagetable walks working nicely for the coherent T620 in the Arm Juno SoC.

Reviewed-by Steven Price <[email protected]>

Tested-by Neil Armstrong <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/io-pgtable-arm.c | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable-arm.c +0 -0

index dc7bcf858b6d..b4072a18e45d 100644
@@ -440,7 +440,13 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
  				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
  	}
 

drivers/iommu/io-pgtable-arm.c static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, · line 452

Quoted 11 lines

drivers/iommu/io-pgtable-arm.c +7 -1

-	if (prot & IOMMU_CACHE)
+	/*
+	 * Also Mali has its own notions of shareability wherein its Inner
+	 * domain covers the cores within the GPU, and its Outer domain is
+	 * "outside the GPU" (i.e. either the Inner or System domain in CPU
+	 * terms, depending on coherency).
+	 */
+	if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE)
  		pte |= ARM_LPAE_PTE_SH_IS;
  	else
  		pte |= ARM_LPAE_PTE_SH_OS;

Actually, it still doesn't work on s922x :-/. For it to work I correctly, I need to drop the outer shareable flag here.

drivers/iommu/io-pgtable-arm.c arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie) · line 1063

Quoted 10 lines

drivers/iommu/io-pgtable-arm.c +3 -0

@@ -1049,6 +1055,9 @@ arm_mali_lpae_alloc_pgtable(struct io_pgtable_cfg *cfg, void *cookie)
  	cfg->arm_mali_lpae_cfg.transtab = virt_to_phys(data->pgd) |
  					  ARM_MALI_LPAE_TTBR_READ_INNER |
  					  ARM_MALI_LPAE_TTBR_ADRMODE_TABLE;
+	if (cfg->coherent_walk)
+		cfg->arm_mali_lpae_cfg.transtab |= ARM_MALI_LPAE_TTBR_SHARE_OUTER;
+
  	return &data->iop;
 
  out_free_data:

Quoted 26 lines · Boris Brezillon

Quoted 23 lines · On Tue, 22 Sep 2020 15:16:48 +0100 Robin Murphy <[email protected]> wrote:

Midgard GPUs have ACE-Lite master interfaces which allows systems to integrate them in an I/O-coherent manner. It seems that from the GPU's viewpoint, the rest of the system is its outer shareable domain, and so even when snoop signals are wired up, they are only emitted for outer shareable accesses. As such, setting the TTBR_SHARE_OUTER bit does indeed get coherent pagetable walks working nicely for the coherent T620 in the Arm Juno SoC.

Reviewed-by Steven Price <[email protected]>

Tested-by Neil Armstrong <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/io-pgtable-arm.c | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

drivers/iommu/io-pgtable-arm.c +0 -0

index dc7bcf858b6d..b4072a18e45d 100644
@@ -440,7 +440,13 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
  				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
  	}
 

Quoted 14 lines

drivers/iommu/io-pgtable-arm.c static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data, · line 452

Quoted 11 lines

drivers/iommu/io-pgtable-arm.c +7 -1

-	if (prot & IOMMU_CACHE)
+	/*
+	 * Also Mali has its own notions of shareability wherein its Inner
+	 * domain covers the cores within the GPU, and its Outer domain is
+	 * "outside the GPU" (i.e. either the Inner or System domain in CPU
+	 * terms, depending on coherency).
+	 */
+	if (prot & IOMMU_CACHE && data->iop.fmt != ARM_MALI_LPAE)
  		pte |= ARM_LPAE_PTE_SH_IS;
  	else
  		pte |= ARM_LPAE_PTE_SH_OS;  

Actually, it still doesn't work on s922x :-/. For it to work I correctly, I need to drop the outer shareable flag here.

The logic here does seem a bit odd. Originally it was:

IOMMU_CACHE -> Inner shared (value 3)
!IOMMU_CACHE -> Outer shared (value 2)

For Mali we're forcing everything to the second option. But Mali being Mali doesn't do things the same as LPAE, so for Mali we have:

0 - not shared
1 - reserved
2 - inner(*) and outer shareable
3 - inner shareable only

(*) where "inner" means internal to the GPU, and "outer" means shared with the CPU "inner". Very confusing!

So originally we had:
IOMMU_CACHE -> not shared with CPU (only internally in the GPU) !IOMMU_CACHE -> shared with CPU

The change above gets us to "always shared", dropping the SH_OS bit would get us to not even shareable between cores (which doesn't sound like what we want).

It's not at all clear to me why the change helps, but I suspect we want at least "inner" shareable.

Hi all,

Here's a quick v2 with the tags so far picked up and some inline commentary about the shareability domains for the pagetable code.

Robin.

Robin Murphy (3):
iommu/io-pgtable-arm: Support coherency for Mali LPAE drm/panfrost: Support cache-coherent integrations
arm64: dts: meson: Describe G12b GPU as coherent

 arch/arm64/boot/dts/amlogic/meson-g12b.dtsi |  4 ++++
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c     |  2 ++
 drivers/gpu/drm/panfrost/panfrost_gem.c     |  2 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c     |  1 +
 drivers/iommu/io-pgtable-arm.c              | 11 ++++++++++-
 6 files changed, 20 insertions(+), 1 deletion(-)

Signature · 1 line

2.28.0.dirty

Hi all,

Here's a quick v2 with the tags so far picked up and some inline commentary about the shareability domains for the pagetable code.

Robin.

Robin Murphy (3):
iommu/io-pgtable-arm: Support coherency for Mali LPAE drm/panfrost: Support cache-coherent integrations
arm64: dts: meson: Describe G12b GPU as coherent

 arch/arm64/boot/dts/amlogic/meson-g12b.dtsi |  4 ++++
 drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
 drivers/gpu/drm/panfrost/panfrost_drv.c     |  2 ++
 drivers/gpu/drm/panfrost/panfrost_gem.c     |  2 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c     |  1 +
 drivers/iommu/io-pgtable-arm.c              | 11 ++++++++++-
 6 files changed, 20 insertions(+), 1 deletion(-)

Hi all,

Here's a quick v2 with the tags so far picked up and some inline commentary about the shareability domains for the pagetable code.

Robin.

Robin Murphy (3):
iommu/io-pgtable-arm: Support coherency for Mali LPAE drm/panfrost: Support cache-coherent integrations
arm64: dts: meson: Describe G12b GPU as coherent

  arch/arm64/boot/dts/amlogic/meson-g12b.dtsi |  4 ++++
  drivers/gpu/drm/panfrost/panfrost_device.h  |  1 +
  drivers/gpu/drm/panfrost/panfrost_drv.c     |  2 ++
  drivers/gpu/drm/panfrost/panfrost_gem.c     |  2 ++
  drivers/gpu/drm/panfrost/panfrost_mmu.c     |  1 +
  drivers/iommu/io-pgtable-arm.c              | 11 ++++++++++-
  6 files changed, 20 insertions(+), 1 deletion(-)

Hi Greg,

Quoted 5 lines · Robin Murphy

Hi all,

Here's a quick v2 with the tags so far picked up and some inline commentary about the shareability domains for the pagetable code.

Quoted 7 lines

Robin.

Robin Murphy (3):
iommu/io-pgtable-arm: Support coherency for Mali LPAE drm/panfrost: Support cache-coherent integrations
arm64: dts: meson: Describe G12b GPU as coherent

Please would you consider applying these patches to 5.10 stable? The mainline commit IDs are now:

728da60da7c1 iommu/io-pgtable-arm: Support coherency for Mali LPAE 268af50f38b1 drm/panfrost: Support cache-coherent integrations 03544505cb10 arm64: dts: meson: Describe G12b GPU as coherent

and I've checked that they cherry-pick to the current 5.10.y branch (5.10.12) cleanly.

Amlogic-based boards that require this support are quite popular, and end-users are now starting to run into the weird behaviour that ensues without it, which is all to easy to misattribute to the userspace driver in Mesa, e.g. [1],[2]. Fortunately 5.10 also happens to be the first kernel version to start probing the particular GPU models on these SoCs anyway, and I'm not aware of any other significant systems that are affected, so I don't believe we will need to worry about any other stable versions.