Re: [PATCH igt] gem_concurrent_blit: Don't call igt_require() outside of a subtest/fixture

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 08, 2016 at 09:10:38AM +0000, Chris Wilson wrote:
> gem_concurrent_blit tries to ensure that it doesn't try and run a test
> that would grind the system to a halt, i.e. unexpectedly cause swap
> thrashing. It currently calls intel_require_memory(), but outside of
> the subtest (as the tests use fork, it cannot do requirement testing
> within the test children) - but intel_require_memory() calls
> igt_require() and triggers and abort. Wrapping that initial require
> within an igt_fixture() stops the abort(), but also prevents any further
> testing.
> 
> This patch restructures the requirement checking to ordinary conditions,
> which though allowing the test to run, also prevents listing of subtests
> on machines which cannot handle them.


> ---
>  lib/igt_aux.h              |  2 ++
>  lib/intel_os.c             | 53 +++++++++++++++++++++++-------------
>  tests/gem_concurrent_all.c | 67 +++++++++++++++++++++++++++++++++-------------
>  3 files changed, 85 insertions(+), 37 deletions(-)
> 
> diff --git a/lib/igt_aux.h b/lib/igt_aux.h
> index 6e11ee6..5a88c2a 100644
> --- a/lib/igt_aux.h
> +++ b/lib/igt_aux.h
> @@ -88,6 +88,8 @@ uint64_t intel_get_total_swap_mb(void);
>  
>  #define CHECK_RAM 0x1
>  #define CHECK_SWAP 0x2
> +int __intel_check_memory(uint32_t count, uint64_t size, unsigned mode,
> +			 uint64_t *out_required, uint64_t *out_total);
>  void intel_require_memory(uint32_t count, uint64_t size, unsigned mode);
>  int intel_num_objects_for_memory(uint32_t size, unsigned mode);
>  
> diff --git a/lib/intel_os.c b/lib/intel_os.c
> index dba9e17..90f9bb3 100644
> --- a/lib/intel_os.c
> +++ b/lib/intel_os.c
> @@ -192,6 +192,38 @@ intel_get_total_swap_mb(void)
>  	return retval / (1024*1024);
>  }
>  

Please add the usual gtkdoc boilerplate here with a mention of
intel_check_memory. Ack with that.
-Daniel

> +int __intel_check_memory(uint32_t count, uint64_t size, unsigned mode,
> +			 uint64_t *out_required, uint64_t *out_total)
> +{
> +/* rough estimate of how many bytes the kernel requires to track each object */
> +#define KERNEL_BO_OVERHEAD 512
> +	uint64_t required, total;
> +
> +	required = count;
> +	required *= size + KERNEL_BO_OVERHEAD;
> +	required = ALIGN(required, 4096);
> +
> +	igt_debug("Checking %'u surfaces of size %'llu bytes (total %'llu) against %s%s\n",
> +		  count, (long long)size, (long long)required,
> +		  mode & (CHECK_RAM | CHECK_SWAP) ? "RAM" : "",
> +		  mode & CHECK_SWAP ? " + swap": "");
> +
> +	total = 0;
> +	if (mode & (CHECK_RAM | CHECK_SWAP))
> +		total += intel_get_avail_ram_mb();
> +	if (mode & CHECK_SWAP)
> +		total += intel_get_total_swap_mb();
> +	total *= 1024 * 1024;
> +
> +	if (out_required)
> +		*out_required = required;
> +
> +	if (out_total)
> +		*out_total = total;
> +
> +	return required < total;
> +}
> +
>  /**
>   * intel_require_memory:
>   * @count: number of surfaces that will be created
> @@ -217,27 +249,10 @@ intel_get_total_swap_mb(void)
>   */
>  void intel_require_memory(uint32_t count, uint64_t size, unsigned mode)
>  {
> -/* rough estimate of how many bytes the kernel requires to track each object */
> -#define KERNEL_BO_OVERHEAD 512
>  	uint64_t required, total;
>  
> -	required = count;
> -	required *= size + KERNEL_BO_OVERHEAD;
> -	required = ALIGN(required, 4096);
> -
> -	igt_debug("Checking %'u surfaces of size %'llu bytes (total %'llu) against %s%s\n",
> -		  count, (long long)size, (long long)required,
> -		  mode & (CHECK_RAM | CHECK_SWAP) ? "RAM" : "",
> -		  mode & CHECK_SWAP ? " + swap": "");
> -
> -	total = 0;
> -	if (mode & (CHECK_RAM | CHECK_SWAP))
> -		total += intel_get_avail_ram_mb();
> -	if (mode & CHECK_SWAP)
> -		total += intel_get_total_swap_mb();
> -	total *= 1024 * 1024;
> -
> -	igt_skip_on_f(total <= required,
> +	igt_skip_on_f(!__intel_check_memory(count, size, mode,
> +					    &required, &total),
>  		      "Estimated that we need %'llu bytes for the test, but only have %'llu bytes available (%s%s)\n",
>  		      (long long)required, (long long)total,
>  		      mode & (CHECK_RAM | CHECK_SWAP) ? "RAM" : "",
> diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
> index 0e873c4..9a2fb6d 100644
> --- a/tests/gem_concurrent_all.c
> +++ b/tests/gem_concurrent_all.c
> @@ -155,9 +155,9 @@ static bool can_create_stolen(void)
>  static drm_intel_bo *
>  (*create_func)(drm_intel_bufmgr *bufmgr, uint64_t size);
>  
> -static void create_cpu_require(void)
> +static bool create_cpu_require(void)
>  {
> -	igt_require(create_func != create_stolen_bo);
> +	return create_func != create_stolen_bo;
>  }
>  
>  static drm_intel_bo *
> @@ -375,7 +375,7 @@ gpu_cmp_bo(drm_intel_bo *bo, uint32_t val, int width, int height, drm_intel_bo *
>  
>  const struct access_mode {
>  	const char *name;
> -	void (*require)(void);
> +	bool (*require)(void);
>  	void (*set_bo)(drm_intel_bo *bo, uint32_t val, int w, int h);
>  	void (*cmp_bo)(drm_intel_bo *bo, uint32_t val, int w, int h, drm_intel_bo *tmp);
>  	drm_intel_bo *(*create_bo)(drm_intel_bufmgr *bufmgr, int width, int height);
> @@ -1294,23 +1294,22 @@ run_basic_modes(const char *prefix,
>  }
>  
>  static void
> -run_modes(const char *style, const struct access_mode *mode)
> +run_modes(const char *style, const struct access_mode *mode, unsigned allow_mem)
>  {
> -	if (mode->require)
> -		mode->require();
> +	if (mode->require && !mode->require())
> +		return;
>  
> -	igt_debug("%s: using 2x%d buffers, each 1MiB\n", style, num_buffers);
> -	intel_require_memory(2*num_buffers, 1024*1024, CHECK_RAM);
> +	igt_debug("%s: using 2x%d buffers, each 1MiB\n",
> +			style, num_buffers);
> +	if (!__intel_check_memory(2*num_buffers, 1024*1024, allow_mem,
> +				  NULL, NULL))
> +		return;
>  
> -	if (all) {
> -		run_basic_modes(style, mode, "", run_single);
> -
> -		igt_fork_signal_helper();
> -		run_basic_modes(style, mode, "-interruptible", run_interruptible);
> -		igt_stop_signal_helper();
> -	}
> +	run_basic_modes(style, mode, "", run_single);
>  
>  	igt_fork_signal_helper();
> +	run_basic_modes(style, mode, "-interruptible",
> +			run_interruptible);
>  	run_basic_modes(style, mode, "-forked", run_forked);
>  	run_basic_modes(style, mode, "-bomb", run_bomb);
>  	igt_stop_signal_helper();
> @@ -1328,6 +1327,8 @@ igt_main
>  		{ "stolen-", create_stolen_bo, can_create_stolen },
>  		{ NULL, NULL }
>  	}, *c;
> +	void *pinned;
> +	uint64_t pin_sz;
>  	int i;
>  
>  	igt_skip_on_simulation();
> @@ -1354,7 +1355,7 @@ igt_main
>  		if (c->require()) {
>  			snprintf(name, sizeof(name), "%s%s", c->name, "small");
>  			for (i = 0; i < ARRAY_SIZE(access_modes); i++)
> -				run_modes(name, &access_modes[i]);
> +				run_modes(name, &access_modes[i], CHECK_RAM);
>  		}
>  
>  		igt_fixture {
> @@ -1364,7 +1365,7 @@ igt_main
>  		if (c->require()) {
>  			snprintf(name, sizeof(name), "%s%s", c->name, "thrash");
>  			for (i = 0; i < ARRAY_SIZE(access_modes); i++)
> -				run_modes(name, &access_modes[i]);
> +				run_modes(name, &access_modes[i], CHECK_RAM);
>  		}
>  
>  		igt_fixture {
> @@ -1374,7 +1375,37 @@ igt_main
>  		if (c->require()) {
>  			snprintf(name, sizeof(name), "%s%s", c->name, "full");
>  			for (i = 0; i < ARRAY_SIZE(access_modes); i++)
> -				run_modes(name, &access_modes[i]);
> +				run_modes(name, &access_modes[i], CHECK_RAM);
> +		}
> +
> +		igt_fixture {
> +			num_buffers = gem_mappable_aperture_size() / (1024 * 1024);
> +			pin_sz = intel_get_avail_ram_mb() - num_buffers;
> +
> +			igt_debug("Pinning %ld MiB\n", pin_sz);
> +			pin_sz *= 1024 * 1024;
> +
> +			if (posix_memalign(&pinned, 4096, pin_sz) ||
> +			    mlock(pinned, pin_sz) ||
> +			    madvise(pinned, pin_sz, MADV_DONTFORK)) {
> +				free(pinned);
> +				pinned = NULL;
> +			}
> +			igt_require(pinned);
> +		}
> +
> +		if (c->require()) {
> +			snprintf(name, sizeof(name), "%s%s", c->name, "swap");
> +			for (i = 0; i < ARRAY_SIZE(access_modes); i++)
> +				run_modes(name, &access_modes[i], CHECK_RAM | CHECK_SWAP);
> +		}
> +
> +		igt_fixture {
> +			if (pinned) {
> +				munlock(pinned, pin_sz);
> +				free(pinned);
> +				pinned = NULL;
> +			}
>  		}
>  	}
>  }
> -- 
> 2.7.0.rc3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux