| Message ID | 20180905203827.n7vpugbs6bjvijpg@smtp.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,v2] Add support for forcing specific module | expand |
Quoting Rodrigo Siqueira (2018-09-05 21:38:27) > This commit adds a new option for forcing the use of a specific driver > indicated via an environment variable. > > Changes since V1: > Petri: > - Use an environment variable instead of command line > - Refactor the loop in __search_and_open to accept forced module > - Don't try to load kernel modules I am still not convinced this is a good solution to the problem of running tests against all applicable devices, along with generic filtering of that set (both from the test profile and user config). Short term wise I'd rather see DRIVER_ANY translated into a known DRIVER_X selector (so that we have a complete list of drivers for later exploitation), say diff --git a/lib/drmtest.c b/lib/drmtest.c index bfa2e0f..5c96c1d 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -257,11 +257,16 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset) return -1; } +static unsigned int driver_any_chipset = DRIVER_ANY; // give me a better name + static int __open_driver(const char *base, int offset, unsigned int chipset) { static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; int fd; + if (chipset == DRIVER_ANY) + chipset = driver_any_chipset; + fd = __search_and_open(base, offset, chipset); if (fd != -1) return fd; @@ -428,3 +433,21 @@ void igt_require_intel(int fd) { igt_require(is_i915_device(fd) && has_known_intel_chipset(fd)); } + +bool set_driver_any(const char *name) +{ + for (int start = 0, end = ARRAY_SIZE(modules) - 1; start < end; ) { // repetitive much? + int mid = start + (end - start) / 2; + int ret = strcmp(modules[mid].module, name); + if (ret < 0) { + start = mid + 1; + } else if (ret > 0) { + end = mid; + } else { + driver_any_chipset = modules[mid].bit; + return true; + } + } + + return false; +}
On Wed, Sep 05, 2018 at 05:38:27PM -0300, Rodrigo Siqueira wrote: > This commit adds a new option for forcing the use of a specific driver > indicated via an environment variable. > > Changes since V1: > Petri: > - Use an environment variable instead of command line > - Refactor the loop in __search_and_open to accept forced module > - Don't try to load kernel modules > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Note: You can't drop the s-o-b line if your patch contains work by other people, like from Petri here. Proper way to resend a patch by someone else is to just add a subject prefix of "PATCH RESEND" and otherwise keep everything unchanged (including author and everything). https://patchwork.freedesktop.org/patch/245532/ Cheers, Daniel > --- > lib/drmtest.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > lib/drmtest.h | 2 ++ > lib/igt_core.c | 5 +++++ > 3 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/lib/drmtest.c b/lib/drmtest.c > index bfa2e0f0..6e35d1be 100644 > --- a/lib/drmtest.c > +++ b/lib/drmtest.c > @@ -123,6 +123,36 @@ static bool has_known_intel_chipset(int fd) > return true; > } > > +static char _forced_driver[5] = ""; > + > +/** > + * __set_forced_driver: > + * @name: name of driver to forcibly use > + * > + * Set the name of a driver to use when calling #drm_open_driver with > + * the #DRIVER_ANY flag. > + */ > +void __set_forced_driver(const char *name) > +{ > + if (!strcmp(name, "")) { > + igt_warn("IGT_FORCE_DRIVER flag specified without a value," > + "ignoring force option\n"); > + return; > + } > + > + igt_info("Attempt to force module %s\n", name); > + > + strncpy(_forced_driver, name, 4); > +} > + > +static const char *forced_driver(void) > +{ > + if (_forced_driver[0]) > + return _forced_driver; > + > + return NULL; > +} > + > #define LOCAL_I915_EXEC_VEBOX (4 << 0) > /** > * gem_quiescent_gpu: > @@ -250,8 +280,18 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset) > > sprintf(name, "%s%u", base, i + offset); > fd = open_device(name, chipset); > - if (fd != -1) > - return fd; > + if (fd == -1) > + continue; > + > + // Force module > + if (chipset == DRIVER_ANY && forced_driver()) { > + if (__is_device(fd, forced_driver())) > + return fd; > + close(fd); > + continue; > + } > + > + return fd; > } > > return -1; > diff --git a/lib/drmtest.h b/lib/drmtest.h > index 949865ee..62f53ec3 100644 > --- a/lib/drmtest.h > +++ b/lib/drmtest.h > @@ -51,6 +51,8 @@ > */ > #define DRIVER_ANY ~(DRIVER_VGEM) > > +void __set_forced_driver(const char *name); > + > /** > * ARRAY_SIZE: > * @arr: static array > diff --git a/lib/igt_core.c b/lib/igt_core.c > index 23bb858f..8e65b5e3 100644 > --- a/lib/igt_core.c > +++ b/lib/igt_core.c > @@ -647,6 +647,11 @@ static void common_init_env(void) > igt_frame_dump_path = getenv("IGT_FRAME_DUMP_PATH"); > > stderr_needs_sentinel = getenv("IGT_SENTINEL_ON_STDERR") != NULL; > + > + env = getenv("IGT_FORCE_DRIVER"); > + if (env) { > + __set_forced_driver(env); > + } > } > > static int common_init(int *argc, char **argv, > -- > 2.18.0 > > > -- > 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
On 12/17, Daniel Vetter wrote: > On Wed, Sep 05, 2018 at 05:38:27PM -0300, Rodrigo Siqueira wrote: > > This commit adds a new option for forcing the use of a specific driver > > indicated via an environment variable. > > > > Changes since V1: > > Petri: > > - Use an environment variable instead of command line > > - Refactor the loop in __search_and_open to accept forced module > > - Don't try to load kernel modules > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Note: You can't drop the s-o-b line if your patch contains work by other > people, like from Petri here. Proper way to resend a patch by someone else > is to just add a subject prefix of "PATCH RESEND" and otherwise keep > everything unchanged (including author and everything). > > https://patchwork.freedesktop.org/patch/245532/ Hi, Thanks for your feedback. Next time I will take care for not make this mistake again. Best Regards Rodrigo Siqueira > Cheers, Daniel > > > --- > > lib/drmtest.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > > lib/drmtest.h | 2 ++ > > lib/igt_core.c | 5 +++++ > > 3 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/lib/drmtest.c b/lib/drmtest.c > > index bfa2e0f0..6e35d1be 100644 > > --- a/lib/drmtest.c > > +++ b/lib/drmtest.c > > @@ -123,6 +123,36 @@ static bool has_known_intel_chipset(int fd) > > return true; > > } > > > > +static char _forced_driver[5] = ""; > > + > > +/** > > + * __set_forced_driver: > > + * @name: name of driver to forcibly use > > + * > > + * Set the name of a driver to use when calling #drm_open_driver with > > + * the #DRIVER_ANY flag. > > + */ > > +void __set_forced_driver(const char *name) > > +{ > > + if (!strcmp(name, "")) { > > + igt_warn("IGT_FORCE_DRIVER flag specified without a value," > > + "ignoring force option\n"); > > + return; > > + } > > + > > + igt_info("Attempt to force module %s\n", name); > > + > > + strncpy(_forced_driver, name, 4); > > +} > > + > > +static const char *forced_driver(void) > > +{ > > + if (_forced_driver[0]) > > + return _forced_driver; > > + > > + return NULL; > > +} > > + > > #define LOCAL_I915_EXEC_VEBOX (4 << 0) > > /** > > * gem_quiescent_gpu: > > @@ -250,8 +280,18 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset) > > > > sprintf(name, "%s%u", base, i + offset); > > fd = open_device(name, chipset); > > - if (fd != -1) > > - return fd; > > + if (fd == -1) > > + continue; > > + > > + // Force module > > + if (chipset == DRIVER_ANY && forced_driver()) { > > + if (__is_device(fd, forced_driver())) > > + return fd; > > + close(fd); > > + continue; > > + } > > + > > + return fd; > > } > > > > return -1; > > diff --git a/lib/drmtest.h b/lib/drmtest.h > > index 949865ee..62f53ec3 100644 > > --- a/lib/drmtest.h > > +++ b/lib/drmtest.h > > @@ -51,6 +51,8 @@ > > */ > > #define DRIVER_ANY ~(DRIVER_VGEM) > > > > +void __set_forced_driver(const char *name); > > + > > /** > > * ARRAY_SIZE: > > * @arr: static array > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > index 23bb858f..8e65b5e3 100644 > > --- a/lib/igt_core.c > > +++ b/lib/igt_core.c > > @@ -647,6 +647,11 @@ static void common_init_env(void) > > igt_frame_dump_path = getenv("IGT_FRAME_DUMP_PATH"); > > > > stderr_needs_sentinel = getenv("IGT_SENTINEL_ON_STDERR") != NULL; > > + > > + env = getenv("IGT_FORCE_DRIVER"); > > + if (env) { > > + __set_forced_driver(env); > > + } > > } > > > > static int common_init(int *argc, char **argv, > > -- > > 2.18.0 > > > > > > -- > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Mon, Dec 17, 2018 at 7:49 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, Sep 05, 2018 at 05:38:27PM -0300, Rodrigo Siqueira wrote: > > This commit adds a new option for forcing the use of a specific driver > > indicated via an environment variable. > > > > Changes since V1: > > Petri: > > - Use an environment variable instead of command line > > - Refactor the loop in __search_and_open to accept forced module > > - Don't try to load kernel modules > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > Note: You can't drop the s-o-b line if your patch contains work by other > people, like from Petri here. Proper way to resend a patch by someone else > is to just add a subject prefix of "PATCH RESEND" and otherwise keep > everything unchanged (including author and everything). > > https://patchwork.freedesktop.org/patch/245532/ Last time I was told I have to _add_ my s-o-b nonetheless, even if just re-sending the patch. I don't think I should, but in the end I had to change the series, add and change patches, so it didn't matter. Maybe we need some clarification on this? https://lists.freedesktop.org/archives/intel-gfx/2018-November/183291.html Lucas De Marchi > > Cheers, Daniel > > > --- > > lib/drmtest.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- > > lib/drmtest.h | 2 ++ > > lib/igt_core.c | 5 +++++ > > 3 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/lib/drmtest.c b/lib/drmtest.c > > index bfa2e0f0..6e35d1be 100644 > > --- a/lib/drmtest.c > > +++ b/lib/drmtest.c > > @@ -123,6 +123,36 @@ static bool has_known_intel_chipset(int fd) > > return true; > > } > > > > +static char _forced_driver[5] = ""; > > + > > +/** > > + * __set_forced_driver: > > + * @name: name of driver to forcibly use > > + * > > + * Set the name of a driver to use when calling #drm_open_driver with > > + * the #DRIVER_ANY flag. > > + */ > > +void __set_forced_driver(const char *name) > > +{ > > + if (!strcmp(name, "")) { > > + igt_warn("IGT_FORCE_DRIVER flag specified without a value," > > + "ignoring force option\n"); > > + return; > > + } > > + > > + igt_info("Attempt to force module %s\n", name); > > + > > + strncpy(_forced_driver, name, 4); > > +} > > + > > +static const char *forced_driver(void) > > +{ > > + if (_forced_driver[0]) > > + return _forced_driver; > > + > > + return NULL; > > +} > > + > > #define LOCAL_I915_EXEC_VEBOX (4 << 0) > > /** > > * gem_quiescent_gpu: > > @@ -250,8 +280,18 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset) > > > > sprintf(name, "%s%u", base, i + offset); > > fd = open_device(name, chipset); > > - if (fd != -1) > > - return fd; > > + if (fd == -1) > > + continue; > > + > > + // Force module > > + if (chipset == DRIVER_ANY && forced_driver()) { > > + if (__is_device(fd, forced_driver())) > > + return fd; > > + close(fd); > > + continue; > > + } > > + > > + return fd; > > } > > > > return -1; > > diff --git a/lib/drmtest.h b/lib/drmtest.h > > index 949865ee..62f53ec3 100644 > > --- a/lib/drmtest.h > > +++ b/lib/drmtest.h > > @@ -51,6 +51,8 @@ > > */ > > #define DRIVER_ANY ~(DRIVER_VGEM) > > > > +void __set_forced_driver(const char *name); > > + > > /** > > * ARRAY_SIZE: > > * @arr: static array > > diff --git a/lib/igt_core.c b/lib/igt_core.c > > index 23bb858f..8e65b5e3 100644 > > --- a/lib/igt_core.c > > +++ b/lib/igt_core.c > > @@ -647,6 +647,11 @@ static void common_init_env(void) > > igt_frame_dump_path = getenv("IGT_FRAME_DUMP_PATH"); > > > > stderr_needs_sentinel = getenv("IGT_SENTINEL_ON_STDERR") != NULL; > > + > > + env = getenv("IGT_FORCE_DRIVER"); > > + if (env) { > > + __set_forced_driver(env); > > + } > > } > > > > static int common_init(int *argc, char **argv, > > -- > > 2.18.0 > > > > > > -- > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > igt-dev mailing list > igt-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/igt-dev -- Lucas De Marchi
On Mon, Dec 17, 2018 at 09:22:31AM -0800, Lucas De Marchi wrote: > On Mon, Dec 17, 2018 at 7:49 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Wed, Sep 05, 2018 at 05:38:27PM -0300, Rodrigo Siqueira wrote: > > > This commit adds a new option for forcing the use of a specific driver > > > indicated via an environment variable. > > > > > > Changes since V1: > > > Petri: > > > - Use an environment variable instead of command line > > > - Refactor the loop in __search_and_open to accept forced module > > > - Don't try to load kernel modules > > > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > > Note: You can't drop the s-o-b line if your patch contains work by other > > people, like from Petri here. Proper way to resend a patch by someone else > > is to just add a subject prefix of "PATCH RESEND" and otherwise keep > > everything unchanged (including author and everything). > > > > https://patchwork.freedesktop.org/patch/245532/ > > Last time I was told I have to _add_ my s-o-b nonetheless, even if > just re-sending the patch. > I don't think I should, but in the end I had to change the series, add > and change patches, > so it didn't matter. Communication error here? Rodrigo's resend didn't have my S-o-b, that's what Daniel was pointing at. Removing S-o-b is never ok. Whether it's correct and/or required to add your own S-o-b to resends is another matter. > Maybe we need some clarification on this? > > https://lists.freedesktop.org/archives/intel-gfx/2018-November/183291.html That was about a kernel patch, and kernel patches are _very_ strict about having to add your S-o-b.
On Tue, Dec 18, 2018 at 02:08:21PM +0200, Petri Latvala wrote: > On Mon, Dec 17, 2018 at 09:22:31AM -0800, Lucas De Marchi wrote: > > On Mon, Dec 17, 2018 at 7:49 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Wed, Sep 05, 2018 at 05:38:27PM -0300, Rodrigo Siqueira wrote: > > > > This commit adds a new option for forcing the use of a specific driver > > > > indicated via an environment variable. > > > > > > > > Changes since V1: > > > > Petri: > > > > - Use an environment variable instead of command line > > > > - Refactor the loop in __search_and_open to accept forced module > > > > - Don't try to load kernel modules > > > > > > > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > > > > > > Note: You can't drop the s-o-b line if your patch contains work by other > > > people, like from Petri here. Proper way to resend a patch by someone else > > > is to just add a subject prefix of "PATCH RESEND" and otherwise keep > > > everything unchanged (including author and everything). > > > > > > https://patchwork.freedesktop.org/patch/245532/ > > > > Last time I was told I have to _add_ my s-o-b nonetheless, even if > > just re-sending the patch. > > I don't think I should, but in the end I had to change the series, add > > and change patches, > > so it didn't matter. > > > Communication error here? Rodrigo's resend didn't have my S-o-b, > that's what Daniel was pointing at. Removing S-o-b is never > ok. Whether it's correct and/or required to add your own S-o-b to > resends is another matter. Correct. > > Maybe we need some clarification on this? > > > > https://lists.freedesktop.org/archives/intel-gfx/2018-November/183291.html > > That was about a kernel patch, and kernel patches are _very_ strict > about having to add your S-o-b. Yes, for the kernel even just sending a patch around by someone, even if you change nothing, needs your sob. -Daniel
diff --git a/lib/drmtest.c b/lib/drmtest.c index bfa2e0f0..6e35d1be 100644 --- a/lib/drmtest.c +++ b/lib/drmtest.c @@ -123,6 +123,36 @@ static bool has_known_intel_chipset(int fd) return true; } +static char _forced_driver[5] = ""; + +/** + * __set_forced_driver: + * @name: name of driver to forcibly use + * + * Set the name of a driver to use when calling #drm_open_driver with + * the #DRIVER_ANY flag. + */ +void __set_forced_driver(const char *name) +{ + if (!strcmp(name, "")) { + igt_warn("IGT_FORCE_DRIVER flag specified without a value," + "ignoring force option\n"); + return; + } + + igt_info("Attempt to force module %s\n", name); + + strncpy(_forced_driver, name, 4); +} + +static const char *forced_driver(void) +{ + if (_forced_driver[0]) + return _forced_driver; + + return NULL; +} + #define LOCAL_I915_EXEC_VEBOX (4 << 0) /** * gem_quiescent_gpu: @@ -250,8 +280,18 @@ static int __search_and_open(const char *base, int offset, unsigned int chipset) sprintf(name, "%s%u", base, i + offset); fd = open_device(name, chipset); - if (fd != -1) - return fd; + if (fd == -1) + continue; + + // Force module + if (chipset == DRIVER_ANY && forced_driver()) { + if (__is_device(fd, forced_driver())) + return fd; + close(fd); + continue; + } + + return fd; } return -1; diff --git a/lib/drmtest.h b/lib/drmtest.h index 949865ee..62f53ec3 100644 --- a/lib/drmtest.h +++ b/lib/drmtest.h @@ -51,6 +51,8 @@ */ #define DRIVER_ANY ~(DRIVER_VGEM) +void __set_forced_driver(const char *name); + /** * ARRAY_SIZE: * @arr: static array diff --git a/lib/igt_core.c b/lib/igt_core.c index 23bb858f..8e65b5e3 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -647,6 +647,11 @@ static void common_init_env(void) igt_frame_dump_path = getenv("IGT_FRAME_DUMP_PATH"); stderr_needs_sentinel = getenv("IGT_SENTINEL_ON_STDERR") != NULL; + + env = getenv("IGT_FORCE_DRIVER"); + if (env) { + __set_forced_driver(env); + } } static int common_init(int *argc, char **argv,
This commit adds a new option for forcing the use of a specific driver indicated via an environment variable. Changes since V1: Petri: - Use an environment variable instead of command line - Refactor the loop in __search_and_open to accept forced module - Don't try to load kernel modules Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- lib/drmtest.c | 44 ++++++++++++++++++++++++++++++++++++++++++-- lib/drmtest.h | 2 ++ lib/igt_core.c | 5 +++++ 3 files changed, 49 insertions(+), 2 deletions(-)