[igt-dev] [PATCH v7 0/2] Add a new test for driver/device hot reload

[igt-dev] [PATCH v7 0/2] Add a new test for driver/device hot reload

[Janusz Krzysztofik]

The test should help resolving driver bugs which exhibit themselves
when a device is unplugged / driver unbind from a device while the
device is busy (different from simple module unload which requires 
device references being put first).

Janusz Krzysztofik (2):
  tests: Add a new test for driver/device hot reload
  tests/core_hot_reload: Accept external workload

 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 305 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 307 insertions(+)
 create mode 100644 tests/core_hot_reload.c


Last time (submission marked as v5) I tried to restore original
version numbering since initial internal submission and I failed,
sorry.  There was one more public submission I missed, that's why
we now have v7, not v6, sorry.

I've also rearranged v5->v6 description to better reflect ideas
standing behind, and credited authors of comments, requests and
suggestions I followed.
 

Changelog:
v6->v7:
- add missing igt_exit() needed with the second patch.

v5->v6 (third public submission, incorrectly marked as v5, sorry):
- run workload inside an igt helper subprocess so resources consumed
  by the workload are cleaned up automatically on workload subprocess
  crash, without affecting test results,
- move the igt helper with workload back from subtests to initial
  fixture so workload crash also does not affect test results,
- re-add the second patch which extends the test with an option for
  using an external command as a workload,
- other cleanups suggested by Kasia and Chris.

v4->v5 (second public submission, marked as v2):
- try to restore the device to a working state after each subtest
  (Peter, Daniel).

v3->v4 (first public submission, not marked with any version number):
- run dummy_workolad from inside subtests (Antonio).

v2->v3 (internal submission):
- run dummy_workload from the test process directly (Antonio),
- drop the patch for running external workload (Antonio).

v1->v2 (internal submission):
- run a subprocess with dummy_workload instead of external command
  (Antonio),
- keep use of external workload command as an option, move that to a
  separate patch.

-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v6 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a dummy load in background to put some workload on a device, then try
to either remove (unplug) the device from its bus, or unbind the device's
driver from it, depending on which subtest has been selected.  If
succeeded, unload the driver, rescan the device's bus if needed and
perform health checks on the device with the driver reloaded.

The dummy load is run from igt_fixture and in a sub-process, not directly
from subtests,  as it is expected to fail and it's more simple to ignore
igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
and exits, resources it was using are freed automatically so there is no
need to do any cleanups required for smooth module unload from the test
level itself.  Those cleanups might also make the subtests fail if simply
using igt library functions for that instead of reimplementing their safe
parts only.

The driver hot unbind / device hot unplug operation is expected to succeed
and the background workload sub-process to die in a reasonable time,
however long timeouts are used to let kernel level timeouts pop up first
if hit by a bug.

The dummy load works only on i915 driver.  The test is skipped on other
hardware unless they provide their implementation of igt_spin_batch_new()
and friends.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 258 insertions(+)
 create mode 100644 tests/core_hot_reload.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 214698da..d2c0941d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -16,6 +16,7 @@ TESTS_progs = \
 	core_getclient \
 	core_getstats \
 	core_getversion \
+	core_hot_reload \
 	core_setmaster_vs_auth \
 	debugfs_test \
 	drm_import_export \
diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
new file mode 100644
index 00000000..d862c99c
--- /dev/null
+++ b/tests/core_hot_reload.c
@@ -0,0 +1,256 @@
+/*
+ * Copyright © 2019 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.
+ */
+
+#include "igt.h"
+#include "igt_device.h"
+#include "igt_dummyload.h"
+#include "igt_kmod.h"
+#include "igt_sysfs.h"
+
+#include <getopt.h>
+#include <limits.h>
+#include <string.h>
+#include <unistd.h>
+
+
+typedef int (*action_t)(int dir);
+typedef void (*workload_wait_t)(void *priv);
+typedef void (*workload_t)(int device, const void *priv);
+
+
+/*
+ * Actions
+ *
+ * Purpose: make the device disappear
+ *
+ * @dir: file descriptor of an open device sysfs directory
+ *
+ * Return value: file descriptor of an open device bus' sysfs directory
+ * 		 or -1 if no bus rescan is needed
+ */
+
+/* Unbind the driver from the device */
+static int driver_unbind(int dir)
+{
+	char path[PATH_MAX], *dev_bus_addr;
+	int len;
+
+	len = readlinkat(dir, "device", path, sizeof(path) - 1);
+	path[len] = '\0';
+	dev_bus_addr = strrchr(path, '/') + 1;
+
+	igt_set_timeout(60, "Driver unbind timeout!");
+	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
+
+	/* No need for bus rescan */
+	return -1;
+}
+
+/* Remove (virtually unplug) the device from its bus */
+static int device_unplug(int dir)
+{
+	int bus;
+
+	bus = openat(dir, "device/subsystem", O_DIRECTORY);
+	igt_assert(bus >= 0);
+
+	igt_set_timeout(60, "Device unplug timeout!");
+	igt_sysfs_set(dir, "device/remove", "1");
+
+	return bus;
+}
+
+
+/*
+ * Workloads
+ *
+ * Purpose: Put some long lasting load on the device
+ *
+ * @device: open device file descriptor,
+ * @priv: pointer to an optional argument passed to the workload
+ *
+ * Return value: none
+ */
+
+/* Workload using igt_spin_batch_run() */
+
+static void spin_batch(int device, const void *priv)
+{
+	igt_spin_t *spin;
+
+	/* submit the job */
+	spin = igt_spin_batch_new(device);
+
+	/* wait for the job to crash */
+	gem_sync(device, spin->handle);
+
+	/* clean up if still possible */
+	igt_spin_batch_free(device, spin);
+}
+
+
+/*
+ * Skeleton
+ */
+
+static void healthcheck(int chipset)
+{
+	if (chipset == DRIVER_INTEL) {
+		/*
+		 * We have it perfectly implemented in i915_module_load,
+		 * just use it.
+		 */
+		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
+			   == IGT_EXIT_SUCCESS);
+	} else {
+		/*
+		 * We don't know how to check an unidentified device for health,
+		 * device reopen must suffice.
+		 */
+	}
+}
+
+static void driver_unload(int chipset, char *driver)
+{
+	if (chipset == DRIVER_INTEL)
+		igt_assert(igt_i915_driver_unload() == IGT_EXIT_SUCCESS);
+	else
+		igt_assert(igt_kmod_unload(driver, 0) == 0);
+}
+
+static int operation(int device, action_t action, workload_wait_t workload_wait,
+		     void *workload_priv)
+{
+	int dir, bus;
+
+	dir = igt_sysfs_open(device);
+
+	bus = action(dir);
+	close(dir);
+
+	if (workload_wait)
+		workload_wait(workload_priv);
+	igt_reset_timeout();
+
+	return bus;
+}
+
+
+static void __subtest(int device, int chipset, char *driver, action_t action,
+		      workload_wait_t workload_wait, void *workload_priv)
+{
+	int bus = operation(device, action, workload_wait, workload_priv);
+
+	close(device);
+	driver_unload(chipset, driver);
+
+	/* Valid file descriptor indicates we should rescan the bus */
+	if (bus >= 0) {
+		igt_sysfs_set(bus, "rescan", "1");
+		close(bus);
+	}
+}
+
+static void run_subtest(int *device, int chipset, char *driver, action_t action,
+			workload_wait_t workload_wait, void *workload_priv)
+{
+	__subtest(*device, chipset, driver, action, workload_wait, workload_priv);
+
+	healthcheck(chipset);
+
+	*device = __drm_open_driver(chipset);
+	igt_assert(*device >= 0);
+}
+
+static void __workload(workload_t workload, int device, const void *priv,
+		      struct igt_helper_process *proc)
+{
+	igt_fork_helper(proc)
+		workload(device, priv);
+	/* let the background process start doing its job */
+	sleep(2);
+}
+
+static void __workload_wait(void *priv)
+{
+	struct igt_helper_process *proc = priv;
+
+	/* wait until the workload has crashed */
+	igt_wait_helper(proc);
+}
+
+
+igt_main {
+	int device, chipset;
+	char *driver;
+	struct igt_helper_process proc = {};
+	workload_wait_t workload_wait;
+	void *workload_priv;
+
+	igt_fixture {
+		char path[PATH_MAX];
+		int dir, len;
+
+		/*
+		 * Since the test depends on successful unload of driver module,
+		 * don't use drm_open_driver() as it keeps a file descriptor
+		 * open for exit handler use that effectively locks the module.
+		 */
+		device = __drm_open_driver(DRIVER_ANY);
+		igt_assert(device >= 0);
+
+		if (is_i915_device(device)) {
+			chipset = DRIVER_INTEL;
+			driver = strdup("i915");
+		} else {
+			chipset = DRIVER_ANY;
+
+			/* Capture module name to be unloaded */
+			dir = igt_sysfs_open(device);
+			len = readlinkat(dir, "device/driver/module", path,
+					 sizeof(path) - 1);
+			close(dir);
+			path[len] = '\0';
+			driver = strdup(strrchr(path, '/') + 1);
+		}
+		igt_info("Running the test on driver \"%s\", chipset mask %#0x\n",
+			 driver, chipset);
+
+		workload_wait = __workload_wait;
+		workload_priv = &proc;
+		__workload(spin_batch, device, NULL, &proc);
+	}
+
+	igt_subtest("unplug")
+		run_subtest(&device, chipset, driver, device_unplug,
+			    workload_wait, workload_priv);
+
+	igt_subtest("unbind")
+		run_subtest(&device, chipset, driver, driver_unbind,
+			    workload_wait, workload_priv);
+
+	igt_fixture {
+		free(driver);
+		close(device);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 5167a6cc..1b91e5a2 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -3,6 +3,7 @@ test_progs = [
 	'core_getclient',
 	'core_getstats',
 	'core_getversion',
+	'core_hot_reload',
 	'core_setmaster_vs_auth',
 	'debugfs_test',
 	'drm_import_export',
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v6 2/2] tests/core_hot_reload: Accept external workload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a user specified command, possibly one of existing tests, in
background instead of the default dummy load to put some alternative
workload on a device while trying to either remove (unplug) the device
from its bus, or unbind the device's driver from it, depending on which
subtest has been selected.

The command selected as the alternative workload should make real use of
the device as much as possible and its execution should take
significantly more than 2 seconds in order to get reliable results from
the test.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/core_hot_reload.c | 53 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
index d862c99c..4a902ce4 100644
--- a/tests/core_hot_reload.c
+++ b/tests/core_hot_reload.c
@@ -33,6 +33,9 @@
 #include <unistd.h>
 
 
+#define OPT_WORKLOAD_CMD	'c'
+
+
 typedef int (*action_t)(int dir);
 typedef void (*workload_wait_t)(void *priv);
 typedef void (*workload_t)(int device, const void *priv);
@@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
 	igt_spin_batch_free(device, spin);
 }
 
+/* Workload using external command */
+
+static void ext_cmd(int device, const void *priv)
+{
+	const char *cmd = priv;
+
+	/* just run the user provided command line */
+	igt_system_quiet(cmd);
+}
+
 
 /*
  * Skeleton
@@ -200,12 +213,40 @@ static void __workload_wait(void *priv)
 }
 
 
-igt_main {
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	const char **cmd = data;
+
+	switch (opt) {
+	case 'c':
+		*cmd = strdup(optarg);
+		break;
+	}
+
+	return 0;
+}
+
+int main (int argc, char **argv)
+{
 	int device, chipset;
 	char *driver;
 	struct igt_helper_process proc = {};
 	workload_wait_t workload_wait;
 	void *workload_priv;
+	const struct option long_opts[] = {
+		{ "workload-cmd", required_argument, 0, OPT_WORKLOAD_CMD },
+		{ 0, 0, 0, 0 }
+	};
+	const char *help_str =
+		"  --workload-cmd, -c\n"
+		"\t\tCommand line to run in backgroud as an optional workload, e.g.,\n"
+		"\t\tan existing test name, possibly with options/arguments, quoted.\n"
+		"\t\tExecution must take way more than 2 seconds for reliable results.\n"
+		"\t\tNo default - internal dummy load is used if external not specified.\n";
+	const char *cmd = NULL;
+
+	igt_subtest_init_parse_opts(&argc, argv, "", long_opts, help_str,
+                                    opt_handler, &cmd);
 
 	igt_fixture {
 		char path[PATH_MAX];
@@ -238,7 +279,13 @@ igt_main {
 
 		workload_wait = __workload_wait;
 		workload_priv = &proc;
-		__workload(spin_batch, device, NULL, &proc);
+
+		if (cmd) {
+			igt_device_drop_master(device);
+			__workload(ext_cmd, -1, cmd, &proc);
+		} else {
+			__workload(spin_batch, device, NULL, &proc);
+		}
 	}
 
 	igt_subtest("unplug")
@@ -253,4 +300,6 @@ igt_main {
 		free(driver);
 		close(device);
 	}
+
+	igt_exit();
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 0/2] Add a new test for driver/device hot reload

[Janusz Krzysztofik]

On Wednesday, April 10, 2019 12:02:24 PM CEST Janusz Krzysztofik wrote:
> The test should help resolving driver bugs which exhibit themselves
> when a device is unplugged / driver unbind from a device while the
> device is busy (different from simple module unload which requires
> device references being put first).
> 
> Janusz Krzysztofik (2):
>   tests: Add a new test for driver/device hot reload
>   tests/core_hot_reload: Accept external workload
> 
>  tests/Makefile.sources  |   1 +
>  tests/core_hot_reload.c | 305 ++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build       |   1 +
>  3 files changed, 307 insertions(+)
>  create mode 100644 tests/core_hot_reload.c
> 
> 
> Last time (submission marked as v5) I tried to restore original
> version numbering since initial internal submission and I failed,
> sorry.  There was one more public submission I missed, that's why
> we now have v7, not v6, sorry.

I messed it up again,  Sorry for versions of patches not matching that of the 
cover letter.

Janusz


> 
> I've also rearranged v5->v6 description to better reflect ideas
> standing behind, and credited authors of comments, requests and
> suggestions I followed.
> 
> 
> Changelog:
> v6->v7:
> - add missing igt_exit() needed with the second patch.
> 
> v5->v6 (third public submission, incorrectly marked as v5, sorry):
> - run workload inside an igt helper subprocess so resources consumed
>   by the workload are cleaned up automatically on workload subprocess
>   crash, without affecting test results,
> - move the igt helper with workload back from subtests to initial
>   fixture so workload crash also does not affect test results,
> - re-add the second patch which extends the test with an option for
>   using an external command as a workload,
> - other cleanups suggested by Kasia and Chris.
> 
> v4->v5 (second public submission, marked as v2):
> - try to restore the device to a working state after each subtest
>   (Peter, Daniel).
> 
> v3->v4 (first public submission, not marked with any version number):
> - run dummy_workolad from inside subtests (Antonio).
> 
> v2->v3 (internal submission):
> - run dummy_workload from the test process directly (Antonio),
> - drop the patch for running external workload (Antonio).
> 
> v1->v2 (internal submission):
> - run a subprocess with dummy_workload instead of external command
>   (Antonio),
> - keep use of external workload command as an option, move that to a
>   separate patch.




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for Add a new test for driver/device hot reload (rev2)

[Patchwork]

== Series Details ==

Series: Add a new test for driver/device hot reload (rev2)
URL   : https://patchwork.freedesktop.org/series/59221/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5898 -> IGTPW_2835
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59221/revisions/2/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2835 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic-flip-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278


Participating hosts (49 -> 39)
------------------------------

  Missing    (10): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-j1900 fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ctg-p8600 fi-gdg-551 fi-bdw-samus 


Build changes
-------------

    * IGT: IGT_4937 -> IGTPW_2835

  CI_DRM_5898: b7ec63a9ba82930148c90769d04f957f10b6b6da @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2835: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2835/
  IGT_4937: 256c6107ee127d2ff07d23dfeb3b8d828cb43b37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+igt@core_hot_reload@unbind
+igt@core_hot_reload@unplug

== Logs ==

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

[igt-dev] ✗ Fi.CI.IGT: failure for Add a new test for driver/device hot reload (rev2)

[Patchwork]

== Series Details ==

Series: Add a new test for driver/device hot reload (rev2)
URL   : https://patchwork.freedesktop.org/series/59221/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5898_full -> IGTPW_2835_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2835_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2835_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59221/revisions/2/mbox/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_2835_full:

### IGT changes ###

#### Possible regressions ####

  * {igt@core_hot_reload@unbind} (NEW):
    - shard-snb:          NOTRUN -> DMESG-FAIL +1
    - shard-iclb:         NOTRUN -> INCOMPLETE +1

  * {igt@core_hot_reload@unplug} (NEW):
    - shard-hsw:          NOTRUN -> DMESG-FAIL +1

  * igt@runner@aborted:
    - shard-hsw:          NOTRUN -> ( 2 FAIL )
    - shard-snb:          NOTRUN -> ( 2 FAIL )

  
New tests
---------

  New tests have been introduced between CI_DRM_5898_full and IGTPW_2835_full:

### New IGT tests (2) ###

  * igt@core_hot_reload@unbind:
    - Statuses : 2 dmesg-fail(s) 3 incomplete(s)
    - Exec time: [0.0, 1.42] s

  * igt@core_hot_reload@unplug:
    - Statuses : 2 dmesg-fail(s) 4 incomplete(s)
    - Exec time: [0.0, 0.77] s

  

Known issues
------------

  Here are the changes found in IGTPW_2835_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * {igt@core_hot_reload@unbind} (NEW):
    - shard-apl:          NOTRUN -> INCOMPLETE [fdo#103927] +1
    - shard-kbl:          NOTRUN -> INCOMPLETE [fdo#103665] +1

  * {igt@core_hot_reload@unplug} (NEW):
    - shard-glk:          NOTRUN -> INCOMPLETE [fdo#103359] / [k.org#198133]

  * igt@gem_exec_flush@basic-batch-kernel-default-cmd:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +12

  * igt@gem_pread@stolen-snoop:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109277] +1

  * igt@gem_pwrite@huge-gtt-fbr:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109290]

  * igt@gem_tiled_swapping@non-threaded:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#108686]
    - shard-hsw:          PASS -> FAIL [fdo#108686]

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          PASS -> DMESG-WARN [fdo#108566] +5

  * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109308]

  * igt@i915_pm_rps@min-max-config-loaded:
    - shard-iclb:         NOTRUN -> FAIL [fdo#108059] / [fdo#109960]

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         PASS -> DMESG-FAIL [fdo#108954]

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_atomic_transition@4x-modeset-transitions:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +16

  * igt@kms_atomic_transition@plane-toggle-modeset-transition:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_busy@extended-pageflip-hang-oldfb-render-d:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109278] +7

  * igt@kms_chamelium@hdmi-cmp-nv61:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109284] +3

  * igt@kms_color@pipe-a-ctm-max:
    - shard-iclb:         NOTRUN -> FAIL [fdo#108147]

  * igt@kms_content_protection@legacy:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109300]
    - shard-apl:          NOTRUN -> FAIL [fdo#110321] / [fdo#110336]

  * igt@kms_cursor_crc@cursor-512x512-sliding:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109279]

  * igt@kms_cursor_legacy@2x-flip-vs-cursor-legacy:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274] +4

  * igt@kms_cursor_legacy@pipe-c-single-bo:
    - shard-kbl:          PASS -> DMESG-WARN [fdo#103558] / [fdo#105602] +18

  * igt@kms_fbcon_fbt@psr:
    - shard-iclb:         NOTRUN -> FAIL [fdo#103833]

  * igt@kms_flip@dpms-vs-vblank-race:
    - shard-apl:          PASS -> FAIL [fdo#103060]

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          PASS -> INCOMPLETE [fdo#103540]

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-render:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-msflip-blt:
    - shard-iclb:         PASS -> FAIL [fdo#103167] / [fdo#109960]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +18

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-blt:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +14

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-cur-indfb-move:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +85

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-indfb-plflip-blt:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +15

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-pri-indfb-draw-render:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] / [fdo#109960] +1

  * igt@kms_lease@cursor_implicit_plane:
    - shard-apl:          NOTRUN -> FAIL [fdo#110278]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-e:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +5

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-d:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb:
    - shard-apl:          NOTRUN -> FAIL [fdo#108145] +1

  * igt@kms_plane_alpha_blend@pipe-c-alpha-opaque-fb:
    - shard-glk:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         PASS -> FAIL [fdo#103166]

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109642]

  * igt@kms_psr@no_drrs:
    - shard-iclb:         PASS -> FAIL [fdo#108341]

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +1

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109441]

  * igt@kms_psr@sprite_blt:
    - shard-iclb:         NOTRUN -> FAIL [fdo#107383] / [fdo#110215] +1

  * igt@kms_rotation_crc@bad-pixel-format:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109289] +1

  * igt@kms_setmode@basic:
    - shard-kbl:          PASS -> FAIL [fdo#99912]

  * igt@kms_sysfs_edid_timing:
    - shard-apl:          NOTRUN -> FAIL [fdo#100047]

  * igt@kms_vblank@pipe-a-ts-continuation-modeset:
    - shard-apl:          PASS -> FAIL [fdo#104894]

  * igt@perf_pmu@busy-check-all-vecs0:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +108

  * igt@perf_pmu@most-busy-check-all-vcs1:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] +12

  * igt@perf_pmu@rc6:
    - shard-kbl:          PASS -> SKIP [fdo#109271]

  * igt@prime_busy@wait-after-bsd2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276] / [fdo#109960] +3

  * igt@prime_nv_api@nv_i915_import_twice_check_flink_name:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109291] +2

  * igt@v3d_mmap@mmap-bad-handle:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109315] / [fdo#109960]

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@bcs0-s3:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-kbl:          SKIP [fdo#109271] -> PASS

  * igt@i915_pm_rpm@system-suspend-execbuf:
    - shard-kbl:          INCOMPLETE [fdo#103665] / [fdo#107807] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-random:
    - shard-iclb:         INCOMPLETE [fdo#109960] -> PASS

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-atomic:
    - shard-glk:          FAIL [fdo#104873] -> PASS

  * igt@kms_cursor_legacy@cursor-vs-flip-legacy:
    - shard-hsw:          FAIL [fdo#103355] -> PASS

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          FAIL [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt:
    - shard-iclb:         FAIL [fdo#103167] / [fdo#109960] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-tilingchange:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +10

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - shard-apl:          DMESG-WARN [fdo#108566] -> PASS +1

  * igt@kms_psr@psr2_sprite_mmap_gtt:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +3

  * igt@kms_psr@sprite_mmap_cpu:
    - shard-iclb:         FAIL [fdo#107383] / [fdo#110215] -> PASS +1

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         FAIL [fdo#100047] -> PASS

  
#### Warnings ####

  * igt@perf_pmu@semaphore-wait-bcs0:
    - shard-hsw:          SKIP [fdo#109271] -> INCOMPLETE [fdo#103540]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103833]: https://bugs.freedesktop.org/show_bug.cgi?id=103833
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108059]: https://bugs.freedesktop.org/show_bug.cgi?id=108059
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109277]: https://bugs.freedesktop.org/show_bug.cgi?id=109277
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109279]: https://bugs.freedesktop.org/show_bug.cgi?id=109279
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109290]: https://bugs.freedesktop.org/show_bug.cgi?id=109290
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109308]: https://bugs.freedesktop.org/show_bug.cgi?id=109308
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109960]: https://bugs.freedesktop.org/show_bug.cgi?id=109960
  [fdo#110215]: https://bugs.freedesktop.org/show_bug.cgi?id=110215
  [fdo#110278]: https://bugs.freedesktop.org/show_bug.cgi?id=110278
  [fdo#110321]: https://bugs.freedesktop.org/show_bug.cgi?id=110321
  [fdo#110336]: https://bugs.freedesktop.org/show_bug.cgi?id=110336
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 6)
------------------------------

  Missing    (4): pig-skl-6260u shard-skl pig-hsw-4770r pig-glk-j5005 


Build changes
-------------

    * IGT: IGT_4937 -> IGTPW_2835
    * Piglit: piglit_4509 -> None

  CI_DRM_5898: b7ec63a9ba82930148c90769d04f957f10b6b6da @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2835: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2835/
  IGT_4937: 256c6107ee127d2ff07d23dfeb3b8d828cb43b37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

[igt-dev] [RESUBMIT][PATCH v7 0/2] Add a new test for driver/device hot reload

[Janusz Krzysztofik]

(Resubmitting with patch versions in commit titles fixed as v7)

The test should help resolving driver bugs which exhibit themselves
when a device is unplugged / driver unbind from a device while the
device is busy (different from simple module unload which requires 
device references being put first).

Janusz Krzysztofik (2):
  tests: Add a new test for driver/device hot reload
  tests/core_hot_reload: Accept external workload

 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 305 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 307 insertions(+)
 create mode 100644 tests/core_hot_reload.c


Last time (submission marked as v5) I tried to restore original
version numbering since initial internal submission and I failed,
sorry.  There was one more public submission I missed, that's why
we now have v7, not v6, sorry.

I've also rearranged v5->v6 description to better reflect ideas
standing behind, and credited authors of comments, requests and
suggestions I followed.
 

Changelog:
v6->v7:
- add missing igt_exit() needed with the second patch.

v5->v6 (third public submission, incorrectly marked as v5, sorry):
- run workload inside an igt helper subprocess so resources consumed
  by the workload are cleaned up automatically on workload subprocess
  crash, without affecting test results,
- move the igt helper with workload back from subtests to initial
  fixture so workload crash also does not affect test results,
- re-add the second patch which extends the test with an option for
  using an external command as a workload,
- other cleanups suggested by Kasia and Chris.

v4->v5 (second public submission, marked as v2):
- try to restore the device to a working state after each subtest
  (Petri, Daniel).

v3->v4 (first public submission, not marked with any version number):
- run dummy_workolad from inside subtests (Antonio).

v2->v3 (internal submission):
- run dummy_workload from the test process directly (Antonio),
- drop the patch for running external workload (Antonio).

v1->v2 (internal submission):
- run a subprocess with dummy_workload instead of external command
  (Antonio),
- keep use of external workload command as an option, move that to a
  separate patch.

-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a dummy load in background to put some workload on a device, then try
to either remove (unplug) the device from its bus, or unbind the device's
driver from it, depending on which subtest has been selected.  If
succeeded, unload the driver, rescan the device's bus if needed and
perform health checks on the device with the driver reloaded.

The dummy load is run from igt_fixture and in a sub-process, not directly
from subtests,  as it is expected to fail and it's more simple to ignore
igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
and exits, resources it was using are freed automatically so there is no
need to do any cleanups required for smooth module unload from the test
level itself.  Those cleanups might also make the subtests fail if simply
using igt library functions for that instead of reimplementing their safe
parts only.

The driver hot unbind / device hot unplug operation is expected to succeed
and the background workload sub-process to die in a reasonable time,
however long timeouts are used to let kernel level timeouts pop up first
if hit by a bug.

The dummy load works only on i915 driver.  The test is skipped on other
hardware unless they provide their implementation of igt_spin_batch_new()
and friends.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/Makefile.sources  |   1 +
 tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
 tests/meson.build       |   1 +
 3 files changed, 258 insertions(+)
 create mode 100644 tests/core_hot_reload.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 214698da..d2c0941d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -16,6 +16,7 @@ TESTS_progs = \
 	core_getclient \
 	core_getstats \
 	core_getversion \
+	core_hot_reload \
 	core_setmaster_vs_auth \
 	debugfs_test \
 	drm_import_export \
diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
new file mode 100644
index 00000000..d862c99c
--- /dev/null
+++ b/tests/core_hot_reload.c
@@ -0,0 +1,256 @@
+/*
+ * Copyright © 2019 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.
+ */
+
+#include "igt.h"
+#include "igt_device.h"
+#include "igt_dummyload.h"
+#include "igt_kmod.h"
+#include "igt_sysfs.h"
+
+#include <getopt.h>
+#include <limits.h>
+#include <string.h>
+#include <unistd.h>
+
+
+typedef int (*action_t)(int dir);
+typedef void (*workload_wait_t)(void *priv);
+typedef void (*workload_t)(int device, const void *priv);
+
+
+/*
+ * Actions
+ *
+ * Purpose: make the device disappear
+ *
+ * @dir: file descriptor of an open device sysfs directory
+ *
+ * Return value: file descriptor of an open device bus' sysfs directory
+ * 		 or -1 if no bus rescan is needed
+ */
+
+/* Unbind the driver from the device */
+static int driver_unbind(int dir)
+{
+	char path[PATH_MAX], *dev_bus_addr;
+	int len;
+
+	len = readlinkat(dir, "device", path, sizeof(path) - 1);
+	path[len] = '\0';
+	dev_bus_addr = strrchr(path, '/') + 1;
+
+	igt_set_timeout(60, "Driver unbind timeout!");
+	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
+
+	/* No need for bus rescan */
+	return -1;
+}
+
+/* Remove (virtually unplug) the device from its bus */
+static int device_unplug(int dir)
+{
+	int bus;
+
+	bus = openat(dir, "device/subsystem", O_DIRECTORY);
+	igt_assert(bus >= 0);
+
+	igt_set_timeout(60, "Device unplug timeout!");
+	igt_sysfs_set(dir, "device/remove", "1");
+
+	return bus;
+}
+
+
+/*
+ * Workloads
+ *
+ * Purpose: Put some long lasting load on the device
+ *
+ * @device: open device file descriptor,
+ * @priv: pointer to an optional argument passed to the workload
+ *
+ * Return value: none
+ */
+
+/* Workload using igt_spin_batch_run() */
+
+static void spin_batch(int device, const void *priv)
+{
+	igt_spin_t *spin;
+
+	/* submit the job */
+	spin = igt_spin_batch_new(device);
+
+	/* wait for the job to crash */
+	gem_sync(device, spin->handle);
+
+	/* clean up if still possible */
+	igt_spin_batch_free(device, spin);
+}
+
+
+/*
+ * Skeleton
+ */
+
+static void healthcheck(int chipset)
+{
+	if (chipset == DRIVER_INTEL) {
+		/*
+		 * We have it perfectly implemented in i915_module_load,
+		 * just use it.
+		 */
+		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
+			   == IGT_EXIT_SUCCESS);
+	} else {
+		/*
+		 * We don't know how to check an unidentified device for health,
+		 * device reopen must suffice.
+		 */
+	}
+}
+
+static void driver_unload(int chipset, char *driver)
+{
+	if (chipset == DRIVER_INTEL)
+		igt_assert(igt_i915_driver_unload() == IGT_EXIT_SUCCESS);
+	else
+		igt_assert(igt_kmod_unload(driver, 0) == 0);
+}
+
+static int operation(int device, action_t action, workload_wait_t workload_wait,
+		     void *workload_priv)
+{
+	int dir, bus;
+
+	dir = igt_sysfs_open(device);
+
+	bus = action(dir);
+	close(dir);
+
+	if (workload_wait)
+		workload_wait(workload_priv);
+	igt_reset_timeout();
+
+	return bus;
+}
+
+
+static void __subtest(int device, int chipset, char *driver, action_t action,
+		      workload_wait_t workload_wait, void *workload_priv)
+{
+	int bus = operation(device, action, workload_wait, workload_priv);
+
+	close(device);
+	driver_unload(chipset, driver);
+
+	/* Valid file descriptor indicates we should rescan the bus */
+	if (bus >= 0) {
+		igt_sysfs_set(bus, "rescan", "1");
+		close(bus);
+	}
+}
+
+static void run_subtest(int *device, int chipset, char *driver, action_t action,
+			workload_wait_t workload_wait, void *workload_priv)
+{
+	__subtest(*device, chipset, driver, action, workload_wait, workload_priv);
+
+	healthcheck(chipset);
+
+	*device = __drm_open_driver(chipset);
+	igt_assert(*device >= 0);
+}
+
+static void __workload(workload_t workload, int device, const void *priv,
+		      struct igt_helper_process *proc)
+{
+	igt_fork_helper(proc)
+		workload(device, priv);
+	/* let the background process start doing its job */
+	sleep(2);
+}
+
+static void __workload_wait(void *priv)
+{
+	struct igt_helper_process *proc = priv;
+
+	/* wait until the workload has crashed */
+	igt_wait_helper(proc);
+}
+
+
+igt_main {
+	int device, chipset;
+	char *driver;
+	struct igt_helper_process proc = {};
+	workload_wait_t workload_wait;
+	void *workload_priv;
+
+	igt_fixture {
+		char path[PATH_MAX];
+		int dir, len;
+
+		/*
+		 * Since the test depends on successful unload of driver module,
+		 * don't use drm_open_driver() as it keeps a file descriptor
+		 * open for exit handler use that effectively locks the module.
+		 */
+		device = __drm_open_driver(DRIVER_ANY);
+		igt_assert(device >= 0);
+
+		if (is_i915_device(device)) {
+			chipset = DRIVER_INTEL;
+			driver = strdup("i915");
+		} else {
+			chipset = DRIVER_ANY;
+
+			/* Capture module name to be unloaded */
+			dir = igt_sysfs_open(device);
+			len = readlinkat(dir, "device/driver/module", path,
+					 sizeof(path) - 1);
+			close(dir);
+			path[len] = '\0';
+			driver = strdup(strrchr(path, '/') + 1);
+		}
+		igt_info("Running the test on driver \"%s\", chipset mask %#0x\n",
+			 driver, chipset);
+
+		workload_wait = __workload_wait;
+		workload_priv = &proc;
+		__workload(spin_batch, device, NULL, &proc);
+	}
+
+	igt_subtest("unplug")
+		run_subtest(&device, chipset, driver, device_unplug,
+			    workload_wait, workload_priv);
+
+	igt_subtest("unbind")
+		run_subtest(&device, chipset, driver, driver_unbind,
+			    workload_wait, workload_priv);
+
+	igt_fixture {
+		free(driver);
+		close(device);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 5167a6cc..1b91e5a2 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -3,6 +3,7 @@ test_progs = [
 	'core_getclient',
 	'core_getstats',
 	'core_getversion',
+	'core_hot_reload',
 	'core_setmaster_vs_auth',
 	'debugfs_test',
 	'drm_import_export',
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Katarzyna Dec]

On Thu, Apr 11, 2019 at 02:26:28PM +0200, Janusz Krzysztofik wrote:
> From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> 
> Run a dummy load in background to put some workload on a device, then try
> to either remove (unplug) the device from its bus, or unbind the device's
> driver from it, depending on which subtest has been selected.  If
> succeeded, unload the driver, rescan the device's bus if needed and
> perform health checks on the device with the driver reloaded.
> 
> The dummy load is run from igt_fixture and in a sub-process, not directly
> from subtests,  as it is expected to fail and it's more simple to ignore
> igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
> and exits, resources it was using are freed automatically so there is no
> need to do any cleanups required for smooth module unload from the test
> level itself.  Those cleanups might also make the subtests fail if simply

> using igt library functions for that instead of reimplementing their safe
> parts only.
> 
> The driver hot unbind / device hot unplug operation is expected to succeed
> and the background workload sub-process to die in a reasonable time,
> however long timeouts are used to let kernel level timeouts pop up first
> if hit by a bug.
> 
> The dummy load works only on i915 driver.  The test is skipped on other
> hardware unless they provide their implementation of igt_spin_batch_new()
> and friends.
> 
There are few not needed spaces above, no need to send new version if this will
be the only thing (can be fixed during merging).
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> ---
>  tests/Makefile.sources  |   1 +
>  tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
>  tests/meson.build       |   1 +
>  3 files changed, 258 insertions(+)
>  create mode 100644 tests/core_hot_reload.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 214698da..d2c0941d 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -16,6 +16,7 @@ TESTS_progs = \
>  	core_getclient \
>  	core_getstats \
>  	core_getversion \
> +	core_hot_reload \
>  	core_setmaster_vs_auth \
>  	debugfs_test \
>  	drm_import_export \
> diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> new file mode 100644
> index 00000000..d862c99c
> --- /dev/null
> +++ b/tests/core_hot_reload.c
> @@ -0,0 +1,256 @@
> +/*
> + * Copyright © 2019 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.
> + */
> +
> +#include "igt.h"
> +#include "igt_device.h"
> +#include "igt_dummyload.h"
> +#include "igt_kmod.h"
> +#include "igt_sysfs.h"
> +
> +#include <getopt.h>
> +#include <limits.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +
I think that 1 blank line will be fine.
> +typedef int (*action_t)(int dir);
> +typedef void (*workload_wait_t)(void *priv);
> +typedef void (*workload_t)(int device, const void *priv);
> +
> +
same here :)
> +/*
> + * Actions
> + *
> + * Purpose: make the device disappear
> + *
> + * @dir: file descriptor of an open device sysfs directory
> + *
> + * Return value: file descriptor of an open device bus' sysfs directory
> + * 		 or -1 if no bus rescan is needed
> + */
> +
I went though your answers on my quesions. It is more then ok to have a wider
overview on what is going on in whole binary, although I would prefer to have
some summary doc at the begining and small docs aboe functions (if needed).

> +/* Unbind the driver from the device */
> +static int driver_unbind(int dir)
> +{
> +	char path[PATH_MAX], *dev_bus_addr;
> +	int len;
> +
> +	len = readlinkat(dir, "device", path, sizeof(path) - 1);
> +	path[len] = '\0';
> +	dev_bus_addr = strrchr(path, '/') + 1;
> +
> +	igt_set_timeout(60, "Driver unbind timeout!");
> +	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
> +
> +	/* No need for bus rescan */
> +	return -1;
> +}
> +
> +/* Remove (virtually unplug) the device from its bus */
> +static int device_unplug(int dir)
> +{
> +	int bus;
> +
> +	bus = openat(dir, "device/subsystem", O_DIRECTORY);
> +	igt_assert(bus >= 0);
> +
> +	igt_set_timeout(60, "Device unplug timeout!");
> +	igt_sysfs_set(dir, "device/remove", "1");
> +
> +	return bus;
> +}
> +
> +
> +/*
> + * Workloads
> + *
> + * Purpose: Put some long lasting load on the device
> + *
> + * @device: open device file descriptor,
> + * @priv: pointer to an optional argument passed to the workload
> + *
> + * Return value: none
> + */
> +
> +/* Workload using igt_spin_batch_run() */
> +
> +static void spin_batch(int device, const void *priv)
> +{
> +	igt_spin_t *spin;
> +
> +	/* submit the job */
> +	spin = igt_spin_batch_new(device);
> +
> +	/* wait for the job to crash */
> +	gem_sync(device, spin->handle);
> +
> +	/* clean up if still possible */
> +	igt_spin_batch_free(device, spin);
> +}
> +
> +
> +/*
> + * Skeleton
> + */
> +
> +static void healthcheck(int chipset)
> +{
> +	if (chipset == DRIVER_INTEL) {
> +		/*
> +		 * We have it perfectly implemented in i915_module_load,
> +		 * just use it.
> +		 */
> +		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
> +			   == IGT_EXIT_SUCCESS);
> +	} else {
> +		/*
> +		 * We don't know how to check an unidentified device for health,
> +		 * device reopen must suffice.
> +		 */
> +	}
I saw you answer about this else and I agree that it is not for us to decide
what should code for other vendors look like. That is why we do not need else
here at all - healthcheck will be run only for INTEL + person who runs code will
not see any message that 'if' here was not hit.

The rest looks fine.
Kasia :)
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

