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

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

[Vinay Belgaumkar]

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 */
 	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);
 	}
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for tests/gem_exec_basic: Documentation for subtests

[Patchwork]

== Series Details ==

Series: tests/gem_exec_basic: Documentation for subtests
URL   : https://patchwork.freedesktop.org/series/28524/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
27ff12f00c94a5363f581a4353f08bfde62c59c4 lib: Remove illegal instructions from hang injection

with latest DRM-Tip kernel build CI_DRM_2936
e2586470ba9f drm-tip: 2017y-08m-08d-20h-39m-09s UTC integration manifest

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-n2820) fdo#101705

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:434s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:418s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:361s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:497s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:495s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:515s
fi-byt-n2820     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:507s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:582s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:427s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:403s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:425s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:509s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:480s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:456s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:573s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:577s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:526s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:455s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:650s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:432s
fi-skl-x1585l    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:488s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:547s
fi-snb-2600      total:279  pass:248  dwarn:0   dfail:0   fail:2   skip:29  time:408s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_33/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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 */
>  	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);
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Szwichtenberg, Radoslaw]

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

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

[Arkadiusz Hiler]

On Tue, Aug 08, 2017 at 03:09:00PM -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.

Hey Vinay,

I get similar feelings towards this RFC as Lukasz and Radek do.

Was your intention to propose format of the comments? Or maybe force
people to comment more on the code? Or just pointing out that we could
use some subtest documentation?

You are not documenting subtests, you are documenting arbitrary
functions that may or may not be used as a subtest.

I cannot help but feel lost here.

Being explicit as of your intention and coming up with more abstract or
better examples would also help, as current ones are detracting from the
idea itself.

I do not get this RFC and it's purpose but I am looking forward to
seeing revised version that is clearer on your intentions and easier to
grasp.

-- 
Cheers,
Arek

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Belgaumkar, Vinay]

On 8/9/2017 7:32 AM, Arkadiusz Hiler wrote:
> On Tue, Aug 08, 2017 at 03:09:00PM -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.
> Hey Vinay,
>
> I get similar feelings towards this RFC as Lukasz and Radek do.
>
> Was your intention to propose format of the comments? Or maybe force
> people to comment more on the code? Or just pointing out that we could
> use some subtest documentation?
>
> You are not documenting subtests, you are documenting arbitrary
> functions that may or may not be used as a subtest.
>
> I cannot help but feel lost here.
>
> Being explicit as of your intention and coming up with more abstract or
> better examples would also help, as current ones are detracting from the
> idea itself.
>
> I do not get this RFC and it's purpose but I am looking forward to
> seeing revised version that is clearer on your intentions and easier to
> grasp.
>

Hi Arek,
      The purpose of this RFC is to complement Petri's subtest 
documentation patch. That patch will give us
an ability to add a line of documentation per subtest, which is 
definitely useful. However, what I noticed is
that when you actually start debugging a test issue and step into the 
subtest code, it is very hard to understand
what the purpose of certain commands are. My intention was to provide a 
text only documentation in the test source
to allow test developers to understand the code better. It's hard to 
explain the how and and why all in a single sentence.

If we provided an ability/guideline to test developers for mentioning 
the same at the beginning of the actual subtest code,
it can make debugging a lot more simpler rather than having to jump back 
to the main function to figure out what the subtest
is supposed to do. I have also included some comments inside the test 
function to explain why we use certain system calls.
Idea was not to define the system call, but explain why it is being used.

Again, I did not mean to duplicate the subtest documentation effort. My 
initial plan was to send out an
RFC using Petri's patch as well, so that the intention is more clear. I 
can do that with the second version of the patch if needed.
However, the main aim is to agree on a convention to add more 
documentation to the subtest code so that it simplifies
debugging and helps with understanding of the aim behind writing the 
particular subtest.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Arkadiusz Hiler]

On Wed, Aug 09, 2017 at 10:00:16AM -0700, Belgaumkar, Vinay wrote:
> 
> 
> On 8/9/2017 7:32 AM, Arkadiusz Hiler wrote:
> > On Tue, Aug 08, 2017 at 03:09:00PM -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.
> > Hey Vinay,
> > 
> > I get similar feelings towards this RFC as Lukasz and Radek do.
> > 
> > Was your intention to propose format of the comments? Or maybe force
> > people to comment more on the code? Or just pointing out that we could
> > use some subtest documentation?
> > 
> > You are not documenting subtests, you are documenting arbitrary
> > functions that may or may not be used as a subtest.
> > 
> > I cannot help but feel lost here.
> > 
> > Being explicit as of your intention and coming up with more abstract or
> > better examples would also help, as current ones are detracting from the
> > idea itself.
> > 
> > I do not get this RFC and it's purpose but I am looking forward to
> > seeing revised version that is clearer on your intentions and easier to
> > grasp.
> > 
> 
> Hi Arek,
>      The purpose of this RFC is to complement Petri's subtest documentation
> patch. That patch will give us
> an ability to add a line of documentation per subtest, which is definitely
> useful. However, what I noticed is
> that when you actually start debugging a test issue and step into the
> subtest code, it is very hard to understand
> what the purpose of certain commands are. My intention was to provide a text
> only documentation in the test source
> to allow test developers to understand the code better. It's hard to explain
> the how and and why all in a single sentence.
> 
> If we provided an ability/guideline to test developers for mentioning the
> same at the beginning of the actual subtest code,

The ability is already there - the comments.
What we need is the drive to do the commenting.

IMO the issue is that one thing may be obvious to some and at the same
time look arcane to others - people are generally biased and treat
themselves as the baseline.

This is not solvable by guideline or a RFC imposing a commenting format.

This should be a joint effort:

 * authors trying to be more empathetic - hard to do and impossible to
   enforce

 * reviewers should point out things that are hard to understand and
   ask for clarification, preferably in a form of a slight rewrite of
   the code so it's more manageable, or as a comment if the former is not
   possible

 * people debugging such arcane parts (or asked the author) should take
   the effort and either make it more human readable or provide such
   commentary to save time of those who will come there after them

In case you haven't noticed - recently we started requiring that all
commits go through the mailing list, so everything is reviewed.

Feel free to participate in the discussion. It's okay even if you ask
for clarifications post-merge. They can be done in follow-up series, and
those don't need to be done by the original author.

As of guideline, having Petri's approach would already help a lot. I
would prefer to see how it does, and after it proves (or not) take
the further steps.

> it can make debugging a lot more simpler rather than having to jump back to
> the main function to figure out what the subtest
> is supposed to do. I have also included some comments inside the test
> function to explain why we use certain system calls.
> Idea was not to define the system call, but explain why it is being used.

Since you are "debugging" the subtest  I don't think it's this much of
an effort to do --list-subtest-with-doc or do jump in the code once or
twice.

IMO it's better to have the information provided only once. It's already
hard to keep doc/comments up to date not dealing with such redundancy.

> Again, I did not mean to duplicate the subtest documentation effort. My
> initial plan was to send out an
> RFC using Petri's patch as well, so that the intention is more clear. I can
> do that with the second version of the patch if needed.
> However, the main aim is to agree on a convention to add more documentation
> to the subtest code so that it simplifies
> debugging and helps with understanding of the aim behind writing the
> particular subtest.

Waiting for v2 then :-)

-- 
Cheers,
Arek

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Belgaumkar, Vinay]

On 8/10/2017 1:32 AM, Arkadiusz Hiler wrote:
> On Wed, Aug 09, 2017 at 10:00:16AM -0700, Belgaumkar, Vinay wrote:
>>
>> On 8/9/2017 7:32 AM, Arkadiusz Hiler wrote:
>>> On Tue, Aug 08, 2017 at 03:09:00PM -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.
>>> Hey Vinay,
>>>
>>> I get similar feelings towards this RFC as Lukasz and Radek do.
>>>
>>> Was your intention to propose format of the comments? Or maybe force
>>> people to comment more on the code? Or just pointing out that we could
>>> use some subtest documentation?
>>>
>>> You are not documenting subtests, you are documenting arbitrary
>>> functions that may or may not be used as a subtest.
>>>
>>> I cannot help but feel lost here.
>>>
>>> Being explicit as of your intention and coming up with more abstract or
>>> better examples would also help, as current ones are detracting from the
>>> idea itself.
>>>
>>> I do not get this RFC and it's purpose but I am looking forward to
>>> seeing revised version that is clearer on your intentions and easier to
>>> grasp.
>>>
>> Hi Arek,
>>       The purpose of this RFC is to complement Petri's subtest documentation
>> patch. That patch will give us
>> an ability to add a line of documentation per subtest, which is definitely
>> useful. However, what I noticed is
>> that when you actually start debugging a test issue and step into the
>> subtest code, it is very hard to understand
>> what the purpose of certain commands are. My intention was to provide a text
>> only documentation in the test source
>> to allow test developers to understand the code better. It's hard to explain
>> the how and and why all in a single sentence.
>>
>> If we provided an ability/guideline to test developers for mentioning the
>> same at the beginning of the actual subtest code,
> The ability is already there - the comments.
> What we need is the drive to do the commenting.
>
> IMO the issue is that one thing may be obvious to some and at the same
> time look arcane to others - people are generally biased and treat
> themselves as the baseline.
>
> This is not solvable by guideline or a RFC imposing a commenting format.
>
> This should be a joint effort:
>
>   * authors trying to be more empathetic - hard to do and impossible to
>     enforce
>
>   * reviewers should point out things that are hard to understand and
>     ask for clarification, preferably in a form of a slight rewrite of
>     the code so it's more manageable, or as a comment if the former is not
>     possible
>
>   * people debugging such arcane parts (or asked the author) should take
>     the effort and either make it more human readable or provide such
>     commentary to save time of those who will come there after them
>
> In case you haven't noticed - recently we started requiring that all
> commits go through the mailing list, so everything is reviewed.
>
> Feel free to participate in the discussion. It's okay even if you ask
> for clarifications post-merge. They can be done in follow-up series, and
> those don't need to be done by the original author.
>
> As of guideline, having Petri's approach would already help a lot. I
> would prefer to see how it does, and after it proves (or not) take
> the further steps.

Totally agree with all the above points. I am fine with the approach of 
using a combination
of Petri's subtest documentation function and adding any extra comments 
where necessary.
The test code will get much easier to understand as long as we answer 
the WHAT, WHY and
to some extent, the HOW of the code. The guideline I was proposing was 
merely a way to
achieve all of the above, we do not need a stringent format as such.

>
>> it can make debugging a lot more simpler rather than having to jump back to
>> the main function to figure out what the subtest
>> is supposed to do. I have also included some comments inside the test
>> function to explain why we use certain system calls.
>> Idea was not to define the system call, but explain why it is being used.
> Since you are "debugging" the subtest  I don't think it's this much of
> an effort to do --list-subtest-with-doc or do jump in the code once or
> twice.
>
> IMO it's better to have the information provided only once. It's already
> hard to keep doc/comments up to date not dealing with such redundancy.
>
>> Again, I did not mean to duplicate the subtest documentation effort. My
>> initial plan was to send out an
>> RFC using Petri's patch as well, so that the intention is more clear. I can
>> do that with the second version of the patch if needed.
>> However, the main aim is to agree on a convention to add more documentation
>> to the subtest code so that it simplifies
>> debugging and helps with understanding of the aim behind writing the
>> particular subtest.
> Waiting for v2 then :-)
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx