drm/cache: Build-in drm_clflush_*() functions

From: Thierry Reding <[email protected]>

Irrespective of whether or not the DRM core is built as a module, build the cache flush functions into the kernel. This is required because the implementation may require functions that shouldn't be exported to most drivers. DRM is somewhat of a special case here because it requires the cache maintenance functions to properly flush pages backed by shmemfs. These pages are directly given to display hardware for scanout, so they must be purged from any caches to avoid visual corruption on screen.

To achieve this, add a new boolean Kconfig symbol that drivers can select if they use any of these functions. drm_cache.c is then built if and only if this symbol is selected. TTM and the i915 driver already use these functions, so make them select DRM_CACHE.

---
 drivers/gpu/drm/Kconfig      | 5 +++++
 drivers/gpu/drm/Makefile     | 3 ++-
 drivers/gpu/drm/i915/Kconfig | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

index 47f2ce81b412..25bffdb89304 100644
@@ -21,6 +21,10 @@ menuconfig DRM
 	  details.  You should also select and configure AGP
 	  (/dev/agpgart) support if it is available for your platform.
 
+config DRM_CACHE
+	bool
+	depends on DRM
+
 config DRM_MIPI_DSI
 	bool
 	depends on DRM

@@ -55,6 +59,7 @@ config DRM_LOAD_EDID_FIRMWARE
 config DRM_TTM
 	tristate
 	depends on DRM
+	select DRM_CACHE
 	help
 	  GPU memory management subsystem for devices with multiple
 	  GPU memory types. Will be enabled automatically if a device driver

index 7d4944e1a60c..1ad54a0e4467 100644
@@ -4,7 +4,7 @@
 
 ccflags-y := -Iinclude/drm
 
-drm-y       :=	drm_auth.o drm_bufs.o drm_cache.o \
+drm-y       :=	drm_auth.o drm_bufs.o \
 		drm_context.o drm_dma.o \
 		drm_fops.o drm_gem.o drm_ioctl.o drm_irq.o \
 		drm_lock.o drm_memory.o drm_drv.o drm_vm.o \

@@ -33,6 +33,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
 CFLAGS_drm_trace_points.o := -I$(src)
 
 obj-$(CONFIG_DRM)	+= drm.o
+obj-$(CONFIG_DRM_CACHE) += drm_cache.o
 obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
 obj-$(CONFIG_DRM_TTM)	+= ttm/
 obj-$(CONFIG_DRM_TDFX)	+= tdfx/

index 74acca9bcd9d..237be03e8a7c 100644
@@ -10,6 +10,7 @@ config DRM_I915
 	# the shmem_readpage() which depends upon tmpfs
 	select SHMEM
 	select TMPFS
+	select DRM_CACHE
 	select DRM_KMS_HELPER
 	select DRM_PANEL
 	select DRM_MIPI_DSI

2.3.2

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Thierry Reding <[email protected]>

Add implementations for drm_clflush_*() on ARM by borrowing code from the DMA mapping API implementation. Unfortunately ARM doesn't export an API to flush caches on a page by page basis, so this replicates most of the code.

---
 drivers/gpu/drm/drm_cache.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

index 9a62d7a53553..200d86c3d72d 100644
@@ -67,6 +67,41 @@ static void drm_cache_flush_clflush(struct page *pages[],
 }
 #endif
 
+#if defined(CONFIG_ARM)
+
+#include <asm/cacheflush.h>
+#include <asm/cachetype.h>
+#include <asm/highmem.h>
+#include <asm/outercache.h>
+
+static void drm_clflush_page(struct page *page)
+{
+	enum dma_data_direction dir = DMA_TO_DEVICE;
+	phys_addr_t phys = page_to_phys(page);
+	size_t size = PAGE_SIZE;
+	void *virt;
+
+	if (PageHighMem(page)) {
+		if (cache_is_vipt_nonaliasing()) {
+			virt = kmap_atomic(page);
+			dmac_map_area(virt, size, dir);
+			kunmap_atomic(virt);
+		} else {
+			virt = kmap_high_get(page);
+			if (virt) {
+				dmac_map_area(virt, size, dir);
+				kunmap_high(page);
+			}
+		}
+	} else {
+		virt = page_address(page);
+		dmac_map_area(virt, size, dir);
+	}
+
+	outer_flush_range(phys, phys + PAGE_SIZE);
+}
+#endif
+
 void
 drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 {

@@ -94,6 +129,11 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 				   (unsigned long)page_virtual + PAGE_SIZE);
 		kunmap_atomic(page_virtual);
 	}
+#elif defined(CONFIG_ARM)
+	unsigned long i;
+
+	for (i = 0; i < num_pages; i++)
+		drm_clflush_page(pages[i]);
 #else
 	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
 	WARN_ON_ONCE(1);

@@ -118,6 +158,11 @@ drm_clflush_sg(struct sg_table *st)
 
 	if (wbinvd_on_all_cpus())
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
+#elif defined(CONFIG_ARM)
+	struct sg_page_iter sg_iter;
+
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
+		drm_clflush_page(sg_page_iter_page(&sg_iter));
 #else
 	printk(KERN_ERR "Architecture has no drm_cache.c support\n");
 	WARN_ON_ONCE(1);

2.3.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

I'm _really_ not happy with this, because it's poking about in ARM internal implementation details of the DMA API. It's also not going to work too well on aliasing caches either - especially when you consider that the userspace mapping of a page may have no relationship to the address you get from kmap.

For an aliasing cache, the way things work with the DMA API, we ensure that the kernel alias is clean whenever pages are un-kmapped, which means that unmapped highmem pages never have L1 cache lines associated with the kernel alias of them. The user aliases are handled separately via the normal flush_dcache_page()/flush_anon_page() calls.

None of this exists here...

It gets even more hairly on older ARM CPUs - but I hope no one is expecting to support DRM's clflush there - we should make that explicit though, and ensure that clflush support returns an error there.

That aside, we have most of this logic already inside dma_cache_maint_page(), and even if it was the right thing to be doing, we shouldn't be duplicating this architecture specific code inside a driver.

So you can take that as a NAK on this.

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Perhaps I should take a step back and explain what I'm trying to solve, maybe that'll allow us to come up with a more proper solution.

Both the MSM and Tegra drivers allocate framebuffers from shmem in the presence of an IOMMU. The problem with allocating pages from the shmem is that pages end up not being flushed, resulting in visual artifacts on the screen (horizontal black lines) when the cachelines from earlier allocations of these pages get flushed. The drivers also use the IOMMU API directly to manage the IOVA space.

Currently both drivers work around this by faking up an sg_table and calling dma_map_sg(), which ends up doing the right thing. Unfortunately this only works if nothing backs the DMA API and we end up getting the regular arm_dma_ops. If an IOMMU registers with the ARM/DMA glue, we'll end up getting a different set of dma_ops and things break (buffers are mapped through the IOMMU twice, ...).

Still, this has worked fine on Tegra (and I assume the same is true for MSM) so far because we don't register an IOMMU with the ARM/DMA glue. However while porting this to 64-bit ARM I started seeing failures to map buffers because SWIOTLB is enabled by default and gets in the way.

With regards to code duplication, I suspect we could call something like arm_dma_ops.sync_sg_for_device() directly, but that raises the question about which device to pass in. It isn't clear at allocation time which device will use the buffer. It may in fact be used by multiple devices at once.

Do you have an alternative suggestion on how to fix this?

Thierry

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Thierry Reding <[email protected]>

Add implementations for drm_clflush_*() on 64-bit ARM. This shares a lot of code with the 32-bit ARM implementation.

---
 drivers/gpu/drm/drm_cache.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

index 200d86c3d72d..0c3072b4cdc9 100644
@@ -102,6 +102,17 @@ static void drm_clflush_page(struct page *page)
 }
 #endif
 
+#if defined(CONFIG_ARM64)
+static void drm_clflush_page(struct page *page)
+{
+	void *virt;
+
+	virt = kmap_atomic(page);
+	__dma_flush_range(virt, virt + PAGE_SIZE);
+	kunmap_atomic(virt);
+}
+#endif
+
 void
 drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 {

@@ -129,7 +140,7 @@ drm_clflush_pages(struct page *pages[], unsigned long num_pages)
 				   (unsigned long)page_virtual + PAGE_SIZE);
 		kunmap_atomic(page_virtual);
 	}
-#elif defined(CONFIG_ARM)
+#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 	unsigned long i;
 
 	for (i = 0; i < num_pages; i++)

@@ -158,7 +169,7 @@ drm_clflush_sg(struct sg_table *st)
 
 	if (wbinvd_on_all_cpus())
 		printk(KERN_ERR "Timed out waiting for cache flush.\n");
-#elif defined(CONFIG_ARM)
+#elif defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 	struct sg_page_iter sg_iter;
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)

2.3.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Thierry Reding <[email protected]>

Instead of going through the DMA mapping API for cache maintenance, use the drm_clflush_*() family of functions to achieve the same effect.

---
 drivers/gpu/drm/tegra/Kconfig |  1 +
 drivers/gpu/drm/tegra/gem.c   | 42 +++++++++++-------------------------------
 2 files changed, 12 insertions(+), 31 deletions(-)

index 74d9d621453d..4901f20f99a1 100644
@@ -4,6 +4,7 @@ config DRM_TEGRA
 	depends on COMMON_CLK
 	depends on DRM
 	depends on RESET_CONTROLLER
+	select DRM_CACHE
 	select DRM_KMS_HELPER
 	select DRM_MIPI_DSI
 	select DRM_PANEL

2.3.2

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Thierry Reding <[email protected]>

Instead of going through the DMA mapping API for cache maintenance, use the drm_clflush_*() family of functions to achieve the same effect.

---
 drivers/gpu/drm/armada/Kconfig      |  1 +
 drivers/gpu/drm/armada/armada_gem.c | 13 ++-----------
 2 files changed, 3 insertions(+), 11 deletions(-)

index 50ae88ad4d76..7b7070128a05 100644
@@ -4,6 +4,7 @@ config DRM_ARMADA
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select DRM_CACHE
 	select DRM_KMS_HELPER
 	select DRM_KMS_FB_HELPER
 	help

index 580e10acaa3a..c2d4414031ab 100644
@@ -453,19 +453,14 @@ armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
 			sg_set_page(sg, page, PAGE_SIZE, 0);
 		}
 
-		if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0) {
-			num = sgt->nents;
-			goto release;
-		}
+		drm_clflush_sg(sgt);
 	} else if (dobj->page) {
 		/* Single contiguous page */
 		if (sg_alloc_table(sgt, 1, GFP_KERNEL))
 			goto free_sgt;
 
 		sg_set_page(sgt->sgl, dobj->page, dobj->obj.size, 0);
-
-		if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
-			goto free_table;
+		drm_clflush_sg(sgt);
 	} else if (dobj->linear) {
 		/* Single contiguous physical region - no struct page */
 		if (sg_alloc_table(sgt, 1, GFP_KERNEL))

@@ -480,7 +475,6 @@ armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,
  release:
 	for_each_sg(sgt->sgl, sg, num, i)
 		page_cache_release(sg_page(sg));
- free_table:
 	sg_free_table(sgt);
  free_sgt:
 	kfree(sgt);

@@ -494,9 +488,6 @@ static void armada_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach,
 	struct armada_gem_object *dobj = drm_to_armada_gem(obj);
 	int i;
 
-	if (!dobj->linear)
-		dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
-
 	if (dobj->obj.filp) {
 		struct scatterlist *sg;
 		for_each_sg(sgt->sgl, sg, sgt->nents, i)

2.3.2

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

I think it isn't the right thing to do in this case indeed. Both Tegra and MSM were using this pattern to make sure that caches are invalidated upon allocation of memory from shmemfs, but I now realize that for the Armada driver this isn't the case.

Thierry

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

From: Thierry Reding <[email protected]>

Instead of going through the DMA mapping API for cache maintenance, use the drm_clflush_*() family of functions to achieve the same effect.

---
 drivers/gpu/drm/msm/Kconfig   | 1 +
 drivers/gpu/drm/msm/msm_gem.c | 9 +--------
 2 files changed, 2 insertions(+), 8 deletions(-)

index 0a6f6764a37c..5da7fe793e89 100644
@@ -5,6 +5,7 @@ config DRM_MSM
 	depends on ARCH_QCOM || (ARM && COMPILE_TEST)
 	depends on OF && COMMON_CLK
 	select REGULATOR
+	select DRM_CACHE
 	select DRM_KMS_HELPER
 	select DRM_PANEL
 	select SHMEM

index 52839769eb6c..ce265085fc57 100644
@@ -101,8 +101,7 @@ static struct page **get_pages(struct drm_gem_object *obj)
 		 * because display controller, GPU, etc. are not coherent:
 		 */
 		if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-			dma_map_sg(dev->dev, msm_obj->sgt->sgl,
-					msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
+			drm_clflush_sg(msm_obj->sgt);
 	}
 
 	return msm_obj->pages;

@@ -113,12 +112,6 @@ static void put_pages(struct drm_gem_object *obj)
 	struct msm_gem_object *msm_obj = to_msm_bo(obj);
 
 	if (msm_obj->pages) {
-		/* For non-cached buffers, ensure the new pages are clean
-		 * because display controller, GPU, etc. are not coherent:
-		 */
-		if (msm_obj->flags & (MSM_BO_WC|MSM_BO_UNCACHED))
-			dma_unmap_sg(obj->dev->dev, msm_obj->sgt->sgl,
-					msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
 		sg_free_table(msm_obj->sgt);
 		kfree(msm_obj->sgt);
 

2.3.2

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx