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

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

 



On Tue, Aug 21, 2018 at 11:22:45AM -0300, Rodrigo Siqueira wrote:
> 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@xxxxxxxxx>
> > > ---
> > >  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?


I mean that when a test calls drm_open_driver(DRIVER_ANY), it will
happily open /dev/dri/card0 and use that if it's, say, an i915
device. Even though you enabled the force option to "vkms".




>  
> > 
> > >  		/* 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?


Exactly.



> 
> > 
> > 
> > > +		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


MODULE=vkms kms_flip --run-subtest basic-plain-flip

Or the more usual way of running tests:

IGT_TEST_ROOT=builddir/tests MODULE=vkms piglit run igt -t igt@kms_flip@basic-plain-flip results-directory

or

MODULE=vkms builddir/runner/igt_runner -t igt@kms_flip@basic-plain-flip builddir/tests results-directory



-- 
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