Re: [Intel-gfx] [PATCH i-g-t] lib: Don't specify a non-existent blitter ring

[Antonio Argenziano]

On 18/02/20 06:47, Chris Wilson wrote:

I915_EXEC_BLT only exists on gen6+

Closes: https://gitlab.freedesktop.org/drm/intel/issues/1256
Signed-off-by: Chris Wilson 


Acked-by: Antonio Argenziano 


---
  lib/intel_batchbuffer.c | 23 ---
  1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 5a00481cf..f1a45b473 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -695,20 +695,13 @@ fill_object(struct drm_i915_gem_exec_object2 *obj, 
uint32_t gem_handle,
  
  static void exec_blit(int fd,

  struct drm_i915_gem_exec_object2 *objs, uint32_t count,
- uint32_t batch_len /* in dwords */)
+ int gen)
  {
-   struct drm_i915_gem_execbuffer2 exec;
-
-   exec.buffers_ptr = to_user_pointer(objs);
-   exec.buffer_count = count;
-   exec.batch_start_offset = 0;
-   exec.batch_len = batch_len * 4;
-   exec.DR1 = exec.DR4 = 0;
-   exec.num_cliprects = 0;
-   exec.cliprects_ptr = 0;
-   exec.flags = I915_EXEC_BLT;
-   i915_execbuffer2_set_context_id(exec, 0);
-   exec.rsvd2 = 0;
+   struct drm_i915_gem_execbuffer2 exec = {
+   .buffers_ptr = to_user_pointer(objs),
+   .buffer_count = count,
+   .flags = gen >= 6 ? I915_EXEC_BLT : 0,
+   };
  
  	gem_execbuf(fd, );

  }
@@ -892,7 +885,7 @@ void igt_blitter_src_copy(int fd,
objs[0].flags |= EXEC_OBJECT_NEEDS_FENCE;
objs[1].flags |= EXEC_OBJECT_NEEDS_FENCE;
  
-	exec_blit(fd, objs, 3, ARRAY_SIZE(batch));

+   exec_blit(fd, objs, 3, gen);
  
  	gem_close(fd, batch_handle);

  }
@@ -985,7 +978,7 @@ void igt_blitter_fast_copy__raw(int fd,
fill_object([1], src_handle, NULL, 0);
fill_object([2], batch_handle, relocs, 2);
  
-	exec_blit(fd, objs, 3, ARRAY_SIZE(batch));

+   exec_blit(fd, objs, 3, intel_gen(intel_get_drm_devid(fd)));
  
  	gem_close(fd, batch_handle);

  }


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_schedule: Exercise implicit ordering between engines

[Antonio Argenziano]

On 18/02/20 09:42, Chris Wilson wrote:

Check that reads are serialised after a write, and that a subsequent
write is after all reads.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
Cc: Sravan Kumar Nedunoori 
---
  tests/i915/gem_exec_schedule.c | 73 ++
  1 file changed, 73 insertions(+)

diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
index a20985864..cfd06aa55 100644
--- a/tests/i915/gem_exec_schedule.c
+++ b/tests/i915/gem_exec_schedule.c
@@ -241,6 +241,61 @@ static void fifo(int fd, unsigned ring)
igt_assert_eq_u32(result, 2);
  }
  
+enum implicit_dir {

+   READ_WRITE = 0x1,
+   WRITE_READ = 0x2,
+};
+
+static void implicit_rw(int i915, unsigned ring, enum implicit_dir dir)
+{
+   IGT_CORK_FENCE(cork);
+   unsigned int count;
+   uint32_t scratch;
+   uint32_t result;
+   int fence;
+
+   count = 0;
+   for_each_physical_engine(other, i915) {
+   if (eb_ring(other) == ring)
+   continue;
+
+   count++;
+   }
+   igt_require(count);
+
+   scratch = gem_create(i915, 4096);
+   fence = igt_cork_plug(, i915);
+
+   if (dir & WRITE_READ)
+   store_dword_fenced(i915, 0,
+  ring, scratch, 0, -ring,
+  fence, I915_GEM_DOMAIN_RENDER);
+
+   for_each_physical_engine(other, i915) {
+   if (eb_ring(other) == ring)
+   continue;
+
+   store_dword_fenced(i915, 0,
+  eb_ring(other), scratch, 0, eb_ring(other),
+  fence, 0);
+   }
+
+   if (dir & READ_WRITE)
+   store_dword_fenced(i915, 0,
+  ring, scratch, 0, ring,
+  fence, I915_GEM_DOMAIN_RENDER);
+
+   unplug_show_queue(i915, , ring);
+   close(fence);
+
+   result =  __sync_read_u32(i915, scratch, 0);
+   gem_close(i915, scratch);
+
+   igt_assert_neq_u32(result, -ring);


if (dir & WRITE_READ) ?

Other that that LGTM.

Reviewed-by: Antonio Argenziano 


+   if (dir & READ_WRITE)
+   igt_assert_eq_u32(result, ring);
+}
+
  static void independent(int fd, unsigned int engine)
  {
IGT_CORK_FENCE(cork);
@@ -2042,6 +2097,24 @@ igt_main
fifo(fd, eb_ring(e));
}
  
+			igt_subtest_f("implicit-read-write-%s", e->name) {

+   igt_require(gem_ring_has_physical_engine(fd, 
eb_ring(e)));
+   igt_require(gem_can_store_dword(fd, 
eb_ring(e)));
+   implicit_rw(fd, eb_ring(e), READ_WRITE);
+   }
+
+   igt_subtest_f("implicit-write-read-%s", e->name) {
+   igt_require(gem_ring_has_physical_engine(fd, 
eb_ring(e)));
+   igt_require(gem_can_store_dword(fd, 
eb_ring(e)));
+   implicit_rw(fd, eb_ring(e), WRITE_READ);
+   }
+
+   igt_subtest_f("implicit-both-%s", e->name) {
+   igt_require(gem_ring_has_physical_engine(fd, 
eb_ring(e)));
+   igt_require(gem_can_store_dword(fd, 
eb_ring(e)));
+   implicit_rw(fd, eb_ring(e), READ_WRITE | 
WRITE_READ);
+   }
+
igt_subtest_f("independent-%s", e->name) {
igt_require(gem_ring_has_physical_engine(fd, 
eb_ring(e)));
igt_require(gem_can_store_dword(fd, 
eb_ring(e)));


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/4] i915/gem_ctx_engine: Exercise for_each_context_engine() with custom engine[]

[Antonio Argenziano]

On 14/02/20 11:40, Chris Wilson wrote:

Set up a custom engine map with no engines, and check that the
for_each_context_engine() correctly iterates over nothing.

Signed-off-by: Chris Wilson 
---
  tests/i915/gem_ctx_engines.c | 28 
  1 file changed, 28 insertions(+)

diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
index 063140e0f..6a2edd1e0 100644
--- a/tests/i915/gem_ctx_engines.c
+++ b/tests/i915/gem_ctx_engines.c
@@ -549,6 +549,31 @@ static void independent(int i915)
gem_context_destroy(i915, param.ctx_id);
  }
  
+static void libapi(int i915)

+{
+   struct i915_context_param_engines engines = {};


Is there a case for invalid engines as well?

Acked-by: Antonio Argenziano 


+   struct drm_i915_gem_context_param p = {
+   .ctx_id = gem_context_create(i915),
+   .param = I915_CONTEXT_PARAM_ENGINES,
+   .value = to_user_pointer(),
+   .size = sizeof(engines),
+   };
+   const struct intel_execution_engine2 *e;
+   unsigned int count = 0;
+
+   gem_context_set_param(i915, );
+
+   for_each_context_engine(i915, p.ctx_id, e)
+   count++;
+   igt_assert_eq(count, 0);
+
+   for_each_physical_engine(i915, p.ctx_id, e)
+   count++;
+   igt_assert_eq(count, 0);
+
+   gem_context_destroy(i915, p.ctx_id);
+}
+
  igt_main
  {
int i915 = -1;
@@ -584,6 +609,9 @@ igt_main
igt_subtest("independent")
independent(i915);
  
+	igt_subtest("libapi")

+   libapi(i915);
+
igt_fixture
igt_stop_hang_detector();
  }


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/4] i915/gem_ctx_engines: Exercise 0 engines[]

[Antonio Argenziano]

On 14/02/20 11:40, Chris Wilson wrote:

Setup a context with no engines, and make sure we reject all execution
attempts.

Signed-off-by: Chris Wilson 


Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_ctx_engines.c | 45 
  1 file changed, 45 insertions(+)

diff --git a/tests/i915/gem_ctx_engines.c b/tests/i915/gem_ctx_engines.c
index cb82f08ef..063140e0f 100644
--- a/tests/i915/gem_ctx_engines.c
+++ b/tests/i915/gem_ctx_engines.c
@@ -242,6 +242,48 @@ static void idempotent(int i915)
gem_context_destroy(i915, p.ctx_id);
  }
  
+static uint32_t batch_create(int i915)

+{
+   const uint32_t bbe = MI_BATCH_BUFFER_END;
+   uint32_t handle = gem_create(i915, 4096);
+
+   gem_write(i915, handle, 0, , sizeof(bbe));
+   return handle;
+}
+
+static void none(int i915)
+{
+   struct i915_context_param_engines engines = {};
+   struct drm_i915_gem_context_param p = {
+   .ctx_id = gem_context_create(i915),
+   .param = I915_CONTEXT_PARAM_ENGINES,
+   .value = to_user_pointer(),
+   .size = sizeof(engines),
+   };
+
+   gem_context_set_param(i915, );
+
+   {
+   struct drm_i915_gem_exec_object2 obj = {
+   .handle = batch_create(i915),
+   };
+   struct drm_i915_gem_execbuffer2 execbuf = {
+   .buffers_ptr = to_user_pointer(),
+   .buffer_count = 1,
+   .rsvd1 = p.ctx_id,
+   };
+
+   for (execbuf.flags = 0;
+execbuf.flags <= I915_EXEC_RING_MASK;
+execbuf.flags++)
+   igt_assert_eq(__gem_execbuf(i915, ), -EINVAL);
+
+   gem_close(i915, obj.handle);
+   }
+
+   gem_context_destroy(i915, p.ctx_id);
+}
+
  static void execute_one(int i915)
  {
I915_DEFINE_CONTEXT_PARAM_ENGINES(engines , I915_EXEC_RING_MASK + 1);
@@ -527,6 +569,9 @@ igt_main
igt_subtest("idempotent")
idempotent(i915);
  
+	igt_subtest("none")

+   none(i915);
+
igt_subtest("execute-one")
execute_one(i915);
  


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

Re: [Intel-gfx] [PATCH i-g-t] lib/i915/gem_engine_topology.c - intel_get_current_engine invalid result

[Antonio Argenziano]

On 13/02/20 11:26, Dale B Stimson wrote:

Function intel_get_current_engine() should return NULL (instead of
engine 0) if there are no engines.

Function intel_init_engine_list() should not store potential engine
data in the output structure unless the engine is present.

Function intel_init_engine_list() should arguably not filter the static
engine list with gem_has_ring if fd == -1, so that subtests can still
be individually invoked to show subtest FAIL instead of test notrun.

Symptom: A device open failure in gem_ctx_isolation resulted in
an endless __for_each_physical_engine "per-engine" loop with the
purported last potential engine being processed every time.

Diagnosis: device open (or debugfs open) failed, leaving fd == -1.
Control skipped the rest of the initial igt_fixture block, after
which an attempt was made to iterate through engines using macro
__for_each_physical_engine.

Macro __for_each_physical_engine called intel_init_engine_list()
to initialize the loop control data.  Because fd == -1,
intel_init_engine_list() fell back to using __for_each_static_engine().
All of the engines in the static engine list are rejected due to
gem_has_ring returning false (because of fd == -1), leaving 0 engines.
That resulted in loop control data with engine_data.nengines == 0
and the data for the last engine considered stored at index 0.

Still in macro __for_each_physical_engine, intel_get_current_engine()
was called to get the engine to process.  It should have returned NULL,
but instead returned the engine entry at index 0, which
had received information describing the last potential engine.
This happened without end.

Signed-off-by: Dale B Stimson 
---
  lib/i915/gem_engine_topology.c | 29 -
  1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/lib/i915/gem_engine_topology.c b/lib/i915/gem_engine_topology.c
index 9daa03df4..b8ed49bc9 100644
--- a/lib/i915/gem_engine_topology.c
+++ b/lib/i915/gem_engine_topology.c
@@ -156,10 +156,10 @@ static void query_engine_list(int fd, struct 
intel_engine_data *ed)
  struct intel_execution_engine2 *
  intel_get_current_engine(struct intel_engine_data *ed)
  {
-   if (!ed->n)
-   ed->current_engine = >engines[0];
-   else if (ed->n >= ed->nengines)
+   if (ed->n >= ed->nengines)
ed->current_engine = NULL;
+   else if (!ed->n)
+   ed->current_engine = >engines[0];
  
  	return ed->current_engine;

  }
@@ -222,18 +222,21 @@ struct intel_engine_data intel_init_engine_list(int fd, 
uint32_t ctx_id)
igt_debug("using pre-allocated engine list\n");
  
  		__for_each_static_engine(e2) {

-   struct intel_execution_engine2 *__e2 =
-   _data.engines[engine_data.nengines];
-
-   strcpy(__e2->name, e2->name);
-   __e2->instance   = e2->instance;
-   __e2->class  = e2->class;
-   __e2->flags  = e2->flags;
-   __e2->is_virtual = false;
-
if (igt_only_list_subtests() ||
-   gem_has_ring(fd, e2->flags))
+   (fd < 0) ||


Patch LGTM, Chris do you have any issues merging this before someone 
implements some tests for the infrastructure?


Acked-by: Antonio Argenziano 


+   gem_has_ring(fd, e2->flags)) {
+   struct intel_execution_engine2 *__e2 =
+   _data.engines[
+   engine_data.nengines];
+
+   strcpy(__e2->name, e2->name);
+   __e2->instance   = e2->instance;
+   __e2->class  = e2->class;
+   __e2->flags  = e2->flags;
+   __e2->is_virtual = false;
+
engine_data.nengines++;
+}
}
return engine_data;
}


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

Re: [Intel-gfx] [PATCH i-g-t 2/2] i915/gem_ctx_sseu: Extend the mmapped parameters test

[Antonio Argenziano]

On 06/02/20 02:06, Chris Wilson wrote:

The current implementation of the test only maps the arguments in gtt,
but we have similar problems arising from any of our own custom
pagefault handlers.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
Cc: Dixit Ashutosh 


For the series:

Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_ctx_sseu.c | 18 --
  1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/i915/gem_ctx_sseu.c b/tests/i915/gem_ctx_sseu.c
index 38dc584bc..d558c8baa 100644
--- a/tests/i915/gem_ctx_sseu.c
+++ b/tests/i915/gem_ctx_sseu.c
@@ -364,7 +364,7 @@ test_invalid_args(int fd)
   * Verify that ggtt mapped area can be used as the sseu pointer.
   */
  static void
-test_ggtt_args(int fd)
+test_mmapped_args(int fd, const struct mmap_offset *t)
  {
struct drm_i915_gem_context_param_sseu *sseu;
struct drm_i915_gem_context_param arg = {
@@ -372,17 +372,19 @@ test_ggtt_args(int fd)
.ctx_id = gem_context_create(fd),
.size = sizeof(*sseu),
};
+   void *ptr;
uint32_t bo;
  
  	bo = gem_create(fd, 4096);

-   arg.value = to_user_pointer(gem_mmap__gtt(fd, bo, 4096,
- PROT_READ | PROT_WRITE));
+   ptr = __gem_mmap_offset(fd, bo, 0, 4096, PROT_WRITE, t->type);
+   gem_close(fd, bo);
+   igt_require(ptr);
  
+	arg.value = to_user_pointer(ptr);

igt_assert_eq(__gem_context_get_param(fd, ), 0);
igt_assert_eq(__gem_context_set_param(fd, ), 0);
  
  	munmap((void *)(uintptr_t)arg.value, 4096);

-   gem_close(fd, bo);
gem_context_destroy(fd, arg.ctx_id);
  }
  
@@ -528,8 +530,12 @@ igt_main

igt_subtest("invalid-sseu")
test_invalid_sseu(fd);
  
-		igt_subtest("ggtt-args")

-   test_ggtt_args(fd);
+   igt_subtest_with_dynamic("mmap-args") {
+   for_each_mmap_offset_type(t) {
+   igt_dynamic_f("%s", t->name)
+   test_mmapped_args(fd, t);
+   }
+   }
  
  		igt_subtest("engines")

test_engines(fd);


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_basic: Drop per-engine testing of *execbuf

[Antonio Argenziano]

On 03/02/20 13:45, Chris Wilson wrote:

The gtt/readonly tests are nothing to do with execution and engines;
they are strictly checking the copy-from-user of the ioctl arguments.
Drop the silly per-engine tests.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 


LGTM.

Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_exec_basic.c  | 109 +--
  tests/i915/gem_exec_params.c |  61 
  2 files changed, 76 insertions(+), 94 deletions(-)

diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index 70dce34b5..0d05819ce 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -36,84 +36,6 @@ static uint32_t batch_create(int fd)
return handle;
  }
  
-static void batch_fini(int fd, uint32_t handle)

-{
-   gem_sync(fd, handle); /* catch any GPU hang */
-   gem_close(fd, handle);
-}
-
-static void noop(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 execbuf;
-   struct drm_i915_gem_exec_object2 exec;
-
-   gem_require_ring(fd, flags);
-
-   memset(, 0, sizeof(exec));
-
-   exec.handle = batch_create(fd);
-
-   memset(, 0, sizeof(execbuf));
-   execbuf.buffers_ptr = to_user_pointer();
-   execbuf.buffer_count = 1;
-   execbuf.flags = flags;
-   gem_execbuf(fd, );
-
-   batch_fini(fd, exec.handle);
-}
-
-static void readonly(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 *execbuf;
-   struct drm_i915_gem_exec_object2 exec;
-
-   gem_require_ring(fd, flags);
-
-   memset(, 0, sizeof(exec));
-   exec.handle = batch_create(fd);
-
-   execbuf = mmap(NULL, 4096, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
-   igt_assert(execbuf != NULL);
-
-   execbuf->buffers_ptr = to_user_pointer();
-   execbuf->buffer_count = 1;
-   execbuf->flags = flags;
-   igt_assert(mprotect(execbuf, 4096, PROT_READ) == 0);
-
-   gem_execbuf(fd, execbuf);
-
-   munmap(execbuf, 4096);
-
-   batch_fini(fd, exec.handle);
-}
-
-static void gtt(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 *execbuf;
-   struct drm_i915_gem_exec_object2 *exec;
-   uint32_t handle;
-
-   gem_require_ring(fd, flags);
-
-   handle = gem_create(fd, 4096);
-
-   gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
-   execbuf = gem_mmap__gtt(fd, handle, 4096, PROT_WRITE);
-   exec = (struct drm_i915_gem_exec_object2 *)(execbuf + 1);
-   gem_close(fd, handle);
-
-   exec->handle = batch_create(fd);
-
-   execbuf->buffers_ptr = to_user_pointer(exec);
-   execbuf->buffer_count = 1;
-   execbuf->flags = flags;
-
-   gem_execbuf(fd, execbuf);
-
-   batch_fini(fd, exec->handle);
-   munmap(execbuf, 4096);
-}
-
  igt_main
  {
const struct intel_execution_engine2 *e;
@@ -121,30 +43,29 @@ igt_main
  
  	igt_fixture {

fd = drm_open_driver(DRIVER_INTEL);
-   igt_require_gem(fd);
-
+   /* igt_require_gem(fd); // test is mandatory */
igt_fork_hang_detector(fd);
}
  
  	igt_subtest_with_dynamic("basic") {

-   __for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   noop(fd, e->flags);
-   }
-   }
+   struct drm_i915_gem_exec_object2 exec = {
+   .handle = batch_create(fd),
+   };
  
-	igt_subtest_with_dynamic("readonly") {

__for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   readonly(fd, e->flags);
+   igt_dynamic_f("%s", e->name) {
+   struct drm_i915_gem_execbuffer2 execbuf = {
+   .buffers_ptr = to_user_pointer(),
+   .buffer_count = 1,
+   .flags = e->flags,
+   };
+
+   gem_execbuf(fd, );
+   }
}
-   }
  
-	igt_subtest_with_dynamic("gtt") {

-   __for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   gtt(fd, e->flags);
-   }
+   gem_sync(fd, exec.handle); /* catch any GPU hang */
+   gem_close(fd, exec.handle);
}
  
  	igt_fixture {

diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
index 9c3525698..094fa904c 100644
--- a/tests/i915/gem_exec_params.c
+++ b/tests/i915/gem_exec_params.c
@@ -206,6 +206,61 @@ static int has_secure_batches(const int fd)
return v > 0;
  }
  
+static uint32_t batch_create(int fd)

+{
+   const uint32_t bbe

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_basic: Drop per-engine testing of *execbuf

[Antonio Argenziano]

On 03/02/20 14:18, Chris Wilson wrote:

Quoting Antonio Argenziano (2020-02-03 22:15:26)



On 03/02/20 13:45, Chris Wilson wrote:

@@ -121,30 +43,29 @@ igt_main
   
   igt_fixture {

   fd = drm_open_driver(DRIVER_INTEL);
- igt_require_gem(fd);
-
+ /* igt_require_gem(fd); // test is mandatory */


What if that fd is bad? Assert?


fd can't be bad, that's taken care of by drm_open_driver(). If the
driver is unusable for execbuf, we want the test failure. It's a choice.

Not sure the right answer, both have advantages.


Fair enough.




+static void mmapped(int i915)
+{
+ struct drm_i915_gem_execbuffer2 *execbuf;
+ struct drm_i915_gem_exec_object2 *exec;
+ uint32_t handle;


gem_require_mappable_ggtt()?


No... I was dropping a hint in the name. How's the toolbox?


I'm afraid I'm not the right person to develop new stuff for IGT... 
gem_require_mappable_ggtt() /* insert comment about extending the test 
*/? :)


Antonio


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_basic: Drop per-engine testing of *execbuf

[Antonio Argenziano]

On 03/02/20 13:45, Chris Wilson wrote:

The gtt/readonly tests are nothing to do with execution and engines;
they are strictly checking the copy-from-user of the ioctl arguments.
Drop the silly per-engine tests.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
---
  tests/i915/gem_exec_basic.c  | 109 +--
  tests/i915/gem_exec_params.c |  61 
  2 files changed, 76 insertions(+), 94 deletions(-)

diff --git a/tests/i915/gem_exec_basic.c b/tests/i915/gem_exec_basic.c
index 70dce34b5..0d05819ce 100644
--- a/tests/i915/gem_exec_basic.c
+++ b/tests/i915/gem_exec_basic.c
@@ -36,84 +36,6 @@ static uint32_t batch_create(int fd)
return handle;
  }
  
-static void batch_fini(int fd, uint32_t handle)

-{
-   gem_sync(fd, handle); /* catch any GPU hang */
-   gem_close(fd, handle);
-}
-
-static void noop(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 execbuf;
-   struct drm_i915_gem_exec_object2 exec;
-
-   gem_require_ring(fd, flags);
-
-   memset(, 0, sizeof(exec));
-
-   exec.handle = batch_create(fd);
-
-   memset(, 0, sizeof(execbuf));
-   execbuf.buffers_ptr = to_user_pointer();
-   execbuf.buffer_count = 1;
-   execbuf.flags = flags;
-   gem_execbuf(fd, );
-
-   batch_fini(fd, exec.handle);
-}
-
-static void readonly(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 *execbuf;
-   struct drm_i915_gem_exec_object2 exec;
-
-   gem_require_ring(fd, flags);
-
-   memset(, 0, sizeof(exec));
-   exec.handle = batch_create(fd);
-
-   execbuf = mmap(NULL, 4096, PROT_WRITE, MAP_ANON | MAP_PRIVATE, -1, 0);
-   igt_assert(execbuf != NULL);
-
-   execbuf->buffers_ptr = to_user_pointer();
-   execbuf->buffer_count = 1;
-   execbuf->flags = flags;
-   igt_assert(mprotect(execbuf, 4096, PROT_READ) == 0);
-
-   gem_execbuf(fd, execbuf);
-
-   munmap(execbuf, 4096);
-
-   batch_fini(fd, exec.handle);
-}
-
-static void gtt(int fd, uint64_t flags)
-{
-   struct drm_i915_gem_execbuffer2 *execbuf;
-   struct drm_i915_gem_exec_object2 *exec;
-   uint32_t handle;
-
-   gem_require_ring(fd, flags);
-
-   handle = gem_create(fd, 4096);
-
-   gem_set_domain(fd, handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
-   execbuf = gem_mmap__gtt(fd, handle, 4096, PROT_WRITE);
-   exec = (struct drm_i915_gem_exec_object2 *)(execbuf + 1);
-   gem_close(fd, handle);
-
-   exec->handle = batch_create(fd);
-
-   execbuf->buffers_ptr = to_user_pointer(exec);
-   execbuf->buffer_count = 1;
-   execbuf->flags = flags;
-
-   gem_execbuf(fd, execbuf);
-
-   batch_fini(fd, exec->handle);
-   munmap(execbuf, 4096);
-}
-
  igt_main
  {
const struct intel_execution_engine2 *e;
@@ -121,30 +43,29 @@ igt_main
  
  	igt_fixture {

fd = drm_open_driver(DRIVER_INTEL);
-   igt_require_gem(fd);
-
+   /* igt_require_gem(fd); // test is mandatory */


What if that fd is bad? Assert?


igt_fork_hang_detector(fd);
}
  
  	igt_subtest_with_dynamic("basic") {

-   __for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   noop(fd, e->flags);
-   }
-   }
+   struct drm_i915_gem_exec_object2 exec = {
+   .handle = batch_create(fd),
+   };
  
-	igt_subtest_with_dynamic("readonly") {

__for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   readonly(fd, e->flags);
+   igt_dynamic_f("%s", e->name) {
+   struct drm_i915_gem_execbuffer2 execbuf = {
+   .buffers_ptr = to_user_pointer(),
+   .buffer_count = 1,
+   .flags = e->flags,
+   };
+
+   gem_execbuf(fd, );
+   }
}
-   }
  
-	igt_subtest_with_dynamic("gtt") {

-   __for_each_physical_engine(fd, e) {
-   igt_dynamic_f("%s", e->name)
-   gtt(fd, e->flags);
-   }
+   gem_sync(fd, exec.handle); /* catch any GPU hang */
+   gem_close(fd, exec.handle);
}
  
  	igt_fixture {

diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
index 9c3525698..094fa904c 100644
--- a/tests/i915/gem_exec_params.c
+++ b/tests/i915/gem_exec_params.c
@@ -206,6 +206,61 @@ static int has_secure_batches(const int fd)
return v > 0;
  }
  
+static uint32_t batch_create(int fd)

+{
+   const uint32_t bbe

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_reloc: Add SIGINT injection

[Antonio Argenziano]

On 29/01/20 14:24, Chris Wilson wrote:

Do a pass over gem_exec_reloc where we inject lots of SIGINTs.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 


LGTM.

Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_exec_reloc.c | 13 +
  1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_reloc.c b/tests/i915/gem_exec_reloc.c
index bc904a0ae..1aa03fba3 100644
--- a/tests/i915/gem_exec_reloc.c
+++ b/tests/i915/gem_exec_reloc.c
@@ -379,7 +379,8 @@ static bool has_64b_reloc(int fd)
  
  #define NORELOC 1

  #define ACTIVE 2
-#define HANG 4
+#define INTERRUPTIBLE 4
+#define HANG 8
  static void basic_reloc(int fd, unsigned before, unsigned after, unsigned 
flags)
  {
  #define OBJSZ 8192
@@ -735,6 +736,7 @@ igt_main
{ "", 0 , true},
{ "-noreloc", NORELOC, true },
{ "-active", ACTIVE, true },
+   { "-interruptible", ACTIVE | INTERRUPTIBLE },
{ "-hang", ACTIVE | HANG },
{ },
}, *f;
@@ -762,14 +764,17 @@ igt_main
  f->name) {
if ((m->before | m->after) & 
I915_GEM_DOMAIN_WC)

igt_require(gem_mmap__has_wc(fd));
-   basic_reloc(fd, m->before, m->after, 
f->flags);
+   igt_while_interruptible(f->flags & 
INTERRUPTIBLE)
+   basic_reloc(fd, m->before, 
m->after, f->flags);
}
}
  
  			if (!(f->flags & NORELOC)) {

igt_subtest_f("%srange%s",
- f->basic ? "basic-" : "", f->name)
-   basic_range(fd, f->flags);
+ f->basic ? "basic-" : "", 
f->name) {
+   igt_while_interruptible(f->flags & 
INTERRUPTIBLE)
+   basic_range(fd, f->flags);
+   }
}
  
  			igt_fixture {



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

Re: [Intel-gfx] [RFC PATCH i-g-t] tests/gem_userptr_blits: Enhance invalid mapping exercise

[Antonio Argenziano]

On 22/01/20 08:26, Janusz Krzysztofik wrote:

Working with a userptr GEM object backed by any type of mapping to
another GEM object, not only GTT mapping currently examined bu the
test, may cause a currently unavoidable lockdep splat inside the i915
driver.  Then, such operations are expected to fail in advance to
prevent from that badness to happen.

Extend the scope of the test by adding subtests which exercise other,
non-GTT mapping types.  Moreover, also succeed if a failure happens
as soon as a userptr object is created on top of an invalid mapping.

Suggested-by: Chris Wilson 
Signed-off-by: Janusz Krzysztofik 
---
  tests/i915/gem_userptr_blits.c | 87 +++---
  1 file changed, 59 insertions(+), 28 deletions(-)

diff --git a/tests/i915/gem_userptr_blits.c b/tests/i915/gem_userptr_blits.c
index a8d3783f..69e5bd1f 100644
--- a/tests/i915/gem_userptr_blits.c
+++ b/tests/i915/gem_userptr_blits.c
@@ -60,6 +60,7 @@
  
  #include "drm.h"

  #include "i915_drm.h"
+#include "i915/gem_mman.h"
  
  #include "intel_bufmgr.h"
  
@@ -577,11 +578,11 @@ static int test_invalid_null_pointer(int fd)

return 0;
  }
  
-static int test_invalid_gtt_mapping(int fd)

+static int test_invalid_mapping(int fd, uint64_t flags)
  {
-   struct drm_i915_gem_mmap_gtt arg;
+   struct drm_i915_gem_mmap_offset arg;
uint32_t handle;
-   char *gtt, *map;
+   char *offset, *map;
  
  	/* Anonymous mapping to find a hole */

map = mmap(NULL, sizeof(linear) + 2 * PAGE_SIZE,
@@ -602,39 +603,46 @@ static int test_invalid_gtt_mapping(int fd)
igt_assert_eq(copy(fd, handle, handle), 0);
gem_close(fd, handle);
  
-	/* GTT mapping */

+   /* object mapping */
memset(, 0, sizeof(arg));
arg.handle = create_bo(fd, 0);
-   do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, );
-   gtt = mmap(map + PAGE_SIZE, sizeof(linear),
-  PROT_READ | PROT_WRITE,
-  MAP_SHARED | MAP_FIXED,
-  fd, arg.offset);
-   igt_assert(gtt == map + PAGE_SIZE);
+   arg.flags = flags;
+   do_ioctl(fd, DRM_IOCTL_I915_GEM_MMAP_OFFSET, );
+   offset = mmap(map + PAGE_SIZE, sizeof(linear), PROT_READ | PROT_WRITE,
+ MAP_SHARED | MAP_FIXED, fd, arg.offset);
+   igt_assert(offset == map + PAGE_SIZE);
gem_close(fd, arg.handle);
-   igt_assert(((unsigned long)gtt & (PAGE_SIZE - 1)) == 0);
+   igt_assert(((unsigned long)offset & (PAGE_SIZE - 1)) == 0);
igt_assert((sizeof(linear) & (PAGE_SIZE - 1)) == 0);
  
-	gem_userptr(fd, gtt, sizeof(linear), 0, userptr_flags, );

-   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
-   gem_close(fd, handle);
+   if (!__gem_userptr(fd, offset, sizeof(linear), 0, userptr_flags,
+   )) {


Not sure I follow why you converted this to an if. Do we expect the 
userptr IOCTL not to work? Could you add a small comment.


Antonio


+   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
+   gem_close(fd, handle);
+   }
  
-	gem_userptr(fd, gtt, PAGE_SIZE, 0, userptr_flags, );

-   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
-   gem_close(fd, handle);
+   if (!__gem_userptr(fd, offset, PAGE_SIZE, 0, userptr_flags, )) {
+   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
+   gem_close(fd, handle);
+   }
  
-	gem_userptr(fd, gtt + sizeof(linear) - PAGE_SIZE, PAGE_SIZE, 0, userptr_flags, );

-   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
-   gem_close(fd, handle);
+   if (!__gem_userptr(fd, offset + sizeof(linear) - PAGE_SIZE, PAGE_SIZE,
+   0, userptr_flags, )) {
+   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
+   gem_close(fd, handle);
+   }
  
  	/* boundaries */

-   gem_userptr(fd, map, 2*PAGE_SIZE, 0, userptr_flags, );
-   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
-   gem_close(fd, handle);
+   if (!__gem_userptr(fd, map, 2 * PAGE_SIZE, 0, userptr_flags, )) {
+   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
+   gem_close(fd, handle);
+   }
  
-	gem_userptr(fd, map + sizeof(linear), 2*PAGE_SIZE, 0, userptr_flags, );

-   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
-   gem_close(fd, handle);
+   if (!__gem_userptr(fd, map + sizeof(linear), 2 * PAGE_SIZE, 0,
+   userptr_flags, )) {
+   igt_assert_eq(copy(fd, handle, handle), -EFAULT);
+   gem_close(fd, handle);
+   }
  
  	munmap(map, sizeof(linear) + 2*PAGE_SIZE);
  
@@ -2009,8 +2017,31 @@ igt_main_args("c:", NULL, help_str, opt_handler, NULL)

igt_subtest("invalid-null-pointer")
test_invalid_null_pointer(fd);
  
-		igt_subtest("invalid-gtt-mapping")

-   test_invalid_gtt_mapping(fd);
+   igt_describe("Verify userptr on top of GTT mapping to GEM object 
will 

Re: [Intel-gfx] [PATCH i-g-t] i915: Mark up a few more tests that only target GGTT

[Antonio Argenziano]

On 12/11/19 10:21, Chris Wilson wrote:

Quoting Antonio Argenziano (2019-11-12 18:17:41)



On 12/11/19 07:47, Chris Wilson wrote:

If a test is only targeting the GGTT API and its corner cases, it can
only run if we have a mappable aperture.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
---
   lib/i915/gem_mman.c | 19 +++
   lib/i915/gem_mman.h |  3 +++
   tests/i915/gem_gtt_cpu_tlb.c|  1 +
   tests/i915/gem_gtt_hog.c|  1 +
   tests/i915/gem_gtt_speed.c  |  2 ++
   tests/i915/gem_mmap_gtt.c   |  5 +
   tests/i915/gem_tiled_swapping.c |  1 +
   7 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index 3cf9a6bbd..76d0be82e 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -252,3 +252,22 @@ void *gem_mmap__cpu(int fd, uint32_t handle, uint64_t 
offset, uint64_t size, uns
   igt_assert(ptr);
   return ptr;
   }
+
+bool gem_has_ggtt(int i915)


nit: I would put mapping or map or something in the name to make it
clear that the mapping is not accessible but, your call :).


I could go with has_ggtt_aperture()? Or has_mappable_ggtt()?
Both have their merits. Leaning towards has_mappable_ggtt.


+1 has_mappable_ggtt.


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t] i915: Mark up a few more tests that only target GGTT

[Antonio Argenziano]

On 12/11/19 07:47, Chris Wilson wrote:

If a test is only targeting the GGTT API and its corner cases, it can
only run if we have a mappable aperture.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
---
  lib/i915/gem_mman.c | 19 +++
  lib/i915/gem_mman.h |  3 +++
  tests/i915/gem_gtt_cpu_tlb.c|  1 +
  tests/i915/gem_gtt_hog.c|  1 +
  tests/i915/gem_gtt_speed.c  |  2 ++
  tests/i915/gem_mmap_gtt.c   |  5 +
  tests/i915/gem_tiled_swapping.c |  1 +
  7 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/lib/i915/gem_mman.c b/lib/i915/gem_mman.c
index 3cf9a6bbd..76d0be82e 100644
--- a/lib/i915/gem_mman.c
+++ b/lib/i915/gem_mman.c
@@ -252,3 +252,22 @@ void *gem_mmap__cpu(int fd, uint32_t handle, uint64_t 
offset, uint64_t size, uns
igt_assert(ptr);
return ptr;
  }
+
+bool gem_has_ggtt(int i915)


nit: I would put mapping or map or something in the name to make it 
clear that the mapping is not accessible but, your call :).


The patch looks good to me but IGT is quite different than the last time 
I played around with it so:

Acked-by: Antonio Argenziano 


+{
+   struct drm_i915_gem_mmap_gtt arg = {};
+   int err;
+
+   err = 0;
+   if (ioctl(i915, DRM_IOCTL_I915_GEM_MMAP_GTT, ))
+   err = errno;
+   errno = 0;
+
+   return errno != ENODEV;
+}
+
+void gem_require_ggtt(int i915)
+{
+   igt_require_f(gem_has_ggtt(i915),
+ "HW & kernel support for indirect detiling aperture\n");
+}
diff --git a/lib/i915/gem_mman.h b/lib/i915/gem_mman.h
index f7242ed77..67891c8de 100644
--- a/lib/i915/gem_mman.h
+++ b/lib/i915/gem_mman.h
@@ -35,6 +35,9 @@ void *gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, 
uint64_t size, unsi
  #define I915_GEM_DOMAIN_WC 0x80
  #endif
  
+bool gem_has_ggtt(int i915);

+void gem_require_ggtt(int i915);
+
  void *__gem_mmap__gtt(int fd, uint32_t handle, uint64_t size, unsigned prot);
  void *__gem_mmap__cpu(int fd, uint32_t handle, uint64_t offset, uint64_t 
size, unsigned prot);
  void *__gem_mmap__wc(int fd, uint32_t handle, uint64_t offset, uint64_t size, 
unsigned prot);
diff --git a/tests/i915/gem_gtt_cpu_tlb.c b/tests/i915/gem_gtt_cpu_tlb.c
index cf3c543df..a4cbb1034 100644
--- a/tests/i915/gem_gtt_cpu_tlb.c
+++ b/tests/i915/gem_gtt_cpu_tlb.c
@@ -79,6 +79,7 @@ igt_simple_main
igt_skip_on_simulation();
  
  	fd = drm_open_driver(DRIVER_INTEL);

+   gem_require_ggtt(fd);
  
  	handle = gem_create(fd, OBJ_SIZE);
  
diff --git a/tests/i915/gem_gtt_hog.c b/tests/i915/gem_gtt_hog.c

index ca730649c..7a6273936 100644
--- a/tests/i915/gem_gtt_hog.c
+++ b/tests/i915/gem_gtt_hog.c
@@ -161,6 +161,7 @@ igt_simple_main
/* check for an intel gpu before goint nuts. */
int fd = drm_open_driver(DRIVER_INTEL);
igt_require_gem(fd);
+   gem_require_ggtt(fd);
close(fd);
  
  	igt_skip_on_simulation();

diff --git a/tests/i915/gem_gtt_speed.c b/tests/i915/gem_gtt_speed.c
index dfa7216cc..0cdd51dc3 100644
--- a/tests/i915/gem_gtt_speed.c
+++ b/tests/i915/gem_gtt_speed.c
@@ -124,7 +124,9 @@ igt_simple_main_args("s:", NULL, help_str, opt_handler, 
NULL)
  
  	buf = malloc(size);

memset(buf, 0, size);
+
fd = drm_open_driver(DRIVER_INTEL);
+   gem_require_ggtt(fd);
  
  	handle = gem_create(fd, size);

igt_assert(handle);
diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
index ac25f13e2..d31c73ed4 100644
--- a/tests/i915/gem_mmap_gtt.c
+++ b/tests/i915/gem_mmap_gtt.c
@@ -1020,8 +1020,6 @@ igt_main
OBJECT_SIZE = 1 * 1024 * 1024;
  
  	igt_fixture {

-   struct drm_i915_gem_mmap_gtt arg = {};
-
fd = drm_open_driver(DRIVER_INTEL);
  
  		/*

@@ -1029,8 +1027,7 @@ igt_main
 * detiling access from untrusted userspace to the objects,
 * the kernel does an early rejection of the mmap_gtt ioctl.
 */
-   igt_require_f(mmap_ioctl(fd, ) != -ENODEV,
- "HW & kernel support for indirect detiling 
aperture\n");
+   gem_require_ggtt(fd);
}
  
  	igt_subtest("bad-object") {

diff --git a/tests/i915/gem_tiled_swapping.c b/tests/i915/gem_tiled_swapping.c
index 1b70c1e51..3a95c9469 100644
--- a/tests/i915/gem_tiled_swapping.c
+++ b/tests/i915/gem_tiled_swapping.c
@@ -175,6 +175,7 @@ igt_main
current_tiling_mode = I915_TILING_X;
  
  		fd = drm_open_driver(DRIVER_INTEL);

+   gem_require_ggtt(fd);
  
  		intel_purge_vm_caches(fd);

check_memory_layout(fd);


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

Re: [Intel-gfx] [PATCH i-g-t] overlay: Drop legacy mmio access

[Antonio Argenziano]

On 03/07/19 08:30, Chris Wilson wrote:

Before the i915_pmu kernel interface was available, we had to rely on
doing some decidedly dodgy mmio access to registers. However, now that
we have a stable interface via perf for grabbing all the details we
need, that and its supporting infrastructure can be discarded.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
Cc: Tvrtko Ursulin 


LGTM.

Acked-by: Antonio Argenziano 


---
  overlay/Makefile.am |   2 -
  overlay/gpu-top.c   | 167 +---
  overlay/igfx.c  | 264 
  overlay/igfx.h  |  44 
  overlay/meson.build |   1 -
  5 files changed, 1 insertion(+), 477 deletions(-)
  delete mode 100644 overlay/igfx.c
  delete mode 100644 overlay/igfx.h

diff --git a/overlay/Makefile.am b/overlay/Makefile.am
index 51643e498..eeeddbba4 100644
--- a/overlay/Makefile.am
+++ b/overlay/Makefile.am
@@ -31,8 +31,6 @@ intel_gpu_overlay_SOURCES = \
gpu-perf.c \
gpu-freq.h \
gpu-freq.c \
-   igfx.h \
-   igfx.c \
overlay.h \
overlay.c \
power.h \
diff --git a/overlay/gpu-top.c b/overlay/gpu-top.c
index 61b8f62fd..6cec2e943 100644
--- a/overlay/gpu-top.c
+++ b/overlay/gpu-top.c
@@ -33,7 +33,6 @@
  
  #include "igt_perf.h"
  
-#include "igfx.h"

  #include "gpu-top.h"
  
  #define RING_TAIL  0x00

@@ -99,176 +98,12 @@ static int perf_init(struct gpu_top *gt)
return 0;
  }
  
-struct mmio_ring {

-   int id;
-   uint32_t base;
-   void *mmio;
-   int idle, wait, sema;
-};
-
-static uint32_t mmio_ring_read(struct mmio_ring *ring, uint32_t reg)
-{
-   return igfx_read(ring->mmio, ring->base + reg);
-}
-
-static int has_execlists(void)
-{
-   int detected = 0;
-   FILE *file;
-
-   file = fopen("/sys/module/i915/parameters/enable_execlists", "r");
-   if (file) {
-   int value;
-   if (fscanf(file, "%d", ) == 1)
-   detected = value != 0;
-   fclose(file);
-   }
-
-   return detected;
-
-}
-
-static void mmio_ring_init(struct mmio_ring *ring, void *mmio)
-{
-   uint32_t ctl;
-
-   ring->mmio = mmio;
-
-   ctl = mmio_ring_read(ring, RING_CTL);
-   if ((ctl & 1) == 0 && !has_execlists())
-   ring->id = -1;
-}
-
-static void mmio_ring_reset(struct mmio_ring *ring)
-{
-   ring->idle = 0;
-   ring->wait = 0;
-   ring->sema = 0;
-}
-
-static void mmio_ring_sample(struct mmio_ring *ring)
-{
-   uint32_t head, tail, ctl;
-
-   if (ring->id == -1)
-   return;
-
-   head = mmio_ring_read(ring, RING_HEAD) & ADDR_MASK;
-   tail = mmio_ring_read(ring, RING_TAIL) & ADDR_MASK;
-   ring->idle += head == tail;
-
-   ctl = mmio_ring_read(ring, RING_CTL);
-   ring->wait += !!(ctl & RING_WAIT);
-   ring->sema += !!(ctl & RING_WAIT_SEMAPHORE);
-}
-
-static void mmio_ring_emit(struct mmio_ring *ring, int samples, union 
gpu_top_payload *payload)
-{
-   if (ring->id == -1)
-   return;
-
-   payload[ring->id].u.busy = 100 - 100 * ring->idle / samples;
-   payload[ring->id].u.wait = 100 * ring->wait / samples;
-   payload[ring->id].u.sema = 100 * ring->sema / samples;
-}
-
-static void mmio_init(struct gpu_top *gt)
-{
-   struct mmio_ring render_ring = {
-   .base = 0x2030,
-   .id = 0,
-   }, bsd_ring = {
-   .base = 0x4030,
-   .id = 1,
-   }, bsd6_ring = {
-   .base = 0x12030,
-   .id = 1,
-   }, blt_ring = {
-   .base = 0x22030,
-   .id = 2,
-   };
-   const struct igfx_info *info;
-   struct pci_device *igfx;
-   void *mmio;
-   int fd[2], i;
-
-   igfx = igfx_get();
-   if (!igfx)
-   return;
-
-   if (pipe(fd) < 0)
-   return;
-
-   info = igfx_get_info(igfx);
-
-   switch (fork()) {
-   case -1: return;
-   default:
-fcntl(fd[0], F_SETFL, fcntl(fd[0], F_GETFL) | O_NONBLOCK);
-gt->fd = fd[0];
-gt->type = MMIO;
-gt->ring[0].name = "render";
-gt->num_rings = 1;
-if (info->gen >= 040) {
-gt->ring[1].name = "bitstream";
-gt->num_rings++;
-}
-if (info->gen >= 060) {
-gt->ring[2].name = "blt";
-gt->num_rings++;
-}
-close(fd[1]);
-return;
-   case 0:
-close(fd[0]);
-break;
-   }
-
-   mmio = igfx_get_mmio(igfx);
-   if (mmio == NULL)
-   

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_ctx_shared: Fixup vec0 mmio base for icl

[Antonio Argenziano]

On 04/06/19 09:29, Chris Wilson wrote:

I told vecs0 to use vecs1 registers...

Signed-off-by: Chris Wilson 
---
  tests/i915/gem_ctx_shared.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/i915/gem_ctx_shared.c b/tests/i915/gem_ctx_shared.c
index 67ecd0953..069964546 100644
--- a/tests/i915/gem_ctx_shared.c
+++ b/tests/i915/gem_ctx_shared.c
@@ -544,9 +544,11 @@ static void independent(int i915, unsigned ring, unsigned 
flags)
mmio_base = 0x22000;
break;
  
+#define GEN11_VECS0_BASE 0x1c8000

+#define GEN11_VECS1_BASE 0x1d8000


VECS1 coming next?


case I915_EXEC_VEBOX:


There is a commented-out case for BSD, why is that?


if (intel_gen(intel_get_drm_devid(i915)) >= 11)
-   mmio_base = 0x1d8000;
+   mmio_base = GEN11_VECS0_BASE;


Might as well define all bases.

Still, fixes the test so:

Reviewed-by: Antonio Argenziano 

Antonio


else
mmio_base = 0x1a000;
break;


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

Re: [Intel-gfx] [PATCH i-g-t 4/5] testdisplay: use first available option values

[Antonio Argenziano]

On 29/05/19 16:27, Lucas De Marchi wrote:

Now that core options are set to 500 and above, start from the lowest
values without causing problems with conflicts. This also rename the
constants to follow the names from the core.

Signed-off-by: Lucas De Marchi 


Acked-by: Antonio Argenziano 


---
  tests/testdisplay.c | 14 --
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/testdisplay.c b/tests/testdisplay.c
index b4f0d45f..32590547 100644
--- a/tests/testdisplay.c
+++ b/tests/testdisplay.c
@@ -69,8 +69,10 @@
  #include 
  #include 
  
-#define Yb_OPT		 5

-#define Yf_OPT  6
+enum {
+   OPT_YB,
+   OPT_YF,
+};
  
  static int tio_fd;

  struct termios saved_tio;
@@ -573,8 +575,8 @@ static void set_termio_mode(void)
  
  static char optstr[] = "3iaf:s:d:p:mrto:j:y";

  static struct option long_opts[] = {
-   {"yb", 0, 0, Yb_OPT},
-   {"yf", 0, 0, Yf_OPT},
+   {"yb", 0, 0, OPT_YB},
+   {"yf", 0, 0, OPT_YF},
{ 0, 0, 0, 0 }
  };
  
@@ -648,10 +650,10 @@ static int opt_handler(int opt, int opt_index, void *data)

tiling = LOCAL_I915_FORMAT_MOD_X_TILED;
break;
case 'y':
-   case Yb_OPT:
+   case OPT_YB:
tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
break;
-   case Yf_OPT:
+   case OPT_YF:
tiling = LOCAL_I915_FORMAT_MOD_Yf_TILED;
break;
case 'r':


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

Re: [Intel-gfx] [PATCH i-g-t 2/5] lib/igt_core: reserve long options for individual tests

[Antonio Argenziano]

On 29/05/19 16:27, Lucas De Marchi wrote:

Start the core optiosn from 500 so the individual tests can have their
own options starting from 0. This makes it easier to set the long
options without conflicting.

500 is just a magic number, higher than any ascii char that could be
used in the individual test.

While at it, fix the coding style to use tab rather than space.

Signed-off-by: Lucas De Marchi 


Acked-by: Antonio Argenziano 


---
  lib/igt_core.c | 16 ++--
  1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index 9c86d664..814f5c72 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -280,12 +280,16 @@ int test_children_sz;
  bool test_child;
  
  enum {

- OPT_LIST_SUBTESTS,
- OPT_RUN_SUBTEST,
- OPT_DESCRIPTION,
- OPT_DEBUG,
- OPT_INTERACTIVE_DEBUG,
- OPT_HELP = 'h'
+   /*
+* Let the first values be used by individual tests so options don't
+* conflict with core ones
+*/
+   OPT_LIST_SUBTESTS = 500,
+   OPT_RUN_SUBTEST,
+   OPT_DESCRIPTION,
+   OPT_DEBUG,
+   OPT_INTERACTIVE_DEBUG,
+   OPT_HELP = 'h'
  };
  
  static int igt_exitcode = IGT_EXIT_SUCCESS;



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

[Intel-gfx] [PATCH i-g-t] tests/i915/gem_madvise.c: Add more mappings

[Antonio Argenziano]

Check madvise versus more memory mappings.

Suggested-by: Chris Wilson 
Signed-off-by: Antonio Argenziano 
---
 tests/i915/gem_madvise.c | 115 ++-
 1 file changed, 76 insertions(+), 39 deletions(-)

diff --git a/tests/i915/gem_madvise.c b/tests/i915/gem_madvise.c
index 729a4d33..bcaaa22e 100644
--- a/tests/i915/gem_madvise.c
+++ b/tests/i915/gem_madvise.c
@@ -47,66 +47,103 @@ IGT_TEST_DESCRIPTION("Checks that the kernel reports 
EFAULT when trying to use"
  *
  */
 
-static jmp_buf jmp;
+static sigjmp_buf jmp;
 
 static void __attribute__((noreturn)) sigtrap(int sig)
 {
-   longjmp(jmp, sig);
+   siglongjmp(jmp, sig);
 }
 
+enum mode { CPU, WC, GTT };
+const char* modes[] = {[CPU] = "cpu", [WC] = "wc", [GTT] = "gtt"};
+
 static void
 dontneed_before_mmap(void)
 {
-   int fd = drm_open_driver(DRIVER_INTEL);
+   int fd;
uint32_t handle;
char *ptr;
 
-   handle = gem_create(fd, OBJECT_SIZE);
-   gem_madvise(fd, handle, I915_MADV_DONTNEED);
-   ptr = gem_mmap__gtt(fd, handle, OBJECT_SIZE, PROT_READ | PROT_WRITE);
-   close(fd);
-
-   signal(SIGSEGV, sigtrap);
-   signal(SIGBUS, sigtrap);
-   switch (setjmp(jmp)) {
-   case SIGBUS:
-   break;
-   case 0:
-   *ptr = 0;
-   default:
-   igt_assert(!"reached");
-   break;
+   for (unsigned mode = CPU; mode <= GTT; mode++) {
+   igt_debug("Mapping mode: %s\n", modes[mode]);
+
+   fd = drm_open_driver(DRIVER_INTEL);
+   handle = gem_create(fd, OBJECT_SIZE);
+   gem_madvise(fd, handle, I915_MADV_DONTNEED);
+
+   switch (mode) {
+   case GTT:
+   ptr = gem_mmap__gtt(fd, handle, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   case CPU:
+   ptr = gem_mmap__cpu(fd, handle, 0, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   case WC:
+   ptr = gem_mmap__wc(fd, handle, 0, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   }
+
+   close(fd);
+
+   signal(SIGSEGV, sigtrap);
+   signal(SIGBUS, sigtrap);
+   switch (sigsetjmp(jmp, SIGBUS | SIGSEGV)) {
+   case SIGBUS:
+   break;
+   case 0:
+   *ptr = 0;
+   default:
+   igt_assert(!"reached");
+   break;
+   }
+   munmap(ptr, OBJECT_SIZE);
+   signal(SIGBUS, SIG_DFL);
+   signal(SIGSEGV, SIG_DFL);
}
-   munmap(ptr, OBJECT_SIZE);
-   signal(SIGBUS, SIG_DFL);
-   signal(SIGSEGV, SIG_DFL);
 }
 
 static void
 dontneed_after_mmap(void)
 {
-   int fd = drm_open_driver(DRIVER_INTEL);
+   int fd;
uint32_t handle;
char *ptr;
 
-   handle = gem_create(fd, OBJECT_SIZE);
-   ptr = gem_mmap__gtt(fd, handle, OBJECT_SIZE, PROT_READ | PROT_WRITE);
-   igt_assert(ptr);
-   gem_madvise(fd, handle, I915_MADV_DONTNEED);
-   close(fd);
-
-   signal(SIGBUS, sigtrap);
-   switch (setjmp(jmp)) {
-   case SIGBUS:
-   break;
-   case 0:
-   *ptr = 0;
-   default:
-   igt_assert(!"reached");
-   break;
+   for (unsigned mode = CPU; mode <= GTT; mode++) {
+   igt_debug("Mapping mode: %s\n", modes[mode]);
+
+   fd = drm_open_driver(DRIVER_INTEL);
+   handle = gem_create(fd, OBJECT_SIZE);
+
+   switch (mode) {
+   case GTT:
+   ptr = gem_mmap__gtt(fd, handle, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   case CPU:
+   ptr = gem_mmap__gtt(fd, handle, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   case WC:
+   ptr = gem_mmap__wc(fd, handle, 0, OBJECT_SIZE, 
PROT_READ | PROT_WRITE);
+   break;
+   }
+
+   igt_assert(ptr);
+   gem_madvise(fd, handle, I915_MADV_DONTNEED);
+   close(fd);
+
+   signal(SIGBUS, sigtrap);
+   switch (sigsetjmp(jmp, SIGBUS)) {
+   case SIGBUS:
+   break;
+   case 0:
+   *ptr = 0;
+   default:
+   igt_assert(!"reached");
+  

Re: [Intel-gfx] [PATCH i-g-t] lib: sync with the newer i915_pciids.h from the Kernel

[Antonio Argenziano]

On 21/03/19 11:02, Anusha wrote:

From: Anusha Srivatsa 

Add CML IDS and additional CNL ID.


Please add the kernel commits to be consistent with the previous updates.

Acked-by: Antonio Argenziano 



v2: Copy header from kernel (Jose)
- Change commit message (Lucas)

Cc: José Roberto de Souza 
Cc: Lucas De Marchi 
Signed-off-by: Anusha Srivatsa 
---
  lib/i915_pciids.h | 31 +--
  1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/lib/i915_pciids.h b/lib/i915_pciids.h
index d2fad7b0..291b5e3f 100644
--- a/lib/i915_pciids.h
+++ b/lib/i915_pciids.h
@@ -373,6 +373,30 @@
  #define INTEL_AML_CFL_GT2_IDS(info) \
INTEL_VGA_DEVICE(0x87CA, info)
  
+/* CML GT1 */

+#define INTEL_CML_GT1_IDS(info)\
+   INTEL_VGA_DEVICE(0x9B21, info), \
+   INTEL_VGA_DEVICE(0x9BAA, info), \
+   INTEL_VGA_DEVICE(0x9BAB, info), \
+   INTEL_VGA_DEVICE(0x9BAC, info), \
+   INTEL_VGA_DEVICE(0x9BA0, info), \
+   INTEL_VGA_DEVICE(0x9BA5, info), \
+   INTEL_VGA_DEVICE(0x9BA8, info), \
+   INTEL_VGA_DEVICE(0x9BA4, info), \
+   INTEL_VGA_DEVICE(0x9BA2, info)
+
+/* CML GT2 */
+#define INTEL_CML_GT2_IDS(info)\
+   INTEL_VGA_DEVICE(0x9B41, info), \
+   INTEL_VGA_DEVICE(0x9BCA, info), \
+   INTEL_VGA_DEVICE(0x9BCB, info), \
+   INTEL_VGA_DEVICE(0x9BCC, info), \
+   INTEL_VGA_DEVICE(0x9BC0, info), \
+   INTEL_VGA_DEVICE(0x9BC5, info), \
+   INTEL_VGA_DEVICE(0x9BC8, info), \
+   INTEL_VGA_DEVICE(0x9BC4, info), \
+   INTEL_VGA_DEVICE(0x9BC2, info)
+
  #define INTEL_KBL_IDS(info) \
INTEL_KBL_GT1_IDS(info), \
INTEL_KBL_GT2_IDS(info), \
@@ -436,7 +460,9 @@
INTEL_WHL_U_GT1_IDS(info), \
INTEL_WHL_U_GT2_IDS(info), \
INTEL_WHL_U_GT3_IDS(info), \
-   INTEL_AML_CFL_GT2_IDS(info)
+   INTEL_AML_CFL_GT2_IDS(info), \
+   INTEL_CML_GT1_IDS(info), \
+   INTEL_CML_GT2_IDS(info)
  
  /* CNL */

  #define INTEL_CNL_IDS(info) \
@@ -469,6 +495,7 @@
INTEL_VGA_DEVICE(0x8A57, info), \
INTEL_VGA_DEVICE(0x8A56, info), \
INTEL_VGA_DEVICE(0x8A71, info), \
-   INTEL_VGA_DEVICE(0x8A70, info)
+   INTEL_VGA_DEVICE(0x8A70, info), \
+   INTEL_VGA_DEVICE(0x8A53, info)
  
  #endif /* _I915_PCIIDS_H */



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

Re: [Intel-gfx] [RFC PATCH 2/2] drm/i915: Account for platform without mappable aperture when returning size

[Antonio Argenziano]

On 07/03/19 13:59, Chris Wilson wrote:

Quoting Antonio Argenziano (2019-03-07 19:11:15)

Some devices will not expose a mappable aperture anymore so we need to
return an appropriate value in that case.

Signed-off-by: Antonio Argenziano 

Cc: Matthew Auld 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_gem.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d109f6dbe992..5125a5329100 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -221,7 +221,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void 
*data,
 args->aper_size = ggtt->vm.total;
 args->aper_available_size = args->aper_size - pinned;
  
-   args->mappable_aperture_size = ggtt->mappable_end;

+   args->mappable_aperture_size =
+   HAS_MAPPABLE_APERTURE(to_i915(dev)) ? 
ggtt->mappable_end : (__u64)-ENODEV;


Looking above, args->aper_size will also be 0 so that'll be a good clue


Is it? ggtt should still be there so that shouldn't be zero.

Antonio


for ENODEV. So maybe we can get away with the ambiguous 0 here as we can
use aper_size to differentiate the classes of HW.
-Chris


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

[Intel-gfx] [RFC PATCH 1/2] drm/i915: Return mappable aperture size

[Antonio Argenziano]

Extend the API to return the mappable aperture size directly in the
I915_GEM_GET_APERTURE IOCTL.

Signed-off-by: Antonio Argenziano 
Cc: Matthew Auld 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 include/uapi/drm/i915_drm.h | 5 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0b23cf3be718..d109f6dbe992 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -221,6 +221,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void 
*data,
args->aper_size = ggtt->vm.total;
args->aper_available_size = args->aper_size - pinned;
 
+   args->mappable_aperture_size = ggtt->mappable_end;
+
return 0;
 }
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index aa2d4c73a97d..bcab4579 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1284,6 +1284,11 @@ struct drm_i915_gem_get_aperture {
 * bytes
 */
__u64 aper_available_size;
+
+   /**
+* Total size of the mappable aperture.
+*/
+   __u64 mappable_aperture_size;
 };
 
 struct drm_i915_get_pipe_from_crtc_id {
-- 
2.20.1

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

[Intel-gfx] [RFC PATCH 2/2] drm/i915: Account for platform without mappable aperture when returning size

[Antonio Argenziano]

Some devices will not expose a mappable aperture anymore so we need to
return an appropriate value in that case.

Signed-off-by: Antonio Argenziano 

Cc: Matthew Auld 
Cc: Daniele Ceraolo Spurio 
Cc: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d109f6dbe992..5125a5329100 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -221,7 +221,8 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void 
*data,
args->aper_size = ggtt->vm.total;
args->aper_available_size = args->aper_size - pinned;
 
-   args->mappable_aperture_size = ggtt->mappable_end;
+   args->mappable_aperture_size =
+   HAS_MAPPABLE_APERTURE(to_i915(dev)) ? 
ggtt->mappable_end : (__u64)-ENODEV;
 
return 0;
 }
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_eio: Not everyone actually has contexts

[Antonio Argenziano]

On 21/02/19 02:01, Chris Wilson wrote:

Eek, I assumed the 'banned' subtest only applied to context platforms,
ti doesn't. The basic test works for all, checking whether a second

   ^--- Typo? :).

context works after the first is banned however only applies to
platforms with contexts!



Yeah, I missed that.


Signed-off-by: Chris Wilson 
---
  tests/i915/gem_eio.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index c5fd07585..3f941071d 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -334,13 +334,13 @@ static void __test_banned(int fd)
  
  			/* Only this context, not the file, should be banned */

igt_assert_neq(__gem_context_create(fd, ), -EIO);
-   igt_assert_neq(ctx, 0);


Although this assert seems to suggest it didn't apply to context-less 
platforms as it would fail here.


I think it still makes sense to test you get banned on context 0 so,
Reviwed-by: Antonio Argenziano 


-
-   /* And check it actually works! */
-   execbuf.rsvd1 = ctx;
-   gem_execbuf(fd, );
+   if (ctx) { /* remember the contextless! */
+   /* And check it actually works! */
+   execbuf.rsvd1 = ctx;
+   gem_execbuf(fd, );
  
-			gem_context_destroy(fd, ctx);

+   gem_context_destroy(fd, ctx);
+   }
return;
}
  


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

Re: [Intel-gfx] [PATCH i-g-t 1/2] lib: Only require we have i915.reset module parameter for allow-hang

[Antonio Argenziano]

On 21/02/19 08:11, Chris Wilson wrote:

Quoting Antonio Argenziano (2019-02-21 16:08:04)



On 21/02/19 02:19, Chris Wilson wrote:

To control hang detection, we manipulate the i915.reset module
parameter. However, to be nice we should SKIP if we cannot modify the
parameter as opposed to outright FAILing.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108891
Signed-off-by: Chris Wilson 
---
   lib/igt_gt.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 646696727..c98a7553b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -176,8 +176,8 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned 
flags)
   if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
   igt_require(has_gpu_reset(fd));
   
- igt_assert(igt_sysfs_set_parameter

-(fd, "reset", "%d", INT_MAX /* any reset method */));
+ igt_require(igt_sysfs_set_parameter
+ (fd, "reset", "%d", INT_MAX /* any reset method */));


How come we were not able to write that parameter? Isn't that sysfs
always there?


No. It can be compiled out.


Right...

Reviewed-by: Antonio Argenziano 


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/2] igt/drv_hangman: Skip if resets are disallowed

[Antonio Argenziano]

On 21/02/19 02:19, Chris Wilson wrote:

If we tell the machine to reset but they are disallowed, we will leave
the system in a wedged state, preventing the majority of subsequent
tests.

Signed-off-by: Chris Wilson 


LGTM.

Reviewed-by: Antonio Argenziano 


---
  tests/i915/i915_hangman.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c
index df1e0afed..4e515e3a0 100644
--- a/tests/i915/i915_hangman.c
+++ b/tests/i915/i915_hangman.c
@@ -257,6 +257,7 @@ static void hangcheck_unterminated(void)
  igt_main
  {
const struct intel_execution_engine *e;
+   igt_hang_t hang = {};
  
  	igt_skip_on_simulation();
  
@@ -266,6 +267,8 @@ igt_main

device = drm_open_driver(DRIVER_INTEL);
igt_require_gem(device);
  
+		hang = igt_allow_hang(device, 0, HANG_ALLOW_CAPTURE);

+
sysfs = igt_sysfs_open(device, );
igt_assert(sysfs != -1);
  
@@ -288,4 +291,8 @@ igt_main
  
  	igt_subtest("hangcheck-unterminated")

hangcheck_unterminated();
+
+   igt_fixture {
+   igt_disallow_hang(device, hang);
+   }
  }


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

Re: [Intel-gfx] [PATCH i-g-t 1/2] lib: Only require we have i915.reset module parameter for allow-hang

[Antonio Argenziano]

On 21/02/19 02:19, Chris Wilson wrote:

To control hang detection, we manipulate the i915.reset module
parameter. However, to be nice we should SKIP if we cannot modify the
parameter as opposed to outright FAILing.

References: https://bugs.freedesktop.org/show_bug.cgi?id=108891
Signed-off-by: Chris Wilson 
---
  lib/igt_gt.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 646696727..c98a7553b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -176,8 +176,8 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned 
flags)
if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
igt_require(has_gpu_reset(fd));
  
-	igt_assert(igt_sysfs_set_parameter

-  (fd, "reset", "%d", INT_MAX /* any reset method */));
+   igt_require(igt_sysfs_set_parameter
+   (fd, "reset", "%d", INT_MAX /* any reset method */));


How come we were not able to write that parameter? Isn't that sysfs 
always there?


Antonio

  
  	if ((flags & HANG_ALLOW_CAPTURE) == 0) {

param.param = I915_CONTEXT_PARAM_NO_ERROR_CAPTURE;


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_schedule: Switch bach to gem_set_domain()

[Antonio Argenziano]

On 17/02/19 10:16, Chris Wilson wrote:

The write hazard lies extend also to the cache-dirty tracking; as we
purposefully do not tell the kernel we are writing to the bo, it fails
to note the CPU cache as dirty and so the gem_read() may not
sufficiently flush the caches prior to reading back from the GPU.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 


LGTM.

Reviewed-by: Antonio Argenziano 

Antonio


---
  tests/i915/gem_exec_schedule.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/i915/gem_exec_schedule.c b/tests/i915/gem_exec_schedule.c
index 59102b6bc..a9383000a 100644
--- a/tests/i915/gem_exec_schedule.c
+++ b/tests/i915/gem_exec_schedule.c
@@ -54,7 +54,8 @@ uint32_t __sync_read_u32(int fd, uint32_t handle, uint64_t 
offset)
  {
uint32_t value;
  
-	gem_sync(fd, handle); /* No write hazard lies! */

+   gem_set_domain(fd, handle, /* No write hazard lies! */
+  I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
gem_read(fd, handle, offset, , sizeof(value));
  
  	return value;

@@ -63,7 +64,8 @@ uint32_t __sync_read_u32(int fd, uint32_t handle, uint64_t 
offset)
  static inline
  void __sync_read_u32_count(int fd, uint32_t handle, uint32_t *dst, uint64_t 
size)
  {
-   gem_sync(fd, handle); /* No write hazard lies! */
+   gem_set_domain(fd, handle, /* No write hazard lies! */
+  I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
gem_read(fd, handle, 0, dst, size);
  }
  


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 3/8] lib: Restore the i915.reset modparam before cleaning up

[Antonio Argenziano]

On 17/02/19 06:35, Chris Wilson wrote:

We force a reset on test exit so that we can rapidly cleanup after a
naughty test, it is not unknown for us to leave a queue of hanging
batches around. However, if we have also fiddled with the i915.reset
parameter in the meantime, this can leave the kernel unable to fulfil


typo -^


our request (and those naughty batches continue to disrupt testing).

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Petri Latvala 


Re-enabling reset sounds good.

Acked-by: Antonio Argenziano 


---
  lib/drmtest.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 1964795a6..6c0a0e381 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -54,6 +54,7 @@
  #include "igt_device.h"
  #include "igt_gt.h"
  #include "igt_kmod.h"
+#include "igt_sysfs.h"
  #include "version.h"
  #include "config.h"
  #include "intel_reg.h"
@@ -345,6 +346,7 @@ static void __cancel_work_at_exit(int fd)
  {
igt_terminate_spin_batches(); /* for older kernels */
  
+	igt_sysfs_set_parameter(fd, "reset", "%x", -1u /* any method */);

igt_drop_caches_set(fd,
/* cancel everything */
DROP_RESET_ACTIVE | DROP_RESET_SEQNO |


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_eio: Check we only ban the context

[Antonio Argenziano]

On 19/02/19 09:11, Chris Wilson wrote:

In trigger the ban, we only want to observe the local context be banned
and not the fpriv as a whole.

v2: And send an execbuf down the new context.

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
Cc: Antonio Argenziano 


Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_eio.c | 12 
  1 file changed, 12 insertions(+)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index ac85a2eff..b0be128ef 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -313,8 +313,20 @@ static void __test_banned(int fd)
igt_spin_t *hang;
  
  		if (__gem_execbuf(fd, ) == -EIO) {

+   uint32_t ctx = 0;
+
igt_info("Banned after causing %lu hangs\n", count);
igt_assert(count > 1);
+
+   /* Only this context, not the file, should be banned */
+   igt_assert_neq(__gem_context_create(fd, ), -EIO);
+   igt_assert_neq(ctx, 0);
+
+   /* And check it actually works! */
+   execbuf.rsvd1 = ctx;
+   gem_execbuf(fd, );
+
+   gem_context_destroy(fd, ctx);
return;
}
  


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/8] i915/gem_eio: Check we only ban the context

[Antonio Argenziano]

On 17/02/19 06:35, Chris Wilson wrote:

In trigger the ban, we only want to observe the local context be banned
and not the fpriv as a whole.

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 
---
  tests/i915/gem_eio.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 3c54820c9..3afdbd69e 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -324,8 +324,15 @@ static void __test_banned(int fd)
igt_spin_t *hang;
  
  		if (__gem_execbuf(fd, ) == -EIO) {

+   uint32_t ctx = 0;
+
igt_info("Banned after causing %lu hangs\n", count);
igt_assert(count > 1);
+
+   /* Only this context, not the file, should be banned */
+   igt_assert_neq(__gem_context_create(fd, ), -EIO);


Should we check submission works on the new context?

Antonio


+   igt_assert_neq(ctx, 0);
+   gem_context_destroy(fd, ctx);
return;
}
  


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

Re: [Intel-gfx] [PATCH i-g-t 1/8] i915/gem_eio: Check that context create fails when wedged

[Antonio Argenziano]

On 17/02/19 06:35, Chris Wilson wrote:

Lock down the new uABI that DRM_IOCTL_I915_GEM_CONTEXT_CREATE returns
-EIO when wedged.

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 


LGTM.

Reviewed-by: Antonio Argenziano 


---
  tests/i915/gem_eio.c | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index ac85a2eff..3c54820c9 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -118,6 +118,17 @@ static void test_throttle(int fd)
trigger_reset(fd);
  }
  
+static void test_context_create(int fd)

+{
+   uint32_t ctx;
+
+   wedge_gpu(fd);
+
+   igt_assert_eq(__gem_context_create(fd, ), -EIO);
+
+   trigger_reset(fd);
+}
+
  static void test_execbuf(int fd)
  {
struct drm_i915_gem_execbuffer2 execbuf;
@@ -807,6 +818,9 @@ igt_main
igt_subtest("throttle")
test_throttle(fd);
  
+	igt_subtest("context-create")

+   test_context_create(fd);
+
igt_subtest("execbuf")
test_execbuf(fd);
  


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] i915/gem_exec_latency: Normalize results into ns

[Antonio Argenziano]

On 29/01/19 10:01, Chris Wilson wrote:

Quoting Antonio Argenziano (2019-01-29 17:55:45)



On 29/01/19 01:55, Chris Wilson wrote:

Present the latency results in nanoseconds not RCS cycles.

Signed-off-by: Chris Wilson 
---
   tests/i915/gem_exec_latency.c | 38 +++
   1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_latency.c b/tests/i915/gem_exec_latency.c
index de16322a6..ea44adc14 100644
--- a/tests/i915/gem_exec_latency.c
+++ b/tests/i915/gem_exec_latency.c
@@ -59,6 +59,7 @@
   #define PREEMPT 0x2
   
   static unsigned int ring_size;

+static double rcs_clock;
   
   static void

   poll_ring(int fd, unsigned ring, const char *name)
@@ -207,7 +208,7 @@ static void latency_on_ring(int fd,
   igt_cork_unplug();
   
   gem_set_domain(fd, obj[1].handle, I915_GEM_DOMAIN_GTT, 0);

- gpu_latency = (results[repeats-1] - results[0]) / (double)(repeats-1);
+ gpu_latency = (results[repeats-1] - results[1]) / (double)(repeats-2);


How come you don't like the value at 0? Maybe adding a comment would
make it clearer.


I was thinking of trying to reduce some context warmup latency, but
it doesn't matter and the spinner in the second patch is much more
effective overall.


OK.

Sorry for the long delay, it ended-up in my spam folder. If you still 
need it, the series is:


Reviewed-by: Antonio Argenziano 



-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_exec_reuse: stop the hang detector afterwards

[Antonio Argenziano]

On 11/02/19 09:52, Chris Wilson wrote:

Take responsibility for the state we create, and in particular remember
to kill our child process (the hang detector) before exiting.

Reported-by: Daniel Vetter 
Signed-off-by: Chris Wilson 
Cc: Daniel Vetter 
---
  tests/i915/gem_exec_reuse.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/i915/gem_exec_reuse.c b/tests/i915/gem_exec_reuse.c
index df220be7b..44946528f 100644
--- a/tests/i915/gem_exec_reuse.c
+++ b/tests/i915/gem_exec_reuse.c
@@ -116,7 +116,7 @@ static unsigned int max_nfd(void)
  
  igt_main

  {
-   struct noop no;
+   struct noop no = { .fd = -1 };
unsigned engines[16];
unsigned nengine;
unsigned n;
@@ -213,4 +213,7 @@ igt_main
for (n = 0; n < ncontexts; n++)
gem_context_destroy(no.fd, contexts[n]);
}
+
+   igt_fixture
+   igt_stop_hang_detector(no.fd);


This doesn't take an fd... incoming changes to the lib?

Still makes sense, with fix,

Reviewed-by: Antonio Argenziano 


  }


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

Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_capture: Check the strlen() for an empty file

[Antonio Argenziano]

On 29/01/19 03:24, Chris Wilson wrote:

igt_sysfs_get() only returns NULL if the open() fails, and a valid
string otherwise. So if the read() fails with ENODEV (because sysfs
doesn't provide the driver with an ->open() callback), we return an
empty string, and "No error captured" otherwise.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109481
Signed-off-by: Chris Wilson 


LGTM.

Reviwed-by: Antonio Argenziano 


---
  tests/i915/gem_exec_capture.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index eca73ab11..56837dfca 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -519,6 +519,11 @@ static bool has_capture(int fd)
return async > 0;
  }
  
+static size_t safer_strlen(const char *s)

+{
+   return s ? strlen(s) : 0;
+}
+
  igt_main
  {
const struct intel_execution_engine *e;
@@ -544,7 +549,7 @@ igt_main
  
  		dir = igt_sysfs_open(fd, NULL);

igt_require(igt_sysfs_set(dir, "error", "Begone!"));
-   igt_require(igt_sysfs_get(dir, "error"));
+   igt_require(safer_strlen(igt_sysfs_get(dir, "error")) > 0);
}
  
  	for (e = intel_execution_engines; e->name; e++) {



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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_eio: Check for allow-hang prior to issuing a reset

[Antonio Argenziano]

On 27/01/19 04:49, Chris Wilson wrote:

Check that we are allowed to hang/reset the GPU before we actually do so
for the first time.

Signed-off-by: Chris Wilson 
---
  tests/i915/gem_eio.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 5250a414c..09059c311 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -745,14 +745,14 @@ igt_main
fd = drm_open_driver(DRIVER_INTEL);
igt_device_drop_master(fd);
  
-		igt_require(i915_reset_control(true));

-   igt_force_gpu_reset(fd);
-   igt_install_exit_handler(exit_handler);
-
gem_submission_print_method(fd);
igt_require_gem(fd);
  
  		igt_allow_hang(fd, 0, 0);

+
+   igt_require(i915_reset_control(true));


Don't we do this already in allow_hang?

Antnoio


+   igt_force_gpu_reset(fd);
+   igt_install_exit_handler(exit_handler);
}
  
  	igt_subtest("throttle")



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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] i915/gem_exec_latency: Normalize results into ns

[Antonio Argenziano]

On 29/01/19 01:55, Chris Wilson wrote:

Present the latency results in nanoseconds not RCS cycles.

Signed-off-by: Chris Wilson 
---
  tests/i915/gem_exec_latency.c | 38 +++
  1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/tests/i915/gem_exec_latency.c b/tests/i915/gem_exec_latency.c
index de16322a6..ea44adc14 100644
--- a/tests/i915/gem_exec_latency.c
+++ b/tests/i915/gem_exec_latency.c
@@ -59,6 +59,7 @@
  #define PREEMPT 0x2
  
  static unsigned int ring_size;

+static double rcs_clock;
  
  static void

  poll_ring(int fd, unsigned ring, const char *name)
@@ -207,7 +208,7 @@ static void latency_on_ring(int fd,
igt_cork_unplug();
  
  	gem_set_domain(fd, obj[1].handle, I915_GEM_DOMAIN_GTT, 0);

-   gpu_latency = (results[repeats-1] - results[0]) / (double)(repeats-1);
+   gpu_latency = (results[repeats-1] - results[1]) / (double)(repeats-2);


How come you don't like the value at 0? Maybe adding a comment would 
make it clearer.


  
  	gem_set_domain(fd, obj[2].handle,

   I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
@@ -238,10 +239,11 @@ static void latency_on_ring(int fd,
igt_assert(offset == obj[2].offset);
  
  	gem_set_domain(fd, obj[1].handle, I915_GEM_DOMAIN_GTT, 0);

-   igt_info("%s: dispatch latency: %.2f, execution latency: %.2f (target 
%.2f)\n",
+   igt_info("%s: dispatch latency: %.1fns, execution latency: %.1fns (target 
%.1fns)\n",
 name,
-(end - start) / (double)repeats,
-gpu_latency, (results[repeats - 1] - results[0]) / 
(double)(repeats - 1));
+(end - start) / (double)repeats * rcs_clock,
+gpu_latency * rcs_clock,
+(results[repeats - 1] - results[0]) / (double)(repeats - 1) * 
rcs_clock);
  
  	munmap(map, 64*1024);

munmap(results, 4096);
@@ -620,6 +622,30 @@ rthog_latency_on_ring(int fd, unsigned int engine, const 
char *name, unsigned in
munmap(results, MMAP_SZ);
  }
  
+static double clockrate(void)

+{
+   volatile uint32_t *reg;
+   uint32_t r_start, r_end;
+   struct timespec tv;
+   uint64_t t_start, t_end;
+   uint64_t elapsed;
+
+   reg = (volatile uint32_t *)((volatile char *)igt_global_mmio + 
RCS_TIMESTAMP);
+
+   t_start = igt_nsec_elapsed();
+   r_start = *reg;
+   elapsed = igt_nsec_elapsed() - t_start;
+
+   usleep(1000);
+
+   t_end = igt_nsec_elapsed();
+   r_end = *reg;
+   elapsed += igt_nsec_elapsed() - t_end;
+
+   elapsed = (t_end - t_start) + elapsed / 2;
+   return (r_end - r_start) * 1e9 / elapsed;
+}
+
  igt_main
  {
const struct intel_execution_engine *e;
@@ -640,6 +666,10 @@ igt_main
ring_size = 1024;
  
  		intel_register_access_init(intel_get_pci_device(), false, device);

+   rcs_clock = clockrate();
+   igt_info("RCS timestamp clock: %.3fKHz, %.1fns\n",
+rcs_clock / 1e3, 1e9 / rcs_clock);
+   rcs_clock = 1e9 / rcs_clock;
}
  
  	igt_subtest("all-rtidle-submit")



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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/pm_rpm: Check for functional GEM before use

[Antonio Argenziano]

On 27/01/19 05:13, Chris Wilson wrote:

Check the GPU (using GEM) is up an operational before submitting
commands.


Always a good idea.
Reviewed-by: Antonio Argenziano 



Signed-off-by: Chris Wilson 
---
  tests/pm_rpm.c | 4 
  1 file changed, 4 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 7dcb5586d..be296f525 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -1220,6 +1220,8 @@ static void gem_execbuf_subtest(void)
int sq_x = 5, sq_y = 10, sq_w = 15, sq_h = 20;
uint32_t color;
  
+	igt_require_gem(drm_fd);

+
/* Create and set data while the device is active. */
enable_one_screen_and_wait(_data);
  
@@ -1308,6 +1310,8 @@ static void gem_execbuf_stress_subtest(int rounds, int wait_flags)

struct drm_i915_gem_execbuffer2 execbuf = {};
struct drm_i915_gem_exec_object2 objs[1] = {{}};
  
+	igt_require_gem(drm_fd);

+
if (wait_flags & WAIT_PC8_RES)
igt_require(has_pc8);
  


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

Re: [Intel-gfx] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Antonio Argenziano]

On 16/01/19 09:42, Antonio Argenziano wrote:



On 16/01/19 08:15, Tvrtko Ursulin wrote:


On 11/01/2019 21:28, John Harrison wrote:


On 1/11/2019 09:31, Antonio Argenziano wrote:


On 11/01/19 00:22, Tvrtko Ursulin wrote:


On 11/01/2019 00:47, Antonio Argenziano wrote:

On 07/01/19 08:58, Tvrtko Ursulin wrote:

On 07/01/2019 13:57, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2019-01-07 13:43:29)


On 07/01/2019 11:58, Tvrtko Ursulin wrote:

[snip]

Note about future interaction with preemption: Preemption 
could happen
in a command sequence prior to watchdog counter getting 
disabled,
resulting in watchdog being triggered following preemption 
(e.g. when
watchdog had been enabled in the low priority batch). The 
driver will

need to explicitly disable the watchdog counter as part of the
preemption sequence.


Does the series take care of preemption?


I did not find that it does.


Oh. I hoped that the watchdog was saved as part of the 
context... Then
despite preemption, the timeout would resume from where we left 
off as

soon as it was back on the gpu.

If the timeout remaining was context saved it would be much 
simpler (at

least on first glance), please say it is.


I made my comments going only by the text from the commit message 
and the absence of any preemption special handling.


Having read the spec, the situation seems like this:

  * Watchdog control and threshold register are context saved and 
restored.


  * On a context switch watchdog counter is reset to zero and 
automatically disabled until enabled by a context restore or 
explicitly.


So it sounds the commit message could be wrong that special 
handling is needed from this direction. But read till the end on 
the restriction listed.


  * Watchdog counter is reset to zero and is not accumulated 
across multiple submission of the same context (due preemption).


I read this as - after preemption contexts gets a new full 
timeout allocation. Or in other words, if a context is preempted 
N times, it's cumulative watchdog timeout will be N * set value.


This could be theoretically exploitable to bypass the timeout. If 
a client sets up two contexts with prio -1 and -2, and keeps 
submitting periodical no-op batches against prio -1 context, 
while prio -2 is it's own hog, then prio -2 context defeats the 
watchdog timer. I think.. would appreciate is someone challenged 
this conclusion.


I think you are right that is a possibility but, is that a 
problem? The client can just not set the threshold to bypass the 
timeout. Also because you need the hanging batch to be simply 
preemptible, you cannot disrupt any work from another client that 
is higher priority. This is 


But I think higher priority client can have the same effect on the 
lower priority purely by accident, no?


As a real world example, user kicks off an background transcoding 
job, which happens to use prio -2, and uses the watchdog timer.


At the same time user watches a video from a player of normal 
priority. This causes periodic, say 24Hz, preemption events, due 
frame decoding activity on the same engine as the transcoding client.


Does this defeat the watchdog timer for the former is the question? 
Then the questions of can we do something about it and whether it 
really isn't a problem?


I guess it depends if you consider that timeout as the maximum 
lifespan a workload can have or max contiguous active time.


I believe the intended purpose of the watchdog is to prevent broken 
bitstreams hanging the transcoder/player. That is, it is a form of 
error detection used by the media driver to handle bad user input. So 
if there is a way for the watchdog to be extended indefinitely under 
normal situations, that would be a problem. It means the transcoder 
will not detect the broken input data in a timely manner and 
effectively hang rather than skip over to the next packet. And note 
that broken input data can be caused by something as innocent as a 
dropped packet due to high network contention. No need for any 
malicious activity at all.


My understanding of the intended purpose is the same. And it would be 
a very useful feature.


I'm not familiar enough with the application but, in the scenario above, 
what if the batch that is being preempted is not stuck but just nice 
enough to be preempted enough times so that it wouldn't complete in the 
given wall clock time but would be fast enough by itself.


Ignore me, re-reading this I now get you are trying to advocate for an 
active-time timeout not pure wall clock time.






Chris mentioned the other day that until hardware is fixed to context 
save/restore the watchdog counter this could simply be implemented 
using timers. And I have to say I agree. Shouldn't be too hard to 
prototype it using  hrtimers - start on context in, stop on context 
out and kick forward on user interrupts. More or less.


Would this implement the feature on the driver side just like it would 
for the HW? I mean have the same

Re: [Intel-gfx] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Antonio Argenziano]

On 16/01/19 08:15, Tvrtko Ursulin wrote:


On 11/01/2019 21:28, John Harrison wrote:


On 1/11/2019 09:31, Antonio Argenziano wrote:


On 11/01/19 00:22, Tvrtko Ursulin wrote:


On 11/01/2019 00:47, Antonio Argenziano wrote:

On 07/01/19 08:58, Tvrtko Ursulin wrote:

On 07/01/2019 13:57, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2019-01-07 13:43:29)


On 07/01/2019 11:58, Tvrtko Ursulin wrote:

[snip]

Note about future interaction with preemption: Preemption 
could happen

in a command sequence prior to watchdog counter getting disabled,
resulting in watchdog being triggered following preemption 
(e.g. when
watchdog had been enabled in the low priority batch). The 
driver will

need to explicitly disable the watchdog counter as part of the
preemption sequence.


Does the series take care of preemption?


I did not find that it does.


Oh. I hoped that the watchdog was saved as part of the context... 
Then
despite preemption, the timeout would resume from where we left 
off as

soon as it was back on the gpu.

If the timeout remaining was context saved it would be much 
simpler (at

least on first glance), please say it is.


I made my comments going only by the text from the commit message 
and the absence of any preemption special handling.


Having read the spec, the situation seems like this:

  * Watchdog control and threshold register are context saved and 
restored.


  * On a context switch watchdog counter is reset to zero and 
automatically disabled until enabled by a context restore or 
explicitly.


So it sounds the commit message could be wrong that special 
handling is needed from this direction. But read till the end on 
the restriction listed.


  * Watchdog counter is reset to zero and is not accumulated 
across multiple submission of the same context (due preemption).


I read this as - after preemption contexts gets a new full timeout 
allocation. Or in other words, if a context is preempted N times, 
it's cumulative watchdog timeout will be N * set value.


This could be theoretically exploitable to bypass the timeout. If 
a client sets up two contexts with prio -1 and -2, and keeps 
submitting periodical no-op batches against prio -1 context, while 
prio -2 is it's own hog, then prio -2 context defeats the watchdog 
timer. I think.. would appreciate is someone challenged this 
conclusion.


I think you are right that is a possibility but, is that a problem? 
The client can just not set the threshold to bypass the timeout. 
Also because you need the hanging batch to be simply preemptible, 
you cannot disrupt any work from another client that is higher 
priority. This is 


But I think higher priority client can have the same effect on the 
lower priority purely by accident, no?


As a real world example, user kicks off an background transcoding 
job, which happens to use prio -2, and uses the watchdog timer.


At the same time user watches a video from a player of normal 
priority. This causes periodic, say 24Hz, preemption events, due 
frame decoding activity on the same engine as the transcoding client.


Does this defeat the watchdog timer for the former is the question? 
Then the questions of can we do something about it and whether it 
really isn't a problem?


I guess it depends if you consider that timeout as the maximum 
lifespan a workload can have or max contiguous active time.


I believe the intended purpose of the watchdog is to prevent broken 
bitstreams hanging the transcoder/player. That is, it is a form of 
error detection used by the media driver to handle bad user input. So 
if there is a way for the watchdog to be extended indefinitely under 
normal situations, that would be a problem. It means the transcoder 
will not detect the broken input data in a timely manner and 
effectively hang rather than skip over to the next packet. And note 
that broken input data can be caused by something as innocent as a 
dropped packet due to high network contention. No need for any 
malicious activity at all.


My understanding of the intended purpose is the same. And it would be a 
very useful feature.


I'm not familiar enough with the application but, in the scenario above, 
what if the batch that is being preempted is not stuck but just nice 
enough to be preempted enough times so that it wouldn't complete in the 
given wall clock time but would be fast enough by itself.




Chris mentioned the other day that until hardware is fixed to context 
save/restore the watchdog counter this could simply be implemented using 
timers. And I have to say I agree. Shouldn't be too hard to prototype it 
using  hrtimers - start on context in, stop on context out and kick 
forward on user interrupts. More or less.


Would this implement the feature on the driver side just like it would 
for the HW? I mean have the same IOCTL and silently discard workload 
that hit the timeout. Also, would it discard batches while they are in 
the queue (not active)?


Antonio



Then if the cost

Re: [Intel-gfx] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Antonio Argenziano]

On 11/01/19 00:22, Tvrtko Ursulin wrote:


On 11/01/2019 00:47, Antonio Argenziano wrote:

On 07/01/19 08:58, Tvrtko Ursulin wrote:

On 07/01/2019 13:57, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2019-01-07 13:43:29)


On 07/01/2019 11:58, Tvrtko Ursulin wrote:

[snip]

Note about future interaction with preemption: Preemption could 
happen

in a command sequence prior to watchdog counter getting disabled,
resulting in watchdog being triggered following preemption (e.g. 
when
watchdog had been enabled in the low priority batch). The driver 
will

need to explicitly disable the watchdog counter as part of the
preemption sequence.


Does the series take care of preemption?


I did not find that it does.


Oh. I hoped that the watchdog was saved as part of the context... Then
despite preemption, the timeout would resume from where we left off as
soon as it was back on the gpu.

If the timeout remaining was context saved it would be much simpler (at
least on first glance), please say it is.


I made my comments going only by the text from the commit message and 
the absence of any preemption special handling.


Having read the spec, the situation seems like this:

  * Watchdog control and threshold register are context saved and 
restored.


  * On a context switch watchdog counter is reset to zero and 
automatically disabled until enabled by a context restore or explicitly.


So it sounds the commit message could be wrong that special handling 
is needed from this direction. But read till the end on the 
restriction listed.


  * Watchdog counter is reset to zero and is not accumulated across 
multiple submission of the same context (due preemption).


I read this as - after preemption contexts gets a new full timeout 
allocation. Or in other words, if a context is preempted N times, 
it's cumulative watchdog timeout will be N * set value.


This could be theoretically exploitable to bypass the timeout. If a 
client sets up two contexts with prio -1 and -2, and keeps submitting 
periodical no-op batches against prio -1 context, while prio -2 is 
it's own hog, then prio -2 context defeats the watchdog timer. I 
think.. would appreciate is someone challenged this conclusion.


I think you are right that is a possibility but, is that a problem? 
The client can just not set the threshold to bypass the timeout. Also 
because you need the hanging batch to be simply preemptible, you 
cannot disrupt any work from another client that is higher priority. 
This is 


But I think higher priority client can have the same effect on the lower 
priority purely by accident, no?


As a real world example, user kicks off an background transcoding job, 
which happens to use prio -2, and uses the watchdog timer.


At the same time user watches a video from a player of normal priority. 
This causes periodic, say 24Hz, preemption events, due frame decoding 
activity on the same engine as the transcoding client.


Does this defeat the watchdog timer for the former is the question? Then 
the questions of can we do something about it and whether it really 
isn't a problem?


I guess it depends if you consider that timeout as the maximum lifespan 
a workload can have or max contiguous active time.




Maybe it is not disrupting higher priority clients but it is causing an 
time unbound power drain.


pretty much the same behavior of hangcheck IIRC so something we 
already accept.


You mean today hangcheck wouldn't notice a hanging batch in the same 
scenario as above? If so it sounds like a huge gap we need to try and fix.


My understanding of it is that we only keep a record of what was running 
the last time hangcheck was run so it is possible to trick it into 
resetting when a preemption occurs but I could be missing something.






And finally there is one programming restriction which says:

  * SW must not preempt the workload which has watchdog enabled. 
Either it must:


a) disable preemption for that workload completely, or
b) disable the watchdog via mmio write before any write to ELSP

This seems it contradiction with the statement that the counter gets 
disabled on context switch and stays disabled.


I did not spot anything like this in the series. So it would seem the 
commit message is correct after all.


It would be good if someone could re-read the bspec text on register 
0x2178 to double check what I wrote.


The way I read it is that the restriction applies only to some 
platforms where the 'normal' description doesn't apply.


You are right. Are the listed parts in the field so the series would 
have to handle this or we can ignore it?


I think there is something we need to handle e.g. BXT.

Antonio



Regards,

Tvrtko

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

Re: [Intel-gfx] drm/i915: Watchdog timeout: IRQ handler for gen8+

[Antonio Argenziano]

On 07/01/19 08:58, Tvrtko Ursulin wrote:


On 07/01/2019 13:57, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2019-01-07 13:43:29)


On 07/01/2019 11:58, Tvrtko Ursulin wrote:

[snip]


Note about future interaction with preemption: Preemption could happen
in a command sequence prior to watchdog counter getting disabled,
resulting in watchdog being triggered following preemption (e.g. when
watchdog had been enabled in the low priority batch). The driver will
need to explicitly disable the watchdog counter as part of the
preemption sequence.


Does the series take care of preemption?


I did not find that it does.


Oh. I hoped that the watchdog was saved as part of the context... Then
despite preemption, the timeout would resume from where we left off as
soon as it was back on the gpu.

If the timeout remaining was context saved it would be much simpler (at
least on first glance), please say it is.


I made my comments going only by the text from the commit message and 
the absence of any preemption special handling.


Having read the spec, the situation seems like this:

  * Watchdog control and threshold register are context saved and restored.

  * On a context switch watchdog counter is reset to zero and 
automatically disabled until enabled by a context restore or explicitly.


So it sounds the commit message could be wrong that special handling is 
needed from this direction. But read till the end on the restriction 
listed.


  * Watchdog counter is reset to zero and is not accumulated across 
multiple submission of the same context (due preemption).


I read this as - after preemption contexts gets a new full timeout 
allocation. Or in other words, if a context is preempted N times, it's 
cumulative watchdog timeout will be N * set value.


This could be theoretically exploitable to bypass the timeout. If a 
client sets up two contexts with prio -1 and -2, and keeps submitting 
periodical no-op batches against prio -1 context, while prio -2 is it's 
own hog, then prio -2 context defeats the watchdog timer. I think.. 
would appreciate is someone challenged this conclusion.


I think you are right that is a possibility but, is that a problem? The 
client can just not set the threshold to bypass the timeout. Also 
because you need the hanging batch to be simply preemptible, you cannot 
disrupt any work from another client that is higher priority. This is 
pretty much the same behavior of hangcheck IIRC so something we already 
accept.




And finally there is one programming restriction which says:

  * SW must not preempt the workload which has watchdog enabled. Either 
it must:


a) disable preemption for that workload completely, or
b) disable the watchdog via mmio write before any write to ELSP

This seems it contradiction with the statement that the counter gets 
disabled on context switch and stays disabled.


I did not spot anything like this in the series. So it would seem the 
commit message is correct after all.


It would be good if someone could re-read the bspec text on register 
0x2178 to double check what I wrote.


The way I read it is that the restriction applies only to some platforms 
where the 'normal' description doesn't apply.


Antonio



Regards,

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

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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/hangman: Skip if disabled by the kernel

[Antonio Argenziano]

On 10/01/19 01:19, Chris Wilson wrote:

Some kernels may have to disable error capture for some hardware or by
it being configured out. Since it is conditionally available, asserting
it exists is not an actual requirement. For hardware where we are unable
to provide error state capture, skip.

Signed-off-by: Chris Wilson 


LGTM.
Acked-by: Antonio Argenziano 

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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_ctx_isolation: Ignore the low bits of BB_OFFSET

[Antonio Argenziano]

On 10/01/19 13:29, Chris Wilson wrote:

Quoting Chris Wilson (2019-01-10 21:27:54)

Quoting Antonio Argenziano (2019-01-10 21:24:56)



On 07/01/19 04:41, Chris Wilson wrote:

On Skylake, BB_OFFSET seems to be unstable. Since this is an
offset into the batch at the time of CS execution, it should be actively
written to as we read from the register so allow it a qword of
discrepancy (since the CS should be reading in qwords). This still
allows us to detect dirt across the rest of the register field, should
that be required.

Signed-off-by: Chris Wilson 
---
   tests/i915/gem_ctx_isolation.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 058cf3ec1..78a244382 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -96,7 +96,7 @@ static const struct named_register {
   { "GPGPU_THREADS_DISPATCHED", GEN8, RCS0, 0x2290, 2 },
   { "PS_INVOCATION_COUNT_1", GEN8, RCS0, 0x22f0, 2 },
   { "PS_DEPTH_COUNT_1", GEN8, RCS0, 0x22f8, 2 },
- { "BB_OFFSET", GEN8, RCS0, 0x2158 },
+ { "BB_OFFSET", GEN8, RCS0, 0x2158, .ignore_bits = 0x7 },


The batch offset starts at bit 2. Do we observe changes in bit 0-1 as well?


Not, it is just off by bit 2 (0x4). Bit 0 is also set when I don't
really expect it to be, I guess I really should just read what the
register is meant to be rather than guessing solely on the basis of its
name.


Bit 2 flip flops between reference value and observed (test failure).

Bit 0 simply differs from my own expectations.


I guess if it gets overwritten we catch it even if we ignore the lowest 
3 bits but something weird would have happened if 0-1 change.


With or without modifying the mask,
Reviewed-by: Antonio Argenziano 


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_ctx_isolation: Ignore the low bits of BB_OFFSET

[Antonio Argenziano]

On 07/01/19 04:41, Chris Wilson wrote:

On Skylake, BB_OFFSET seems to be unstable. Since this is an
offset into the batch at the time of CS execution, it should be actively
written to as we read from the register so allow it a qword of
discrepancy (since the CS should be reading in qwords). This still
allows us to detect dirt across the rest of the register field, should
that be required.

Signed-off-by: Chris Wilson 
---
  tests/i915/gem_ctx_isolation.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 058cf3ec1..78a244382 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -96,7 +96,7 @@ static const struct named_register {
{ "GPGPU_THREADS_DISPATCHED", GEN8, RCS0, 0x2290, 2 },
{ "PS_INVOCATION_COUNT_1", GEN8, RCS0, 0x22f0, 2 },
{ "PS_DEPTH_COUNT_1", GEN8, RCS0, 0x22f8, 2 },
-   { "BB_OFFSET", GEN8, RCS0, 0x2158 },
+   { "BB_OFFSET", GEN8, RCS0, 0x2158, .ignore_bits = 0x7 },


The batch offset starts at bit 2. Do we observe changes in bit 0-1 as well?

Antonio


{ "MI_PREDICATE_RESULT_1", GEN8, RCS0, 0x241c },
{ "CS_GPR", GEN8, RCS0, 0x2600, 32 },
{ "OA_CTX_CONTROL", GEN8, RCS0, 0x2360 },


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/amdgpu_amd_prime: Bail if we fail to create more contexts

[Antonio Argenziano]

On 13/12/18 03:57, Chris Wilson wrote:

amdgpu has started to report out of space after creating a few contexts.
This is not the scope of this test as here we just verifying that fences
created in amd can be imported and used for synchronisation by i915 and
for that we just need at least one context created!

References: https://bugs.freedesktop.org/show_bug.cgi?id=109049
Signed-off-by: Chris Wilson 


LGTM.

Reviwed-by: Antonio Argenziano 


---
  tests/amdgpu/amd_prime.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index bda0ce83d..518c88963 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -354,8 +354,8 @@ static void amd_to_i915(int i915, int amd, 
amdgpu_device_handle device)


doesn't i915_to_amd have the same issue?

Antonio


contexts = realloc(contexts, size * sizeof(*contexts));
}
  
-		r = amdgpu_cs_ctx_create(device, [count]);

-   igt_assert_eq(r, 0);
+   if (amdgpu_cs_ctx_create(device, [count]))
+   break;
  
  		r = amdgpu_cs_submit(contexts[count], 0, _request, 1);

igt_assert_eq(r, 0);
@@ -364,6 +364,7 @@ static void amd_to_i915(int i915, int amd, 
amdgpu_device_handle device)
}
  
  	igt_info("Reservation width = %ld\n", count);

+   igt_require(count);
  
  	amdgpu_bo_export(ib_result_handle,

 amdgpu_bo_handle_type_dma_buf_fd,


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Show the contents of i915_runtime_pm_status on setup

[Antonio Argenziano]

On 05/12/18 02:48, Chris Wilson wrote:

To try and help debug situations where the driver has leaked a runtime
reference before executing the pm_rpm tests, show the contents of
i915_runtime_pm_status at startup, which will include all currently held
wakerefs.


LGTM.

Reviewed-by: Antonio Argenziano 



Signed-off-by: Chris Wilson 
---
  tests/pm_rpm.c | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index f0d6db525..7427958dc 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -725,6 +725,18 @@ static bool dmc_loaded(void)
return strstr(buf, "fw loaded: yes");
  }
  
+static void dump_file(int dir, const char *filename)

+{
+   char *contents;
+
+   contents = igt_sysfs_get(dir, filename);
+   if (!contents)
+   return;
+
+   igt_debug("%s:\n%s\n", filename, contents);
+   free(contents);
+}
+
  static bool setup_environment(void)
  {
if (has_runtime_pm)
@@ -744,6 +756,8 @@ static bool setup_environment(void)
has_runtime_pm = igt_setup_runtime_pm();
setup_pc8();
  
+	dump_file(debugfs, "i915_runtime_pm_status");

+
igt_info("Runtime PM support: %d\n", has_runtime_pm);
igt_info("PC8 residency support: %d\n", has_pc8);
igt_require(has_runtime_pm);


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use

[Antonio Argenziano]

On 02/10/18 01:30, Joonas Lahtinen wrote:

Quoting Antonio Argenziano (2018-10-01 22:53:46)

Fair enough.

Acked-by: Antonio Argenziano 

for the series.


Please, read the following chapters (they're applicable for the patch
tag meanings in IGT, too):

https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
https://www.kernel.org/doc/html/v4.18/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes

If we spend the time to actually review the patches, that should be
documented with a proper Reviewed-by and not a vague Acked-by.


KMS is really an area I do not know much about. While I can say the 
patches are looking good on the IGT side, I cannot guarantee they use 
the KMS interface appropriately therefore the 'Acked-by'. After reading 
the documentation you linked I think it fits rather well since the only 
feedback I gave was on a small oversight.


Thanks,
Antonio



Regards, Joonas


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use

[Antonio Argenziano]

On 01/10/18 12:43, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-10-01 19:36:24)



On 28/09/18 03:19, Chris Wilson wrote:

If the driver doesn't support the getfb iface (e.g. because KMS has been
disabled), the ioctls will fail with ENOTSUP. This is expected, so skip
the test as nothing useful can be learnt.

Signed-off-by: Chris Wilson 
---
   tests/kms_getfb.c | 40 ++--
   1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index 81d796a42..71d65488f 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -40,6 +40,40 @@
   #include "drm.h"
   #include "drm_fourcc.h"
   
+static bool has_getfb_iface(int fd)

+{
+ struct drm_mode_fb_cmd arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, ))
+ err = -errno;
+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;
+ }
+}
+
+static bool has_addfb2_iface(int fd)
+{
+ struct drm_mode_fb_cmd2 arg = { };
+ int err;
+
+ err = 0;
+ if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, ) == 0)
+ err = -errno;


Shouldn't this^ be != 0?


Yup.
  

+ switch (err) {
+ case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+ case -ENOTSUP: /* driver doesn't support KMS */
+ return false;
+ default:
+ return true;


Shouldn't we fail on every errno?


No. We want to be very careful in only fail at this point for errno we
know imply the interface is not supported so that we do not confuse an
illegal addfb2 request and fail later in the actual test.


Fair enough.

Acked-by: Antonio Argenziano 

for the series.


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/5] igt/kms_getfb: Check the iface exists before use

[Antonio Argenziano]

On 28/09/18 03:19, Chris Wilson wrote:

If the driver doesn't support the getfb iface (e.g. because KMS has been
disabled), the ioctls will fail with ENOTSUP. This is expected, so skip
the test as nothing useful can be learnt.

Signed-off-by: Chris Wilson 
---
  tests/kms_getfb.c | 40 ++--
  1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/tests/kms_getfb.c b/tests/kms_getfb.c
index 81d796a42..71d65488f 100644
--- a/tests/kms_getfb.c
+++ b/tests/kms_getfb.c
@@ -40,6 +40,40 @@
  #include "drm.h"
  #include "drm_fourcc.h"
  
+static bool has_getfb_iface(int fd)

+{
+   struct drm_mode_fb_cmd arg = { };
+   int err;
+
+   err = 0;
+   if (drmIoctl(fd, DRM_IOCTL_MODE_GETFB, ))
+   err = -errno;
+   switch (err) {
+   case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+   case -ENOTSUP: /* driver doesn't support KMS */
+   return false;
+   default:
+   return true;
+   }
+}
+
+static bool has_addfb2_iface(int fd)
+{
+   struct drm_mode_fb_cmd2 arg = { };
+   int err;
+
+   err = 0;
+   if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, ) == 0)
+   err = -errno;


Shouldn't this^ be != 0?


+   switch (err) {
+   case -ENOTTY: /* ioctl unrecognised (kernel too old) */
+   case -ENOTSUP: /* driver doesn't support KMS */
+   return false;
+   default:
+   return true;


Shouldn't we fail on every errno?

Thanks,
Antonio


+   }
+}
+
  static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
  {
struct drm_mode_fb_cmd2 add = {
@@ -54,6 +88,7 @@ static void get_ccs_fb(int fd, struct drm_mode_fb_cmd2 *ret)
};
int size;
  
+	igt_require(has_addfb2_iface(fd));

igt_require_intel(fd);
  
  	/* An explanation of the magic numbers can be found in kms_ccs.c. */

@@ -191,15 +226,16 @@ static void test_duplicate_handles(int fd)
do_ioctl(fd, DRM_IOCTL_MODE_RMFB, _id);
gem_close(fd, add.handles[0]);
}
-
  }
  
  igt_main

  {
int fd;
  
-	igt_fixture

+   igt_fixture {
fd = drm_open_driver_master(DRIVER_ANY);
+   igt_require(has_getfb_iface(fd));
+   }
  
  	igt_subtest_group

test_handle_input(fd);


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] lib: Cancel all outstanding requests at the end of a test

[Antonio Argenziano]

On 07/09/18 01:41, Chris Wilson wrote:

Quite often on catastrophic failure the test leaves a long queue of
unterminated batches pending execution. Each runs until hangcheck fires
and skips onto the next, leaving us waiting for a very long time at test
exit.

On older kernels, this gracefully degrades into the existing mechanism.

Signed-off-by: Chris Wilson 


LGTM. Just, if I have to be really picky, the comment for 
quiescent_gpu() says it is installed as an exit handler.


Reviewed-by: Antonio Argenziano 


---
  lib/drmtest.c | 25 +++--
  lib/igt_debugfs.h | 12 
  2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index bfa2e0f0a..fee9d33ad 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -302,22 +302,35 @@ static int __drm_open_driver_render(int chipset)
  static int at_exit_drm_fd = -1;
  static int at_exit_drm_render_fd = -1;
  
-static void quiescent_gpu_at_exit(int sig)

+static void __cancel_work_at_exit(int fd)
+{
+   igt_terminate_spin_batches(); /* for older kernels */
+
+   igt_drop_caches_set(fd,
+   /* cancel everything */
+   DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
+   /* cleanup */
+   DROP_ACTIVE | DROP_RETIRE | DROP_IDLE | DROP_FREED);
+}
+
+static void cancel_work_at_exit(int sig)
  {
if (at_exit_drm_fd < 0)
return;
  
-	gem_quiescent_gpu(at_exit_drm_fd);

+   __cancel_work_at_exit(at_exit_drm_fd);
+
close(at_exit_drm_fd);
at_exit_drm_fd = -1;
  }
  
-static void quiescent_gpu_at_exit_render(int sig)

+static void cancel_work_at_exit_render(int sig)
  {
if (at_exit_drm_render_fd < 0)
return;
  
-	gem_quiescent_gpu(at_exit_drm_render_fd);

+   __cancel_work_at_exit(at_exit_drm_render_fd);
+
close(at_exit_drm_render_fd);
at_exit_drm_render_fd = -1;
  }
@@ -369,7 +382,7 @@ int drm_open_driver(int chipset)
gem_quiescent_gpu(fd);
  
  			at_exit_drm_fd = __drm_open_driver(chipset);

-   igt_install_exit_handler(quiescent_gpu_at_exit);
+   igt_install_exit_handler(cancel_work_at_exit);
}
}
  
@@ -418,7 +431,7 @@ int drm_open_driver_render(int chipset)

at_exit_drm_render_fd = __drm_open_driver(chipset);
if(chipset & DRIVER_INTEL){
gem_quiescent_gpu(fd);
-   igt_install_exit_handler(quiescent_gpu_at_exit_render);
+   igt_install_exit_handler(cancel_work_at_exit_render);
}
  
  	return fd;

diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index ff8612dc6..9f81be0a2 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -190,6 +190,18 @@ void igt_require_hpd_storm_ctl(int fd);
   * Flush the driver's idle_worker, releasing internal caches and wakerefs.
   */
  #define DROP_IDLE 0x40
+/**
+ * DROP_RESET_ACTIVE:
+ *
+ * Cancel all outstanding requests by forcing a gpu reset
+ */
+#define DROP_RESET_ACTIVE 0x80
+/**
+ * DROP_RESET_SEQNO:
+ *
+ * Reset the global request seqno counter back to 0
+ */
+#define DROP_RESET_SEQNO 0x100
  /**
   * DROP_ALL:
   *


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Antonio Argenziano]

On 17/08/18 10:49, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-17 18:29:09)



On 15/08/18 02:25, Chris Wilson wrote:

Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module 
reload")
Signed-off-by: Chris Wilson 
---
   tests/pm_rpm.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 65489bcdb..a4f9f783e 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
   teardown_environment();
   
   igt_subtest("module-reload") {

+ teardown_environment();


There is already a fixture with a call to 'teardown_environment()'
surrounding this test, is it missing a couple of subtests groups?


It was intended for sequential execution (and I had forgotten that I had
purposely placed it after the teardown_environment -- the confusion was
caused by teardown_environment being incomplete). Adding a subtest group
for the preceding bunch would have the debatable consequence of module-
reload reporting a FAIL if something else caused a wakeref leak. My
opinion is to prefer SKIP for external artefacts so that it is clear when
a test failed of its own accord (and so be useful for debugging). It just
so happens that this test was to reproduce the trigger for the external
failures elsewhere and so detect the wakeref leak.


Agreed. Maybe a comment would help, your choice.

Reviewed-by: Antonio Argenziano 


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/2] igt/pm_rpm: Avoid at_exit_drm_fd

[Antonio Argenziano]

On 15/08/18 13:59, Chris Wilson wrote:

Keep the drm_fd owned by pm_rpm as we need to relinquish all ownership
of the device in order to unload the module.

Signed-off-by: Chris Wilson 


LGTM.
Reviewed-by: Antonio Argenziano 


---
  tests/pm_rpm.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index c0212ed70..f0781617c 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -697,7 +697,10 @@ static bool setup_environment(void)
if (has_runtime_pm)
goto out;
  
-	drm_fd = drm_open_driver_master(DRIVER_INTEL);

+   drm_fd = __drm_open_driver(DRIVER_INTEL);
+   igt_require(drm_fd != -1);
+   igt_device_set_master(drm_fd);
+
debugfs = igt_debugfs_dir(drm_fd);
igt_require(debugfs != -1);
  


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Antonio Argenziano]

On 15/08/18 02:25, Chris Wilson wrote:

Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module 
reload")
Signed-off-by: Chris Wilson 
---
  tests/pm_rpm.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 65489bcdb..a4f9f783e 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
teardown_environment();
  
  	igt_subtest("module-reload") {

+   teardown_environment();


There is already a fixture with a call to 'teardown_environment()' 
surrounding this test, is it missing a couple of subtests groups?


Antonio


+
igt_debug("Reload w/o display\n");
igt_i915_driver_unload();
igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/3] gem_sync: Measure wakeup latency while also scheduling the next batch

[Antonio Argenziano]

On 10/08/18 04:01, Chris Wilson wrote:

More variants on stress waits to serve the dual purpose of investigating
different aspects of the latency (this time while also serving
execlists interrupts) while also checking that we never miss the wakeup.

Signed-off-by: Chris Wilson 
---
  tests/gem_sync.c | 142 +++
  1 file changed, 142 insertions(+)

diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index 495ca3b53..c697220ad 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -294,6 +294,144 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
  }
  
+static void

+active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
+{
+   unsigned engines[16];
+   const char *names[16];
+   int num_engines = 0;
+
+   if (ring == ALL_ENGINES) {
+   for_each_physical_engine(fd, ring) {
+   if (!gem_can_store_dword(fd, ring))
+   continue;
+
+   names[num_engines] = e__->name;
+   engines[num_engines++] = ring;
+   if (num_engines == ARRAY_SIZE(engines))


If num_engines is larger than engines it is time to get  a bigger array 
:). I think a require/assert would actually force someone to make the 
change (otherwise none will ever do it)



+   break;
+   }
+   igt_require(num_engines);
+   } else {
+   gem_require_ring(fd, ring);
+   igt_require(gem_can_store_dword(fd, ring));
+   names[num_engines] = NULL;
+   engines[num_engines++] = ring;
+   }
+
+   intel_detect_and_clear_missed_interrupts(fd);
+   igt_fork(child, num_engines) {
+   const uint32_t bbe = MI_BATCH_BUFFER_END;
+   struct drm_i915_gem_exec_object2 object;
+   struct drm_i915_gem_execbuffer2 execbuf;
+   double end, this, elapsed, now, baseline;
+   unsigned long cycles;
+   igt_spin_t *spin[2];
+   uint32_t cmd;
+
+   memset(, 0, sizeof(object));
+   object.handle = gem_create(fd, 4096);
+   gem_write(fd, object.handle, 0, , sizeof(bbe));
+
+   memset(, 0, sizeof(execbuf));
+   execbuf.buffers_ptr = to_user_pointer();
+   execbuf.buffer_count = 1;
+   execbuf.flags = engines[child % num_engines];
+
+   spin[0] = __igt_spin_batch_new(fd,
+  .engine = execbuf.flags,
+  .flags = (IGT_SPIN_POLL_RUN |
+IGT_SPIN_FAST));
+   igt_assert(spin[0]->running);
+   cmd = *spin[0]->batch;
+
+   spin[1] = __igt_spin_batch_new(fd,
+  .engine = execbuf.flags,
+  .flags = (IGT_SPIN_POLL_RUN |
+IGT_SPIN_FAST));
+
+   gem_execbuf(fd, );
+
+   igt_spin_batch_end(spin[1]);
+   igt_spin_batch_end(spin[0]);
+   gem_sync(fd, object.handle);
+
+   for (int warmup = 0; warmup <= 1; warmup++) {
+   *spin[0]->batch = cmd;
+   *spin[0]->running = 0;
+   gem_execbuf(fd, [0]->execbuf);
+
+   end = gettime() + timeout/10.;
+   elapsed = 0;
+   cycles = 0;
+   do {
+   while (!READ_ONCE(*spin[0]->running))
+   ;
+
+   *spin[1]->batch = cmd;
+   *spin[1]->running = 0;
+   gem_execbuf(fd, [1]->execbuf);
+
+   this = gettime();
+   igt_spin_batch_end(spin[0]);
+   gem_sync(fd, spin[0]->handle);
+   now = gettime();
+
+   elapsed += now - this;
+   cycles++;
+   igt_swap(spin[0], spin[1]);
+   } while (now < end);
+   igt_spin_batch_end(spin[0]);
+   baseline = elapsed / cycles;
+   }
+   igt_info("%s%saseline %ld cycles: %.3f us\n",
+names[child % num_engines] ?: "",
+names[child % num_engines] ? " b" : "B",
+cycles, elapsed*1e6/cycles);
+
+   *spin[0]->batch = cmd;
+   *spin[0]->running = 0;
+   gem_execbuf(fd, [0]->execbuf);
+
+   end = gettime() + timeout;
+   elapsed 

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_sync: Exercise sync after context switch

[Antonio Argenziano]

On 16/08/18 00:08, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-16 00:59:30)



On 15/08/18 10:24, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-15 18:20:10)



On 15/08/18 03:26, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-15 00:50:43)



On 10/08/18 04:01, Chris Wilson wrote:

This exercises a special case that may be of interest, waiting for a
context that may be preempted in order to reduce the wait.

Signed-off-by: Chris Wilson 
---
+ cycles = 0;
+ elapsed = 0;
+ start = gettime();
+ do {
+ do {
+ double this;
+
+ gem_execbuf(fd, [0].execbuf);
+ gem_execbuf(fd, [1].execbuf);


I'm not sure where the preemption, mentioned in the commit message, is
coming in.


Internally. I've suggested that we reorder equivalent contexts in order
to satisfy client waits earlier. So having created two independent
request queues, userspace should be oblivious to the execution order.


But there isn't an assert because you don't want that to be part of the
contract between the driver and userspace, is that correct?


Correct. Userspace hasn't specified an order between the two contexts so
can't actually assert it happens in a particular order. We are free then
to do whatever we like, but that also means no assertion. Just the
figures look pretty and ofc we have to check that nothing actually
breaks.


The last question I have is about the batches, why not choosing a spin
batch so to make sure that context[0] (and [1]) hasn't completed by the
time it starts waiting.


It would be exercising fewer possibilities. Not that it would be any
less valid. (If I can't do a pair of trivial execbuf faster than the gpu
can execute a no-op from idle, shoot me. Each execbuf will take ~500ns,
the gpu will take 20-50us [bdw-kbl] to execute the first batch from idle.)


It would generate some odd looking numbers anyways.

Reviewed-By: Antonio Argenziano 


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_sync: Exercise sync after context switch

[Antonio Argenziano]

On 15/08/18 10:24, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-15 18:20:10)



On 15/08/18 03:26, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-15 00:50:43)



On 10/08/18 04:01, Chris Wilson wrote:

This exercises a special case that may be of interest, waiting for a
context that may be preempted in order to reduce the wait.

Signed-off-by: Chris Wilson 
---
+ cycles = 0;
+ elapsed = 0;
+ start = gettime();
+ do {
+ do {
+ double this;
+
+ gem_execbuf(fd, [0].execbuf);
+ gem_execbuf(fd, [1].execbuf);


I'm not sure where the preemption, mentioned in the commit message, is
coming in.


Internally. I've suggested that we reorder equivalent contexts in order
to satisfy client waits earlier. So having created two independent
request queues, userspace should be oblivious to the execution order.


But there isn't an assert because you don't want that to be part of the
contract between the driver and userspace, is that correct?


Correct. Userspace hasn't specified an order between the two contexts so
can't actually assert it happens in a particular order. We are free then
to do whatever we like, but that also means no assertion. Just the
figures look pretty and ofc we have to check that nothing actually
breaks.


The last question I have is about the batches, why not choosing a spin 
batch so to make sure that context[0] (and [1]) hasn't completed by the 
time it starts waiting.


Antonio


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_sync: Exercise sync after context switch

[Antonio Argenziano]

On 15/08/18 03:26, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-15 00:50:43)



On 10/08/18 04:01, Chris Wilson wrote:

This exercises a special case that may be of interest, waiting for a
context that may be preempted in order to reduce the wait.

Signed-off-by: Chris Wilson 
---
+ cycles = 0;
+ elapsed = 0;
+ start = gettime();
+ do {
+ do {
+ double this;
+
+ gem_execbuf(fd, [0].execbuf);
+ gem_execbuf(fd, [1].execbuf);


I'm not sure where the preemption, mentioned in the commit message, is
coming in.


Internally. I've suggested that we reorder equivalent contexts in order
to satisfy client waits earlier. So having created two independent
request queues, userspace should be oblivious to the execution order.


But there isn't an assert because you don't want that to be part of the 
contract between the driver and userspace, is that correct?


Antonio


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_sync: Exercise sync after context switch

[Antonio Argenziano]

On 10/08/18 04:01, Chris Wilson wrote:

This exercises a special case that may be of interest, waiting for a
context that may be preempted in order to reduce the wait.

Signed-off-by: Chris Wilson 
---
  tests/gem_sync.c | 146 +++
  1 file changed, 146 insertions(+)

diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index 493ae61df..495ca3b53 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -409,6 +409,144 @@ store_ring(int fd, unsigned ring, int num_children, int 
timeout)
igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
  }
  
+static void

+switch_ring(int fd, unsigned ring, int num_children, int timeout)
+{
+   const int gen = intel_gen(intel_get_drm_devid(fd));
+   unsigned engines[16];
+   const char *names[16];
+   int num_engines = 0;
+
+   gem_require_contexts(fd);
+
+   if (ring == ALL_ENGINES) {
+   for_each_physical_engine(fd, ring) {
+   if (!gem_can_store_dword(fd, ring))
+   continue;
+
+   names[num_engines] = e__->name;
+   engines[num_engines++] = ring;
+   if (num_engines == ARRAY_SIZE(engines))
+   break;
+   }
+
+   num_children *= num_engines;
+   } else {
+   gem_require_ring(fd, ring);
+   igt_require(gem_can_store_dword(fd, ring));
+   names[num_engines] = NULL;
+   engines[num_engines++] = ring;
+   }
+
+   intel_detect_and_clear_missed_interrupts(fd);
+   igt_fork(child, num_children) {
+   struct context {
+   struct drm_i915_gem_exec_object2 object[2];
+   struct drm_i915_gem_relocation_entry reloc[1024];
+   struct drm_i915_gem_execbuffer2 execbuf;
+   } contexts[2];
+   double start, elapsed;
+   unsigned long cycles;
+
+   for (int i = 0; i < ARRAY_SIZE(contexts); i++) {
+   const uint32_t bbe = MI_BATCH_BUFFER_END;
+   const uint32_t sz = 32 << 10;
+   struct context *c = [i];
+   uint32_t *batch, *b;
+
+   memset(>execbuf, 0, sizeof(c->execbuf));
+   c->execbuf.buffers_ptr = to_user_pointer(c->object);
+   c->execbuf.flags = engines[child % num_engines];
+   c->execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
+   c->execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
+   if (gen < 6)
+   c->execbuf.flags |= I915_EXEC_SECURE;
+   c->execbuf.rsvd1 = gem_context_create(fd);
+
+   memset(c->object, 0, sizeof(c->object));
+   c->object[0].handle = gem_create(fd, 4096);
+   gem_write(fd, c->object[0].handle, 0, , 
sizeof(bbe));
+   c->execbuf.buffer_count = 1;
+   gem_execbuf(fd, >execbuf);
+
+   c->object[0].flags |= EXEC_OBJECT_WRITE;
+   c->object[1].handle = gem_create(fd, sz);
+
+   c->object[1].relocs_ptr = to_user_pointer(c->reloc);
+   c->object[1].relocation_count = 1024;
+
+   batch = gem_mmap__cpu(fd, c->object[1].handle, 0, sz,
+   PROT_WRITE | PROT_READ);
+   gem_set_domain(fd, c->object[1].handle,
+   I915_GEM_DOMAIN_CPU, 
I915_GEM_DOMAIN_CPU);
+
+   memset(c->reloc, 0, sizeof(c->reloc));
+   b = batch;
+   for (int r = 0; r < 1024; r++) {
+   uint64_t offset;
+
+   c->reloc[r].presumed_offset = 
c->object[0].offset;
+   c->reloc[r].offset = (b - batch + 1) * 
sizeof(*batch);
+   c->reloc[r].delta = r * sizeof(uint32_t);
+   c->reloc[r].read_domains = 
I915_GEM_DOMAIN_INSTRUCTION;
+   c->reloc[r].write_domain = 
I915_GEM_DOMAIN_INSTRUCTION;
+
+   offset = c->object[0].offset + 
c->reloc[r].delta;
+   *b++ = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 
: 0);
+   if (gen >= 8) {
+   *b++ = offset;
+   *b++ = offset >> 32;
+   } else if (gen >= 4) {
+   *b++ = 0;
+   *b++ = offset;
+   c->reloc[r].offset += sizeof(*batch);
+   } else {
+  

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 3/3] igt/gem_sync: Exercising waiting while keeping the GPU busy

[Antonio Argenziano]

On 14/08/18 11:27, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-10 19:11:02)



On 10/08/18 10:51, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-10 18:41:22)

How does the test fail if the sync goes wrong? Hang detector on the
queued batch?


We have a hang detector for both missed wakeups and GPU hangs. As tests
goes it's fairly tame, but in essence this entire file is about trying
to trick the HW+driver into not sending an interrupt back to userspace.
Just a very narrow stress test, over and over again from slightly
different angles.


I see.

Reviewed-by: Antonio Argenziano 


Was that a general r-b for the very similar series or just this last
patch?


I've only read this last patch, I'll have a look at the rest.

Antonio


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 3/3] igt/gem_sync: Exercising waiting while keeping the GPU busy

[Antonio Argenziano]

On 10/08/18 10:51, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-08-10 18:41:22)



On 10/08/18 04:01, Chris Wilson wrote:

Normally we wait on the last request, but that overlooks any
difficulties in waiting on a request while the next is being qeued.


/s/qeued/queued


Check those.

Signed-off-by: Chris Wilson 
---
   tests/gem_sync.c | 72 
   1 file changed, 72 insertions(+)

diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index c697220ad..fb209977d 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -294,6 +294,74 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
   igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
   }
   



+ intel_detect_and_clear_missed_interrupts(fd);
+ igt_fork(child, num_engines) {
+ double start, end, elapsed;
+ unsigned long cycles;
+ igt_spin_t *spin[2];
+ uint32_t cmd;
+
+ spin[0] = __igt_spin_batch_new(fd,
+.engine = ring,
+.flags = IGT_SPIN_FAST);
+ cmd = *spin[0]->batch;
+
+ spin[1] = __igt_spin_batch_new(fd,
+.engine = ring,
+.flags = IGT_SPIN_FAST);
+ igt_assert(*spin[1]->batch == cmd);
+
+ start = gettime();
+ end = start + timeout;
+ cycles = 0;
+ do {
+ for (int loop = 0; loop < 1024; loop++) {
+ igt_spin_t *s = spin[loop & 1];
+
+ igt_spin_batch_end(s);
+ gem_sync(fd, s->handle);


How does the test fail if the sync goes wrong? Hang detector on the
queued batch?


We have a hang detector for both missed wakeups and GPU hangs. As tests
goes it's fairly tame, but in essence this entire file is about trying
to trick the HW+driver into not sending an interrupt back to userspace.
Just a very narrow stress test, over and over again from slightly
different angles.


I see.

Reviewed-by: Antonio Argenziano 


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 3/3] igt/gem_sync: Exercising waiting while keeping the GPU busy

[Antonio Argenziano]

On 10/08/18 04:01, Chris Wilson wrote:

Normally we wait on the last request, but that overlooks any
difficulties in waiting on a request while the next is being qeued.


/s/qeued/queued


Check those.

Signed-off-by: Chris Wilson 
---
  tests/gem_sync.c | 72 
  1 file changed, 72 insertions(+)

diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index c697220ad..fb209977d 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -294,6 +294,74 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
  }
  



+   intel_detect_and_clear_missed_interrupts(fd);
+   igt_fork(child, num_engines) {
+   double start, end, elapsed;
+   unsigned long cycles;
+   igt_spin_t *spin[2];
+   uint32_t cmd;
+
+   spin[0] = __igt_spin_batch_new(fd,
+  .engine = ring,
+  .flags = IGT_SPIN_FAST);
+   cmd = *spin[0]->batch;
+
+   spin[1] = __igt_spin_batch_new(fd,
+  .engine = ring,
+  .flags = IGT_SPIN_FAST);
+   igt_assert(*spin[1]->batch == cmd);
+
+   start = gettime();
+   end = start + timeout;
+   cycles = 0;
+   do {
+   for (int loop = 0; loop < 1024; loop++) {
+   igt_spin_t *s = spin[loop & 1];
+
+   igt_spin_batch_end(s);
+   gem_sync(fd, s->handle);


How does the test fail if the sync goes wrong? Hang detector on the 
queued batch?


Antonio


+
+   *s->batch = cmd;
+   gem_execbuf(fd, >execbuf);
+   }
+   cycles += 1024;
+   } while ((elapsed = gettime()) < end);
+   igt_spin_batch_free(fd, spin[1]);
+   igt_spin_batch_free(fd, spin[0]);
+
+   igt_info("%s%sompleted %ld cycles: %.3f us\n",
+names[child % num_engines] ?: "",
+names[child % num_engines] ? " c" : "C",
+cycles, (elapsed - start)*1e6/cycles);
+   }
+   igt_waitchildren_timeout(2*timeout, NULL);
+   igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
+}
+


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt: Remove gvt_basic

[Antonio Argenziano]

On 17/07/18 05:43, Chris Wilson wrote:

Quoting Chris Wilson (2018-07-13 18:57:41)

This was always a placeholder for GVT stakeholders to provide some
better tests. 2 years later and none have been put forward so stop
wasting CI's time running a placeholder.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106989
Signed-off-by: Chris Wilson 
Cc: Zhi Wang 


Martin, would you care to ack?


Do we cover loading the module with enable_gvt=1 somewhere?

Thanks,
Antonio


-Chris


---
  tests/Makefile.sources|  1 -
  tests/gvt_basic.c | 44 ---
  tests/intel-ci/fast-feedback.testlist |  1 -
  tests/meson.build |  1 -
  4 files changed, 47 deletions(-)
  delete mode 100644 tests/gvt_basic.c

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


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

Re: [Intel-gfx] [PATCH i-g-t] lib/gt: Make use of dummyload library to create recursive batch

[Antonio Argenziano]

On 13/07/18 03:06, Chris Wilson wrote:

From: Antonio Argenziano 

An hanging batch is nothing more than a spinning batch that never gets
stopped, so re-use the routines implemented in dummyload.c.

v2: Let caller decide spin loop size
v3: Only use loose loops for hangs (Chris)
v4: No requires
v5: Free the spinner
v6: Chamelium exists.

Signed-off-by: Antonio Argenziano 
Signed-off-by: Chris Wilson 
---



  /**
@@ -377,11 +329,11 @@ igt_hang_t igt_hang_ring(int fd, int ring)
   */
  void igt_post_hang_ring(int fd, igt_hang_t arg)
  {
-   if (arg.handle == 0)
+   if (!arg.spin)
return;
  
-	gem_sync(fd, arg.handle);

-   gem_close(fd, arg.handle);
+   gem_sync(fd, arg.spin->handle); /* Wait until it hangs */


I was expecting a poll spinner + manual reset here.


+   igt_spin_batch_free(fd, arg.spin);
  
  	context_set_ban(fd, arg.ctx, arg.ban);
  
diff --git a/lib/igt_gt.h b/lib/igt_gt.h



+
+#define igt_list_for_each_safe(pos, tmp, head, member) \


(nitpick) extra tab.


+   for (pos = igt_list_first_entry(head, pos, member), \
+tmp = igt_list_next_entry(pos, member);\


I trust you and the compiler got all the places that needed changing.

Reviewed-by: Antonio Argenziano 

Thanks for picking-up this. At my speed it would have taken years :).
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/2] lib: Spin fast, sweet chariot, coming to carry me home

[Antonio Argenziano]

On 19/06/18 06:55, Chris Wilson wrote:

When using the pollable spinner, we often want to use it as a means of
ensuring the task is running on the GPU before switching to something
else. In which case we don't want to add extra delay inside the spinner,
but the current 1000 NOPs add on order of 5us, which is often larger
than the target latency.

Signed-off-by: Chris Wilson 


Reviewed-by: Antonio Argenziano 


---
  lib/igt_dummyload.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index d32b421c6..b090b8004 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -78,6 +78,7 @@ fill_reloc(struct drm_i915_gem_relocation_entry *reloc,
  #define OUT_FENCE (1 << 0)
  #define POLL_RUN  (1 << 1)
  #define NO_PREEMPTION   (1 << 2)
+#define SPIN_FAST   (1 << 3)
  
  static int

  emit_recursive_batch(igt_spin_t *spin, int fd, uint32_t ctx, unsigned engine,
@@ -212,7 +213,8 @@ emit_recursive_batch(igt_spin_t *spin, int fd, uint32_t 
ctx, unsigned engine,
 * between function calls, that appears enough to keep SNB out of
 * trouble. See https://bugs.freedesktop.org/show_bug.cgi?id=102262
 */
-   batch += 1000;
+   if (!(flags & SPIN_FAST))
+   batch += 1000;
  
  	/* recurse */

r = [obj[BATCH].relocation_count++];
@@ -369,7 +371,7 @@ igt_spin_batch_new_fence(int fd, uint32_t ctx, unsigned 
engine)
  igt_spin_t *
  __igt_spin_batch_new_poll(int fd, uint32_t ctx, unsigned engine)
  {
-   return ___igt_spin_batch_new(fd, ctx, engine, 0, POLL_RUN);
+   return ___igt_spin_batch_new(fd, ctx, engine, 0, POLL_RUN | SPIN_FAST);
  }
  
  igt_spin_t *



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

Re: [Intel-gfx] [PATCH i-g-t] igt/gem_eio: Make reset-stress safe

[Antonio Argenziano]

On 15/06/18 11:56, Chris Wilson wrote:

As we hang ctx0 quite frequently, it needs to be harden against being
banned.

Signed-off-by: Chris Wilson 
Cc: Mika Kuoppala 


For some reason I thought we had more hang stress tests.

Reviewed-by: Antonio Argenziano 


---
  tests/gem_eio.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index e1aff639d..5faf7502b 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -642,7 +642,7 @@ static void test_inflight_internal(int fd, unsigned int 
wait)
   */
  static void test_reset_stress(int fd, unsigned int flags)
  {
-   uint32_t ctx0 = gem_context_create(fd);
+   uint32_t ctx0 = context_create_safe(fd);


I guess until (before Mika's patch) now we were covered by 'ctx'
being created and destroyed all the time.

Thanks,
Antonio

  
  	igt_until_timeout(5) {

struct drm_i915_gem_execbuffer2 execbuf = { };


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2] igt/drv_module_reload: Revamp fault-injection

[Antonio Argenziano]

On 06/06/18 10:42, Chris Wilson wrote:

The current method of checking for a failed module load is flawed, as we
only report the error on probing it is not being reported back by
modprobe. So we have to dig inside the module_parameters while the
module is still loaded to discover the error.

v2: Expect i915.inject_load_failure to be zero on success

Signed-off-by: Chris Wilson 
Cc: Michał Winiarski 
Cc: Imre Deak 
Reviewed-by: Michał Winiarski 
---
  tests/drv_module_reload.c | 45 ++-
  1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c
index 092982960..e18aaea5e 100644
--- a/tests/drv_module_reload.c
+++ b/tests/drv_module_reload.c
@@ -234,6 +234,38 @@ reload(const char *opts_i915)
return err;
  }
  



  static void
  gem_sanitycheck(void)
  {
@@ -323,12 +355,15 @@ igt_main
igt_assert_eq(reload("disable_display=1"), 0);
  
  	igt_subtest("basic-reload-inject") {

-   char buf[64];
int i = 0;
-   do {
-   snprintf(buf, sizeof(buf),
-"inject_load_failure=%d", ++i);
-   } while (reload(buf));
+
+   igt_i915_driver_unload();
+
+   while (inject_fault("i915", "inject_load_failure", ++i) == 0)
+   ;
+
+   /* We expect to hit at least one fault! */
+   igt_assert(i > 1);


I think Michal's patch adds the number of available checkpoints in a 
debugfs, should we trust the driver and assert on: amount of checkpoints 
hit != available checkpoints? Or maybe just spew out a warning.


Thanks,
Antonio


}
  
  	igt_fixture {



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

Re: [Intel-gfx] [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Antonio Argenziano]

On 30/05/18 12:52, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-05-30 18:30:36)



On 30/05/18 03:33, Chris Wilson wrote:

After hitting the SIGINT from execbuf, wait until the next timer signal
before trying again. This aligns the start of the ioctl to the timer,
hopefully maximising the amount of time we have for processing before
the next signal -- trying to prevent the case where we are scheduled out
in the middle of processing and so hit the timer signal too early.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
Signed-off-by: Chris Wilson 


Not sure I understand what is the sequence of events, is the problem we
get a signal in the middle of a 'good' execbuf and exit the while loop
prematurely? If so maybe we can also think of making the timer 'VIRTUAL'
so that it would decrement only when the process is executing.


If it's VIRTUAL it'll never fire when we wait for space (as being asleep
no user/sys time is consumed).

The only way I can explain 106695 would be with some very strange
scheduler behaviour, but even then it requires us to hit a path where we
actually check for a pending signal -- which should only happen when we
run out of ring space for this setup. Not even the device being wedged
(which it wasn't) would cause the ring to drain. Possibly going over 10s
and the cork being unplugged? Very stange.


Just a bit concerned that we might be covering up some weird corner case 
where we are sleeping where we shouldn't.


But the patch does what advertised and seems sensible so:

Acked-by: Antonio Argenziano 




---
   lib/i915/gem_ring.c | 2 ++
   1 file changed, 2 insertions(+)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 7d64165eb..0c061000c 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum 
measure_ring_flags
   if (last == count)
   break;
   
+ /* sleep until the next timer interrupt (woken on signal) */

+ pause();


Does it cause any (sensible) slowdown?


Adds at most one timer interval, 10us. Ok, at a push 2 timer intervals
if it takes longer than first to setup the sleep.
-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 3/3] lib: Double check ring measurement

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:

Check twice for the signal interrupting the execbuf, because the real
world is messy.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 


LGTM.

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

Re: [Intel-gfx] [PATCH i-g-t 2/3] lib: Align ring measurement to timer

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:

After hitting the SIGINT from execbuf, wait until the next timer signal
before trying again. This aligns the start of the ioctl to the timer,
hopefully maximising the amount of time we have for processing before
the next signal -- trying to prevent the case where we are scheduled out
in the middle of processing and so hit the timer signal too early.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106695
Signed-off-by: Chris Wilson 


Not sure I understand what is the sequence of events, is the problem we 
get a signal in the middle of a 'good' execbuf and exit the while loop 
prematurely? If so maybe we can also think of making the timer 'VIRTUAL' 
so that it would decrement only when the process is executing.


Thanks,
Antonio


---
  lib/i915/gem_ring.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 7d64165eb..0c061000c 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -96,6 +96,8 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, enum 
measure_ring_flags
if (last == count)
break;
  
+		/* sleep until the next timer interrupt (woken on signal) */

+   pause();


Does it cause any (sensible) slowdown?

Thanks,
Antonio


last = count;
} while (1);
  


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] lib: Assert that we do manage to submit at least one batch when measuring

[Antonio Argenziano]

On 30/05/18 03:33, Chris Wilson wrote:

As we measure the ring size, we never expect to find we can not submit
no batches at all. Assert against the unexpected.

Signed-off-by: Chris Wilson 
Cc: Antonio Argenziano 
---
  lib/i915/gem_ring.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/lib/i915/gem_ring.c b/lib/i915/gem_ring.c
index 10d2f2cd4..7d64165eb 100644
--- a/lib/i915/gem_ring.c
+++ b/lib/i915/gem_ring.c
@@ -100,6 +100,7 @@ __gem_measure_ring_inflight(int fd, unsigned int engine, 
enum measure_ring_flags
} while (1);
  
  	igt_assert_eq(__execbuf(fd, ), -EINTR);

+   igt_assert(count);


Maybe a courtesy print?

With or without,
Reviewed-by: Antonio Argenziano 

  
  	memset(, 0, sizeof(itv));

setitimer(ITIMER_REAL, , NULL);


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/perf_pmu: Flush to idle after hang

[Antonio Argenziano]

On 25/05/18 08:14, Chris Wilson wrote:

We may not idle immediately after a hang, and indeed may send a pulse
down the pipeline periodically to become idle. Rather than make a flimsy
assumption about how long we need to sleep before the system idles,
wait for the system to declare itself idle; flushing it to idle in the
process!

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 


LGTM.
Reviewed-by: Antonio Argenziano 


---
  tests/perf_pmu.c | 11 ++-
  1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
index 590e6526b..9af192dd8 100644
--- a/tests/perf_pmu.c
+++ b/tests/perf_pmu.c
@@ -281,16 +281,9 @@ single(int gem_fd, const struct intel_execution_engine2 
*e, unsigned int flags)
  
  	/* Check for idle after hang. */

if (flags & FLAG_HANG) {
-   /* Sleep for a bit for reset unwind to settle. */
-   usleep(500e3);
-   /*
-* Ensure batch was executing before reset, meaning it must be
-* idle by now. Unless it did not even manage to start before we
-* triggered the reset, in which case the idleness check below
-* might fail. The latter is very unlikely since there are two
-* sleeps during which it had an opportunity to start.
-*/
+   gem_quiescent_gpu(gem_fd);
igt_assert(!gem_bo_busy(gem_fd, spin->handle));
+
val = pmu_read_single(fd);
slept = measured_usleep(batch_duration_ns / 1000);
val = pmu_read_single(fd) - val;


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] tests/drv_suspend: Suspend under memory pressure

[Antonio Argenziano]

On 24/05/18 05:42, Chris Wilson wrote:

Recently we discovered that we have a race between swapping and
suspend in our resume path (we might be trying to page in an object
after disabling the block devices). Let's try to exercise that by
exhausting all of system memory before suspend.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106640
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Tomi Sarvela <tomi.p.sarv...@intel.com>


LGTM:
Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>


---
  lib/igt_core.c  | 34 --
  lib/igt_core.h  |  1 +
  tests/drv_suspend.c | 42 ++
  3 files changed, 63 insertions(+), 14 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index e292ca24c..804ce4578 100644



+static void
+test_shrink(int fd, unsigned int mode)
+{
+   uint64_t *can_mlock;
+   void *locked;
+   uint64_t pin;
+
+   gem_quiescent_gpu(fd);
+   intel_purge_vm_caches(fd);
+
+   can_mlock = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
+   igt_assert(can_mlock != MAP_FAILED);
+
+   pin = intel_get_total_ram_mb() << 20;
+
+   igt_debug("Locking %'"PRIu64" MiB\n", pin >> 20);
+   locked = malloc(pin);
+   igt_assert(locked);


Shouldn't this^ be a require as well?


+
+   /* Lock all the system memory, forcing the driver into swap and OOM */



+
  static void
  test_forcewake(int fd, bool hibernate)
  {
@@ -199,6 +238,9 @@ igt_main
igt_subtest("sysfs-reader")
test_sysfs_reader(false);
  
+	igt_subtest("shrink")

+   test_shrink(fd, SUSPEND_STATE_MEM);


I am assuming you plan to have tests for other suspend modes.

Thanks,
Antonio


+
igt_subtest("forcewake")
test_forcewake(fd, false);
  


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

Re: [Intel-gfx] [PATCH i-g-t] igt/kms_frontbuffer_tracking: Skip over IGT_DRAW_BLT when there's no BLT

[Antonio Argenziano]

On 17/05/18 02:56, Chris Wilson wrote:

If the blitter is not available, we cannot use it as a source for dirty
rectangles. We shall have to rely on the other engines to create GPU
dirty instead.

v2: Try using lots of subgroup+fixtures

Signed-off-by: Chris Wilson 



TEST_MODE_ITER_BEGIN(t)
+   igt_fixture {
+   if (t.method == IGT_DRAW_BLT)
+   gem_require_blitter(drm.fd); > + }


Put the require inside the subtest so to leave a constant sub-tests 
list. Unless I got lost in the nested macros ;).


Thanks,
Antonio


igt_subtest_f("%s-%s-%s-%s-%s-draw-%s",
  feature_str(t.feature),
  pipes_str(t.pipes),


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

Re: [Intel-gfx] [PATCH i-g-t 2/3] igt/gem_blits: Check for blitter support before use

[Antonio Argenziano]

On 17/05/18 01:23, Chris Wilson wrote:

Not all HW supports XY blitter commands, so check before use.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
  lib/i915/gem_submission.c | 16 
  lib/i915/gem_submission.h |  3 +++
  tests/gem_linear_blits.c  |  1 +
  tests/gem_tiled_blits.c   |  1 +
  tests/gem_tiled_fence_blits.c |  1 +
  5 files changed, 22 insertions(+)

diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index 2fd460d5e..1d1adcf6f 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -229,3 +229,19 @@ void gem_test_engine(int i915, unsigned int engine)
igt_assert(!is_wedged(i915));
close(i915);
  }
+
+bool gem_has_blitter(int i915)
+{
+   unsigned int blt;
+
+   blt = 0;
+   if (intel_gen(intel_get_drm_devid(i915)) >= 6)


Looks like we have a "HAS_BLT_RING" macro we use in other places. Also, 
we have a gem_has_blt() function which is slightly different. Which is 
making things quite confusing for me... I wish I had a better name but I 
think we should add at least a comment to differentiate the two.


With that:
Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>

Thanks,
Antonio


+   blt = I915_EXEC_BLT; > +
+   return gem_has_ring(i915, blt);
+}
+
+void gem_require_blitter(int i915)
+{
+   igt_require(gem_has_blitter(i915));
+}
diff --git a/lib/i915/gem_submission.h b/lib/i915/gem_submission.h
index f94eabb20..f2b18d9dc 100644
--- a/lib/i915/gem_submission.h
+++ b/lib/i915/gem_submission.h
@@ -33,6 +33,9 @@ bool gem_has_semaphores(int fd);
  bool gem_has_execlists(int fd);
  bool gem_has_guc_submission(int fd);
  
+bool gem_has_blitter(int i915);

+void gem_require_blitter(int i915);
+
  void gem_test_engine(int fd, unsigned int engine);
  
  int gem_reopen_driver(int fd);

diff --git a/tests/gem_linear_blits.c b/tests/gem_linear_blits.c
index 8297416c0..7d05fa865 100644
--- a/tests/gem_linear_blits.c
+++ b/tests/gem_linear_blits.c
@@ -226,6 +226,7 @@ int main(int argc, char **argv)
igt_fixture {
fd = drm_open_driver(DRIVER_INTEL);
igt_require_gem(fd);
+   gem_require_blitter(fd);
}
  
  	igt_subtest("basic")

diff --git a/tests/gem_tiled_blits.c b/tests/gem_tiled_blits.c
index a81226a15..0d472e3a1 100644
--- a/tests/gem_tiled_blits.c
+++ b/tests/gem_tiled_blits.c
@@ -203,6 +203,7 @@ int main(int argc, char **argv)
igt_fixture {
fd = drm_open_driver(DRIVER_INTEL);
igt_require_gem(fd);
+   gem_require_blitter(fd);
  
  		bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);

drm_intel_bufmgr_gem_enable_reuse(bufmgr);
diff --git a/tests/gem_tiled_fence_blits.c b/tests/gem_tiled_fence_blits.c
index 693e96cec..9ab58b5d6 100644
--- a/tests/gem_tiled_fence_blits.c
+++ b/tests/gem_tiled_fence_blits.c
@@ -176,6 +176,7 @@ igt_main
igt_fixture {
fd = drm_open_driver(DRIVER_INTEL);
igt_require_gem(fd);
+   gem_require_blitter(fd);
}
  
  	igt_subtest("basic") {



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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_cpu_reloc: Check HW exists before attempting to use it

[Antonio Argenziano]

On 17/05/18 09:52, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-05-17 17:29:26)



On 17/05/18 08:37, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-05-17 16:08:14)



On 17/05/18 01:23, Chris Wilson wrote:

Confirm we have the available HW before asserting it succeeds.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
tests/gem_cpu_reloc.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tests/gem_cpu_reloc.c b/tests/gem_cpu_reloc.c
index 882c312d4..e3bbcd239 100644
--- a/tests/gem_cpu_reloc.c
+++ b/tests/gem_cpu_reloc.c
@@ -167,6 +167,7 @@ static void run_test(int fd, int count)
use_blt = 0;


Is this^ meant to be EXEC_DEFAULT?


Depends on your viewpoint. EXEC_DEFAULT is zero.


Just wandering if it should enforce EXEC_RENDER. Which I think is what
we want for gen 5-.


Not really. It just wants the default mixed ring. You definitely don't
want to suggest sending blitter commands down the 3D pipe, that would be
even more confusing.


The comment below answers this question as well.




if (intel_gen(noop) >= 6)
use_blt = I915_EXEC_BLT;
+ gem_require_ring(fd, use_blt);


Are any gens 6+ that do not have a BLT ring? if that is the case
shouldn't we use '0' like we do for 5- gens?


No, it has to match the engine for which the blitter commands are valid. If
that engine does not exist, there is no alternative except to rewrite the
test not to use those commands. If there was, it indeed would be included
in the selection above.


So, just to wrap my head around it, the commands we are talking about
here are allowed on render for gen5- but only on blitter on 6+. Right?


There is no render for gen5- either. There is a universal ringbuffer
that can handle multiple different client commands, and on the odd
machine a bit stream decoder.


I see, I wasn't making any sense then :).

Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_cpu_reloc: Check HW exists before attempting to use it

[Antonio Argenziano]

On 17/05/18 08:37, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-05-17 16:08:14)



On 17/05/18 01:23, Chris Wilson wrote:

Confirm we have the available HW before asserting it succeeds.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
   tests/gem_cpu_reloc.c | 1 +
   1 file changed, 1 insertion(+)

diff --git a/tests/gem_cpu_reloc.c b/tests/gem_cpu_reloc.c
index 882c312d4..e3bbcd239 100644
--- a/tests/gem_cpu_reloc.c
+++ b/tests/gem_cpu_reloc.c
@@ -167,6 +167,7 @@ static void run_test(int fd, int count)
   use_blt = 0;


Is this^ meant to be EXEC_DEFAULT?


Depends on your viewpoint. EXEC_DEFAULT is zero.


Just wandering if it should enforce EXEC_RENDER. Which I think is what 
we want for gen 5-.





   if (intel_gen(noop) >= 6)
   use_blt = I915_EXEC_BLT;
+ gem_require_ring(fd, use_blt);


Are any gens 6+ that do not have a BLT ring? if that is the case
shouldn't we use '0' like we do for 5- gens?


No, it has to match the engine for which the blitter commands are valid. If
that engine does not exist, there is no alternative except to rewrite the
test not to use those commands. If there was, it indeed would be included
in the selection above.


So, just to wrap my head around it, the commands we are talking about 
here are allowed on render for gen5- but only on blitter on 6+. Right?


Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [PATCH i-g-t 1/3] igt/gem_cpu_reloc: Check HW exists before attempting to use it

[Antonio Argenziano]

On 17/05/18 01:23, Chris Wilson wrote:

Confirm we have the available HW before asserting it succeeds.

Signed-off-by: Chris Wilson 
---
  tests/gem_cpu_reloc.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/tests/gem_cpu_reloc.c b/tests/gem_cpu_reloc.c
index 882c312d4..e3bbcd239 100644
--- a/tests/gem_cpu_reloc.c
+++ b/tests/gem_cpu_reloc.c
@@ -167,6 +167,7 @@ static void run_test(int fd, int count)
use_blt = 0;


Is this^ meant to be EXEC_DEFAULT?


if (intel_gen(noop) >= 6)
use_blt = I915_EXEC_BLT;
+   gem_require_ring(fd, use_blt);


Are any gens 6+ that do not have a BLT ring? if that is the case 
shouldn't we use '0' like we do for 5- gens?


Thanks,
Antonio

  
  	if (intel_gen(noop) >= 8) {

batch = gen8_batch;


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/gem_eio: Exercise banning

[Antonio Argenziano]

On 14/05/18 08:02, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-05-14 15:51:04)



On 12/05/18 02:03, Chris Wilson wrote:

If we trigger "too many" resets, the context and even the file, will be
banend and subsequent execbufs should fail with -EIO.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>


Does this replace gem_reset_stats@test_ban?


gem_reset_stats was queued to be rewritten from scratch a few years ago.

In short, no it doesn't replace as they are asking slightly different
questions. gem_eio is asking that if banned we get EIO. I have no idea
what API gem_reset_stats is supposed to be asking about, since banning
is not an aspect of DRM_IOCTL_I915_GET_RESET_STATS and so should be
treated very lightly to avoid over-specificity. (Banning is an internal
kernel policy in the name of DoS prevention and not a rigorous defense
or subject to user control.)


I am not sure how much the intention of the tests are different :), but 
if that is the case then we need to check that other contexts are not 
being affected after a ban and they do not report -EIO on submission.


Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] igt/gem_eio: Exercise banning

[Antonio Argenziano]

On 12/05/18 02:03, Chris Wilson wrote:

If we trigger "too many" resets, the context and even the file, will be
banend and subsequent execbufs should fail with -EIO.

Signed-off-by: Chris Wilson 


Does this replace gem_reset_stats@test_ban?

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

Re: [Intel-gfx] [igt-dev] [PATCH igt] test/gem_exec_schedule: Check each engine is an independent timeline

[Antonio Argenziano]

On 23/04/18 08:51, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-04-23 16:37:17)



On 23/04/18 06:43, Chris Wilson wrote:

In the existing ABI, each engine operates its own timeline
(fence.context) and so should execute independently of any other. If we
install a blocker on all other engines, that should not affect execution
on the local engine.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>



+static void independent(int fd, unsigned int engine)
+{
+ IGT_CORK_HANDLE(cork);
+ uint32_t scratch, plug, batch;
+ igt_spin_t *spin = NULL;
+ unsigned int other;
+ uint32_t *ptr;
+
+ igt_require(engine != 0);
+
+ scratch = gem_create(fd, 4096);
+ plug = igt_cork_plug(, fd);
+
+ /* Check that we can submit to engine while all others are blocked */
+ for_each_physical_engine(fd, other) {
+ if (other == engine)
+ continue;
+
+ if (spin == NULL) {
+ spin = __igt_spin_batch_new(fd, 0, other, 0);
+ } else {
+ struct drm_i915_gem_exec_object2 obj = {
+ .handle = spin->handle,
+ };
+ struct drm_i915_gem_execbuffer2 eb = {
+ .buffer_count = 1,
+ .buffers_ptr = to_user_pointer(),
+ .flags = other,
+ };
+ gem_execbuf(fd, );
+ }
+
+ store_dword(fd, 0, other, scratch, 0, other, plug, 0);
+ }
+ igt_require(spin);
+
+ /* Same priority, but different timeline (as different engine) */
+ batch = __store_dword(fd, 0, engine, scratch, 0, engine, plug, 0);


It would be interesting to check that priority scheduling/preemption is
still happening on the free engine.


It's being run on machines without scheduling as well. Reordering tests
are later; not sure if I care about reordering while blocking, that's an
entirely different set of tests being worked on for queues.


Cool, a different set of tests is what I had in mind as well :).

Oh BTW, with the igt_require in the subtests this is:

Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>

Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH igt] test/gem_exec_schedule: Check each engine is an independent timeline

[Antonio Argenziano]

On 23/04/18 06:43, Chris Wilson wrote:

In the existing ABI, each engine operates its own timeline
(fence.context) and so should execute independently of any other. If we
install a blocker on all other engines, that should not affect execution
on the local engine.

Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 



+static void independent(int fd, unsigned int engine)
+{
+   IGT_CORK_HANDLE(cork);
+   uint32_t scratch, plug, batch;
+   igt_spin_t *spin = NULL;
+   unsigned int other;
+   uint32_t *ptr;
+
+   igt_require(engine != 0);
+
+   scratch = gem_create(fd, 4096);
+   plug = igt_cork_plug(, fd);
+
+   /* Check that we can submit to engine while all others are blocked */
+   for_each_physical_engine(fd, other) {
+   if (other == engine)
+   continue;
+
+   if (spin == NULL) {
+   spin = __igt_spin_batch_new(fd, 0, other, 0);
+   } else {
+   struct drm_i915_gem_exec_object2 obj = {
+   .handle = spin->handle,
+   };
+   struct drm_i915_gem_execbuffer2 eb = {
+   .buffer_count = 1,
+   .buffers_ptr = to_user_pointer(),
+   .flags = other,
+   };
+   gem_execbuf(fd, );
+   }
+
+   store_dword(fd, 0, other, scratch, 0, other, plug, 0);
+   }
+   igt_require(spin);
+
+   /* Same priority, but different timeline (as different engine) */
+   batch = __store_dword(fd, 0, engine, scratch, 0, engine, plug, 0);


It would be interesting to check that priority scheduling/preemption is 
still happening on the free engine.



+
+   unplug_show_queue(fd, , engine);
+   gem_close(fd, plug);
+
+   gem_sync(fd, batch);
+   gem_close(fd, batch);
+   igt_assert(gem_bo_busy(fd, spin->handle));
+
+   ptr = gem_mmap__gtt(fd, scratch, 4096, PROT_READ);
+   gem_close(fd, scratch);
+   igt_assert_eq(ptr[0], engine);
+
+   igt_spin_batch_free(fd, spin);
+   gem_quiescent_gpu(fd);
+
+   /* And we expect the others to have overwritten us, order unspecified */
+   igt_assert_neq(ptr[0], engine);
+   munmap(ptr, 4096);
+}
+
  static void smoketest(int fd, unsigned ring, unsigned timeout)
  {
const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -1070,10 +1138,16 @@ igt_main
if (e->exec_id == 0)
continue;
  
-			igt_subtest_f("fifo-%s", e->name) {

-   igt_require(gem_ring_has_physical_engine(fd, 
e->exec_id | e->flags));
-   igt_require(gem_can_store_dword(fd, e->exec_id) | 
e->flags);
-   fifo(fd, e->exec_id | e->flags);
+   igt_subtest_group {
+   igt_fixture {
+   igt_require(gem_ring_has_physical_engine(fd, 
e->exec_id | e->flags));


I thought that we added this to the subtests to have a constant list of 
tests for all platforms.


Thanks,
Antonio


+   igt_require(gem_can_store_dword(fd, 
e->exec_id) | e->flags);
+   }
+
+   igt_subtest_f("fifo-%s", e->name)
+   fifo(fd, e->exec_id | e->flags);
+   igt_subtest_f("independent-%s", e->name)
+   independent(fd, e->exec_id | e->flags);
}
}
}


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

Re: [Intel-gfx] [PATCH igt] igt/gem_exec_schedule: Exercise preemption timeout

[Antonio Argenziano]

On 13/04/18 08:59, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-04-13 16:54:27)



On 13/04/18 07:14, Chris Wilson wrote:

Set up a unpreemptible spinner such that the only way we can inject a
high priority request onto the GPU is by resetting the spinner. The test
fails if we trigger hangcheck rather than the fast timeout mechanism.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
   lib/i915/gem_context.c| 72 +++
   lib/i915/gem_context.h|  3 ++
   lib/igt_dummyload.c   | 12 +--
   lib/igt_dummyload.h   |  3 ++
   tests/gem_exec_schedule.c | 34 ++
   5 files changed, 106 insertions(+), 18 deletions(-)



...


@@ -449,8 +457,6 @@ void igt_spin_batch_end(igt_spin_t *spin)
   if (!spin)
   return;
   
- igt_assert(*spin->batch == MI_ARB_CHK ||

-*spin->batch == MI_BATCH_BUFFER_END);


I am not sure why we needed this, but it seems safe to remove.


   *spin->batch = MI_BATCH_BUFFER_END;
   __sync_synchronize();
   }



diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 6ff15b6ef..93254945b 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -656,6 +656,37 @@ static void preemptive_hang(int fd, unsigned ring)
   gem_context_destroy(fd, ctx[HI]);
   }
   
+static void preempt_timeout(int fd, unsigned ring)

+{
+ igt_spin_t *spin[3];
+ uint32_t ctx;
+
+ igt_require(__gem_context_set_preempt_timeout(fd, 0, 0));
+
+ ctx = gem_context_create(fd);
+ gem_context_set_priority(fd, ctx, MIN_PRIO);
+ spin[0] = __igt_spin_batch_new_hang(fd, ctx, ring);
+ spin[1] = __igt_spin_batch_new_hang(fd, ctx, ring);

Should we send MAX_ELSP_QLEN batches to match other preemption tests?


+ gem_context_destroy(fd, ctx);
+
+ ctx = gem_context_create(fd);
+ gem_context_set_priority(fd, ctx, MAX_PRIO);
+ gem_context_set_preempt_timeout(fd, ctx, 1000 * 1000);
+ spin[2] = __igt_spin_batch_new(fd, ctx, ring, 0);
+ gem_context_destroy(fd, ctx);
+
+ igt_spin_batch_end(spin[2]);
+ gem_sync(fd, spin[2]->handle);


Does this guarantee that spin[1] did not overtake spin[2]?


It does as well. Neither spin[0] or spin[1] can complete without being
reset at this point. If they are reset (by hangcheck) we detect that and


Cool.


die. What we expect to happen is spin[0] is (more or less, there is still
dmesg) silently killed by the preempt timeout. If that timeout doesn't


The silent part is interesting, how do we make sure that during normal 
preemption operations (e.g. preempt on an ARB_CHECK) we didn't silently 
discard the preempted batch? Do we care?


Test looks good,
Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>

Thanks,
Antonio


happen, more hangcheck. What we don't check here is how quick. Now we
could reasonably assert that the spin[2] -> gem_sync takes less than 2ms.
-Chris


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

Re: [Intel-gfx] [PATCH igt] igt/gem_exec_schedule: Exercise preemption timeout

[Antonio Argenziano]

On 13/04/18 07:14, Chris Wilson wrote:

Set up a unpreemptible spinner such that the only way we can inject a
high priority request onto the GPU is by resetting the spinner. The test
fails if we trigger hangcheck rather than the fast timeout mechanism.

Signed-off-by: Chris Wilson 
---
  lib/i915/gem_context.c| 72 +++
  lib/i915/gem_context.h|  3 ++
  lib/igt_dummyload.c   | 12 +--
  lib/igt_dummyload.h   |  3 ++
  tests/gem_exec_schedule.c | 34 ++
  5 files changed, 106 insertions(+), 18 deletions(-)



...


@@ -449,8 +457,6 @@ void igt_spin_batch_end(igt_spin_t *spin)
if (!spin)
return;
  
-	igt_assert(*spin->batch == MI_ARB_CHK ||

-  *spin->batch == MI_BATCH_BUFFER_END);


I am not sure why we needed this, but it seems safe to remove.


*spin->batch = MI_BATCH_BUFFER_END;
__sync_synchronize();
  }



diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 6ff15b6ef..93254945b 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -656,6 +656,37 @@ static void preemptive_hang(int fd, unsigned ring)
gem_context_destroy(fd, ctx[HI]);
  }
  
+static void preempt_timeout(int fd, unsigned ring)

+{
+   igt_spin_t *spin[3];
+   uint32_t ctx;
+
+   igt_require(__gem_context_set_preempt_timeout(fd, 0, 0));
+
+   ctx = gem_context_create(fd);
+   gem_context_set_priority(fd, ctx, MIN_PRIO);
+   spin[0] = __igt_spin_batch_new_hang(fd, ctx, ring);
+   spin[1] = __igt_spin_batch_new_hang(fd, ctx, ring);
+   gem_context_destroy(fd, ctx);
+
+   ctx = gem_context_create(fd);
+   gem_context_set_priority(fd, ctx, MAX_PRIO);
+   gem_context_set_preempt_timeout(fd, ctx, 1000 * 1000);
+   spin[2] = __igt_spin_batch_new(fd, ctx, ring, 0);
+   gem_context_destroy(fd, ctx);
+
+   igt_spin_batch_end(spin[2]);
+   gem_sync(fd, spin[2]->handle);


Does this guarantee that spin[1] did not overtake spin[2]?

Thanks,
Antonio


+
+   /* spin[0] is kicked, leaving spin[1] running */
+
+   igt_assert(gem_bo_busy(fd, spin[1]->handle));
+
+   igt_spin_batch_free(fd, spin[2]);
+   igt_spin_batch_free(fd, spin[1]);
+   igt_spin_batch_free(fd, spin[0]);
+}
+
  static void deep(int fd, unsigned ring)
  {
  #define XS 8
@@ -1120,6 +1151,9 @@ igt_main
igt_subtest_f("preempt-self-%s", 
e->name)
preempt_self(fd, e->exec_id | 
e->flags);
  
+	igt_subtest_f("preempt-timeout-%s", e->name)

+   preempt_timeout(fd, e->exec_id | 
e->flags);
+
igt_subtest_f("preempt-other-%s", 
e->name)
preempt_other(fd, e->exec_id | 
e->flags, 0);
  


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

Re: [Intel-gfx] [igt-dev] [PATCH igt] igt/kms_plane_scaling: Open DRM_MASTER

[Antonio Argenziano]

On 12/04/18 01:43, Chris Wilson wrote:

Modesetting requires DRM_MASTER privileges, so use
drm_open_driver_master() to assert that we do acquire them.

References: https://bugs.freedesktop.org/show_bug.cgi?id=105997
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>


Acked-by: Antonio Argenziano <antonio.argenzi...@intel.com>

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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2 2/2] tests/gem_eio: Add reset and unwedge stress testing

[Antonio Argenziano]

On 04/04/18 02:58, Tvrtko Ursulin wrote:


On 03/04/2018 19:34, Antonio Argenziano wrote:



On 03/04/18 11:24, Antonio Argenziano wrote:



On 03/04/18 04:36, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>

Reset and unwedge stress testing is supposed to trigger wedging or 
resets
at incovenient times and then re-use the context so either the 
context or

driver tracking might get confused and break.

v2:
  * Renamed for more sensible naming.
  * Added some comments to explain what the test is doing. (Chris 
Wilson)


Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
---
  tests/gem_eio.c | 74 
+

  1 file changed, 74 insertions(+)

diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index b7c5047f0816..9599e73db736 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -591,6 +591,74 @@ static void test_inflight_internal(int fd, 
unsigned int wait)

  close(fd);
  }
+/*
+ * Verify that we can submit and execute work after unwedging the GPU.
+ */
+static void test_reset_stress(int fd, unsigned int flags)
+{
+    uint32_t ctx0 = gem_context_create(fd);
+
+    igt_until_timeout(5) {
+    struct drm_i915_gem_execbuffer2 execbuf = { };
+    struct drm_i915_gem_exec_object2 obj = { };
+    uint32_t bbe = MI_BATCH_BUFFER_END;
+    igt_spin_t *hang;
+    unsigned int i;
+    uint32_t ctx;
+
+    gem_quiescent_gpu(fd);
+
+    igt_require(i915_reset_control(flags & TEST_WEDGE ?
+   false : true));
+
+    ctx = context_create_safe(fd);
+
+    /*
+ * Start executing a spin batch with some queued batches
+ * against a different context after it.
+ */


Aren't all batches queued on ctx0? Or is this a reference to the 
check on ctx you have later in the test.


Yes, a mistake in comment text.


+    hang = spin_sync(fd, ctx0, 0);


I think you meant to send this^ on ctx.


Why do you think so? Did you find a different or better way to trigger 
the bug this test is trying to hit?


Nope, I just misunderstood the code :). I thought you were creating ctx 
as 'safe' to be not 'bannable' because you were going to reuse the same 
context across multiple resets and didn't want it to be banned. BTW 
given that this is not the case wouldn't ctx0 be banned after so many 
resets?


Apologies for the cryptic comment,
Antonio.



Regards,

Tvrtko


Antonio.


+
+    obj.handle = gem_create(fd, 4096);
+    gem_write(fd, obj.handle, 0, , sizeof(bbe));
+
+    execbuf.buffers_ptr = to_user_pointer();
+    execbuf.buffer_count = 1;
+    execbuf.rsvd1 = ctx0;
+
+    for (i = 0; i < 10; i++)
+    gem_execbuf(fd, );
+
+    /* Wedge after a small delay. */
+    igt_assert_eq(__check_wait(fd, obj.handle, 100e3), 0);
+
+    /* Unwedge by forcing a reset. */
+    igt_assert(i915_reset_control(true));
+    trigger_reset(fd);
+
+    gem_quiescent_gpu(fd);
+
+    /*
+ * Verify that we are able to submit work after unwedging from
+ * both contexts.
+ */
+    execbuf.rsvd1 = ctx;
+    for (i = 0; i < 5; i++)
+    gem_execbuf(fd, );
+
+    execbuf.rsvd1 = ctx0;
+    for (i = 0; i < 5; i++)
+    gem_execbuf(fd, );
+
+    gem_sync(fd, obj.handle);
+    igt_spin_batch_free(fd, hang);
+    gem_context_destroy(fd, ctx);
+    gem_close(fd, obj.handle);
+    }
+
+    gem_context_destroy(fd, ctx0);
+}
+
  static int fd = -1;
  static void
@@ -635,6 +703,12 @@ igt_main
  igt_subtest("in-flight-suspend")
  test_inflight_suspend(fd);
+    igt_subtest("reset-stress")
+    test_reset_stress(fd, 0);
+
+    igt_subtest("unwedge-stress")
+    test_reset_stress(fd, TEST_WEDGE);
+
  igt_subtest_group {
  const struct {
  unsigned int wait;


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

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

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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2 2/2] tests/gem_eio: Add reset and unwedge stress testing

[Antonio Argenziano]

On 03/04/18 11:24, Antonio Argenziano wrote:



On 03/04/18 04:36, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>

Reset and unwedge stress testing is supposed to trigger wedging or resets
at incovenient times and then re-use the context so either the context or
driver tracking might get confused and break.

v2:
  * Renamed for more sensible naming.
  * Added some comments to explain what the test is doing. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
---
  tests/gem_eio.c | 74 
+

  1 file changed, 74 insertions(+)

diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index b7c5047f0816..9599e73db736 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -591,6 +591,74 @@ static void test_inflight_internal(int fd, 
unsigned int wait)

  close(fd);
  }
+/*
+ * Verify that we can submit and execute work after unwedging the GPU.
+ */
+static void test_reset_stress(int fd, unsigned int flags)
+{
+    uint32_t ctx0 = gem_context_create(fd);
+
+    igt_until_timeout(5) {
+    struct drm_i915_gem_execbuffer2 execbuf = { };
+    struct drm_i915_gem_exec_object2 obj = { };
+    uint32_t bbe = MI_BATCH_BUFFER_END;
+    igt_spin_t *hang;
+    unsigned int i;
+    uint32_t ctx;
+
+    gem_quiescent_gpu(fd);
+
+    igt_require(i915_reset_control(flags & TEST_WEDGE ?
+   false : true));
+
+    ctx = context_create_safe(fd);
+
+    /*
+ * Start executing a spin batch with some queued batches
+ * against a different context after it.
+ */


Aren't all batches queued on ctx0? Or is this a reference to the check 
on ctx you have later in the test.


Thanks,
Antonio


+    hang = spin_sync(fd, ctx0, 0);


I think you meant to send this^ on ctx.

Antonio.


+
+    obj.handle = gem_create(fd, 4096);
+    gem_write(fd, obj.handle, 0, , sizeof(bbe));
+
+    execbuf.buffers_ptr = to_user_pointer();
+    execbuf.buffer_count = 1;
+    execbuf.rsvd1 = ctx0;
+
+    for (i = 0; i < 10; i++)
+    gem_execbuf(fd, );
+
+    /* Wedge after a small delay. */
+    igt_assert_eq(__check_wait(fd, obj.handle, 100e3), 0);
+
+    /* Unwedge by forcing a reset. */
+    igt_assert(i915_reset_control(true));
+    trigger_reset(fd);
+
+    gem_quiescent_gpu(fd);
+
+    /*
+ * Verify that we are able to submit work after unwedging from
+ * both contexts.
+ */
+    execbuf.rsvd1 = ctx;
+    for (i = 0; i < 5; i++)
+    gem_execbuf(fd, );
+
+    execbuf.rsvd1 = ctx0;
+    for (i = 0; i < 5; i++)
+    gem_execbuf(fd, );
+
+    gem_sync(fd, obj.handle);
+    igt_spin_batch_free(fd, hang);
+    gem_context_destroy(fd, ctx);
+    gem_close(fd, obj.handle);
+    }
+
+    gem_context_destroy(fd, ctx0);
+}
+
  static int fd = -1;
  static void
@@ -635,6 +703,12 @@ igt_main
  igt_subtest("in-flight-suspend")
  test_inflight_suspend(fd);
+    igt_subtest("reset-stress")
+    test_reset_stress(fd, 0);
+
+    igt_subtest("unwedge-stress")
+    test_reset_stress(fd, TEST_WEDGE);
+
  igt_subtest_group {
  const struct {
  unsigned int wait;


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

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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t v2 2/2] tests/gem_eio: Add reset and unwedge stress testing

[Antonio Argenziano]

On 03/04/18 04:36, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

Reset and unwedge stress testing is supposed to trigger wedging or resets
at incovenient times and then re-use the context so either the context or
driver tracking might get confused and break.

v2:
  * Renamed for more sensible naming.
  * Added some comments to explain what the test is doing. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin 
---
  tests/gem_eio.c | 74 +
  1 file changed, 74 insertions(+)

diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index b7c5047f0816..9599e73db736 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -591,6 +591,74 @@ static void test_inflight_internal(int fd, unsigned int 
wait)
close(fd);
  }
  
+/*

+ * Verify that we can submit and execute work after unwedging the GPU.
+ */
+static void test_reset_stress(int fd, unsigned int flags)
+{
+   uint32_t ctx0 = gem_context_create(fd);
+
+   igt_until_timeout(5) {
+   struct drm_i915_gem_execbuffer2 execbuf = { };
+   struct drm_i915_gem_exec_object2 obj = { };
+   uint32_t bbe = MI_BATCH_BUFFER_END;
+   igt_spin_t *hang;
+   unsigned int i;
+   uint32_t ctx;
+
+   gem_quiescent_gpu(fd);
+
+   igt_require(i915_reset_control(flags & TEST_WEDGE ?
+  false : true));
+
+   ctx = context_create_safe(fd);
+
+   /*
+* Start executing a spin batch with some queued batches
+* against a different context after it.
+*/


Aren't all batches queued on ctx0? Or is this a reference to the check 
on ctx you have later in the test.


Thanks,
Antonio


+   hang = spin_sync(fd, ctx0, 0);
+
+   obj.handle = gem_create(fd, 4096);
+   gem_write(fd, obj.handle, 0, , sizeof(bbe));
+
+   execbuf.buffers_ptr = to_user_pointer();
+   execbuf.buffer_count = 1;
+   execbuf.rsvd1 = ctx0;
+
+   for (i = 0; i < 10; i++)
+   gem_execbuf(fd, );
+
+   /* Wedge after a small delay. */
+   igt_assert_eq(__check_wait(fd, obj.handle, 100e3), 0);
+
+   /* Unwedge by forcing a reset. */
+   igt_assert(i915_reset_control(true));
+   trigger_reset(fd);
+
+   gem_quiescent_gpu(fd);
+
+   /*
+* Verify that we are able to submit work after unwedging from
+* both contexts.
+*/
+   execbuf.rsvd1 = ctx;
+   for (i = 0; i < 5; i++)
+   gem_execbuf(fd, );
+
+   execbuf.rsvd1 = ctx0;
+   for (i = 0; i < 5; i++)
+   gem_execbuf(fd, );
+
+   gem_sync(fd, obj.handle);
+   igt_spin_batch_free(fd, hang);
+   gem_context_destroy(fd, ctx);
+   gem_close(fd, obj.handle);
+   }
+
+   gem_context_destroy(fd, ctx0);
+}
+
  static int fd = -1;
  
  static void

@@ -635,6 +703,12 @@ igt_main
igt_subtest("in-flight-suspend")
test_inflight_suspend(fd);
  
+	igt_subtest("reset-stress")

+   test_reset_stress(fd, 0);
+
+   igt_subtest("unwedge-stress")
+   test_reset_stress(fd, TEST_WEDGE);
+
igt_subtest_group {
const struct {
unsigned int wait;


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/3] tests/gem_eio: Speed up test execution

[Antonio Argenziano]

On 22/03/18 11:14, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-03-22 17:32:46)



On 22/03/18 05:42, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-03-22 12:36:58)


On 22/03/2018 11:39, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-03-22 11:17:11)


   trigger_reset(fd);
+
+   /* HACK for CI */
+   igt_assert(igt_nsec_elapsed() < 5e9);


igt_seconds_elapsed() the approximation is worth the readability.

In this case you might like to try igt_set_timeout(), as I think each
subtest and exithandlers are in place to make them robust against
premature failures.


Well this was just to see that will happen on the shards here. As
mentioned in the commit I get that yet unexplained GPU hang at subtest
exit here. So the assert above is just to notice if the same happens on
shards.


And I was thinking it was a reasonable enhancement :) Probably more so
for igt/gem_wait itself to ask that if we reset the request we are
waiting upon it completes in a timely manner. (We don't care about
wedged handling there, just reset handling.)


How about checking for reset when we do gem_test_engine(), which seems
to not fail on reset, (crudely https://paste.debian.net/1016059/)?


I was thinking that the timeout would be good around the test as a
whole, because it is now meant to be uberfast.

Makes sense.

Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/3] tests/gem_eio: Speed up test execution

[Antonio Argenziano]

On 22/03/18 05:42, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-03-22 12:36:58)


On 22/03/2018 11:39, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-03-22 11:17:11)


  trigger_reset(fd);
+
+   /* HACK for CI */
+   igt_assert(igt_nsec_elapsed() < 5e9);


igt_seconds_elapsed() the approximation is worth the readability.

In this case you might like to try igt_set_timeout(), as I think each
subtest and exithandlers are in place to make them robust against
premature failures.


Well this was just to see that will happen on the shards here. As
mentioned in the commit I get that yet unexplained GPU hang at subtest
exit here. So the assert above is just to notice if the same happens on
shards.


And I was thinking it was a reasonable enhancement :) Probably more so
for igt/gem_wait itself to ask that if we reset the request we are
waiting upon it completes in a timely manner. (We don't care about
wedged handling there, just reset handling.)


How about checking for reset when we do gem_test_engine(), which seems 
to not fail on reset, (crudely https://paste.debian.net/1016059/)?


Antonio


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


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

Re: [Intel-gfx] [igt-dev] [PATCH igt 2/3] igt/gem_exec_fence: Exercise merging fences

[Antonio Argenziano]

On 16/03/18 15:14, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-03-16 22:11:11)



On 13/03/18 05:31, Chris Wilson wrote:

Execute the same batch on each engine and check that the composite fence
across all engines completes only after the batch is completed on every
engine.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
   tests/gem_exec_fence.c | 127 
+
   1 file changed, 127 insertions(+)

diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 26bde788..95fc65e5 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -208,6 +208,113 @@ static void test_fence_busy(int fd, unsigned ring, 
unsigned flags)
   gem_quiescent_gpu(fd);
   }
   
+static void test_fence_busy_all(int fd, unsigned flags)

+{
+ const int gen = intel_gen(intel_get_drm_devid(fd));
+ struct drm_i915_gem_exec_object2 obj;
+ struct drm_i915_gem_relocation_entry reloc;
+ struct drm_i915_gem_execbuffer2 execbuf;
+ struct timespec tv;
+ uint32_t *batch;
+ unsigned int engine;
+ int all, i, timeout;
+
+ gem_quiescent_gpu(fd);
+
+ memset(, 0, sizeof(execbuf));
+ execbuf.buffers_ptr = to_user_pointer();
+ execbuf.buffer_count = 1;
+
+ memset(, 0, sizeof(obj));
+ obj.handle = gem_create(fd, 4096);
+
+ obj.relocs_ptr = to_user_pointer();
+ obj.relocation_count = 1;
+ memset(, 0, sizeof(reloc));
+
+ batch = gem_mmap__wc(fd, obj.handle, 0, 4096, PROT_WRITE);
+ gem_set_domain(fd, obj.handle,
+I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+ reloc.target_handle = obj.handle; /* recurse */
+ reloc.presumed_offset = 0;
+ reloc.offset = sizeof(uint32_t);
+ reloc.delta = 0;
+ reloc.read_domains = I915_GEM_DOMAIN_COMMAND;
+ reloc.write_domain = 0;
+
+ i = 0;
+ batch[i] = MI_BATCH_BUFFER_START;
+ if (gen >= 8) {
+ batch[i] |= 1 << 8 | 1;
+ batch[++i] = 0;
+ batch[++i] = 0;
+ } else if (gen >= 6) {
+ batch[i] |= 1 << 8;
+ batch[++i] = 0;
+ } else {
+ batch[i] |= 2 << 6;
+ batch[++i] = 0;
+ if (gen < 4) {
+ batch[i] |= 1;
+ reloc.delta = 1;
+ }
+ }
+ i++;
+
+ all = -1;
+ for_each_engine(fd, engine) {


for_each_physical_engines to avoid submitting twice to the same engine.


iirc, I thought about that in passing and decided that the ABI exercise
was the intent here and not the HW exercise.


Makes sense, my RB stands then without the change.

Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [igt-dev] [PATCH igt 2/3] igt/gem_exec_fence: Exercise merging fences

[Antonio Argenziano]

On 13/03/18 05:31, Chris Wilson wrote:

Execute the same batch on each engine and check that the composite fence
across all engines completes only after the batch is completed on every
engine.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
  tests/gem_exec_fence.c | 127 +
  1 file changed, 127 insertions(+)

diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 26bde788..95fc65e5 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -208,6 +208,113 @@ static void test_fence_busy(int fd, unsigned ring, 
unsigned flags)
gem_quiescent_gpu(fd);
  }
  
+static void test_fence_busy_all(int fd, unsigned flags)

+{
+   const int gen = intel_gen(intel_get_drm_devid(fd));
+   struct drm_i915_gem_exec_object2 obj;
+   struct drm_i915_gem_relocation_entry reloc;
+   struct drm_i915_gem_execbuffer2 execbuf;
+   struct timespec tv;
+   uint32_t *batch;
+   unsigned int engine;
+   int all, i, timeout;
+
+   gem_quiescent_gpu(fd);
+
+   memset(, 0, sizeof(execbuf));
+   execbuf.buffers_ptr = to_user_pointer();
+   execbuf.buffer_count = 1;
+
+   memset(, 0, sizeof(obj));
+   obj.handle = gem_create(fd, 4096);
+
+   obj.relocs_ptr = to_user_pointer();
+   obj.relocation_count = 1;
+   memset(, 0, sizeof(reloc));
+
+   batch = gem_mmap__wc(fd, obj.handle, 0, 4096, PROT_WRITE);
+   gem_set_domain(fd, obj.handle,
+  I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+   reloc.target_handle = obj.handle; /* recurse */
+   reloc.presumed_offset = 0;
+   reloc.offset = sizeof(uint32_t);
+   reloc.delta = 0;
+   reloc.read_domains = I915_GEM_DOMAIN_COMMAND;
+   reloc.write_domain = 0;
+
+   i = 0;
+   batch[i] = MI_BATCH_BUFFER_START;
+   if (gen >= 8) {
+   batch[i] |= 1 << 8 | 1;
+   batch[++i] = 0;
+   batch[++i] = 0;
+   } else if (gen >= 6) {
+   batch[i] |= 1 << 8;
+   batch[++i] = 0;
+   } else {
+   batch[i] |= 2 << 6;
+   batch[++i] = 0;
+   if (gen < 4) {
+   batch[i] |= 1;
+   reloc.delta = 1;
+   }
+   }
+   i++;
+
+   all = -1;
+   for_each_engine(fd, engine) {


for_each_physical_engines to avoid submitting twice to the same engine.

Other than that it LGTM.

Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>


+   int fence, new;
+
+   execbuf.flags = engine | LOCAL_EXEC_FENCE_OUT;
+   execbuf.rsvd2 = -1;
+   gem_execbuf_wr(fd, );
+   fence = execbuf.rsvd2 >> 32;
+   igt_assert(fence != -1);
+
+   if (all < 0) {
+   all = fence;
+   break;
+   }
+
+   new = sync_fence_merge(all, fence);
+   igt_assert_lte(0, new);
+   close(all);
+   close(fence);
+
+   all = new;
+   }
+
+   igt_assert(gem_bo_busy(fd, obj.handle));
+   igt_assert(fence_busy(all));
+
+   timeout = 120;
+   if ((flags & HANG) == 0) {
+   *batch = MI_BATCH_BUFFER_END;
+   __sync_synchronize();
+   timeout = 1;
+   }
+   munmap(batch, 4096);
+
+   if (flags & WAIT) {
+   struct pollfd pfd = { .fd = all, .events = POLLIN };
+   igt_assert(poll(, 1, timeout*1000) == 1);
+   } else {
+   memset(, 0, sizeof(tv));
+   while (fence_busy(all))
+   igt_assert(igt_seconds_elapsed() < timeout);
+   }
+
+   igt_assert(!gem_bo_busy(fd, obj.handle));
+   igt_assert_eq(sync_fence_status(all),
+ flags & HANG ? -EIO : SYNC_FENCE_OK);
+
+   close(all);
+   gem_close(fd, obj.handle);
+
+   gem_quiescent_gpu(fd);
+}
+
  static void test_fence_await(int fd, unsigned ring, unsigned flags)
  {
const int gen = intel_gen(intel_get_drm_devid(fd));
@@ -1465,6 +1572,26 @@ igt_main
gem_submission_print_method(i915);
}
  
+	igt_subtest_group {

+   igt_fixture {
+   igt_fork_hang_detector(i915);
+   }
+
+   igt_subtest("basic-busy-all")
+   test_fence_busy_all(i915, 0);
+   igt_subtest("basic-wait-all")
+   test_fence_busy_all(i915, WAIT);
+
+   igt_fixture {
+   igt_stop_hang_detector();
+   }
+
+   igt_subtest("busy-hang-all")
+   test_fence_busy_all(i915, HANG);
+   igt_subtest("wait-hang-all")
+   test_fence_busy_all(i915, WAIT | HANG);

Re: [Intel-gfx] [igt-dev] [PATCH igt 1/3] igt/gem_eio: Exercise set-wedging against request submission

[Antonio Argenziano]

On 13/03/18 05:31, Chris Wilson wrote:

Build up a large stockpile of requests, ~500,000, and feed them into the
system at 20KHz whilst simultaneously triggering set-wedged in order to
try and race i915_gem_set_wedged() against the engine->submit_request()
callback.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com>


LGTM.

Acked-by: Antonio Argenziano <antonio.argenzi...@intel.com>


---
  tests/gem_eio.c | 108 
  1 file changed, 108 insertions(+)



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

Re: [Intel-gfx] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Antonio Argenziano]

On 09/03/18 13:35, Chris Wilson wrote:

Exercise some new API that allows applications to request that
individual contexts are executed within a desired frequency range.

v2: Split single/continuous set_freq subtests
v3: Do an up/down ramp for individual freq request, check nothing
changes after each invalid request
v4: Check the frequencies reported by the kernel across the entire
range.
v5: Rewrite sandwich to create a sandwich between multiple concurrent
engines.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Praveen Paneri <praveen.pan...@intel.com>
Cc: Sagar A Kamble <sagar.a.kam...@intel.com>
Cc: Antonio Argenziano <antonio.argenzi...@intel.com>


LGTM.

Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Antonio Argenziano]

On 08/03/18 17:03, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-03-09 00:45:42)



On 08/03/18 09:13, Chris Wilson wrote:

Exercise some new API that allows applications to request that
individual contexts are executed within a desired frequency range.

v2: Split single/continuous set_freq subtests

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Paneri, Praveen <praveen.pan...@intel.com>
Cc: Kamble, Sagar A <sagar.a.kam...@intel.com>
Cc: Antonio Argenziano <antonio.argenzi...@intel.com>
---
   tests/Makefile.am  |   1 +
   tests/Makefile.sources |   1 +
   tests/gem_ctx_freq.c   | 604 
+
   tests/meson.build  |   1 +
   4 files changed, 607 insertions(+)
   create mode 100644 tests/gem_ctx_freq.c






+ uint32_t cur, discard;
+
+ set_freq(fd, ctx, freq, freq);
+ get_freq(fd, ctx, , );


igt_assert_eq(freq, cur)?


Not quite. The trick is that the interface is not strictly idempotent,
since we pass in MHz, the driver converts that into freq bins and spits
it back out to the nearest MHz. So cur is not strictly freq, it just
happens that 50MHz is the bin size on gen9.

The idea here is that we grab the adjusted freq from the driver to
validate with.


I see, can we enforce a tolerance? It feels like we are trusting the 
kernel too much, but if that is the only thing we can do...



+static void smoketest(int fd, int timeout)
+{
+ unsigned int engines[16];


use a macro instead of magic number 16.


#define 
THIS_IS_FAR_MORE_MAGIC_THAN_A_MEANINGLESS_BARE_NUMBER_THAT_YOU_SHOULD_NOT_BE_READING_ANYTHING_INTO
 16
/rant


We call that MAX_EGINES in gem_exec_schedule ;).

Thanks,
Antonio




+static void invalid_param(int fd)
+{


gem_ctx_param is going to be upset again pretty soon ;).


Poor thing.
-Chris


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

Re: [Intel-gfx] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Antonio Argenziano]

On 08/03/18 09:13, Chris Wilson wrote:

Exercise some new API that allows applications to request that
individual contexts are executed within a desired frequency range.

v2: Split single/continuous set_freq subtests

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Paneri, Praveen <praveen.pan...@intel.com>
Cc: Kamble, Sagar A <sagar.a.kam...@intel.com>
Cc: Antonio Argenziano <antonio.argenzi...@intel.com>
---
  tests/Makefile.am  |   1 +
  tests/Makefile.sources |   1 +
  tests/gem_ctx_freq.c   | 604 +
  tests/meson.build  |   1 +
  4 files changed, 607 insertions(+)
  create mode 100644 tests/gem_ctx_freq.c




+static void single(int fd, const struct intel_execution_engine *e)
+{
+   const unsigned int engine = e->exec_id | e->flags;
+   uint32_t ctx = gem_context_create(fd);
+   uint32_t min, max;
+   double measured;
+   igt_spin_t *spin;
+   int pmu;
+
+   get_freq(fd, ctx, , );
+   igt_info("Min freq: %dMHz; Max freq: %dMHz\n", min, max);
+
+   pmu = perf_i915_open(I915_PMU_REQUESTED_FREQUENCY);
+   igt_require(pmu >= 0);
+
+   for (uint32_t freq = min + 50; freq <= max; freq += 100) {


Although it is done in the smoke test, it would be interesting if freq's 
values were a bit randomized.



+   uint32_t cur, discard;
+
+   set_freq(fd, ctx, freq, freq);
+   get_freq(fd, ctx, , );


igt_assert_eq(freq, cur)?


+
+   gem_quiescent_gpu(fd);
+   spin = __igt_spin_batch_new(fd, ctx, engine, 0);
+   usleep(1);


I guess here we wait for the frequency changes to take effect, maybe a 
small comment would help.



+
+   measured = measure_frequency(pmu, SAMPLE_PERIOD);
+   igt_debugfs_dump(fd, "i915_rps_boost_info");
+
+   igt_spin_batch_free(fd, spin);
+   igt_info("%s(single): Measured %.1fMHz, expected %dMhz\n",
+e->name, measured, cur);
+   igt_assert(measured > cur - 100 && measured < cur + 100);
+   }
+   gem_quiescent_gpu(fd);
+
+   close(pmu);
+   gem_context_destroy(fd, ctx);
+}
+



+
+static void sandwich(int fd)
+{
+   uint32_t ctx = gem_context_create(fd);
+   unsigned int engine;
+   uint32_t min, max;
+   igt_spin_t *spin;
+   int pmu;
+
+   pmu = perf_i915_open(I915_PMU_REQUESTED_FREQUENCY);
+   igt_require(pmu >= 0);
+
+   spin = igt_spin_batch_new(fd, ctx, 0, 0);
+   get_freq(fd, ctx, , );
+   set_freq(fd, ctx, min, min);
+   for_each_physical_engine(fd, engine) {
+   struct drm_i915_gem_exec_object2 obj = {
+   .handle = spin->handle,
+   };
+   struct drm_i915_gem_execbuffer2 eb = {
+   .buffer_count = 1,
+   .buffers_ptr = to_user_pointer(),
+   .flags = engine,
+   .rsvd1 = ctx,
+   };
+   uint32_t cur, discard;
+   double measured;
+
+   min += 50;
+   if (min > max)
+   break;
+
+   set_freq(fd, ctx, min, min);
+   get_freq(fd, ctx, , );
+
+   gem_execbuf(fd, );
+   usleep(1);
+
+   measured = measure_frequency(pmu, SAMPLE_PERIOD);
+   igt_debugfs_dump(fd, "i915_rps_boost_info");
+
+   igt_info("Measured %.1fMHz, expected %dMhz\n", measured, cur);
+   igt_assert(measured > cur - 100 && measured < cur + 100);


Does the frequency change after each execbuf?


+   }
+   igt_spin_batch_free(fd, spin);
+   gem_quiescent_gpu(fd);
+
+   gem_context_destroy(fd, ctx);
+   close(pmu);
+}
+
+static void pwm(int fd, unsigned int *engines, unsigned int nengine, int link)
+{
+   uint32_t ctx[nengine];
+
+   fcntl(link, F_SETFL, fcntl(fd, F_GETFL) | O_NONBLOCK);
+
+   for (unsigned int n = 0; n < nengine; n++)
+   ctx[n] = gem_context_create(fd);
+
+   do {
+   igt_spin_t *spin;
+   struct {
+   uint32_t engine;
+   uint32_t min;
+   uint32_t max;
+   } req;
+
+   while (read(link, , sizeof(req)) > 0) {
+   if ((req.engine | req.min | req.max) == 0)
+   goto out;
+
+   igt_assert(req.engine < nengine);
+   set_freq(fd, ctx[req.engine], req.min, req.max);
+   }
+
+   /* Create a 20% load using busy spinners */
+   spin = __igt_spin_batch_new(fd, ctx[0], engines[0], 0);
+   for (unsigned int n = 1; n &l

Re: [Intel-gfx] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Antonio Argenziano]

On 07/03/18 17:18, Chris Wilson wrote:

Quoting Antonio Argenziano (2018-03-08 00:55:47)



On 07/03/18 14:49, Chris Wilson wrote:

+static void single(int fd, const struct intel_execution_engine *e)
+{
+ const unsigned int engine = e->exec_id | e->flags;
+ uint32_t ctx = gem_context_create(fd);
+ uint32_t min, max;
+ double measured;
+ igt_spin_t *spin;
+ int pmu;
+
+ get_freq(fd, ctx, , );
+ igt_info("Min freq: %dMHz; Max freq: %dMHz\n", min, max);
+
+ pmu = perf_i915_open(I915_PMU_REQUESTED_FREQUENCY);
+ igt_require(pmu >= 0);
+
+ gem_quiescent_gpu(fd);
+ measured = measure_frequency(pmu, 1);
+ igt_info("Initial (idle) freq: %.1fMHz\n",measured);
+ igt_require(measured >= min - 50 && measured <= min + 50);
+
+ for (uint32_t freq = min + 50; freq <= max; freq += 100) {
+ set_freq(fd, ctx, freq, freq);
+
+ gem_quiescent_gpu(fd);
+ spin = __igt_spin_batch_new(fd, ctx, engine, 0);
+ usleep(1);
+
+ measured = measure_frequency(pmu, 5);
+ igt_debugfs_dump(fd, "i915_rps_boost_info");
+
+ igt_spin_batch_free(fd, spin);
+ igt_info("%s(single): Measured %.1fMHz, expected %dMhz\n",
+  e->name, measured, freq);
+ igt_assert(measured > freq - 100 && measured < freq + 100);
+ }
+ gem_quiescent_gpu(fd);


Check frequency has gone back to ~min.


It's not that interesting a test (covered already by pmu) as we
essentially lie anyway over idle.


Agreed.




I would suggest to split here into two sub-tests.


+ spin = __igt_spin_batch_new(fd, ctx, engine, 0);
+ for (uint32_t freq = min + 50; freq <= max; freq += 100) {
+ igt_spin_t *kick;
+
+ set_freq(fd, ctx, freq, freq);
+
+ /*
+  * When requesting a new frequency on the currently
+  * executing context, it does not take effect until the
+  * next context switch. In this case, we trigger a lite
+  * restore.


Is this enforced by the ABI?


Enforced? No. The comment is precisely because it's not checked on
calling whether the context is currently on the HW and trying hard to be
sure that no one expects us to do that check. i.e. that set_freq()
doesn't change frequency itself, but doesn't rule it out either as it
may appear to have that effect due to many external factors.


That is what I thought :).

I see that you had a new version with more tests, I'll have a look at that.

Thanks,
Antonio


-Chris


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

Re: [Intel-gfx] [PATCH igt] igt: Add gem_ctx_freq to exercise requesting freq on a ctx

[Antonio Argenziano]

On 07/03/18 14:49, Chris Wilson wrote:

Exercise some new API that allows applications to request that
individual contexts are executed within a desired frequency range.

Signed-off-by: Chris Wilson 
---
  tests/Makefile.am  |   2 +-
  tests/Makefile.sources |   1 +
  tests/gem_ctx_freq.c   | 190 +
  tests/meson.build  |   1 +
  4 files changed, 193 insertions(+), 1 deletion(-)
  create mode 100644 tests/gem_ctx_freq.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index edd689a4..f42641f6 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -104,6 +104,7 @@ drm_import_export_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  drm_import_export_LDADD = $(LDADD) -lpthread
  gem_close_race_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  gem_close_race_LDADD = $(LDADD) -lpthread
+gem_ctx_freq_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
  gem_ctx_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  gem_ctx_thrash_LDADD = $(LDADD) -lpthread
  gem_exec_parallel_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
@@ -128,7 +129,6 @@ prime_self_import_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  prime_self_import_LDADD = $(LDADD) -lpthread
  gem_userptr_blits_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
  gem_userptr_blits_LDADD = $(LDADD) -lpthread
-perf_pmu_LDADD = $(LDADD) $(top_builddir)/lib/libigt_perf.la
  
  gem_wait_LDADD = $(LDADD) -lrt

  kms_flip_LDADD = $(LDADD) -lrt -lpthread
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 05cdc1ef..06e729ef 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -59,6 +59,7 @@ TESTS_progs = \
gem_ctx_bad_exec \
gem_ctx_create \
gem_ctx_exec \
+   gem_ctx_freq \
gem_ctx_isolation \
gem_ctx_param \
gem_ctx_shared \
diff --git a/tests/gem_ctx_freq.c b/tests/gem_ctx_freq.c
new file mode 100644
index ..a01ce01b
--- /dev/null
+++ b/tests/gem_ctx_freq.c
@@ -0,0 +1,190 @@
+/*
+ * Copyright © 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "igt.h"
+#include "igt_perf.h"
+
+#define LOCAL_CONTEXT_PARAM_FREQUENCY 8
+
+static void set_freq(int fd, uint32_t ctx, uint32_t min, uint32_t max)
+{
+   struct drm_i915_gem_context_param param = {
+   .ctx_id = ctx,
+   .param = LOCAL_CONTEXT_PARAM_FREQUENCY,
+   .value = (uint64_t)max << 32 | min,
+   };
+
+   gem_context_set_param(fd, );
+}
+
+static void get_freq(int fd, uint32_t ctx, uint32_t *min, uint32_t *max)
+{
+   struct drm_i915_gem_context_param param = {
+   .ctx_id = ctx,
+   .param = LOCAL_CONTEXT_PARAM_FREQUENCY,
+   };
+
+   gem_context_get_param(fd, );
+
+   *min = param.value & 0x;
+   *max = param.value >> 32;
+}
+
+static double measure_frequency(int pmu, int delay)
+{
+   uint64_t data[2];
+   uint64_t d_t, d_v;
+
+   igt_assert_eq(read(pmu, data, sizeof(data)), sizeof(data));
+   d_v = -data[0];
+   d_t = -data[1];
+
+   usleep(delay);
+
+   igt_assert_eq(read(pmu, data, sizeof(data)), sizeof(data));
+   d_v += data[0];
+   d_t += data[1];
+
+   return d_v * 1e9 / d_t;
+}
+
+static void single(int fd, const struct intel_execution_engine *e)
+{
+   const unsigned int engine = e->exec_id | e->flags;
+   uint32_t ctx = gem_context_create(fd);
+   uint32_t min, max;
+   double measured;
+   igt_spin_t *spin;
+   int pmu;
+
+   get_freq(fd, ctx, , );
+   igt_info("Min freq: %dMHz; Max freq: %dMHz\n", min, max);
+
+   pmu = perf_i915_open(I915_PMU_REQUESTED_FREQUENCY);
+   igt_require(pmu >= 0);
+
+   gem_quiescent_gpu(fd);
+   measured = 

Re: [Intel-gfx] [PATCH igt] igt/drv_hangman: Check that the error state does hold the expected state

[Antonio Argenziano]

On 05/03/18 02:09, Chris Wilson wrote:

Actually check the error state exists (!"No error state captured") and
that it contains the expected engine dump.

v2: Throw in some debug clues.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
---
  tests/drv_hangman.c | 12 
  1 file changed, 12 insertions(+)

diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
index 38cb20c3..fa7becf5 100644
--- a/tests/drv_hangman.c
+++ b/tests/drv_hangman.c
@@ -129,6 +129,14 @@ static void check_error_state(const char 
*expected_ring_name,
FILE *file = open_error();
char *line = NULL;
size_t line_size = 0;
+   bool found = false;
+
+   igt_debug("%s(expected ring name=%s, expected offset=%"PRIx64")\n",
+ __func__, expected_ring_name, expected_offset);
+   igt_debugfs_dump(device, "i915_error_state");
+
+   getline(, _size, file);
+   igt_assert(strcasecmp(line, "No error state captured"));
  
  	while (getline(, _size, file) > 0) {

char *dashes;
@@ -168,12 +176,16 @@ static void check_error_state(const char 
*expected_ring_name,
 4*i, batch[i]);
igt_assert(strstr(line, expected_line));
}
+
+   found = true;
break;
}
}
  
  	free(line);

fclose(file);
+
+   igt_assert(found);


Test changes look fine to me, failures on CI seems to be caused by the 
test not waiting for reset to happen only before we would have not 
caught it.


Reviwed-by: Antonio Argenziano <antonio.argenzi...@intel.com>


  }
  
  static void test_error_state_capture(unsigned ring_id,



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

Re: [Intel-gfx] [PATCH igt] Bump measure_ring_size() timer interval

[Antonio Argenziano]

On 05/03/18 03:03, Chris Wilson wrote:

It appears that waiting for a 100us period whereby we are unable to
submit another batch and proclaim the ring full, may have the false
positive where the scheduler intervenes and we are signalled twice
before having slept on ring space. Increasing the interval reduces the
likelihood of the scheduler stealing the cpu from us, but does not
eliminate it. Fortuitously it appears to be a rare false positive.

For the library routine, we can fork a RT process but that seems a bit
overkill!

References: https://bugs.freedesktop.org/show_bug.cgi?id=105343
Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Antonio Argenziano <antonio.argenzi...@intel.com>


Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt v2] igt/gem_ctx_switch: Exercise all engines at once

[Antonio Argenziano]

On 28/02/18 23:51, Chris Wilson wrote:

Just a small variant to apply a continuous context-switch load to all
engines.

v2: Adapt to for_each_physical_engine() and sane gem_context_create()

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Antonio Argenziano <antonio.argenzi...@intel.com>


LGTM.

Reviewed-by: Antonio Argenziano <antonio.argenzi...@intel.com>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx