| Message ID | 20161114085703.16540-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 14/11/2016 08:56, Chris Wilson wrote: > Localise the static struct lock_class_key to the caller of > i915_sw_fence_init() so that we create a lock_class instance for each > unique sw_fence rather than all sw_fences sharing the same > lock_class. This eliminate some lockdep false positive when using fences > from within fence callbacks. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> This one had Joonas' r-b. Regads, Tvrtko > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 7 +++++-- > drivers/gpu/drm/i915/i915_sw_fence.h | 11 ++++++++++- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 95f2f12e0917..147420ccf49c 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -116,11 +116,14 @@ static void i915_sw_fence_await(struct i915_sw_fence *fence) > WARN_ON(atomic_inc_return(&fence->pending) <= 1); > } > > -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn) > +void __i915_sw_fence_init(struct i915_sw_fence *fence, > + i915_sw_fence_notify_t fn, > + const char *name, > + struct lock_class_key *key) > { > BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); > > - init_waitqueue_head(&fence->wait); > + __init_waitqueue_head(&fence->wait, name, key); > kref_init(&fence->kref); > atomic_set(&fence->pending, 1); > fence->flags = (unsigned long)fn; > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h > index 707dfc4f0da5..a5546eb2b5cd 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.h > +++ b/drivers/gpu/drm/i915/i915_sw_fence.h > @@ -40,7 +40,16 @@ typedef int (*i915_sw_fence_notify_t)(struct i915_sw_fence *, > enum i915_sw_fence_notify state); > #define __i915_sw_fence_call __aligned(4) > > -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn); > +void __i915_sw_fence_init(struct i915_sw_fence *fence, > + i915_sw_fence_notify_t fn, > + const char *name, > + struct lock_class_key *key); > +#define i915_sw_fence_init(fence, fn) do { \ > + static struct lock_class_key __key; \ > + \ > + __i915_sw_fence_init((fence), fn, #fence, &__key); \ > +} while (0) > + > void i915_sw_fence_commit(struct i915_sw_fence *fence); > > int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, >
On Mon, Nov 14, 2016 at 09:01:00AM +0000, Tvrtko Ursulin wrote: > > On 14/11/2016 08:56, Chris Wilson wrote: > >Localise the static struct lock_class_key to the caller of > >i915_sw_fence_init() so that we create a lock_class instance for each > >unique sw_fence rather than all sw_fences sharing the same > >lock_class. This eliminate some lockdep false positive when using fences > >from within fence callbacks. > > > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > This one had Joonas' r-b. No, this is a new patch. Tackling the same issue of nested annotations but this time adding the classes to the fence not to the timeline (which is the next patch). This is the difference between v2 and v3, and kills that remining nasty spin_lock_irqsave_nested() from v2. -Chris
On 14/11/2016 08:56, Chris Wilson wrote: > Localise the static struct lock_class_key to the caller of > i915_sw_fence_init() so that we create a lock_class instance for each > unique sw_fence rather than all sw_fences sharing the same > lock_class. This eliminate some lockdep false positive when using fences > from within fence callbacks. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_sw_fence.c | 7 +++++-- > drivers/gpu/drm/i915/i915_sw_fence.h | 11 ++++++++++- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c > index 95f2f12e0917..147420ccf49c 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.c > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c > @@ -116,11 +116,14 @@ static void i915_sw_fence_await(struct i915_sw_fence *fence) > WARN_ON(atomic_inc_return(&fence->pending) <= 1); > } > > -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn) > +void __i915_sw_fence_init(struct i915_sw_fence *fence, > + i915_sw_fence_notify_t fn, > + const char *name, > + struct lock_class_key *key) > { > BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); > > - init_waitqueue_head(&fence->wait); > + __init_waitqueue_head(&fence->wait, name, key); > kref_init(&fence->kref); > atomic_set(&fence->pending, 1); > fence->flags = (unsigned long)fn; > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h > index 707dfc4f0da5..a5546eb2b5cd 100644 > --- a/drivers/gpu/drm/i915/i915_sw_fence.h > +++ b/drivers/gpu/drm/i915/i915_sw_fence.h > @@ -40,7 +40,16 @@ typedef int (*i915_sw_fence_notify_t)(struct i915_sw_fence *, > enum i915_sw_fence_notify state); > #define __i915_sw_fence_call __aligned(4) > > -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn); > +void __i915_sw_fence_init(struct i915_sw_fence *fence, > + i915_sw_fence_notify_t fn, > + const char *name, > + struct lock_class_key *key); > +#define i915_sw_fence_init(fence, fn) do { \ > + static struct lock_class_key __key; \ > + \ > + __i915_sw_fence_init((fence), fn, #fence, &__key); \ (fn) ? > +} while (0) > + > void i915_sw_fence_commit(struct i915_sw_fence *fence); > > int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence, > Looks good to me. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On ma, 2016-11-14 at 08:56 +0000, Chris Wilson wrote: > Localise the static struct lock_class_key to the caller of > i915_sw_fence_init() so that we create a lock_class instance for each > unique sw_fence rather than all sw_fences sharing the same > lock_class. This eliminate some lockdep false positive when using fences > from within fence callbacks. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> <SNIP> > @@ -40,7 +40,16 @@ typedef int (*i915_sw_fence_notify_t)(struct i915_sw_fence *, > enum i915_sw_fence_notify state); > #define __i915_sw_fence_call __aligned(4) > > -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn); > +void __i915_sw_fence_init(struct i915_sw_fence *fence, > + i915_sw_fence_notify_t fn, > + const char *name, > + struct lock_class_key *key); > +#define i915_sw_fence_init(fence, fn) do { \ Gimme a (line) break here. > + static struct lock_class_key __key; \ When lockdep is disabled, this becomes zero size. We might still get rid of the #fence strings, with some #ifdef, did you measure the impact? I remember some for_each_engine_masked cry over bytes. > + \ > + __i915_sw_fence_init((fence), fn, #fence, &__key); \ > +} while (0) > + Above addressed, and assuming we're not compiling in extra; Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Regards, Joonas
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c b/drivers/gpu/drm/i915/i915_sw_fence.c index 95f2f12e0917..147420ccf49c 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/i915_sw_fence.c @@ -116,11 +116,14 @@ static void i915_sw_fence_await(struct i915_sw_fence *fence) WARN_ON(atomic_inc_return(&fence->pending) <= 1); } -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn) +void __i915_sw_fence_init(struct i915_sw_fence *fence, + i915_sw_fence_notify_t fn, + const char *name, + struct lock_class_key *key) { BUG_ON((unsigned long)fn & ~I915_SW_FENCE_MASK); - init_waitqueue_head(&fence->wait); + __init_waitqueue_head(&fence->wait, name, key); kref_init(&fence->kref); atomic_set(&fence->pending, 1); fence->flags = (unsigned long)fn; diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h b/drivers/gpu/drm/i915/i915_sw_fence.h index 707dfc4f0da5..a5546eb2b5cd 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence.h +++ b/drivers/gpu/drm/i915/i915_sw_fence.h @@ -40,7 +40,16 @@ typedef int (*i915_sw_fence_notify_t)(struct i915_sw_fence *, enum i915_sw_fence_notify state); #define __i915_sw_fence_call __aligned(4) -void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn); +void __i915_sw_fence_init(struct i915_sw_fence *fence, + i915_sw_fence_notify_t fn, + const char *name, + struct lock_class_key *key); +#define i915_sw_fence_init(fence, fn) do { \ + static struct lock_class_key __key; \ + \ + __i915_sw_fence_init((fence), fn, #fence, &__key); \ +} while (0) + void i915_sw_fence_commit(struct i915_sw_fence *fence); int i915_sw_fence_await_sw_fence(struct i915_sw_fence *fence,
Localise the static struct lock_class_key to the caller of i915_sw_fence_init() so that we create a lock_class instance for each unique sw_fence rather than all sw_fences sharing the same lock_class. This eliminate some lockdep false positive when using fences from within fence callbacks. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_sw_fence.c | 7 +++++-- drivers/gpu/drm/i915/i915_sw_fence.h | 11 ++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-)