[igt-dev] [RFC i-g-t 1/4] runner: Introduce --resolution to igt_runner

[Petri Latvala]

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?


-- 
Petri Latvala