| Message ID | 1461691808-12414-10-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 26, 2016 at 07:29:42PM +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. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > include/drm/drmP.h | 12 ++++++++- > include/drm/drm_gem.h | 45 ++------------------------------- > 3 files changed, 65 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 25dac31eef37..8f2eff448bb5 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 != NULL) > + dev->driver->gem_free_object_unlocked(obj); > + else if (dev->driver->gem_free_object != NULL) > 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) > +{ I have i915.ko in a state where we could use this as well. I would like to have our inlines back though. I would add static inline void i915_gem_object_put(struct drm_i915_gem_object *obj) { kref_put(&obj->base.refcount, drm_gem_object_free); } Hmm, considering how simple that is, maybe I won't ask for static inline void __drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) { BUG_ON(obj->dev->driver->gem_free_object == NULL); kref_put(&obj->refcount, drm_gem_object_free); } after all! -Chris
On Tue, Apr 26, 2016 at 08:47:32PM +0100, Chris Wilson wrote: > On Tue, Apr 26, 2016 at 07:29:42PM +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. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > include/drm/drmP.h | 12 ++++++++- > > include/drm/drm_gem.h | 45 ++------------------------------- > > 3 files changed, 65 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 25dac31eef37..8f2eff448bb5 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 != NULL) > > + dev->driver->gem_free_object_unlocked(obj); > > + else if (dev->driver->gem_free_object != NULL) > > 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) > > +{ > > I have i915.ko in a state where we could use this as well. I would like > to have our inlines back though. I would add > > static inline void > i915_gem_object_put(struct drm_i915_gem_object *obj) > { > kref_put(&obj->base.refcount, drm_gem_object_free); > } > > Hmm, considering how simple that is, maybe I won't ask for > > static inline void > __drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) > { > BUG_ON(obj->dev->driver->gem_free_object == NULL); > kref_put(&obj->refcount, drm_gem_object_free); > } Yeah, something like that makes sense. Imo the core versions really need all the silly locking checks and all that to make sure no one abuses them by accident, or breaks a driver still using struct_mutex. Adding a fastpath for drivers using gem_free_object_unlocked just wrapping the kref_put certainly looks like a good idea. And if we document the exact use case I think we could even nuke the BUG_ON too. This can be audited with a simple $ git grep __drm_gem_object_unreference_unlocked -- drivers/gpu/drm/$driver so imo nothing too dangerous. Ack/r-b on the patch itself? -Daniel
On Tue, Apr 26, 2016 at 10:10:14PM +0200, Daniel Vetter wrote: > On Tue, Apr 26, 2016 at 08:47:32PM +0100, Chris Wilson wrote: > > On Tue, Apr 26, 2016 at 07:29:42PM +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. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > > include/drm/drmP.h | 12 ++++++++- > > > include/drm/drm_gem.h | 45 ++------------------------------- > > > 3 files changed, 65 insertions(+), 56 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > > index 25dac31eef37..8f2eff448bb5 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 != NULL) > > > + dev->driver->gem_free_object_unlocked(obj); > > > + else if (dev->driver->gem_free_object != NULL) > > > 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) > > > +{ > > > > I have i915.ko in a state where we could use this as well. I would like > > to have our inlines back though. I would add > > > > static inline void > > i915_gem_object_put(struct drm_i915_gem_object *obj) > > { > > kref_put(&obj->base.refcount, drm_gem_object_free); > > } > > > > Hmm, considering how simple that is, maybe I won't ask for > > > > static inline void > > __drm_gem_object_unreference_unlocked(struct drm_gem_object *obj) > > { > > BUG_ON(obj->dev->driver->gem_free_object == NULL); > > kref_put(&obj->refcount, drm_gem_object_free); > > } > > Yeah, something like that makes sense. Imo the core versions really need > all the silly locking checks and all that to make sure no one abuses them > by accident, or breaks a driver still using struct_mutex. Adding a > fastpath for drivers using gem_free_object_unlocked just wrapping the > kref_put certainly looks like a good idea. > > And if we document the exact use case I think we could even nuke the > BUG_ON too. This can be audited with a simple > > $ git grep __drm_gem_object_unreference_unlocked -- drivers/gpu/drm/$driver > > so imo nothing too dangerous. > > Ack/r-b on the patch itself? Only an ack, I'm afraid. I haven't considered the ramifictions to know if the intermediate steps are going to be painful (in some cases we'll have an all a new hot function in our profiles, but overall there'll be little change I hope). The end goal is definitely an improvement. I've talked myself into having a better read of the core patches... -Chris
On Tue, Apr 26, 2016 at 1:29 PM, 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. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> This and the rest of the series are: Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > include/drm/drmP.h | 12 ++++++++- > include/drm/drm_gem.h | 45 ++------------------------------- > 3 files changed, 65 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 25dac31eef37..8f2eff448bb5 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 != NULL) > + dev->driver->gem_free_object_unlocked(obj); > + else if (dev->driver->gem_free_object != NULL) > 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 != NULL) > + 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 != NULL) { > + 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..7e30b3d2b25c 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -583,9 +583,19 @@ struct drm_driver { > * Driver-specific constructor for drm_gem_objects, to set up > * obj->driver_private. > * > - * 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); > + > + /** > + * Driver-specific constructor for drm_gem_objects, to set up > + * obj->driver_private. 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, > -- > 2.8.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Apr 26, 2016 at 05:52:31PM -0400, Alex Deucher wrote: > On Tue, Apr 26, 2016 at 1:29 PM, 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. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > This and the rest of the series are: > Acked-by: Alex Deucher <alexander.deucher@amd.com> Before I pull this in, I'd like at least an r-b on this and one of the driver patches. t-b even better. -Daniel > > > --- > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > include/drm/drmP.h | 12 ++++++++- > > include/drm/drm_gem.h | 45 ++------------------------------- > > 3 files changed, 65 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 25dac31eef37..8f2eff448bb5 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 != NULL) > > + dev->driver->gem_free_object_unlocked(obj); > > + else if (dev->driver->gem_free_object != NULL) > > 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 != NULL) > > + 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 != NULL) { > > + 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..7e30b3d2b25c 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -583,9 +583,19 @@ struct drm_driver { > > * Driver-specific constructor for drm_gem_objects, to set up > > * obj->driver_private. > > * > > - * 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); > > + > > + /** > > + * Driver-specific constructor for drm_gem_objects, to set up > > + * obj->driver_private. 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, > > -- > > 2.8.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am Dienstag, den 26.04.2016, 19:29 +0200 schrieb Daniel Vetter: > 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. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Two mostly cosmetic comments below, otherwise looks good: Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > --- > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > include/drm/drmP.h | 12 ++++++++- > include/drm/drm_gem.h | 45 ++------------------------------- > 3 files changed, 65 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 25dac31eef37..8f2eff448bb5 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 != NULL) Why those explicit != NULL checks? The style mostly used in this file is to omit those, like "if (dev->driver->gem_free_object_unlocked)". Same comment applies to several hunks of this patch. > + dev->driver->gem_free_object_unlocked(obj); > + else if (dev->driver->gem_free_object != NULL) > 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 != NULL) > + 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 != NULL) { > + 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..7e30b3d2b25c 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -583,9 +583,19 @@ struct drm_driver { > * Driver-specific constructor for drm_gem_objects, to set up > * obj->driver_private. > * > - * 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); > + > + /** > + * Driver-specific constructor for drm_gem_objects, to set up > + * obj->driver_private. This part of the comment is really off. I see it's just copy&paste from the comment above gem_free_object, but I think it should have been placed above gem_open_object and has nothing to do with the free* callbacks. > 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,
On Wed, Apr 27, 2016 at 11:31:00AM +0200, Lucas Stach wrote: > Am Dienstag, den 26.04.2016, 19:29 +0200 schrieb Daniel Vetter: > > 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. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Two mostly cosmetic comments below, otherwise looks good: > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de> > > > --- > > drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- > > include/drm/drmP.h | 12 ++++++++- > > include/drm/drm_gem.h | 45 ++------------------------------- > > 3 files changed, 65 insertions(+), 56 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 25dac31eef37..8f2eff448bb5 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 != NULL) > > Why those explicit != NULL checks? The style mostly used in this file is > to omit those, like "if (dev->driver->gem_free_object_unlocked)". Same > comment applies to several hunks of this patch. I move some of these functions around, the != NULL checks where there already. I can easily change all the ones I touch. > > + dev->driver->gem_free_object_unlocked(obj); > > + else if (dev->driver->gem_free_object != NULL) > > 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 != NULL) > > + 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 != NULL) { > > + 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..7e30b3d2b25c 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -583,9 +583,19 @@ struct drm_driver { > > * Driver-specific constructor for drm_gem_objects, to set up > > * obj->driver_private. > > * > > - * 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); > > + > > + /** > > + * Driver-specific constructor for drm_gem_objects, to set up > > + * obj->driver_private. > > This part of the comment is really off. I see it's just copy&paste from > the comment above gem_free_object, but I think it should have been > placed above gem_open_object and has nothing to do with the free* > callbacks. Oops, wanted to add proper kerneldoc for this new hook and the old one to explain the differences, and totally failed at that. Will fix up. -Daniel > > > 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, > >
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 25dac31eef37..8f2eff448bb5 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 != NULL) + dev->driver->gem_free_object_unlocked(obj); + else if (dev->driver->gem_free_object != NULL) 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 != NULL) + 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 != NULL) { + 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..7e30b3d2b25c 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -583,9 +583,19 @@ struct drm_driver { * Driver-specific constructor for drm_gem_objects, to set up * obj->driver_private. * - * 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); + + /** + * Driver-specific constructor for drm_gem_objects, to set up + * obj->driver_private. 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,
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. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_gem.c | 64 ++++++++++++++++++++++++++++++++++++++--------- include/drm/drmP.h | 12 ++++++++- include/drm/drm_gem.h | 45 ++------------------------------- 3 files changed, 65 insertions(+), 56 deletions(-)