On Thu, Oct 15, 2015 at 01:11:36PM +0200, Daniel Vetter wrote:
> On Thu, Oct 15, 2015 at 11:06:59AM +0100, Chris Wilson wrote:
> > On Thu, Oct 15, 2015 at 11:33:43AM +0200, Daniel Vetter wrote:
> > > Compared to wrapping the final kref_put with dev->struct_mutex this
> > > allows us to only acquire the offset manager look both in the final
> > > cleanup and in the lookup. Which has the upside that no locks leak out
> > > of the core abstractions. But it means that we need to hold a
> > > temporary reference to the object while checking mmap constraints, to
> > > make sure the object doesn't disappear. Extended the critical region
> > > would have worked too, but would result in more leaky locking.
> > >
> > > Also, this is the final bit which required dev->struct_mutex in gem
> > > core, now modern drivers can be completely struct_mutex free!
> > >
> > > This needs a new drm_vma_offset_exact_lookup_locked and makes both
> > > drm_vma_offset_exact_lookup and drm_vma_offset_lookup unused.
> > >
> > > v2: Don't leak object references in failure paths (David).
> > >
> > > Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> > > Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/drm_gem.c | 30 +++++++++++++++++------------
> > > drivers/gpu/drm/drm_vma_manager.c | 40 ++++++++++++---------------------------
> > > include/drm/drm_vma_manager.h | 22 ++++++---------------
> > > 3 files changed, 36 insertions(+), 56 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index ab8ea42264f4..663fadbe979e 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -862,30 +862,36 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> > > {
> > > struct drm_file *priv = filp->private_data;
> > > struct drm_device *dev = priv->minor->dev;
> > > - struct drm_gem_object *obj;
> > > + struct drm_gem_object *obj = NULL;
> > > struct drm_vma_offset_node *node;
> > > int ret;
> > >
> > > if (drm_device_is_unplugged(dev))
> > > return -ENODEV;
> > >
> >
> > /* bla bla bla
> > * When the object is being freed, after it hits 0-refcnt
> > * it acquires the struct_mutex and proceeds to tear down
> > * the object. In the process it will attempt to remove
> > * the VMA offset and so acquire this mgr->vm_lock.
> > * Therefore if we find an object with a 0-refcnt that matches
> > * our range, we know it is in the process of being destroyed
> > * and will be freed as soon as we release the lock - so
> > * we have to check for the 0-refcnted object and treat it as
> > * invalid.
> > */
>
> As discussed on irc struct_mutex is just a digression, so I left that part
> out when adding your comment.
>
> > > - mutex_lock(&dev->struct_mutex);
> > > + drm_vma_offset_lock_lookup(dev->vma_offset_manager);
> > > + node = drm_vma_offset_exact_lookup_locked(dev->vma_offset_manager,
> > > + vma->vm_pgoff,
> > > + vma_pages(vma));
> > > + if (likely(node)) {
> > > + obj = container_of(node, struct drm_gem_object, vma_node);
> > > + if (!kref_get_unless_zero(&obj->refcount))
> > > + obj = NULL;
> > > + }
> > > + drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> >
> > Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>
> All four core gem patches now merged in drm-misc, thanks for the review.
Argh, last patch needs one of the vgem ones as prep, so dropped it again.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
