Isabella Basso: 2 igt_kmod: add compatibility for KUnit tests/kms_kunit: add KUnit damage helper test 4 files changed, 236 insertions(+), 0 deletions(-)
21/2022/ > +
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~lkcamp/patches/patches/30240/mbox | git am -3Learn more about email & git
This adds functions for both executing the tests as well as parsing TAP kmsg output.
Rodrigo Siqueira <siqueirajordao@riseup.net>Please, expand your commit message a little bit more. For example, many people might not know what TAP is. Also, add some examples.
Changes since v1: - Correct handling of read to deal with partial buffer.
Rodrigo Siqueira <siqueirajordao@riseup.net>When you send this patch to the upstream, you can drop this part because this is a new patch for the upstream.
Signed-off-by: Isabella Basso <isabbasso@riseup.net> --- lib/igt_kmod.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++ lib/igt_kmod.h | 13 ++++ 2 files changed, 201 insertions(+) diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c index cf7a3b22..14f1939d 100644 --- a/lib/igt_kmod.c +++ b/lib/igt_kmod.c @@ -345,6 +345,194 @@ igt_kmod_list_loaded(void) kmod_module_unref_list(list); } +static void kmsg_parse_tap(int fd)
Rodrigo Siqueira <siqueirajordao@riseup.net>I know this is a static function, and we usually don't document it. However, this function does many things that imho deserve a brief explanation to set up the developer's mind to reading this code.
+{ + char record[4096 + 1]; + const char *num_tests_str, *nok_str, *ok_str, *end_str_success, *end_str_fail;
Rodrigo Siqueira <siqueirajordao@riseup.net>Let's use better variable names here, something like: nok_str -> not_ok_str end_str_success -> end_of_the_test_success end_str_fail -> end_of_the_test_fail You don't need to use those suggested names, just use something more meaningful.
+ bool has_num = false; + + num_tests_str = ".."; + nok_str = " not ok "; + ok_str = " ok ";
Rodrigo Siqueira <siqueirajordao@riseup.net>Do you know if we have a link from the spec that says these spaces are part of the TAP specification? If so, let's add the link here.
+ end_str_fail = "not ok "; + end_str_success = "ok ";
Rodrigo Siqueira <siqueirajordao@riseup.net>I thought this part was a bit confusing the first time I read it. We have the same string set for the substest pass and the complete test pass. Maybe new variable names can mitigate this issue, and a comment or two can also help.
+ + if (fd == -1) { + igt_warn("Unable to retrieve kernel log (from /dev/kmsg)\n"); + return; + }
Rodrigo Siqueira <siqueirajordao@riseup.net>I think it is okay to check this, but since this function returns nothing, we should expect to validate the fd before calling this function.
+ + record[sizeof(record) - 1] = '\0'; + + while (true) { + const char *end, *result; + ssize_t r;
Rodrigo Siqueira <siqueirajordao@riseup.net>Let's call it ret.
+ bool skip = true; + + if (!skip) {
Rodrigo Siqueira <siqueirajordao@riseup.net>Please, correct me if I'm wrong, but I think we never enter into this if condition because you set `skip = true` before. Or did I miss something?
+ end = strchrnul(record, '\n'); + } else { + end = NULL; + skip = false; + } + + r = read(fd, record, + end != NULL ? + (int) (end - record) : + sizeof(record) - 1);
Rodrigo Siqueira <siqueirajordao@riseup.net>Can we create a variable that will have the result of `end != NULL ? (int) (end - record) : sizeof(record) - 1)` and just use it here?
+ + if (r <= 0) { + if (errno == EINTR) + continue;
Rodrigo Siqueira <siqueirajordao@riseup.net>Do we really want to continue in this case?
+ + if (errno == EPIPE) { + igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n"); + continue; + } + + if (errno != EAGAIN) + igt_warn("kmsg truncated: unknown error (%m)\n"); + + break; + } + + if (!has_num) {
Rodrigo Siqueira <siqueirajordao@riseup.net>Maybe we can use a variable name that better describe this number?
+ result = strstr(record, num_tests_str); + if (result) { + result += strlen(num_tests_str); + end = strchrnul(result, '\n'); + /* in case line > 4096 */ + if (end == NULL) {
Rodrigo Siqueira <siqueirajordao@riseup.net>You don't need `{` and `}` in this if.
+ igt_info("%s\n", record); + } + else { + igt_info("[IGT] running %.*s tests...\n", + (int)(end - result), result); + } + has_num = true; + } + continue; + } + + /* all results have 'ok' in them */ + result = strstr(record, "ok"); + + if (result) { + end = strchrnul(result, '\n'); + /* in case line > 4096 */ + if (end == NULL) { + igt_info("%s\n", record); + continue; + } + + /* subtest succeded (i.e. ok_str) */ + result = strstr(record, ok_str); + + if (result) { + result += strlen(ok_str); + igt_info("[IGT] SUBTEST %.*s\n", + (int)(end - result), result); + continue; + } + + /* subtest failed (i.e. nok_str) */
Rodrigo Siqueira <siqueirajordao@riseup.net>How about creating a function for handling the pass and failed cases instead of keeping these codes here?
+ result = strstr(record, nok_str); + + if (result) { + result += strlen(nok_str); + igt_warn("[IGT] SUBTEST FAILED %.*s\n", + (int)(end - result), result); + continue; + } + + /* test ended (i.e. end_str_success) */ + /* we should skip the ; in the beginning of the string */ + result = strchr(record, ';') + 1; + + if (strncmp(result, end_str_success, strlen(end_str_success)) == 0) { + igt_info("[IGT] TEST SUCCEEDED %.*s\n", + (int)(end - result), result); + + igt_success(); + } + + if (strncmp(result, end_str_fail, strlen(end_str_fail)) == 0) { + igt_warn("[IGT] TEST FAILED %.*s\n", + (int)(end - result), result); + igt_fail(IGT_EXIT_FAILURE); + } + } + skip = true; + } +} + +/** + * igt_kunit: tests a kunit module
Rodrigo Siqueira <siqueirajordao@riseup.net>/:/ -/
+ * @mod_name: the name of the module + * @options: options to load the module + * @req_mods: array with additional dependencies to run the KUnit test + * @req_mod_len: size of req_mods array + * + * Loads the kunit module, parses its dmesg output, then unloads it + */ +void igt_kunit(const char *mod_name, const char *options, + const char **req_mods, int req_mod_len) +{ + int kmsg_fd, cmi;
Rodrigo Siqueira <siqueirajordao@riseup.net>cmi? What is it?
+ struct igt_kselftest tst; + const char *norm_name;
Rodrigo Siqueira <siqueirajordao@riseup.net>Just for curiosity, what does normalize a name mean? Thanks Siqueira
+ + /* get normalized module name */ + igt_require(igt_kselftest_init(&tst, mod_name) == 0); + + norm_name = kmod_module_get_name(tst.kmod); + + if (!igt_kmod_is_loaded(norm_name)) { + /* The kunit module is required for running any kunit tests */ + if (!igt_kmod_is_loaded("kunit")) + igt_require(igt_kmod_load("kunit", NULL) == 0); + + for (cmi = 0; cmi < req_mod_len; cmi++) { + if (strcmp(req_mods[cmi], "kunit") == 0) + continue; + if (igt_kmod_load(req_mods[cmi], NULL)) + goto unload; + } + } else { + /* Unload module if loaded (maybe previous run bailed out?) */ + igt_kmod_unload(mod_name, 0); + } + + kmsg_fd = open("/dev/kmsg", O_RDONLY | O_NONBLOCK); + + if (kmsg_fd < 0) { + igt_warn("Could not open /dev/kmsg"); + goto unload; + } + + if (lseek(kmsg_fd, 0, SEEK_END)) { + igt_warn("Could not seek the end of /dev/kmsg"); + close(kmsg_fd); + goto unload; + } + + igt_require(igt_kmod_load(mod_name, options) == 0); + + kmsg_parse_tap(kmsg_fd); + + igt_kmod_unload(mod_name, 0); + + close(kmsg_fd); +unload: + igt_kmod_unload("kunit", 0); + + for (cmi--; cmi >= 0; cmi--) { + if (strcmp(req_mods[cmi], "kunit") == 0) + continue; + igt_kmod_unload(req_mods[cmi], 0); + } +} + /** * igt_i915_driver_load: * @opts: options to pass to i915 driver diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h index 0898122b..60e7a750 100644 --- a/lib/igt_kmod.h +++ b/lib/igt_kmod.h @@ -28,6 +28,16 @@ #include "igt_list.h" +#define MAX_MOD_NAME_LEN 40 + +/** + * ARRAY_SIZE: + * @arr: static array + * + * Macro to compute the size of the static array @arr. + */ +#define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0])) + bool igt_kmod_is_loaded(const char *mod_name); void igt_kmod_list_loaded(void); @@ -36,6 +46,9 @@ bool igt_kmod_has_param(const char *mod_name, const char *param); int igt_kmod_load(const char *mod_name, const char *opts); int igt_kmod_unload(const char *mod_name, unsigned int flags); +void igt_kunit(const char *mod_name, const char *options, + const char **req_mods, int req_mod_len); + int igt_i915_driver_load(const char *opts); int igt_i915_driver_unload(void); int __igt_i915_driver_unload(const char **whom); -- 2.35.1
Rodrigo Siqueira <siqueirajordao@riseup.net>Hi Isabella,
This adds a converted KUnit test module to IGT.
Rodrigo Siqueira <siqueirajordao@riseup.net>Could you elaborate more about this patch in the commit message?
Signed-off-by: Isabella Basso <isabbasso@riseup.net> --- tests/kms_kunit.c | 34 ++++++++++++++++++++++++++++++++++ tests/meson.build | 1 + 2 files changed, 35 insertions(+) create mode 100644 tests/kms_kunit.c diff --git a/tests/kms_kunit.c b/tests/kms_kunit.c new file mode 100644 index 00000000..31566910 --- /dev/null +++ b/tests/kms_kunit.c @@ -0,0 +1,34 @@ +/* + * Copyright © 2021 Isabella Basso do Amaral <isabbasso@riseup.net> + * + * 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. + */ + +#include <libkmod.h> + +#include "igt.h" +#include "igt_kmod.h" + +IGT_TEST_DESCRIPTION("KUnit DRM damage helper module testing");
Rodrigo Siqueira <siqueirajordao@riseup.net>uppose this file will be expanded to all kunit tests, right? If so, I think you will need a more generic description. Thanks > +
+ +igt_simple_main +{ + igt_kunit("test-drm_damage_helper", NULL, NULL, 0); +} diff --git a/tests/meson.build b/tests/meson.build index 3dbba7a1..6d3685c3 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -36,6 +36,7 @@ test_progs = [ 'kms_hdmi_inject', 'kms_hdr', 'kms_invalid_mode', + 'kms_kunit', 'kms_lease', 'kms_multipipe_modeset', 'kms_panel_fitting', -- 2.35.1
Rodrigo Siqueira <siqueirajordao@riseup.net>21/2022/ > +