Re: [igt-dev] [PATCH i-g-t 2/2] Add support for forcing specific module

[Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>]

On 08/21, Petri Latvala wrote:
> On Sat, Jul 07, 2018 at 08:25:07PM -0300, Rodrigo Siqueira wrote:
> > This commit adds a new option for forcing the use of a specific module
> > indicated via command line.  The force command expects a module name
> > which will be used on the target test (changing the standard behavior).
> > 
> > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > ---
> >  lib/drmtest.c        | 33 +++++++++++++++++++++++++++------
> >  lib/igt_core.c       | 23 +++++++++++++++++++++++
> >  lib/igt_core.h       |  4 ++++
> >  scripts/run-tests.sh |  4 +++-
> >  4 files changed, 57 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/drmtest.c b/lib/drmtest.c
> > index eee733eb..a77e2231 100644
> > --- a/lib/drmtest.c
> > +++ b/lib/drmtest.c
> > @@ -247,6 +247,20 @@ static int __open_device(const char *base, int offset, unsigned int chipset)
> >  		if (chipset & DRIVER_AMDGPU && is_amd_device(fd))
> >  			return fd;
> >  
> > +		// Force options
> > +		if (get_target_module() && chipset == DRIVER_ANY) {
> > +			if (__is_device(fd, get_target_module())) {
> > +				return fd;
> > +			}
> > +			else {
> > +				memset(name, 0, sizeof(name));
> > +				__get_drm_device_name(fd, name);
> > +				close(fd);
> > +				igt_kmod_unload(name, 0);
> > +				return -1;
> > +			}
> > +		}
> > +
> 
> 
> You're unloading modules (which you shouldn't need to do here anyway),
> and even worse, unloading them if the first loop iteration doesn't
> land on the forced name.
> 
> This chunk needs to be before the matches for the known bits for
> DRIVER_ANY or you'll just get one of them when forcing a custom name.
Hi,

I think I did not fully understand your point; I'm a little bit confused
with this part:

"[..] or you'll just get one of them when forcing a custom name."

Do we want more than one?
 
> 
> >  		/* Only VGEM-specific tests should be run on VGEM */
> >  		if (chipset == DRIVER_ANY && !is_vgem_device(fd))
> >  			return fd;
> > @@ -260,6 +274,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;
> > @@ -278,13 +293,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 {
> 
> 
> The name of the driver (as returned by DRM_IOCTL_VERSION) doesn't have
> to match the kernel module filename. Case in point: virtio-gpu.ko is
> "virtio_gpu".
> 
> I would suggest leaving the kernel module loading plans later for now
> and get the DRIVER_ANY match forcing in place. Or even forget kernel
> module loading entirely. If the user is able to set a parameter to a
> module name, the user can load the module themselves before running
> tests. It won't get unloaded if IGT doesn't have a test that does it,
> and it won't have such a test unless it knows about the
> driver/module. And at that point, IGT will know how to load that
> module.

Basically, leave the modprobe part for the user, Right?

> 
> 
> > +		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..ee085e2a 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,23 @@ static pthread_mutex_t log_buffer_mutex = PTHREAD_MUTEX_INITIALIZER;
> >  GKeyFile *igt_key_file;
> >  #endif
> >  
> > +char *target_module = NULL;
> > +
> > +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)
> > +{
> > +	if (target_module)
> > +		return target_module;
> > +	return NULL;
> > +}
> > +
> >  char *igt_frame_dump_path;
> >  
> >  const char *igt_test_name(void)
> > @@ -555,6 +573,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 +685,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 +783,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..29aa50a8 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 "  -f <module>     force specific module load"
> >  	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" ;;
> > +		f) FILTER="$FILTER --force-module $OPTARG" ;;
> 
> 
> This command line is given to piglit, not the IGT tests.
> 
> Furthermore, run-tests.sh is not required to run tests. One can
> execute piglit or igt_runner directly.
> 
> Rather, consider having the force driver name passed via environment
> variables.

Do you mean, something like this:

kms_flip  --run-subtest basic-plain-flip MODULE=vkms
 
Thanks a lot for the review!
Best Regards
> 
> -- 
> Petri Latvala

-- 
Rodrigo Siqueira
http://siqueira.tech
Graduate Student
Department of Computer Science
University of São Paulo
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev