Message ID | 1461757800-29086-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Wed, Apr 27, 2016 at 01:50:00PM +0200, Daniel Vetter wrote: > Finally all the core gem and a lot of drivers are entirely free of > dev->struct_mutex depencies, and we can start to have an entirely > lockless unref path. > > To make sure that no one who touches the core code accidentally breaks > existing drivers which still require dev->struct_mutex I've made the > might_lock check unconditional. > > While at it de-inline the ref/unref functions, they've become a bit > too big. > > v2: Make it not leak like a sieve. > > v3: Review from Lucas: > - drop != NULL in pointer checks. > - fixup copypasted kerneldoc to actually match the functions. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Alex Deucher <alexdeucher@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > include/drm/drmP.h | 15 ++++++++--- > include/drm/drm_gem.h | 45 ++------------------------------- > 3 files changed, 66 insertions(+), 58 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 25dac31eef37..a4684a306c48 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) > } > EXPORT_SYMBOL(drm_gem_object_release); > > -/** > - * drm_gem_object_free - free a GEM object > - * @kref: kref of the object to free > - * > - * Called after the last reference to the object has been lost. > - * Must be called holding struct_ mutex > - * > - * Frees the object > - */ > -void > +static void > drm_gem_object_free(struct kref *kref) > { > struct drm_gem_object *obj = > @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref) > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > - if (dev->driver->gem_free_object != NULL) > + if (dev->driver->gem_free_object_unlocked) > + dev->driver->gem_free_object_unlocked(obj); > + else if (dev->driver->gem_free_object) > dev->driver->gem_free_object(obj); > } > -EXPORT_SYMBOL(drm_gem_object_free); With my plan, I want to keep drm_gem_object_free exported. The plan being a __drm_gem_object_free_lockless fastpath. -Chris
On Wed, Apr 27, 2016 at 12:58:51PM +0100, Chris Wilson wrote: > On Wed, Apr 27, 2016 at 01:50:00PM +0200, Daniel Vetter wrote: > > Finally all the core gem and a lot of drivers are entirely free of > > dev->struct_mutex depencies, and we can start to have an entirely > > lockless unref path. > > > > To make sure that no one who touches the core code accidentally breaks > > existing drivers which still require dev->struct_mutex I've made the > > might_lock check unconditional. > > > > While at it de-inline the ref/unref functions, they've become a bit > > too big. > > > > v2: Make it not leak like a sieve. > > > > v3: Review from Lucas: > > - drop != NULL in pointer checks. > > - fixup copypasted kerneldoc to actually match the functions. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Alex Deucher <alexdeucher@gmail.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > include/drm/drmP.h | 15 ++++++++--- > > include/drm/drm_gem.h | 45 ++------------------------------- > > 3 files changed, 66 insertions(+), 58 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 25dac31eef37..a4684a306c48 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) > > } > > EXPORT_SYMBOL(drm_gem_object_release); > > > > -/** > > - * drm_gem_object_free - free a GEM object > > - * @kref: kref of the object to free > > - * > > - * Called after the last reference to the object has been lost. > > - * Must be called holding struct_ mutex > > - * > > - * Frees the object > > - */ > > -void > > +static void > > drm_gem_object_free(struct kref *kref) > > { > > struct drm_gem_object *obj = > > @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref) > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > > - if (dev->driver->gem_free_object != NULL) > > + if (dev->driver->gem_free_object_unlocked) > > + dev->driver->gem_free_object_unlocked(obj); > > + else if (dev->driver->gem_free_object) > > dev->driver->gem_free_object(obj); > > } > > -EXPORT_SYMBOL(drm_gem_object_free); > > With my plan, I want to keep drm_gem_object_free exported. > The plan being a __drm_gem_object_free_lockless fastpath. Yeah, but I think we can reexport again in the patch that adds __drm_gem_object_free_lockless. Just not much personally a friend of sprawling EXPORT_SYMBOL when not needed. But I can drop that part too if you feel strongly. -Daniel
On Wed, Apr 27, 2016 at 02:12:46PM +0200, Daniel Vetter wrote: > On Wed, Apr 27, 2016 at 12:58:51PM +0100, Chris Wilson wrote: > > On Wed, Apr 27, 2016 at 01:50:00PM +0200, Daniel Vetter wrote: > > > Finally all the core gem and a lot of drivers are entirely free of > > > dev->struct_mutex depencies, and we can start to have an entirely > > > lockless unref path. > > > > > > To make sure that no one who touches the core code accidentally breaks > > > existing drivers which still require dev->struct_mutex I've made the > > > might_lock check unconditional. > > > > > > While at it de-inline the ref/unref functions, they've become a bit > > > too big. > > > > > > v2: Make it not leak like a sieve. > > > > > > v3: Review from Lucas: > > > - drop != NULL in pointer checks. > > > - fixup copypasted kerneldoc to actually match the functions. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Alex Deucher <alexdeucher@gmail.com> > > > Cc: Lucas Stach <l.stach@pengutronix.de> > > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > > include/drm/drmP.h | 15 ++++++++--- > > > include/drm/drm_gem.h | 45 ++------------------------------- > > > 3 files changed, 66 insertions(+), 58 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > > index 25dac31eef37..a4684a306c48 100644 > > > --- a/drivers/gpu/drm/drm_gem.c > > > +++ b/drivers/gpu/drm/drm_gem.c > > > @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) > > > } > > > EXPORT_SYMBOL(drm_gem_object_release); > > > > > > -/** > > > - * drm_gem_object_free - free a GEM object > > > - * @kref: kref of the object to free > > > - * > > > - * Called after the last reference to the object has been lost. > > > - * Must be called holding struct_ mutex > > > - * > > > - * Frees the object > > > - */ > > > -void > > > +static void > > > drm_gem_object_free(struct kref *kref) > > > { > > > struct drm_gem_object *obj = > > > @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref) > > > > > > WARN_ON(!mutex_is_locked(&dev->struct_mutex)); > > > > > > - if (dev->driver->gem_free_object != NULL) > > > + if (dev->driver->gem_free_object_unlocked) > > > + dev->driver->gem_free_object_unlocked(obj); > > > + else if (dev->driver->gem_free_object) > > > dev->driver->gem_free_object(obj); > > > } > > > -EXPORT_SYMBOL(drm_gem_object_free); > > > > With my plan, I want to keep drm_gem_object_free exported. > > The plan being a __drm_gem_object_free_lockless fastpath. > > Yeah, but I think we can reexport again in the patch that adds > __drm_gem_object_free_lockless. Just not much personally a friend of > sprawling EXPORT_SYMBOL when not needed. But I can drop that part too if > you feel strongly. The alternative is to add the function now and then I can just drop the support into my tree and start testing :) -Chris
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..a4684a306c48 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -788,16 +788,7 @@ drm_gem_object_release(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_object_release); -/** - * drm_gem_object_free - free a GEM object - * @kref: kref of the object to free - * - * Called after the last reference to the object has been lost. - * Must be called holding struct_ mutex - * - * Frees the object - */ -void +static void drm_gem_object_free(struct kref *kref) { struct drm_gem_object *obj = @@ -806,10 +797,59 @@ drm_gem_object_free(struct kref *kref) WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - if (dev->driver->gem_free_object != NULL) + if (dev->driver->gem_free_object_unlocked) + dev->driver->gem_free_object_unlocked(obj); + else if (dev->driver->gem_free_object) dev->driver->gem_free_object(obj); } -EXPORT_SYMBOL(drm_gem_object_free); + +/** + * drm_gem_object_unreference_unlocked - release a GEM BO reference + * @obj: GEM buffer object + * + * This releases a reference to @obj. Callers must not hold the + * dev->struct_mutex lock when calling this function. + */ +void +drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev; + + if (!obj) + return; + + dev = obj->dev; + might_lock(&dev->struct_mutex); + + if (dev->driver->gem_free_object) + kref_put(&obj->refcount, drm_gem_object_free); + else if (kref_put_mutex(&obj->refcount, drm_gem_object_free, + &dev->struct_mutex)) + mutex_unlock(&dev->struct_mutex); +} +EXPORT_SYMBOL(drm_gem_object_unreference_unlocked); + +/** + * drm_gem_object_unreference - release a GEM BO reference + * @obj: GEM buffer object + * + * This releases a reference to @obj. Callers must hold the dev->struct_mutex + * lock when calling this function, even when the driver doesn't use + * dev->struct_mutex for anything. + * + * For drivers not encumbered with legacy locking use + * drm_gem_object_unreference_unlocked() instead. + */ +void +drm_gem_object_unreference(struct drm_gem_object *obj) +{ + if (obj) { + WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); + + kref_put(&obj->refcount, drm_gem_object_free); + } +} +EXPORT_SYMBOL(drm_gem_object_unreference); /** * drm_gem_vm_open - vma->ops->open implementation for GEM diff --git a/include/drm/drmP.h b/include/drm/drmP.h index c81dd2250fc6..bd7b262d7af0 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -580,12 +580,21 @@ struct drm_driver { void (*debugfs_cleanup)(struct drm_minor *minor); /** - * Driver-specific constructor for drm_gem_objects, to set up - * obj->driver_private. + * @gem_free_object: deconstructor for drm_gem_objects * - * Returns 0 on success. + * This is deprecated and should not be used by new drivers. Use + * @gem_free_object_unlocked instead. */ void (*gem_free_object) (struct drm_gem_object *obj); + + /** + * @gem_free_object_unlocked: deconstructor for drm_gem_objects + * + * This is for drivers which are not encumbered with dev->struct_mutex + * legacy locking schemes. Use this hook instead of @gem_free_object. + */ + void (*gem_free_object_unlocked) (struct drm_gem_object *obj); + int (*gem_open_object) (struct drm_gem_object *, struct drm_file *); void (*gem_close_object) (struct drm_gem_object *, struct drm_file *); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 0b3e11ab8757..ae1c7f18eec0 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -175,7 +175,6 @@ struct drm_gem_object { }; void drm_gem_object_release(struct drm_gem_object *obj); -void drm_gem_object_free(struct kref *kref); int drm_gem_object_init(struct drm_device *dev, struct drm_gem_object *obj, size_t size); void drm_gem_private_object_init(struct drm_device *dev, @@ -199,48 +198,8 @@ drm_gem_object_reference(struct drm_gem_object *obj) kref_get(&obj->refcount); } -/** - * drm_gem_object_unreference - release a GEM BO reference - * @obj: GEM buffer object - * - * This releases a reference to @obj. Callers must hold the dev->struct_mutex - * lock when calling this function, even when the driver doesn't use - * dev->struct_mutex for anything. - * - * For drivers not encumbered with legacy locking use - * drm_gem_object_unreference_unlocked() instead. - */ -static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) -{ - if (obj != NULL) { - WARN_ON(!mutex_is_locked(&obj->dev->struct_mutex)); - - kref_put(&obj->refcount, drm_gem_object_free); - } -} - -/** - * drm_gem_object_unreference_unlocked - release a GEM BO reference - * @obj: GEM buffer object - * - * This releases a reference to @obj. Callers must not hold the - * dev->struct_mutex lock when calling this function. - */ -static inline void -drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) -{ - struct drm_device *dev; - - if (!obj) - return; - - dev = obj->dev; - if (kref_put_mutex(&obj->refcount, drm_gem_object_free, &dev->struct_mutex)) - mutex_unlock(&dev->struct_mutex); - else - might_lock(&dev->struct_mutex); -} +void drm_gem_object_unreference_unlocked(struct drm_gem_object *obj); +void drm_gem_object_unreference(struct drm_gem_object *obj); int drm_gem_handle_create(struct drm_file *file_priv, struct drm_gem_object *obj,