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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 01, 2013 at 09:54:17PM +0200, David Herrmann wrote:
> On Mon, Jul 1, 2013 at 9:42 PM, Daniel Vetter <daniel@xxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux