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 <[email protected]>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
