Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 01/21] scripts/trace.pl: Fix after intel_engine_notify removal
On 10/05/2019 13:33, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-05-08 13:10:38)
From: Tvrtko Ursulin
After the removal of engine global seqnos and the corresponding
intel_engine_notify tracepoints the script needs to be adjusted to cope
with the new state of things.
To keep working it switches over using the dma_fence:dma_fence_signaled:
tracepoint and keeps one extra internal map to connect the ctx-seqno pairs
with engines.
It also needs to key the completion events on the full engine/ctx/seqno
tokens, and adjust correspondingly the timeline sorting logic.
v2:
* Do not use late notifications (received after context complete) when
splitting up coalesced requests. They are now much more likely and can
not be used.
Signed-off-by: Tvrtko Ursulin
---
scripts/trace.pl | 82
1 file changed, 41 insertions(+), 41 deletions(-)
diff --git a/scripts/trace.pl b/scripts/trace.pl
index 18f9f3b18396..95dc3a645e8e 100755
--- a/scripts/trace.pl
+++ b/scripts/trace.pl
@@ -27,7 +27,8 @@ use warnings;
use 5.010;
my $gid = 0;
-my (%db, %queue, %submit, %notify, %rings, %ctxdb, %ringmap, %reqwait,
%ctxtimelines);
+my (%db, %queue, %submit, %notify, %rings, %ctxdb, %ringmap, %reqwait,
+%ctxtimelines, %ctxengines);
my @freqs;
So what's ctxengines? Or rings for that matter?
rings go back to the beginnings of the tool when I think the
visualizaiton library needed an unique integer value for every timeline
(so engine). And there is a ringmap from this id back to our engine
name. Perhaps this would be clearer if reversed, but I am not sure how
much churn would that be without actually doing it. Renaming rings to
engines would also make sense.
I take it ctxengines is really the last engine which we saw this context
execute on?
Correct.
I guess there is a problem if dma_fence_signaled is delayed past another
request_in. Hm but I also have a die if engine is different.. that
cannot be right, but why it didn't fail.. I need to double check this.
my $max_items = 3000;
@@ -66,7 +67,7 @@ Notes:
i915:i915_request_submit, \
i915:i915_request_in, \
i915:i915_request_out, \
- i915:intel_engine_notify, \
+ dma_fence:dma_fence_signaled, \
i915:i915_request_wait_begin, \
i915:i915_request_wait_end \
[command-to-be-profiled]
@@ -161,7 +162,7 @@ sub arg_trace
'i915:i915_request_submit',
'i915:i915_request_in',
'i915:i915_request_out',
- 'i915:intel_engine_notify',
+ 'dma_fence:dma_fence_signaled',
'i915:i915_request_wait_begin',
'i915:i915_request_wait_end' );
@@ -312,13 +313,6 @@ sub db_key
return $ring . '/' . $ctx . '/' . $seqno;
}
-sub global_key
-{
- my ($ring, $seqno) = @_;
-
- return $ring . '/' . $seqno;
-}
-
sub sanitize_ctx
{
my ($ctx, $ring) = @_;
@@ -419,6 +413,8 @@ while (<>) {
$req{'ring'} = $ring;
$req{'seqno'} = $seqno;
$req{'ctx'} = $ctx;
+ die if exists $ctxengines{$ctx} and $ctxengines{$ctx} ne $ring;
+ $ctxengines{$ctx} = $ring;
$ctxtimelines{$ctx . '/' . $ring} = 1;
$req{'name'} = $ctx . '/' . $seqno;
$req{'global'} = $tp{'global'};
@@ -429,16 +425,29 @@ while (<>) {
$ringmap{$rings{$ring}} = $ring;
$db{$key} = \%req;
} elsif ($tp_name eq 'i915:i915_request_out:') {
- my $gkey = global_key($ring, $tp{'global'});
+ my $gkey;
+
# Must be paired with a previous i915_request_in
+ die unless exists $ctxengines{$ctx};
I'd suggest next unless, because there's always a change the capture is
started part way though someone's workload.
That would need much more work.
+ $gkey = db_key($ctxengines{$ctx}, $ctx, $seqno);
+
+ if ($tp{'completed?'}) {
+ die unless exists $db{$key};
+ die unless exists $db{$key}->{'start'};
+ die if exists $db{$key}->{'end'};
+
+ $db{$key}->{'end'} = $time;
+ $db{$key}->{'notify'} = $notify{$gkey}
+ if exists $notify{$gkey};
Hmm. With preempt-to-busy, a request can complete when we are no longer
tracking it (it completes before we preempt it).
They will still get the schedule-out tracepoint, but marked as
incomplete, and there will be a signaled tp later before we try and
resubmit.
This sounds like the request would disappear from the
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 01/21] scripts/trace.pl: Fix after intel_engine_notify removal
Quoting Tvrtko Ursulin (2019-05-08 13:10:38)
> From: Tvrtko Ursulin
>
> After the removal of engine global seqnos and the corresponding
> intel_engine_notify tracepoints the script needs to be adjusted to cope
> with the new state of things.
>
> To keep working it switches over using the dma_fence:dma_fence_signaled:
> tracepoint and keeps one extra internal map to connect the ctx-seqno pairs
> with engines.
>
> It also needs to key the completion events on the full engine/ctx/seqno
> tokens, and adjust correspondingly the timeline sorting logic.
>
> v2:
> * Do not use late notifications (received after context complete) when
>splitting up coalesced requests. They are now much more likely and can
>not be used.
>
> Signed-off-by: Tvrtko Ursulin
> ---
> scripts/trace.pl | 82
> 1 file changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/scripts/trace.pl b/scripts/trace.pl
> index 18f9f3b18396..95dc3a645e8e 100755
> --- a/scripts/trace.pl
> +++ b/scripts/trace.pl
> @@ -27,7 +27,8 @@ use warnings;
> use 5.010;
>
> my $gid = 0;
> -my (%db, %queue, %submit, %notify, %rings, %ctxdb, %ringmap, %reqwait,
> %ctxtimelines);
> +my (%db, %queue, %submit, %notify, %rings, %ctxdb, %ringmap, %reqwait,
> +%ctxtimelines, %ctxengines);
> my @freqs;
So what's ctxengines? Or rings for that matter?
I take it ctxengines is really the last engine which we saw this context
execute on?
>
> my $max_items = 3000;
> @@ -66,7 +67,7 @@ Notes:
>i915:i915_request_submit, \
>i915:i915_request_in, \
>i915:i915_request_out, \
> - i915:intel_engine_notify, \
> + dma_fence:dma_fence_signaled, \
>i915:i915_request_wait_begin, \
>i915:i915_request_wait_end \
>[command-to-be-profiled]
> @@ -161,7 +162,7 @@ sub arg_trace
>'i915:i915_request_submit',
>'i915:i915_request_in',
>'i915:i915_request_out',
> - 'i915:intel_engine_notify',
> + 'dma_fence:dma_fence_signaled',
>'i915:i915_request_wait_begin',
>'i915:i915_request_wait_end' );
>
> @@ -312,13 +313,6 @@ sub db_key
> return $ring . '/' . $ctx . '/' . $seqno;
> }
>
> -sub global_key
> -{
> - my ($ring, $seqno) = @_;
> -
> - return $ring . '/' . $seqno;
> -}
> -
> sub sanitize_ctx
> {
> my ($ctx, $ring) = @_;
> @@ -419,6 +413,8 @@ while (<>) {
> $req{'ring'} = $ring;
> $req{'seqno'} = $seqno;
> $req{'ctx'} = $ctx;
> + die if exists $ctxengines{$ctx} and $ctxengines{$ctx} ne
> $ring;
> + $ctxengines{$ctx} = $ring;
> $ctxtimelines{$ctx . '/' . $ring} = 1;
> $req{'name'} = $ctx . '/' . $seqno;
> $req{'global'} = $tp{'global'};
> @@ -429,16 +425,29 @@ while (<>) {
> $ringmap{$rings{$ring}} = $ring;
> $db{$key} = \%req;
> } elsif ($tp_name eq 'i915:i915_request_out:') {
> - my $gkey = global_key($ring, $tp{'global'});
> + my $gkey;
> +
# Must be paired with a previous i915_request_in
> + die unless exists $ctxengines{$ctx};
I'd suggest next unless, because there's always a change the capture is
started part way though someone's workload.
> + $gkey = db_key($ctxengines{$ctx}, $ctx, $seqno);
> +
> + if ($tp{'completed?'}) {
> + die unless exists $db{$key};
> + die unless exists $db{$key}->{'start'};
> + die if exists $db{$key}->{'end'};
> +
> + $db{$key}->{'end'} = $time;
> + $db{$key}->{'notify'} = $notify{$gkey}
> + if exists $notify{$gkey};
Hmm. With preempt-to-busy, a request can complete when we are no longer
tracking it (it completes before we preempt it).
They will still get the schedule-out tracepoint, but marked as
incomplete, and there will be a signaled tp later before we try and
resubmit.
> + } else {
> + delete $db{$key};
> + }
> + } elsif ($tp_name eq 'dma_fence:dma_fence_signaled:') {
> + my $gkey;
>
> - die unless exists $db{$key};
> - die unless exists $db{$key}->{'start'};
> - die if exists $db{$key}->{'end'};
> + die unless exists $ctxengines{$tp{'context'}};
>
> - $db{$key}->{'end'} = $time;
> - $db{$key}->{'notify'} = $notify{$gkey} if exists
> $notify{$gkey};
> - } elsif ($tp_name eq
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 01/21] scripts/trace.pl: Fix after intel_engine_notify removal
On 08/05/2019 13:17, Chris Wilson wrote: Quoting Tvrtko Ursulin (2019-05-08 13:10:38) From: Tvrtko Ursulin After the removal of engine global seqnos and the corresponding intel_engine_notify tracepoints the script needs to be adjusted to cope with the new state of things. To keep working it switches over using the dma_fence:dma_fence_signaled: tracepoint and keeps one extra internal map to connect the ctx-seqno pairs with engines. Is the map suitable for the planned (by me) s/i915_request_wait_begin/dma_fence_wait_begin/ I guess it should be. I think it would be workable. One complication would be that engine is not guaranteed to be known ahead of the wait, like it is ahead of the signal. But since ctx.seqno is unique it can be tracked and added later I think. 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 i-g-t 01/21] scripts/trace.pl: Fix after intel_engine_notify removal
Quoting Tvrtko Ursulin (2019-05-08 13:10:38) > From: Tvrtko Ursulin > > After the removal of engine global seqnos and the corresponding > intel_engine_notify tracepoints the script needs to be adjusted to cope > with the new state of things. > > To keep working it switches over using the dma_fence:dma_fence_signaled: > tracepoint and keeps one extra internal map to connect the ctx-seqno pairs > with engines. Is the map suitable for the planned (by me) s/i915_request_wait_begin/dma_fence_wait_begin/ I guess it should be. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
