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.belgaum...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Petri Latvala <petri.latv...@intel.com>
> Cc: Chris Wilson <ch...@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
