[PATCH 03/45] drm/i915: Add unit tests for the breadcrumb rbtree, insert/remove

[Chris Wilson]

On Tue, Dec 06, 2016 at 02:02:07PM +0000, Tvrtko Ursulin wrote:
> 
> On 06/12/2016 00:12, Chris Wilson wrote:
> >First retroactive test, make sure that the waiters are in global seqno
> >order after random inserts and removals.
> >
> >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >---
> > drivers/gpu/drm/i915/i915_selftests.h    |   1 +
> > drivers/gpu/drm/i915/intel_breadcrumbs.c | 198 +++++++++++++++++++++++++++++++
> > drivers/gpu/drm/i915/intel_ringbuffer.h  |   1 +
> > 3 files changed, 200 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_selftests.h b/drivers/gpu/drm/i915/i915_selftests.h
> >index 3f8015c8a520..9562ec81c781 100644
> >--- a/drivers/gpu/drm/i915/i915_selftests.h
> >+++ b/drivers/gpu/drm/i915/i915_selftests.h
> >@@ -8,4 +8,5 @@
> >  *
> >  * Tests are executed in reverse order by igt/drv_selftest
> >  */
> >+selftest(breadcrumbs, intel_breadcrumbs_selftest)
> > selftest(sanitycheck, i915_selftest_sanitycheck) /* keep last */
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >index 53ae7884babd..2c02939fd5b6 100644
> >--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -109,6 +109,11 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> > 	if (b->rpm_wakelock)
> > 		return;
> >
> >+	if (unlikely(b->mock)) {
> >+		b->rpm_wakelock = true;
> >+		return;
> >+	}
> 
> I think it will be very important to mark these very clearly both in
> code and declarations.
> 
> Quick idea:
> 
> struct ... {
> 	I915_SELFTEST_DECLARE(bool : 1, mock);
> };

I'm getting rusty, I thought C complained about extra ';' inside
structs, but #define I915_SELFTEST_DECLARE(x) with the above seems fine.

> I915_SELFTEST() {
> 	if (unlikely(b->mock)) {
> 		b->rpm_wakelock = true;
> 		return;
> 	}
> }

vim's indenter doesn't like this (the block has to be inside the macro
to avoid compilation without b->mock), so what I have is:

--- a/drivers/gpu/drm/i915/i915_selftest.h
+++ b/drivers/gpu/drm/i915/i915_selftest.h
@@ -51,4 +51,12 @@ int __i915_subtests(const char *caller,
 
 #define SUBTEST(x) { x, #x }
 
+#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
+#define I915_SELFTEST_DECLARE(x) x
+#define I915_SELFTEST_ONLY(x) unlikely(x)
+#else
+#define I915_SELFTEST_DECLARE(x)
+#define I915_SELFTEST_ONLY(x) 0
+#endif
+
 #endif /* __I915_SELFTEST_H__ */
diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 0616f8cc50c1..69be04ecbd5b 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -109,7 +109,7 @@ static void __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
        if (b->rpm_wakelock)
                return;
 
-       if (unlikely(b->mock)) {
+       if (I915_SELFTEST_ONLY(b->mock)) {
                b->rpm_wakelock = true;
                return;
        }
@@ -148,7 +148,7 @@ static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
        if (!b->rpm_wakelock)
                return;
 
-       if (unlikely(b->mock)) {
+       if (I915_SELFTEST_ONLY(b->mock)) {
                b->rpm_wakelock = false;
                return;
        }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c44d0b3f6520..aa586e4dbc70 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -5,6 +5,7 @@
 #include "i915_gem_batch_pool.h"
 #include "i915_gem_request.h"
 #include "i915_gem_timeline.h"
+#include "i915_selftest.h"
 
 #define I915_CMD_HASH_ORDER 9
 
@@ -244,7 +245,7 @@ struct intel_engine_cs {
 
                bool irq_enabled : 1;
                bool rpm_wakelock : 1;
-               bool mock : 1;
+               I915_SELFTEST_DECLARE(bool mock : 1);
        } breadcrumbs;
 

Pro: selftest code stands out.
Con: can't think of one macro to do both, need to add i915_selftest.h
into common code.

Mulling over the names but looks beneficial.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre