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

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