* [Intel-gfx] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
@ 2020-02-11 0:37 Chris Wilson
2020-02-11 0:42 ` Andi Shyti
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2020-02-11 0:37 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
2020-02-11 0:37 [Intel-gfx] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names Chris Wilson
@ 2020-02-11 0:42 ` Andi Shyti
2020-02-11 10:37 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Andi Shyti @ 2020-02-11 0:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
2020-02-11 0:42 ` Andi Shyti
@ 2020-02-11 10:37 ` Chris Wilson
2020-02-11 13:00 ` [Intel-gfx] [igt-dev] " Tvrtko Ursulin
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2020-02-11 10:37 UTC (permalink / raw)
To: Andi Shyti; +Cc: igt-dev, 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
2020-02-11 10:37 ` Chris Wilson
@ 2020-02-11 13:00 ` Tvrtko Ursulin
2020-02-11 13:05 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Tvrtko Ursulin @ 2020-02-11 13:00 UTC (permalink / raw)
To: Chris Wilson, Andi Shyti; +Cc: igt-dev, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
2020-02-11 13:00 ` [Intel-gfx] [igt-dev] " Tvrtko Ursulin
@ 2020-02-11 13:05 ` Chris Wilson
2020-02-11 13:21 ` Tvrtko Ursulin
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2020-02-11 13:05 UTC (permalink / raw)
To: Andi Shyti, Tvrtko Ursulin; +Cc: igt-dev, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names
2020-02-11 13:05 ` Chris Wilson
@ 2020-02-11 13:21 ` Tvrtko Ursulin
0 siblings, 0 replies; 6+ messages in thread
From: Tvrtko Ursulin @ 2020-02-11 13:21 UTC (permalink / raw)
To: Chris Wilson, Andi Shyti; +Cc: igt-dev, intel-gfx
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-11 13:21 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 0:37 [Intel-gfx] [PATCH i-g-t] i915/gem_exec_nop: Keep a copy of the names Chris Wilson
2020-02-11 0:42 ` Andi Shyti
2020-02-11 10:37 ` Chris Wilson
2020-02-11 13:00 ` [Intel-gfx] [igt-dev] " Tvrtko Ursulin
2020-02-11 13:05 ` Chris Wilson
2020-02-11 13:21 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).