Re: [Intel-gfx] [PATCH v3 01/14] drm/i915: Give each sw_fence its own lockclass

[Chris Wilson]

On Mon, Nov 14, 2016 at 04:48:00PM +0200, Joonas Lahtinen wrote:
> On ma, 2016-11-14 at 08:56 +, Chris Wilson wrote:
> > +   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.

I was copying mutex_init. To avoid it is not just a little ifdeffery :|

The strings currently cost us around 160 bytes of text.

   textdata bss dec hex filename
12225245077 608 1228209  12bdb1 drivers/gpu/drm/i915/i915.ko
12223645077 608 1228049  12bd11 drivers/gpu/drm/i915/i915.ko

diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c 
b/drivers/gpu/drm/i915/i915_sw_fence.c
index 65fded24a9eb..804af5766650 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.c
+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
@@ -110,6 +110,9 @@ static void i915_sw_fence_await(struct i915_sw_fence *fence)
WARN_ON(atomic_inc_return(>pending) <= 1);
 }
 
+#ifndef CONFIG_LOCKDEP
+static
+#endif
 void __i915_sw_fence_init(struct i915_sw_fence *fence,
  i915_sw_fence_notify_t fn,
  const char *name,
@@ -123,6 +126,16 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
fence->flags = (unsigned long)fn;
 }
 
+#ifndef CONFIG_LOCKDEP
+void i915_sw_fence_init(struct i915_sw_fence *fence,
+   i915_sw_fence_notify_t fn)
+{
+   static struct lock_class_key __key;
+
+   __i915_sw_fence_init(fence, fn, NULL, &__key);
+}
+#endif
+
 void i915_sw_fence_commit(struct i915_sw_fence *fence)
 {
i915_sw_fence_complete(fence);
diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h 
b/drivers/gpu/drm/i915/i915_sw_fence.h
index 23748a1ae6ae..d8510a4b02bd 100644
--- a/drivers/gpu/drm/i915/i915_sw_fence.h
+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
@@ -40,6 +40,7 @@ typedef int (*i915_sw_fence_notify_t)(struct i915_sw_fence *,
  enum i915_sw_fence_notify state);
 #define __i915_sw_fence_call __aligned(4)
 
+#ifdef CONFIG_LOCKDEP
 void __i915_sw_fence_init(struct i915_sw_fence *fence,
  i915_sw_fence_notify_t fn,
  const char *name,
@@ -49,6 +50,10 @@ void __i915_sw_fence_init(struct i915_sw_fence *fence,
\
__i915_sw_fence_init((fence), (fn), #fence, &__key);\
 } while (0)
+#else
+void i915_sw_fence_init(struct i915_sw_fence *fence,
+   i915_sw_fence_notify_t fn);
+#endif
 
 void i915_sw_fence_commit(struct i915_sw_fence *fence);
 

Can we do that more neatly?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 01/14] drm/i915: Give each sw_fence its own lockclass

[Joonas Lahtinen]

On ma, 2016-11-14 at 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 



> @@ -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 

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 01/14] drm/i915: Give each sw_fence its own lockclass

[Tvrtko Ursulin]

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 
---
 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(>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(>wait);
+   __init_waitqueue_head(>wait, name, key);
kref_init(>kref);
atomic_set(>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 

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 01/14] drm/i915: Give each sw_fence its own lockclass

[Chris Wilson]

On Mon, Nov 14, 2016 at 09:01:00AM +, 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 
> 
> 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

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 01/14] drm/i915: Give each sw_fence its own lockclass

[Tvrtko Ursulin]

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 


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(>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(>wait);
+   __init_waitqueue_head(>wait, name, key);
kref_init(>kref);
atomic_set(>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,


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx