Re: [Intel-gfx] [igt-dev] [PATCH igt] igt/gem_ctx_isolation: Reset a scratch context

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-03-27 16:41:14)
> 
> [resend for typo in cc]

The small advantage in redundancy, I guess.
-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] igt/gem_ctx_isolation: Reset a scratch context

[Tvrtko Ursulin]

[resend for typo in cc]

On 27/03/2018 14:59, Chris Wilson wrote:

Quoting Chris Wilson (2018-03-27 14:52:20)

Quoting Chris Wilson (2018-03-27 14:48:43)

If we inject a reset into the target context, there is a risk that the
register state is never saved back to memory. The exact interaction
between reset, the context image and the precise timing of our execution
are not well defined. Since we cannot ensure that the context image
remains valid, force a context switch prior to the reset.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105270
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105457
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105545
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
  tests/gem_ctx_isolation.c | 28 +++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tests/gem_ctx_isolation.c b/tests/gem_ctx_isolation.c
index d8109aa0..4968e367 100644
--- a/tests/gem_ctx_isolation.c
+++ b/tests/gem_ctx_isolation.c
@@ -522,6 +522,32 @@ static void isolation(int fd,
  #define S4 (4 << 8)
  #define SLEEP_MASK (0xf << 8)
  
+static void inject_reset_context(int fd, unsigned int engine)

+{
+   igt_spin_t *spin;
+   uint32_t ctx;
+
+   /*
+* Force a context switch before triggering the reset, or else
+* we risk corrupting the target context and we can't blame the
+* HW for screwing up if the context was already broken.
+*/
+
+   ctx = gem_context_create(fd);
+   if (gem_can_store_dword(fd, engine)) {
+   spin = __igt_spin_batch_new_poll(fd, ctx, engine);
+   igt_spin_busywait_until_running(spin);
+   } else {
+   spin = __igt_spin_batch_new(fd, ctx, engine, 0);
+   usleep(1000); /* better than nothing */
+   }


Tvrtko, maybe we want igt_spin_batch_run()? Not sure though, so far we
have an example where you need precise control and a couple of examples
where we just want a running spinner.


I wasn't sure it is in good taste to put a thing with that usleep in 
lib/, since it incurs a delay to unsuspecting callers. And I couldn't 
decide how to handle it better - skip from lib/? Not so great. Return 
value and make callers handle it - even more cumbersome.


It only affects old gens, but do we want tests there suddenly take much 
longer because someone spotted a cool new API and went to use it?


But it is a third user now so not great to copy paste it all around. I 
don't know. Make the lib functions skip where unsupported and add double 
underscore prefix flavour which sleeps where not supported?



Also this whole function might want to make its way into lib/i915 as the
basis of all hang/reset injection. Some improvement required to set
context parameters (e.g. disabling error state capturing) and skipping
if no reset is available or disabled (although that's the job for the
fixture, so may not be required for the library function). I'm sure
someone will catch me reusing this chunk over and over again...


Sounds OK to me.

Regards,

Tvrtko


___
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/gem_ctx_isolation: Reset a scratch context

[Tvrtko Ursulin]

[resend for typo in cc]

On 27/03/2018 14:48, Chris Wilson wrote:

If we inject a reset into the target context, there is a risk that the
register state is never saved back to memory. The exact interaction
between reset, the context image and the precise timing of our execution
are not well defined. Since we cannot ensure that the context image
remains valid, force a context switch prior to the reset.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105270
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105457
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105545
Signed-off-by: Chris Wilson 
Cc: Tvrtko Ursulin 
Cc: Joonas Lahtinen 
---
  tests/gem_ctx_isolation.c | 28 +++-
  1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tests/gem_ctx_isolation.c b/tests/gem_ctx_isolation.c
index d8109aa0..4968e367 100644
--- a/tests/gem_ctx_isolation.c
+++ b/tests/gem_ctx_isolation.c
@@ -522,6 +522,32 @@ static void isolation(int fd,
  #define S4 (4 << 8)
  #define SLEEP_MASK (0xf << 8)
  
+static void inject_reset_context(int fd, unsigned int engine)

+{
+   igt_spin_t *spin;
+   uint32_t ctx;
+
+   /*
+* Force a context switch before triggering the reset, or else
+* we risk corrupting the target context and we can't blame the
+* HW for screwing up if the context was already broken.
+*/
+
+   ctx = gem_context_create(fd);
+   if (gem_can_store_dword(fd, engine)) {
+   spin = __igt_spin_batch_new_poll(fd, ctx, engine);
+   igt_spin_busywait_until_running(spin);
+   } else {
+   spin = __igt_spin_batch_new(fd, ctx, engine, 0);
+   usleep(1000); /* better than nothing */
+   }
+
+   igt_force_gpu_reset(fd);
+
+   igt_spin_batch_free(fd, spin);
+   gem_context_destroy(fd, ctx);
+}
+
  static void preservation(int fd,
 const struct intel_execution_engine2 *e,
 unsigned int flags)
@@ -558,7 +584,7 @@ static void preservation(int fd,
igt_spin_batch_free(fd, spin);
  
  	if (flags & RESET)

-   igt_force_gpu_reset(fd);
+   inject_reset_context(fd, engine);
  
  	switch (flags & SLEEP_MASK) {

case NOSLEEP:



Reviewed-by: Tvrtko Ursulin 

Regards,

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