DRM: Unified VMA Offset Manager

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/drm_gem.c +2 -6

index cf919e3..88f0322 100644
@@ -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;
 }

drivers/gpu/drm/drm_mm.c +1 -3

index 07cf99c..7917729 100644
@@ -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);
 

drivers/gpu/drm/ttm/ttm_bo.c +1 -5

index 9b07b7d..e97c5a0 100644
@@ -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;
 }

drivers/gpu/drm/ttm/ttm_bo_manager.c +1 -7

index 9212494..e4367f9 100644
@@ -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;

include/drm/drm_mm.h +3 -3

index 88591ef..de92425 100644
@@ -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);

Signature · 6 lines

1.8.3.2

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

Quoted 4 lines · 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/drm_mm.c | 1 +
 include/drm/drm_mm.h     | 5 +++++
 2 files changed, 6 insertions(+)

drivers/gpu/drm/drm_mm.c +1 -0

index 7917729..3bd43ce 100644
@@ -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);
 

include/drm/drm_mm.h +0 -0

index de92425..d83b966 100644

include/drm/drm_mm.h +1 -0

+
 static inline bool drm_mm_initialized(struct drm_mm *mm)
 {
 	return mm->hole_stack.next;

Signature · 6 lines

1.8.3.2

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

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 <[email protected]>

Signed-off-by David Herrmann <[email protected]>

---
 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

drivers/gpu/drm/Makefile +1 -1

index 1c9f2439..f8851cc 100644
@@ -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

drivers/gpu/drm/drm_vma_manager.c new file +68 -0

new file mode 100644
index 0000000..c28639f
@@ -0,0 +1,224 @@
+/*
+ * Copyright (c) 2012 David Airlie <[email protected]>
+ * Copyright (c) 2013 David Herrmann <[email protected]>
+ *
+ * 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()
+ *

drivers/gpu/drm/drm_vma_manager.c new file +98 -0

+
+	/* 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;

drivers/gpu/drm/drm_vma_manager.c new file +28 -0

+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);

include/drm/drm_vma_manager.h new file +91 -0

new file mode 100644
index 0000000..8a1975c
@@ -0,0 +1,107 @@
+#ifndef __DRM_VMA_MANAGER_H__
+#define __DRM_VMA_MANAGER_H__
+
+/*
+ * Copyright (c) 2013 David Herrmann <[email protected]>
+ *
+ * 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.
+ */

include/drm/drm_vma_manager.h new file +6 -0

+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__ */

Signature · 6 lines

1.8.3.2

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

Quoted 16 lines · 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.

Quoted 3 lines

This is a rewrite of the 2012-proposal by David Airlie <[email protected]>

Signed-off-by David Herrmann <[email protected]>

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.

Quoted 101 lines

---
 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

drivers/gpu/drm/Makefile +1 -1

index 1c9f2439..f8851cc 100644
@@ -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

drivers/gpu/drm/drm_vma_manager.c new file +74 -0

new file mode 100644
index 0000000..c28639f
@@ -0,0 +1,224 @@
+/*
+ * Copyright (c) 2012 David Airlie <[email protected]>
+ * Copyright (c) 2013 David Herrmann <[email protected]>
+ *
+ * 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.

drivers/gpu/drm/drm_vma_manager.c line 78

Quoted 4 lines

drivers/gpu/drm/drm_vma_manager.c new file +4 -0

+ */
+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.

Quoted 104 lines

drivers/gpu/drm/drm_vma_manager.c new file +104 -0

+
+     /* 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;

drivers/gpu/drm/drm_vma_manager.c line 186

Quoted 4 lines

drivers/gpu/drm/drm_vma_manager.c new file +4 -0

+     }
+
+     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.

Quoted 5 lines

drivers/gpu/drm/drm_vma_manager.c new file +5 -0

+     if (unlikely(ret))
+             goto out_reset;
+
+     node->vm_pages = pages;
+     _drm_vma_offset_add_rb(mgr, node);

drivers/gpu/drm/drm_vma_manager.c line 196

Quoted 5 lines

drivers/gpu/drm/drm_vma_manager.c new file +5 -0

+     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

Quoted 131 lines

drivers/gpu/drm/drm_vma_manager.c new file +28 -0

+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);

include/drm/drm_vma_manager.h new file +97 -0

new file mode 100644
index 0000000..8a1975c
@@ -0,0 +1,107 @@
+#ifndef __DRM_VMA_MANAGER_H__
+#define __DRM_VMA_MANAGER_H__
+
+/*
+ * Copyright (c) 2013 David Herrmann <[email protected]>
+ *
+ * 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()

include/drm/drm_vma_manager.h line 101

Quoted 4 lines

include/drm/drm_vma_manager.h new file +4 -0

+ *
+ * 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.

Quoted 9 lines

include/drm/drm_vma_manager.h new file +6 -1

+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

Quoted 4 lines

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

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

Quoted 17 lines · Daniel Vetter

drivers/gpu/drm/drm_vma_manager.c line 78

Quoted 12 lines · David Herrmann

drivers/gpu/drm/drm_vma_manager.c new file +12 -0

+/** 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.

Quoted 105 lines

Quoted 105 lines

drivers/gpu/drm/drm_vma_manager.c new file +104 -0

+
+     /* 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;

Quoted 8 lines

drivers/gpu/drm/drm_vma_manager.c line 186

Quoted 4 lines

drivers/gpu/drm/drm_vma_manager.c new file +4 -0

+     }
+
+     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()?

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_gem.c +1 -5

index 1617166..43f9aaf 100644
@@ -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;
 }
 

drivers/gpu/drm/ttm/ttm_bo.c +0 -0

index 2b96a75..2979070 100644

drivers/gpu/drm/ttm/ttm_bo.c +1 -6

-	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);
 }
 

include/drm/drm_vma_manager.h +16 -0

index 4d6a734..185e9aa 100644
@@ -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__ */

Signature · 6 lines

1.8.3.2

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

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

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

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

Signature · 6 lines

1.8.3.2

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

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