Re: [Intel-gfx] [PATCH igt v3] igt/gen7_forcewake_mt: Make the mmio register as volatile
Quoting Tvrtko Ursulin (2018-03-06 09:37:50) > > On 05/03/2018 17:30, Chris Wilson wrote: > > -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. When you stub your toe, you should amputate the leg. Just to be sure. > > 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? If there wasn't an ::memory barrier inside the loop, the compiler can assume that the value doesn't change. (It is not marked as volatile). However, we do have volatiles (and so external memory barriers) inside the loop, so should be a moot point. > > *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; > > } > > + > > + for (i = 2; i < 16; i++) { > > + void *result; > > + > > + t[i].done = true; > > + pthread_join(t[i].thread, ); > > Check return value just in case? Here it can only fail in a programming error, shrug. We are better off with an igt_assert() on pthread_create(). -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH igt v3] igt/gen7_forcewake_mt: Make the mmio register as volatile
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 WilsonCc: Tvrtko Ursulin --- 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, -); - 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); } + +
[Intel-gfx] [PATCH igt v3] igt/gen7_forcewake_mt: Make the mmio register as volatile
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 WilsonCc: Tvrtko Ursulin --- 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) { 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, -); - 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)) { *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, ); + if (result) { + igt_warn("Thread BIT(%d) failed to %s forcewake\n", i,
[Intel-gfx] [PATCH igt v3] igt/gen7_forcewake_mt: Make the mmio register as volatile
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. Signed-off-by: Chris WilsonCc: Tvrtko Ursulin --- One igt_assert() left inside the thread that was already duplicated into the soft return. --- tests/gen7_forcewake_mt.c | 58 +-- 1 file changed, 46 insertions(+), 12 deletions(-) diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c index 07320ef9..49e92124 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,27 @@ 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); + if (igt_wait((*forcewake_mt & bit) == 0, 50, 1)) + return (void *)release_error; } return NULL; @@ -124,10 +132,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 +147,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 +202,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, ); + 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