[igt-dev] [RFC i-g-t 1/4] runner: Introduce --resolution to igt_runner
Knop, Ryszard
ryszard.knop at intel.com
Fri Jun 10 12:16:25 UTC 2022
On Fri, 2022-06-10 at 14:31 +0300, Petri Latvala wrote:
> On Fri, May 27, 2022 at 03:39:22PM +0300, Petri Latvala wrote:
> > On Fri, May 27, 2022 at 01:43:50PM +0530, Bhanuprakash Modem wrote:
> > > Allow finer control of taking default DRM display mode for
> > > subtests
> > > instead of taking the preferred mode exposed by the Kernel.
> > >
> > > Example:
> > > Eventhough we have an 8K panel, Kernel may expose 4K or lesser
> > > mode
> > > as a preferred mode. So all subtests will take this mode as a
> > > default
> > > may cause losing the coverage.
> > >
> > > This patch will set the environment variable based on the user
> > > request,
> > > and IGT will parse this environment variable and take the
> > > decision to
> > > choose the mode from the available list.
> > >
> > > The default is still to take the preferred mode exposed by the
> > > Kernel.
> > > Other options are:
> > > * highest: Choose the mode with highest possible resolution.
> > > * lowest: Choose the mode with lowest possible resolution.
> > >
> > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > Cc: Swati Sharma <swati2.sharma at intel.com>
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > > ---
> > > runner/executor.c | 3 +++
> > > runner/settings.c | 47
> > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > runner/settings.h | 7 +++++++
> > > 3 files changed, 57 insertions(+)
> > >
> > > diff --git a/runner/executor.c b/runner/executor.c
> > > index 6e6ca9cc..fc805c30 100644
> > > --- a/runner/executor.c
> > > +++ b/runner/executor.c
> > > @@ -2056,6 +2056,9 @@ bool execute(struct execute_state *state,
> > > }
> > >
> > > end:
> > > + if (getenv("IGT_KMS_RESOLUTION"))
> > > + unsetenv("IGT_KMS_RESOLUTION");
> > > +
> > > if (settings->enable_code_coverage && !settings-
> > > >cov_results_per_test) {
> > > char *reason = NULL;
> > >
> > > diff --git a/runner/settings.c b/runner/settings.c
> > > index bb63a9f4..a20bcd01 100644
> > > --- a/runner/settings.c
> > > +++ b/runner/settings.c
> > > @@ -30,6 +30,7 @@ enum {
> > > OPT_COV_RESULTS_PER_TEST,
> > > OPT_VERSION,
> > > OPT_PRUNE_MODE,
> > > + OPT_RESOLUTION,
> > > OPT_HELP = 'h',
> > > OPT_NAME = 'n',
> > > OPT_DRY_RUN = 'd',
> > > @@ -78,6 +79,16 @@ static struct {
> > > { 0, 0 },
> > > };
> > >
> > > +static struct {
> > > + int value;
> > > + const char *name;
> > > +} resolution_modes[] = {
> > > + { RESOLUTION_DEFAULT, "default" },
> > > + { RESOLUTION_HIGHEST, "highest" },
> > > + { RESOLUTION_LOWEST, "lowest" },
> > > + { 0, 0 },
> > > +};
> > > +
> > > static bool set_log_level(struct settings* settings, const char
> > > *level)
> > > {
> > > typeof(*log_levels) *it;
> > > @@ -130,6 +141,26 @@ static bool set_prune_mode(struct settings*
> > > settings, const char *mode)
> > > return false;
> > > }
> > >
> > > +static bool set_resolution_mode(struct settings *settings, const
> > > char *mode)
> > > +{
> > > + typeof(*resolution_modes) *it;
> > > + char res[2];
> > > +
> > > + for (it = resolution_modes; it->name; it++) {
> > > + if (!strcmp(mode, it->name)) {
> > > + settings->resolution = it->value;
> > > +
> > > + sprintf(res, "%d", it->value);
> > > + setenv("IGT_KMS_RESOLUTION", res, 1);
> > > +
> > > + return true;
> > > + }
> > > + }
> > > + setenv("IGT_KMS_RESOLUTION", "0", 1);
> > > +
> > > + return false;
> > > +}
> > > +
> > > static bool parse_abort_conditions(struct settings *settings,
> > > const char *optarg)
> > > {
> > > char *dup, *origdup, *p;
> > > @@ -279,6 +310,12 @@ static const char *usage_str =
> > > " not in
> > > the requested test set.\n"
> > > " Useful
> > > when you have a hand-written\n"
> > > "
> > > testlist.\n"
> > > + " --resolution <mode> Control of taking default DRM
> > > display mode for subtests\n"
> > > + " instead of taking the preferred
> > > mode exposed by the Kernel.\n"
> > > + " Possible options:\n"
> > > + " default - Use preferred mode.
> > > (default)\n"
> > > + " highest - Use the mode with
> > > highest possible resolution.\n"
> > > + " lowest - Use the mode with
> > > lowest possible resolution.\n"
> > > " -b, --blacklist FILENAME\n"
> > > " Exclude all test matching to
> > > regexes from FILENAME\n"
> > > " (can be used more than once)\n"
> > > @@ -532,6 +569,7 @@ bool parse_options(int argc, char **argv,
> > > {"dmesg-warn-level", required_argument, NULL,
> > > OPT_DMESG_WARN_LEVEL},
> > > {"prune-mode", required_argument, NULL,
> > > OPT_PRUNE_MODE},
> > > {"blacklist", required_argument, NULL,
> > > OPT_BLACKLIST},
> > > + {"resolution", optional_argument, NULL,
> > > OPT_RESOLUTION},
> > > {"list-all", no_argument, NULL, OPT_LIST_ALL},
> > > { 0, 0, 0, 0},
> > > };
> > > @@ -640,6 +678,12 @@ bool parse_options(int argc, char **argv,
> > >
> > > absolute_path(optarg)))
> > > goto error;
> > > break;
> > > + case OPT_RESOLUTION:
> > > + if (!set_resolution_mode(settings,
> > > optarg)) {
> > > + usage("Cannot parse resolution
> > > mode", stderr);
> > > + goto error;
> > > + }
> > > + break;
> >
> > If you only set the env var here, it won't be set on resumes.
> >
> > > case OPT_LIST_ALL:
> > > settings->list_all = true;
> > > break;
> > > @@ -854,6 +898,7 @@ bool serialize_settings(struct settings
> > > *settings)
> > > SERIALIZE_LINE(f, settings, piglit_style_dmesg, "%d");
> > > SERIALIZE_LINE(f, settings, dmesg_warn_level, "%d");
> > > SERIALIZE_LINE(f, settings, prune_mode, "%d");
> > > + SERIALIZE_LINE(f, settings, resolution, "%d");
> > > SERIALIZE_LINE(f, settings, test_root, "%s");
> > > SERIALIZE_LINE(f, settings, results_path, "%s");
> > > SERIALIZE_LINE(f, settings, enable_code_coverage, "%d");
> > > @@ -886,6 +931,7 @@ bool read_settings_from_file(struct settings
> > > *settings, FILE *f)
> > > char *name = NULL, *val = NULL;
> > >
> > > settings->dmesg_warn_level = -1;
> > > + settings->resolution = 0;
> >
> > If a zero val is the default, you don't need to set it here
> > explicitly. Anything calling this function has called init_settings
> > which memsets the whole object to 0.
> >
> >
> >
> > I'll look into this in more detail next week. I need to form an
> > actual
> > opinion on a few subjects, such as whether this is needed in runner
> > at
> > all (one can set environment variables when executing and let
> > runner
> > just pass them through, it doesn't clean the environment), whether
> > --resolution is a good name for the switch, and whether I want a
> > more
> > generic environment storing/handling in the runner instead now or
> > later.
>
> Alright, my opinion is that it's better to have a generic --env flag
> on igt_runner instead. It's useful to have the support for making
> sure
> other env vars also get correctly set on resumes, notably IGT_DEVICE.
>
> In short, we need --environment=key=value, and also --environment=key
> (gets the var from the current environment, stores the
> value). Repeated flags store more values.
>
> If you don't have the bandwidth to work on that change yourself, I
> suppose Ryszard might be able to pick this up. Ryszard, what do you
> think?
>
>
Yeah, I can take this. The runner should not touch the
IGT_KMS_RESOLUTION var at all in that case, rather have that handled in
the test process, and any vars marked via --environment will get saved
for resumes.
Just one question, if the var is not set in the environment and the
flag gets set to just the name, should the runner error out, or just
set an empty var?
Ryszard
More information about the igt-dev
mailing list