Hi Kasia,

On Friday, April 12, 2019 10:16:41 AM CEST Katarzyna Dec wrote:
> On Thu, Apr 11, 2019 at 02:26:28PM +0200, Janusz Krzysztofik wrote:
> > From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > 
> > Run a dummy load in background to put some workload on a device, then try
> > to either remove (unplug) the device from its bus, or unbind the device's
> > driver from it, depending on which subtest has been selected.  If
> > succeeded, unload the driver, rescan the device's bus if needed and
> > perform health checks on the device with the driver reloaded.
> > 
> > The dummy load is run from igt_fixture and in a sub-process, not directly
> > from subtests,  as it is expected to fail and it's more simple to ignore
> > igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
> > and exits, resources it was using are freed automatically so there is no
> > need to do any cleanups required for smooth module unload from the test
> > level itself.  Those cleanups might also make the subtests fail if simply
> > 
> > using igt library functions for that instead of reimplementing their safe
> > parts only.
> > 
> > The driver hot unbind / device hot unplug operation is expected to succeed
> > and the background workload sub-process to die in a reasonable time,
> > however long timeouts are used to let kernel level timeouts pop up first
> > if hit by a bug.
> > 
> > The dummy load works only on i915 driver.  The test is skipped on other
> > hardware unless they provide their implementation of igt_spin_batch_new()
> > and friends.
> 
> There are few not needed spaces above, no need to send new version if this
> will be the only thing (can be fixed during merging).

OK.

> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > ---
> > 
> >  tests/Makefile.sources  |   1 +
> >  tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
> >  tests/meson.build       |   1 +
> >  3 files changed, 258 insertions(+)
> >  create mode 100644 tests/core_hot_reload.c
> > 
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 214698da..d2c0941d 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -16,6 +16,7 @@ TESTS_progs = \
> > 
> >  	core_getclient \
> >  	core_getstats \
> >  	core_getversion \
> > 
> > +	core_hot_reload \
> > 
> >  	core_setmaster_vs_auth \
> >  	debugfs_test \
> >  	drm_import_export \
> > 
> > diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> > new file mode 100644
> > index 00000000..d862c99c
> > --- /dev/null
> > +++ b/tests/core_hot_reload.c
> > @@ -0,0 +1,256 @@
> > +/*
> > + * Copyright © 2019 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.
> > + */
> > +
> > +#include "igt.h"
> > +#include "igt_device.h"
> > +#include "igt_dummyload.h"
> > +#include "igt_kmod.h"
> > +#include "igt_sysfs.h"
> > +
> > +#include <getopt.h>
> > +#include <limits.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +
> > +
> 
> I think that 1 blank line will be fine.

OK.

> > +typedef int (*action_t)(int dir);
> > +typedef void (*workload_wait_t)(void *priv);
> > +typedef void (*workload_t)(int device, const void *priv);
> > +
> > +
> 
> same here :)

OK.

> > +/*
> > + * Actions
> > + *
> > + * Purpose: make the device disappear
> > + *
> > + * @dir: file descriptor of an open device sysfs directory
> > + *
> > + * Return value: file descriptor of an open device bus' sysfs directory
> > + * 		 or -1 if no bus rescan is needed
> > + */
> > +
> 
> I went though your answers on my quesions. It is more then ok to have a
> wider overview on what is going on in whole binary, although I would prefer
> to have some summary doc at the begining and small docs aboe functions (if
> needed).

Let's try to agree on that before I submit another version.

There are now 3 multiline comments, each of them open one of three sections of 
the code:
1) a section with different action functions,
2) a section with different workload functions,
3) a section with common code, I called it skeleton.

If I move all those three comments at the top, they will loose their purpose 
of opening the sections.  That's why I propose to keep them where they are 
now, and to add another comment at the top.  Here is how it may look like:

 /**
 * This binary includes subtests for performing the following two actions:
 * - unbinding the driver from a device under load,
 * - virtually unplugging the device under load from its bus.
 * Both actions are implemented as separate functions sharing the same API,
 * wrapped with a common skeleton.  A similar approach has been taken for
 * defining different workloads.  Even if only one type of workload is currently
 * implemented, the code is ready for extending it with other workloads defined
 * as additional workload functions which follow the same API.
 * Since the unplug action actually removes the device instance from the system,
 * a bus rescan operation is needed to reinstantiate the device after completion
 * of the test.  For that purpose, the action passes back a file descriptor of
 * an open sysfs directory representing the bus on which the rescan is then 
 * performed.  In case of the unbind action, it always returns an invalid file
 * descriptor to inform the skeleton that no bus rescan is needed.
 * Since a workload is expected to crash as a result of an action, its
 * invocation is performed from a helper process run from the igt_fixture block
 * in order to avoid direct influence of that crash on a test result.
 */

What do you think?

> > +/* Unbind the driver from the device */
> > +static int driver_unbind(int dir)
> > +{
> > +	char path[PATH_MAX], *dev_bus_addr;
> > +	int len;
> > +
> > +	len = readlinkat(dir, "device", path, sizeof(path) - 1);
> > +	path[len] = '\0';
> > +	dev_bus_addr = strrchr(path, '/') + 1;
> > +
> > +	igt_set_timeout(60, "Driver unbind timeout!");
> > +	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
> > +
> > +	/* No need for bus rescan */
> > +	return -1;
> > +}
> > +
> > +/* Remove (virtually unplug) the device from its bus */
> > +static int device_unplug(int dir)
> > +{
> > +	int bus;
> > +
> > +	bus = openat(dir, "device/subsystem", O_DIRECTORY);
> > +	igt_assert(bus >= 0);
> > +
> > +	igt_set_timeout(60, "Device unplug timeout!");
> > +	igt_sysfs_set(dir, "device/remove", "1");
> > +
> > +	return bus;
> > +}
> > +
> > +
> > +/*
> > + * Workloads
> > + *
> > + * Purpose: Put some long lasting load on the device
> > + *
> > + * @device: open device file descriptor,
> > + * @priv: pointer to an optional argument passed to the workload
> > + *
> > + * Return value: none
> > + */
> > +
> > +/* Workload using igt_spin_batch_run() */
> > +
> > +static void spin_batch(int device, const void *priv)
> > +{
> > +	igt_spin_t *spin;
> > +
> > +	/* submit the job */
> > +	spin = igt_spin_batch_new(device);
> > +
> > +	/* wait for the job to crash */
> > +	gem_sync(device, spin->handle);
> > +
> > +	/* clean up if still possible */
> > +	igt_spin_batch_free(device, spin);
> > +}
> > +
> > +
> > +/*
> > + * Skeleton
> > + */
> > +
> > +static void healthcheck(int chipset)
> > +{
> > +	if (chipset == DRIVER_INTEL) {
> > +		/*
> > +		 * We have it perfectly implemented in i915_module_load,
> > +		 * just use it.
> > +		 */
> > +		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
> > +			   == IGT_EXIT_SUCCESS);
> > +	} else {
> > +		/*
> > +		 * We don't know how to check an unidentified device for health,
> > +		 * device reopen must suffice.
> > +		 */
> > +	}
> 
> I saw you answer about this else and I agree that it is not for us to decide
> what should code for other vendors look like. That is why we do not need
> else here at all

OK.

Janusz

> - healthcheck will be run only for INTEL + person who runs
> code will not see any message that 'if' here was not hit.
> 
> The rest looks fine.
> Kasia :)




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Katarzyna Dec]

On Tue, Apr 16, 2019 at 09:17:21AM +0200, Janusz Krzysztofik wrote:
> Hi Kasia,
> 
> On Friday, April 12, 2019 10:16:41 AM CEST Katarzyna Dec wrote:
> > On Thu, Apr 11, 2019 at 02:26:28PM +0200, Janusz Krzysztofik wrote:
> > > From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > > 
> > > Run a dummy load in background to put some workload on a device, then try
> > > to either remove (unplug) the device from its bus, or unbind the device's
> > > driver from it, depending on which subtest has been selected.  If
> > > succeeded, unload the driver, rescan the device's bus if needed and
> > > perform health checks on the device with the driver reloaded.
> > > 
> > > The dummy load is run from igt_fixture and in a sub-process, not directly
> > > from subtests,  as it is expected to fail and it's more simple to ignore
> > > igt_abort() in a sub-process.  Moreover, as soon as the sub-process fails
> > > and exits, resources it was using are freed automatically so there is no
> > > need to do any cleanups required for smooth module unload from the test
> > > level itself.  Those cleanups might also make the subtests fail if simply
> > > 
> > > using igt library functions for that instead of reimplementing their safe
> > > parts only.
> > > 
> > > The driver hot unbind / device hot unplug operation is expected to succeed
> > > and the background workload sub-process to die in a reasonable time,
> > > however long timeouts are used to let kernel level timeouts pop up first
> > > if hit by a bug.
> > > 
> > > The dummy load works only on i915 driver.  The test is skipped on other
> > > hardware unless they provide their implementation of igt_spin_batch_new()
> > > and friends.
> > 
> > There are few not needed spaces above, no need to send new version if this
> > will be the only thing (can be fixed during merging).
> 
> OK.
> 
> > > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
> > > ---
> > > 
> > >  tests/Makefile.sources  |   1 +
> > >  tests/core_hot_reload.c | 256 ++++++++++++++++++++++++++++++++++++++++
> > >  tests/meson.build       |   1 +
> > >  3 files changed, 258 insertions(+)
> > >  create mode 100644 tests/core_hot_reload.c
> > > 
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index 214698da..d2c0941d 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -16,6 +16,7 @@ TESTS_progs = \
> > > 
> > >  	core_getclient \
> > >  	core_getstats \
> > >  	core_getversion \
> > > 
> > > +	core_hot_reload \
> > > 
> > >  	core_setmaster_vs_auth \
> > >  	debugfs_test \
> > >  	drm_import_export \
> > > 
> > > diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
> > > new file mode 100644
> > > index 00000000..d862c99c
> > > --- /dev/null
> > > +++ b/tests/core_hot_reload.c
> > > @@ -0,0 +1,256 @@
> > > +/*
> > > + * Copyright © 2019 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.
> > > + */
> > > +
> > > +#include "igt.h"
> > > +#include "igt_device.h"
> > > +#include "igt_dummyload.h"
> > > +#include "igt_kmod.h"
> > > +#include "igt_sysfs.h"
> > > +
> > > +#include <getopt.h>
> > > +#include <limits.h>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +
> > > +
> > 
> > I think that 1 blank line will be fine.
> 
> OK.
> 
> > > +typedef int (*action_t)(int dir);
> > > +typedef void (*workload_wait_t)(void *priv);
> > > +typedef void (*workload_t)(int device, const void *priv);
> > > +
> > > +
> > 
> > same here :)
> 
> OK.
> 
> > > +/*
> > > + * Actions
> > > + *
> > > + * Purpose: make the device disappear
> > > + *
> > > + * @dir: file descriptor of an open device sysfs directory
> > > + *
> > > + * Return value: file descriptor of an open device bus' sysfs directory
> > > + * 		 or -1 if no bus rescan is needed
> > > + */
> > > +
> > 
> > I went though your answers on my quesions. It is more then ok to have a
> > wider overview on what is going on in whole binary, although I would prefer
> > to have some summary doc at the begining and small docs aboe functions (if
> > needed).
> 
> Let's try to agree on that before I submit another version.
> 
> There are now 3 multiline comments, each of them open one of three sections of 
> the code:
> 1) a section with different action functions,
> 2) a section with different workload functions,
> 3) a section with common code, I called it skeleton.
> 
> If I move all those three comments at the top, they will loose their purpose 
> of opening the sections.  That's why I propose to keep them where they are 
> now, and to add another comment at the top.  Here is how it may look like:
> 
>  /**
>  * This binary includes subtests for performing the following two actions:
>  * - unbinding the driver from a device under load,
>  * - virtually unplugging the device under load from its bus.
>  * Both actions are implemented as separate functions sharing the same API,
>  * wrapped with a common skeleton.  A similar approach has been taken for
>  * defining different workloads.  Even if only one type of workload is currently
>  * implemented, the code is ready for extending it with other workloads defined
>  * as additional workload functions which follow the same API.
>  * Since the unplug action actually removes the device instance from the system,
>  * a bus rescan operation is needed to reinstantiate the device after completion
>  * of the test.  For that purpose, the action passes back a file descriptor of
>  * an open sysfs directory representing the bus on which the rescan is then 
>  * performed.  In case of the unbind action, it always returns an invalid file
>  * descriptor to inform the skeleton that no bus rescan is needed.
>  * Since a workload is expected to crash as a result of an action, its
>  * invocation is performed from a helper process run from the igt_fixture block
>  * in order to avoid direct influence of that crash on a test result.
>  */
> 
> What do you think?
It looks a little too long, but it is ok :).
> 
> > > +/* Unbind the driver from the device */
> > > +static int driver_unbind(int dir)
> > > +{
> > > +	char path[PATH_MAX], *dev_bus_addr;
> > > +	int len;
> > > +
> > > +	len = readlinkat(dir, "device", path, sizeof(path) - 1);
> > > +	path[len] = '\0';
> > > +	dev_bus_addr = strrchr(path, '/') + 1;
> > > +
> > > +	igt_set_timeout(60, "Driver unbind timeout!");
> > > +	igt_sysfs_set(dir, "device/driver/unbind", dev_bus_addr);
> > > +
> > > +	/* No need for bus rescan */
> > > +	return -1;
> > > +}
> > > +
> > > +/* Remove (virtually unplug) the device from its bus */
> > > +static int device_unplug(int dir)
> > > +{
> > > +	int bus;
> > > +
> > > +	bus = openat(dir, "device/subsystem", O_DIRECTORY);
> > > +	igt_assert(bus >= 0);
> > > +
> > > +	igt_set_timeout(60, "Device unplug timeout!");
> > > +	igt_sysfs_set(dir, "device/remove", "1");
> > > +
> > > +	return bus;
> > > +}
> > > +
> > > +
> > > +/*
> > > + * Workloads
> > > + *
> > > + * Purpose: Put some long lasting load on the device
> > > + *
> > > + * @device: open device file descriptor,
> > > + * @priv: pointer to an optional argument passed to the workload
> > > + *
> > > + * Return value: none
> > > + */
> > > +
> > > +/* Workload using igt_spin_batch_run() */
> > > +
> > > +static void spin_batch(int device, const void *priv)
> > > +{
> > > +	igt_spin_t *spin;
> > > +
> > > +	/* submit the job */
> > > +	spin = igt_spin_batch_new(device);
> > > +
> > > +	/* wait for the job to crash */
> > > +	gem_sync(device, spin->handle);
> > > +
> > > +	/* clean up if still possible */
> > > +	igt_spin_batch_free(device, spin);
> > > +}
> > > +
> > > +
> > > +/*
> > > + * Skeleton
> > > + */
> > > +
> > > +static void healthcheck(int chipset)
> > > +{
> > > +	if (chipset == DRIVER_INTEL) {
> > > +		/*
> > > +		 * We have it perfectly implemented in i915_module_load,
> > > +		 * just use it.
> > > +		 */
> > > +		igt_assert(igt_system_quiet("i915_module_load --run-subtest reload")
> > > +			   == IGT_EXIT_SUCCESS);
> > > +	} else {
> > > +		/*
> > > +		 * We don't know how to check an unidentified device for health,
> > > +		 * device reopen must suffice.
> > > +		 */
> > > +	}
> > 
> > I saw you answer about this else and I agree that it is not for us to decide
> > what should code for other vendors look like. That is why we do not need
> > else here at all
> 
> OK.
> 
> Janusz
> 
> > - healthcheck will be run only for INTEL + person who runs
> > code will not see any message that 'if' here was not hit.
> > 
> > The rest looks fine.
> > Kasia :)
> 
> 
>
We have a discussion at the office together with knr about this binary. The
outcome was:
* we should not run test from inside this one, so should avoid cases when we run 
'i915_module_load' inside healthcheck, you should add test code - exactly the
functions that you need.
* It would be nice to have bind/unbind scenario with DRIVER_ANY located
somewhere in igt/tests/* (vendor agnostic)
* we have also doubts about second patch. Even if it looks cool, it cannot be
probably used by CI. And if CI is not using it, most probably developers will
not be using it too.

For more questions/clarifications or concerns, please start discussion on
intel-gfx pointing people that you need :) It is the fastest way to have
feedback.

Kasia :)
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH v7 1/2] tests: Add a new test for driver/device hot reload

[Janusz Krzysztofik]

On Wednesday, April 17, 2019 10:53:05 AM CEST Katarzyna Dec wrote:
> On Tue, Apr 16, 2019 at 09:17:21AM +0200, Janusz Krzysztofik wrote:
> > ...
> > On Friday, April 12, 2019 10:16:41 AM CEST Katarzyna Dec wrote:
> > > On Thu, Apr 11, 2019 at 02:26:28PM +0200, Janusz Krzysztofik wrote:
> > > > ...
> > > > +/*
> > > > + * Actions
> > > > + *
> > > > + * Purpose: make the device disappear
> > > > + *
> > > > + * @dir: file descriptor of an open device sysfs directory
> > > > + *
> > > > + * Return value: file descriptor of an open device bus' sysfs
> > > > directory
> > > > + * 		 or -1 if no bus rescan is needed
> > > > + */
> > > > +
> > > 
> > > I went though your answers on my quesions. It is more then ok to have a
> > > wider overview on what is going on in whole binary, although I would
> > > prefer
> > > to have some summary doc at the begining and small docs aboe functions
> > > (if
> > > needed).
> > 
> > Let's try to agree on that before I submit another version.
> > 
> > There are now 3 multiline comments, each of them open one of three
> > sections of the code:
> > 1) a section with different action functions,
> > 2) a section with different workload functions,
> > 3) a section with common code, I called it skeleton.
> > 
> > If I move all those three comments at the top, they will loose their
> > purpose of opening the sections.  That's why I propose to keep them where
> > they are> 
> > now, and to add another comment at the top.  Here is how it may look like:
> >  /**
> >  * This binary includes subtests for performing the following two actions:
> >  * - unbinding the driver from a device under load,
> >  * - virtually unplugging the device under load from its bus.
> >  * Both actions are implemented as separate functions sharing the same
> >  API,
> >  * wrapped with a common skeleton.  A similar approach has been taken for
> >  * defining different workloads.  Even if only one type of workload is
> >  currently * implemented, the code is ready for extending it with other
> >  workloads defined * as additional workload functions which follow the
> >  same API.
> >  * Since the unplug action actually removes the device instance from the
> >  system, * a bus rescan operation is needed to reinstantiate the device
> >  after completion * of the test.  For that purpose, the action passes
> >  back a file descriptor of * an open sysfs directory representing the bus
> >  on which the rescan is then * performed.  In case of the unbind action,
> >  it always returns an invalid file * descriptor to inform the skeleton
> >  that no bus rescan is needed.
> >  * Since a workload is expected to crash as a result of an action, its
> >  * invocation is performed from a helper process run from the igt_fixture
> >  block * in order to avoid direct influence of that crash on a test
> >  result. */
> > 
> > What do you think?
> 
> It looks a little too long, but it is ok :).

