Message ID | 1462178451-1765-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, May 2, 2016 at 4:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > v4: > Add __drm_gem_object_unreference as a fastpath helper for drivers who > abolished dev->struct_mutex, requested by Chris. > > v5: Fix silly mistake in drm_gem_object_unreference_unlocked caught by > intel-gfx CI - I checked for gem_free_object instead of > gem_free_object_unlocked ... > > 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> (v3) > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v4) > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_gem.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++- > include/drm/drmP.h | 15 ++++++++++--- > include/drm/drm_gem.h | 48 +++++++++++++---------------------------- > 3 files changed, 80 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 25dac31eef37..973eb8805ce0 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -806,12 +806,64 @@ 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. > + * > + * See also __drm_gem_object_unreference(). > + */ > +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_unlocked) > + 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 > * @vma: VM area structure > * > 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..408d6c47d98b 100644 > --- a/include/drm/drm_gem.h > +++ b/include/drm/drm_gem.h > @@ -200,47 +200,29 @@ drm_gem_object_reference(struct drm_gem_object *obj) > } > > /** > - * drm_gem_object_unreference - release a GEM BO reference > + * __drm_gem_object_unreference - raw function to 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. > + * This function is meant to be used by drivers which are not encumbered with > + * dev->struct_mutex legacy locking and which are using the > + * gem_free_object_unlocked callback. It avoids all the locking checks and > + * locking overhead of drm_gem_object_unreference() and > + * drm_gem_object_unreference_unlocked(). > * > - * For drivers not encumbered with legacy locking use > - * drm_gem_object_unreference_unlocked() instead. > + * Drivers should never call this directly in their code. Instead they should > + * wrap it up into a driver_gem_object_unreference(struct driver_gem_object > + * *obj) wrapper function, and use that. Shared code should never call this, to > + * avoid breaking drivers by accident which still depend upon dev->struct_mutex > + * locking. > */ > static inline void > -drm_gem_object_unreference(struct drm_gem_object *obj) > +__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); > - } > + 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, > -- > 2.8.1 >
On Tue, May 03, 2016 at 11:59:19AM -0400, Alex Deucher wrote: > On Mon, May 2, 2016 at 4:40 AM, Daniel Vetter <daniel.vetter@ffwll.ch> 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. > > > > v4: > > Add __drm_gem_object_unreference as a fastpath helper for drivers who > > abolished dev->struct_mutex, requested by Chris. > > > > v5: Fix silly mistake in drm_gem_object_unreference_unlocked caught by > > intel-gfx CI - I checked for gem_free_object instead of > > gem_free_object_unlocked ... > > > > 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> (v3) > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> (v4) > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Alex Deucher <alexander.deucher@amd.com> Thanks for the review. I merged this one plus the driver patches acked by maintainers to drm-misc. -Daniel > > > --- > > drivers/gpu/drm/drm_gem.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++- > > include/drm/drmP.h | 15 ++++++++++--- > > include/drm/drm_gem.h | 48 +++++++++++++---------------------------- > > 3 files changed, 80 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 25dac31eef37..973eb8805ce0 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -806,12 +806,64 @@ 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. > > + * > > + * See also __drm_gem_object_unreference(). > > + */ > > +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_unlocked) > > + 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 > > * @vma: VM area structure > > * > > 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..408d6c47d98b 100644 > > --- a/include/drm/drm_gem.h > > +++ b/include/drm/drm_gem.h > > @@ -200,47 +200,29 @@ drm_gem_object_reference(struct drm_gem_object *obj) > > } > > > > /** > > - * drm_gem_object_unreference - release a GEM BO reference > > + * __drm_gem_object_unreference - raw function to 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. > > + * This function is meant to be used by drivers which are not encumbered with > > + * dev->struct_mutex legacy locking and which are using the > > + * gem_free_object_unlocked callback. It avoids all the locking checks and > > + * locking overhead of drm_gem_object_unreference() and > > + * drm_gem_object_unreference_unlocked(). > > * > > - * For drivers not encumbered with legacy locking use > > - * drm_gem_object_unreference_unlocked() instead. > > + * Drivers should never call this directly in their code. Instead they should > > + * wrap it up into a driver_gem_object_unreference(struct driver_gem_object > > + * *obj) wrapper function, and use that. Shared code should never call this, to > > + * avoid breaking drivers by accident which still depend upon dev->struct_mutex > > + * locking. > > */ > > static inline void > > -drm_gem_object_unreference(struct drm_gem_object *obj) > > +__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); > > - } > > + 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, > > -- > > 2.8.1 > >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..973eb8805ce0 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -806,12 +806,64 @@ 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. + * + * See also __drm_gem_object_unreference(). + */ +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_unlocked) + 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 * @vma: VM area structure * 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..408d6c47d98b 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -200,47 +200,29 @@ drm_gem_object_reference(struct drm_gem_object *obj) } /** - * drm_gem_object_unreference - release a GEM BO reference + * __drm_gem_object_unreference - raw function to 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. + * This function is meant to be used by drivers which are not encumbered with + * dev->struct_mutex legacy locking and which are using the + * gem_free_object_unlocked callback. It avoids all the locking checks and + * locking overhead of drm_gem_object_unreference() and + * drm_gem_object_unreference_unlocked(). * - * For drivers not encumbered with legacy locking use - * drm_gem_object_unreference_unlocked() instead. + * Drivers should never call this directly in their code. Instead they should + * wrap it up into a driver_gem_object_unreference(struct driver_gem_object + * *obj) wrapper function, and use that. Shared code should never call this, to + * avoid breaking drivers by accident which still depend upon dev->struct_mutex + * locking. */ static inline void -drm_gem_object_unreference(struct drm_gem_object *obj) +__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); - } + 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,