diff mbox

drm/i915: Make global seqno known in i915_gem_request_execute tracepoint

Message ID 20180220104742.565-1-tvrtko.ursulin@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tvrtko Ursulin Feb. 20, 2018, 10:47 a.m. UTC 
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Commit fe49789fab97 ("drm/i915: Deconstruct execute fence") re-arranged
the code and moved the i915_gem_request_execute tracepoint to before the
global seqno is assigned to the request.

We need to move the tracepoint a bit later so this information is once
again available.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Fixes: fe49789fab97 ("drm/i915: Deconstruct execute fence")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/i915/i915_gem_request.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Chris Wilson Feb. 20, 2018, 10:57 a.m. UTC | #1 
Quoting Tvrtko Ursulin (2018-02-20 10:47:42)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Commit fe49789fab97 ("drm/i915: Deconstruct execute fence") re-arranged
> the code and moved the i915_gem_request_execute tracepoint to before the
> global seqno is assigned to the request.
> 
> We need to move the tracepoint a bit later so this information is once
> again available.

Ok, iirc my thinking was just to have the tracing early to show the
entry to i915_request_submit. If you are happy with having it at the
end with the details filled in, so am I :)
 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Fixes: fe49789fab97 ("drm/i915: Deconstruct execute fence")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Tvrtko Ursulin Feb. 20, 2018, 11:14 a.m. UTC | #2 
On 20/02/2018 10:57, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-20 10:47:42)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Commit fe49789fab97 ("drm/i915: Deconstruct execute fence") re-arranged
>> the code and moved the i915_gem_request_execute tracepoint to before the
>> global seqno is assigned to the request.
>>
>> We need to move the tracepoint a bit later so this information is once
>> again available.
> 
> Ok, iirc my thinking was just to have the tracing early to show the
> entry to i915_request_submit. If you are happy with having it at the
> end with the details filled in, so am I :)

We have trace_i915_gem_request_submit in the fence->submit_notify so I 
think it is OK.

Would it be better to move it to just after req->global_seqno is set, so 
before enable_signalling (it would be under the lock)?

I am just thinking that the spirit of the tracepoint was that global 
seqno should be known.

Regards,

Tvrtko

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Fixes: fe49789fab97 ("drm/i915: Deconstruct execute fence")
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Chris Wilson Feb. 20, 2018, 11:20 a.m. UTC | #3 
Quoting Tvrtko Ursulin (2018-02-20 11:14:38)
> 
> On 20/02/2018 10:57, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-02-20 10:47:42)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> Commit fe49789fab97 ("drm/i915: Deconstruct execute fence") re-arranged
> >> the code and moved the i915_gem_request_execute tracepoint to before the
> >> global seqno is assigned to the request.
> >>
> >> We need to move the tracepoint a bit later so this information is once
> >> again available.
> > 
> > Ok, iirc my thinking was just to have the tracing early to show the
> > entry to i915_request_submit. If you are happy with having it at the
> > end with the details filled in, so am I :)
> 
> We have trace_i915_gem_request_submit in the fence->submit_notify so I 
> think it is OK.
> 
> Would it be better to move it to just after req->global_seqno is set, so 
> before enable_signalling (it would be under the lock)?

Personally, either the beginning or the end of this function make the
most sense to me (in the grand scheme of request flow). Or at least it
keeps the tracepoint out of the way :) There's some merit I feel to
avoid having it appear coupled to the assignment of global_seqno.
-Chris
Tvrtko Ursulin Feb. 21, 2018, 2:12 p.m. UTC | #4 
On 20/02/2018 11:18, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: Make global seqno known in i915_gem_request_execute tracepoint
> URL   : https://patchwork.freedesktop.org/series/38578/
> State : success
> 
> == Summary ==
> 
> Series 38578v1 drm/i915: Make global seqno known in i915_gem_request_execute tracepoint
> https://patchwork.freedesktop.org/api/1.0/series/38578/revisions/1/mbox/
> 
> Test debugfs_test:
>          Subgroup read_all_entries:
>                  incomplete -> PASS       (fi-snb-2520m) fdo#103713 +1
> Test kms_chamelium:
>          Subgroup dp-crc-fast:
>                  dmesg-fail -> PASS       (fi-kbl-7500u) fdo#103841
> Test prime_vgem:
>          Subgroup basic-fence-flip:
>                  pass       -> FAIL       (fi-ilk-650) fdo#104008
> 
> fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
> fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
> fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
> 
> fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:413s
> fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
> fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:370s
> fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:483s
> fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
> fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
> fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:482s
> fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:464s
> fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:454s
> fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:567s
> fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:420s
> fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:283s
> fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:507s
> fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
> fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:412s
> fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:456s
> fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:412s
> fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:450s
> fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:489s
> fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
> fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:494s
> fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
> fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:427s
> fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
> fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:516s
> fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
> fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:486s
> fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
> fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:426s
> fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33
> fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:393s
> 
> 67f148034b8b6afdb0811e88245b67ad7730bb1a drm-tip: 2018y-02m-20d-09h-12m-29s UTC integration manifest
> d5724507a190 drm/i915: Make global seqno known in i915_gem_request_execute tracepoint

Pushed, thanks for the review!

Regards,

Tvrtko
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 8bc7c50b8418..0deca06fdf0e 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -490,8 +490,6 @@  void __i915_gem_request_submit(struct drm_i915_gem_request *request)
 	GEM_BUG_ON(!irqs_disabled());
 	lockdep_assert_held(&engine->timeline->lock);
 
-	trace_i915_gem_request_execute(request);
-
 	/* Transfer from per-context onto the global per-engine timeline */
 	timeline = engine->timeline;
 	GEM_BUG_ON(timeline == request->timeline);
@@ -515,6 +513,8 @@  void __i915_gem_request_submit(struct drm_i915_gem_request *request)
 	list_move_tail(&request->link, &timeline->requests);
 	spin_unlock(&request->timeline->lock);
 
+	trace_i915_gem_request_execute(request);
+
 	wake_up_all(&request->execute);
 }