[PATCH 3/6] drm: add unified vma offset manager
David Herrmann
dh.herrmann at gmail.com
Mon Jul 1 12:54:17 PDT 2013
Hi
On Mon, Jul 1, 2013 at 9:42 PM, Daniel Vetter <daniel at 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 at linux.ie>
>>
>> Signed-off-by: David Herrmann <dh.herrmann at 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 at linux.ie>
>> + * Copyright (c) 2013 David Herrmann <dh.herrmann at 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 at 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 at 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
More information about the dri-devel
mailing list