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

["Szwichtenberg, Radoslaw" <radoslaw.szwichtenberg@intel.com>]

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@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Chris Wilson <chris@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 */
I am not convinced that comment describing what mmap does is needed. I think we
should not document what system calls do.
>  	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);
Same as before - mprotect is a system call, this does not need any documentation
in my opinion. If there is a reason of making buffer read only that is not
obvious or not described in test description then it is worth stating why you
are marking it this way. 
>  
>  	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);
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx