Re: [PATCH i-g-t] Add dmesg capture and dumping to tests and a test for it.

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ti, 2015-11-17 at 13:05 +0000, Thomas Wood wrote:
> On 16 November 2015 at 13:22, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
> > +static void _igt_kmsg_capture_dump(void)
> > +{
> > 
<SNIP>
> > +               //fputs(strchr(igt_kmsg_capture_dump_buf, ';') + 1,
> > stderr);
> 
> This line can be removed if it is no longer needed.
> 

Yeah, already removed from my v2.

> 
> > @@ -1072,16 +1158,21 @@ void __igt_fail_assert(const char *domain,
> > const char *file, const int line,
> >   */
> >  void igt_exit(void)
> >  {
> > +       int status = IGT_EXIT_SUCCESS;
> > +
> >         igt_exit_called = true;
> > 
> >         if (run_single_subtest && !run_single_subtest_found) {
> >                 igt_warn("Unknown subtest: %s\n",
> > run_single_subtest);
> > -               exit(IGT_EXIT_INVALID);
> > +               status = IGT_EXIT_INVALID;
> > +               goto do_exit;
> >         }
> > 
> > 
> > -       if (igt_only_list_subtests())
> > -               exit(IGT_EXIT_SUCCESS);
> > +       if (igt_only_list_subtests()) {
> > +               status = IGT_EXIT_SUCCESS;
> > +               goto do_exit;
> 
> It might be good to avoid allocating the kmsg buffer and file
> descriptor altogether if only listing subtests. There are also a few
> cases where igt_exit won't be called, such as when using the help
> options or when a test has no subtests and one of the subtest options
> is specified.

Yes, Chris suggested only opening the FD when a subtest starts and
closing after each one. Which comes to the point that the "single test
or 1 or more subtests" leads to quite a few different codepaths, and I
think it should be considered converting all the single tests to tests
with only one subtest, to make the code maintainable.


> >         if (failed_one)
> > -               exit(igt_exitcode);
> > +               status = igt_exitcode;
> >         else if (succeeded_one)
> > -               exit(IGT_EXIT_SUCCESS);
> > +               status = IGT_EXIT_SUCCESS;
> >         else
> > -               exit(IGT_EXIT_SKIP);
> > +               status = IGT_EXIT_SKIP;
> > +do_exit:
> > +       if (igt_kmsg_capture_fd != -1)
> > +               close(igt_kmsg_capture_fd);
> > +       if (igt_kmsg_capture_dump_buf != NULL)
> > +               free(igt_kmsg_capture_dump_buf);
> 
> Since this always needs to be run, would it be simpler to move it to
> the top of this function?
> 

True, originally I did the dumping in this function so the position is
kind of leftover from there.

> 
> > +
> > +       exit(status);
> >  }
> > 
> >  /* fork support code */
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 8fb2de8..25f0c4a 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -62,6 +62,7 @@ TESTS_progs_M = \
> >         gem_tiled_partial_pwrite_pread \
> >         gem_userptr_blits \
> >         gem_write_read_ring_switch \
> > +       igt_capture \
> >         kms_addfb_basic \
> >         kms_atomic \
> >         kms_cursor_crc \
> > diff --git a/tests/igt_capture.c b/tests/igt_capture.c
> > new file mode 100644
> > index 0000000..fd008d0
> > --- /dev/null
> > +++ b/tests/igt_capture.c
> 
> Since this is an infrastructure test, it would be better placed in
> lib/tests. The binary will also need adding to .gitignore.
> 

Ok, will move it. I do notice there already is igt_stats in there.

Regards, Joonas

> 
> > @@ -0,0 +1,93 @@
> > +/*
> > + * Copyright © 2015 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to
> > whom the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice
> > (including the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + *    Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > + *
> > + */
> > +
> > +#include "igt.h"
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/ioctl.h>
> > +
> > +static FILE* kmsg;
> > +
> > +static void
> > +test_kmsg(void)
> > +{
> > +       fputs("TEST (KMSG)\n", kmsg);
> > +       fflush(kmsg);
> > +       igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_warn(void)
> > +{
> > +       igt_warn("TEST (WARN)\n");
> > +       igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_debug(void)
> > +{
> > +       igt_debug("TEST (DEBUG)\n");
> > +       igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +static void
> > +test_combined(void)
> > +{
> > +       igt_warn("TEST #1 (WARN)\n");
> > +       fputs("TEST #1\n", kmsg);
> > +       igt_warn("TEST #2 (WARN)\n");
> > +       fputs("TEST #2\n", kmsg);
> > +       fflush(kmsg);
> > +       igt_fail(IGT_EXIT_FAILURE);
> > +}
> > +
> > +igt_main
> > +{
> > +       igt_fixture {
> > +               kmsg = fopen("/dev/kmsg", "w");
> > +               igt_require(kmsg != NULL);
> > +       }
> > +
> > +       igt_subtest("kmsg")
> > +               test_kmsg();
> > +       igt_subtest("warn")
> > +               test_warn();
> > +       igt_subtest("debug")
> > +               test_debug();
> > +       igt_subtest("combined")
> > +               test_combined();
> > +
> > +       igt_fixture {
> > +               fclose(kmsg);
> > +       }
> > +}
> > --
> > 2.4.3
> > 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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