[Intel-gfx] [PATCH igt] igt: Add basic framework for GVT-g testing

[Wang, Zhi A]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Tuesday, June 21, 2016 3:08 PM
> To: Wang, Zhi A <zhi.a.wang at intel.com>
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [PATCH igt] igt: Add basic framework for GVT-g testing
> 
> On Tue, Jun 21, 2016 at 12:01:02PM +0000, Wang, Zhi A wrote:
> >
> >
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> > > Sent: Tuesday, June 21, 2016 2:55 PM
> > > To: intel-gfx at lists.freedesktop.org
> > > Cc: Wang, Zhi A <zhi.a.wang at intel.com>
> > > Subject: Re: [PATCH igt] igt: Add basic framework for GVT-g testing
> > >
> > > On Tue, Jun 21, 2016 at 12:36:06PM +0100, Chris Wilson wrote:
> > > > +static bool is_gvt_enabled(void)
> > > > +{
> > > > +	FILE *file;
> > > > +	int value;
> > > > +	bool enabled = false;
> > > > +
> > > > +	file = fopen("/sys/module/i915/parameters/enable_gvt", "r");
> > > > +	if (!file)
> > > > +		return false;
> > > > +
> > > > +	if (fscanf(file, "%d", &value) == 1)
> > > > +		enabled = value;
> > > > +	fclose(file);
> > > > +
> > > > +	errno = 0;
> > > > +	return enabled;
> > > > +}
> > > > +
> > > > +static void unbind_fbcon(void)
> > > > +{
> > > > +	char buf[128];
> > > > +	const char *path = "/sys/class/vtconsole";
> > > > +	DIR *dir;
> > > > +	struct dirent *vtcon;
> > > > +
> > > > +	dir = opendir(path);
> > > > +	if (!dir)
> > > > +		return;
> > > > +
> > > > +	while ((vtcon = readdir(dir))) {
> > > > +		int fd, len;
> > > > +
> > > > +		if (strncmp(vtcon->d_name, "vtcon", 5))
> > > > +			continue;
> > > > +
> > > > +		sprintf(buf, "%s/%s/name", path, vtcon->d_name);
> > > > +		fd = open(buf, O_RDONLY);
> > > > +		if (fd < 0)
> > > > +			continue;
> > > > +
> > > > +		len = read(fd, buf, sizeof(buf) - 1);
> > > > +		close(fd);
> > > > +		if (len >= 0)
> > > > +			buf[len] = '\0';
> > > > +
> > > > +		if (strstr(buf, "frame buffer device")) {
> > > > +			sprintf(buf, "%s/%s/bind", path, vtcon->d_name);
> > > > +			fd = open(buf, O_WRONLY);
> > > > +			if (fd != -1) {
> > > > +				buf[0] = '1';
> > > > +				buf[1] = '\n';
> > > > +				write(fd, buf, 2);
> > > > +				close(fd);
> > > > +			}
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +	closedir(dir);
> > > > +}
> > > > +
> > > > +static void unload_i915(void)
> > > > +{
> > > > +	unbind_fbcon();
> > > > +	/* pkill alsact */
> > > > +
> > > > +	system("/sbin/modprobe -s -r i915"); }
> > > > +
> > > > +bool igt_gvt_load_module(void)
> > > > +{
> > > > +	if (is_gvt_enabled())
> > > > +		return true;
> > > > +
> > > > +	unload_i915();
> > > > +	system("/sbin/modprobe -s i915 enable_gvt=1");
> > > > +
> > > > +	return is_gvt_enabled();
> > >
> > > Would it be safe to put igt_gvt_unload_module() into an exit handler?
> > >
> > Would you mind to elaborate your concern here? I assume you want to
> > register igt_gvt_unload_module() via atexit(). :D
> 
> Yes. I was thinking that it would be more convenient to tests to automatically
> cleanup and restore the previous state. However, an error path is likely to leave
> the module in-use during our atexit handler and so prevent us from unloading
> the module. Still it is one thing less to remember when writing a test case.
> 
> The problem with the atexit handler is that it needs to be sigsafe. The use of
> fopen here could be problematic for instance.


To me, atexit is not reliable. As if something wrong happened during the test and an unexpected signal was received, the prog would be aborted. atexit() would not work on that case. Better do failsafe work in parent process and run the test case in child process I think.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre