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