Re: [PATCH i-g-t 2/4 v4] lib/igt_gvt: Make use of libkmod helpers and fix reading gvt parameter.

[Chris Wilson <chris@chris-wilson.co.uk>]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx