| Message ID | 20161028093129.13275-3-marius.c.vlad@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 28, 2016 at 12:31:27PM +0300, Marius Vlad wrote: > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > --- > lib/igt_gvt.c | 42 +++++++++++++++++++++++++++++++++++------- > tests/gvt_basic.c | 2 +- > 2 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/lib/igt_gvt.c b/lib/igt_gvt.c > index 8bbf9bd..d868cb3 100644 > --- a/lib/igt_gvt.c > +++ b/lib/igt_gvt.c > @@ -24,23 +24,26 @@ > #include "igt.h" > #include "igt_gvt.h" > #include "igt_sysfs.h" > +#include "igt_kmod.h" > > +#include <signal.h> > #include <dirent.h> > #include <unistd.h> > #include <fcntl.h> > +#include <time.h> > > static bool is_gvt_enabled(void) > { > FILE *file; > - int value; > + char value; > bool enabled = false; > > file = fopen("/sys/module/i915/parameters/enable_gvt", "r"); > if (!file) > return false; > > - if (fscanf(file, "%d", &value) == 1) > - enabled = value; > + if (fscanf(file, "%c", &value) == 1) > + enabled = (value == 'Y' ? true : false); enabled = value == 'Y'; else if (fscanf(file, "%d", &value) == 1) enabled = value; Do I see a igt_kmod_parameter_get_boolean() in the future, I think I do! > fclose(file); > > errno = 0; > @@ -50,9 +53,20 @@ static bool is_gvt_enabled(void) > static void unload_i915(void) > { > kick_fbcon(false); > - /* pkill alsact */ > > - igt_ignore_warn(system("/sbin/modprobe -s -r i915")); > + > + if (igt_kmod_is_loaded("i915")) { > + > + if (igt_kmod_is_loaded("snd_hda_intel")) { > + igt_assert(!igt_pkill(SIGTERM, "alsactl")); > + igt_assert(!igt_kmod_unload("snd_hda_intel", 0)); > + } > + > + igt_assert(!igt_kmod_unload("i915", 0)); > + igt_assert(!igt_kmod_unload("drm_kms_helper", 0)); > + igt_assert(!igt_kmod_unload("drm", 0)); But don't we already have this routine... For this test, we shouldn't fail if we can't setup the environment as we need, but skip the test. -Chris
On Fri, Oct 28, 2016 at 11:08:19AM +0100, Chris Wilson wrote: > On Fri, Oct 28, 2016 at 12:31:27PM +0300, Marius Vlad wrote: > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > --- > > lib/igt_gvt.c | 42 +++++++++++++++++++++++++++++++++++------- > > tests/gvt_basic.c | 2 +- > > 2 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/lib/igt_gvt.c b/lib/igt_gvt.c > > index 8bbf9bd..d868cb3 100644 > > --- a/lib/igt_gvt.c > > +++ b/lib/igt_gvt.c > > @@ -24,23 +24,26 @@ > > #include "igt.h" > > #include "igt_gvt.h" > > #include "igt_sysfs.h" > > +#include "igt_kmod.h" > > > > +#include <signal.h> > > #include <dirent.h> > > #include <unistd.h> > > #include <fcntl.h> > > +#include <time.h> > > > > static bool is_gvt_enabled(void) > > { > > FILE *file; > > - int value; > > + char value; > > bool enabled = false; > > > > file = fopen("/sys/module/i915/parameters/enable_gvt", "r"); > > if (!file) > > return false; > > > > - if (fscanf(file, "%d", &value) == 1) > > - enabled = value; > > + if (fscanf(file, "%c", &value) == 1) > > + enabled = (value == 'Y' ? true : false); > enabled = value == 'Y'; > else if (fscanf(file, "%d", &value) == 1) > enabled = value; > > Do I see a igt_kmod_parameter_get_boolean() in the future, I think I do! > > > fclose(file); > > > > errno = 0; > > @@ -50,9 +53,20 @@ static bool is_gvt_enabled(void) > > static void unload_i915(void) > > { > > kick_fbcon(false); > > - /* pkill alsact */ > > > > - igt_ignore_warn(system("/sbin/modprobe -s -r i915")); > > + > > + if (igt_kmod_is_loaded("i915")) { > > + > > + if (igt_kmod_is_loaded("snd_hda_intel")) { > > + igt_assert(!igt_pkill(SIGTERM, "alsactl")); > > + igt_assert(!igt_kmod_unload("snd_hda_intel", 0)); > > + } > > + > > + igt_assert(!igt_kmod_unload("i915", 0)); > > + igt_assert(!igt_kmod_unload("drm_kms_helper", 0)); > > + igt_assert(!igt_kmod_unload("drm", 0)); > > But don't we already have this routine... No. But I guess I can add a driver_load()/driver_unload() in lib/igt_kmod which does that, if that is what you meant. > > For this test, we shouldn't fail if we can't setup the environment as we > need, but skip the test. Right, maybe I got it wrong, but you call igt_require(igt_gvt_load_module()) which in turn calls is_gvt_enabled(). igt_require will skip if igt_gvt_load_module() is false. That is why I kept like that. Besides that, I haven't checked what happens when the modules are built-in, so the assert might fail in that case. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Sun, Oct 30, 2016 at 02:47:56PM +0200, Marius Vlad wrote: > On Fri, Oct 28, 2016 at 11:08:19AM +0100, Chris Wilson wrote: > > On Fri, Oct 28, 2016 at 12:31:27PM +0300, Marius Vlad wrote: > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > --- > > > lib/igt_gvt.c | 42 +++++++++++++++++++++++++++++++++++------- > > > tests/gvt_basic.c | 2 +- > > > 2 files changed, 36 insertions(+), 8 deletions(-) > > > > > > diff --git a/lib/igt_gvt.c b/lib/igt_gvt.c > > > index 8bbf9bd..d868cb3 100644 > > > --- a/lib/igt_gvt.c > > > +++ b/lib/igt_gvt.c > > > @@ -24,23 +24,26 @@ > > > #include "igt.h" > > > #include "igt_gvt.h" > > > #include "igt_sysfs.h" > > > +#include "igt_kmod.h" > > > > > > +#include <signal.h> > > > #include <dirent.h> > > > #include <unistd.h> > > > #include <fcntl.h> > > > +#include <time.h> > > > > > > static bool is_gvt_enabled(void) > > > { > > > FILE *file; > > > - int value; > > > + char value; > > > bool enabled = false; > > > > > > file = fopen("/sys/module/i915/parameters/enable_gvt", "r"); > > > if (!file) > > > return false; > > > > > > - if (fscanf(file, "%d", &value) == 1) > > > - enabled = value; > > > + if (fscanf(file, "%c", &value) == 1) > > > + enabled = (value == 'Y' ? true : false); > > enabled = value == 'Y'; > > else if (fscanf(file, "%d", &value) == 1) > > enabled = value; > > > > Do I see a igt_kmod_parameter_get_boolean() in the future, I think I do! > > > > > fclose(file); > > > > > > errno = 0; > > > @@ -50,9 +53,20 @@ static bool is_gvt_enabled(void) > > > static void unload_i915(void) > > > { > > > kick_fbcon(false); > > > - /* pkill alsact */ > > > > > > - igt_ignore_warn(system("/sbin/modprobe -s -r i915")); > > > + > > > + if (igt_kmod_is_loaded("i915")) { > > > + > > > + if (igt_kmod_is_loaded("snd_hda_intel")) { > > > + igt_assert(!igt_pkill(SIGTERM, "alsactl")); > > > + igt_assert(!igt_kmod_unload("snd_hda_intel", 0)); > > > + } > > > + > > > + igt_assert(!igt_kmod_unload("i915", 0)); > > > + igt_assert(!igt_kmod_unload("drm_kms_helper", 0)); > > > + igt_assert(!igt_kmod_unload("drm", 0)); > > > > But don't we already have this routine... > > No. But I guess I can add a driver_load()/driver_unload() in > lib/igt_kmod which does that, if that is what you meant. This is the same sequence as required for reloading any kms driver. However, note here we only require i915 to be reloaded, we can avoid taking further risk in reloading the drm layer. So perhaps we don't want the same code here as drv_module_reload. (Or a more complete interface that gives finer control.) > > > > For this test, we shouldn't fail if we can't setup the environment as we > > need, but skip the test. > > Right, maybe I got it wrong, but you call > igt_require(igt_gvt_load_module()) which in turn calls is_gvt_enabled(). > igt_require will skip if igt_gvt_load_module() is false. That is why I > kept like that. Besides that, I haven't checked what happens when the > modules are built-in, so the assert might fail in that case. The asserts are indeed testing something unrelated to the test, only the setup of its environment. If we can't set up the environment for the test, we skip. -Chris
diff --git a/lib/igt_gvt.c b/lib/igt_gvt.c index 8bbf9bd..d868cb3 100644 --- a/lib/igt_gvt.c +++ b/lib/igt_gvt.c @@ -24,23 +24,26 @@ #include "igt.h" #include "igt_gvt.h" #include "igt_sysfs.h" +#include "igt_kmod.h" +#include <signal.h> #include <dirent.h> #include <unistd.h> #include <fcntl.h> +#include <time.h> static bool is_gvt_enabled(void) { FILE *file; - int value; + char value; bool enabled = false; file = fopen("/sys/module/i915/parameters/enable_gvt", "r"); if (!file) return false; - if (fscanf(file, "%d", &value) == 1) - enabled = value; + if (fscanf(file, "%c", &value) == 1) + enabled = (value == 'Y' ? true : false); fclose(file); errno = 0; @@ -50,9 +53,20 @@ static bool is_gvt_enabled(void) static void unload_i915(void) { kick_fbcon(false); - /* pkill alsact */ - igt_ignore_warn(system("/sbin/modprobe -s -r i915")); + + if (igt_kmod_is_loaded("i915")) { + + if (igt_kmod_is_loaded("snd_hda_intel")) { + igt_assert(!igt_pkill(SIGTERM, "alsactl")); + igt_assert(!igt_kmod_unload("snd_hda_intel", 0)); + } + + igt_assert(!igt_kmod_unload("i915", 0)); + igt_assert(!igt_kmod_unload("drm_kms_helper", 0)); + igt_assert(!igt_kmod_unload("drm", 0)); + } + } bool igt_gvt_load_module(void) @@ -61,8 +75,15 @@ bool igt_gvt_load_module(void) return true; unload_i915(); - igt_ignore_warn(system("/sbin/modprobe -s i915 enable_gvt=1")); + igt_assert(!igt_kmod_load("drm", NULL)); + igt_assert(!igt_kmod_load("drm_kms_helper", NULL)); + + igt_assert(!igt_kmod_load("i915", "enable_gvt=1")); + + kick_fbcon(true); + + igt_assert(!igt_kmod_load("snd_hda_intel", NULL)); return is_gvt_enabled(); } @@ -72,7 +93,14 @@ void igt_gvt_unload_module(void) return; unload_i915(); - igt_ignore_warn(system("/sbin/modprobe -s i915 enable_gvt=0")); + + igt_assert(!igt_kmod_load("drm", NULL)); + igt_assert(!igt_kmod_load("drm_kms_helper", NULL)); + + igt_assert(!igt_kmod_load("i915", "enable_gvt=0")); igt_assert(!is_gvt_enabled()); + + kick_fbcon(true); + igt_assert(!igt_kmod_load("snd_hda_intel", NULL)); } diff --git a/tests/gvt_basic.c b/tests/gvt_basic.c index 48b853a..4e909a5 100644 --- a/tests/gvt_basic.c +++ b/tests/gvt_basic.c @@ -32,7 +32,7 @@ igt_main igt_fixture { igt_require(igt_gvt_load_module()); - fd = drm_open_driver(DRIVER_INTEL); + fd = __drm_open_driver(DRIVER_INTEL); } igt_subtest_f("invalid-placeholder-test");
Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> --- lib/igt_gvt.c | 42 +++++++++++++++++++++++++++++++++++------- tests/gvt_basic.c | 2 +- 2 files changed, 36 insertions(+), 8 deletions(-)