| Message ID | 20180808113137.3747-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] igt/gem_eio: Preserve batch between reset-stress iterations | expand |
On 08/08/2018 12:31, Chris Wilson wrote: > We can keep the original batch around and avoid recreating it between > reset iterations to focus on the impact of resets. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > --- > tests/gem_eio.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/tests/gem_eio.c b/tests/gem_eio.c > index de161332d..5250a414c 100644 > --- a/tests/gem_eio.c > +++ b/tests/gem_eio.c > @@ -650,35 +650,38 @@ static void reset_stress(int fd, > uint32_t ctx0, unsigned int engine, > unsigned int flags) > { > + const uint32_t bbe = MI_BATCH_BUFFER_END; > + struct drm_i915_gem_exec_object2 obj = { > + .handle = gem_create(fd, 4096) > + }; > + struct drm_i915_gem_execbuffer2 execbuf = { > + .buffers_ptr = to_user_pointer(&obj), > + .buffer_count = 1, > + .flags = engine, > + }; > + gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); > + > igt_until_timeout(5) { > - struct drm_i915_gem_execbuffer2 execbuf = { }; > - struct drm_i915_gem_exec_object2 obj = { }; > - uint32_t bbe = MI_BATCH_BUFFER_END; > + uint32_t ctx = context_create_safe(fd); There is still this per loop... > 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. > */ > hang = spin_sync(fd, ctx0, engine); ... and a ton of operations in this one, so I wonder why bother with one batch? > > - obj.handle = gem_create(fd, 4096); > - gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); > + execbuf.rsvd1 = ctx; > + for (i = 0; i < 10; i++) > + gem_execbuf(fd, &execbuf); > > - execbuf.buffers_ptr = to_user_pointer(&obj); > - execbuf.buffer_count = 1; > execbuf.rsvd1 = ctx0; > - execbuf.flags = engine; > - > for (i = 0; i < 10; i++) > gem_execbuf(fd, &execbuf); > > @@ -706,8 +709,9 @@ static void reset_stress(int fd, > gem_sync(fd, obj.handle); > igt_spin_batch_free(fd, hang); > gem_context_destroy(fd, ctx); > - gem_close(fd, obj.handle); > } > + > + gem_close(fd, obj.handle); > } > > /* > But anyway - no technical complaints: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-08-08 13:38:53) > > On 08/08/2018 12:31, Chris Wilson wrote: > > We can keep the original batch around and avoid recreating it between > > reset iterations to focus on the impact of resets. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> > > --- > > tests/gem_eio.c | 30 +++++++++++++++++------------- > > 1 file changed, 17 insertions(+), 13 deletions(-) > > > > diff --git a/tests/gem_eio.c b/tests/gem_eio.c > > index de161332d..5250a414c 100644 > > --- a/tests/gem_eio.c > > +++ b/tests/gem_eio.c > > @@ -650,35 +650,38 @@ static void reset_stress(int fd, > > uint32_t ctx0, unsigned int engine, > > unsigned int flags) > > { > > + const uint32_t bbe = MI_BATCH_BUFFER_END; > > + struct drm_i915_gem_exec_object2 obj = { > > + .handle = gem_create(fd, 4096) > > + }; > > + struct drm_i915_gem_execbuffer2 execbuf = { > > + .buffers_ptr = to_user_pointer(&obj), > > + .buffer_count = 1, > > + .flags = engine, > > + }; > > + gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); > > + > > igt_until_timeout(5) { > > - struct drm_i915_gem_execbuffer2 execbuf = { }; > > - struct drm_i915_gem_exec_object2 obj = { }; > > - uint32_t bbe = MI_BATCH_BUFFER_END; > > + uint32_t ctx = context_create_safe(fd); > > There is still this per loop... I thought that was intentional :) It felt like the spirit of the test to try and mix up the contexts as much as possible; one constant, one fresh. > > 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. > > */ > > hang = spin_sync(fd, ctx0, engine); > > ... and a ton of operations in this one, so I wonder why bother with one > batch? Because I don't have spin_sync() in my pattern recognition matrix yet. One excuse is that it doesn't have any create verb in its name, so easy to forget its hidden costs. -Chris
On 08/08/2018 13:47, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-08-08 13:38:53) >> >> On 08/08/2018 12:31, Chris Wilson wrote: >>> We can keep the original batch around and avoid recreating it between >>> reset iterations to focus on the impact of resets. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> --- >>> tests/gem_eio.c | 30 +++++++++++++++++------------- >>> 1 file changed, 17 insertions(+), 13 deletions(-) >>> >>> diff --git a/tests/gem_eio.c b/tests/gem_eio.c >>> index de161332d..5250a414c 100644 >>> --- a/tests/gem_eio.c >>> +++ b/tests/gem_eio.c >>> @@ -650,35 +650,38 @@ static void reset_stress(int fd, >>> uint32_t ctx0, unsigned int engine, >>> unsigned int flags) >>> { >>> + const uint32_t bbe = MI_BATCH_BUFFER_END; >>> + struct drm_i915_gem_exec_object2 obj = { >>> + .handle = gem_create(fd, 4096) >>> + }; >>> + struct drm_i915_gem_execbuffer2 execbuf = { >>> + .buffers_ptr = to_user_pointer(&obj), >>> + .buffer_count = 1, >>> + .flags = engine, >>> + }; >>> + gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); >>> + >>> igt_until_timeout(5) { >>> - struct drm_i915_gem_execbuffer2 execbuf = { }; >>> - struct drm_i915_gem_exec_object2 obj = { }; >>> - uint32_t bbe = MI_BATCH_BUFFER_END; >>> + uint32_t ctx = context_create_safe(fd); >> >> There is still this per loop... > > I thought that was intentional :) > > It felt like the spirit of the test to try and mix up the contexts as > much as possible; one constant, one fresh. Yes definitely intentional (required), I was just puzzled why you are concerned with removing one gem_create when we have more ioctls in the loop anyway. > >>> 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. >>> */ >>> hang = spin_sync(fd, ctx0, engine); >> >> ... and a ton of operations in this one, so I wonder why bother with one >> batch? > > Because I don't have spin_sync() in my pattern recognition matrix yet. > One excuse is that it doesn't have any create verb in its name, so easy > to forget its hidden costs. Okay. :) I blame the poor name on it being local. :) Regards, Tvrtko
Quoting Tvrtko Ursulin (2018-08-08 13:57:56) > > On 08/08/2018 13:47, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2018-08-08 13:38:53) > >> > >> On 08/08/2018 12:31, Chris Wilson wrote: > >>> igt_until_timeout(5) { > >>> - struct drm_i915_gem_execbuffer2 execbuf = { }; > >>> - struct drm_i915_gem_exec_object2 obj = { }; > >>> - uint32_t bbe = MI_BATCH_BUFFER_END; > >>> + uint32_t ctx = context_create_safe(fd); > >> > >> There is still this per loop... > > > > I thought that was intentional :) > > > > It felt like the spirit of the test to try and mix up the contexts as > > much as possible; one constant, one fresh. > > Yes definitely intentional (required), I was just puzzled why you are > concerned with removing one gem_create when we have more ioctls in the > loop anyway. Low hanging fruit. Besides, I would cache the context objects if someone would just turn a blind eye. -Chris
diff --git a/tests/gem_eio.c b/tests/gem_eio.c index de161332d..5250a414c 100644 --- a/tests/gem_eio.c +++ b/tests/gem_eio.c @@ -650,35 +650,38 @@ static void reset_stress(int fd, uint32_t ctx0, unsigned int engine, unsigned int flags) { + const uint32_t bbe = MI_BATCH_BUFFER_END; + struct drm_i915_gem_exec_object2 obj = { + .handle = gem_create(fd, 4096) + }; + struct drm_i915_gem_execbuffer2 execbuf = { + .buffers_ptr = to_user_pointer(&obj), + .buffer_count = 1, + .flags = engine, + }; + gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); + igt_until_timeout(5) { - struct drm_i915_gem_execbuffer2 execbuf = { }; - struct drm_i915_gem_exec_object2 obj = { }; - uint32_t bbe = MI_BATCH_BUFFER_END; + uint32_t ctx = context_create_safe(fd); 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. */ hang = spin_sync(fd, ctx0, engine); - obj.handle = gem_create(fd, 4096); - gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe)); + execbuf.rsvd1 = ctx; + for (i = 0; i < 10; i++) + gem_execbuf(fd, &execbuf); - execbuf.buffers_ptr = to_user_pointer(&obj); - execbuf.buffer_count = 1; execbuf.rsvd1 = ctx0; - execbuf.flags = engine; - for (i = 0; i < 10; i++) gem_execbuf(fd, &execbuf); @@ -706,8 +709,9 @@ static void reset_stress(int fd, gem_sync(fd, obj.handle); igt_spin_batch_free(fd, hang); gem_context_destroy(fd, ctx); - gem_close(fd, obj.handle); } + + gem_close(fd, obj.handle); } /*
We can keep the original batch around and avoid recreating it between reset iterations to focus on the impact of resets. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> --- tests/gem_eio.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)