[Intel-gfx] [PATCH igt] igt/gem_exec_parse: generalise test_lri + debug info

[Chris Wilson]

On Thu, Nov 24, 2016 at 01:47:46PM +0000, Robert Bragg wrote:
> This further generalises the description passed to test_lri so we only
> need one loop over the entries with test_lri deducing the exected errno
> and value based on whether the register is marked as whitelisted and
> depending on the current command parser version.
> 
> Each tested register LRI now test gets its own subtest like:
> igt_subtest_f("test-lri-%s", reg_name)
> 
> The test_lri helper now also double checks that the initial
> intel_register_write() takes before issuing the LRI.
> 
> In case of a failure the test_lri helper now uses igt_debug to log the
> register name, address and value being tested.

That looks like it should help immensely if one of these should ever
fire. Thanks.
 
> Signed-off-by: Robert Bragg <robert at sixbynine.org>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

> ---
>  tests/gem_exec_parse.c | 102 +++++++++++++++++++++++++------------------------
>  1 file changed, 52 insertions(+), 50 deletions(-)
> 
> diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c
> index cc2103a..0cd7053 100644
> --- a/tests/gem_exec_parse.c
> +++ b/tests/gem_exec_parse.c
> @@ -258,12 +258,17 @@ static void exec_batch_chained(int fd, uint32_t cmd_bo, uint32_t *cmds,
>   * from...
>   */
>  struct test_lri {
> -	uint32_t reg, read_mask, init_val, test_val;
> +	const char *name; /* register name for debug info */
> +	uint32_t reg; /* address to test */

I would use offset, but we often mix the two anyway.

> +	uint32_t read_mask; /* ignore things like HW status bits */
> +	uint32_t init_val; /* initial identifiable value to set without LRI */
> +	uint32_t test_val; /* value to attempt loading via LRI command */
> +	bool whitelisted; /* expect to become NOOP / fail if not whitelisted */
> +	int min_ver; /* required command parser version to test */
>  };
>  
>  static void
> -test_lri(int fd, uint32_t handle,
> -	 struct test_lri *test, int expected_errno, uint32_t expect)
> +test_lri(int fd, uint32_t handle, struct test_lri *test)
>  {
>  	uint32_t lri[] = {
>  		MI_LOAD_REGISTER_IMM,
> @@ -271,9 +276,20 @@ test_lri(int fd, uint32_t handle,
>  		test->test_val,
>  		MI_BATCH_BUFFER_END,
>  	};
> +	int bad_lri_errno = parser_version >= 8 ? 0 : -EINVAL;
> +	int expected_errno = test->whitelisted ? 0 : bad_lri_errno;
> +	uint32_t expect = test->whitelisted ? test->test_val : test->init_val;
> +
> +	igt_debug("Testing %s LRI: addr=%x, val=%x, expected errno=%d, expected val=%x\n",
> +		  test->name, test->reg, test->test_val,
> +		  expected_errno, expect);
>  
>  	intel_register_write(test->reg, test->init_val);
>  
> +	igt_assert_eq_u32((intel_register_read(test->reg) &
> +			   test->read_mask),
> +			  test->init_val);
> +
>  	exec_batch(fd, handle,
>  		   lri, sizeof(lri),
>  		   I915_EXEC_RENDER,

I would still be verbose in the assert, it is the first thing you read
on seeing the error before digging through the debug log.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre