From: Daniel Vetter Subject: Re: [PATCH 3/6] drm: add unified vma offset manager Date: Mon, 1 Jul 2013 22:17:32 +0200 Message-ID: <20130701201731.GC18285@phenom.ffwll.local> References: <1372703583-9550-1-git-send-email-dh.herrmann@gmail.com> <1372703583-9550-4-git-send-email-dh.herrmann@gmail.com> <20130701194225.GZ18285@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f44.google.com (mail-ee0-f44.google.com [74.125.83.44]) by gabe.freedesktop.org (Postfix) with ESMTP id D5E18E60AE for ; Mon, 1 Jul 2013 13:17:33 -0700 (PDT) Received: by mail-ee0-f44.google.com with SMTP id c13so2305924eek.31 for ; Mon, 01 Jul 2013 13:17:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: David Herrmann Cc: Dave Airlie , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On Mon, Jul 01, 2013 at 09:54:17PM +0200, David Herrmann wrote: > On Mon, Jul 1, 2013 at 9:42 PM, Daniel Vetter wrote: > > On Mon, Jul 01, 2013 at 08:33:00PM +0200, David Herrmann wrote: > >> +/** drm_vma_offset_manager_destroy() > >> + * > >> + * Destroy an object manager which was previously created via > >> + * drm_vma_offset_manager_init(). The caller must remove all allocated nodes > >> + * before destroying the manager. Otherwise, drm_mm will refuse to free the > >> + * requested resources. > >> + * > >> + * The manager must not be accessed after this function is called. > >> + */ > >> +void drm_vma_offset_manager_destroy(struct drm_vma_offset_manager *mgr) > >> +{ > >> + BUG_ON(!drm_mm_clean(&mgr->vm_addr_space_mm)); > > > > Please convert this to a WARN_ON - drm_mm has code to cobble over buggy > > drivers, and killing the driver with a BUG_ON if we could keep on going > > with just a WARN_ON is a real pain for development. > > Ok. Actually I think the check in the takedown function should be good enough already. I've just dumped a patch onto dri-devel which will convert that to a WARN, so I think adding a second one here is redundant. > > >> + > >> + /* take the lock to protect against buggy drivers */ > >> + write_lock(&mgr->vm_lock); > >> + drm_mm_takedown(&mgr->vm_addr_space_mm); > >> + write_unlock(&mgr->vm_lock); > >> +} > >> +EXPORT_SYMBOL(drm_vma_offset_manager_destroy); > >> + > >> +/** drm_vma_offset_lookup() > >> + * > >> + * Find a node given a start address and object size. This returns the _best_ > >> + * match for the given node. That is, @start may point somewhere into a valid > >> + * region and the given node will be returned, as long as the node spans the > >> + * whole requested area (given the size in number of pages as @pages). > >> + * > >> + * Returns NULL if no suitable node can be found. Otherwise, the best match > >> + * is returned. It's the caller's responsibility to make sure the node doesn't > >> + * get destroyed before the caller can access it. > >> + */ > >> +struct drm_vma_offset_node *drm_vma_offset_lookup(struct drm_vma_offset_manager *mgr, > >> + unsigned long start, > >> + unsigned long pages) > >> +{ > >> + struct drm_vma_offset_node *node, *best; > >> + struct rb_node *iter; > >> + unsigned long offset; > >> + > >> + read_lock(&mgr->vm_lock); > >> + > >> + iter = mgr->vm_addr_space_rb.rb_node; > >> + best = NULL; > >> + > >> + while (likely(iter)) { > >> + node = rb_entry(iter, struct drm_vma_offset_node, vm_rb); > >> + offset = node->vm_node.start; > >> + if (start >= offset) { > >> + iter = iter->rb_right; > >> + best = node; > >> + if (start == offset) > >> + break; > >> + } else { > >> + iter = iter->rb_left; > >> + } > >> + } > >> + > >> + /* verify that the node spans the requested area */ > >> + if (likely(best)) { > >> + offset = best->vm_node.start + best->vm_pages; > >> + if (offset > start + pages) > >> + best = NULL; > >> + } > >> + > >> + read_unlock(&mgr->vm_lock); > >> + > >> + return best; > >> +} > >> +EXPORT_SYMBOL(drm_vma_offset_lookup); > >> + > >> +/* internal helper to link @node into the rb-tree */ > >> +static void _drm_vma_offset_add_rb(struct drm_vma_offset_manager *mgr, > >> + struct drm_vma_offset_node *node) > >> +{ > >> + struct rb_node **iter = &mgr->vm_addr_space_rb.rb_node; > >> + struct rb_node *parent = NULL; > >> + struct drm_vma_offset_node *iter_node; > >> + > >> + while (likely(*iter)) { > >> + parent = *iter; > >> + iter_node = rb_entry(*iter, struct drm_vma_offset_node, vm_rb); > >> + > >> + if (node->vm_node.start < iter_node->vm_node.start) > >> + iter = &(*iter)->rb_left; > >> + else if (node->vm_node.start > iter_node->vm_node.start) > >> + iter = &(*iter)->rb_right; > >> + else > >> + BUG(); > >> + } > >> + > >> + rb_link_node(&node->vm_rb, parent, iter); > >> + rb_insert_color(&node->vm_rb, &mgr->vm_addr_space_rb); > >> +} > >> + > >> +/** drm_vma_offset_add() > >> + * > >> + * Add a node to the offset-manager. If the node was already added, this does > >> + * nothing and return 0. @pages is the size of the object given in number of > >> + * pages. > >> + * After this call succeeds, you can access the offset of the node until it > >> + * is removed again. > >> + * > >> + * If this call fails, it is safe to retry the operation or call > >> + * drm_vma_offset_remove(), anyway. However, no cleanup is required in that > >> + * case. > >> + */ > >> +int drm_vma_offset_add(struct drm_vma_offset_manager *mgr, > >> + struct drm_vma_offset_node *node, unsigned long pages) > >> +{ > >> + int ret; > >> + > >> + write_lock(&mgr->vm_lock); > >> + > >> + if (unlikely(drm_mm_node_linked(&node->vm_node))) { > >> + ret = 0; > >> + goto out_unlock; > >> + } > >> + > >> + ret = drm_mm_insert_node_generic(&mgr->vm_addr_space_mm, > >> + &node->vm_node, pages, 0, 0); > > > > This allocates memory, so potentially blocks. This doesn't mesh well with > > the spinning rwlock (lockdep should seriously complain about this btw). > > Please use an rwsemaphore instead. > > Ugh? This shouldn't allocate any memory. I use pre-allocated nodes and > looking at drm_mm.c this just links the node at the correct position. > As long as the color_adjust() callbacks are atomic, this should be > fine here. Did you mix this up with get_block()? Oops, indeed I've mixed stuff up ;-) I'd still vote for an rwsem though since these codepaths shouldn't be performance critical and in pathalogical corner cases the O(n) list walking drm_mm does could result in quite awful long spinlock holding times. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch