On Tue, Sep 05, 2017 at 04:34:32PM -0700, Belgaumkar, Vinay wrote:
>
>
> On 9/4/2017 1:30 AM, Daniel Vetter wrote:
> > On Thu, Aug 31, 2017 at 02:33:23PM -0700, Vinay Belgaumkar wrote:
> > > Added the missing IGT_TEST_DESCRIPTION and some subtest
> > > descriptions. Trying to establish a method to document
> > > subtests, it should describe the feature being tested
> > > rather than how. The HOW part can, if needed, be
> > > described in the test code.
> > >
> > > Documenting subtests will give us a good way to trace
> > > feature test coverage, and also help a faster ramp
> > > for understanding the test code.
> > >
> > > v2: Removed duplication, addressed comments, cc'd test author
> > >
> > > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > > Cc: Eric Anholt <eric@xxxxxxxxxx>
> > >
> > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
> > > ---
> > > tests/gem_flink_basic.c | 36 ++++++++++++++++++++++++++----------
> > > 1 file changed, 26 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tests/gem_flink_basic.c b/tests/gem_flink_basic.c
> > > index 26ae7d6..9c8c4c3 100644
> > > --- a/tests/gem_flink_basic.c
> > > +++ b/tests/gem_flink_basic.c
> > > @@ -36,6 +36,8 @@
> > > #include <sys/ioctl.h>
> > > #include "drm.h"
> > >
> > > +IGT_TEST_DESCRIPTION("Tests for flink - a way to export a gem object by name");
> > > +
> > > static void
> > > test_flink(int fd)
> > > {
> > > @@ -44,8 +46,6 @@ test_flink(int fd)
> > > struct drm_gem_open open_struct;
> > > int ret;
> > >
> > > - igt_info("Testing flink and open.\n");
> >
> > Do we really want to remove runtime-dumped information (maybe we could
> > tune it down to debug level) with comments that in my experience, no one
> > ever reads?
>
> I removed that to avoid duplication as per the previous review comments. I
> do think it's useful to have both.
tbh I think the log output is more useful than 1-line comments that say
nothing more than what the testname already hints at.
> > I just looked again at the igt library docs, and like last year when I've
> > done that, we've accumulated sizeable chunks of missing docs and warnings.
> > So who's going to maintain this? We can barely keep docs in shape for the
> > core libs, how exactly are we going to keep docs in shape for the hundreds
> > of testcases we have? Do you have a team of 10+ people working on this?
>
> I think this will be more effective if this becomes a collective
> responsibility, not just an individual team's effort. We should enforce
> regular updates to the documentation during code reviews. We do not need to
> start updating all 300+ tests for now. We can add this form of documentation
> whenever we encounter a test that is hard to follow or needs clarification.
> Same goes for the libraries.
Yes. But just wishing that it becomes a collective effort doesn't make it
one. This requires hard work of convincing an overloaded team that the
added effort is worth it. And we can't even sustain that well for
libraries.
If you expect others to do more work, it won't happen if you don't put
some serious effort (and lots of good convincing in). And without that
effort this is just going to be another doc effort that fizzles out before
it pays off.
> > Same about IGT_TEST_DESCRIPTION, not enough people seem to care even for
> > that simple one-liner to make it work.
> >
> > My proposal would be that we first try to get the docs we have into decent
> > shape, which means establishing as something everyone takes care of.
> > Adding more lofty documentation goals that won't pan out imo just doesn't
> > make much sense.
>
> Agree, but if we really want to make this efficient, we can look up the API
> documentation while describing a certain test and update both as necessary.
>
> >
> > And yes this is from the person who did push for docs almost everywhere.
> > It's hard work, and it's a lot of hard work.
> > -Daniel
> >
>
> I agree. There still seem to be mixed views on even whether we need
> documentation or not.
Yes.
-Daniel
>
> > > -
> > > memset(&create, 0, sizeof(create));
> > > create.size = 16 * 1024;
> > > ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> > > @@ -69,8 +69,6 @@ test_double_flink(int fd)
> > > struct drm_gem_flink flink2;
> > > int ret;
> > >
> > > - igt_info("Testing repeated flink.\n");
> > > -
> > > memset(&create, 0, sizeof(create));
> > > create.size = 16 * 1024;
> > > ret = ioctl(fd, DRM_IOCTL_I915_GEM_CREATE, &create);
> > > @@ -92,8 +90,6 @@ test_bad_flink(int fd)
> > > struct drm_gem_flink flink;
> > > int ret;
> > >
> > > - igt_info("Testing error return on bad flink ioctl.\n");
> > > -
> > > flink.handle = 0x10101010;
> > > ret = ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink);
> > > igt_assert(ret == -1 && errno == ENOENT);
> > > @@ -105,8 +101,6 @@ test_bad_open(int fd)
> > > struct drm_gem_open open_struct;
> > > int ret;
> > >
> > > - igt_info("Testing error return on bad open ioctl.\n");
> > > -
> > > open_struct.name = 0x10101010;
> > > ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
> > >
> > > @@ -121,8 +115,6 @@ test_flink_lifetime(int fd)
> > > struct drm_gem_open open_struct;
> > > int ret, fd2;
> > >
> > > - igt_info("Testing flink lifetime.\n");
> > > -
> > > fd2 = drm_open_driver(DRIVER_INTEL);
> > >
> > > memset(&create, 0, sizeof(create));
> > > @@ -134,11 +126,13 @@ test_flink_lifetime(int fd)
> > > ret = ioctl(fd2, DRM_IOCTL_GEM_FLINK, &flink);
> > > igt_assert_eq(ret, 0);
> > >
> > > + /* Open another reference to the gem object */
> > > open_struct.name = flink.name;
> > > ret = ioctl(fd, DRM_IOCTL_GEM_OPEN, &open_struct);
> > > igt_assert_eq(ret, 0);
> > > igt_assert(open_struct.handle != 0);
> > >
> > > + /* Before closing the previous one */
> > > close(fd2);
> > > fd2 = drm_open_driver(DRIVER_INTEL);
> > >
> > > @@ -155,14 +149,36 @@ igt_main
> > > igt_fixture
> > > fd = drm_open_driver(DRIVER_INTEL);
> > >
> > > + /**
> > > + * basic:
> > > + * Test creation and use of flink.
> > > + */
> > > igt_subtest("basic")
> > > test_flink(fd);
> > > +
> > > + /**
> > > + * double-flink:
> > > + * This test validates the ability to create multiple flinks
> > > + * for the same gem object. They should obtain the same name.
> > > + */
> > > igt_subtest("double-flink")
> > > test_double_flink(fd);
> > > +
> > > + /**
> > > + * bad-flink:
> > > + * Negative test for invalid flink usage.
> > > + */
> > > igt_subtest("bad-flink")
> > > test_bad_flink(fd);
> > > +
> > > igt_subtest("bad-open")
> > > test_bad_open(fd);
> > > +
> > > + /**
> > > + * flink-lifetime:
> > > + * Flink lifetime is limited to that of the gem object it
> > > + * points to.
> > > + */
> > > igt_subtest("flink-lifetime")
> > > test_flink_lifetime(fd);
> > > }
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
