Re: [PATCH i-g-t] [RFC] Add support to force specific module load

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 29, 2018 at 09:45:20PM -0300, Rodrigo Siqueira wrote:
> This patch adds a new option to force the use of a module specified from
> the command line.  The force command expects a module name which will be
> used on the target test (changing the standard behavior). This feature
> can be useful for developers that have to create a new module since it
> is possible to use some of the tests already provided by IGT (e.g.,
> kms_color, core, etc.) as a start point. Additionally, it can
> also be useful for someone that wants to implement a new set of tests
> for a specific driver because the developer can first check the behavior
> of any test in the target module. It is important to highlight, that a
> force option can produce unexpected results and developers should be
> aware of that.


Is this meant for testing replacement drivers for hardware with
already existing drivers? If not, I'm not sure what the goal is here.

Setting a forced module target in this patch changes which module is
loaded by the kernel, but the driver that's opened by IGT is
unchanged. Force-loading my-fancy-driver.ko still makes
drm_open_driver(DRIVER_INTEL) open the one driven by i915.ko, and
drm_open_driver(DRIVER_ANY) still opens the first one that is
recognized.

If this is for testing new drivers for not-already-supported devices,
you need to instead force what drm_open_driver(DRIVER_ANY) will open,
and not reject unknown devices.

Some additional drive-by feedback below.


> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index fae6f86f..1d2ba178 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -258,6 +258,7 @@ static int __open_device(const char *base, int offset, unsigned int chipset)
>  static int __open_driver(const char *base, int offset, unsigned int chipset)
>  {
>  	static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
> +	const char* target;
>  	static const struct module {
>  		unsigned int bit;
>  		const char *module;
> @@ -276,13 +277,19 @@ static int __open_driver(const char *base, int offset, unsigned int chipset)
>  	if (fd != -1)
>  		return fd;
>  
> +	target = get_target_module();
> +
>  	pthread_mutex_lock(&mutex);
> -	for (const struct module *m = modules; m->module; m++) {
> -		if (chipset & m->bit) {
> -			if (m->modprobe)
> -				m->modprobe(m->module);
> -			else
> -				modprobe(m->module);
> +	if (target) {
> +		modprobe(target);
> +	} else {
> +		for (const struct module *m = modules; m->module; m++) {
> +			if (chipset & m->bit) {
> +				if (m->modprobe)
> +					m->modprobe(m->module);
> +				else
> +					modprobe(m->module);
> +			}
>  		}
>  	}
>  	pthread_mutex_unlock(&mutex);
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 5092a3f0..ed66eae1 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -286,6 +286,7 @@ enum {
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
>   OPT_INTERACTIVE_DEBUG,
> + OPT_FORCE_MODULE,
>   OPT_HELP = 'h'
>  };
>  
> @@ -303,6 +304,20 @@ static pthread_mutex_t log_buffer_mutex = PTHREAD_MUTEX_INITIALIZER;
>  GKeyFile *igt_key_file;
>  #endif
>  
> +char *target_module;
> +
> +void set_target_module(char *target)
> +{
> +  if (!target)
> +    igt_warn("No module specified, keep default behaviour");
> +  target_module = target;
> +}
> +
> +const char *get_target_module(void)
> +{
> +  return target_module;
> +}
> +
>  char *igt_frame_dump_path;
>  
>  const char *igt_test_name(void)
> @@ -555,6 +570,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>  		   "  --debug[=log-domain]\n"
>  		   "  --interactive-debug[=domain]\n"
>  		   "  --help-description\n"
> +		   "  --force-module <module>\n"
>  		   "  --help\n");
>  	if (help_str)
>  		fprintf(f, "%s\n", help_str);
> @@ -666,6 +682,7 @@ static int common_init(int *argc, char **argv,
>  		{"debug", optional_argument, 0, OPT_DEBUG},
>  		{"interactive-debug", optional_argument, 0, OPT_INTERACTIVE_DEBUG},
>  		{"help", 0, 0, OPT_HELP},
> +		{"force-module", required_argument, 0, OPT_FORCE_MODULE},
>  		{0, 0, 0, 0}
>  	};
>  	char *short_opts;
> @@ -763,6 +780,9 @@ static int common_init(int *argc, char **argv,
>  			print_test_description();
>  			ret = -1;
>  			goto out;
> +		case OPT_FORCE_MODULE:
> +			set_target_module(strdup(optarg));
> +			break;
>  		case OPT_HELP:
>  			print_usage(help_str, false);
>  			ret = -1;
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index bf8cd66c..6d635ebd 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -50,6 +50,10 @@ extern const char* __igt_test_description __attribute__((weak));
>  extern bool __igt_plain_output;
>  extern char *igt_frame_dump_path;
>  
> +extern char *target_module;
> +void set_target_module(char *target);
> +const char *get_target_module(void);
> +
>  /**
>   * IGT_TEST_DESCRIPTION:
>   * @str: description string
> diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh
> index 4209dd8c..c810a8dd 100755
> --- a/scripts/run-tests.sh
> +++ b/scripts/run-tests.sh
> @@ -80,6 +80,7 @@ function print_help {
>  	echo "                  (can be used more than once)"
>  	echo "  -T <filename>   run tests listed in testlist"
>  	echo "                  (overrides -t and -x)"
> +	echo "  -m <module>     force load a specific module"
>  	echo "  -v              enable verbose mode"
>  	echo "  -x <regex>      exclude tests that match the regular expression"
>  	echo "                  (can be used more than once)"
> @@ -91,7 +92,7 @@ function print_help {
>  	echo "Useful patterns for test filtering are described in the API documentation."
>  }
>  
> -while getopts ":dhlr:st:T:vx:Rn" opt; do
> +while getopts ":dhlr:st:T:m:vx:Rn" opt; do
>  	case $opt in
>  		d) download_piglit; exit ;;
>  		h) print_help; exit ;;
> @@ -100,6 +101,7 @@ while getopts ":dhlr:st:T:vx:Rn" opt; do
>  		s) SUMMARY="html" ;;
>  		t) FILTER="$FILTER -t $OPTARG" ;;
>  		T) FILTER="$FILTER --test-list $OPTARG" ;;
> +		m) FILTER="$FILTER -m $OPTARG" ;;


This will change what is passed to _piglit_ command line, not the test
command line. Above, you added --force-module long option, but didn't
add the -m short option, so this wouldn't do anything anyway.

Consider if passing the force option through the environment would be
easier instead.


-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux