[Intel-gfx] [Intel-gfx 0/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Intel-gfx] [Intel-gfx 0/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Alan Previn]

This change ensures we don't collect new stat counters too soon
after the last sample.

Alan Previn (1):
  drm/i915/guc: Don't update engine busyness stats too frequently

 drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
 2 files changed, 19 insertions(+)


base-commit: cb89eb64792fd1a78c5ffc473f7e208b88e62fad
-- 
2.25.1

[Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Alan Previn]

Using igt's gem-create and with additional patches to track object
creation time, it was measured that guc_update_engine_gt_clks was
getting called over 188 thousand times in the span of 15 seconds
(running the test three times).

Get a jiffies sample on every trigger and ensure we skip sampling
if we are being called too soon. Use half of the ping_delay as a
safe threshold.

NOTE: with this change, the number of calls went down to just 14
over the same span of time (matching the original intent of running
about once every 24 seconds, at 19.2Mhz GT freq, per engine).

Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 2286f96f5f87..63f4ecdf1606 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
 	 * @start_gt_clk: GT clock time of last idle to active transition.
 	 */
 	u64 start_gt_clk;
+
+	/**
+	 * @last_jiffies: Jiffies at last actual stats collection time
+	 *
+	 * We use this timestamp to ensure we don't oversample the
+	 * stats because runtime power management events can trigger
+	 * stats collection at much higher rates than required.
+	 */
+	u64 last_jiffies;
+
 };
 
 struct intel_engine_cs {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 5a1dfacf24ea..8f8bf6e40ccb 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1167,6 +1167,15 @@ static void guc_update_engine_gt_clks(struct intel_engine_cs *engine)
 	struct intel_engine_guc_stats *stats = &engine->stats.guc;
 	struct intel_guc *guc = &engine->gt->uc.guc;
 	u32 last_switch, ctx_id, total;
+	u64 newjiffs;
+
+	/* Don't worry about jiffies wrap-around, a rare additional sample won't have any impact */
+	newjiffs = get_jiffies_64();
+	if (stats->last_jiffies && (newjiffs - stats->last_jiffies <
+	   (guc->timestamp.ping_delay << 1)))
+		return;
+
+	stats->last_jiffies = newjiffs;
 
 	lockdep_assert_held(&guc->timestamp.lock);
 
-- 
2.25.1

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Tvrtko Ursulin]

On 11/06/2022 18:27, Alan Previn wrote:
> Using igt's gem-create and with additional patches to track object
> creation time, it was measured that guc_update_engine_gt_clks was
> getting called over 188 thousand times in the span of 15 seconds
> (running the test three times).
> 
> Get a jiffies sample on every trigger and ensure we skip sampling
> if we are being called too soon. Use half of the ping_delay as a
> safe threshold.
> 
> NOTE: with this change, the number of calls went down to just 14
> over the same span of time (matching the original intent of running
> about once every 24 seconds, at 19.2Mhz GT freq, per engine).

+ Umesh

What is the effect on accuracy? AFAIR up to date clock was needed for 
correct accounting of running contexts.

Regards,

Tvrtko

> 
> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 2286f96f5f87..63f4ecdf1606 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
>   	 * @start_gt_clk: GT clock time of last idle to active transition.
>   	 */
>   	u64 start_gt_clk;
> +
> +	/**
> +	 * @last_jiffies: Jiffies at last actual stats collection time
> +	 *
> +	 * We use this timestamp to ensure we don't oversample the
> +	 * stats because runtime power management events can trigger
> +	 * stats collection at much higher rates than required.
> +	 */
> +	u64 last_jiffies;
> +
>   };
>   
>   struct intel_engine_cs {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 5a1dfacf24ea..8f8bf6e40ccb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -1167,6 +1167,15 @@ static void guc_update_engine_gt_clks(struct intel_engine_cs *engine)
>   	struct intel_engine_guc_stats *stats = &engine->stats.guc;
>   	struct intel_guc *guc = &engine->gt->uc.guc;
>   	u32 last_switch, ctx_id, total;
> +	u64 newjiffs;
> +
> +	/* Don't worry about jiffies wrap-around, a rare additional sample won't have any impact */
> +	newjiffs = get_jiffies_64();
> +	if (stats->last_jiffies && (newjiffs - stats->last_jiffies <
> +	   (guc->timestamp.ping_delay << 1)))
> +		return;
> +
> +	stats->last_jiffies = newjiffs;
>   
>   	lockdep_assert_held(&guc->timestamp.lock);
>   

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/guc: Don't update engine busyness stats too frequently (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 9569 bytes --]

== Series Details ==

Series: drm/i915/guc: Don't update engine busyness stats too frequently (rev2)
URL   : https://patchwork.freedesktop.org/series/105023/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11755 -> Patchwork_105023v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_105023v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_105023v2, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/index.html

Participating hosts (44 -> 41)
------------------------------

  Missing    (3): fi-cml-u2 bat-adlm-1 bat-jsl-2 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_105023v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_engines:
    - fi-rkl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html
    - bat-dg1-6:          [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-dg1-6/igt@i915_selftest@live@gt_engines.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-dg1-6/igt@i915_selftest@live@gt_engines.html
    - bat-dg1-5:          [PASS][5] -> [INCOMPLETE][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-dg1-5/igt@i915_selftest@live@gt_engines.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-dg1-5/igt@i915_selftest@live@gt_engines.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@gt_engines:
    - {bat-dg2-9}:        [DMESG-WARN][7] ([i915#5763]) -> [INCOMPLETE][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-dg2-9/igt@i915_selftest@live@gt_engines.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-dg2-9/igt@i915_selftest@live@gt_engines.html

  
Known issues
------------

  Here are the changes found in Patchwork_105023v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@coherency:
    - fi-bdw-5557u:       [PASS][9] -> [INCOMPLETE][10] ([i915#5674] / [i915#5685])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-bdw-5557u/igt@i915_selftest@live@coherency.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-bdw-5557u/igt@i915_selftest@live@coherency.html

  * igt@i915_selftest@live@gem:
    - fi-pnv-d510:        NOTRUN -> [DMESG-FAIL][11] ([i915#4528])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-pnv-d510/igt@i915_selftest@live@gem.html

  * igt@i915_selftest@live@gt_engines:
    - bat-adlp-4:         [PASS][12] -> [INCOMPLETE][13] ([i915#5401])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-adlp-4/igt@i915_selftest@live@gt_engines.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-adlp-4/igt@i915_selftest@live@gt_engines.html

  * igt@i915_selftest@live@requests:
    - fi-blb-e6850:       [PASS][14] -> [DMESG-FAIL][15] ([i915#4528])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-blb-e6850/igt@i915_selftest@live@requests.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-blb-e6850/igt@i915_selftest@live@requests.html

  * igt@kms_busy@basic@modeset:
    - fi-tgl-u2:          [PASS][16] -> [DMESG-WARN][17] ([i915#402])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-tgl-u2/igt@kms_busy@basic@modeset.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-tgl-u2/igt@kms_busy@basic@modeset.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-kbl-guc:         NOTRUN -> [SKIP][18] ([fdo#109271])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-kbl-guc/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - bat-adlp-4:         [PASS][19] -> [DMESG-WARN][20] ([i915#3576])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-adlp-4/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-adlp-4/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@runner@aborted:
    - bat-adlp-4:         NOTRUN -> [FAIL][21] ([i915#4312])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-adlp-4/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][22] ([i915#4312])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-rkl-guc/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-cfl-8109u:       [DMESG-FAIL][23] ([i915#62]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-cfl-8109u/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_heartbeat:
    - {fi-jsl-1}:         [DMESG-FAIL][25] ([i915#5334]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-jsl-1/igt@i915_selftest@live@gt_heartbeat.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-jsl-1/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@late_gt_pm:
    - fi-cfl-8109u:       [DMESG-WARN][27] ([i915#5904]) -> [PASS][28] +29 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-cfl-8109u/igt@i915_selftest@live@late_gt_pm.html

  * igt@i915_selftest@live@requests:
    - fi-pnv-d510:        [DMESG-FAIL][29] ([i915#4528]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-pnv-d510/igt@i915_selftest@live@requests.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-pnv-d510/igt@i915_selftest@live@requests.html

  * igt@i915_selftest@live@uncore:
    - {bat-dg2-9}:        [DMESG-WARN][31] ([i915#5763]) -> [PASS][32] +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-dg2-9/igt@i915_selftest@live@uncore.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-dg2-9/igt@i915_selftest@live@uncore.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-cfl-8109u:       [DMESG-WARN][33] ([i915#5904] / [i915#62]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-cfl-8109u/igt@i915_suspend@basic-s2idle-without-i915.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-cfl-8109u/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@kms_busy@basic@flip:
    - bat-adlp-4:         [DMESG-WARN][35] ([i915#3576]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/bat-adlp-4/igt@kms_busy@basic@flip.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/bat-adlp-4/igt@kms_busy@basic@flip.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-cfl-8109u:       [DMESG-WARN][37] ([i915#62]) -> [PASS][38] +15 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11755/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/fi-cfl-8109u/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5401]: https://gitlab.freedesktop.org/drm/intel/issues/5401
  [i915#5674]: https://gitlab.freedesktop.org/drm/intel/issues/5674
  [i915#5685]: https://gitlab.freedesktop.org/drm/intel/issues/5685
  [i915#5763]: https://gitlab.freedesktop.org/drm/intel/issues/5763
  [i915#5904]: https://gitlab.freedesktop.org/drm/intel/issues/5904
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62


Build changes
-------------

  * Linux: CI_DRM_11755 -> Patchwork_105023v2

  CI-20190529: 20190529
  CI_DRM_11755: 65b93b94d6bc932ed60bb3fd9d68242db25b1f3b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6522: 5be5a1a1f168a59614101b77385f05f12ec7d30a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_105023v2: 65b93b94d6bc932ed60bb3fd9d68242db25b1f3b @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

040b2cdaa7fd drm/i915/guc: Don't update engine busyness stats too frequently

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105023v2/index.html

[-- Attachment #2: Type: text/html, Size: 11254 bytes --]

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Umesh Nerlige Ramappa]

On Sat, Jun 11, 2022 at 10:27:11AM -0700, Alan Previn wrote:
>Using igt's gem-create and with additional patches to track object
>creation time, it was measured that guc_update_engine_gt_clks was
>getting called over 188 thousand times in the span of 15 seconds
>(running the test three times).
>
>Get a jiffies sample on every trigger and ensure we skip sampling
>if we are being called too soon. Use half of the ping_delay as a
>safe threshold.
>
>NOTE: with this change, the number of calls went down to just 14
>over the same span of time (matching the original intent of running
>about once every 24 seconds, at 19.2Mhz GT freq, per engine).
>
>Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>---
> drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
> 2 files changed, 19 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>index 2286f96f5f87..63f4ecdf1606 100644
>--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>@@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
> 	 * @start_gt_clk: GT clock time of last idle to active transition.
> 	 */
> 	u64 start_gt_clk;
>+
>+	/**
>+	 * @last_jiffies: Jiffies at last actual stats collection time
>+	 *
>+	 * We use this timestamp to ensure we don't oversample the
>+	 * stats because runtime power management events can trigger
>+	 * stats collection at much higher rates than required.
>+	 */
>+	u64 last_jiffies;
>+
> };
>
> struct intel_engine_cs {
>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>index 5a1dfacf24ea..8f8bf6e40ccb 100644
>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>@@ -1167,6 +1167,15 @@ static void guc_update_engine_gt_clks(struct intel_engine_cs *engine)

A user query will end up in guc_engine_busyness which will call 
guc_update_engine_gt_clks. Adding this logic here will affect accuracy.  

The other place where guc_update_engine_gt_clks is called is in the ping 
worker, but that worker runs at 1/8th the wrap around time for the gt 
clocks (32 bit). The last I checked the wrap around was at 22 seconds.

That leaves only the gt_park path. fwiu, this path runs too frequently 
and here we are updating the busyness stats. That is causing the 
enormous PCI traffic (lmem accesses). Only this path needs to be fixed, 
as in just use the same logic in the intel_guc_busyness_park() to decide 
whether to call __update_guc_busyness_stats or not.

Of course, if you are running the user query too frequently, then IMO we 
should not fix that in i915. 

If you haven't already, please make sure the igt@perf_pmu@ tests are all 
passing with any of these changes. There's also a selftest - 
live_engine_busy_stats that you need to make sure passes.

> 	struct intel_engine_guc_stats *stats = &engine->stats.guc;
> 	struct intel_guc *guc = &engine->gt->uc.guc;
> 	u32 last_switch, ctx_id, total;
>+	u64 newjiffs;
>+
>+	/* Don't worry about jiffies wrap-around, a rare additional sample won't have any impact */
>+	newjiffs = get_jiffies_64();
>+	if (stats->last_jiffies && (newjiffs - stats->last_jiffies <
>+	   (guc->timestamp.ping_delay << 1)))

You want to right shift by 1 for half the ping delay here.

Thanks,
Umesh

>+		return;
>+
>+	stats->last_jiffies = newjiffs;
>
> 	lockdep_assert_held(&guc->timestamp.lock);
>
>-- 
>2.25.1
>

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Tvrtko Ursulin]

On 14/06/2022 02:10, Umesh Nerlige Ramappa wrote:
> On Sat, Jun 11, 2022 at 10:27:11AM -0700, Alan Previn wrote:
>> Using igt's gem-create and with additional patches to track object
>> creation time, it was measured that guc_update_engine_gt_clks was
>> getting called over 188 thousand times in the span of 15 seconds
>> (running the test three times).
>>
>> Get a jiffies sample on every trigger and ensure we skip sampling
>> if we are being called too soon. Use half of the ping_delay as a
>> safe threshold.
>>
>> NOTE: with this change, the number of calls went down to just 14
>> over the same span of time (matching the original intent of running
>> about once every 24 seconds, at 19.2Mhz GT freq, per engine).
>>
>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
>> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> index 2286f96f5f87..63f4ecdf1606 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>> @@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
>>      * @start_gt_clk: GT clock time of last idle to active transition.
>>      */
>>     u64 start_gt_clk;
>> +
>> +    /**
>> +     * @last_jiffies: Jiffies at last actual stats collection time
>> +     *
>> +     * We use this timestamp to ensure we don't oversample the
>> +     * stats because runtime power management events can trigger
>> +     * stats collection at much higher rates than required.
>> +     */
>> +    u64 last_jiffies;
>> +
>> };
>>
>> struct intel_engine_cs {
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> index 5a1dfacf24ea..8f8bf6e40ccb 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -1167,6 +1167,15 @@ static void guc_update_engine_gt_clks(struct 
>> intel_engine_cs *engine)
> 
> A user query will end up in guc_engine_busyness which will call 
> guc_update_engine_gt_clks. Adding this logic here will affect accuracy.
> The other place where guc_update_engine_gt_clks is called is in the ping 
> worker, but that worker runs at 1/8th the wrap around time for the gt 
> clocks (32 bit). The last I checked the wrap around was at 22 seconds.
> 
> That leaves only the gt_park path. fwiu, this path runs too frequently 
> and here we are updating the busyness stats. That is causing the 
> enormous PCI traffic (lmem accesses). Only this path needs to be fixed, 
> as in just use the same logic in the intel_guc_busyness_park() to decide 
> whether to call __update_guc_busyness_stats or not.

Not updating the driver state in park will not negatively impact 
accuracy in some scenarios? That needs to balanced against the questions 
whether or not there are real world scenarios impacted by the update 
cost or it is just for IGT.

Regards,

Tvrtko

> 
> Of course, if you are running the user query too frequently, then IMO we 
> should not fix that in i915.
> If you haven't already, please make sure the igt@perf_pmu@ tests are all 
> passing with any of these changes. There's also a selftest - 
> live_engine_busy_stats that you need to make sure passes.
> 
>>     struct intel_engine_guc_stats *stats = &engine->stats.guc;
>>     struct intel_guc *guc = &engine->gt->uc.guc;
>>     u32 last_switch, ctx_id, total;
>> +    u64 newjiffs;
>> +
>> +    /* Don't worry about jiffies wrap-around, a rare additional 
>> sample won't have any impact */
>> +    newjiffs = get_jiffies_64();
>> +    if (stats->last_jiffies && (newjiffs - stats->last_jiffies <
>> +       (guc->timestamp.ping_delay << 1)))
> 
> You want to right shift by 1 for half the ping delay here.
> 
> Thanks,
> Umesh
> 
>> +        return;
>> +
>> +    stats->last_jiffies = newjiffs;
>>
>>     lockdep_assert_held(&guc->timestamp.lock);
>>
>> -- 
>> 2.25.1
>>

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Lucas De Marchi]

On Tue, Jun 14, 2022 at 08:07:04AM +0100, Tvrtko Ursulin wrote:
>
>On 14/06/2022 02:10, Umesh Nerlige Ramappa wrote:
>>On Sat, Jun 11, 2022 at 10:27:11AM -0700, Alan Previn wrote:
>>>Using igt's gem-create and with additional patches to track object
>>>creation time, it was measured that guc_update_engine_gt_clks was
>>>getting called over 188 thousand times in the span of 15 seconds
>>>(running the test three times).
>>>
>>>Get a jiffies sample on every trigger and ensure we skip sampling
>>>if we are being called too soon. Use half of the ping_delay as a
>>>safe threshold.
>>>
>>>NOTE: with this change, the number of calls went down to just 14
>>>over the same span of time (matching the original intent of running
>>>about once every 24 seconds, at 19.2Mhz GT freq, per engine).
>>>
>>>Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>>>---
>>>drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
>>>drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
>>>2 files changed, 19 insertions(+)
>>>
>>>diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
>>>b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>index 2286f96f5f87..63f4ecdf1606 100644
>>>--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>@@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
>>>     * @start_gt_clk: GT clock time of last idle to active transition.
>>>     */
>>>    u64 start_gt_clk;
>>>+
>>>+    /**
>>>+     * @last_jiffies: Jiffies at last actual stats collection time
>>>+     *
>>>+     * We use this timestamp to ensure we don't oversample the
>>>+     * stats because runtime power management events can trigger
>>>+     * stats collection at much higher rates than required.
>>>+     */
>>>+    u64 last_jiffies;
>>>+
>>>};
>>>
>>>struct intel_engine_cs {
>>>diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>>>b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>index 5a1dfacf24ea..8f8bf6e40ccb 100644
>>>--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>@@ -1167,6 +1167,15 @@ static void 
>>>guc_update_engine_gt_clks(struct intel_engine_cs *engine)
>>
>>A user query will end up in guc_engine_busyness which will call 
>>guc_update_engine_gt_clks. Adding this logic here will affect 
>>accuracy.
>>The other place where guc_update_engine_gt_clks is called is in the 
>>ping worker, but that worker runs at 1/8th the wrap around time for 
>>the gt clocks (32 bit). The last I checked the wrap around was at 22 
>>seconds.
>>
>>That leaves only the gt_park path. fwiu, this path runs too 
>>frequently and here we are updating the busyness stats. That is 
>>causing the enormous PCI traffic (lmem accesses). Only this path 
>>needs to be fixed, as in just use the same logic in the 
>>intel_guc_busyness_park() to decide whether to call 
>>__update_guc_busyness_stats or not.
>
>Not updating the driver state in park will not negatively impact 
>accuracy in some scenarios? That needs to balanced against the 
>questions whether or not there are real world scenarios impacted by 
>the update cost or it is just for IGT.

there is, which was what motivated 
https://patchwork.freedesktop.org/series/105011/ and in parallel Alan
worked on this. I view both as orthogonal  thought. I used it to make
the single-word-from-lmem faster, but if we can reduce
the frequency this code path is called, it should be even better.
Per Umesh's and your comment I'm unsure if we can... but if
there is no user monitoring the usage, should we still be calling this?
"Nobody is looking, why are we sampling?" kind of thought.

Summarizing the first patch in my series: it improved igt in ~50% and a
real world case in ~12%

Lucas De Marchi

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Tvrtko Ursulin]

On 15/06/2022 19:59, Lucas De Marchi wrote:
> On Tue, Jun 14, 2022 at 08:07:04AM +0100, Tvrtko Ursulin wrote:
>>
>> On 14/06/2022 02:10, Umesh Nerlige Ramappa wrote:
>>> On Sat, Jun 11, 2022 at 10:27:11AM -0700, Alan Previn wrote:
>>>> Using igt's gem-create and with additional patches to track object
>>>> creation time, it was measured that guc_update_engine_gt_clks was
>>>> getting called over 188 thousand times in the span of 15 seconds
>>>> (running the test three times).
>>>>
>>>> Get a jiffies sample on every trigger and ensure we skip sampling
>>>> if we are being called too soon. Use half of the ping_delay as a
>>>> safe threshold.
>>>>
>>>> NOTE: with this change, the number of calls went down to just 14
>>>> over the same span of time (matching the original intent of running
>>>> about once every 24 seconds, at 19.2Mhz GT freq, per engine).
>>>>
>>>> Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/gt/intel_engine_types.h      | 10 ++++++++++
>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c |  9 +++++++++
>>>> 2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h 
>>>> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>> index 2286f96f5f87..63f4ecdf1606 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
>>>> @@ -323,6 +323,16 @@ struct intel_engine_guc_stats {
>>>>      * @start_gt_clk: GT clock time of last idle to active transition.
>>>>      */
>>>>     u64 start_gt_clk;
>>>> +
>>>> +    /**
>>>> +     * @last_jiffies: Jiffies at last actual stats collection time
>>>> +     *
>>>> +     * We use this timestamp to ensure we don't oversample the
>>>> +     * stats because runtime power management events can trigger
>>>> +     * stats collection at much higher rates than required.
>>>> +     */
>>>> +    u64 last_jiffies;
>>>> +
>>>> };
>>>>
>>>> struct intel_engine_cs {
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>> index 5a1dfacf24ea..8f8bf6e40ccb 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>>>> @@ -1167,6 +1167,15 @@ static void guc_update_engine_gt_clks(struct 
>>>> intel_engine_cs *engine)
>>>
>>> A user query will end up in guc_engine_busyness which will call 
>>> guc_update_engine_gt_clks. Adding this logic here will affect accuracy.
>>> The other place where guc_update_engine_gt_clks is called is in the 
>>> ping worker, but that worker runs at 1/8th the wrap around time for 
>>> the gt clocks (32 bit). The last I checked the wrap around was at 22 
>>> seconds.
>>>
>>> That leaves only the gt_park path. fwiu, this path runs too 
>>> frequently and here we are updating the busyness stats. That is 
>>> causing the enormous PCI traffic (lmem accesses). Only this path 
>>> needs to be fixed, as in just use the same logic in the 
>>> intel_guc_busyness_park() to decide whether to call 
>>> __update_guc_busyness_stats or not.
>>
>> Not updating the driver state in park will not negatively impact 
>> accuracy in some scenarios? That needs to balanced against the 
>> questions whether or not there are real world scenarios impacted by 
>> the update cost or it is just for IGT.
> 
> there is, which was what motivated 
> https://patchwork.freedesktop.org/series/105011/ and in parallel Alan
> worked on this. I view both as orthogonal  thought. I used it to make
> the single-word-from-lmem faster, but if we can reduce
> the frequency this code path is called, it should be even better.
> Per Umesh's and your comment I'm unsure if we can... but if
> there is no user monitoring the usage, should we still be calling this?
> "Nobody is looking, why are we sampling?" kind of thought.

Who did you find is doing the sampling in the real world use case? AFAIR 
if one one is querying busyness, I thought there would only be the GuC 
ping worker which runs extremely infrequently (to avoid some counter 
overflow).

Regards,

Tvrtko

> Summarizing the first patch in my series: it improved igt in ~50% and a
> real world case in ~12%
> 
> Lucas De Marchi

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Teres Alexis, Alan Previn]

> Who did you find is doing the sampling in the real world use case? AFAIR 
> if one one is querying busyness, I thought there would only be the GuC 
> ping worker which runs extremely infrequently (to avoid some counter 
> overflow).
> 
> Regards,
> 
> Tvrtko
> 
> > 

Hi Tvrtko, the case where we are launching many tiny transcode workloads in quick succession sees the gt_unpark/park
getting his hundreds or thousands of time - this is where the biggest hit comes from (as per the worst case example
number in the comment). 

...alan

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Teres Alexis, Alan Previn]

> > > Not updating the driver state in park will not negatively impact 
> > > accuracy in some scenarios? That needs to balanced against the 
> > > questions whether or not there are real world scenarios impacted by 
> > > the update cost or it is just for IGT.
> > 
If i understand it correctly (how its calculated), no. not capturing on every park/unpark
does not mean less accuracy. Umesh mentioned to verify that with igt@perf-pmu@accuracy
tests

Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Don't update engine busyness stats too frequently

[Tvrtko Ursulin]

On 17/06/2022 22:43, Teres Alexis, Alan Previn wrote:
> 
>>>> Not updating the driver state in park will not negatively impact
>>>> accuracy in some scenarios? That needs to balanced against the
>>>> questions whether or not there are real world scenarios impacted by
>>>> the update cost or it is just for IGT.
>>>
> If i understand it correctly (how its calculated), no. not capturing on every park/unpark
> does not mean less accuracy. Umesh mentioned to verify that with igt@perf-pmu@accuracy
> tests

Okay you moved it from guc_update_engine_gt_clks to 
intel_guc_busyness_park to work around the loss of accuracy. I almost 
missed it since patch itself has not changelog. Works for me if Umesh 
approves it is correct.

Regards,

Tvrtko