[Intel-gfx] [PATCH] drm: Don't complain too much about struct_mutex.

[Daniel Vetter]

On Sat, Jul 15, 2017 at 12:02:47PM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2017-07-15 10:53:28)
> > For modern drivers the DRM core doesn't use struct_mutex at all, which
> > means it's defacto a driver-private lock. But since we still need it
> > for legacy drivers we can't initialize it in drivers, which means all
> > the different instances share one lockdep key. Despite that they might
> > be placed in totally different places in the locking hierarchy.
> > 
> > This results in a lot of bogus lockdep splats when running stuff on
> > systems with multiple gpus. Partially remedy the situation by only
> > doing might_lock checks on drivers that do use struct_mutex still for
> > gem locking.
> > 
> > A more complete solution would be to do the mutex_init in the drm core
> > only for legacy drivers, plus add it to each modern driver that still
> > needs it, which would also give each its own lockdep key. Trying to do
> > that dynamically doesn't work, because lockdep requires it's keys to
> > be statically allocated.
> > 
> > Cc: Hans de Goede <hdegoede at redhat.com>
> > Cc: Ben Skeggs <bskeggs at redhat.com>
> > Cc: Jiri Slaby <jirislaby at gmail.com>
> > Cc: Peter Zijlstra <peterz at infradead.org>
> > Cc: Ingo Molnar <mingo at redhat.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> But fwiw, the patch isn't wrong and fixes a false positive, so
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> >  drivers/gpu/drm/drm_gem.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 8dc11064253d..9663a79dd363 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -826,13 +826,15 @@ drm_gem_object_put_unlocked(struct drm_gem_object *obj)
> >                 return;
> >  
> >         dev = obj->dev;
> > -       might_lock(&dev->struct_mutex);
> >  
> >         if (dev->driver->gem_free_object_unlocked)
> 
> coding-style nit, if one branch needs {} they all need {}.
> (The real reason why I didn't want to move might_lock around in this
> function ;)

Fixed while applying, thanks for your review.
-Daniel
> 
> >                 kref_put(&obj->refcount, drm_gem_object_free);
> > -       else if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> > +       else {
> > +               might_lock(&dev->struct_mutex);
> > +               if (kref_put_mutex(&obj->refcount, drm_gem_object_free,
> >                                 &dev->struct_mutex))
> > -               mutex_unlock(&dev->struct_mutex);
> > +                       mutex_unlock(&dev->struct_mutex);
> > +       }
> >  }
> >  EXPORT_SYMBOL(drm_gem_object_put_unlocked);
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch