| Message ID | 20200211003742.863630-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] i915/gem_exec_nop: Keep a copy of the names | expand |
Hi Chris, On Tue, Feb 11, 2020 at 12:37:42AM +0000, Chris Wilson wrote: > The engine names are now stored inside the iterator and not as static > strings. If we wish to use them later, we need to make a copy. But we are not using them later. Your patch just copies and frees an array. Is there a follow-up? Andi > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > tests/i915/gem_exec_nop.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tests/i915/gem_exec_nop.c b/tests/i915/gem_exec_nop.c > index ed9568e5a..fc7f11827 100644 > --- a/tests/i915/gem_exec_nop.c > +++ b/tests/i915/gem_exec_nop.c > @@ -436,7 +436,7 @@ static void parallel(int fd, uint32_t handle, int timeout) > struct drm_i915_gem_execbuffer2 execbuf; > struct drm_i915_gem_exec_object2 obj; > unsigned engines[16]; > - const char *names[16]; > + char *names[16]; > unsigned nengine; > unsigned long count; > double time, sum; > @@ -445,7 +445,7 @@ static void parallel(int fd, uint32_t handle, int timeout) > nengine = 0; > __for_each_physical_engine(fd, e) { > engines[nengine] = e->flags; > - names[nengine++] = e->name; > + names[nengine++] = strdup(e->name); > > time = nop_on_ring(fd, handle, e, 1, &count) / count; > sum += time; > @@ -485,10 +485,11 @@ static void parallel(int fd, uint32_t handle, int timeout) > time = elapsed(&start, &now) / count; > igt_info("%s: %ld cycles, %.3fus\n", names[child], count, 1e6*time); > } > + while (nengine--) > + free(names[nengine]); > > igt_waitchildren(); > igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0); > - > } > > static void series(int fd, uint32_t handle, int timeout) > -- > 2.25.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Andi Shyti (2020-02-11 00:42:55) > Hi Chris, > > On Tue, Feb 11, 2020 at 12:37:42AM +0000, Chris Wilson wrote: > > The engine names are now stored inside the iterator and not as static > > strings. If we wish to use them later, we need to make a copy. > > But we are not using them later. Your patch just copies and frees > an array. They are used inside an igt_info() in the child processes. Output at the moment is quite strange. -Chris
On 11/02/2020 10:37, Chris Wilson wrote: > Quoting Andi Shyti (2020-02-11 00:42:55) >> Hi Chris, >> >> On Tue, Feb 11, 2020 at 12:37:42AM +0000, Chris Wilson wrote: >>> The engine names are now stored inside the iterator and not as static >>> strings. If we wish to use them later, we need to make a copy. >> >> But we are not using them later. Your patch just copies and frees >> an array. > > They are used inside an igt_info() in the child processes. Output at the > moment is quite strange. I also don't get this - child has a copy of everything how can it not work? Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-02-11 13:00:19) > > On 11/02/2020 10:37, Chris Wilson wrote: > > Quoting Andi Shyti (2020-02-11 00:42:55) > >> Hi Chris, > >> > >> On Tue, Feb 11, 2020 at 12:37:42AM +0000, Chris Wilson wrote: > >>> The engine names are now stored inside the iterator and not as static > >>> strings. If we wish to use them later, we need to make a copy. > >> > >> But we are not using them later. Your patch just copies and frees > >> an array. > > > > They are used inside an igt_info() in the child processes. Output at the > > moment is quite strange. > > I also don't get this - child has a copy of everything how can it not work? A copy of what? The intel_engine_data is scoped to the for_each_engine loop. We're leaking pointers to locations on stack, so they get clobbered and we print garbage in %s. -Chris
On 11/02/2020 13:05, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-02-11 13:00:19) >> >> On 11/02/2020 10:37, Chris Wilson wrote: >>> Quoting Andi Shyti (2020-02-11 00:42:55) >>>> Hi Chris, >>>> >>>> On Tue, Feb 11, 2020 at 12:37:42AM +0000, Chris Wilson wrote: >>>>> The engine names are now stored inside the iterator and not as static >>>>> strings. If we wish to use them later, we need to make a copy. >>>> >>>> But we are not using them later. Your patch just copies and frees >>>> an array. >>> >>> They are used inside an igt_info() in the child processes. Output at the >>> moment is quite strange. >> >> I also don't get this - child has a copy of everything how can it not work? > > A copy of what? The intel_engine_data is scoped to the for_each_engine > loop. We're leaking pointers to locations on stack, so they get > clobbered and we print garbage in %s. I misread the the patch while glancing it as forking being done inside the for_engine_loop. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
diff --git a/tests/i915/gem_exec_nop.c b/tests/i915/gem_exec_nop.c index ed9568e5a..fc7f11827 100644 --- a/tests/i915/gem_exec_nop.c +++ b/tests/i915/gem_exec_nop.c @@ -436,7 +436,7 @@ static void parallel(int fd, uint32_t handle, int timeout) struct drm_i915_gem_execbuffer2 execbuf; struct drm_i915_gem_exec_object2 obj; unsigned engines[16]; - const char *names[16]; + char *names[16]; unsigned nengine; unsigned long count; double time, sum; @@ -445,7 +445,7 @@ static void parallel(int fd, uint32_t handle, int timeout) nengine = 0; __for_each_physical_engine(fd, e) { engines[nengine] = e->flags; - names[nengine++] = e->name; + names[nengine++] = strdup(e->name); time = nop_on_ring(fd, handle, e, 1, &count) / count; sum += time; @@ -485,10 +485,11 @@ static void parallel(int fd, uint32_t handle, int timeout) time = elapsed(&start, &now) / count; igt_info("%s: %ld cycles, %.3fus\n", names[child], count, 1e6*time); } + while (nengine--) + free(names[nengine]); igt_waitchildren(); igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0); - } static void series(int fd, uint32_t handle, int timeout)
The engine names are now stored inside the iterator and not as static strings. If we wish to use them later, we need to make a copy. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- tests/i915/gem_exec_nop.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)