OK, I'll see if I can make it shorter.

> > > > ...
> > > > +static void healthcheck(int chipset)
> > > > +{
> > > > +	if (chipset == DRIVER_INTEL) {
> > > > +		/*
> > > > +		 * We have it perfectly implemented in i915_module_load,
> > > > +		 * just use it.
> > > > +		 */
> > > > +		igt_assert(igt_system_quiet("i915_module_load --run-
subtest
> > > > reload")
> > > > +			   == IGT_EXIT_SUCCESS);
> > > > ...
> 
> We have a discussion at the office together with knr about this binary.

Yes, I know, and I've been waiting for some response from Michał with results 
of that discussion, as he has promised, before going any further.  I'm 
assuming this message provides that information.

> The
> outcome was:
> * we should not run test from inside this one, so should avoid cases when we
> run 'i915_module_load' inside healthcheck, you should add test code -
> exactly the functions that you need.

With my still not enough experience with i915 I don't really know what I need 
but I've already had a suggestion on that from Daniel to follow:

On Tuesday, April 2, 2019 10:47:20 AM CEST Daniel Vetter wrote:
> ... good sanity check is to issue a batch with an MI write and check it 
> still completes and does something ...

so I'll try to implement it instead of calling i915_module_load.

> * It would be nice to have bind/unbind scenario with DRIVER_ANY located
> somewhere in igt/tests/* (vendor agnostic)

I didn't know about igt/tests/ being the right place to put vendor agnostic 
tests, I thought that tests/ was the right location, with subdirs dedicated to 
vendors.  But anyway, does that mean you prefer a separate source file for 
each vendor specific (or agnostic) implementation, regardless of possibly high 
code redundancy?  If that's the case then maybe I should consider moving some 
of that under lib/ for reuse by those separate implementations (that would 
take additional time, of course).

> * we have also doubts about second patch. Even if it looks cool, it cannot
> be probably used by CI. And if CI is not using it, most probably developers
> will not be using it too.

OK, I'll drop it from the series if you think it's useless.  Since I think I 
may be using it anyway, I'll keep it in my private branch :-)

Regarding additional workloads, not mentioned here but a few times before by 
Daniel and Chris, I fully understand they are needed (that's why I proposed 
that workaround with optional external workloads as a quick win).  However, 
for them to be implemented in a way consistent with subtest naming convention 
as I see it, I first need to move workload invocation back to subtests.  For 
that, I need to reimplement some of existing library functions now containing 
igt_assert()s etc. so obviously expected workload crashes inside subtests 
won't affect test results in any way.  While working on that, I'd like to put 
possibly reusable modified functions inside libraries, not only in this 
particular test sources.  That will certainly take still more time.

> For more questions/clarifications or concerns, please start discussion on
> intel-gfx pointing people that you need :) It is the fastest way to have
> feedback.

OK

Thanks,
Janusz

> 
> Kasia :)




_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH v7 2/2] tests/core_hot_reload: Accept external workload

[Janusz Krzysztofik]

From: Janusz Krzysztofik <janusz.krzysztofik@intel.com>

Run a user specified command, possibly one of existing tests, in
background instead of the default dummy load to put some alternative
workload on a device while trying to either remove (unplug) the device
from its bus, or unbind the device's driver from it, depending on which
subtest has been selected.

The command selected as the alternative workload should make real use of
the device as much as possible and its execution should take
significantly more than 2 seconds in order to get reliable results from
the test.

Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@intel.com>
---
 tests/core_hot_reload.c | 53 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/tests/core_hot_reload.c b/tests/core_hot_reload.c
index d862c99c..4a902ce4 100644
--- a/tests/core_hot_reload.c
+++ b/tests/core_hot_reload.c
@@ -33,6 +33,9 @@
 #include <unistd.h>
 
 
+#define OPT_WORKLOAD_CMD	'c'
+
+
 typedef int (*action_t)(int dir);
 typedef void (*workload_wait_t)(void *priv);
 typedef void (*workload_t)(int device, const void *priv);
@@ -108,6 +111,16 @@ static void spin_batch(int device, const void *priv)
 	igt_spin_batch_free(device, spin);
 }
 
+/* Workload using external command */
+
+static void ext_cmd(int device, const void *priv)
+{
+	const char *cmd = priv;
+
+	/* just run the user provided command line */
+	igt_system_quiet(cmd);
+}
+
 
 /*
  * Skeleton
@@ -200,12 +213,40 @@ static void __workload_wait(void *priv)
 }
 
 
-igt_main {
+static int opt_handler(int opt, int opt_index, void *data)
+{
+	const char **cmd = data;
+
+	switch (opt) {
+	case 'c':
+		*cmd = strdup(optarg);
+		break;
+	}
+
+	return 0;
+}
+
+int main (int argc, char **argv)
+{
 	int device, chipset;
 	char *driver;
 	struct igt_helper_process proc = {};
 	workload_wait_t workload_wait;
 	void *workload_priv;
+	const struct option long_opts[] = {
+		{ "workload-cmd", required_argument, 0, OPT_WORKLOAD_CMD },
+		{ 0, 0, 0, 0 }
+	};
+	const char *help_str =
+		"  --workload-cmd, -c\n"
+		"\t\tCommand line to run in backgroud as an optional workload, e.g.,\n"
+		"\t\tan existing test name, possibly with options/arguments, quoted.\n"
+		"\t\tExecution must take way more than 2 seconds for reliable results.\n"
+		"\t\tNo default - internal dummy load is used if external not specified.\n";
+	const char *cmd = NULL;
+
+	igt_subtest_init_parse_opts(&argc, argv, "", long_opts, help_str,
+                                    opt_handler, &cmd);
 
 	igt_fixture {
 		char path[PATH_MAX];
@@ -238,7 +279,13 @@ igt_main {
 
 		workload_wait = __workload_wait;
 		workload_priv = &proc;
-		__workload(spin_batch, device, NULL, &proc);
+
+		if (cmd) {
+			igt_device_drop_master(device);
+			__workload(ext_cmd, -1, cmd, &proc);
+		} else {
+			__workload(spin_batch, device, NULL, &proc);
+		}
 	}
 
 	igt_subtest("unplug")
@@ -253,4 +300,6 @@ igt_main {
 		free(driver);
 		close(device);
 	}
+
+	igt_exit();
 }
-- 
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev