All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
@ 2017-09-11 13:09 Michał Winiarski
  2017-09-11 13:10 ` [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable Michał Winiarski
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Michał Winiarski @ 2017-09-11 13:09 UTC (permalink / raw)
  To: intel-gfx

There's no reason to hide those tracepoints.
Let's also remove the DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: John Harrison <john.c.harrison@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/Kconfig.debug | 11 -----------
 drivers/gpu/drm/i915/i915_trace.h  | 24 ------------------------
 2 files changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index aed7d207ea84..63bfac79a403 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -90,17 +90,6 @@ config DRM_I915_SELFTEST
 
 	  If in doubt, say "N".
 
-config DRM_I915_LOW_LEVEL_TRACEPOINTS
-        bool "Enable low level request tracing events"
-        depends on DRM_I915
-        default n
-        help
-          Choose this option to turn on low level request tracing events.
-          This provides the ability to precisely monitor engine utilisation
-          and also analyze the request dependency resolving timeline.
-
-          If in doubt, say "N".
-
 config DRM_I915_DEBUG_VBLANK_EVADE
 	bool "Enable extra debug warnings for vblank evasion"
 	depends on DRM_I915
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 92f4c5bb7aa7..b8f037986ae2 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -702,7 +702,6 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
 	    TP_ARGS(req)
 );
 
-#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS)
 DEFINE_EVENT(i915_gem_request, i915_gem_request_submit,
 	     TP_PROTO(struct drm_i915_gem_request *req),
 	     TP_ARGS(req)
@@ -751,29 +750,6 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_out,
 	     TP_PROTO(struct drm_i915_gem_request *req),
 	     TP_ARGS(req)
 );
-#else
-#if !defined(TRACE_HEADER_MULTI_READ)
-static inline void
-trace_i915_gem_request_submit(struct drm_i915_gem_request *req)
-{
-}
-
-static inline void
-trace_i915_gem_request_execute(struct drm_i915_gem_request *req)
-{
-}
-
-static inline void
-trace_i915_gem_request_in(struct drm_i915_gem_request *req, unsigned int port)
-{
-}
-
-static inline void
-trace_i915_gem_request_out(struct drm_i915_gem_request *req)
-{
-}
-#endif
-#endif
 
 TRACE_EVENT(intel_engine_notify,
 	    TP_PROTO(struct intel_engine_cs *engine, bool waiters),
-- 
2.13.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable
  2017-09-11 13:09 [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Michał Winiarski
@ 2017-09-11 13:10 ` Michał Winiarski
  2017-09-11 13:14   ` Chris Wilson
  2017-09-11 14:17 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Patchwork
  2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
  2 siblings, 1 reply; 9+ messages in thread
From: Michał Winiarski @ 2017-09-11 13:10 UTC (permalink / raw)
  To: intel-gfx

Originally removed in:
c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs")
f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs")

Were accidentaly restored in:
925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next")

We can also remove unused variable and replace it with a WARN.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
 drivers/gpu/drm/i915/intel_uc.h            | 4 ----
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 48a1e9349a2c..8a550785b257 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
 	struct intel_guc *guc = &rq->i915->guc;
 	struct i915_guc_client *client = guc->execbuf_client;
 	unsigned long flags;
-	int b_ret;
 
 	/* WA to flush out the pending GMADR writes to ring buffer. */
 	if (i915_vma_is_map_and_fenceable(rq->ring->vma))
@@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
 	spin_lock_irqsave(&client->wq_lock, flags);
 
 	guc_wq_item_append(client, rq);
-	b_ret = guc_ring_doorbell(client);
+	WARN_ON(guc_ring_doorbell(client));
 
 	client->submissions[engine_id] += 1;
 
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 22ae52b17b0f..69daf4c01cd0 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -59,10 +59,6 @@ struct drm_i915_gem_request;
  *                available in the work queue (note, the queue is shared,
  *                not per-engine). It is OK for this to be nonzero, but
  *                it should not be huge!
- *   b_fail: failed to ring the doorbell. This should never happen, unless
- *           somehow the hardware misbehaves, or maybe if the GuC firmware
- *           crashes? We probably need to reset the GPU to recover.
- *   retcode: errno from last guc_submit()
  */
 struct i915_guc_client {
 	struct i915_vma *vma;
-- 
2.13.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable
  2017-09-11 13:10 ` [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable Michał Winiarski
@ 2017-09-11 13:14   ` Chris Wilson
  0 siblings, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2017-09-11 13:14 UTC (permalink / raw)
  To: Michał Winiarski, intel-gfx

Quoting Michał Winiarski (2017-09-11 14:10:00)
> Originally removed in:
> c1adab970348 ("drm/i915/guc: Remove failed doorbell stat from debugfs")
> f1448a62a103 ("drm/i915/guc: Remove last submission result from debugfs")
> 
> Were accidentaly restored in:
> 925344ccc91d ("BackMerge tag 'v4.12-rc5' into drm-next")
> 
> We can also remove unused variable and replace it with a WARN.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c | 3 +--
>  drivers/gpu/drm/i915/intel_uc.h            | 4 ----
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 48a1e9349a2c..8a550785b257 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -602,7 +602,6 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
>         struct intel_guc *guc = &rq->i915->guc;
>         struct i915_guc_client *client = guc->execbuf_client;
>         unsigned long flags;
> -       int b_ret;
>  
>         /* WA to flush out the pending GMADR writes to ring buffer. */
>         if (i915_vma_is_map_and_fenceable(rq->ring->vma))
> @@ -611,7 +610,7 @@ static void __i915_guc_submit(struct drm_i915_gem_request *rq)
>         spin_lock_irqsave(&client->wq_lock, flags);
>  
>         guc_wq_item_append(client, rq);
> -       b_ret = guc_ring_doorbell(client);
> +       WARN_ON(guc_ring_doorbell(client));
>  
>         client->submissions[engine_id] += 1;
>  
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 22ae52b17b0f..69daf4c01cd0 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -59,10 +59,6 @@ struct drm_i915_gem_request;
>   *                available in the work queue (note, the queue is shared,
>   *                not per-engine). It is OK for this to be nonzero, but
>   *                it should not be huge!

no_wq_space^^ is also dead


> - *   b_fail: failed to ring the doorbell. This should never happen, unless
> - *           somehow the hardware misbehaves, or maybe if the GuC firmware
> - *           crashes? We probably need to reset the GPU to recover.
> - *   retcode: errno from last guc_submit()
>   */
>  struct i915_guc_client {
>         struct i915_vma *vma;

Reviewed-y 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 13:09 [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Michał Winiarski
  2017-09-11 13:10 ` [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable Michał Winiarski
@ 2017-09-11 14:17 ` Patchwork
  2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2017-09-11 14:17 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
URL   : https://patchwork.freedesktop.org/series/30129/
State : warning

== Summary ==

Series 30129v1 series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
https://patchwork.freedesktop.org/api/1.0/series/30129/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-snb-2600) fdo#100215
Test drv_module_reload:
        Subgroup basic-reload-inject:
                pass       -> DMESG-WARN (fi-skl-6770hq)

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:456s
fi-blb-e6850     total:289  pass:224  dwarn:1   dfail:0   fail:0   skip:64  time:377s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:526s
fi-bwr-2160      total:289  pass:184  dwarn:0   dfail:0   fail:0   skip:105 time:268s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:510s
fi-byt-j1900     total:289  pass:254  dwarn:1   dfail:0   fail:0   skip:34  time:499s
fi-byt-n2820     total:289  pass:250  dwarn:1   dfail:0   fail:0   skip:38  time:495s
fi-cfl-s         total:289  pass:250  dwarn:4   dfail:0   fail:0   skip:35  time:451s
fi-elk-e7500     total:289  pass:230  dwarn:0   dfail:0   fail:0   skip:59  time:449s
fi-glk-2a        total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:606s
fi-hsw-4770      total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:427s
fi-hsw-4770r     total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:408s
fi-ilk-650       total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:435s
fi-ivb-3520m     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:487s
fi-ivb-3770      total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:462s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:489s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:581s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:586s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:459s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:519s
fi-skl-6770hq    total:289  pass:268  dwarn:1   dfail:0   fail:0   skip:20  time:508s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:460s
fi-skl-x1585l    total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:478s
fi-snb-2520m     total:289  pass:251  dwarn:0   dfail:0   fail:0   skip:38  time:569s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:1   skip:39  time:422s
fi-pnv-d510 failed to connect after reboot

a06ff73a7522af231e77776d7948e4d8099558c6 drm-tip: 2017y-09m-11d-13h-31m-34s UTC integration manifest
1f89761f81c6 drm/i915/guc: Remove obsolete comments and remove unused variable
83d95cfd1301 drm/i915/tracepoints: Don't compile-out low-level tracepoints

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5643/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 13:09 [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Michał Winiarski
  2017-09-11 13:10 ` [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable Michał Winiarski
  2017-09-11 14:17 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Patchwork
@ 2017-09-11 15:34 ` Tvrtko Ursulin
  2017-09-11 19:52   ` Chris Wilson
                     ` (2 more replies)
  2 siblings, 3 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2017-09-11 15:34 UTC (permalink / raw)
  To: Michał Winiarski, intel-gfx


On 11/09/2017 14:09, Michał Winiarski wrote:
> There's no reason to hide those tracepoints.
> Let's also remove the DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option.

No numbers from (micro-)bechmarks showing how small the impact of doing 
this is? I thought John was compiling this data. It will be just a no-op 
on the fast path, but a bit more generated code.

Assuming that will be fine, the only potentially problematic aspect that 
comes to mind is the fact meaning of these tracepoints is a bit 
different between execlists and guc. But maybe that is thinking to low 
level (!) - in fact they are in both cases at points where i915 is 
passing/receiving requests to/from hardware so not an issue?

Regards,

Tvrtko

> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: John Harrison <john.c.harrison@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>   drivers/gpu/drm/i915/Kconfig.debug | 11 -----------
>   drivers/gpu/drm/i915/i915_trace.h  | 24 ------------------------
>   2 files changed, 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
> index aed7d207ea84..63bfac79a403 100644
> --- a/drivers/gpu/drm/i915/Kconfig.debug
> +++ b/drivers/gpu/drm/i915/Kconfig.debug
> @@ -90,17 +90,6 @@ config DRM_I915_SELFTEST
>   
>   	  If in doubt, say "N".
>   
> -config DRM_I915_LOW_LEVEL_TRACEPOINTS
> -        bool "Enable low level request tracing events"
> -        depends on DRM_I915
> -        default n
> -        help
> -          Choose this option to turn on low level request tracing events.
> -          This provides the ability to precisely monitor engine utilisation
> -          and also analyze the request dependency resolving timeline.
> -
> -          If in doubt, say "N".
> -
>   config DRM_I915_DEBUG_VBLANK_EVADE
>   	bool "Enable extra debug warnings for vblank evasion"
>   	depends on DRM_I915
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 92f4c5bb7aa7..b8f037986ae2 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -702,7 +702,6 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_add,
>   	    TP_ARGS(req)
>   );
>   
> -#if defined(CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS)
>   DEFINE_EVENT(i915_gem_request, i915_gem_request_submit,
>   	     TP_PROTO(struct drm_i915_gem_request *req),
>   	     TP_ARGS(req)
> @@ -751,29 +750,6 @@ DEFINE_EVENT(i915_gem_request, i915_gem_request_out,
>   	     TP_PROTO(struct drm_i915_gem_request *req),
>   	     TP_ARGS(req)
>   );
> -#else
> -#if !defined(TRACE_HEADER_MULTI_READ)
> -static inline void
> -trace_i915_gem_request_submit(struct drm_i915_gem_request *req)
> -{
> -}
> -
> -static inline void
> -trace_i915_gem_request_execute(struct drm_i915_gem_request *req)
> -{
> -}
> -
> -static inline void
> -trace_i915_gem_request_in(struct drm_i915_gem_request *req, unsigned int port)
> -{
> -}
> -
> -static inline void
> -trace_i915_gem_request_out(struct drm_i915_gem_request *req)
> -{
> -}
> -#endif
> -#endif
>   
>   TRACE_EVENT(intel_engine_notify,
>   	    TP_PROTO(struct intel_engine_cs *engine, bool waiters),
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
@ 2017-09-11 19:52   ` Chris Wilson
  2017-09-11 20:23     ` Rogozhkin, Dmitry V
  2020-01-31 21:17   ` [Intel-gfx] " Egor Suldin
  2020-01-31 21:32   ` Egor Suldin
  2 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2017-09-11 19:52 UTC (permalink / raw)
  To: Tvrtko Ursulin, Michał Winiarski, intel-gfx

Quoting Tvrtko Ursulin (2017-09-11 16:34:08)
> 
> On 11/09/2017 14:09, Michał Winiarski wrote:
> > There's no reason to hide those tracepoints.
> > Let's also remove the DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option.
> 
> No numbers from (micro-)bechmarks showing how small the impact of doing 
> this is? I thought John was compiling this data. It will be just a no-op 
> on the fast path, but a bit more generated code.
> 
> Assuming that will be fine, the only potentially problematic aspect that 
> comes to mind is the fact meaning of these tracepoints is a bit 
> different between execlists and guc. But maybe that is thinking to low 
> level (!) - in fact they are in both cases at points where i915 is 
> passing/receiving requests to/from hardware so not an issue?

Along the same lines is that this implies that these are important
enough to be ABI, and that means we need to make a long term decision on
the viability and meaning of such tracepoints.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 19:52   ` Chris Wilson
@ 2017-09-11 20:23     ` Rogozhkin, Dmitry V
  0 siblings, 0 replies; 9+ messages in thread
From: Rogozhkin, Dmitry V @ 2017-09-11 20:23 UTC (permalink / raw)
  To: chris; +Cc: intel-gfx

On Mon, 2017-09-11 at 20:52 +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-09-11 16:34:08)
> > 
> > On 11/09/2017 14:09, Michał Winiarski wrote:
> > > There's no reason to hide those tracepoints.
> > > Let's also remove the DRM_I915_LOW_LEVEL_TRACEPOINTS Kconfig option.
> > 
> > No numbers from (micro-)bechmarks showing how small the impact of doing 
> > this is? I thought John was compiling this data. It will be just a no-op 
> > on the fast path, but a bit more generated code.
> > 
> > Assuming that will be fine, the only potentially problematic aspect that 
> > comes to mind is the fact meaning of these tracepoints is a bit 
> > different between execlists and guc. But maybe that is thinking to low 
> > level (!) - in fact they are in both cases at points where i915 is 
> > passing/receiving requests to/from hardware so not an issue?
> 
> Along the same lines is that this implies that these are important
> enough to be ABI, and that means we need to make a long term decision on
> the viability and meaning of such tracepoints.
> -Chris
There is a number of applications which use these tracepoints for tasks
profiling putting them on the time scale for visualization. For example,
VTune and GPA, - these 2 are closed source. Not sure whether there are
open source profiling tools with such support (at least for GPU). Right
now VTune and GPA are stuck with custom patches for the i915 in the
better case, thus limiting themselves with the customers who are ok with
the custom patching. Thus, making this ABI (if possible) or at least
making sure this does not get broken or disappears (with for example GUC
enabling) would be highly beneficial.

Dmitry.

> _______________________________________________
> 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] 9+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
  2017-09-11 19:52   ` Chris Wilson
@ 2020-01-31 21:17   ` Egor Suldin
  2020-01-31 21:32   ` Egor Suldin
  2 siblings, 0 replies; 9+ messages in thread
From: Egor Suldin @ 2020-01-31 21:17 UTC (permalink / raw)
  To: intel-gfx

Hi!
I use GPUVis and now Intel Vtune Profiler. These tools don't work 
out-of-the-box on all Linux based systems for Intel integrated graphics.
It is needed to rebuild at least i915 module. And each time when the 
kernel is updated it is needed to rebuild i915 module again.

 > No numbers from (micro-)bechmarks showing how small the impact of doing
 > this is? I thought John was compiling this data. It will be just a no-op
 > on the fast path, but a bit more generated code.
Have you collected the results? If not, I've done it for you:
Benchmark for Metro 2033 Last Light Redux:
w/o events:
1st run aver. fps: 36.06
2nd run aver. fps: 35.87
w events:
1st run aver. fps: 36.05
2nd run aver. fps: 35.92

There is no difference. It was run on Intel Core i9-9900K CPU @ 3.60GHz 
on integrated graphics.

 > Assuming that will be fine, the only potentially problematic aspect that
 > comes to mind is the fact meaning of these tracepoints is a bit
 > different between execlists and guc. But maybe that is thinking to low
 > level (!) - in fact they are in both cases at points where i915 is
 >passing/receiving requests to/from hardware so not an issue?
In my view, it is not an issue. The real issue now that you cannot 
collect performance results for Intel GPU
on Linux systems without rebuilding the i915 module. You cannot debug 
performance problems
on the system even if you use tools from Intel. Do you have ETA to 
accept this patch?

Thanks,
Egor
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints
  2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
  2017-09-11 19:52   ` Chris Wilson
  2020-01-31 21:17   ` [Intel-gfx] " Egor Suldin
@ 2020-01-31 21:32   ` Egor Suldin
  2 siblings, 0 replies; 9+ messages in thread
From: Egor Suldin @ 2020-01-31 21:32 UTC (permalink / raw)
  To: intel-gfx

Hi!
I use GPUVis and now Intel Vtune Profiler. These tools don't work 
out-of-the-box on all Linux based systems for Intel integrated graphics.
It is needed to rebuild at least i915 module. And each time when the 
kernel is updated it is needed to rebuild i915 module again.

 > No numbers from (micro-)bechmarks showing how small the impact of doing
 > this is? I thought John was compiling this data. It will be just a no-op
 > on the fast path, but a bit more generated code.
Have you collected the results? If not, I've done it for you:
Benchmark for Metro 2033 Last Light Redux:
w/o events:
1st run aver. fps: 36.06
2nd run aver. fps: 35.87
w events:
1st run aver. fps: 36.05
2nd run aver. fps: 35.92

There is no difference. It was run on Intel Core i9-9900K CPU @ 3.60GHz 
on integrated graphics.

 > Assuming that will be fine, the only potentially problematic aspect that
 > comes to mind is the fact meaning of these tracepoints is a bit
 > different between execlists and guc. But maybe that is thinking to low
 > level (!) - in fact they are in both cases at points where i915 is
 >passing/receiving requests to/from hardware so not an issue?
In my view, it is not an issue. The real issue now that you cannot 
collect performance results for Intel GPU
on Linux systems without rebuilding the i915 module. You cannot debug 
performance problems
on the system even if you use tools from Intel. Do you have ETA to 
accept this patch?

Thanks,
Egor
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-01-31 21:35 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-11 13:09 [PATCH 1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Michał Winiarski
2017-09-11 13:10 ` [PATCH 2/2] drm/i915/guc: Remove obsolete comments and remove unused variable Michał Winiarski
2017-09-11 13:14   ` Chris Wilson
2017-09-11 14:17 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915/tracepoints: Don't compile-out low-level tracepoints Patchwork
2017-09-11 15:34 ` [PATCH 1/2] " Tvrtko Ursulin
2017-09-11 19:52   ` Chris Wilson
2017-09-11 20:23     ` Rogozhkin, Dmitry V
2020-01-31 21:17   ` [Intel-gfx] " Egor Suldin
2020-01-31 21:32   ` Egor Suldin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.