[Intel-gfx] [RFC i-g-t] tests/gem_exec_basic: Documentation for subtests

[Fiedorowicz, Lukasz]

Hi, I'm a bit confused by this patch. How is this idea different from
what previous patch from Petri offered? I mean what additional value
does this solution brings? Is this something we want on top of previous
solution or instead of previous solution?
This doesn't take in to account auto generated tests and is more
'description of functions' then 'subtest documentation'.
If we take Petris example here we have basically the same information:

"(WHAT) Frob knobs 
"(WHY) to see if one of the crossbeams will go out of skew on the
treadle."

but directly related to the subtest.

On Tue, 2017-08-08 at 15:09 -0700, Vinay Belgaumkar wrote:
> This is an RFC for adding documentation to IGT subtests. Each subtest
> can have
> something similar to a WHAT - explaining what the subtest actually
> does,
> and a WHY - which explains a use case, if applicable. Additionally,
> include comments for anything in the subtest code which can help
> explain HOW the test has been implemented. We don't actually need the
> WHAT
> and WHY tags in the documentation.
> 
> These comments will not be linked to gtkdoc as of now, since we do
> not have a
>  mechanism to link it to every subtest name.
> 
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  tests/gem_exec_basic.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/tests/gem_exec_basic.c b/tests/gem_exec_basic.c
> index 2f057ef..b1491cd 100644
> --- a/tests/gem_exec_basic.c
> +++ b/tests/gem_exec_basic.c
> @@ -25,6 +25,11 @@
>  
>  IGT_TEST_DESCRIPTION("Basic sanity check of execbuf-ioctl rings.");
>  
> +/*
> +(WHAT) This subtest submits an empty batch to each ring and verifies
> +that it is executed successfully
> +(WHY) It validates that GT buffer submission mechanism is functional
> +*/
>  static void noop(int fd, unsigned ring)
>  {
>  	uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -45,6 +50,11 @@ static void noop(int fd, unsigned ring)
>  	gem_close(fd, exec.handle);
>  }
>  
> +/*
> +(WHAT) This subtest memory maps a buffer, marks it as read only, 
> +and submits it to each ring for execution.
> +(WHY) It helps us validate that the GT can execute read-only buffers
> +*/
>  static void readonly(int fd, unsigned ring)
>  {
>  	uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -57,12 +67,15 @@ static void readonly(int fd, unsigned ring)
>  	exec.handle = gem_create(fd, 4096);
>  	gem_write(fd, exec.handle, 0, &bbe, sizeof(bbe));
>  
> +	/* Memory map a buffer and use it as the execbuf to be
> submitted */
>  	execbuf = mmap(NULL, 4096, PROT_WRITE, MAP_ANON |
> MAP_PRIVATE, -1, 0);
>  	igt_assert(execbuf != NULL);
>  
>  	execbuf->buffers_ptr = to_user_pointer(&exec);
>  	execbuf->buffer_count = 1;
>  	execbuf->flags = ring;
> +
> +	/* Now mark the buffer as read-only */
>  	igt_assert(mprotect(execbuf, 4096, PROT_READ) == 0);
>  
>  	gem_execbuf(fd, execbuf);
> @@ -70,6 +83,10 @@ static void readonly(int fd, unsigned ring)
>  	gem_close(fd, exec.handle);
>  }
>  
> +/*
> +(WHAT) Create a gtt mapped buffer and submit to the GPU.
> +(WHY) It ensures GPU can properly map and access GTT mapped buffers
> +*/
>  static void gtt(int fd, unsigned ring)
>  {
>  	uint32_t bbe = MI_BATCH_BUFFER_END;
> @@ -82,6 +99,8 @@ static void gtt(int fd, unsigned ring)
>  	handle = gem_create(fd, 4096);
>  
>  	gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT,
> I915_GEM_DOMAIN_GTT);
> +
> +	/* Create a memory mapping through GTT */
>  	execbuf = gem_mmap__gtt(fd, handle, 4096, PROT_WRITE);
>  	exec = (struct drm_i915_gem_exec_object2 *)(execbuf + 1);
>  	gem_close(fd, handle);
> @@ -108,6 +127,8 @@ igt_main
>  		fd = drm_open_driver(DRIVER_INTEL);
>  		igt_require_gem(fd);
>  
> +		/* Start the hang detector process. Test will fail
> +		if a GPU hang is detected during any subtest */
>  		igt_fork_hang_detector(fd);
>  	}
>