[PATCH 0/7] omapdrm: Fix GEM objects DMA unmapping

[PATCH 0/7] omapdrm: Fix GEM objects DMA unmapping

[Laurent Pinchart]

Hello,

Memory leaks have been reported when allocating a cached omap_bo (with
OMAP_BO_CACHED. Investigation showed that this can only come from the DMA
mapping debug layer, as on ARM32 the non-coherent, non-IOMMU DMA mapping code
doesn't allocate memory to map a page (and kmemcheck facility is only
available on x86, so it can't be a source of memory leaks either on ARM).

The DMA debug layer pre-allocates DMA debugging entries and stores them in a
list. As the omapdrm driver maps cached buffer page by page, the list of 4096
pre-allocated entries is starved very soon. However, raising the number of DMA
mapping debug entries to 32 * 4096 (through the dma_debug_entries kernel
command line argument) led to more interesting results.

The number of entries being large enough to handle all the pages mapped by
kmstest, monitoring the DMA mapping statistics through
/sys/kernel/debug/dma-api/ showed that the number of free entries dropped
significantly when kmstest was started and didn't raise when it was stopped.
In particular, running kmstest without flipping resulting in a drop of 1266
free entries, which corresponds to one 1440x900 framebuffer in XR24. The
proved that the pages backing the framebuffer, while freed when the
framebuffer was destroyed, were not unmapped.

I've thus started investigating the driver GEM implementation. After a few
confusing moments that resulted in the 1/7 to 6/7 cleanup patches, I wrote
patch 7/7 that should fix the issue.

No memory leak was noticed through /sys/kernel/debug/dma-api/ or /proc/meminfo
running kmstest --flip for 300 frames in a loop for one hour with cached
buffers on the latest upstream kernel (merge of v4.11-rc1, drm-next and
drm-misc-next). With this patch series the same results should be reproducible
for cached buffers too, but I haven't been able to test that as I don't have
access to a display right now. Tomi, would you be able to perform tests ?

Patches for kms++ and kmstest to support cached mappings are available at

	git://git.ideasonboard.com/renesas/kmsxx.git omap/cache

Laurent Pinchart (7):
  drm: omapdrm: Remove remap argument to omap_gem_get_paddr()
  drm: omapdrm: Rename occurrences of paddr to dma_addr
  drm: omapdrm: Rename omap_gem_(get|put)_paddr() to omap_gem_(un)pin()
  drm: omapdrm: Lower indentation level in omap_gem_dma_sync_buffer()
  drm: omapdrm: Rename the omap_gem_object addrs field to dma_addrs
  drm: omapdrm: Fix incorrect usage of the term 'cache coherency'
  drm: omapdrm: DMA-unmap pages for all buffer types when freeing
    buffers

 drivers/gpu/drm/omapdrm/omap_drv.h        |   9 +-
 drivers/gpu/drm/omapdrm/omap_fb.c         |  25 ++--
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  15 ++-
 drivers/gpu/drm/omapdrm/omap_gem.c        | 196 ++++++++++++++++--------------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  12 +-
 5 files changed, 136 insertions(+), 121 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/7] drm: omapdrm: Remove remap argument to omap_gem_get_paddr()

[Laurent Pinchart]

The function is always called with the remap argument set to true.
Hardcode that behaviour and remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h        |  3 +--
 drivers/gpu/drm/omapdrm/omap_fb.c         |  2 +-
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  2 +-
 drivers/gpu/drm/omapdrm/omap_gem.c        | 10 ++++------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  2 +-
 5 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 65977982f15f..5641ba2c2825 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -198,8 +198,7 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll);
 void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff);
 void omap_gem_dma_sync(struct drm_gem_object *obj,
 		enum dma_data_direction dir);
-int omap_gem_get_paddr(struct drm_gem_object *obj,
-		dma_addr_t *paddr, bool remap);
+int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr);
 void omap_gem_put_paddr(struct drm_gem_object *obj);
 int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages,
 		bool remap);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 29dc677dd4d3..e249c39961df 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -258,7 +258,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		ret = omap_gem_get_paddr(plane->bo, &plane->paddr, true);
+		ret = omap_gem_get_paddr(plane->bo, &plane->paddr);
 		if (ret)
 			goto fail;
 		omap_gem_dma_sync(plane->bo, DMA_TO_DEVICE);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 942c4d483008..22ab1db2f1bf 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -162,7 +162,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	 * to it).  Then we just need to be sure that we are able to re-
 	 * pin it in case of an opps.
 	 */
-	ret = omap_gem_get_paddr(fbdev->bo, &paddr, true);
+	ret = omap_gem_get_paddr(fbdev->bo, &paddr);
 	if (ret) {
 		dev_err(dev->dev,
 			"could not map (paddr)!  Skipping framebuffer alloc\n");
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 5d5a9f517c30..5019aea35451 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -797,12 +797,10 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 	}
 }
 
-/* Get physical address for DMA.. if 'remap' is true, and the buffer is not
- * already contiguous, remap it to pin in physically contiguous memory.. (ie.
- * map in TILER)
+/* Get physical address for DMA.. if the buffer is not already contiguous, remap
+ * it to pin in physically contiguous memory.. (ie. map in TILER)
  */
-int omap_gem_get_paddr(struct drm_gem_object *obj,
-		dma_addr_t *paddr, bool remap)
+int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr)
 {
 	struct omap_drm_private *priv = obj->dev->dev_private;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
@@ -810,7 +808,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
 
 	mutex_lock(&obj->dev->struct_mutex);
 
-	if (!is_contiguous(omap_obj) && remap && priv->has_dmm) {
+	if (!is_contiguous(omap_obj) && priv->has_dmm) {
 		if (omap_obj->paddr_cnt == 0) {
 			struct page **pages;
 			uint32_t npages = obj->size >> PAGE_SHIFT;
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index af267c35d813..7f380eb31afb 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -41,7 +41,7 @@ static struct sg_table *omap_gem_map_dma_buf(
 	/* camera, etc, need physically contiguous.. but we need a
 	 * better way to know this..
 	 */
-	ret = omap_gem_get_paddr(obj, &paddr, true);
+	ret = omap_gem_get_paddr(obj, &paddr);
 	if (ret)
 		goto out;
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/7] drm: omapdrm: Rename occurrences of paddr to dma_addr

[Laurent Pinchart]

The fields, variables and functions deal with DMA addresses, name them
accordingly. The omap_gem_get_paddr() and omap_gem_put_paddr() will be
addressed differently separately.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h        |  6 +--
 drivers/gpu/drm/omapdrm/omap_fb.c         | 21 +++++-----
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  8 ++--
 drivers/gpu/drm/omapdrm/omap_gem.c        | 70 +++++++++++++++----------------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  8 ++--
 5 files changed, 57 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index 5641ba2c2825..a52987f45195 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -198,14 +198,14 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll);
 void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff);
 void omap_gem_dma_sync(struct drm_gem_object *obj,
 		enum dma_data_direction dir);
-int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr);
+int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *dma_addr);
 void omap_gem_put_paddr(struct drm_gem_object *obj);
 int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages,
 		bool remap);
 int omap_gem_put_pages(struct drm_gem_object *obj);
 uint32_t omap_gem_flags(struct drm_gem_object *obj);
-int omap_gem_rotated_paddr(struct drm_gem_object *obj, uint32_t orient,
-		int x, int y, dma_addr_t *paddr);
+int omap_gem_rotated_dma_addr(struct drm_gem_object *obj, uint32_t orient,
+		int x, int y, dma_addr_t *dma_addr);
 uint64_t omap_gem_mmap_offset(struct drm_gem_object *obj);
 size_t omap_gem_mmap_size(struct drm_gem_object *obj);
 int omap_gem_tiled_stride(struct drm_gem_object *obj, uint32_t orient);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index e249c39961df..0cac20a4bdcd 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -74,7 +74,7 @@ struct plane {
 	struct drm_gem_object *bo;
 	uint32_t pitch;
 	uint32_t offset;
-	dma_addr_t paddr;
+	dma_addr_t dma_addr;
 };
 
 #define to_omap_framebuffer(x) container_of(x, struct omap_framebuffer, base)
@@ -85,7 +85,7 @@ struct omap_framebuffer {
 	const struct drm_format_info *format;
 	enum omap_color_mode dss_format;
 	struct plane planes[2];
-	/* lock for pinning (pin_count and planes.paddr) */
+	/* lock for pinning (pin_count and planes.dma_addr) */
 	struct mutex lock;
 };
 
@@ -130,7 +130,7 @@ static uint32_t get_linear_addr(struct plane *plane,
 	       + (x * format->cpp[n] / (n == 0 ? 1 : format->hsub))
 	       + (y * plane->pitch / (n == 0 ? 1 : format->vsub));
 
-	return plane->paddr + offset;
+	return plane->dma_addr + offset;
 }
 
 bool omap_framebuffer_supports_rotation(struct drm_framebuffer *fb)
@@ -201,7 +201,8 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 		if (orient & MASK_X_INVERT)
 			x += w - 1;
 
-		omap_gem_rotated_paddr(plane->bo, orient, x, y, &info->paddr);
+		omap_gem_rotated_dma_addr(plane->bo, orient, x, y,
+					  &info->paddr);
 		info->rotation_type = OMAP_DSS_ROT_TILER;
 		info->screen_width  = omap_gem_tiled_stride(plane->bo, orient);
 	} else {
@@ -232,8 +233,8 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb,
 
 		if (info->rotation_type == OMAP_DSS_ROT_TILER) {
 			WARN_ON(!(omap_gem_flags(plane->bo) & OMAP_BO_TILED));
-			omap_gem_rotated_paddr(plane->bo, orient,
-					x/2, y/2, &info->p_uv_addr);
+			omap_gem_rotated_dma_addr(plane->bo, orient, x/2, y/2,
+						  &info->p_uv_addr);
 		} else {
 			info->p_uv_addr = get_linear_addr(plane, format, 1, x, y);
 		}
@@ -258,7 +259,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		ret = omap_gem_get_paddr(plane->bo, &plane->paddr);
+		ret = omap_gem_get_paddr(plane->bo, &plane->dma_addr);
 		if (ret)
 			goto fail;
 		omap_gem_dma_sync(plane->bo, DMA_TO_DEVICE);
@@ -274,7 +275,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 	for (i--; i >= 0; i--) {
 		struct plane *plane = &omap_fb->planes[i];
 		omap_gem_put_paddr(plane->bo);
-		plane->paddr = 0;
+		plane->dma_addr = 0;
 	}
 
 	mutex_unlock(&omap_fb->lock);
@@ -300,7 +301,7 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
 		omap_gem_put_paddr(plane->bo);
-		plane->paddr = 0;
+		plane->dma_addr = 0;
 	}
 
 	mutex_unlock(&omap_fb->lock);
@@ -458,7 +459,7 @@ struct drm_framebuffer *omap_framebuffer_init(struct drm_device *dev,
 		plane->bo     = bos[i];
 		plane->offset = mode_cmd->offsets[i];
 		plane->pitch  = pitch;
-		plane->paddr  = 0;
+		plane->dma_addr  = 0;
 	}
 
 	drm_helper_mode_fill_fb_struct(dev, fb, mode_cmd);
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 22ab1db2f1bf..37baf196dda3 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -106,7 +106,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	union omap_gem_size gsize;
 	struct fb_info *fbi = NULL;
 	struct drm_mode_fb_cmd2 mode_cmd = {0};
-	dma_addr_t paddr;
+	dma_addr_t dma_addr;
 	int ret;
 
 	sizes->surface_bpp = 32;
@@ -162,7 +162,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	 * to it).  Then we just need to be sure that we are able to re-
 	 * pin it in case of an opps.
 	 */
-	ret = omap_gem_get_paddr(fbdev->bo, &paddr);
+	ret = omap_gem_get_paddr(fbdev->bo, &dma_addr);
 	if (ret) {
 		dev_err(dev->dev,
 			"could not map (paddr)!  Skipping framebuffer alloc\n");
@@ -193,11 +193,11 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
 	drm_fb_helper_fill_var(fbi, helper, sizes->fb_width, sizes->fb_height);
 
-	dev->mode_config.fb_base = paddr;
+	dev->mode_config.fb_base = dma_addr;
 
 	fbi->screen_base = omap_gem_vaddr(fbdev->bo);
 	fbi->screen_size = fbdev->bo->size;
-	fbi->fix.smem_start = paddr;
+	fbi->fix.smem_start = dma_addr;
 	fbi->fix.smem_len = fbdev->bo->size;
 
 	/* if we have DMM, then we can use it for scrolling by just
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 5019aea35451..0f6ffec5cbbb 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -50,7 +50,7 @@ struct omap_gem_object {
 	uint32_t roll;
 
 	/**
-	 * paddr contains the buffer DMA address. It is valid for
+	 * dma_addr contains the buffer DMA address. It is valid for
 	 *
 	 * - buffers allocated through the DMA mapping API (with the
 	 *   OMAP_BO_MEM_DMA_API flag set)
@@ -58,24 +58,24 @@ struct omap_gem_object {
 	 * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)
 	 *   if they are physically contiguous (when sgt->orig_nents == 1)
 	 *
-	 * - buffers mapped through the TILER when paddr_cnt is not zero, in
+	 * - buffers mapped through the TILER when dma_addr_cnt is not zero, in
 	 *   which case the DMA address points to the TILER aperture
 	 *
 	 * Physically contiguous buffers have their DMA address equal to the
 	 * physical address as we don't remap those buffers through the TILER.
 	 *
 	 * Buffers mapped to the TILER have their DMA address pointing to the
-	 * TILER aperture. As TILER mappings are refcounted (through paddr_cnt)
-	 * the DMA address must be accessed through omap_get_get_paddr() to
-	 * ensure that the mapping won't disappear unexpectedly. References must
-	 * be released with omap_gem_put_paddr().
+	 * TILER aperture. As TILER mappings are refcounted (through
+	 * dma_addr_cnt) the DMA address must be accessed through
+	 * omap_get_get_paddr() to ensure that the mapping won't disappear
+	 * unexpectedly. References must be released with omap_gem_put_paddr().
 	 */
-	dma_addr_t paddr;
+	dma_addr_t dma_addr;
 
 	/**
-	 * # of users of paddr
+	 * # of users of dma_addr
 	 */
-	uint32_t paddr_cnt;
+	uint32_t dma_addr_cnt;
 
 	/**
 	 * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag
@@ -132,7 +132,7 @@ struct omap_gem_object {
 #define NUM_USERGART_ENTRIES 2
 struct omap_drm_usergart_entry {
 	struct tiler_block *block;	/* the reserved tiler block */
-	dma_addr_t paddr;
+	dma_addr_t dma_addr;
 	struct drm_gem_object *obj;	/* the current pinned obj */
 	pgoff_t obj_pgoff;		/* page offset of obj currently
 					   mapped in */
@@ -405,7 +405,7 @@ static int fault_1d(struct drm_gem_object *obj,
 		pfn = page_to_pfn(omap_obj->pages[pgoff]);
 	} else {
 		BUG_ON(!is_contiguous(omap_obj));
-		pfn = (omap_obj->paddr >> PAGE_SHIFT) + pgoff;
+		pfn = (omap_obj->dma_addr >> PAGE_SHIFT) + pgoff;
 	}
 
 	VERB("Inserting %p pfn %lx, pa %lx", (void *)vmf->address,
@@ -498,7 +498,7 @@ static int fault_2d(struct drm_gem_object *obj,
 		return ret;
 	}
 
-	pfn = entry->paddr >> PAGE_SHIFT;
+	pfn = entry->dma_addr >> PAGE_SHIFT;
 
 	VERB("Inserting %p pfn %lx, pa %lx", (void *)vmf->address,
 			pfn, pfn << PAGE_SHIFT);
@@ -800,7 +800,7 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 /* Get physical address for DMA.. if the buffer is not already contiguous, remap
  * it to pin in physically contiguous memory.. (ie. map in TILER)
  */
-int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr)
+int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *dma_addr)
 {
 	struct omap_drm_private *priv = obj->dev->dev_private;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
@@ -809,7 +809,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr)
 	mutex_lock(&obj->dev->struct_mutex);
 
 	if (!is_contiguous(omap_obj) && priv->has_dmm) {
-		if (omap_obj->paddr_cnt == 0) {
+		if (omap_obj->dma_addr_cnt == 0) {
 			struct page **pages;
 			uint32_t npages = obj->size >> PAGE_SHIFT;
 			enum tiler_fmt fmt = gem2fmt(omap_obj->flags);
@@ -846,17 +846,17 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *paddr)
 				goto fail;
 			}
 
-			omap_obj->paddr = tiler_ssptr(block);
+			omap_obj->dma_addr = tiler_ssptr(block);
 			omap_obj->block = block;
 
-			DBG("got paddr: %pad", &omap_obj->paddr);
+			DBG("got dma address: %pad", &omap_obj->dma_addr);
 		}
 
-		omap_obj->paddr_cnt++;
+		omap_obj->dma_addr_cnt++;
 
-		*paddr = omap_obj->paddr;
+		*dma_addr = omap_obj->dma_addr;
 	} else if (is_contiguous(omap_obj)) {
-		*paddr = omap_obj->paddr;
+		*dma_addr = omap_obj->dma_addr;
 	} else {
 		ret = -EINVAL;
 		goto fail;
@@ -877,9 +877,9 @@ void omap_gem_put_paddr(struct drm_gem_object *obj)
 	int ret;
 
 	mutex_lock(&obj->dev->struct_mutex);
-	if (omap_obj->paddr_cnt > 0) {
-		omap_obj->paddr_cnt--;
-		if (omap_obj->paddr_cnt == 0) {
+	if (omap_obj->dma_addr_cnt > 0) {
+		omap_obj->dma_addr_cnt--;
+		if (omap_obj->dma_addr_cnt == 0) {
 			ret = tiler_unpin(omap_obj->block);
 			if (ret) {
 				dev_err(obj->dev->dev,
@@ -890,7 +890,7 @@ void omap_gem_put_paddr(struct drm_gem_object *obj)
 				dev_err(obj->dev->dev,
 					"could not release unmap: %d\n", ret);
 			}
-			omap_obj->paddr = 0;
+			omap_obj->dma_addr = 0;
 			omap_obj->block = NULL;
 		}
 	}
@@ -902,16 +902,16 @@ void omap_gem_put_paddr(struct drm_gem_object *obj)
  * specified orientation and x,y offset from top-left corner of buffer
  * (only valid for tiled 2d buffers)
  */
-int omap_gem_rotated_paddr(struct drm_gem_object *obj, uint32_t orient,
-		int x, int y, dma_addr_t *paddr)
+int omap_gem_rotated_dma_addr(struct drm_gem_object *obj, uint32_t orient,
+		int x, int y, dma_addr_t *dma_addr)
 {
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 	int ret = -EINVAL;
 
 	mutex_lock(&obj->dev->struct_mutex);
-	if ((omap_obj->paddr_cnt > 0) && omap_obj->block &&
+	if ((omap_obj->dma_addr_cnt > 0) && omap_obj->block &&
 			(omap_obj->flags & OMAP_BO_TILED)) {
-		*paddr = tiler_tsptr(omap_obj->block, orient, x, y);
+		*dma_addr = tiler_tsptr(omap_obj->block, orient, x, y);
 		ret = 0;
 	}
 	mutex_unlock(&obj->dev->struct_mutex);
@@ -1032,7 +1032,7 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
 
 	seq_printf(m, "%08x: %2d (%2d) %08llx %pad (%2d) %p %4d",
 			omap_obj->flags, obj->name, kref_read(&obj->refcount),
-			off, &omap_obj->paddr, omap_obj->paddr_cnt,
+			off, &omap_obj->dma_addr, omap_obj->dma_addr_cnt,
 			omap_obj->vaddr, omap_obj->roll);
 
 	if (omap_obj->flags & OMAP_BO_TILED) {
@@ -1289,7 +1289,7 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 	/* this means the object is still pinned.. which really should
 	 * not happen.  I think..
 	 */
-	WARN_ON(omap_obj->paddr_cnt > 0);
+	WARN_ON(omap_obj->dma_addr_cnt > 0);
 
 	if (omap_obj->pages) {
 		if (omap_obj->flags & OMAP_BO_MEM_DMABUF)
@@ -1300,7 +1300,7 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 
 	if (omap_obj->flags & OMAP_BO_MEM_DMA_API) {
 		dma_free_wc(dev->dev, obj->size, omap_obj->vaddr,
-			    omap_obj->paddr);
+			    omap_obj->dma_addr);
 	} else if (omap_obj->vaddr) {
 		vunmap(omap_obj->vaddr);
 	} else if (obj->import_attach) {
@@ -1399,7 +1399,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
 	/* Allocate memory if needed. */
 	if (flags & OMAP_BO_MEM_DMA_API) {
 		omap_obj->vaddr = dma_alloc_wc(dev->dev, size,
-					       &omap_obj->paddr,
+					       &omap_obj->dma_addr,
 					       GFP_KERNEL);
 		if (!omap_obj->vaddr)
 			goto err_release;
@@ -1443,7 +1443,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,
 	omap_obj->sgt = sgt;
 
 	if (sgt->orig_nents == 1) {
-		omap_obj->paddr = sg_dma_address(sgt->sgl);
+		omap_obj->dma_addr = sg_dma_address(sgt->sgl);
 	} else {
 		/* Create pages list from sgt */
 		struct sg_page_iter iter;
@@ -1550,11 +1550,11 @@ void omap_gem_init(struct drm_device *dev)
 						i, j, PTR_ERR(block));
 				return;
 			}
-			entry->paddr = tiler_ssptr(block);
+			entry->dma_addr = tiler_ssptr(block);
 			entry->block = block;
 
-			DBG("%d:%d: %dx%d: paddr=%pad stride=%d", i, j, w, h,
-					&entry->paddr,
+			DBG("%d:%d: %dx%d: dma_addr=%pad stride=%d", i, j, w, h,
+					&entry->dma_addr,
 					usergart[i].stride_pfn << PAGE_SHIFT);
 		}
 	}
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 7f380eb31afb..79e1d36dc1c7 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -31,7 +31,7 @@ static struct sg_table *omap_gem_map_dma_buf(
 {
 	struct drm_gem_object *obj = attachment->dmabuf->priv;
 	struct sg_table *sg;
-	dma_addr_t paddr;
+	dma_addr_t dma_addr;
 	int ret;
 
 	sg = kzalloc(sizeof(*sg), GFP_KERNEL);
@@ -41,7 +41,7 @@ static struct sg_table *omap_gem_map_dma_buf(
 	/* camera, etc, need physically contiguous.. but we need a
 	 * better way to know this..
 	 */
-	ret = omap_gem_get_paddr(obj, &paddr);
+	ret = omap_gem_get_paddr(obj, &dma_addr);
 	if (ret)
 		goto out;
 
@@ -51,8 +51,8 @@ static struct sg_table *omap_gem_map_dma_buf(
 
 	sg_init_table(sg->sgl, 1);
 	sg_dma_len(sg->sgl) = obj->size;
-	sg_set_page(sg->sgl, pfn_to_page(PFN_DOWN(paddr)), obj->size, 0);
-	sg_dma_address(sg->sgl) = paddr;
+	sg_set_page(sg->sgl, pfn_to_page(PFN_DOWN(dma_addr)), obj->size, 0);
+	sg_dma_address(sg->sgl) = dma_addr;
 
 	/* this should be after _get_paddr() to ensure we have pages attached */
 	omap_gem_dma_sync(obj, dir);
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/7] drm: omapdrm: Rename omap_gem_(get|put)_paddr() to omap_gem_(un)pin()

[Laurent Pinchart]

The reflects the purpose of the function better.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h        |  4 ++--
 drivers/gpu/drm/omapdrm/omap_fb.c         |  6 ++---
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |  9 ++++----
 drivers/gpu/drm/omapdrm/omap_gem.c        | 38 ++++++++++++++++++++++---------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  6 ++---
 5 files changed, 39 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index a52987f45195..4e677c069598 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -198,8 +198,8 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll);
 void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff);
 void omap_gem_dma_sync(struct drm_gem_object *obj,
 		enum dma_data_direction dir);
-int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *dma_addr);
-void omap_gem_put_paddr(struct drm_gem_object *obj);
+int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr);
+void omap_gem_unpin(struct drm_gem_object *obj);
 int omap_gem_get_pages(struct drm_gem_object *obj, struct page ***pages,
 		bool remap);
 int omap_gem_put_pages(struct drm_gem_object *obj);
diff --git a/drivers/gpu/drm/omapdrm/omap_fb.c b/drivers/gpu/drm/omapdrm/omap_fb.c
index 0cac20a4bdcd..a25a99fc6a34 100644
--- a/drivers/gpu/drm/omapdrm/omap_fb.c
+++ b/drivers/gpu/drm/omapdrm/omap_fb.c
@@ -259,7 +259,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		ret = omap_gem_get_paddr(plane->bo, &plane->dma_addr);
+		ret = omap_gem_pin(plane->bo, &plane->dma_addr);
 		if (ret)
 			goto fail;
 		omap_gem_dma_sync(plane->bo, DMA_TO_DEVICE);
@@ -274,7 +274,7 @@ int omap_framebuffer_pin(struct drm_framebuffer *fb)
 fail:
 	for (i--; i >= 0; i--) {
 		struct plane *plane = &omap_fb->planes[i];
-		omap_gem_put_paddr(plane->bo);
+		omap_gem_unpin(plane->bo);
 		plane->dma_addr = 0;
 	}
 
@@ -300,7 +300,7 @@ void omap_framebuffer_unpin(struct drm_framebuffer *fb)
 
 	for (i = 0; i < n; i++) {
 		struct plane *plane = &omap_fb->planes[i];
-		omap_gem_put_paddr(plane->bo);
+		omap_gem_unpin(plane->bo);
 		plane->dma_addr = 0;
 	}
 
diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c
index 37baf196dda3..2cbb5f25816f 100644
--- a/drivers/gpu/drm/omapdrm/omap_fbdev.c
+++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c
@@ -162,10 +162,9 @@ static int omap_fbdev_create(struct drm_fb_helper *helper,
 	 * to it).  Then we just need to be sure that we are able to re-
 	 * pin it in case of an opps.
 	 */
-	ret = omap_gem_get_paddr(fbdev->bo, &dma_addr);
+	ret = omap_gem_pin(fbdev->bo, &dma_addr);
 	if (ret) {
-		dev_err(dev->dev,
-			"could not map (paddr)!  Skipping framebuffer alloc\n");
+		dev_err(dev->dev, "could not pin framebuffer\n");
 		ret = -ENOMEM;
 		goto fail;
 	}
@@ -307,8 +306,8 @@ void omap_fbdev_free(struct drm_device *dev)
 
 	fbdev = to_omap_fbdev(priv->fbdev);
 
-	/* release the ref taken in omap_fbdev_create() */
-	omap_gem_put_paddr(fbdev->bo);
+	/* unpin the GEM object pinned in omap_fbdev_create() */
+	omap_gem_unpin(fbdev->bo);
 
 	/* this will free the backing object */
 	if (fbdev->fb)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 0f6ffec5cbbb..96d881bd3a3c 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -66,9 +66,9 @@ struct omap_gem_object {
 	 *
 	 * Buffers mapped to the TILER have their DMA address pointing to the
 	 * TILER aperture. As TILER mappings are refcounted (through
-	 * dma_addr_cnt) the DMA address must be accessed through
-	 * omap_get_get_paddr() to ensure that the mapping won't disappear
-	 * unexpectedly. References must be released with omap_gem_put_paddr().
+	 * dma_addr_cnt) the DMA address must be accessed through omap_gem_pin()
+	 * to ensure that the mapping won't disappear unexpectedly. References
+	 * must be released with omap_gem_unpin().
 	 */
 	dma_addr_t dma_addr;
 
@@ -797,10 +797,21 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 	}
 }
 
-/* Get physical address for DMA.. if the buffer is not already contiguous, remap
- * it to pin in physically contiguous memory.. (ie. map in TILER)
+/**
+ * omap_gem_pin() - Pin a GEM object in memory
+ * @obj: the GEM object
+ * @dma_addr: the DMA address
+ *
+ * Pin the given GEM object in memory and fill the dma_addr pointer with the
+ * object's DMA address. If the buffer is not physically contiguous it will be
+ * remapped through the TILER to provide a contiguous view.
+ *
+ * Pins are reference-counted, calling this function multiple times is allowed
+ * as long the corresponding omap_gem_unpin() calls are balanced.
+ *
+ * Return 0 on success or a negative error code otherwise.
  */
-int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *dma_addr)
+int omap_gem_pin(struct drm_gem_object *obj, dma_addr_t *dma_addr)
 {
 	struct omap_drm_private *priv = obj->dev->dev_private;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
@@ -868,10 +879,15 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, dma_addr_t *dma_addr)
 	return ret;
 }
 
-/* Release physical address, when DMA is no longer being performed.. this
- * could potentially unpin and unmap buffers from TILER
+/**
+ * omap_gem_unpin() - Unpin a GEM object from memory
+ * @obj: the GEM object
+ *
+ * Unpin the given GEM object previously pinned with omap_gem_pin(). Pins are
+ * reference-counted, the actualy unpin will only be performed when the number
+ * of calls to this function matches the number of calls to omap_gem_pin().
  */
-void omap_gem_put_paddr(struct drm_gem_object *obj)
+void omap_gem_unpin(struct drm_gem_object *obj)
 {
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 	int ret;
@@ -932,9 +948,9 @@ int omap_gem_tiled_stride(struct drm_gem_object *obj, uint32_t orient)
  * increasing the pin count (which we don't really do yet anyways,
  * because we don't support swapping pages back out).  And 'remap'
  * might not be quite the right name, but I wanted to keep it working
- * similarly to omap_gem_get_paddr().  Note though that mutex is not
+ * similarly to omap_gem_pin().  Note though that mutex is not
  * aquired if !remap (because this can be called in atomic ctxt),
- * but probably omap_gem_get_paddr() should be changed to work in the
+ * but probably omap_gem_unpin() should be changed to work in the
  * same way.  If !remap, a matching omap_gem_put_pages() call is not
  * required (and should not be made).
  */
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 79e1d36dc1c7..a4e22f0065c0 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -41,7 +41,7 @@ static struct sg_table *omap_gem_map_dma_buf(
 	/* camera, etc, need physically contiguous.. but we need a
 	 * better way to know this..
 	 */
-	ret = omap_gem_get_paddr(obj, &dma_addr);
+	ret = omap_gem_pin(obj, &dma_addr);
 	if (ret)
 		goto out;
 
@@ -54,7 +54,7 @@ static struct sg_table *omap_gem_map_dma_buf(
 	sg_set_page(sg->sgl, pfn_to_page(PFN_DOWN(dma_addr)), obj->size, 0);
 	sg_dma_address(sg->sgl) = dma_addr;
 
-	/* this should be after _get_paddr() to ensure we have pages attached */
+	/* this must be after omap_gem_pin() to ensure we have pages attached */
 	omap_gem_dma_sync(obj, dir);
 
 	return sg;
@@ -67,7 +67,7 @@ static void omap_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
 		struct sg_table *sg, enum dma_data_direction dir)
 {
 	struct drm_gem_object *obj = attachment->dmabuf->priv;
-	omap_gem_put_paddr(obj);
+	omap_gem_unpin(obj);
 	sg_free_table(sg);
 	kfree(sg);
 }
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm: omapdrm: Lower indentation level in omap_gem_dma_sync_buffer()

[Laurent Pinchart]

This makes the function more readable.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 43 +++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 96d881bd3a3c..5ee7ac3d5be3 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -765,36 +765,35 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 {
 	struct drm_device *dev = obj->dev;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
+	int i, npages = obj->size >> PAGE_SHIFT;
+	struct page **pages = omap_obj->pages;
+	bool dirty = false;
 
-	if (is_cached_coherent(obj)) {
-		int i, npages = obj->size >> PAGE_SHIFT;
-		struct page **pages = omap_obj->pages;
-		bool dirty = false;
-
-		for (i = 0; i < npages; i++) {
-			if (!omap_obj->addrs[i]) {
-				dma_addr_t addr;
+	if (!is_cached_coherent(obj))
+		return;
 
-				addr = dma_map_page(dev->dev, pages[i], 0,
-						PAGE_SIZE, DMA_BIDIRECTIONAL);
+	for (i = 0; i < npages; i++) {
+		if (!omap_obj->addrs[i]) {
+			dma_addr_t addr;
 
-				if (dma_mapping_error(dev->dev, addr)) {
-					dev_warn(dev->dev,
-						"%s: failed to map page\n",
-						__func__);
-					break;
-				}
+			addr = dma_map_page(dev->dev, pages[i], 0,
+					    PAGE_SIZE, DMA_BIDIRECTIONAL);
 
-				dirty = true;
-				omap_obj->addrs[i] = addr;
+			if (dma_mapping_error(dev->dev, addr)) {
+				dev_warn(dev->dev, "%s: failed to map page\n",
+					__func__);
+				break;
 			}
-		}
 
-		if (dirty) {
-			unmap_mapping_range(obj->filp->f_mapping, 0,
-					omap_gem_mmap_size(obj), 1);
+			dirty = true;
+			omap_obj->addrs[i] = addr;
 		}
 	}
+
+	if (dirty) {
+		unmap_mapping_range(obj->filp->f_mapping, 0,
+				    omap_gem_mmap_size(obj), 1);
+	}
 }
 
 /**
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm: omapdrm: Rename the omap_gem_object addrs field to dma_addrs

[Laurent Pinchart]

The field contains DMA addresses, clarify that by renaming it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 5ee7ac3d5be3..945bda8e330d 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -95,7 +95,7 @@ struct omap_gem_object {
 	struct page **pages;
 
 	/** addresses corresponding to pages in above array */
-	dma_addr_t *addrs;
+	dma_addr_t *dma_addrs;
 
 	/**
 	 * Virtual address, if mapped.
@@ -290,7 +290,7 @@ static int omap_gem_attach_pages(struct drm_gem_object *obj)
 		}
 	}
 
-	omap_obj->addrs = addrs;
+	omap_obj->dma_addrs = addrs;
 	omap_obj->pages = pages;
 
 	return 0;
@@ -336,15 +336,15 @@ static void omap_gem_detach_pages(struct drm_gem_object *obj)
 	if (omap_obj->flags & (OMAP_BO_WC|OMAP_BO_UNCACHED)) {
 		int i, npages = obj->size >> PAGE_SHIFT;
 		for (i = 0; i < npages; i++) {
-			if (omap_obj->addrs[i])
+			if (omap_obj->dma_addrs[i])
 				dma_unmap_page(obj->dev->dev,
-					       omap_obj->addrs[i],
+					       omap_obj->dma_addrs[i],
 					       PAGE_SIZE, DMA_BIDIRECTIONAL);
 		}
 	}
 
-	kfree(omap_obj->addrs);
-	omap_obj->addrs = NULL;
+	kfree(omap_obj->dma_addrs);
+	omap_obj->dma_addrs = NULL;
 
 	drm_gem_put_pages(obj, omap_obj->pages, true, false);
 	omap_obj->pages = NULL;
@@ -752,10 +752,10 @@ void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff)
 	struct drm_device *dev = obj->dev;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 
-	if (is_cached_coherent(obj) && omap_obj->addrs[pgoff]) {
-		dma_unmap_page(dev->dev, omap_obj->addrs[pgoff],
+	if (is_cached_coherent(obj) && omap_obj->dma_addrs[pgoff]) {
+		dma_unmap_page(dev->dev, omap_obj->dma_addrs[pgoff],
 				PAGE_SIZE, DMA_BIDIRECTIONAL);
-		omap_obj->addrs[pgoff] = 0;
+		omap_obj->dma_addrs[pgoff] = 0;
 	}
 }
 
@@ -773,7 +773,7 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 		return;
 
 	for (i = 0; i < npages; i++) {
-		if (!omap_obj->addrs[i]) {
+		if (!omap_obj->dma_addrs[i]) {
 			dma_addr_t addr;
 
 			addr = dma_map_page(dev->dev, pages[i], 0,
@@ -786,7 +786,7 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 			}
 
 			dirty = true;
-			omap_obj->addrs[i] = addr;
+			omap_obj->dma_addrs[i] = addr;
 		}
 	}
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/7] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Laurent Pinchart]

The is_cache_coherent() function currently returns true when the mapping
is not cache-coherent. This isn't a bug as such as the callers interpret
cache-coherent as meaning that the driver has to handle the coherency
manually, but it is nonetheless very confusing. Fix it and add a bit
more documentation to explain how cached buffers are handled.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 945bda8e330d..59594ec0d159 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -732,16 +732,21 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll)
  * Memory Management & DMA Sync
  */
 
-/**
- * shmem buffers that are mapped cached can simulate coherency via using
- * page faulting to keep track of dirty pages
+/*
+ * shmem buffers that are mapped cached are not coherent.
+ *
+ * We keep track of dirty pages using page faulting to perform cache management.
+ * When a page is mapped to the CPU in read/write mode the device can't access
+ * it and omap_obj->dma_addrs[i] is NULL. When a page is mapped to the device
+ * the omap_obj->dma_addrs[i] is set to the DMA address, and the page is
+ * unmapped from the CPU.
  */
 static inline bool is_cached_coherent(struct drm_gem_object *obj)
 {
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 
-	return (omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
-		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED);
+	return !((omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
+		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED));
 }
 
 /* Sync the buffer for CPU access.. note pages should already be
@@ -752,7 +757,10 @@ void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff)
 	struct drm_device *dev = obj->dev;
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
 
-	if (is_cached_coherent(obj) && omap_obj->dma_addrs[pgoff]) {
+	if (!is_cached_coherent(obj))
+		return;
+
+	if (omap_obj->dma_addrs[pgoff]) {
 		dma_unmap_page(dev->dev, omap_obj->dma_addrs[pgoff],
 				PAGE_SIZE, DMA_BIDIRECTIONAL);
 		omap_obj->dma_addrs[pgoff] = 0;
@@ -769,7 +777,7 @@ void omap_gem_dma_sync(struct drm_gem_object *obj,
 	struct page **pages = omap_obj->pages;
 	bool dirty = false;
 
-	if (!is_cached_coherent(obj))
+	if (is_cached_coherent(obj))
 		return;
 
 	for (i = 0; i < npages; i++) {
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2394 bytes --]

On 10/03/17 11:39, Laurent Pinchart wrote:
> The is_cache_coherent() function currently returns true when the mapping
> is not cache-coherent. This isn't a bug as such as the callers interpret
> cache-coherent as meaning that the driver has to handle the coherency
> manually, but it is nonetheless very confusing. Fix it and add a bit
> more documentation to explain how cached buffers are handled.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_gem.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index 945bda8e330d..59594ec0d159 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -732,16 +732,21 @@ int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll)
>   * Memory Management & DMA Sync
>   */
>  
> -/**
> - * shmem buffers that are mapped cached can simulate coherency via using
> - * page faulting to keep track of dirty pages
> +/*
> + * shmem buffers that are mapped cached are not coherent.
> + *
> + * We keep track of dirty pages using page faulting to perform cache management.
> + * When a page is mapped to the CPU in read/write mode the device can't access
> + * it and omap_obj->dma_addrs[i] is NULL. When a page is mapped to the device
> + * the omap_obj->dma_addrs[i] is set to the DMA address, and the page is
> + * unmapped from the CPU.
>   */
>  static inline bool is_cached_coherent(struct drm_gem_object *obj)
>  {
>  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
>  
> -	return (omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> -		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED);
> +	return !((omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> +		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED));
>  }
>  
>  /* Sync the buffer for CPU access.. note pages should already be
> @@ -752,7 +757,10 @@ void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff)
>  	struct drm_device *dev = obj->dev;
>  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
>  
> -	if (is_cached_coherent(obj) && omap_obj->dma_addrs[pgoff]) {
> +	if (!is_cached_coherent(obj))
> +		return;

I think the ! is extra there. If the obj _is_ coherent, we can just return.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm: omapdrm: Fix incorrect usage of the term 'cache coherency'

[Laurent Pinchart]

Hi Tomi,

On Friday 10 Mar 2017 14:13:08 Tomi Valkeinen wrote:
> On 10/03/17 11:39, Laurent Pinchart wrote:
> > The is_cache_coherent() function currently returns true when the mapping
> > is not cache-coherent. This isn't a bug as such as the callers interpret
> > cache-coherent as meaning that the driver has to handle the coherency
> > manually, but it is nonetheless very confusing. Fix it and add a bit
> > more documentation to explain how cached buffers are handled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_gem.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c
> > b/drivers/gpu/drm/omapdrm/omap_gem.c index 945bda8e330d..59594ec0d159
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> > @@ -732,16 +732,21 @@ int omap_gem_roll(struct drm_gem_object *obj,
> > uint32_t roll)> 
> >   * Memory Management & DMA Sync
> >   */
> > 
> > -/**
> > - * shmem buffers that are mapped cached can simulate coherency via using
> > - * page faulting to keep track of dirty pages
> > +/*
> > + * shmem buffers that are mapped cached are not coherent.
> > + *
> > + * We keep track of dirty pages using page faulting to perform cache
> > management. + * When a page is mapped to the CPU in read/write mode the
> > device can't access + * it and omap_obj->dma_addrs[i] is NULL. When a
> > page is mapped to the device + * the omap_obj->dma_addrs[i] is set to the
> > DMA address, and the page is + * unmapped from the CPU.
> > 
> >   */
> >  
> >  static inline bool is_cached_coherent(struct drm_gem_object *obj)
> >  {
> >  
> >  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> > 
> > -	return (omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> > -		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED);
> > +	return !((omap_obj->flags & OMAP_BO_MEM_SHMEM) &&
> > +		((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED));
> > 
> >  }
> >  
> >  /* Sync the buffer for CPU access.. note pages should already be
> > 
> > @@ -752,7 +757,10 @@ void omap_gem_cpu_sync(struct drm_gem_object *obj,
> > int pgoff)> 
> >  	struct drm_device *dev = obj->dev;
> >  	struct omap_gem_object *omap_obj = to_omap_bo(obj);
> > 
> > -	if (is_cached_coherent(obj) && omap_obj->dma_addrs[pgoff]) {
> > +	if (!is_cached_coherent(obj))
> > +		return;
> 
> I think the ! is extra there. If the obj _is_ coherent, we can just return.

You're right, my bad. I'll fix that in v2.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm: omapdrm: DMA-unmap pages for all buffer types when freeing buffers

[Laurent Pinchart]

Both coherent (uncached) and non-coherent (cached) buffers can have
their pages mapped to the device through the DMA mapping API. Make sure
to unmap any mapped page when freeing a buffer, regardless of its type.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_gem.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 59594ec0d159..22859d40b251 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -329,18 +329,13 @@ static int get_pages(struct drm_gem_object *obj, struct page ***pages)
 static void omap_gem_detach_pages(struct drm_gem_object *obj)
 {
 	struct omap_gem_object *omap_obj = to_omap_bo(obj);
+	unsigned int npages = obj->size >> PAGE_SHIFT;
+	unsigned int i;
 
-	/* for non-cached buffers, ensure the new pages are clean because
-	 * DSS, GPU, etc. are not cache coherent:
-	 */
-	if (omap_obj->flags & (OMAP_BO_WC|OMAP_BO_UNCACHED)) {
-		int i, npages = obj->size >> PAGE_SHIFT;
-		for (i = 0; i < npages; i++) {
-			if (omap_obj->dma_addrs[i])
-				dma_unmap_page(obj->dev->dev,
-					       omap_obj->dma_addrs[i],
-					       PAGE_SIZE, DMA_BIDIRECTIONAL);
-		}
+	for (i = 0; i < npages; i++) {
+		if (omap_obj->dma_addrs[i])
+			dma_unmap_page(obj->dev->dev, omap_obj->dma_addrs[i],
+				       PAGE_SIZE, DMA_BIDIRECTIONAL);
 	}
 
 	kfree(omap_obj->dma_addrs);
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] omapdrm: Fix GEM objects DMA unmapping

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2299 bytes --]

On 10/03/17 11:39, Laurent Pinchart wrote:
> Hello,
> 
> Memory leaks have been reported when allocating a cached omap_bo (with
> OMAP_BO_CACHED. Investigation showed that this can only come from the DMA
> mapping debug layer, as on ARM32 the non-coherent, non-IOMMU DMA mapping code
> doesn't allocate memory to map a page (and kmemcheck facility is only
> available on x86, so it can't be a source of memory leaks either on ARM).
> 
> The DMA debug layer pre-allocates DMA debugging entries and stores them in a
> list. As the omapdrm driver maps cached buffer page by page, the list of 4096
> pre-allocated entries is starved very soon. However, raising the number of DMA
> mapping debug entries to 32 * 4096 (through the dma_debug_entries kernel
> command line argument) led to more interesting results.
> 
> The number of entries being large enough to handle all the pages mapped by
> kmstest, monitoring the DMA mapping statistics through
> /sys/kernel/debug/dma-api/ showed that the number of free entries dropped
> significantly when kmstest was started and didn't raise when it was stopped.
> In particular, running kmstest without flipping resulting in a drop of 1266
> free entries, which corresponds to one 1440x900 framebuffer in XR24. The
> proved that the pages backing the framebuffer, while freed when the
> framebuffer was destroyed, were not unmapped.
> 
> I've thus started investigating the driver GEM implementation. After a few
> confusing moments that resulted in the 1/7 to 6/7 cleanup patches, I wrote
> patch 7/7 that should fix the issue.

So, possibly this series could be applied with the one issue fixed that
I reported, as it looks like a nice cleanup, but I think this is still
far from making cached bos usable and I would rather have it all in one
series.

One issue is that if creating a cached buffer without DMM,
omap_gem_mmap_obj() will hit if (WARN_ON(!obj->filp)) and break.

The other is that the cached bos are unusably slow. I hope that is
because of the constant fault, map, unmap cycle, and can be fixed by
keeping the buffer mapped and using explicit sync ioctls.

But if the performance issue cannot be solved, then I think we should
just drop the cached bo support as it's unusable at the moment.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel