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

[Tvrtko Ursulin]

On 06/12/2016 14:44, Chris Wilson wrote:
> 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.

Names are okayish and it sticks out nicely so it is fine by me.

Regards,

Tvrtko