Re: [Intel-gfx] [PATCH igt v2] igt/gen7_forcewake_mt: Make the mmio register as volatile

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 02, 2018 at 08:12:35AM +0000, Chris Wilson wrote:
> Prevent the compiler from caching reads/writes to the hw register as we
> do want to perform mmio.
> 
> Whilst fixing up the mmio access, also ensure that we do not leave the
> test with any other bits still set in the forcewake register to prevent
> affecting other tests, as spotted by Tvrtko.

I wonder why this test isn't using intel_register_access_init() & co.?
Maybe because the library code used to take the forcewake always? But
that could be prevented with a setenv() now.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  tests/gen7_forcewake_mt.c | 59 +++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c
> index 07320ef9..218f674c 100644
> --- a/tests/gen7_forcewake_mt.c
> +++ b/tests/gen7_forcewake_mt.c
> @@ -44,9 +44,10 @@ IGT_TEST_DESCRIPTION("Exercise a suspect workaround required for"
>  
>  struct thread {
>  	pthread_t thread;
> -	void *mmio;
> +	volatile uint32_t *mmio;
>  	int fd;
>  	int bit;
> +	bool done;
>  };
>  
>  static const struct pci_id_match match[] = {
> @@ -80,7 +81,7 @@ static struct pci_device *__igfx_get(void)
>  	return dev;
>  }
>  
> -static void *igfx_get_mmio(void)
> +static volatile uint32_t *igfx_get_forcewake_mt(void)
>  {
>  	struct pci_device *pci = __igfx_get();
>  	void *mmio = NULL;
> @@ -100,20 +101,28 @@ static void *igfx_get_mmio(void)
>  	igt_assert_eq(error, 0);
>  	igt_assert(mmio != NULL);
>  
> -	return mmio;
> +	return (volatile uint32_t *)((char *)mmio + FORCEWAKE_MT);
>  }
>  
> +
>  static void *thread(void *arg)
>  {
> +	static const char acquire_error[] = "acquire";
> +	static const char release_error[] = "release";
> +
>  	struct thread *t = arg;
> -	uint32_t *forcewake_mt = (uint32_t *)((char *)t->mmio + FORCEWAKE_MT);
> -	uint32_t bit = 1 << t->bit;
> +	const uint32_t bit = 1 << t->bit;
> +	volatile uint32_t *forcewake_mt = t->mmio;
>  
> -	while (1) {
> +	while (!t->done) {
>  		*forcewake_mt = bit << 16 | bit;
> -		igt_assert(*forcewake_mt & bit);
> +		if (igt_wait(*forcewake_mt & bit, 50, 1))
> +			return (void *)acquire_error;
> +
>  		*forcewake_mt = bit << 16;
> -		igt_assert((*forcewake_mt & bit) == 0);
> +		igt_assert(igt_wait((*forcewake_mt & bit) == 0, 50, 1) == 0);
> +		if (igt_wait((*forcewake_mt & bit) == 0, 50, 1))
> +			return (void *)release_error;
>  	}
>  
>  	return NULL;
> @@ -124,10 +133,12 @@ static void *thread(void *arg)
>  igt_simple_main
>  {
>  	struct thread t[16];
> +	bool success = true;
>  	int i;
>  
>  	t[0].fd = drm_open_driver(DRIVER_INTEL);
> -	t[0].mmio = igfx_get_mmio();
> +	t[0].mmio = igfx_get_forcewake_mt();
> +	t[0].done = false;
>  
>  	for (i = 2; i < 16; i++) {
>  		t[i] = t[0];
> @@ -137,7 +148,7 @@ igt_simple_main
>  
>  	sleep(2);
>  
> -	for (i = 0; i < 1000; i++) {
> +	igt_until_timeout(2) {
>  		uint32_t *p;
>  		struct drm_i915_gem_execbuffer2 execbuf;
>  		struct drm_i915_gem_exec_object2 exec[2];
> @@ -192,13 +203,37 @@ igt_simple_main
>  		p = gem_mmap__gtt(t[0].fd, exec[0].handle, 4096, PROT_READ);
>  
>  		igt_info("[%d]={ %08x %08x }\n", i, p[0], p[1]);
> -		igt_assert(p[0] & 2);
> -		igt_assert((p[1] & 2) == 0);
> +		if ((p[0] & 2) == 0) {
> +			igt_warn("Failed to acquire forcewake BIT(1) from batch\n");
> +			success = false;
> +		}
> +		if ((p[1] & 2)) {
> +			igt_warn("Failed to release forcewake BIT(1) from batch\n");
> +			success = false;
> +		}
>  
>  		munmap(p, 4096);
>  		gem_close(t[0].fd, exec[0].handle);
>  		gem_close(t[0].fd, exec[1].handle);
> +		if (!success)
> +			break;
>  
>  		usleep(1000);
>  	}
> +
> +	for (i = 2; i < 16; i++) {
> +		void *result;
> +
> +		t[i].done = true;
> +		pthread_join(t[i].thread, &result);
> +		if (result) {
> +			igt_warn("Thread BIT(%d) failed to %s forcewake\n", i, (char *)result);
> +			success = false;
> +		}
> +	}
> +
> +	/* And clear all forcewake bits before disappearing */
> +	*t[0].mmio = 0xfffe << 16;
> +
> +	igt_assert(success);
>  }
> -- 
> 2.16.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx