iommu/drm/msm: Allow non-coherent masters to use system cache

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		else if (prot & IOMMU_CACHE)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_LLC)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 
 	if (prot & IOMMU_CACHE)

include/linux/iommu.h +0 -0

index ffaa389ea128..1f82057df531 100644

Quoted 39 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct 
 arm_lpae_io_pgtable *data,
 		else if (prot & IOMMU_CACHE)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_LLC)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 

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

 	if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set 

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will introduce a mismatched virtual alias with the CPU (we don't even have a MAIR set up for this memory type). Now, we also have that issue for the PTW, but since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However, if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

So I can take patch (1) as a trivial rename, but unfortunately I think this needs more thought before exposing it beyond the PTW.

Quoted 52 lines · Will Deacon

Quoted 40 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte
 arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		else if (prot & IOMMU_CACHE)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_LLC)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 

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

 	if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR

Quoted 9 lines

set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Can't we add the syscached memory type similar to what is done on android?

Quoted 63 lines · Sai Prakash Ranjan

Quoted 61 lines · Will Deacon

Quoted 40 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte
 arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
          else if (prot & IOMMU_CACHE)
                  pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
                          << ARM_LPAE_PTE_ATTRINDX_SHIFT);
+         else if (prot & IOMMU_LLC)
+                 pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+                         << ARM_LPAE_PTE_ATTRINDX_SHIFT);
  }
 

if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV       (1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Quoted 67 lines · Will Deacon

Quoted 64 lines · Sai Prakash Ranjan

Quoted 52 lines · Will Deacon

Quoted 40 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte
 arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
          else if (prot & IOMMU_CACHE)
                  pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
                          << ARM_LPAE_PTE_ATTRINDX_SHIFT);
+         else if (prot & IOMMU_LLC)
+                 pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+                         << ARM_LPAE_PTE_ATTRINDX_SHIFT);
  }
 

if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV       (1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR

Quoted 9 lines

set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Can't we add the syscached memory type similar to what is done on android?

Quoted 1 line

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

BR,
-R

Quoted 68 lines · Rob Clark

Quoted 67 lines · Will Deacon

Quoted 64 lines · Sai Prakash Ranjan

Quoted 52 lines · Will Deacon

Quoted 40 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte
 arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
          else if (prot & IOMMU_CACHE)
                  pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
                          << ARM_LPAE_PTE_ATTRINDX_SHIFT);
+         else if (prot & IOMMU_LLC)
+                 pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+                         << ARM_LPAE_PTE_ATTRINDX_SHIFT);
  }
 

if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV       (1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR

Quoted 9 lines

set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Can't we add the syscached memory type similar to what is done on android?

Quoted 8 lines

Quoted 1 line

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

BR,
-R

LLC/system cache doesn't have a relationship with the CPU cache. Its just a little accelerator that sits on the connection from the GPU to DDR and caches accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 68 lines · Rob Clark

Quoted 67 lines · Will Deacon

Quoted 64 lines · Sai Prakash Ranjan

Quoted 52 lines · Will Deacon

Quoted 40 lines · Sai Prakash Ranjan

Add a new page protection flag IOMMU_LLC which can be used by non-coherent masters to set cacheable memory attributes for an outer level of cache called as last-level cache or system cache. Initial user of this page protection flag is the adreno gpu and then can later be used by other clients such as video where this can be used for per-buffer based mapping.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 include/linux/iommu.h          | 6 ++++++
 2 files changed, 9 insertions(+)

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

index 7439ee7fdcdb..ebe653ef601b 100644
@@ -415,6 +415,9 @@ static arm_lpae_iopte
 arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
          else if (prot & IOMMU_CACHE)
                  pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
                          << ARM_LPAE_PTE_ATTRINDX_SHIFT);
+         else if (prot & IOMMU_LLC)
+                 pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+                         << ARM_LPAE_PTE_ATTRINDX_SHIFT);
  }
 

if (prot & IOMMU_CACHE)

include/linux/iommu.h +2 -0

index ffaa389ea128..1f82057df531 100644
@@ -31,6 +31,12 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV       (1 << 5)
+/*
+ * Non-coherent masters can use this page protection flag to set

Quoted 6 lines

cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR

Quoted 9 lines

set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Can't we add the syscached memory type similar to what is done on android?

Quoted 8 lines

Quoted 1 line

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

BR,
-R

LLC/system cache doesn't have a relationship with the CPU cache. Its just a little accelerator that sits on the connection from the GPU to DDR and caches accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 44 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

Quoted 4 lines

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

Quoted 50 lines · Sai Prakash Ranjan

Quoted 48 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 2 lines

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback).

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Cheers,

Will

Quoted 52 lines · Will Deacon

Quoted 50 lines · Sai Prakash Ranjan

Quoted 48 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 8 lines

Quoted 2 lines

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Currently userspace asks for everything WC, so pgprot_writecombine()

The kernel doesn't enforce this, but so far provides no UAPI to do anything useful with non-coherent cached mappings (although there is interest to support this)

BR,
-R

Quoted 11 lines

Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback).

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Cheers,

Will

Quoted 63 lines · Rob Clark

Quoted 52 lines · Will Deacon

Quoted 50 lines · Sai Prakash Ranjan

Quoted 48 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 8 lines

Quoted 2 lines

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Quoted 6 lines

Currently userspace asks for everything WC, so pgprot_writecombine()

The kernel doesn't enforce this, but so far provides no UAPI to do anything useful with non-coherent cached mappings (although there is interest to support this)

btw, I'm looking at a benchmark (gl_driver2_off) where (after some other in-flight optimizations land) we end up bottlenecked on writing to WC cmdstream buffers. I assume in the current state, WC goes all the way to main memory rather than just to system cache?

BR,
-R

Quoted 14 lines

BR,
-R

Quoted 11 lines

Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback).

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Cheers,

Will

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Currently userspace asks for everything WC, so pgprot_writecombine()

The kernel doesn't enforce this, but so far provides no UAPI to do anything useful with non-coherent cached mappings (although there is interest to support this)

Quoted 5 lines

btw, I'm looking at a benchmark (gl_driver2_off) where (after some other in-flight optimizations land) we end up bottlenecked on writing to WC cmdstream buffers. I assume in the current state, WC goes all the way to main memory rather than just to system cache?

oh, I guess this (mentioned earlier in thread) is what I really want for this benchmark:

https://android-review.googlesource.com/c/kernel/common/+/1549097/3

Hi Rob,

On 2021-03-16 22:46, Rob Clark wrote:

<snip>...

Quoted 18 lines

Quoted 12 lines

Quoted 11 lines

Quoted 4 lines

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Currently userspace asks for everything WC, so pgprot_writecombine()

The kernel doesn't enforce this, but so far provides no UAPI to do anything useful with non-coherent cached mappings (although there is interest to support this)

Quoted 5 lines

btw, I'm looking at a benchmark (gl_driver2_off) where (after some other in-flight optimizations land) we end up bottlenecked on writing to WC cmdstream buffers. I assume in the current state, WC goes all the way to main memory rather than just to system cache?

Quoted 5 lines

oh, I guess this (mentioned earlier in thread) is what I really want for this benchmark:

https://android-review.googlesource.com/c/kernel/common/+/1549097/3

You can also check if the system cache lines are allocated for GPU or not with patch in https://crrev.com/c/2766723

With the above patch applied,
cat /sys/kernel/debug/llcc_stats/llcc_scid_status

The SCIDs for GPU are listed in include/linux/soc/qcom/llcc-qcom.h

Quoted 50 lines · Sai Prakash Ranjan

Quoted 48 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 2 lines

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback).

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Quoted 51 lines · Will Deacon

Quoted 50 lines · Sai Prakash Ranjan

Quoted 48 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

Quoted 9 lines

Quoted 2 lines

For mismatched outer cacheability attributes which Will mentioned, I was referring to [1] in android kernel.

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Rob answered this.

Quoted 5 lines

Finally, we need to be careful when we use the word "hint" as "allocation hint" has a specific meaning in the architecture, and if we only mismatch on those then we're actually ok. But I think IOMMU_LLC is more than just a hint, since it actually drives eviction policy (i.e. it enables writeback).

Quoted 3 lines

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Sorry for the confusion which probably was caused by my mentioning of android, NWA(no write allocate) is an allocation hint which we can ignore for now as it is not introduced yet in upstream.

Quoted 79 lines · Sai Prakash Ranjan

Quoted 55 lines · Will Deacon

Quoted 45 lines · Sai Prakash Ranjan

Quoted 44 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

Quoted 8 lines

Quoted 4 lines

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

For mismatched outer cacheability attributes which Will mentioned, I was
referring to [1] in android kernel.

Quoted 7 lines

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped
into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Rob answered this.

Quoted 12 lines

Finally, we need to be careful when we use the word "hint" as "allocation
hint" has a specific meaning in the architecture, and if we only mismatch on
those then we're actually ok. But I think IOMMU_LLC is more than just a
hint, since it actually drives eviction policy (i.e. it enables writeback).

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Quoted 5 lines

Sorry for the confusion which probably was caused by my mentioning of android, NWA(no write allocate) is an allocation hint which we can ignore
for now as it is not introduced yet in upstream.

Any chance of taking this forward? We do not want to miss out on small fps gain when the product gets released.

Quoted 86 lines · Sai Prakash Ranjan

Quoted 84 lines · Sai Prakash Ranjan

Quoted 55 lines · Will Deacon

Quoted 45 lines · Sai Prakash Ranjan

Quoted 44 lines · Jordan Crouse

Quoted 20 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 18 lines · Sai Prakash Ranjan

Quoted 16 lines · Will Deacon

Quoted 1 line · Sai Prakash Ranjan

+#define IOMMU_LLC (1 << 6)

On reflection, I'm a bit worried about exposing this because I think it will
introduce a mismatched virtual alias with the CPU (we don't even have a MAIR
set up for this memory type). Now, we also have that issue for the PTW, but
since we always use cache maintenance (i.e. the streaming API) for publishing the page-tables to a non-coheren walker, it works out. However,
if somebody expects IOMMU_LLC to be coherent with a DMA API coherent allocation, then they're potentially in for a nasty surprise due to the mismatched outer-cacheability attributes.

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

Can't we add the syscached memory type similar to what is done on android?

Maybe. How does the GPU driver map these things on the CPU side?

Currently we use writecombine mappings for everything, although there are some cases that we'd like to use cached (but have not merged patches that would give userspace a way to flush/invalidate)

LLC/system cache doesn't have a relationship with the CPU cache. Its just a
little accelerator that sits on the connection from the GPU to DDR and caches
accesses. The hint that Sai is suggesting is used to mark the buffers as 'no-write-allocate' to prevent GPU write operations from being cached in the LLC
which a) isn't interesting and b) takes up cache space for read operations.

Its easiest to think of the LLC as a bonus accelerator that has no cost for
us to use outside of the unfortunate per buffer hint.

Quoted 8 lines

Quoted 4 lines

We do have to worry about the CPU cache w.r.t I/O coherency (which is a different hint) and in that case we have all of concerns that Will identified.

For mismatched outer cacheability attributes which Will mentioned, I was
referring to [1] in android kernel.

Quoted 7 lines

I've lost track of the conversation here :/

When the GPU has a buffer mapped with IOMMU_LLC, is the buffer also mapped
into the CPU and with what attributes? Rob said "writecombine for everything" -- does that mean ioremap_wc() / MEMREMAP_WC?

Rob answered this.

Quoted 9 lines

Finally, we need to be careful when we use the word "hint" as "allocation
hint" has a specific meaning in the architecture, and if we only mismatch on
those then we're actually ok. But I think IOMMU_LLC is more than just a
hint, since it actually drives eviction policy (i.e. it enables writeback).

Quoted 3 lines

Sorry for the pedantry, but I just want to make sure we're all talking about the same things!

Sorry for the confusion which probably was caused by my mentioning of android, NWA(no write allocate) is an allocation hint which we can ignore
for now as it is not introduced yet in upstream.

Quoted 2 lines

Any chance of taking this forward? We do not want to miss out on small fps gain when the product gets released.

Do we have a solution to the mismatched virtual alias?

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

The series slightly depends on following 2 patches posted earlier and is based on msm-next branch:
* https://lore.kernel.org/patchwork/patch/1363008/
* https://lore.kernel.org/patchwork/patch/1363010/

Sai Prakash Ranjan (3):
iommu/io-pgtable: Rename last-level cache quirk to
IO_PGTABLE_QUIRK_PTW_LLC
iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag drm/msm: Use IOMMU_LLC page protection flag to map gpu buffers

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 3 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
 drivers/gpu/drm/msm/msm_iommu.c         | 3 +++
 drivers/gpu/drm/msm/msm_mmu.h           | 4 ++++
 drivers/iommu/io-pgtable-arm.c          | 9 ++++++---
 include/linux/io-pgtable.h              | 6 +++---
 include/linux/iommu.h                   | 6 ++++++
 7 files changed, 26 insertions(+), 7 deletions(-)

base-commit: 00fd44a1a4700718d5d962432b55c09820f7e709

Quoted 32 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

The series slightly depends on following 2 patches posted earlier and is based on msm-next branch:
* https://lore.kernel.org/patchwork/patch/1363008/
* https://lore.kernel.org/patchwork/patch/1363010/

Sai Prakash Ranjan (3):
iommu/io-pgtable: Rename last-level cache quirk to
IO_PGTABLE_QUIRK_PTW_LLC
iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag drm/msm: Use IOMMU_LLC page protection flag to map gpu buffers

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 3 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
 drivers/gpu/drm/msm/msm_iommu.c         | 3 +++
 drivers/gpu/drm/msm/msm_mmu.h           | 4 ++++
 drivers/iommu/io-pgtable-arm.c          | 9 ++++++---
 include/linux/io-pgtable.h              | 6 +++---
 include/linux/iommu.h                   | 6 ++++++
 7 files changed, 26 insertions(+), 7 deletions(-)

Quoted 1 line

base-commit: 00fd44a1a4700718d5d962432b55c09820f7e709

Gentle Ping!

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

The series slightly depends on following 2 patches posted earlier and is based on msm-next branch:
* https://lore.kernel.org/patchwork/patch/1363008/
* https://lore.kernel.org/patchwork/patch/1363010/

Sai Prakash Ranjan (3):
iommu/io-pgtable: Rename last-level cache quirk to
IO_PGTABLE_QUIRK_PTW_LLC
iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag drm/msm: Use IOMMU_LLC page protection flag to map gpu buffers

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 3 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
 drivers/gpu/drm/msm/msm_iommu.c         | 3 +++
 drivers/gpu/drm/msm/msm_mmu.h           | 4 ++++
 drivers/iommu/io-pgtable-arm.c          | 9 ++++++---
 include/linux/io-pgtable.h              | 6 +++---
 include/linux/iommu.h                   | 6 ++++++
 7 files changed, 26 insertions(+), 7 deletions(-)

base-commit: 00fd44a1a4700718d5d962432b55c09820f7e709

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Quoted 7 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

Quoted 3 lines

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Hi Sai,

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 28 lines

The series slightly depends on following 2 patches posted earlier and is based on msm-next branch:
* https://lore.kernel.org/patchwork/patch/1363008/
* https://lore.kernel.org/patchwork/patch/1363010/

Sai Prakash Ranjan (3):
iommu/io-pgtable: Rename last-level cache quirk to
IO_PGTABLE_QUIRK_PTW_LLC
iommu/io-pgtable-arm: Add IOMMU_LLC page protection flag drm/msm: Use IOMMU_LLC page protection flag to map gpu buffers

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 3 +++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
 drivers/gpu/drm/msm/msm_iommu.c         | 3 +++
 drivers/gpu/drm/msm/msm_mmu.h           | 4 ++++
 drivers/iommu/io-pgtable-arm.c          | 9 ++++++---
 include/linux/io-pgtable.h              | 6 +++---
 include/linux/iommu.h                   | 6 ++++++
 7 files changed, 26 insertions(+), 7 deletions(-)

base-commit: 00fd44a1a4700718d5d962432b55c09820f7e709
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Quoted 12 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Quoted 3 lines

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 19 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 28 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 35 lines · Will Deacon

Quoted 28 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 42 lines · Rob Clark

Quoted 35 lines · Will Deacon

Quoted 28 lines · Rob Clark

Quoted 19 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Am I missing some reason this isn't solvable?

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Quoted 76 lines · Rob Clark

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag") removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went the memory type setting required for the non-coherent masters to use system cache. Now that system cache support for GPU is added, we will need to set the right PTE attribute for GPU buffers to be sys cached. Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC, renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 5 lines

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the hood.

Anyway, sorry, I should've said that explicitly earlier on. We've done this sort of thing in the Android tree so I assumed Sai knew what needed to be done and then I didn't think to explain to you :(

Quoted 89 lines · Will Deacon

Quoted 76 lines · Rob Clark

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

Quoted 10 lines · Sai Prakash Ranjan

commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
the memory type setting required for the non-coherent masters to use
system cache. Now that system cache support for GPU is added, we will
need to set the right PTE attribute for GPU buffers to be sys cached.
Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC,
renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 5 lines

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Quoted 6 lines

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Right I was aware of that but even in the android tree there is no user :)
I think we can't have a new memory type without any user right in upstream
like android tree?

@Rob, I think you already tried adding a new MT and used pgprot_syscached()
in GPU driver but it was crashing?

Quoted 104 lines · Sai Prakash Ranjan

Quoted 89 lines · Will Deacon

Quoted 76 lines · Rob Clark

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

Quoted 15 lines · Georgi Djakov

On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
the memory type setting required for the non-coherent masters to use
system cache. Now that system cache support for GPU is added, we will
need to set the right PTE attribute for GPU buffers to be sys cached.
Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC,
renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does
not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 5 lines

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Quoted 6 lines

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Right I was aware of that but even in the android tree there is no user :)
I think we can't have a new memory type without any user right in upstream
like android tree?

Quoted 3 lines

@Rob, I think you already tried adding a new MT and used pgprot_syscached()
in GPU driver but it was crashing?

Correct, but IIRC there were some differences in the code for memory types compared to the android tree.. I couldn't figure out the necessary patches to cherry-pick to get the android patch to apply cleanly, so I tried re-implementing it without having much of a clue about how that code works (which was probably the issue) ;-)

Quoted 111 lines · Rob Clark

Quoted 105 lines · Sai Prakash Ranjan

Quoted 89 lines · Will Deacon

Quoted 76 lines · Rob Clark

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 18 lines · Will Deacon

Quoted 17 lines · Sai Prakash Ranjan

On 2021-07-28 19:30, Georgi Djakov wrote:
On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
the memory type setting required for the non-coherent masters to use
system cache. Now that system cache support for GPU is added, we will
need to set the right PTE attribute for GPU buffers to be sys cached.
Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC,
renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does
not apply anymore?

Quoted 7 lines

Quoted 2 lines

I was waiting on Will's reply [1]. If there are no changes needed, then
I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding a new memory type to the SMMU which doesn't exist on the CPU side. That can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 5 lines

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Quoted 6 lines

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Right I was aware of that but even in the android tree there is no user
:)
I think we can't have a new memory type without any user right in upstream
like android tree?

Quoted 3 lines

@Rob, I think you already tried adding a new MT and used pgprot_syscached()
in GPU driver but it was crashing?

Quoted 6 lines

Correct, but IIRC there were some differences in the code for memory types compared to the android tree.. I couldn't figure out the necessary patches to cherry-pick to get the android patch to apply cleanly, so I tried re-implementing it without having much of a clue about how that code works (which was probably the issue) ;-)

Hehe no, even I get the same crash after porting/modifying the required patches from android ;) and I think crashes would be seen in android as well, its just that they don't have any user exercising that code.

Thing is I can't make head and tail of the GPU crash logs, maybe you know
how to decode those errors, if not I can start a thread with QC GPU team and ask them to decode?

Quoted 120 lines · Sai Prakash Ranjan

Quoted 111 lines · Rob Clark

Quoted 105 lines · Sai Prakash Ranjan

Quoted 89 lines · Will Deacon

Quoted 76 lines · Rob Clark

Quoted 75 lines · Will Deacon

Quoted 66 lines · Rob Clark

Quoted 49 lines · Will Deacon

Quoted 40 lines · Rob Clark

Quoted 33 lines · Will Deacon

Quoted 27 lines · Rob Clark

Quoted 25 lines · Will Deacon

On Thu, Jul 29, 2021 at 10:08:22AM +0530, Sai Prakash Ranjan wrote:
On 2021-07-28 19:30, Georgi Djakov wrote:
On Mon, Jan 11, 2021 at 07:45:02PM +0530, Sai Prakash Ranjan wrote:
commit ecd7274fb4cd ("iommu: Remove unused IOMMU_SYS_CACHE_ONLY flag")
removed unused IOMMU_SYS_CACHE_ONLY prot flag and along with it went
the memory type setting required for the non-coherent masters to use
system cache. Now that system cache support for GPU is added, we will
need to set the right PTE attribute for GPU buffers to be sys cached.
Without this, the system cache lines are not allocated for GPU.

So the patches in this series introduces a new prot flag IOMMU_LLC,
renames IO_PGTABLE_QUIRK_ARM_OUTER_WBWA to IO_PGTABLE_QUIRK_PTW_LLC
and makes GPU the user of this protection flag.

Thank you for the patchset! Are you planning to refresh it, as it does
not apply anymore?

I was waiting on Will's reply [1]. If there are no changes needed, then
I can repost the patch.

I still think you need to handle the mismatched alias, no? You're adding
a new memory type to the SMMU which doesn't exist on the CPU side. That
can't be right.

Quoted 4 lines

Just curious, and maybe this is a dumb question, but what is your concern about mismatched aliases? I mean the cache hierarchy on the GPU device side (anything beyond the LLC) is pretty different and doesn't really care about the smmu pgtable attributes..

Quoted 5 lines

If the CPU accesses a shared buffer with different attributes to those which the device is using then you fall into the "mismatched memory attributes" part of the Arm architecture. It's reasonably unforgiving (you should go and read it) and in some cases can apply to speculative accesses as well, but the end result is typically loss of coherency.

Quoted 7 lines

Ok, I might have a few other sections to read first to decipher the terminology..

But my understanding of LLC is that it looks just like system memory to the CPU and GPU (I think that would make it "the point of coherence" between the GPU and CPU?) If that is true, shouldn't it be invisible from the point of view of different CPU mapping options?

Quoted 5 lines

You could certainly build a system where mismatched attributes don't cause loss of coherence, but as it's not guaranteed by the architecture and the changes proposed here affect APIs which are exposed across SoCs, then I don't think it helps much.

Hmm, the description of the new mapping flag is that it applies only to transparent outer level cache:

(quoted diff) +6 -0

+/*
+ * Non-coherent masters can use this page protection flag to set cacheable
+ * memory attributes for only a transparent outer level of cache, also known as
+ * the last-level or system cache.
+ */
+#define IOMMU_LLC      (1 << 6)

Quoted 3 lines

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Quoted 5 lines

Quoted 1 line

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Quoted 6 lines

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Right I was aware of that but even in the android tree there is no user
:)
I think we can't have a new memory type without any user right in upstream
like android tree?

Quoted 3 lines

@Rob, I think you already tried adding a new MT and used pgprot_syscached()
in GPU driver but it was crashing?

Quoted 6 lines

Correct, but IIRC there were some differences in the code for memory types compared to the android tree.. I couldn't figure out the necessary patches to cherry-pick to get the android patch to apply cleanly, so I tried re-implementing it without having much of a clue about how that code works (which was probably the issue) ;-)

Quoted 9 lines

Hehe no, even I get the same crash after porting/modifying the required patches from android ;) and I think crashes would be seen in android as well, its just that they don't have any user exercising that code.

Thing is I can't make head and tail of the GPU crash logs, maybe you know
how to decode those errors, if not I can start a thread with QC GPU team and ask them to decode?

If you have a gpu devcore dump, I can take a look at it with crashdec.. otherwise I can try to find the branch where I had that patch backported.

I'm more familiar with using crashdec to figure out mesa bugs, but maybe I could spot something where what the GPU is seeing disagrees with what the CPU expects it to be seeing.

Quoted 22 lines · Will Deacon

Quoted 6 lines · Rob Clark

Quoted 5 lines · Will Deacon

Quoted 3 lines · Rob Clark

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

Quoted 8 lines

Quoted 4 lines

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Quoted 6 lines

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Right I was aware of that but even in the android tree there is no user :)

I think we can't have a new memory type without any user right in upstream like android tree?

Quoted 30 lines · Sai Prakash Ranjan

Quoted 28 lines · Will Deacon

Quoted 6 lines · Rob Clark

Quoted 5 lines · Will Deacon

Quoted 3 lines · Rob Clark

But I suppose we could call it instead IOMMU_QCOM_LLC or something like that to make it more clear that it is not necessarily something that would work with a different outer level cache implementation?

Quoted 8 lines

Quoted 4 lines

... or we could just deal with the problem so that other people can reuse the code. I haven't really understood the reluctance to solve this properly.

Am I missing some reason this isn't solvable?

Oh, was there another way to solve it (other than foregoing setting INC_OCACHE in the pgtables)? Maybe I misunderstood, is there a corresponding setting on the MMU pgtables side of things?

Right -- we just need to program the CPU's MMU with the matching memory attributes! It's a bit more fiddly if you're just using ioremap_wc() though, as it's usually the DMA API which handles the attributes under the
hood.

Anyway, sorry, I should've said that explicitly earlier on. We've done this
sort of thing in the Android tree so I assumed Sai knew what needed to be
done and then I didn't think to explain to you :(

Quoted 1 line

Right I was aware of that but even in the android tree there is no user :)

I'm assuming there are vendor modules using it there, otherwise we wouldn't have been asked to put it in. Since you work at Qualcomm, maybe you could talk to your colleagues (Isaac and Patrick) directly?

Quoted 2 lines

I think we can't have a new memory type without any user right in upstream like android tree?

Correct. But I don't think we should be adding IOMMU_* anything upstream if we don't have a user.