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
