Hi On Wed, Jul 24, 2013 at 5:35 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Tue, Jul 23, 2013 at 02:47:13PM +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@xxxxxxxx> >> >> v2: >> - fix Docbook integration >> - drop drm_mm_node_linked() and use drm_mm_node_allocated() >> - remove unjustified likely/unlikely usage (but keep for rbtree paths) >> - remove BUG_ON() as drm_mm already does that >> - clarify page-based vs. byte-based addresses >> - use drm_vma_node_reset() for initialization, too >> v4: >> - allow external locking via drm_vma_offset_un/lock_lookup() >> - add locked lookup helper drm_vma_offset_lookup_locked() >> >> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx> >> --- >> Documentation/DocBook/drm.tmpl | 6 + >> drivers/gpu/drm/Makefile | 2 +- >> drivers/gpu/drm/drm_vma_manager.c | 283 ++++++++++++++++++++++++++++++++++++++ >> include/drm/drm_vma_manager.h | 180 ++++++++++++++++++++++++ >> 4 files changed, 470 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/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl >> index 7d1278e..87e22ec 100644 >> --- a/Documentation/DocBook/drm.tmpl >> +++ b/Documentation/DocBook/drm.tmpl >> @@ -2212,6 +2212,12 @@ void intel_crt_init(struct drm_device *dev) >> !Iinclude/drm/drm_rect.h >> !Edrivers/gpu/drm/drm_rect.c >> </sect2> >> + <sect2> >> + <title>VMA Offset Manager</title> >> +!Pdrivers/gpu/drm/drm_vma_manager.c vma offset manager >> +!Edrivers/gpu/drm/drm_vma_manager.c >> +!Iinclude/drm/drm_vma_manager.h >> + </sect2> >> </sect1> >> >> <!-- Internals: kms properties --> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 801bcaf..d943b94 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -13,7 +13,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.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_rect.o >> + drm_rect.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..27760bd >> --- /dev/null >> +++ b/drivers/gpu/drm/drm_vma_manager.c >> @@ -0,0 +1,283 @@ >> +/* >> + * Copyright (c) 2006-2009 VMware, Inc., Palo Alto, CA., USA >> + * Copyright (c) 2012 David Airlie <airlied@xxxxxxxx> >> + * Copyright (c) 2013 David Herrmann <dh.herrmann@xxxxxxxxx> >> + * >> + * 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> >> + >> +/** >> + * DOC: vma offset manager >> + * >> + * 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. >> + * Drivers shouldn't use this for object placement in VMEM. This manager should >> + * only be used to manage mappings into linear user-space VMs. >> + * >> + * 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. >> + * >> + * You must not use multiple offset managers on a single address_space. >> + * Otherwise, mm-core will be unable to tear down memory mappings as the VM will >> + * no longer be linear. Please use VM_NONLINEAR in that case and implement your >> + * own offset managers. >> + * >> + * This offset manager works on page-based addresses. That is, every argument >> + * and return code (with the exception of drm_vma_node_offset_addr()) is given >> + * in number of pages, not number of bytes. That means, object sizes and offsets >> + * must always be page-aligned (as usual). >> + * If you want to get a valid byte-based user-space address for a given offset, >> + * please see drm_vma_node_offset_addr(). >> + */ >> + >> +/** >> + * drm_vma_offset_manager_init - Initialize new offset-manager >> + * @mgr: Manager object >> + * @page_offset: Offset of available memory area (page-based) >> + * @size: Size of available memory area (page-based) > > Bikeshed on naming: s/memory area/address space range/. Feel free to > ignore. Makes sense, yepp. >> + * >> + * 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 offset manager >> + * @mgr: Manager object >> + * >> + * 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) >> +{ >> + /* 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 node in offset space >> + * @mgr: Manager object >> + * @start: Start address for object (page-based) >> + * @pages: Size of object (page-based) >> + * >> + * 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: >> + * 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; >> + >> + read_lock(&mgr->vm_lock); >> + node = drm_vma_offset_lookup_locked(mgr, start, pages); >> + read_unlock(&mgr->vm_lock); >> + >> + return node; >> +} >> +EXPORT_SYMBOL(drm_vma_offset_lookup); >> + >> +/** >> + * drm_vma_offset_lookup_locked() - Find node in offset space >> + * @mgr: Manager object >> + * @start: Start address for object (page-based) >> + * @pages: Size of object (page-based) >> + * >> + * Same as drm_vma_offset_lookup() but requires the caller to lock offset lookup >> + * manually. See drm_vma_offset_lock_lookup() for an example. >> + * >> + * RETURNS: >> + * Returns NULL if no suitable node can be found. Otherwise, the best match >> + * is returned. >> + */ >> +struct drm_vma_offset_node *drm_vma_offset_lookup_locked(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; >> + >> + 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 (best) { >> + offset = best->vm_node.start + best->vm_pages; >> + if (offset > start + pages) >> + best = NULL; >> + } >> + >> + return best; >> +} >> +EXPORT_SYMBOL(drm_vma_offset_lookup_locked); >> + >> +/* 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 offset node to manager >> + * @mgr: Manager object >> + * @node: Node to be added >> + * @pages: Allocation size visible to user-space (in number of pages) >> + * >> + * 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. >> + * >> + * @pages is not required to be the same size as the underlying memory object >> + * that you want to map. It only limits the size that user-space can map into >> + * their address space. >> + * >> + * RETURNS: >> + * 0 on success, negative error code on failure. >> + */ >> +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 (drm_mm_node_allocated(&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 (ret) >> + goto out_unlock; >> + >> + node->vm_pages = pages; > > Why not just use node->vm_node.size and remove vm_pages? In the first draft I initialized the size directly. I added it to drm_vma_offset_add() later, so that's a left-over. I will remove the duplicate and use node->vm_node.size directly. >> + _drm_vma_offset_add_rb(mgr, node); >> + goto out_unlock; > > Unnecessary goto. Fixed. >> + >> +out_unlock: >> + write_unlock(&mgr->vm_lock); >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_vma_offset_add); >> + >> +/** >> + * drm_vma_offset_remove() - Remove offset node from manager >> + * @mgr: Manager object >> + * @node: Node to be removed >> + * >> + * 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. > > I think we could clarify this a little "After this call returns the offset > node must not be used until a new offset is allocatd with > drm_vma_offset_add() again. Helper functions like > drm_vma_node_offset_addr() will return invalid data" They guarantee to return 0. I use that in the debugfs paths so we don't have to check drm_vma_node_has_offset() all the time. So I guess I will change it to "return 0 (invalid)" or alike instead of just "invalid". >> + * 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) >> +{ >> + write_lock(&mgr->vm_lock); >> + >> + if (drm_mm_node_allocated(&node->vm_node)) { >> + rb_erase(&node->vm_rb, &mgr->vm_addr_space_rb); >> + drm_mm_remove_node(&node->vm_node); >> + memset(&node->vm_node, 0, sizeof(node->vm_node)); >> + node->vm_pages = 0; >> + } >> + >> + 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..ace2925 >> --- /dev/null >> +++ b/include/drm/drm_vma_manager.h >> @@ -0,0 +1,180 @@ >> +#ifndef __DRM_VMA_MANAGER_H__ >> +#define __DRM_VMA_MANAGER_H__ >> + >> +/* >> + * Copyright (c) 2013 David Herrmann <dh.herrmann@xxxxxxxxx> >> + * >> + * 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/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); >> +struct drm_vma_offset_node *drm_vma_offset_lookup_locked(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() - Look up node by exact address >> + * @mgr: Manager object >> + * @start: Start address (page-based, not byte-based) >> + * >> + * Same as drm_vma_offset_lookup() but does not allow any offset into the node. >> + * It only returns the exact object with the given start address. >> + * >> + * RETURNS: >> + * Node at exact start address @start. >> + */ >> +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_offset_lock_lookup() - Lock lookup for extended private use >> + * @mgr: Manager object >> + * >> + * Lock VMA manager for extended lookups. Only *_locked() VMA function calls >> + * are allowed while holding this lock. All other contexts are blocked from VMA >> + * until the lock is released via drm_vma_offset_unlock_lookup(). >> + * >> + * Use this if you need to take a reference to the objects returned by >> + * drm_vma_offset_lookup_locked() before releasing this lock again. >> + * >> + * This lock must not be used for anything else than extended lookups. You must >> + * not call any other VMA helpers while holding this lock. >> + * >> + * Note: You're in atomic-context while holding this lock! >> + * >> + * Example: >> + * drm_vma_offset_lock_lookup(mgr); >> + * node = drm_vma_offset_lookup_locked(mgr); >> + * if (node) >> + * kref_get_unless_zero(container_of(node, sth, entr)); >> + * drm_vma_offset_unlock_lookup(mgr); >> + */ >> +static inline void drm_vma_offset_lock_lookup(struct drm_vma_offset_manager *mgr) >> +{ >> + read_lock(&mgr->vm_lock); >> +} >> + >> +/** >> + * drm_vma_offset_unlock_lookup() - Unlock lookup for extended private use >> + * @mgr: Manager object >> + * >> + * Release lookup-lock. See drm_vma_offset_lock_lookup() for more information. >> + */ >> +static inline void drm_vma_offset_unlock_lookup(struct drm_vma_offset_manager *mgr) >> +{ >> + read_unlock(&mgr->vm_lock); >> +} >> + >> +/** >> + * drm_vma_node_reset() - Initialize or reset node object >> + * @node: Node to initialize or reset >> + * >> + * Reset a node to its initial state. You must call this before using @node with >> + * any vma offset manager. > > The must here feels a bit strong since this is just a memset and pretty > much everyone allocates with kzalloc. So calling this is redundant. Maybe > > "This must be called if @node isn't already cleared (e.g. by allocating > with kzalloc) before it using it with any vma offset manager." Makes sense. drm_mm has the same semantics, anyway. >> + * >> + * This must not be called on an already allocated node, or you will leak >> + * memory. >> + */ >> +static inline void drm_vma_node_reset(struct drm_vma_offset_node *node) >> +{ >> + memset(node, 0, sizeof(*node)); >> +} >> + >> +/** >> + * drm_vma_node_start() - Return start address for page-based addressing >> + * @node: Node to inspect >> + * >> + * Return the start address of the given node. This can be used as offset into >> + * the linear VM space that is provided by the VMA offset manager. Note that >> + * this can only be used for page-based addressing. If you need a proper offset >> + * for user-space mappings, you must apply "<< PAGE_SHIFT" or use the >> + * drm_vma_node_offset_addr() helper instead. >> + * >> + * RETURNS: >> + * Start address of @node for page-based addressing. 0 if the node does not >> + * have an offset allocated. >> + */ >> +static inline unsigned long drm_vma_node_start(struct drm_vma_offset_node *node) >> +{ >> + return node->vm_node.start; >> +} >> + >> +/** >> + * drm_vma_node_has_offset() - Check whether node is added to offset manager >> + * @node: Node to be checked >> + * >> + * RETURNS: >> + * 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_allocated(&node->vm_node); >> +} >> + >> +/** >> + * drm_vma_node_offset_addr() - Return sanitized offset for user-space mmaps >> + * @node: Linked offset node >> + * >> + * Same as drm_vma_node_start() but returns the address as a valid offset that >> + * can be used for user-space mappings during mmap(). >> + * This must not be called on unlinked nodes. >> + * >> + * RETURNS: >> + * Offset of @node for byte-based addressing. 0 if the node does not have an >> + * object allocated. >> + */ >> +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__ */ > > With the above tiny bikesheds addressed this is: > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Thanks David _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel