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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 05/03/2018 17:30, 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.
> 
> v2: Use intel_mmio_use_pci_bar() rather open code the ioremap
> v3: Flip igt_wait() checking as it returns true on success, not errno.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   tests/gen7_forcewake_mt.c | 75 +++++++++++++++++++++++++++++++----------------
>   1 file changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c
> index 07320ef9..6818d7aa 100644
> --- a/tests/gen7_forcewake_mt.c
> +++ b/tests/gen7_forcewake_mt.c
> @@ -41,12 +41,14 @@ IGT_TEST_DESCRIPTION("Exercise a suspect workaround required for"
>   		     " FORCEWAKE_MT.");
>   
>   #define FORCEWAKE_MT 0xa188
> +#define READ_ONCE(x) (*(volatile typeof(x) *)(&(x)))
>   
>   struct thread {
>   	pthread_t thread;
> -	void *mmio;
> +	volatile uint32_t *forcewake_mt;
>   	int fd;
>   	int bit;
> +	bool done;
>   };
>   
>   static const struct pci_id_match match[] = {
> @@ -77,43 +79,38 @@ static struct pci_device *__igfx_get(void)
>   		pci_iterator_destroy(iter);
>   	}
>   
> +	pci_device_probe(dev);
>   	return dev;
>   }
>   
> -static void *igfx_get_mmio(void)
> +static volatile uint32_t *igfx_mmio_forcewake_mt(void)

I think its overkill to bother with volatile all throughout the chain, 
it's not like it's const. But never mind.

>   {
>   	struct pci_device *pci = __igfx_get();
> -	void *mmio = NULL;
> -	int error;
>   
> -	igt_skip_on(pci == NULL);
> -	igt_skip_on(intel_gen(pci->device_id) != 7);
> +	igt_require(pci && intel_gen(pci->device_id) == 7);
>   
> -	error = pci_device_probe(pci);
> -	igt_assert_eq(error, 0);
> +	intel_mmio_use_pci_bar(pci);
>   
> -	error = pci_device_map_range(pci,
> -				     pci->regions[0].base_addr,
> -				     2*1024*1024,
> -				     PCI_DEV_MAP_FLAG_WRITABLE,
> -				     &mmio);
> -	igt_assert_eq(error, 0);
> -	igt_assert(mmio != NULL);
> -
> -	return mmio;
> +	return (volatile uint32_t *)((char *)igt_global_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->forcewake_mt;
>   
> -	while (1) {
> +	while (!READ_ONCE(t->done)) {

Not really important, but isn't this not allowed to be compiled out 
since it comes from external to the function memory?

>   		*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);
> +		if (!igt_wait((*forcewake_mt & bit) == 0, 50, 1))
> +			return (void *)release_error;
>   	}
>   
>   	return NULL;
> @@ -124,10 +121,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].forcewake_mt = igfx_mmio_forcewake_mt();
> +	t[0].done = false;
>   
>   	for (i = 2; i < 16; i++) {
>   		t[i] = t[0];
> @@ -137,7 +136,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 +191,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);

Check return value just in case?

> +		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].forcewake_mt = 0xfffe << 16;
> +
> +	igt_assert(success);
>   }
> 

But only minor comments so:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev