[PATCH 0/6] DRM: Unified VMA Offset Manager

[PATCH 0/6] DRM: Unified VMA Offset Manager

[David Herrmann]

Hi

I picked up the initial work from Dave [1], fixed several bugs, rewrote the
drm_mm node handling and adjusted the different drivers.
The series tries to replace the VMA-offset managers from GEM and TTM with a
single unified implementation. It uses the TTM RBTree idea to allow sub-mappings
(which wouldn't be feasible with hashtables).

Changes to Dave's v1:
 * Fixed a ref-count bug in TTM during object lookup
 * Use embedded drm_mm_node objects to avoid allocations
 * Document drm_vma_* API
 * Reviewed TTM locking
 * Fixed all new drivers
 * Use node->vm_pages instead of obj->size for GEM size calculations

Notes:
 * Tested on nouveau only! I will try to test i915 this week. However, the
   gem changes seem pretty trivial.
 * I couldn't even compile-test the ARM drivers. However, the omapdrm diffs
   are the only changes that are non-trivial. Is there any ongoing work to
   remove the arch-deps in DRM drivers?
 * _DRM_GEM is no longer used, but I guess we need to keep it for backwards
   compat?
 * If we replace node_list in drm_mm with an rbtree, we can drop it from
   drm_vma_offset_manager completely. However, I wanted to avoid heavy drm_mm
   changes and left this for follow up patches.
 * This is currently based on linux-3.10 from today. Next series will be
   rebased on drm-next/linux-next, but the current -next trees continously break
   my machines..
   But the only changes should be to fix additional drivers. I didn't see any
   other things to fix for drm-next.

Another series, which I will send later, adds "struct file" lists for each
drm-vma-offset so we can get access control over gem objects. Also, I have an
experimental series to remove the allocation helpers in drm_mm and let drivers
embed drm_mm_node instead. Lets see how that works out.

Comments welcome!
Cheers
David

[1]: http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-vma-manager

David Herrmann (6):
  drm: make drm_mm_init() return void
  drm: mm: add drm_mm_node_linked() helper
  drm: add unified vma offset manager
  drm: gem: convert to new unified vma manager
  drm: ttm: convert to unified vma offset manager
  drm: provide generic drm_vma_node_unmap() helper

 drivers/gpu/drm/Makefile                   |   2 +-
 drivers/gpu/drm/ast/ast_main.c             |   2 +-
 drivers/gpu/drm/cirrus/cirrus_main.c       |   2 +-
 drivers/gpu/drm/drm_gem.c                  |  93 ++----------
 drivers/gpu/drm/drm_gem_cma_helper.c       |   9 +-
 drivers/gpu/drm/drm_mm.c                   |   5 +-
 drivers/gpu/drm/drm_vma_manager.c          | 224 +++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_gem.c    |   7 +-
 drivers/gpu/drm/gma500/gem.c               |   8 +-
 drivers/gpu/drm/i915/i915_gem.c            |  13 +-
 drivers/gpu/drm/mgag200/mgag200_main.c     |   2 +-
 drivers/gpu/drm/nouveau/nouveau_display.c  |   2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c      |   2 +-
 drivers/gpu/drm/omapdrm/omap_gem.c         |  11 +-
 drivers/gpu/drm/omapdrm/omap_gem_helpers.c |  49 +------
 drivers/gpu/drm/qxl/qxl_object.h           |   2 +-
 drivers/gpu/drm/qxl/qxl_release.c          |   2 +-
 drivers/gpu/drm/radeon/radeon_object.h     |   5 +-
 drivers/gpu/drm/ttm/ttm_bo.c               |  82 ++---------
 drivers/gpu/drm/ttm/ttm_bo_manager.c       |   8 +-
 drivers/gpu/drm/ttm/ttm_bo_util.c          |   3 +-
 drivers/gpu/drm/ttm/ttm_bo_vm.c            |  81 ++++-------
 drivers/gpu/drm/udl/udl_gem.c              |   6 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |   4 +-
 include/drm/drmP.h                         |   7 +-
 include/drm/drm_mm.h                       |  11 +-
 include/drm/drm_vma_manager.h              | 122 ++++++++++++++++
 include/drm/ttm/ttm_bo_api.h               |  15 +-
 include/drm/ttm/ttm_bo_driver.h            |   7 +-
 include/uapi/drm/drm.h                     |   2 +-
 30 files changed, 464 insertions(+), 324 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_vma_manager.c
 create mode 100644 include/drm/drm_vma_manager.h

-- 
1.8.3.2

[PATCH 1/6] drm: make drm_mm_init() return void

[David Herrmann]

There is no reason to return "int" as this function never fails.
Furthermore, several drivers (ast, sis) already depend on this.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_gem.c            | 8 ++------
 drivers/gpu/drm/drm_mm.c             | 4 +---
 drivers/gpu/drm/ttm/ttm_bo.c         | 6 +-----
 drivers/gpu/drm/ttm/ttm_bo_manager.c | 8 +-------
 include/drm/drm_mm.h                 | 6 +++---
 5 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index cf919e3..88f0322 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -108,12 +108,8 @@ drm_gem_init(struct drm_device *dev)
 		return -ENOMEM;
 	}
 
-	if (drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
-			DRM_FILE_PAGE_OFFSET_SIZE)) {
-		drm_ht_remove(&mm->offset_hash);
-		kfree(mm);
-		return -ENOMEM;
-	}
+	drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
+		    DRM_FILE_PAGE_OFFSET_SIZE);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 07cf99c..7917729 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -669,7 +669,7 @@ int drm_mm_clean(struct drm_mm * mm)
 }
 EXPORT_SYMBOL(drm_mm_clean);
 
-int drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
+void drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
 {
 	INIT_LIST_HEAD(&mm->hole_stack);
 	INIT_LIST_HEAD(&mm->unused_nodes);
@@ -690,8 +690,6 @@ int drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
 	list_add_tail(&mm->head_node.hole_stack, &mm->hole_stack);
 
 	mm->color_adjust = NULL;
-
-	return 0;
 }
 EXPORT_SYMBOL(drm_mm_init);
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 9b07b7d..e97c5a0 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1619,9 +1619,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
 		goto out_no_sys;
 
 	bdev->addr_space_rb = RB_ROOT;
-	ret = drm_mm_init(&bdev->addr_space_mm, file_page_offset, 0x10000000);
-	if (unlikely(ret != 0))
-		goto out_no_addr_mm;
+	drm_mm_init(&bdev->addr_space_mm, file_page_offset, 0x10000000);
 
 	INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
 	INIT_LIST_HEAD(&bdev->ddestroy);
@@ -1635,8 +1633,6 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
 	mutex_unlock(&glob->device_list_mutex);
 
 	return 0;
-out_no_addr_mm:
-	ttm_bo_clean_mm(bdev, 0);
 out_no_sys:
 	return ret;
 }
diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
index 9212494..e4367f9 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
@@ -103,18 +103,12 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
 			   unsigned long p_size)
 {
 	struct ttm_range_manager *rman;
-	int ret;
 
 	rman = kzalloc(sizeof(*rman), GFP_KERNEL);
 	if (!rman)
 		return -ENOMEM;
 
-	ret = drm_mm_init(&rman->mm, 0, p_size);
-	if (ret) {
-		kfree(rman);
-		return ret;
-	}
-
+	drm_mm_init(&rman->mm, 0, p_size);
 	spin_lock_init(&rman->lock);
 	man->priv = rman;
 	return 0;
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 88591ef..de92425 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -275,9 +275,9 @@ static inline  struct drm_mm_node *drm_mm_search_free_in_range_color(
 	return drm_mm_search_free_in_range_generic(mm, size, alignment, color,
 						   start, end, best_match);
 }
-extern int drm_mm_init(struct drm_mm *mm,
-		       unsigned long start,
-		       unsigned long size);
+extern void drm_mm_init(struct drm_mm *mm,
+			unsigned long start,
+			unsigned long size);
 extern void drm_mm_takedown(struct drm_mm *mm);
 extern int drm_mm_clean(struct drm_mm *mm);
 extern int drm_mm_pre_get(struct drm_mm *mm);
-- 
1.8.3.2

[PATCH 2/6] drm: mm: add drm_mm_node_linked() helper

[David Herrmann]

This helper tests whether a given node is currently linked into a drm_mm
manager. We use the node->mm pointer for that as it is set for all linked
objects.

We also reset node->mm whenever a node is removed. All such access is
currently safe as everyone calls kfree() on the object directly after
removing it. Furthermore, no-one is supposed to access node->mm from
outside drm_mm.c, anyway.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_mm.c | 1 +
 include/drm/drm_mm.h     | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 7917729..3bd43ce 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -374,6 +374,7 @@ void drm_mm_remove_node(struct drm_mm_node *node)
 
 	list_del(&node->node_list);
 	node->allocated = 0;
+	node->mm = NULL;
 }
 EXPORT_SYMBOL(drm_mm_remove_node);
 
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index de92425..d83b966 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -85,6 +85,11 @@ static inline bool drm_mm_node_allocated(struct drm_mm_node *node)
 	return node->allocated;
 }
 
+static inline bool drm_mm_node_linked(struct drm_mm_node *node)
+{
+	return node->mm;
+}
+
 static inline bool drm_mm_initialized(struct drm_mm *mm)
 {
 	return mm->hole_stack.next;
-- 
1.8.3.2

[PATCH 3/6] drm: add unified vma offset manager

[David Herrmann]

If we want to map GPU memory into user-space, we need to linearize the
addresses to not confuse mm-core. Currently, GEM and TTM both implement
their own offset-managers to assign a pgoff to each object for user-space
CPU access. GEM uses a hash-table, TTM uses an rbtree.

This patch provides a unified implementation that can be used to replace
both. TTM allows partial mmaps with a given offset, so we cannot use
hashtables as the start address may not be known at mmap time. Hence, we
use the rbtree-implementation of TTM.

We could easily update drm_mm to use an rbtree instead of a linked list
for it's object list and thus drop the rbtree from the vma-manager.
However, this would slow down drm_mm object allocation for all other
use-cases (rbtree insertion) and add another 4-8 bytes to each mm node.
Hence, use the separate tree but allow for later migration.

This is a rewrite of the 2012-proposal by David Airlie <airlied@linux.ie>

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/Makefile          |   2 +-
 drivers/gpu/drm/drm_vma_manager.c | 224 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_vma_manager.h     | 107 ++++++++++++++++++
 3 files changed, 332 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/drm_vma_manager.c
 create mode 100644 include/drm/drm_vma_manager.h

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c9f2439..f8851cc 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -12,7 +12,7 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
 		drm_crtc.o drm_modes.o drm_edid.o \
 		drm_info.o drm_debugfs.o drm_encoder_slave.o \
-		drm_trace_points.o drm_global.o drm_prime.o
+		drm_trace_points.o drm_global.o drm_prime.o drm_vma_manager.o
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
new file mode 100644
index 0000000..c28639f
--- /dev/null
+++ b/drivers/gpu/drm/drm_vma_manager.c
@@ -0,0 +1,224 @@
+/*
+ * Copyright (c) 2012 David Airlie <airlied@linux.ie>
+ * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_mm.h>
+#include <drm/drm_vma_manager.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/rbtree.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+/** @file drm_vma_manager.c
+ *
+ * The vma-manager is responsible to map arbitrary driver-dependent memory
+ * regions into the linear user address-space. It provides offsets to the
+ * caller which can then be used on the address_space of the drm-device. It
+ * takes care to not overlap regions, size them appropriately and to not
+ * confuse mm-core by inconsistent fake vm_pgoff fields.
+ *
+ * We use drm_mm as backend to manage object allocations. But it is highly
+ * optimized for alloc/free calls, not lookups. Hence, we use an rb-tree to
+ * speed up offset lookups.
+ */
+
+/** drm_vma_offset_manager_init()
+ *
+ * Initialize a new offset-manager. The offset and area size available for the
+ * manager are given as @page_offset and @size. Both are interpreted as
+ * page-numbers, not bytes.
+ *
+ * Adding/removing nodes from the manager is locked internally and protected
+ * against concurrent access. However, node allocation and destruction is left
+ * for the caller. While calling into the vma-manager, a given node must
+ * always be guaranteed to be referenced.
+ */
+void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
+				 unsigned long page_offset, unsigned long size)
+{
+	rwlock_init(&mgr->vm_lock);
+	mgr->vm_addr_space_rb = RB_ROOT;
+	drm_mm_init(&mgr->vm_addr_space_mm, page_offset, size);
+}
+EXPORT_SYMBOL(drm_vma_offset_manager_init);
+
+/** drm_vma_offset_manager_destroy()
+ *
+ * Destroy an object manager which was previously created via
+ * drm_vma_offset_manager_init(). The caller must remove all allocated nodes
+ * before destroying the manager. Otherwise, drm_mm will refuse to free the
+ * requested resources.
+ *
+ * The manager must not be accessed after this function is called.
+ */
+void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
+{
+	BUG_ON(!drm_mm_clean(&mgr->vm_addr_space_mm));
+
+	/* take the lock to protect against buggy drivers */
+	write_lock(&mgr->vm_lock);
+	drm_mm_takedown(&mgr->vm_addr_space_mm);
+	write_unlock(&mgr->vm_lock);
+}
+EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
+
+/** drm_vma_offset_lookup()
+ *
+ * Find a node given a start address and object size. This returns the _best_
+ * match for the given node. That is, @start may point somewhere into a valid
+ * region and the given node will be returned, as long as the node spans the
+ * whole requested area (given the size in number of pages as @pages).
+ *
+ * Returns NULL if no suitable node can be found. Otherwise, the best match
+ * is returned. It's the caller's responsibility to make sure the node doesn't
+ * get destroyed before the caller can access it.
+ */
+struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
+						  unsigned long start,
+						  unsigned long pages)
+{
+	struct drm_vma_offset_node *node, *best;
+	struct rb_node *iter;
+	unsigned long offset;
+
+	read_lock(&mgr->vm_lock);
+
+	iter = mgr->vm_addr_space_rb.rb_node;
+	best = NULL;
+
+	while (likely(iter)) {
+		node = rb_entry(iter, struct drm_vma_offset_node, vm_rb);
+		offset = node->vm_node.start;
+		if (start >= offset) {
+			iter = iter->rb_right;
+			best = node;
+			if (start == offset)
+				break;
+		} else {
+			iter = iter->rb_left;
+		}
+	}
+
+	/* verify that the node spans the requested area */
+	if (likely(best)) {
+		offset = best->vm_node.start + best->vm_pages;
+		if (offset > start + pages)
+			best = NULL;
+	}
+
+	read_unlock(&mgr->vm_lock);
+
+	return best;
+}
+EXPORT_SYMBOL(drm_vma_offset_lookup);
+
+/* internal helper to link @node into the rb-tree */
+static void _drm_vma_offset_add_rb(struct drm_vma_offset_manager *mgr,
+				   struct drm_vma_offset_node *node)
+{
+	struct rb_node **iter = &mgr->vm_addr_space_rb.rb_node;
+	struct rb_node *parent = NULL;
+	struct drm_vma_offset_node *iter_node;
+
+	while (likely(*iter)) {
+		parent = *iter;
+		iter_node = rb_entry(*iter, struct drm_vma_offset_node, vm_rb);
+
+		if (node->vm_node.start < iter_node->vm_node.start)
+			iter = &(*iter)->rb_left;
+		else if (node->vm_node.start > iter_node->vm_node.start)
+			iter = &(*iter)->rb_right;
+		else
+			BUG();
+	}
+
+	rb_link_node(&node->vm_rb, parent, iter);
+	rb_insert_color(&node->vm_rb, &mgr->vm_addr_space_rb);
+}
+
+/** drm_vma_offset_add()
+ *
+ * Add a node to the offset-manager. If the node was already added, this does
+ * nothing and return 0. @pages is the size of the object given in number of
+ * pages.
+ * After this call succeeds, you can access the offset of the node until it
+ * is removed again.
+ *
+ * If this call fails, it is safe to retry the operation or call
+ * drm_vma_offset_remove(), anyway. However, no cleanup is required in that
+ * case.
+ */
+int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
+		       struct drm_vma_offset_node *node, unsigned long pages)
+{
+	int ret;
+
+	write_lock(&mgr->vm_lock);
+
+	if (unlikely(drm_mm_node_linked(&node->vm_node))) {
+		ret = 0;
+		goto out_unlock;
+	}
+
+	ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm,
+					 &node->vm_node, pages, 0, 0);
+	if (unlikely(ret))
+		goto out_reset;
+
+	node->vm_pages = pages;
+	_drm_vma_offset_add_rb(mgr, node);
+	goto out_unlock;
+
+out_reset:
+	/* we don't know what happened to @node->vm_node, so clear it */
+	drm_vma_node_reset(node);
+out_unlock:
+	write_unlock(&mgr->vm_lock);
+	return ret;
+}
+EXPORT_SYMBOL(drm_vma_offset_add);
+
+/** drm_vma_offset_remove()
+ *
+ * Remove a node from the offset manager. If the node wasn't added before, this
+ * does nothing. After this call returns, the offset of the node must be not
+ * accessed, anymore.
+ * It is safe to add the node via drm_vma_offset_add() again.
+ */
+void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
+			   struct drm_vma_offset_node *node)
+{
+	if (unlikely(!drm_mm_node_linked(&node->vm_node)))
+		return;
+
+	write_lock(&mgr->vm_lock);
+
+	rb_erase(&node->vm_rb, &mgr->vm_addr_space_rb);
+	drm_mm_remove_node(&node->vm_node);
+	drm_vma_node_reset(node);
+
+	write_unlock(&mgr->vm_lock);
+}
+EXPORT_SYMBOL(drm_vma_offset_remove);
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
new file mode 100644
index 0000000..8a1975c
--- /dev/null
+++ b/include/drm/drm_vma_manager.h
@@ -0,0 +1,107 @@
+#ifndef __DRM_VMA_MANAGER_H__
+#define __DRM_VMA_MANAGER_H__
+
+/*
+ * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_mm.h>
+#include <linux/module.h>
+#include <linux/rbtree.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+struct drm_vma_offset_node {
+	struct drm_mm_node vm_node;
+	struct rb_node vm_rb;
+	unsigned long vm_pages;
+};
+
+struct drm_vma_offset_manager {
+	rwlock_t vm_lock;
+	struct rb_root vm_addr_space_rb;
+	struct drm_mm vm_addr_space_mm;
+};
+
+void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
+				 unsigned long page_offset, unsigned long size);
+void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
+
+struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
+						  unsigned long start,
+						  unsigned long pages);
+int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
+		       struct drm_vma_offset_node *node, unsigned long pages);
+void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
+			   struct drm_vma_offset_node *node);
+
+/** drm_vma_offset_exact_lookup()
+ *
+ * Same as drm_vma_offset_lookup() but does not allow any offset but only
+ * returns the exact object with the given start address.
+ */
+static inline struct drm_vma_offset_node *
+drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
+			    unsigned long start)
+{
+	return drm_vma_offset_lookup(mgr, start, 1);
+}
+
+/** drm_vma_node_reset()
+ *
+ * Reset a node to its initial state.
+ */
+static inline void drm_vma_node_reset(struct drm_vma_offset_node *node)
+{
+	memset(&node->vm_node, 0, sizeof(node->vm_node));
+}
+
+/** drm_vma_node_start()
+ *
+ * Return the start address of the given node as pfn.
+ */
+static inline unsigned long drm_vma_node_start(struct drm_vma_offset_node *node)
+{
+	return node->vm_node.start;
+}
+
+/** drm_vma_node_has_offset()
+ *
+ * Return true iff the node was previously allocated an offset and added to
+ * an vma offset manager.
+ */
+static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node)
+{
+	return drm_mm_node_linked(&node->vm_node);
+}
+
+/** drm_vma_node_offset_addr()
+ *
+ * Same as drm_vma_node_start() but returns the address as a valid userspace
+ * address (instead of a pfn).
+ */
+static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
+{
+	return ((__u64)node->vm_node.start) << PAGE_SHIFT;
+}
+
+#endif /* __DRM_VMA_MANAGER_H__ */
-- 
1.8.3.2

[PATCH 4/6] drm: gem: convert to new unified vma manager

[David Herrmann]

Use the new vma manager instead of the old hashtable. Also convert all
drivers to use the new convenience helpers. This drops all the
(map_list.hash.key << PAGE_SHIFT) non-sense.

Locking and access-management is exactly the same as before with an
additional lock inside of the vma-manager, which strictly wouldn't be
needed for gem.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/drm_gem.c                  | 89 +++++-------------------------
 drivers/gpu/drm/drm_gem_cma_helper.c       |  9 +--
 drivers/gpu/drm/exynos/exynos_drm_gem.c    |  7 ++-
 drivers/gpu/drm/gma500/gem.c               |  8 +--
 drivers/gpu/drm/i915/i915_gem.c            |  9 +--
 drivers/gpu/drm/omapdrm/omap_gem.c         | 11 ++--
 drivers/gpu/drm/omapdrm/omap_gem_helpers.c | 49 +---------------
 drivers/gpu/drm/udl/udl_gem.c              |  6 +-
 include/drm/drmP.h                         |  7 +--
 include/drm/drm_vma_manager.h              |  1 -
 include/uapi/drm/drm.h                     |  2 +-
 11 files changed, 47 insertions(+), 151 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 88f0322..e1d0f67 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -37,6 +37,7 @@
 #include <linux/shmem_fs.h>
 #include <linux/dma-buf.h>
 #include <drm/drmP.h>
+#include <drm/drm_vma_manager.h>
 
 /** @file drm_gem.c
  *
@@ -102,14 +103,9 @@ drm_gem_init(struct drm_device *dev)
 	}
 
 	dev->mm_private = mm;
-
-	if (drm_ht_create(&mm->offset_hash, 12)) {
-		kfree(mm);
-		return -ENOMEM;
-	}
-
-	drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
-		    DRM_FILE_PAGE_OFFSET_SIZE);
+	drm_vma_offset_manager_init(&mm->vma_manager,
+				    DRM_FILE_PAGE_OFFSET_START,
+				    DRM_FILE_PAGE_OFFSET_SIZE);
 
 	return 0;
 }
@@ -119,8 +115,7 @@ drm_gem_destroy(struct drm_device *dev)
 {
 	struct drm_gem_mm *mm = dev->mm_private;
 
-	drm_mm_takedown(&mm->offset_manager);
-	drm_ht_remove(&mm->offset_hash);
+	drm_vma_offset_manager_destroy(&mm->vma_manager);
 	kfree(mm);
 	dev->mm_private = NULL;
 }
@@ -306,12 +301,8 @@ drm_gem_free_mmap_offset(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
 	struct drm_gem_mm *mm = dev->mm_private;
-	struct drm_map_list *list = &obj->map_list;
 
-	drm_ht_remove_item(&mm->offset_hash, &list->hash);
-	drm_mm_put_block(list->file_offset_node);
-	kfree(list->map);
-	list->map = NULL;
+	drm_vma_offset_remove(&mm->vma_manager, &obj->vma_node);
 }
 EXPORT_SYMBOL(drm_gem_free_mmap_offset);
 
@@ -331,54 +322,9 @@ drm_gem_create_mmap_offset(struct drm_gem_object *obj)
 {
 	struct drm_device *dev = obj->dev;
 	struct drm_gem_mm *mm = dev->mm_private;
-	struct drm_map_list *list;
-	struct drm_local_map *map;
-	int ret;
-
-	/* Set the object up for mmap'ing */
-	list = &obj->map_list;
-	list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL);
-	if (!list->map)
-		return -ENOMEM;
-
-	map = list->map;
-	map->type = _DRM_GEM;
-	map->size = obj->size;
-	map->handle = obj;
-
-	/* Get a DRM GEM mmap offset allocated... */
-	list->file_offset_node = drm_mm_search_free(&mm->offset_manager,
-			obj->size / PAGE_SIZE, 0, false);
-
-	if (!list->file_offset_node) {
-		DRM_ERROR("failed to allocate offset for bo %d\n", obj->name);
-		ret = -ENOSPC;
-		goto out_free_list;
-	}
 
-	list->file_offset_node = drm_mm_get_block(list->file_offset_node,
-			obj->size / PAGE_SIZE, 0);
-	if (!list->file_offset_node) {
-		ret = -ENOMEM;
-		goto out_free_list;
-	}
-
-	list->hash.key = list->file_offset_node->start;
-	ret = drm_ht_insert_item(&mm->offset_hash, &list->hash);
-	if (ret) {
-		DRM_ERROR("failed to add to map hash\n");
-		goto out_free_mm;
-	}
-
-	return 0;
-
-out_free_mm:
-	drm_mm_put_block(list->file_offset_node);
-out_free_list:
-	kfree(list->map);
-	list->map = NULL;
-
-	return ret;
+	return drm_vma_offset_add(&mm->vma_manager, &obj->vma_node,
+				  obj->size / PAGE_SIZE);
 }
 EXPORT_SYMBOL(drm_gem_create_mmap_offset);
 
@@ -660,9 +606,8 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	struct drm_file *priv = filp->private_data;
 	struct drm_device *dev = priv->minor->dev;
 	struct drm_gem_mm *mm = dev->mm_private;
-	struct drm_local_map *map = NULL;
 	struct drm_gem_object *obj;
-	struct drm_hash_item *hash;
+	struct drm_vma_offset_node *node;
 	int ret = 0;
 
 	if (drm_device_is_unplugged(dev))
@@ -670,25 +615,19 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	mutex_lock(&dev->struct_mutex);
 
-	if (drm_ht_find_item(&mm->offset_hash, vma->vm_pgoff, &hash)) {
+	node = drm_vma_offset_exact_lookup(&mm->vma_manager, vma->vm_pgoff);
+	if (!node) {
 		mutex_unlock(&dev->struct_mutex);
 		return drm_mmap(filp, vma);
 	}
-
-	map = drm_hash_entry(hash, struct drm_map_list, hash)->map;
-	if (!map ||
-	    ((map->flags & _DRM_RESTRICTED) && !capable(CAP_SYS_ADMIN))) {
-		ret =  -EPERM;
-		goto out_unlock;
-	}
+	obj = container_of(node, struct drm_gem_object, vma_node);
 
 	/* Check for valid size. */
-	if (map->size < vma->vm_end - vma->vm_start) {
+	if (node->vm_pages < vma_pages(vma)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
 
-	obj = map->handle;
 	if (!obj->dev->driver->gem_vm_ops) {
 		ret = -EINVAL;
 		goto out_unlock;
@@ -696,7 +635,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;
 	vma->vm_ops = obj->dev->driver->gem_vm_ops;
-	vma->vm_private_data = map->handle;
+	vma->vm_private_data = (void*)obj;
 	vma->vm_page_prot =  pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
 
 	/* Take a ref for this mapping of the object, so that the fault
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 0a7e011..905f4ec 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -26,10 +26,11 @@
 #include <drm/drmP.h>
 #include <drm/drm.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_vma_manager.h>
 
 static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj)
 {
-	return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT;
+	return (unsigned int)drm_vma_node_offset_addr(&obj->vma_node);
 }
 
 static void drm_gem_cma_buf_destroy(struct drm_device *drm,
@@ -140,7 +141,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 {
 	struct drm_gem_cma_object *cma_obj;
 
-	if (gem_obj->map_list.map)
+	if (drm_vma_node_has_offset(&gem_obj->vma_node))
 		drm_gem_free_mmap_offset(gem_obj);
 
 	drm_gem_object_release(gem_obj);
@@ -259,8 +260,8 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->map_list.map)
-		off = (uint64_t)obj->map_list.hash.key;
+	if (drm_vma_node_has_offset(&obj->vma_node))
+		off = drm_vma_node_start(&obj->vma_node);
 
 	seq_printf(m, "%2d (%2d) %08llx %08Zx %p %d",
 			obj->name, obj->refcount.refcount.counter,
diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index cf4543f..59488ea 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -10,6 +10,7 @@
  */
 
 #include <drm/drmP.h>
+#include <drm/drm_vma_manager.h>
 
 #include <linux/shmem_fs.h>
 #include <drm/exynos_drm.h>
@@ -154,7 +155,7 @@ out:
 	exynos_drm_fini_buf(obj->dev, buf);
 	exynos_gem_obj->buffer = NULL;
 
-	if (obj->map_list.map)
+	if (drm_vma_node_has_offset(&obj->vma_node))
 		drm_gem_free_mmap_offset(obj);
 
 	/* release file pointer to gem object. */
@@ -721,13 +722,13 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
 		goto unlock;
 	}
 
-	if (!obj->map_list.map) {
+	if (!drm_vma_node_has_offset(&obj->vma_node)) {
 		ret = drm_gem_create_mmap_offset(obj);
 		if (ret)
 			goto out;
 	}
 
-	*offset = (u64)obj->map_list.hash.key << PAGE_SHIFT;
+	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
 
 out:
diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index eefd6cc..9fd0c9f 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -26,6 +26,7 @@
 #include <drm/drmP.h>
 #include <drm/drm.h>
 #include <drm/gma_drm.h>
+#include <drm/drm_vma_manager.h>
 #include "psb_drv.h"
 
 int psb_gem_init_object(struct drm_gem_object *obj)
@@ -38,7 +39,7 @@ void psb_gem_free_object(struct drm_gem_object *obj)
 	struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
 
 	/* Remove the list map if one is present */
-	if (obj->map_list.map)
+	if (drm_vma_node_has_offset(&obj->vma_node))
 		drm_gem_free_mmap_offset(obj);
 	drm_gem_object_release(obj);
 
@@ -81,13 +82,12 @@ int psb_gem_dumb_map_gtt(struct drm_file *file, struct drm_device *dev,
 	/* What validation is needed here ? */
 
 	/* Make it mmapable */
-	if (!obj->map_list.map) {
+	if (!drm_vma_node_has_offset(&obj->vma_node)) {
 		ret = drm_gem_create_mmap_offset(obj);
 		if (ret)
 			goto out;
 	}
-	/* GEM should really work out the hash offsets for us */
-	*offset = (u64)obj->map_list.hash.key << PAGE_SHIFT;
+	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 out:
 	drm_gem_object_unreference(obj);
 unlock:
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9e35daf..1617166 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -26,6 +26,7 @@
  */
 
 #include <drm/drmP.h>
+#include <drm/drm_vma_manager.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "i915_trace.h"
@@ -1428,7 +1429,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 
 	if (obj->base.dev->dev_mapping)
 		unmap_mapping_range(obj->base.dev->dev_mapping,
-				    (loff_t)obj->base.map_list.hash.key<<PAGE_SHIFT,
+				    (loff_t)drm_vma_node_offset_addr(&obj->base.vma_node),
 				    obj->base.size, 1);
 
 	obj->fault_mappable = false;
@@ -1486,7 +1487,7 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	int ret;
 
-	if (obj->base.map_list.map)
+	if (drm_vma_node_has_offset(&obj->base.vma_node))
 		return 0;
 
 	dev_priv->mm.shrinker_no_lock_stealing = true;
@@ -1517,7 +1518,7 @@ out:
 
 static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 {
-	if (!obj->base.map_list.map)
+	if (!drm_vma_node_has_offset(&obj->base.vma_node))
 		return;
 
 	drm_gem_free_mmap_offset(&obj->base);
@@ -1558,7 +1559,7 @@ i915_gem_mmap_gtt(struct drm_file *file,
 	if (ret)
 		goto out;
 
-	*offset = (u64)obj->base.map_list.hash.key << PAGE_SHIFT;
+	*offset = drm_vma_node_offset_addr(&obj->base.vma_node);
 
 out:
 	drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index ebbdf41..0bcfdb9 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -20,6 +20,7 @@
 
 #include <linux/spinlock.h>
 #include <linux/shmem_fs.h>
+#include <drm/drm_vma_manager.h>
 
 #include "omap_drv.h"
 #include "omap_dmm_tiler.h"
@@ -311,7 +312,7 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (!obj->map_list.map) {
+	if (!drm_vma_node_has_offset(&obj->vma_node)) {
 		/* Make it mmapable */
 		size_t size = omap_gem_mmap_size(obj);
 		int ret = _drm_gem_create_mmap_offset_size(obj, size);
@@ -322,7 +323,7 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
 		}
 	}
 
-	return (uint64_t)obj->map_list.hash.key << PAGE_SHIFT;
+	return drm_vma_node_offset_addr(&obj->vma_node);
 }
 
 uint64_t omap_gem_mmap_offset(struct drm_gem_object *obj)
@@ -1001,8 +1002,8 @@ void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m)
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
-	if (obj->map_list.map)
-		off = (uint64_t)obj->map_list.hash.key;
+	if (drm_vma_node_has_offset(&obj->vma_node))
+		off = drm_vma_node_start(&obj->vma_node);
 
 	seq_printf(m, "%08x: %2d (%2d) %08llx %08Zx (%2d) %p %4d",
 			omap_obj->flags, obj->name, obj->refcount.refcount.counter,
@@ -1309,7 +1310,7 @@ void omap_gem_free_object(struct drm_gem_object *obj)
 
 	list_del(&omap_obj->mm_list);
 
-	if (obj->map_list.map)
+	if (drm_vma_node_has_offset(&obj->vma_node))
 		drm_gem_free_mmap_offset(obj);
 
 	/* this means the object is still pinned.. which really should
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_helpers.c b/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
index f9eb679..dbb1575 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_helpers.c
@@ -118,52 +118,7 @@ _drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size)
 {
 	struct drm_device *dev = obj->dev;
 	struct drm_gem_mm *mm = dev->mm_private;
-	struct drm_map_list *list;
-	struct drm_local_map *map;
-	int ret = 0;
-
-	/* Set the object up for mmap'ing */
-	list = &obj->map_list;
-	list->map = kzalloc(sizeof(struct drm_map_list), GFP_KERNEL);
-	if (!list->map)
-		return -ENOMEM;
-
-	map = list->map;
-	map->type = _DRM_GEM;
-	map->size = size;
-	map->handle = obj;
-
-	/* Get a DRM GEM mmap offset allocated... */
-	list->file_offset_node = drm_mm_search_free(&mm->offset_manager,
-			size / PAGE_SIZE, 0, 0);
-
-	if (!list->file_offset_node) {
-		DRM_ERROR("failed to allocate offset for bo %d\n", obj->name);
-		ret = -ENOSPC;
-		goto out_free_list;
-	}
-
-	list->file_offset_node = drm_mm_get_block(list->file_offset_node,
-			size / PAGE_SIZE, 0);
-	if (!list->file_offset_node) {
-		ret = -ENOMEM;
-		goto out_free_list;
-	}
-
-	list->hash.key = list->file_offset_node->start;
-	ret = drm_ht_insert_item(&mm->offset_hash, &list->hash);
-	if (ret) {
-		DRM_ERROR("failed to add to map hash\n");
-		goto out_free_mm;
-	}
-
-	return 0;
-
-out_free_mm:
-	drm_mm_put_block(list->file_offset_node);
-out_free_list:
-	kfree(list->map);
-	list->map = NULL;
 
-	return ret;
+	return drm_vma_offset_add(&mm->vma_manager, &obj->vma_node,
+				  size / PAGE_SIZE);
 }
diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index ef034fa..59aee8a 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -223,7 +223,7 @@ void udl_gem_free_object(struct drm_gem_object *gem_obj)
 	if (obj->pages)
 		udl_gem_put_pages(obj);
 
-	if (gem_obj->map_list.map)
+	if (drm_vma_node_has_offset(&gem_obj->vma_node))
 		drm_gem_free_mmap_offset(gem_obj);
 }
 
@@ -247,13 +247,13 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
 	ret = udl_gem_get_pages(gobj, GFP_KERNEL);
 	if (ret)
 		goto out;
-	if (!gobj->base.map_list.map) {
+	if (!drm_vma_node_has_offset(&gobj->base.vma_node)) {
 		ret = drm_gem_create_mmap_offset(obj);
 		if (ret)
 			goto out;
 	}
 
-	*offset = (u64)gobj->base.map_list.hash.key << PAGE_SHIFT;
+	*offset = drm_vma_node_offset_addr(&gobj->base.vma_node);
 
 out:
 	drm_gem_object_unreference(&gobj->base);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 63d17ee..79ea723 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -74,6 +74,7 @@
 #include <asm/pgalloc.h>
 #include <drm/drm.h>
 #include <drm/drm_sarea.h>
+#include <drm/drm_vma_manager.h>
 
 #include <linux/idr.h>
 
@@ -590,7 +591,6 @@ struct drm_map_list {
 	struct drm_local_map *map;	/**< mapping */
 	uint64_t user_token;
 	struct drm_master *master;
-	struct drm_mm_node *file_offset_node;	/**< fake offset */
 };
 
 /**
@@ -625,8 +625,7 @@ struct drm_ati_pcigart_info {
  * GEM specific mm private for tracking GEM objects
  */
 struct drm_gem_mm {
-	struct drm_mm offset_manager;	/**< Offset mgmt for buffer objects */
-	struct drm_open_hash offset_hash; /**< User token hash table for maps */
+	struct drm_vma_offset_manager vma_manager;
 };
 
 /**
@@ -647,7 +646,7 @@ struct drm_gem_object {
 	struct file *filp;
 
 	/* Mapping info for this object */
-	struct drm_map_list map_list;
+	struct drm_vma_offset_node vma_node;
 
 	/**
 	 * Size of the object, in bytes.  Immutable over the object's
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 8a1975c..4d6a734 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -23,7 +23,6 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
-#include <drm/drmP.h>
 #include <drm/drm_mm.h>
 #include <linux/module.h>
 #include <linux/rbtree.h>
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 5a57be6..1d1c6f0 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -181,7 +181,7 @@ enum drm_map_type {
 	_DRM_AGP = 3,		  /**< AGP/GART */
 	_DRM_SCATTER_GATHER = 4,  /**< Scatter/gather memory for PCI DMA */
 	_DRM_CONSISTENT = 5,	  /**< Consistent memory for PCI DMA */
-	_DRM_GEM = 6,		  /**< GEM object */
+	_DRM_GEM = 6,		  /**< GEM object (obsolete) */
 };
 
 /**
-- 
1.8.3.2

[PATCH 5/6] drm: ttm: convert to unified vma offset manager

[David Herrmann]

Use the new vma-manager infrastructure. This doesn't change any
implementation details as the vma-offset-manager is nearly copied 1-to-1
from TTM.

Even though the vma-manager uses its own locks, we still need bo->vm_lock
to prevent bos from being destroyed before we can get a reference during
lookup. However, this lock is not needed during vm-setup as we still
hold a reference there.

This also drops the addr_space_offset member as it is a copy of vm_start
in vma_node objects. Use the accessor functions instead.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/ast/ast_main.c            |  2 +-
 drivers/gpu/drm/cirrus/cirrus_main.c      |  2 +-
 drivers/gpu/drm/mgag200/mgag200_main.c    |  2 +-
 drivers/gpu/drm/nouveau/nouveau_display.c |  2 +-
 drivers/gpu/drm/nouveau/nouveau_gem.c     |  2 +-
 drivers/gpu/drm/qxl/qxl_object.h          |  2 +-
 drivers/gpu/drm/qxl/qxl_release.c         |  2 +-
 drivers/gpu/drm/radeon/radeon_object.h    |  5 +-
 drivers/gpu/drm/ttm/ttm_bo.c              | 84 ++++++-------------------------
 drivers/gpu/drm/ttm/ttm_bo_util.c         |  3 +-
 drivers/gpu/drm/ttm/ttm_bo_vm.c           | 81 ++++++++++++-----------------
 drivers/gpu/drm/vmwgfx/vmwgfx_resource.c  |  4 +-
 include/drm/ttm/ttm_bo_api.h              | 15 ++----
 include/drm/ttm/ttm_bo_driver.h           |  7 +--
 14 files changed, 65 insertions(+), 148 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index f60fd7b..c195dc2 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -487,7 +487,7 @@ void ast_gem_free_object(struct drm_gem_object *obj)
 
 static inline u64 ast_bo_mmap_offset(struct ast_bo *bo)
 {
-	return bo->bo.addr_space_offset;
+	return drm_vma_node_offset_addr(&bo->bo.vma_node);
 }
 int
 ast_dumb_mmap_offset(struct drm_file *file,
diff --git a/drivers/gpu/drm/cirrus/cirrus_main.c b/drivers/gpu/drm/cirrus/cirrus_main.c
index 35cbae8..3a7a0ef 100644
--- a/drivers/gpu/drm/cirrus/cirrus_main.c
+++ b/drivers/gpu/drm/cirrus/cirrus_main.c
@@ -294,7 +294,7 @@ void cirrus_gem_free_object(struct drm_gem_object *obj)
 
 static inline u64 cirrus_bo_mmap_offset(struct cirrus_bo *bo)
 {
-	return bo->bo.addr_space_offset;
+	return drm_vma_node_offset_addr(&bo->bo.vma_node);
 }
 
 int
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 9905923..1b560a1 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -330,7 +330,7 @@ void mgag200_gem_free_object(struct drm_gem_object *obj)
 
 static inline u64 mgag200_bo_mmap_offset(struct mgag200_bo *bo)
 {
-	return bo->bo.addr_space_offset;
+	return drm_vma_node_offset_addr(&bo->bo.vma_node);
 }
 
 int
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index f17dc2a..52498de 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -705,7 +705,7 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv,
 	gem = drm_gem_object_lookup(dev, file_priv, handle);
 	if (gem) {
 		struct nouveau_bo *bo = gem->driver_private;
-		*poffset = bo->bo.addr_space_offset;
+		*poffset = drm_vma_node_offset_addr(&bo->bo.vma_node);
 		drm_gem_object_unreference_unlocked(gem);
 		return 0;
 	}
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index b4b4d0c..357dace 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -192,7 +192,7 @@ nouveau_gem_info(struct drm_file *file_priv, struct drm_gem_object *gem,
 	}
 
 	rep->size = nvbo->bo.mem.num_pages << PAGE_SHIFT;
-	rep->map_handle = nvbo->bo.addr_space_offset;
+	rep->map_handle = drm_vma_node_offset_addr(&nvbo->bo.vma_node);
 	rep->tile_mode = nvbo->tile_mode;
 	rep->tile_flags = nvbo->tile_flags;
 	return 0;
diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h
index b4fd89f..1fc4e4b 100644
--- a/drivers/gpu/drm/qxl/qxl_object.h
+++ b/drivers/gpu/drm/qxl/qxl_object.h
@@ -64,7 +64,7 @@ static inline bool qxl_bo_is_reserved(struct qxl_bo *bo)
 
 static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo)
 {
-	return bo->tbo.addr_space_offset;
+	return drm_vma_node_offset_addr(&bo->tbo.vma_node);
 }
 
 static inline int qxl_bo_wait(struct qxl_bo *bo, u32 *mem_type,
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index b443d67..1a648e1 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -87,7 +87,7 @@ qxl_release_free(struct qxl_device *qdev,
 
 	for (i = 0 ; i < release->bo_count; ++i) {
 		QXL_INFO(qdev, "release %llx\n",
-			release->bos[i]->tbo.addr_space_offset
+			drm_vma_node_offset_addr(&release->bos[i]->tbo.vma_node)
 						- DRM_FILE_OFFSET);
 		qxl_fence_remove_release(&release->bos[i]->fence, release->id);
 		qxl_bo_unref(&release->bos[i]);
diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h
index e2cb80a..ffa4e82 100644
--- a/drivers/gpu/drm/radeon/radeon_object.h
+++ b/drivers/gpu/drm/radeon/radeon_object.h
@@ -98,13 +98,10 @@ static inline unsigned radeon_bo_gpu_page_alignment(struct radeon_bo *bo)
  * @bo:	radeon object for which we query the offset
  *
  * Returns mmap offset of the object.
- *
- * Note: addr_space_offset is constant after ttm bo init thus isn't protected
- * by any lock.
  */
 static inline u64 radeon_bo_mmap_offset(struct radeon_bo *bo)
 {
-	return bo->tbo.addr_space_offset;
+	return drm_vma_node_offset_addr(&bo->tbo.vma_node);
 }
 
 extern int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type,
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index e97c5a0..2b96a75 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -776,11 +776,7 @@ static void ttm_bo_release(struct kref *kref)
 	struct ttm_mem_type_manager *man = &bdev->man[bo->mem.mem_type];
 
 	write_lock(&bdev->vm_lock);
-	if (likely(bo->vm_node != NULL)) {
-		rb_erase(&bo->vm_rb, &bdev->addr_space_rb);
-		drm_mm_put_block(bo->vm_node);
-		bo->vm_node = NULL;
-	}
+	drm_vma_offset_remove(&bdev->vma_manager, &bo->vma_node);
 	write_unlock(&bdev->vm_lock);
 	ttm_mem_io_lock(man, false);
 	ttm_mem_io_free_vm(bo);
@@ -1289,6 +1285,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev,
 	bo->acc_size = acc_size;
 	bo->sg = sg;
 	atomic_inc(&bo->glob->bo_count);
+	drm_vma_node_reset(&bo->vma_node);
 
 	ret = ttm_bo_check_placement(bo, placement);
 	if (unlikely(ret != 0))
@@ -1588,9 +1585,8 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev)
 		TTM_DEBUG("Swap list was clean\n");
 	spin_unlock(&glob->lru_lock);
 
-	BUG_ON(!drm_mm_clean(&bdev->addr_space_mm));
 	write_lock(&bdev->vm_lock);
-	drm_mm_takedown(&bdev->addr_space_mm);
+	drm_vma_offset_manager_destroy(&bdev->vma_manager);
 	write_unlock(&bdev->vm_lock);
 
 	return ret;
@@ -1618,9 +1614,8 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
 	if (unlikely(ret != 0))
 		goto out_no_sys;
 
-	bdev->addr_space_rb = RB_ROOT;
-	drm_mm_init(&bdev->addr_space_mm, file_page_offset, 0x10000000);
-
+	drm_vma_offset_manager_init(&bdev->vma_manager, file_page_offset,
+				    0x10000000);
 	INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
 	INIT_LIST_HEAD(&bdev->ddestroy);
 	bdev->dev_mapping = NULL;
@@ -1662,12 +1657,17 @@ bool ttm_mem_reg_is_pci(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
-	loff_t offset = (loff_t) bo->addr_space_offset;
-	loff_t holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
+	loff_t offset, holelen;
 
 	if (!bdev->dev_mapping)
 		return;
-	unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
+
+	if (drm_vma_node_has_offset(&bo->vma_node)) {
+		offset = (loff_t) drm_vma_node_offset_addr(&bo->vma_node);
+		holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
+
+		unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
+	}
 	ttm_mem_io_free_vm(bo);
 }
 
@@ -1684,31 +1684,6 @@ void ttm_bo_unmap_virtual(struct ttm_buffer_object *bo)
 
 EXPORT_SYMBOL(ttm_bo_unmap_virtual);
 
-static void ttm_bo_vm_insert_rb(struct ttm_buffer_object *bo)
-{
-	struct ttm_bo_device *bdev = bo->bdev;
-	struct rb_node **cur = &bdev->addr_space_rb.rb_node;
-	struct rb_node *parent = NULL;
-	struct ttm_buffer_object *cur_bo;
-	unsigned long offset = bo->vm_node->start;
-	unsigned long cur_offset;
-
-	while (*cur) {
-		parent = *cur;
-		cur_bo = rb_entry(parent, struct ttm_buffer_object, vm_rb);
-		cur_offset = cur_bo->vm_node->start;
-		if (offset < cur_offset)
-			cur = &parent->rb_left;
-		else if (offset > cur_offset)
-			cur = &parent->rb_right;
-		else
-			BUG();
-	}
-
-	rb_link_node(&bo->vm_rb, parent, cur);
-	rb_insert_color(&bo->vm_rb, &bdev->addr_space_rb);
-}
-
 /**
  * ttm_bo_setup_vm:
  *
@@ -1723,38 +1698,9 @@ static void ttm_bo_vm_insert_rb(struct ttm_buffer_object *bo)
 static int ttm_bo_setup_vm(struct ttm_buffer_object *bo)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
-	int ret;
-
-retry_pre_get:
-	ret = drm_mm_pre_get(&bdev->addr_space_mm);
-	if (unlikely(ret != 0))
-		return ret;
 
-	write_lock(&bdev->vm_lock);
-	bo->vm_node = drm_mm_search_free(&bdev->addr_space_mm,
-					 bo->mem.num_pages, 0, 0);
-
-	if (unlikely(bo->vm_node == NULL)) {
-		ret = -ENOMEM;
-		goto out_unlock;
-	}
-
-	bo->vm_node = drm_mm_get_block_atomic(bo->vm_node,
-					      bo->mem.num_pages, 0);
-
-	if (unlikely(bo->vm_node == NULL)) {
-		write_unlock(&bdev->vm_lock);
-		goto retry_pre_get;
-	}
-
-	ttm_bo_vm_insert_rb(bo);
-	write_unlock(&bdev->vm_lock);
-	bo->addr_space_offset = ((uint64_t) bo->vm_node->start) << PAGE_SHIFT;
-
-	return 0;
-out_unlock:
-	write_unlock(&bdev->vm_lock);
-	return ret;
+	return drm_vma_offset_add(&bdev->vma_manager, &bo->vma_node,
+				  bo->mem.num_pages);
 }
 
 int ttm_bo_wait(struct ttm_buffer_object *bo,
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
index af89458..d8b6e5f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -30,6 +30,7 @@
 
 #include <drm/ttm/ttm_bo_driver.h>
 #include <drm/ttm/ttm_placement.h>
+#include <drm/drm_vma_manager.h>
 #include <linux/io.h>
 #include <linux/highmem.h>
 #include <linux/wait.h>
@@ -450,7 +451,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
 	INIT_LIST_HEAD(&fbo->lru);
 	INIT_LIST_HEAD(&fbo->swap);
 	INIT_LIST_HEAD(&fbo->io_reserve_lru);
-	fbo->vm_node = NULL;
+	drm_vma_node_reset(&fbo->vma_node);
 	atomic_set(&fbo->cpu_writers, 0);
 
 	spin_lock(&bdev->fence_lock);
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 3df9f16..54a67f1 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -33,6 +33,7 @@
 #include <ttm/ttm_module.h>
 #include <ttm/ttm_bo_driver.h>
 #include <ttm/ttm_placement.h>
+#include <drm/drm_vma_manager.h>
 #include <linux/mm.h>
 #include <linux/rbtree.h>
 #include <linux/module.h>
@@ -40,37 +41,6 @@
 
 #define TTM_BO_VM_NUM_PREFAULT 16
 
-static struct ttm_buffer_object *ttm_bo_vm_lookup_rb(struct ttm_bo_device *bdev,
-						     unsigned long page_start,
-						     unsigned long num_pages)
-{
-	struct rb_node *cur = bdev->addr_space_rb.rb_node;
-	unsigned long cur_offset;
-	struct ttm_buffer_object *bo;
-	struct ttm_buffer_object *best_bo = NULL;
-
-	while (likely(cur != NULL)) {
-		bo = rb_entry(cur, struct ttm_buffer_object, vm_rb);
-		cur_offset = bo->vm_node->start;
-		if (page_start >= cur_offset) {
-			cur = cur->rb_right;
-			best_bo = bo;
-			if (page_start == cur_offset)
-				break;
-		} else
-			cur = cur->rb_left;
-	}
-
-	if (unlikely(best_bo == NULL))
-		return NULL;
-
-	if (unlikely((best_bo->vm_node->start + best_bo->num_pages) <
-		     (page_start + num_pages)))
-		return NULL;
-
-	return best_bo;
-}
-
 static int ttm_bo_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 {
 	struct ttm_buffer_object *bo = (struct ttm_buffer_object *)
@@ -146,9 +116,9 @@ static int ttm_bo_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 	}
 
 	page_offset = ((address - vma->vm_start) >> PAGE_SHIFT) +
-	    bo->vm_node->start - vma->vm_pgoff;
+	    drm_vma_node_start(&bo->vma_node) - vma->vm_pgoff;
 	page_last = vma_pages(vma) +
-	    bo->vm_node->start - vma->vm_pgoff;
+	    drm_vma_node_start(&bo->vma_node) - vma->vm_pgoff;
 
 	if (unlikely(page_offset >= bo->num_pages)) {
 		retval = VM_FAULT_SIGBUS;
@@ -249,6 +219,30 @@ static const struct vm_operations_struct ttm_bo_vm_ops = {
 	.close = ttm_bo_vm_close
 };
 
+static struct ttm_buffer_object *ttm_bo_vm_lookup(struct ttm_bo_device *bdev,
+						  unsigned long offset,
+						  unsigned long pages)
+{
+	struct drm_vma_offset_node *node;
+	struct ttm_buffer_object *bo = NULL;
+
+	read_lock(&bdev->vm_lock);
+
+	node = drm_vma_offset_lookup(&bdev->vma_manager, offset, pages);
+	if (likely(node)) {
+		bo = container_of(node, struct ttm_buffer_object, vma_node);
+		if (!kref_get_unless_zero(&bo->kref))
+			bo = NULL;
+	}
+
+	read_unlock(&bdev->vm_lock);
+
+	if (!bo)
+		pr_err("Could not find buffer object to map\n");
+
+	return bo;
+}
+
 int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 		struct ttm_bo_device *bdev)
 {
@@ -256,17 +250,9 @@ int ttm_bo_mmap(struct file *filp, struct vm_area_struct *vma,
 	struct ttm_buffer_object *bo;
 	int ret;
 
-	read_lock(&bdev->vm_lock);
-	bo = ttm_bo_vm_lookup_rb(bdev, vma->vm_pgoff,
-				 vma_pages(vma));
-	if (likely(bo != NULL) && !kref_get_unless_zero(&bo->kref))
-		bo = NULL;
-	read_unlock(&bdev->vm_lock);
-
-	if (unlikely(bo == NULL)) {
-		pr_err("Could not find buffer object to map\n");
+	bo = ttm_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma));
+	if (unlikely(!bo))
 		return -EINVAL;
-	}
 
 	driver = bo->bdev->driver;
 	if (unlikely(!driver->verify_access)) {
@@ -324,12 +310,7 @@ ssize_t ttm_bo_io(struct ttm_bo_device *bdev, struct file *filp,
 	bool no_wait = false;
 	bool dummy;
 
-	read_lock(&bdev->vm_lock);
-	bo = ttm_bo_vm_lookup_rb(bdev, dev_offset, 1);
-	if (likely(bo != NULL))
-		ttm_bo_reference(bo);
-	read_unlock(&bdev->vm_lock);
-
+	bo = ttm_bo_vm_lookup(bdev, dev_offset, 1);
 	if (unlikely(bo == NULL))
 		return -EFAULT;
 
@@ -343,7 +324,7 @@ ssize_t ttm_bo_io(struct ttm_bo_device *bdev, struct file *filp,
 	if (unlikely(ret != 0))
 		goto out_unref;
 
-	kmap_offset = dev_offset - bo->vm_node->start;
+	kmap_offset = dev_offset - drm_vma_node_start(&bo->vma_node);
 	if (unlikely(kmap_offset >= bo->num_pages)) {
 		ret = -EFBIG;
 		goto out_unref;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
index bc78425..b150079 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_resource.c
@@ -500,7 +500,7 @@ int vmw_dmabuf_alloc_ioctl(struct drm_device *dev, void *data,
 		goto out_no_dmabuf;
 
 	rep->handle = handle;
-	rep->map_handle = dma_buf->base.addr_space_offset;
+	rep->map_handle = drm_vma_node_offset_addr(&dma_buf->base.vma_node);
 	rep->cur_gmr_id = handle;
 	rep->cur_gmr_offset = 0;
 
@@ -834,7 +834,7 @@ int vmw_dumb_map_offset(struct drm_file *file_priv,
 	if (ret != 0)
 		return -EINVAL;
 
-	*offset = out_buf->base.addr_space_offset;
+	*offset = drm_vma_node_offset_addr(&out_buf->base.vma_node);
 	vmw_dmabuf_unreference(&out_buf);
 	return 0;
 }
diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
index 3cb5d84..fcbe198 100644
--- a/include/drm/ttm/ttm_bo_api.h
+++ b/include/drm/ttm/ttm_bo_api.h
@@ -32,12 +32,12 @@
 #define _TTM_BO_API_H_
 
 #include <drm/drm_hashtab.h>
+#include <drm/drm_vma_manager.h>
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/wait.h>
 #include <linux/mutex.h>
 #include <linux/mm.h>
-#include <linux/rbtree.h>
 #include <linux/bitmap.h>
 
 struct ttm_bo_device;
@@ -144,7 +144,6 @@ struct ttm_tt;
  * @type: The bo type.
  * @destroy: Destruction function. If NULL, kfree is used.
  * @num_pages: Actual number of pages.
- * @addr_space_offset: Address space offset.
  * @acc_size: Accounted size for this object.
  * @kref: Reference count of this buffer object. When this refcount reaches
  * zero, the object is put on the delayed delete list.
@@ -172,8 +171,7 @@ struct ttm_tt;
  * @reserved: Deadlock-free lock used for synchronization state transitions.
  * @sync_obj: Pointer to a synchronization object.
  * @priv_flags: Flags describing buffer object internal state.
- * @vm_rb: Rb node for the vm rb tree.
- * @vm_node: Address space manager node.
+ * @vma_node: Address space manager node.
  * @offset: The current GPU offset, which can have different meanings
  * depending on the memory type. For SYSTEM type memory, it should be 0.
  * @cur_placement: Hint of current placement.
@@ -200,7 +198,6 @@ struct ttm_buffer_object {
 	enum ttm_bo_type type;
 	void (*destroy) (struct ttm_buffer_object *);
 	unsigned long num_pages;
-	uint64_t addr_space_offset;
 	size_t acc_size;
 
 	/**
@@ -254,13 +251,7 @@ struct ttm_buffer_object {
 	void *sync_obj;
 	unsigned long priv_flags;
 
-	/**
-	 * Members protected by the bdev::vm_lock
-	 */
-
-	struct rb_node vm_rb;
-	struct drm_mm_node *vm_node;
-
+	struct drm_vma_offset_node vma_node;
 
 	/**
 	 * Special members that are protected by the reserve lock
diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
index 9c8dca7..af63da3 100644
--- a/include/drm/ttm/ttm_bo_driver.h
+++ b/include/drm/ttm/ttm_bo_driver.h
@@ -35,6 +35,7 @@
 #include <ttm/ttm_module.h>
 #include <drm/drm_mm.h>
 #include <drm/drm_global.h>
+#include <drm/drm_vma_manager.h>
 #include <linux/workqueue.h>
 #include <linux/fs.h>
 #include <linux/spinlock.h>
@@ -517,7 +518,7 @@ struct ttm_bo_global {
  * @man: An array of mem_type_managers.
  * @fence_lock: Protects the synchronizing members on *all* bos belonging
  * to this device.
- * @addr_space_mm: Range manager for the device address space.
+ * @vma_manager: Address space manager
  * lru_lock: Spinlock that protects the buffer+device lru lists and
  * ddestroy lists.
  * @val_seq: Current validation sequence.
@@ -538,11 +539,11 @@ struct ttm_bo_device {
 	rwlock_t vm_lock;
 	struct ttm_mem_type_manager man[TTM_NUM_MEM_TYPES];
 	spinlock_t fence_lock;
+
 	/*
 	 * Protected by the vm lock.
 	 */
-	struct rb_root addr_space_rb;
-	struct drm_mm addr_space_mm;
+	struct drm_vma_offset_manager vma_manager;
 
 	/*
 	 * Protected by the global:lru lock.
-- 
1.8.3.2

[PATCH 6/6] drm: provide generic drm_vma_node_unmap() helper

[David Herrmann]

Instead of unmapping the nodes in TTM and GEM users manually, we provide
a generic wrapper which does the correct thing for all vma-nodes.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem.c |  6 +-----
 drivers/gpu/drm/ttm/ttm_bo.c    |  8 +-------
 include/drm/drm_vma_manager.h   | 16 ++++++++++++++++
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1617166..43f9aaf 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1427,11 +1427,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
 	if (!obj->fault_mappable)
 		return;
 
-	if (obj->base.dev->dev_mapping)
-		unmap_mapping_range(obj->base.dev->dev_mapping,
-				    (loff_t)drm_vma_node_offset_addr(&obj->base.vma_node),
-				    obj->base.size, 1);
-
+	drm_vma_node_unmap(&obj->base.vma_node, obj->base.dev->dev_mapping);
 	obj->fault_mappable = false;
 }
 
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 2b96a75..2979070 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -1657,17 +1657,11 @@ bool ttm_mem_reg_is_pci(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
 void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
 {
 	struct ttm_bo_device *bdev = bo->bdev;
-	loff_t offset, holelen;
 
 	if (!bdev->dev_mapping)
 		return;
 
-	if (drm_vma_node_has_offset(&bo->vma_node)) {
-		offset = (loff_t) drm_vma_node_offset_addr(&bo->vma_node);
-		holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
-
-		unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
-	}
+	drm_vma_node_unmap(&bo->vma_node, bdev->dev_mapping);
 	ttm_mem_io_free_vm(bo);
 }
 
diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
index 4d6a734..185e9aa 100644
--- a/include/drm/drm_vma_manager.h
+++ b/include/drm/drm_vma_manager.h
@@ -24,6 +24,7 @@
  */
 
 #include <drm/drm_mm.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/rbtree.h>
 #include <linux/spinlock.h>
@@ -103,4 +104,19 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
 	return ((__u64)node->vm_node.start) << PAGE_SHIFT;
 }
 
+/** drm_vma_node_unmap()
+ *
+ * Unmap all userspace mappings for a given offset node. The mappings must be
+ * associated with the @file_mapping address-space. If no offset exists or
+ * the address-space is invalid, nothing is done.
+ */
+static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node,
+				      struct address_space *file_mapping)
+{
+	if (file_mapping && drm_vma_node_has_offset(node))
+		unmap_mapping_range(file_mapping,
+				    drm_vma_node_offset_addr(node),
+				    node->vm_pages << PAGE_SHIFT, 1);
+}
+
 #endif /* __DRM_VMA_MANAGER_H__ */
-- 
1.8.3.2

Re: [PATCH 0/6] DRM: Unified VMA Offset Manager

[Rob Clark]

On Mon, Jul 1, 2013 at 2:32 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> I picked up the initial work from Dave [1], fixed several bugs, rewrote the
> drm_mm node handling and adjusted the different drivers.
> The series tries to replace the VMA-offset managers from GEM and TTM with a
> single unified implementation. It uses the TTM RBTree idea to allow sub-mappings
> (which wouldn't be feasible with hashtables).
>
> Changes to Dave's v1:
>  * Fixed a ref-count bug in TTM during object lookup
>  * Use embedded drm_mm_node objects to avoid allocations
>  * Document drm_vma_* API
>  * Reviewed TTM locking
>  * Fixed all new drivers
>  * Use node->vm_pages instead of obj->size for GEM size calculations
>
> Notes:
>  * Tested on nouveau only! I will try to test i915 this week. However, the
>    gem changes seem pretty trivial.
>  * I couldn't even compile-test the ARM drivers. However, the omapdrm diffs
>    are the only changes that are non-trivial. Is there any ongoing work to
>    remove the arch-deps in DRM drivers?

I think most of the arm drivers should support ARCH_MULTIPLATFORM now,
so at least if you have a cross compiler it should be pretty easy to
compile-test most of the arm drm drivers..

BR,
-R

>  * _DRM_GEM is no longer used, but I guess we need to keep it for backwards
>    compat?
>  * If we replace node_list in drm_mm with an rbtree, we can drop it from
>    drm_vma_offset_manager completely. However, I wanted to avoid heavy drm_mm
>    changes and left this for follow up patches.
>  * This is currently based on linux-3.10 from today. Next series will be
>    rebased on drm-next/linux-next, but the current -next trees continously break
>    my machines..
>    But the only changes should be to fix additional drivers. I didn't see any
>    other things to fix for drm-next.
>
> Another series, which I will send later, adds "struct file" lists for each
> drm-vma-offset so we can get access control over gem objects. Also, I have an
> experimental series to remove the allocation helpers in drm_mm and let drivers
> embed drm_mm_node instead. Lets see how that works out.
>
> Comments welcome!
> Cheers
> David
>
> [1]: http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-vma-manager
>
> David Herrmann (6):
>   drm: make drm_mm_init() return void
>   drm: mm: add drm_mm_node_linked() helper
>   drm: add unified vma offset manager
>   drm: gem: convert to new unified vma manager
>   drm: ttm: convert to unified vma offset manager
>   drm: provide generic drm_vma_node_unmap() helper
>
>  drivers/gpu/drm/Makefile                   |   2 +-
>  drivers/gpu/drm/ast/ast_main.c             |   2 +-
>  drivers/gpu/drm/cirrus/cirrus_main.c       |   2 +-
>  drivers/gpu/drm/drm_gem.c                  |  93 ++----------
>  drivers/gpu/drm/drm_gem_cma_helper.c       |   9 +-
>  drivers/gpu/drm/drm_mm.c                   |   5 +-
>  drivers/gpu/drm/drm_vma_manager.c          | 224 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.c    |   7 +-
>  drivers/gpu/drm/gma500/gem.c               |   8 +-
>  drivers/gpu/drm/i915/i915_gem.c            |  13 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c     |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c  |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c      |   2 +-
>  drivers/gpu/drm/omapdrm/omap_gem.c         |  11 +-
>  drivers/gpu/drm/omapdrm/omap_gem_helpers.c |  49 +------
>  drivers/gpu/drm/qxl/qxl_object.h           |   2 +-
>  drivers/gpu/drm/qxl/qxl_release.c          |   2 +-
>  drivers/gpu/drm/radeon/radeon_object.h     |   5 +-
>  drivers/gpu/drm/ttm/ttm_bo.c               |  82 ++---------
>  drivers/gpu/drm/ttm/ttm_bo_manager.c       |   8 +-
>  drivers/gpu/drm/ttm/ttm_bo_util.c          |   3 +-
>  drivers/gpu/drm/ttm/ttm_bo_vm.c            |  81 ++++-------
>  drivers/gpu/drm/udl/udl_gem.c              |   6 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |   4 +-
>  include/drm/drmP.h                         |   7 +-
>  include/drm/drm_mm.h                       |  11 +-
>  include/drm/drm_vma_manager.h              | 122 ++++++++++++++++
>  include/drm/ttm/ttm_bo_api.h               |  15 +-
>  include/drm/ttm/ttm_bo_driver.h            |   7 +-
>  include/uapi/drm/drm.h                     |   2 +-
>  30 files changed, 464 insertions(+), 324 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_vma_manager.c
>  create mode 100644 include/drm/drm_vma_manager.h
>
> --
> 1.8.3.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm: make drm_mm_init() return void

[Daniel Vetter]

On Mon, Jul 01, 2013 at 08:32:58PM +0200, David Herrmann wrote:
> There is no reason to return "int" as this function never fails.
> Furthermore, several drivers (ast, sis) already depend on this.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Back when I've reworked drm_mm I was still a rookie and didn't want to
touch all drivers, hence why I've left the int return type. Good riddance
to that!

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_gem.c            | 8 ++------
>  drivers/gpu/drm/drm_mm.c             | 4 +---
>  drivers/gpu/drm/ttm/ttm_bo.c         | 6 +-----
>  drivers/gpu/drm/ttm/ttm_bo_manager.c | 8 +-------
>  include/drm/drm_mm.h                 | 6 +++---
>  5 files changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index cf919e3..88f0322 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -108,12 +108,8 @@ drm_gem_init(struct drm_device *dev)
>  		return -ENOMEM;
>  	}
>  
> -	if (drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
> -			DRM_FILE_PAGE_OFFSET_SIZE)) {
> -		drm_ht_remove(&mm->offset_hash);
> -		kfree(mm);
> -		return -ENOMEM;
> -	}
> +	drm_mm_init(&mm->offset_manager, DRM_FILE_PAGE_OFFSET_START,
> +		    DRM_FILE_PAGE_OFFSET_SIZE);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 07cf99c..7917729 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -669,7 +669,7 @@ int drm_mm_clean(struct drm_mm * mm)
>  }
>  EXPORT_SYMBOL(drm_mm_clean);
>  
> -int drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
> +void drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
>  {
>  	INIT_LIST_HEAD(&mm->hole_stack);
>  	INIT_LIST_HEAD(&mm->unused_nodes);
> @@ -690,8 +690,6 @@ int drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
>  	list_add_tail(&mm->head_node.hole_stack, &mm->hole_stack);
>  
>  	mm->color_adjust = NULL;
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL(drm_mm_init);
>  
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 9b07b7d..e97c5a0 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1619,9 +1619,7 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
>  		goto out_no_sys;
>  
>  	bdev->addr_space_rb = RB_ROOT;
> -	ret = drm_mm_init(&bdev->addr_space_mm, file_page_offset, 0x10000000);
> -	if (unlikely(ret != 0))
> -		goto out_no_addr_mm;
> +	drm_mm_init(&bdev->addr_space_mm, file_page_offset, 0x10000000);
>  
>  	INIT_DELAYED_WORK(&bdev->wq, ttm_bo_delayed_workqueue);
>  	INIT_LIST_HEAD(&bdev->ddestroy);
> @@ -1635,8 +1633,6 @@ int ttm_bo_device_init(struct ttm_bo_device *bdev,
>  	mutex_unlock(&glob->device_list_mutex);
>  
>  	return 0;
> -out_no_addr_mm:
> -	ttm_bo_clean_mm(bdev, 0);
>  out_no_sys:
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_manager.c b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> index 9212494..e4367f9 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_manager.c
> @@ -103,18 +103,12 @@ static int ttm_bo_man_init(struct ttm_mem_type_manager *man,
>  			   unsigned long p_size)
>  {
>  	struct ttm_range_manager *rman;
> -	int ret;
>  
>  	rman = kzalloc(sizeof(*rman), GFP_KERNEL);
>  	if (!rman)
>  		return -ENOMEM;
>  
> -	ret = drm_mm_init(&rman->mm, 0, p_size);
> -	if (ret) {
> -		kfree(rman);
> -		return ret;
> -	}
> -
> +	drm_mm_init(&rman->mm, 0, p_size);
>  	spin_lock_init(&rman->lock);
>  	man->priv = rman;
>  	return 0;
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index 88591ef..de92425 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -275,9 +275,9 @@ static inline  struct drm_mm_node *drm_mm_search_free_in_range_color(
>  	return drm_mm_search_free_in_range_generic(mm, size, alignment, color,
>  						   start, end, best_match);
>  }
> -extern int drm_mm_init(struct drm_mm *mm,
> -		       unsigned long start,
> -		       unsigned long size);
> +extern void drm_mm_init(struct drm_mm *mm,
> +			unsigned long start,
> +			unsigned long size);
>  extern void drm_mm_takedown(struct drm_mm *mm);
>  extern int drm_mm_clean(struct drm_mm *mm);
>  extern int drm_mm_pre_get(struct drm_mm *mm);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/6] drm: mm: add drm_mm_node_linked() helper

[Daniel Vetter]

On Mon, Jul 01, 2013 at 08:32:59PM +0200, David Herrmann wrote:
> This helper tests whether a given node is currently linked into a drm_mm
> manager. We use the node->mm pointer for that as it is set for all linked
> objects.
> 
> We also reset node->mm whenever a node is removed. All such access is
> currently safe as everyone calls kfree() on the object directly after
> removing it. Furthermore, no-one is supposed to access node->mm from
> outside drm_mm.c, anyway.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/gpu/drm/drm_mm.c | 1 +
>  include/drm/drm_mm.h     | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> index 7917729..3bd43ce 100644
> --- a/drivers/gpu/drm/drm_mm.c
> +++ b/drivers/gpu/drm/drm_mm.c
> @@ -374,6 +374,7 @@ void drm_mm_remove_node(struct drm_mm_node *node)
>  
>  	list_del(&node->node_list);
>  	node->allocated = 0;
> +	node->mm = NULL;
>  }
>  EXPORT_SYMBOL(drm_mm_remove_node);
>  
> diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
> index de92425..d83b966 100644
> --- a/include/drm/drm_mm.h
> +++ b/include/drm/drm_mm.h
> @@ -85,6 +85,11 @@ static inline bool drm_mm_node_allocated(struct drm_mm_node *node)
>  	return node->allocated;
>  }
>  
> +static inline bool drm_mm_node_linked(struct drm_mm_node *node)
> +{
> +	return node->mm;
> +}

I've only checked a few examples, but this seems to duplicate what the
drm_mm_node_allocated right above was meant for. Or do I miss something
here?
-Daniel

> +
>  static inline bool drm_mm_initialized(struct drm_mm *mm)
>  {
>  	return mm->hole_stack.next;
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm: add unified vma offset manager

[Daniel Vetter]

On Mon, Jul 01, 2013 at 08:33:00PM +0200, David Herrmann wrote:
> If we want to map GPU memory into user-space, we need to linearize the
> addresses to not confuse mm-core. Currently, GEM and TTM both implement
> their own offset-managers to assign a pgoff to each object for user-space
> CPU access. GEM uses a hash-table, TTM uses an rbtree.
> 
> This patch provides a unified implementation that can be used to replace
> both. TTM allows partial mmaps with a given offset, so we cannot use
> hashtables as the start address may not be known at mmap time. Hence, we
> use the rbtree-implementation of TTM.
> 
> We could easily update drm_mm to use an rbtree instead of a linked list
> for it's object list and thus drop the rbtree from the vma-manager.
> However, this would slow down drm_mm object allocation for all other
> use-cases (rbtree insertion) and add another 4-8 bytes to each mm node.
> Hence, use the separate tree but allow for later migration.
> 
> This is a rewrite of the 2012-proposal by David Airlie <airlied@linux.ie>
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

This seems to sprinkle a lot of likely/unlikely. Imo those are only
justified in two cases:
- When we have real performance data proving that they're worth it.
- Sometimes they're also useful as self-documenting code for the truly
  unlikely. But early do-nothing returns and error handling are
  established code patterns, so imo don't qualify.

I think none of the likely/unlikely below qualify, so imo better to remove
them.

Second high-level review request: Can you please convert the very nice
documentation you already added into proper kerneldoc and link it up at an
appropriate section in the drm DocBook?

A few more comments below. I'll postpone reviewing the gem/ttm conversion
patches until this is settled a bit.

> ---
>  drivers/gpu/drm/Makefile          |   2 +-
>  drivers/gpu/drm/drm_vma_manager.c | 224 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_vma_manager.h     | 107 ++++++++++++++++++
>  3 files changed, 332 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_vma_manager.c
>  create mode 100644 include/drm/drm_vma_manager.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c9f2439..f8851cc 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -12,7 +12,7 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>  		drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
>  		drm_crtc.o drm_modes.o drm_edid.o \
>  		drm_info.o drm_debugfs.o drm_encoder_slave.o \
> -		drm_trace_points.o drm_global.o drm_prime.o
> +		drm_trace_points.o drm_global.o drm_prime.o drm_vma_manager.o
>  
>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
> new file mode 100644
> index 0000000..c28639f
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_vma_manager.c
> @@ -0,0 +1,224 @@
> +/*
> + * Copyright (c) 2012 David Airlie <airlied@linux.ie>
> + * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_mm.h>
> +#include <drm/drm_vma_manager.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/rbtree.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +/** @file drm_vma_manager.c
> + *
> + * The vma-manager is responsible to map arbitrary driver-dependent memory
> + * regions into the linear user address-space. It provides offsets to the
> + * caller which can then be used on the address_space of the drm-device. It
> + * takes care to not overlap regions, size them appropriately and to not
> + * confuse mm-core by inconsistent fake vm_pgoff fields.
> + *
> + * We use drm_mm as backend to manage object allocations. But it is highly
> + * optimized for alloc/free calls, not lookups. Hence, we use an rb-tree to
> + * speed up offset lookups.
> + */
> +
> +/** drm_vma_offset_manager_init()
> + *
> + * Initialize a new offset-manager. The offset and area size available for the
> + * manager are given as @page_offset and @size. Both are interpreted as
> + * page-numbers, not bytes.
> + *
> + * Adding/removing nodes from the manager is locked internally and protected
> + * against concurrent access. However, node allocation and destruction is left
> + * for the caller. While calling into the vma-manager, a given node must
> + * always be guaranteed to be referenced.
> + */
> +void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
> +				 unsigned long page_offset, unsigned long size)
> +{
> +	rwlock_init(&mgr->vm_lock);
> +	mgr->vm_addr_space_rb = RB_ROOT;
> +	drm_mm_init(&mgr->vm_addr_space_mm, page_offset, size);
> +}
> +EXPORT_SYMBOL(drm_vma_offset_manager_init);
> +
> +/** drm_vma_offset_manager_destroy()
> + *
> + * Destroy an object manager which was previously created via
> + * drm_vma_offset_manager_init(). The caller must remove all allocated nodes
> + * before destroying the manager. Otherwise, drm_mm will refuse to free the
> + * requested resources.
> + *
> + * The manager must not be accessed after this function is called.
> + */
> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
> +{
> +	BUG_ON(!drm_mm_clean(&mgr->vm_addr_space_mm));

Please convert this to a WARN_ON - drm_mm has code to cobble over buggy
drivers, and killing the driver with a BUG_ON if we could keep on going
with just a WARN_ON is a real pain for development.

> +
> +	/* take the lock to protect against buggy drivers */
> +	write_lock(&mgr->vm_lock);
> +	drm_mm_takedown(&mgr->vm_addr_space_mm);
> +	write_unlock(&mgr->vm_lock);
> +}
> +EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
> +
> +/** drm_vma_offset_lookup()
> + *
> + * Find a node given a start address and object size. This returns the _best_
> + * match for the given node. That is, @start may point somewhere into a valid
> + * region and the given node will be returned, as long as the node spans the
> + * whole requested area (given the size in number of pages as @pages).
> + *
> + * Returns NULL if no suitable node can be found. Otherwise, the best match
> + * is returned. It's the caller's responsibility to make sure the node doesn't
> + * get destroyed before the caller can access it.
> + */
> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
> +						  unsigned long start,
> +						  unsigned long pages)
> +{
> +	struct drm_vma_offset_node *node, *best;
> +	struct rb_node *iter;
> +	unsigned long offset;
> +
> +	read_lock(&mgr->vm_lock);
> +
> +	iter = mgr->vm_addr_space_rb.rb_node;
> +	best = NULL;
> +
> +	while (likely(iter)) {
> +		node = rb_entry(iter, struct drm_vma_offset_node, vm_rb);
> +		offset = node->vm_node.start;
> +		if (start >= offset) {
> +			iter = iter->rb_right;
> +			best = node;
> +			if (start == offset)
> +				break;
> +		} else {
> +			iter = iter->rb_left;
> +		}
> +	}
> +
> +	/* verify that the node spans the requested area */
> +	if (likely(best)) {
> +		offset = best->vm_node.start + best->vm_pages;
> +		if (offset > start + pages)
> +			best = NULL;
> +	}
> +
> +	read_unlock(&mgr->vm_lock);
> +
> +	return best;
> +}
> +EXPORT_SYMBOL(drm_vma_offset_lookup);
> +
> +/* internal helper to link @node into the rb-tree */
> +static void _drm_vma_offset_add_rb(struct drm_vma_offset_manager *mgr,
> +				   struct drm_vma_offset_node *node)
> +{
> +	struct rb_node **iter = &mgr->vm_addr_space_rb.rb_node;
> +	struct rb_node *parent = NULL;
> +	struct drm_vma_offset_node *iter_node;
> +
> +	while (likely(*iter)) {
> +		parent = *iter;
> +		iter_node = rb_entry(*iter, struct drm_vma_offset_node, vm_rb);
> +
> +		if (node->vm_node.start < iter_node->vm_node.start)
> +			iter = &(*iter)->rb_left;
> +		else if (node->vm_node.start > iter_node->vm_node.start)
> +			iter = &(*iter)->rb_right;
> +		else
> +			BUG();
> +	}
> +
> +	rb_link_node(&node->vm_rb, parent, iter);
> +	rb_insert_color(&node->vm_rb, &mgr->vm_addr_space_rb);
> +}
> +
> +/** drm_vma_offset_add()
> + *
> + * Add a node to the offset-manager. If the node was already added, this does
> + * nothing and return 0. @pages is the size of the object given in number of
> + * pages.
> + * After this call succeeds, you can access the offset of the node until it
> + * is removed again.
> + *
> + * If this call fails, it is safe to retry the operation or call
> + * drm_vma_offset_remove(), anyway. However, no cleanup is required in that
> + * case.
> + */
> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
> +		       struct drm_vma_offset_node *node, unsigned long pages)
> +{
> +	int ret;
> +
> +	write_lock(&mgr->vm_lock);
> +
> +	if (unlikely(drm_mm_node_linked(&node->vm_node))) {
> +		ret = 0;
> +		goto out_unlock;
> +	}
> +
> +	ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm,
> +					 &node->vm_node, pages, 0, 0);

This allocates memory, so potentially blocks. This doesn't mesh well with
the spinning rwlock (lockdep should seriously complain about this btw).
Please use an rwsemaphore instead.

> +	if (unlikely(ret))
> +		goto out_reset;
> +
> +	node->vm_pages = pages;
> +	_drm_vma_offset_add_rb(mgr, node);
> +	goto out_unlock;
> +
> +out_reset:
> +	/* we don't know what happened to @node->vm_node, so clear it */
> +	drm_vma_node_reset(node);

If you switch over to drm_mm_node_allocated then this reset could
shouldn't be required - it'd be a pretty serious bug in drm_mm.c

> +out_unlock:
> +	write_unlock(&mgr->vm_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_vma_offset_add);
> +
> +/** drm_vma_offset_remove()
> + *
> + * Remove a node from the offset manager. If the node wasn't added before, this
> + * does nothing. After this call returns, the offset of the node must be not
> + * accessed, anymore.
> + * It is safe to add the node via drm_vma_offset_add() again.
> + */
> +void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
> +			   struct drm_vma_offset_node *node)
> +{
> +	if (unlikely(!drm_mm_node_linked(&node->vm_node)))
> +		return;
> +
> +	write_lock(&mgr->vm_lock);
> +
> +	rb_erase(&node->vm_rb, &mgr->vm_addr_space_rb);
> +	drm_mm_remove_node(&node->vm_node);
> +	drm_vma_node_reset(node);
> +
> +	write_unlock(&mgr->vm_lock);
> +}
> +EXPORT_SYMBOL(drm_vma_offset_remove);
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> new file mode 100644
> index 0000000..8a1975c
> --- /dev/null
> +++ b/include/drm/drm_vma_manager.h
> @@ -0,0 +1,107 @@
> +#ifndef __DRM_VMA_MANAGER_H__
> +#define __DRM_VMA_MANAGER_H__
> +
> +/*
> + * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_mm.h>
> +#include <linux/module.h>
> +#include <linux/rbtree.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
> +struct drm_vma_offset_node {
> +	struct drm_mm_node vm_node;
> +	struct rb_node vm_rb;
> +	unsigned long vm_pages;
> +};
> +
> +struct drm_vma_offset_manager {
> +	rwlock_t vm_lock;
> +	struct rb_root vm_addr_space_rb;
> +	struct drm_mm vm_addr_space_mm;
> +};
> +
> +void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
> +				 unsigned long page_offset, unsigned long size);
> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
> +
> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
> +						  unsigned long start,
> +						  unsigned long pages);
> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
> +		       struct drm_vma_offset_node *node, unsigned long pages);
> +void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
> +			   struct drm_vma_offset_node *node);
> +
> +/** drm_vma_offset_exact_lookup()
> + *
> + * Same as drm_vma_offset_lookup() but does not allow any offset but only
> + * returns the exact object with the given start address.
> + */
> +static inline struct drm_vma_offset_node *
> +drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
> +			    unsigned long start)
> +{
> +	return drm_vma_offset_lookup(mgr, start, 1);
> +}
> +
> +/** drm_vma_node_reset()
> + *
> + * Reset a node to its initial state.
> + */
> +static inline void drm_vma_node_reset(struct drm_vma_offset_node *node)
> +{
> +	memset(&node->vm_node, 0, sizeof(node->vm_node));
> +}
> +
> +/** drm_vma_node_start()
> + *
> + * Return the start address of the given node as pfn.
> + */
> +static inline unsigned long drm_vma_node_start(struct drm_vma_offset_node *node)
> +{
> +	return node->vm_node.start;
> +}
> +
> +/** drm_vma_node_has_offset()
> + *
> + * Return true iff the node was previously allocated an offset and added to
> + * an vma offset manager.
> + */
> +static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node)
> +{
> +	return drm_mm_node_linked(&node->vm_node);
> +}
> +
> +/** drm_vma_node_offset_addr()
> + *
> + * Same as drm_vma_node_start() but returns the address as a valid userspace
> + * address (instead of a pfn).
> + */

Userspace address and pfn are a bit misleading imo in this context. I'd go
with "byte offset for consumption by userspace/mmap" and "page-based
addressing" or something similar.

> +static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
> +{
> +	return ((__u64)node->vm_node.start) << PAGE_SHIFT;
> +}
> +
> +#endif /* __DRM_VMA_MANAGER_H__ */
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/6] drm: provide generic drm_vma_node_unmap() helper

[Daniel Vetter]

On Mon, Jul 01, 2013 at 08:33:03PM +0200, David Herrmann wrote:
> Instead of unmapping the nodes in TTM and GEM users manually, we provide
> a generic wrapper which does the correct thing for all vma-nodes.
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>

Nice. One nitpick below, otherwise:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_gem.c |  6 +-----
>  drivers/gpu/drm/ttm/ttm_bo.c    |  8 +-------
>  include/drm/drm_vma_manager.h   | 16 ++++++++++++++++
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1617166..43f9aaf 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1427,11 +1427,7 @@ i915_gem_release_mmap(struct drm_i915_gem_object *obj)
>  	if (!obj->fault_mappable)
>  		return;
>  
> -	if (obj->base.dev->dev_mapping)
> -		unmap_mapping_range(obj->base.dev->dev_mapping,
> -				    (loff_t)drm_vma_node_offset_addr(&obj->base.vma_node),
> -				    obj->base.size, 1);
> -
> +	drm_vma_node_unmap(&obj->base.vma_node, obj->base.dev->dev_mapping);
>  	obj->fault_mappable = false;
>  }
>  
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 2b96a75..2979070 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -1657,17 +1657,11 @@ bool ttm_mem_reg_is_pci(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
>  void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
>  {
>  	struct ttm_bo_device *bdev = bo->bdev;
> -	loff_t offset, holelen;
>  
>  	if (!bdev->dev_mapping)
>  		return;

Can't we kill this check now since your helper checks for the mapping already?

>  
> -	if (drm_vma_node_has_offset(&bo->vma_node)) {
> -		offset = (loff_t) drm_vma_node_offset_addr(&bo->vma_node);
> -		holelen = ((loff_t) bo->mem.num_pages) << PAGE_SHIFT;
> -
> -		unmap_mapping_range(bdev->dev_mapping, offset, holelen, 1);
> -	}
> +	drm_vma_node_unmap(&bo->vma_node, bdev->dev_mapping);
>  	ttm_mem_io_free_vm(bo);
>  }
>  
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 4d6a734..185e9aa 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -24,6 +24,7 @@
>   */
>  
>  #include <drm/drm_mm.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/rbtree.h>
>  #include <linux/spinlock.h>
> @@ -103,4 +104,19 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
>  	return ((__u64)node->vm_node.start) << PAGE_SHIFT;
>  }
>  
> +/** drm_vma_node_unmap()
> + *
> + * Unmap all userspace mappings for a given offset node. The mappings must be
> + * associated with the @file_mapping address-space. If no offset exists or
> + * the address-space is invalid, nothing is done.
> + */
> +static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node,
> +				      struct address_space *file_mapping)
> +{
> +	if (file_mapping && drm_vma_node_has_offset(node))
> +		unmap_mapping_range(file_mapping,
> +				    drm_vma_node_offset_addr(node),
> +				    node->vm_pages << PAGE_SHIFT, 1);
> +}
> +
>  #endif /* __DRM_VMA_MANAGER_H__ */
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/6] DRM: Unified VMA Offset Manager

[Daniel Vetter]

On Mon, Jul 01, 2013 at 08:32:57PM +0200, David Herrmann wrote:
> Hi
> 
> I picked up the initial work from Dave [1], fixed several bugs, rewrote the
> drm_mm node handling and adjusted the different drivers.
> The series tries to replace the VMA-offset managers from GEM and TTM with a
> single unified implementation. It uses the TTM RBTree idea to allow sub-mappings
> (which wouldn't be feasible with hashtables).
> 
> Changes to Dave's v1:
>  * Fixed a ref-count bug in TTM during object lookup
>  * Use embedded drm_mm_node objects to avoid allocations
>  * Document drm_vma_* API
>  * Reviewed TTM locking
>  * Fixed all new drivers
>  * Use node->vm_pages instead of obj->size for GEM size calculations
> 
> Notes:
>  * Tested on nouveau only! I will try to test i915 this week. However, the
>    gem changes seem pretty trivial.
>  * I couldn't even compile-test the ARM drivers. However, the omapdrm diffs
>    are the only changes that are non-trivial. Is there any ongoing work to
>    remove the arch-deps in DRM drivers?
>  * _DRM_GEM is no longer used, but I guess we need to keep it for backwards
>    compat?

I seriously hope no piece of userspace ever used that. So if you can do
some history digging and that indeed turns out to be the case, then I'd
vote to kill _DRM_GEM. Makes it much clearer where the dri1 dungeons are
if there's no new-world stuff interspersed by accident ;-)
-Daniel

>  * If we replace node_list in drm_mm with an rbtree, we can drop it from
>    drm_vma_offset_manager completely. However, I wanted to avoid heavy drm_mm
>    changes and left this for follow up patches.
>  * This is currently based on linux-3.10 from today. Next series will be
>    rebased on drm-next/linux-next, but the current -next trees continously break
>    my machines..
>    But the only changes should be to fix additional drivers. I didn't see any
>    other things to fix for drm-next.
> 
> Another series, which I will send later, adds "struct file" lists for each
> drm-vma-offset so we can get access control over gem objects. Also, I have an
> experimental series to remove the allocation helpers in drm_mm and let drivers
> embed drm_mm_node instead. Lets see how that works out.
> 
> Comments welcome!
> Cheers
> David
> 
> [1]: http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-vma-manager
> 
> David Herrmann (6):
>   drm: make drm_mm_init() return void
>   drm: mm: add drm_mm_node_linked() helper
>   drm: add unified vma offset manager
>   drm: gem: convert to new unified vma manager
>   drm: ttm: convert to unified vma offset manager
>   drm: provide generic drm_vma_node_unmap() helper
> 
>  drivers/gpu/drm/Makefile                   |   2 +-
>  drivers/gpu/drm/ast/ast_main.c             |   2 +-
>  drivers/gpu/drm/cirrus/cirrus_main.c       |   2 +-
>  drivers/gpu/drm/drm_gem.c                  |  93 ++----------
>  drivers/gpu/drm/drm_gem_cma_helper.c       |   9 +-
>  drivers/gpu/drm/drm_mm.c                   |   5 +-
>  drivers/gpu/drm/drm_vma_manager.c          | 224 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.c    |   7 +-
>  drivers/gpu/drm/gma500/gem.c               |   8 +-
>  drivers/gpu/drm/i915/i915_gem.c            |  13 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c     |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c  |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c      |   2 +-
>  drivers/gpu/drm/omapdrm/omap_gem.c         |  11 +-
>  drivers/gpu/drm/omapdrm/omap_gem_helpers.c |  49 +------
>  drivers/gpu/drm/qxl/qxl_object.h           |   2 +-
>  drivers/gpu/drm/qxl/qxl_release.c          |   2 +-
>  drivers/gpu/drm/radeon/radeon_object.h     |   5 +-
>  drivers/gpu/drm/ttm/ttm_bo.c               |  82 ++---------
>  drivers/gpu/drm/ttm/ttm_bo_manager.c       |   8 +-
>  drivers/gpu/drm/ttm/ttm_bo_util.c          |   3 +-
>  drivers/gpu/drm/ttm/ttm_bo_vm.c            |  81 ++++-------
>  drivers/gpu/drm/udl/udl_gem.c              |   6 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |   4 +-
>  include/drm/drmP.h                         |   7 +-
>  include/drm/drm_mm.h                       |  11 +-
>  include/drm/drm_vma_manager.h              | 122 ++++++++++++++++
>  include/drm/ttm/ttm_bo_api.h               |  15 +-
>  include/drm/ttm/ttm_bo_driver.h            |   7 +-
>  include/uapi/drm/drm.h                     |   2 +-
>  30 files changed, 464 insertions(+), 324 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_vma_manager.c
>  create mode 100644 include/drm/drm_vma_manager.h
> 
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm: add unified vma offset manager

[David Herrmann]

Hi

On Mon, Jul 1, 2013 at 9:42 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 01, 2013 at 08:33:00PM +0200, David Herrmann wrote:
>> If we want to map GPU memory into user-space, we need to linearize the
>> addresses to not confuse mm-core. Currently, GEM and TTM both implement
>> their own offset-managers to assign a pgoff to each object for user-space
>> CPU access. GEM uses a hash-table, TTM uses an rbtree.
>>
>> This patch provides a unified implementation that can be used to replace
>> both. TTM allows partial mmaps with a given offset, so we cannot use
>> hashtables as the start address may not be known at mmap time. Hence, we
>> use the rbtree-implementation of TTM.
>>
>> We could easily update drm_mm to use an rbtree instead of a linked list
>> for it's object list and thus drop the rbtree from the vma-manager.
>> However, this would slow down drm_mm object allocation for all other
>> use-cases (rbtree insertion) and add another 4-8 bytes to each mm node.
>> Hence, use the separate tree but allow for later migration.
>>
>> This is a rewrite of the 2012-proposal by David Airlie <airlied@linux.ie>
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> This seems to sprinkle a lot of likely/unlikely. Imo those are only
> justified in two cases:
> - When we have real performance data proving that they're worth it.
> - Sometimes they're also useful as self-documenting code for the truly
>   unlikely. But early do-nothing returns and error handling are
>   established code patterns, so imo don't qualify.
>
> I think none of the likely/unlikely below qualify, so imo better to remove
> them.

They are copied mostly from ttm_bo.c. However, I agree that they are
probably unneeded. Furthermore, these aren't code-paths I'd expect to
be performance-critical. I will dig into the git-history of TTM to see
whether they have been in-place since the beginning or whether there
is some real reason for them.

> Second high-level review request: Can you please convert the very nice
> documentation you already added into proper kerneldoc and link it up at an
> appropriate section in the drm DocBook?

Sure.

> A few more comments below. I'll postpone reviewing the gem/ttm conversion
> patches until this is settled a bit.
>
>> ---
>>  drivers/gpu/drm/Makefile          |   2 +-
>>  drivers/gpu/drm/drm_vma_manager.c | 224 ++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_vma_manager.h     | 107 ++++++++++++++++++
>>  3 files changed, 332 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/drm_vma_manager.c
>>  create mode 100644 include/drm/drm_vma_manager.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1c9f2439..f8851cc 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -12,7 +12,7 @@ drm-y       :=      drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
>>               drm_platform.o drm_sysfs.o drm_hashtab.o drm_mm.o \
>>               drm_crtc.o drm_modes.o drm_edid.o \
>>               drm_info.o drm_debugfs.o drm_encoder_slave.o \
>> -             drm_trace_points.o drm_global.o drm_prime.o
>> +             drm_trace_points.o drm_global.o drm_prime.o drm_vma_manager.o
>>
>>  drm-$(CONFIG_COMPAT) += drm_ioc32.o
>>  drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
>> diff --git a/drivers/gpu/drm/drm_vma_manager.c b/drivers/gpu/drm/drm_vma_manager.c
>> new file mode 100644
>> index 0000000..c28639f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/drm_vma_manager.c
>> @@ -0,0 +1,224 @@
>> +/*
>> + * Copyright (c) 2012 David Airlie <airlied@linux.ie>
>> + * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_mm.h>
>> +#include <drm/drm_vma_manager.h>
>> +#include <linux/mm.h>
>> +#include <linux/module.h>
>> +#include <linux/rbtree.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +
>> +/** @file drm_vma_manager.c
>> + *
>> + * The vma-manager is responsible to map arbitrary driver-dependent memory
>> + * regions into the linear user address-space. It provides offsets to the
>> + * caller which can then be used on the address_space of the drm-device. It
>> + * takes care to not overlap regions, size them appropriately and to not
>> + * confuse mm-core by inconsistent fake vm_pgoff fields.
>> + *
>> + * We use drm_mm as backend to manage object allocations. But it is highly
>> + * optimized for alloc/free calls, not lookups. Hence, we use an rb-tree to
>> + * speed up offset lookups.
>> + */
>> +
>> +/** drm_vma_offset_manager_init()
>> + *
>> + * Initialize a new offset-manager. The offset and area size available for the
>> + * manager are given as @page_offset and @size. Both are interpreted as
>> + * page-numbers, not bytes.
>> + *
>> + * Adding/removing nodes from the manager is locked internally and protected
>> + * against concurrent access. However, node allocation and destruction is left
>> + * for the caller. While calling into the vma-manager, a given node must
>> + * always be guaranteed to be referenced.
>> + */
>> +void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
>> +                              unsigned long page_offset, unsigned long size)
>> +{
>> +     rwlock_init(&mgr->vm_lock);
>> +     mgr->vm_addr_space_rb = RB_ROOT;
>> +     drm_mm_init(&mgr->vm_addr_space_mm, page_offset, size);
>> +}
>> +EXPORT_SYMBOL(drm_vma_offset_manager_init);
>> +
>> +/** drm_vma_offset_manager_destroy()
>> + *
>> + * Destroy an object manager which was previously created via
>> + * drm_vma_offset_manager_init(). The caller must remove all allocated nodes
>> + * before destroying the manager. Otherwise, drm_mm will refuse to free the
>> + * requested resources.
>> + *
>> + * The manager must not be accessed after this function is called.
>> + */
>> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
>> +{
>> +     BUG_ON(!drm_mm_clean(&mgr->vm_addr_space_mm));
>
> Please convert this to a WARN_ON - drm_mm has code to cobble over buggy
> drivers, and killing the driver with a BUG_ON if we could keep on going
> with just a WARN_ON is a real pain for development.

Ok.

>> +
>> +     /* take the lock to protect against buggy drivers */
>> +     write_lock(&mgr->vm_lock);
>> +     drm_mm_takedown(&mgr->vm_addr_space_mm);
>> +     write_unlock(&mgr->vm_lock);
>> +}
>> +EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
>> +
>> +/** drm_vma_offset_lookup()
>> + *
>> + * Find a node given a start address and object size. This returns the _best_
>> + * match for the given node. That is, @start may point somewhere into a valid
>> + * region and the given node will be returned, as long as the node spans the
>> + * whole requested area (given the size in number of pages as @pages).
>> + *
>> + * Returns NULL if no suitable node can be found. Otherwise, the best match
>> + * is returned. It's the caller's responsibility to make sure the node doesn't
>> + * get destroyed before the caller can access it.
>> + */
>> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
>> +                                               unsigned long start,
>> +                                               unsigned long pages)
>> +{
>> +     struct drm_vma_offset_node *node, *best;
>> +     struct rb_node *iter;
>> +     unsigned long offset;
>> +
>> +     read_lock(&mgr->vm_lock);
>> +
>> +     iter = mgr->vm_addr_space_rb.rb_node;
>> +     best = NULL;
>> +
>> +     while (likely(iter)) {
>> +             node = rb_entry(iter, struct drm_vma_offset_node, vm_rb);
>> +             offset = node->vm_node.start;
>> +             if (start >= offset) {
>> +                     iter = iter->rb_right;
>> +                     best = node;
>> +                     if (start == offset)
>> +                             break;
>> +             } else {
>> +                     iter = iter->rb_left;
>> +             }
>> +     }
>> +
>> +     /* verify that the node spans the requested area */
>> +     if (likely(best)) {
>> +             offset = best->vm_node.start + best->vm_pages;
>> +             if (offset > start + pages)
>> +                     best = NULL;
>> +     }
>> +
>> +     read_unlock(&mgr->vm_lock);
>> +
>> +     return best;
>> +}
>> +EXPORT_SYMBOL(drm_vma_offset_lookup);
>> +
>> +/* internal helper to link @node into the rb-tree */
>> +static void _drm_vma_offset_add_rb(struct drm_vma_offset_manager *mgr,
>> +                                struct drm_vma_offset_node *node)
>> +{
>> +     struct rb_node **iter = &mgr->vm_addr_space_rb.rb_node;
>> +     struct rb_node *parent = NULL;
>> +     struct drm_vma_offset_node *iter_node;
>> +
>> +     while (likely(*iter)) {
>> +             parent = *iter;
>> +             iter_node = rb_entry(*iter, struct drm_vma_offset_node, vm_rb);
>> +
>> +             if (node->vm_node.start < iter_node->vm_node.start)
>> +                     iter = &(*iter)->rb_left;
>> +             else if (node->vm_node.start > iter_node->vm_node.start)
>> +                     iter = &(*iter)->rb_right;
>> +             else
>> +                     BUG();
>> +     }
>> +
>> +     rb_link_node(&node->vm_rb, parent, iter);
>> +     rb_insert_color(&node->vm_rb, &mgr->vm_addr_space_rb);
>> +}
>> +
>> +/** drm_vma_offset_add()
>> + *
>> + * Add a node to the offset-manager. If the node was already added, this does
>> + * nothing and return 0. @pages is the size of the object given in number of
>> + * pages.
>> + * After this call succeeds, you can access the offset of the node until it
>> + * is removed again.
>> + *
>> + * If this call fails, it is safe to retry the operation or call
>> + * drm_vma_offset_remove(), anyway. However, no cleanup is required in that
>> + * case.
>> + */
>> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
>> +                    struct drm_vma_offset_node *node, unsigned long pages)
>> +{
>> +     int ret;
>> +
>> +     write_lock(&mgr->vm_lock);
>> +
>> +     if (unlikely(drm_mm_node_linked(&node->vm_node))) {
>> +             ret = 0;
>> +             goto out_unlock;
>> +     }
>> +
>> +     ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm,
>> +                                      &node->vm_node, pages, 0, 0);
>
> This allocates memory, so potentially blocks. This doesn't mesh well with
> the spinning rwlock (lockdep should seriously complain about this btw).
> Please use an rwsemaphore instead.

Ugh? This shouldn't allocate any memory. I use pre-allocated nodes and
looking at drm_mm.c this just links the node at the correct position.
As long as the color_adjust() callbacks are atomic, this should be
fine here. Did you mix this up with get_block()?

>> +     if (unlikely(ret))
>> +             goto out_reset;
>> +
>> +     node->vm_pages = pages;
>> +     _drm_vma_offset_add_rb(mgr, node);
>> +     goto out_unlock;
>> +
>> +out_reset:
>> +     /* we don't know what happened to @node->vm_node, so clear it */
>> +     drm_vma_node_reset(node);
>
> If you switch over to drm_mm_node_allocated then this reset could
> shouldn't be required - it'd be a pretty serious bug in drm_mm.c

I thought this flag was used for kfree() tracking. Turns out, it's
not. I will fix it up and remove the unnecessary resets.

>> +out_unlock:
>> +     write_unlock(&mgr->vm_lock);
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL(drm_vma_offset_add);
>> +
>> +/** drm_vma_offset_remove()
>> + *
>> + * Remove a node from the offset manager. If the node wasn't added before, this
>> + * does nothing. After this call returns, the offset of the node must be not
>> + * accessed, anymore.
>> + * It is safe to add the node via drm_vma_offset_add() again.
>> + */
>> +void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>> +                        struct drm_vma_offset_node *node)
>> +{
>> +     if (unlikely(!drm_mm_node_linked(&node->vm_node)))
>> +             return;
>> +
>> +     write_lock(&mgr->vm_lock);
>> +
>> +     rb_erase(&node->vm_rb, &mgr->vm_addr_space_rb);
>> +     drm_mm_remove_node(&node->vm_node);
>> +     drm_vma_node_reset(node);
>> +
>> +     write_unlock(&mgr->vm_lock);
>> +}
>> +EXPORT_SYMBOL(drm_vma_offset_remove);
>> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
>> new file mode 100644
>> index 0000000..8a1975c
>> --- /dev/null
>> +++ b/include/drm/drm_vma_manager.h
>> @@ -0,0 +1,107 @@
>> +#ifndef __DRM_VMA_MANAGER_H__
>> +#define __DRM_VMA_MANAGER_H__
>> +
>> +/*
>> + * Copyright (c) 2013 David Herrmann <dh.herrmann@gmail.com>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_mm.h>
>> +#include <linux/module.h>
>> +#include <linux/rbtree.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +
>> +struct drm_vma_offset_node {
>> +     struct drm_mm_node vm_node;
>> +     struct rb_node vm_rb;
>> +     unsigned long vm_pages;
>> +};
>> +
>> +struct drm_vma_offset_manager {
>> +     rwlock_t vm_lock;
>> +     struct rb_root vm_addr_space_rb;
>> +     struct drm_mm vm_addr_space_mm;
>> +};
>> +
>> +void drm_vma_offset_manager_init(struct drm_vma_offset_manager *mgr,
>> +                              unsigned long page_offset, unsigned long size);
>> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr);
>> +
>> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
>> +                                               unsigned long start,
>> +                                               unsigned long pages);
>> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
>> +                    struct drm_vma_offset_node *node, unsigned long pages);
>> +void drm_vma_offset_remove(struct drm_vma_offset_manager *mgr,
>> +                        struct drm_vma_offset_node *node);
>> +
>> +/** drm_vma_offset_exact_lookup()
>> + *
>> + * Same as drm_vma_offset_lookup() but does not allow any offset but only
>> + * returns the exact object with the given start address.
>> + */
>> +static inline struct drm_vma_offset_node *
>> +drm_vma_offset_exact_lookup(struct drm_vma_offset_manager *mgr,
>> +                         unsigned long start)
>> +{
>> +     return drm_vma_offset_lookup(mgr, start, 1);
>> +}
>> +
>> +/** drm_vma_node_reset()
>> + *
>> + * Reset a node to its initial state.
>> + */
>> +static inline void drm_vma_node_reset(struct drm_vma_offset_node *node)
>> +{
>> +     memset(&node->vm_node, 0, sizeof(node->vm_node));
>> +}
>> +
>> +/** drm_vma_node_start()
>> + *
>> + * Return the start address of the given node as pfn.
>> + */
>> +static inline unsigned long drm_vma_node_start(struct drm_vma_offset_node *node)
>> +{
>> +     return node->vm_node.start;
>> +}
>> +
>> +/** drm_vma_node_has_offset()
>> + *
>> + * Return true iff the node was previously allocated an offset and added to
>> + * an vma offset manager.
>> + */
>> +static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node)
>> +{
>> +     return drm_mm_node_linked(&node->vm_node);
>> +}
>> +
>> +/** drm_vma_node_offset_addr()
>> + *
>> + * Same as drm_vma_node_start() but returns the address as a valid userspace
>> + * address (instead of a pfn).
>> + */
>
> Userspace address and pfn are a bit misleading imo in this context. I'd go
> with "byte offset for consumption by userspace/mmap" and "page-based
> addressing" or something similar.

Ok.

Thanks!

>> +static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
>> +{
>> +     return ((__u64)node->vm_node.start) << PAGE_SHIFT;
>> +}
>> +
>> +#endif /* __DRM_VMA_MANAGER_H__ */
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm: add unified vma offset manager

[Daniel Vetter]

On Mon, Jul 01, 2013 at 09:54:17PM +0200, David Herrmann wrote:
> On Mon, Jul 1, 2013 at 9:42 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Jul 01, 2013 at 08:33:00PM +0200, David Herrmann wrote:
> >> +/** drm_vma_offset_manager_destroy()
> >> + *
> >> + * Destroy an object manager which was previously created via
> >> + * drm_vma_offset_manager_init(). The caller must remove all allocated nodes
> >> + * before destroying the manager. Otherwise, drm_mm will refuse to free the
> >> + * requested resources.
> >> + *
> >> + * The manager must not be accessed after this function is called.
> >> + */
> >> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr)
> >> +{
> >> +     BUG_ON(!drm_mm_clean(&mgr->vm_addr_space_mm));
> >
> > Please convert this to a WARN_ON - drm_mm has code to cobble over buggy
> > drivers, and killing the driver with a BUG_ON if we could keep on going
> > with just a WARN_ON is a real pain for development.
> 
> Ok.

Actually I think the check in the takedown function should be good enough
already. I've just dumped a patch onto dri-devel which will convert that
to a WARN, so I think adding a second one here is redundant.

> 
> >> +
> >> +     /* take the lock to protect against buggy drivers */
> >> +     write_lock(&mgr->vm_lock);
> >> +     drm_mm_takedown(&mgr->vm_addr_space_mm);
> >> +     write_unlock(&mgr->vm_lock);
> >> +}
> >> +EXPORT_SYMBOL(drm_vma_offset_manager_destroy);
> >> +
> >> +/** drm_vma_offset_lookup()
> >> + *
> >> + * Find a node given a start address and object size. This returns the _best_
> >> + * match for the given node. That is, @start may point somewhere into a valid
> >> + * region and the given node will be returned, as long as the node spans the
> >> + * whole requested area (given the size in number of pages as @pages).
> >> + *
> >> + * Returns NULL if no suitable node can be found. Otherwise, the best match
> >> + * is returned. It's the caller's responsibility to make sure the node doesn't
> >> + * get destroyed before the caller can access it.
> >> + */
> >> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr,
> >> +                                               unsigned long start,
> >> +                                               unsigned long pages)
> >> +{
> >> +     struct drm_vma_offset_node *node, *best;
> >> +     struct rb_node *iter;
> >> +     unsigned long offset;
> >> +
> >> +     read_lock(&mgr->vm_lock);
> >> +
> >> +     iter = mgr->vm_addr_space_rb.rb_node;
> >> +     best = NULL;
> >> +
> >> +     while (likely(iter)) {
> >> +             node = rb_entry(iter, struct drm_vma_offset_node, vm_rb);
> >> +             offset = node->vm_node.start;
> >> +             if (start >= offset) {
> >> +                     iter = iter->rb_right;
> >> +                     best = node;
> >> +                     if (start == offset)
> >> +                             break;
> >> +             } else {
> >> +                     iter = iter->rb_left;
> >> +             }
> >> +     }
> >> +
> >> +     /* verify that the node spans the requested area */
> >> +     if (likely(best)) {
> >> +             offset = best->vm_node.start + best->vm_pages;
> >> +             if (offset > start + pages)
> >> +                     best = NULL;
> >> +     }
> >> +
> >> +     read_unlock(&mgr->vm_lock);
> >> +
> >> +     return best;
> >> +}
> >> +EXPORT_SYMBOL(drm_vma_offset_lookup);
> >> +
> >> +/* internal helper to link @node into the rb-tree */
> >> +static void _drm_vma_offset_add_rb(struct drm_vma_offset_manager *mgr,
> >> +                                struct drm_vma_offset_node *node)
> >> +{
> >> +     struct rb_node **iter = &mgr->vm_addr_space_rb.rb_node;
> >> +     struct rb_node *parent = NULL;
> >> +     struct drm_vma_offset_node *iter_node;
> >> +
> >> +     while (likely(*iter)) {
> >> +             parent = *iter;
> >> +             iter_node = rb_entry(*iter, struct drm_vma_offset_node, vm_rb);
> >> +
> >> +             if (node->vm_node.start < iter_node->vm_node.start)
> >> +                     iter = &(*iter)->rb_left;
> >> +             else if (node->vm_node.start > iter_node->vm_node.start)
> >> +                     iter = &(*iter)->rb_right;
> >> +             else
> >> +                     BUG();
> >> +     }
> >> +
> >> +     rb_link_node(&node->vm_rb, parent, iter);
> >> +     rb_insert_color(&node->vm_rb, &mgr->vm_addr_space_rb);
> >> +}
> >> +
> >> +/** drm_vma_offset_add()
> >> + *
> >> + * Add a node to the offset-manager. If the node was already added, this does
> >> + * nothing and return 0. @pages is the size of the object given in number of
> >> + * pages.
> >> + * After this call succeeds, you can access the offset of the node until it
> >> + * is removed again.
> >> + *
> >> + * If this call fails, it is safe to retry the operation or call
> >> + * drm_vma_offset_remove(), anyway. However, no cleanup is required in that
> >> + * case.
> >> + */
> >> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr,
> >> +                    struct drm_vma_offset_node *node, unsigned long pages)
> >> +{
> >> +     int ret;
> >> +
> >> +     write_lock(&mgr->vm_lock);
> >> +
> >> +     if (unlikely(drm_mm_node_linked(&node->vm_node))) {
> >> +             ret = 0;
> >> +             goto out_unlock;
> >> +     }
> >> +
> >> +     ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm,
> >> +                                      &node->vm_node, pages, 0, 0);
> >
> > This allocates memory, so potentially blocks. This doesn't mesh well with
> > the spinning rwlock (lockdep should seriously complain about this btw).
> > Please use an rwsemaphore instead.
> 
> Ugh? This shouldn't allocate any memory. I use pre-allocated nodes and
> looking at drm_mm.c this just links the node at the correct position.
> As long as the color_adjust() callbacks are atomic, this should be
> fine here. Did you mix this up with get_block()?

Oops, indeed I've mixed stuff up ;-)

I'd still vote for an rwsem though since these codepaths shouldn't be
performance critical and in pathalogical corner cases the O(n) list
walking drm_mm does could result in quite awful long spinlock holding
times.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm: make drm_mm_init() return void

[Dave Airlie]

On Tue, Jul 2, 2013 at 5:22 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jul 01, 2013 at 08:32:58PM +0200, David Herrmann wrote:
>> There is no reason to return "int" as this function never fails.
>> Furthermore, several drivers (ast, sis) already depend on this.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>
> Back when I've reworked drm_mm I was still a rookie and didn't want to
> touch all drivers, hence why I've left the int return type. Good riddance
> to that!
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks, I've stuck this in -next as it looks like a nice cleanup I'd like now.

Dave.

Re: [PATCH 0/6] DRM: Unified VMA Offset Manager

[Laurent Pinchart]

Hi David,

On Monday 01 July 2013 20:32:57 David Herrmann wrote:
> Hi
> 
> I picked up the initial work from Dave [1], fixed several bugs, rewrote the
> drm_mm node handling and adjusted the different drivers.
> The series tries to replace the VMA-offset managers from GEM and TTM with a
> single unified implementation. It uses the TTM RBTree idea to allow
> sub-mappings (which wouldn't be feasible with hashtables).

Nice work, thank you. Could you please also update 
Documentation/DocBook/drm.tmpl ?

> Changes to Dave's v1:
>  * Fixed a ref-count bug in TTM during object lookup
>  * Use embedded drm_mm_node objects to avoid allocations
>  * Document drm_vma_* API
>  * Reviewed TTM locking
>  * Fixed all new drivers
>  * Use node->vm_pages instead of obj->size for GEM size calculations
> 
> Notes:
>  * Tested on nouveau only! I will try to test i915 this week. However, the
>    gem changes seem pretty trivial.
>  * I couldn't even compile-test the ARM drivers. However, the omapdrm diffs
>    are the only changes that are non-trivial. Is there any ongoing work to
>    remove the arch-deps in DRM drivers?
>  * _DRM_GEM is no longer used, but I guess we need to keep it for backwards
>    compat?
>  * If we replace node_list in drm_mm with an rbtree, we can drop it from
>    drm_vma_offset_manager completely. However, I wanted to avoid heavy
> drm_mm changes and left this for follow up patches.
>  * This is currently based on linux-3.10 from today. Next series will be
>    rebased on drm-next/linux-next, but the current -next trees continously
> break my machines..
>    But the only changes should be to fix additional drivers. I didn't see
> any other things to fix for drm-next.
> 
> Another series, which I will send later, adds "struct file" lists for each
> drm-vma-offset so we can get access control over gem objects. Also, I have
> an experimental series to remove the allocation helpers in drm_mm and let
> drivers embed drm_mm_node instead. Lets see how that works out.
> 
> Comments welcome!
> Cheers
> David
> 
> [1]: http://cgit.freedesktop.org/~airlied/linux/log/?h=drm-vma-manager
> 
> David Herrmann (6):
>   drm: make drm_mm_init() return void
>   drm: mm: add drm_mm_node_linked() helper
>   drm: add unified vma offset manager
>   drm: gem: convert to new unified vma manager
>   drm: ttm: convert to unified vma offset manager
>   drm: provide generic drm_vma_node_unmap() helper
> 
>  drivers/gpu/drm/Makefile                   |   2 +-
>  drivers/gpu/drm/ast/ast_main.c             |   2 +-
>  drivers/gpu/drm/cirrus/cirrus_main.c       |   2 +-
>  drivers/gpu/drm/drm_gem.c                  |  93 ++----------
>  drivers/gpu/drm/drm_gem_cma_helper.c       |   9 +-
>  drivers/gpu/drm/drm_mm.c                   |   5 +-
>  drivers/gpu/drm/drm_vma_manager.c          | 224 ++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.c    |   7 +-
>  drivers/gpu/drm/gma500/gem.c               |   8 +-
>  drivers/gpu/drm/i915/i915_gem.c            |  13 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c     |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_display.c  |   2 +-
>  drivers/gpu/drm/nouveau/nouveau_gem.c      |   2 +-
>  drivers/gpu/drm/omapdrm/omap_gem.c         |  11 +-
>  drivers/gpu/drm/omapdrm/omap_gem_helpers.c |  49 +------
>  drivers/gpu/drm/qxl/qxl_object.h           |   2 +-
>  drivers/gpu/drm/qxl/qxl_release.c          |   2 +-
>  drivers/gpu/drm/radeon/radeon_object.h     |   5 +-
>  drivers/gpu/drm/ttm/ttm_bo.c               |  82 ++---------
>  drivers/gpu/drm/ttm/ttm_bo_manager.c       |   8 +-
>  drivers/gpu/drm/ttm/ttm_bo_util.c          |   3 +-
>  drivers/gpu/drm/ttm/ttm_bo_vm.c            |  81 ++++-------
>  drivers/gpu/drm/udl/udl_gem.c              |   6 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_resource.c   |   4 +-
>  include/drm/drmP.h                         |   7 +-
>  include/drm/drm_mm.h                       |  11 +-
>  include/drm/drm_vma_manager.h              | 122 ++++++++++++++++
>  include/drm/ttm/ttm_bo_api.h               |  15 +-
>  include/drm/ttm/ttm_bo_driver.h            |   7 +-
>  include/uapi/drm/drm.h                     |   2 +-
>  30 files changed, 464 insertions(+), 324 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_vma_manager.c
>  create mode 100644 include/drm/drm_vma_manager.h

-- 
Regards,

Laurent Pinchart