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 <rodrigosiqueiram...@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?
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
