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

[Chris Wilson]

Quoting Chris Wilson (2018-03-06 10:41:41)
>  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;
> +   void *result = NULL;
> +
> +   while (!result && !READ_ONCE(t->done)) {
> +   /*
> +* The HW is fubar; concurrent mmio access to even
> +* the FORCEWAKE_MT results in a machine lockup, nullifying
> +* the entire purpose of FORCEWAKE_MT... Sigh.
> +*/
> +   pthread_mutex_lock(t->lock);
>  
> -   while (1) {
> *forcewake_mt = bit << 16 | bit;
> -   igt_assert(*forcewake_mt & bit);
> +   if (!igt_wait(*forcewake_mt & bit, 50, 1))
> +   result = (void *)acquire_error;
> +

Imagine there was
   pthread_mutex_unlock(t->lock);
   usleep(1);
   pthread_mutex_lock(t->lock);
here.

> *forcewake_mt = bit << 16;
> -   igt_assert((*forcewake_mt & bit) == 0);
> +   if (!igt_wait((*forcewake_mt & bit) == 0, 50, 1))
> +   result = (void *)release_error;
> +
> +   pthread_mutex_unlock(t->lock);
> }
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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.
v4: Hohum, the mmio bug affects FORCEWAKE_MT as well.

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Reviewed-by: Tvrtko Ursulin 
---
 tests/gen7_forcewake_mt.c | 103 +-
 1 file changed, 74 insertions(+), 29 deletions(-)

diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c
index 07320ef9..d4b602ba 100644
--- a/tests/gen7_forcewake_mt.c
+++ b/tests/gen7_forcewake_mt.c
@@ -41,12 +41,15 @@ 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;
+   pthread_mutex_t *lock;
+   volatile uint32_t *forcewake_mt;
int fd;
int bit;
+   bool done;
 };
 
 static const struct pci_id_match match[] = {
@@ -77,46 +80,51 @@ 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;
+   void *result = NULL;
+
+   while (!result && !READ_ONCE(t->done)) {
+   /*
+* The HW is fubar; concurrent mmio access to even
+* the FORCEWAKE_MT results in a machine lockup, nullifying
+* the entire purpose of FORCEWAKE_MT... Sigh.
+*/
+   pthread_mutex_lock(t->lock);
 
-   while (1) {
*forcewake_mt = bit << 16 | bit;
-   igt_assert(*forcewake_mt & bit);
+   if (!igt_wait(*forcewake_mt & bit, 50, 1))
+   result = (void *)acquire_error;
+
*forcewake_mt = bit << 16;
-   igt_assert((*forcewake_mt & bit) == 0);
+   if (!igt_wait((*forcewake_mt & bit) == 0, 50, 1))
+   result = (void *)release_error;
+
+   pthread_mutex_unlock(t->lock);
}
 
-   return NULL;
+   return result;
 }
 
 #define MI_STORE_REGISTER_MEM   (0x24<<23)
@@ -124,20 +132,30 @@ static void *thread(void *arg)
 igt_simple_main
 {
struct thread t[16];
+   pthread_mutex_t lock;
+   bool success = true;
int i;
 
+   igt_assert(pthread_mutex_init(, NULL) == 0);
+
+   t[0].lock = 
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];
t[i].bit = i;
-   pthread_create([i].thread, NULL, thread, [i]);
+   if (pthread_create([i].thread, NULL, thread, [i])) {
+   igt_warn("Failed to create thread for BIT(%d)\n", i);
+   success = false;
+   goto error;
+   }
}
 
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];
@@ -186,19 +204,46 @@ igt_simple_main
execbuf.batch_len = sizeof(b);

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

[Chris Wilson]

Prevent the compiler from caching reads/writes to the hw register as we
do want to perform mmio.

Signed-off-by: Chris Wilson 
---
 tests/gen7_forcewake_mt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/gen7_forcewake_mt.c b/tests/gen7_forcewake_mt.c
index 07320ef9e..8a23d13e0 100644
--- a/tests/gen7_forcewake_mt.c
+++ b/tests/gen7_forcewake_mt.c
@@ -106,8 +106,9 @@ static void *igfx_get_mmio(void)
 static void *thread(void *arg)
 {
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 =
+   (volatile uint32_t *)((char *)t->mmio + FORCEWAKE_MT);
 
while (1) {
*forcewake_mt = bit << 16 | bit;
-- 
2.15.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx