intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
@ 2023-04-01  2:00 Vinay Belgaumkar
  2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Vinay Belgaumkar @ 2023-04-01  2:00 UTC (permalink / raw)
  To: intel-gfx, dri-devel

SLPC enables use of efficient freq at init by default. It is
possible for GuC to request frequencies that are higher than
the 'software' max if user has set it lower than the efficient
level.

Scenarios/tests that require strict fixing of freq below the efficient
level will need to disable it through this interface. Another way
to disable it is to set min frequency below the efficient level.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c   | 35 +++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c   | 49 ++++++++++++++-----
 drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h   |  1 +
 .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h |  1 +
 4 files changed, 75 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
index 28f27091cd3b..7496de5be580 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c
@@ -451,6 +451,33 @@ static ssize_t punit_req_freq_mhz_show(struct kobject *kobj,
 	return sysfs_emit(buff, "%u\n", preq);
 }
 
+static ssize_t slpc_ignore_eff_freq_show(struct kobject *kobj,
+				     struct kobj_attribute *attr,
+				     char *buff)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_guc_slpc *slpc = &gt->uc.guc.slpc;
+
+	return sysfs_emit(buff, "%u\n", slpc->ignore_eff_freq);
+}
+
+static ssize_t slpc_ignore_eff_freq_store(struct kobject *kobj,
+					  struct kobj_attribute *attr,
+					  const char *buff, size_t count)
+{
+	struct intel_gt *gt = intel_gt_sysfs_get_drvdata(kobj, attr->attr.name);
+	struct intel_guc_slpc *slpc = &gt->uc.guc.slpc;
+	int err;
+	uint32_t val;
+
+	err = kstrtou32(buff, 0, &val);
+	if (err)
+		return err;
+
+	err = intel_guc_slpc_set_ignore_eff_freq(slpc, val);
+	return err ?: count;
+}
+
 struct intel_gt_bool_throttle_attr {
 	struct attribute attr;
 	ssize_t (*show)(struct kobject *kobj, struct kobj_attribute *attr,
@@ -663,6 +690,8 @@ static struct kobj_attribute attr_media_freq_factor_scale =
 INTEL_GT_ATTR_RO(media_RP0_freq_mhz);
 INTEL_GT_ATTR_RO(media_RPn_freq_mhz);
 
+INTEL_GT_ATTR_RW(slpc_ignore_eff_freq);
+
 static const struct attribute *media_perf_power_attrs[] = {
 	&attr_media_freq_factor.attr,
 	&attr_media_freq_factor_scale.attr,
@@ -744,6 +773,12 @@ void intel_gt_sysfs_pm_init(struct intel_gt *gt, struct kobject *kobj)
 	if (ret)
 		gt_warn(gt, "failed to create punit_req_freq_mhz sysfs (%pe)", ERR_PTR(ret));
 
+	if (intel_uc_uses_guc_slpc(&gt->uc)) {
+		ret = sysfs_create_file(kobj, &attr_slpc_ignore_eff_freq.attr);
+		if (ret)
+			gt_warn(gt, "failed to create ignore_eff_freq sysfs (%pe)", ERR_PTR(ret));
+	}
+
 	if (i915_mmio_reg_valid(intel_gt_perf_limit_reasons_reg(gt))) {
 		ret = sysfs_create_files(kobj, throttle_reason_attrs);
 		if (ret)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
index 026d73855f36..eaa73c1fba6d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c
@@ -277,6 +277,7 @@ int intel_guc_slpc_init(struct intel_guc_slpc *slpc)
 
 	slpc->max_freq_softlimit = 0;
 	slpc->min_freq_softlimit = 0;
+	slpc->ignore_eff_freq = false;
 	slpc->min_is_rpmax = false;
 
 	slpc->boost_freq = 0;
@@ -457,6 +458,34 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val)
 	return ret;
 }
 
+int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val)
+{
+	struct drm_i915_private *i915 = slpc_to_i915(slpc);
+	intel_wakeref_t wakeref;
+	int ret = 0;
+
+	/* Need a lock now since waitboost can be modifying min as well */
+	mutex_lock(&slpc->lock);
+	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
+
+	/* Ignore efficient freq if lower min freq is requested */
+	ret = slpc_set_param(slpc,
+			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
+			     val);
+	if (ret) {
+		guc_probe_error(slpc_to_guc(slpc), "Failed to set efficient freq(%d): %pe\n",
+				val, ERR_PTR(ret));
+		goto out;
+	}
+
+	slpc->ignore_eff_freq = val;
+
+out:
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+	mutex_unlock(&slpc->lock);
+	return ret;
+}
+
 /**
  * intel_guc_slpc_set_min_freq() - Set min frequency limit for SLPC.
  * @slpc: pointer to intel_guc_slpc.
@@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
 	    val > slpc->max_freq_softlimit)
 		return -EINVAL;
 
+	/* Ignore efficient freq if lower min freq is requested */
+	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
+	if (ret)
+		goto out;
+
 	/* Need a lock now since waitboost can be modifying min as well */
 	mutex_lock(&slpc->lock);
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
-	/* Ignore efficient freq if lower min freq is requested */
-	ret = slpc_set_param(slpc,
-			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
-			     val < slpc->rp1_freq);
-	if (ret) {
-		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
-				ERR_PTR(ret));
-		goto out;
-	}
-
 	ret = slpc_set_param(slpc,
 			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
 			     val);
@@ -499,10 +523,10 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
 	if (!ret)
 		slpc->min_freq_softlimit = val;
 
-out:
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 	mutex_unlock(&slpc->lock);
 
+out:
 	/* Return standardized err code for sysfs calls */
 	if (ret)
 		ret = -EIO;
@@ -752,6 +776,9 @@ int intel_guc_slpc_enable(struct intel_guc_slpc *slpc)
 	/* Set cached media freq ratio mode */
 	intel_guc_slpc_set_media_ratio_mode(slpc, slpc->media_ratio_mode);
 
+	/* Set cached value of ignore efficient freq */
+	intel_guc_slpc_set_ignore_eff_freq(slpc, slpc->ignore_eff_freq);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
index 17ed515f6a85..597eb5413ddf 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h
@@ -46,5 +46,6 @@ void intel_guc_slpc_boost(struct intel_guc_slpc *slpc);
 void intel_guc_slpc_dec_waiters(struct intel_guc_slpc *slpc);
 int intel_guc_slpc_unset_gucrc_mode(struct intel_guc_slpc *slpc);
 int intel_guc_slpc_override_gucrc_mode(struct intel_guc_slpc *slpc, u32 mode);
+int intel_guc_slpc_set_ignore_eff_freq(struct intel_guc_slpc *slpc, bool val);
 
 #endif
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
index a6ef53b04e04..a88651331497 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h
@@ -31,6 +31,7 @@ struct intel_guc_slpc {
 	/* frequency softlimits */
 	u32 min_freq_softlimit;
 	u32 max_freq_softlimit;
+	bool ignore_eff_freq;
 
 	/* cached media ratio mode */
 	u32 media_ratio_mode;
-- 
2.38.1


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

* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
@ 2023-04-01  2:26 ` Patchwork
  2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-01  2:26 UTC (permalink / raw)
  To: Vinay Belgaumkar; +Cc: intel-gfx

== Series Details ==

Series: i915/guc/slpc: Provide sysfs for efficient freq
URL   : https://patchwork.freedesktop.org/series/115975/
State : warning

== Summary ==

Error: dim checkpatch failed
e3bff4466926 i915/guc/slpc: Provide sysfs for efficient freq
-:26: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#26: FILE: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:455:
+static ssize_t slpc_ignore_eff_freq_show(struct kobject *kobj,
+				     struct kobj_attribute *attr,

-:42: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u32' over 'uint32_t'
#42: FILE: drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c:471:
+	uint32_t val;

total: 0 errors, 0 warnings, 2 checks, 152 lines checked



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

* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
  2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2023-04-01  2:26 ` Patchwork
  2023-04-01  2:39 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-01  2:26 UTC (permalink / raw)
  To: Vinay Belgaumkar; +Cc: intel-gfx

== Series Details ==

Series: i915/guc/slpc: Provide sysfs for efficient freq
URL   : https://patchwork.freedesktop.org/series/115975/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.



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

* [Intel-gfx] ✓ Fi.CI.BAT: success for i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
  2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
  2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2023-04-01  2:39 ` Patchwork
  2023-04-01  3:11 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh
  2023-04-02  0:02 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-01  2:39 UTC (permalink / raw)
  To: Vinay Belgaumkar; +Cc: intel-gfx

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

== Series Details ==

Series: i915/guc/slpc: Provide sysfs for efficient freq
URL   : https://patchwork.freedesktop.org/series/115975/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12952 -> Patchwork_115975v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (38 -> 37)
------------------------------

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@slpc:
    - bat-adln-1:         NOTRUN -> [DMESG-FAIL][1] ([i915#6997])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-adln-1/igt@i915_selftest@live@slpc.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-adln-1:         NOTRUN -> [SKIP][2] ([i915#7828])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-adln-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][3] -> [FAIL][4] ([i915#7932])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  
#### Possible fixes ####

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-11:         [FAIL][5] ([i915#8308]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/bat-dg2-11/igt@i915_pm_rps@basic-api.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-dg2-11/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][7] ([i915#5334]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-adln-1:         [INCOMPLETE][9] ([i915#4983] / [i915#7609]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/bat-adln-1/igt@i915_selftest@live@gt_lrc.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-adln-1/igt@i915_selftest@live@gt_lrc.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         [DMESG-FAIL][11] ([i915#6367]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/bat-rpls-1/igt@i915_selftest@live@slpc.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-rpls-1/igt@i915_selftest@live@slpc.html

  
#### Warnings ####

  * igt@i915_selftest@live@slpc:
    - bat-rpls-2:         [DMESG-FAIL][13] ([i915#6367] / [i915#7913] / [i915#7996]) -> [DMESG-FAIL][14] ([i915#6367] / [i915#7913])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/bat-rpls-2/igt@i915_selftest@live@slpc.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/bat-rpls-2/igt@i915_selftest@live@slpc.html

  
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
  [i915#8308]: https://gitlab.freedesktop.org/drm/intel/issues/8308


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

  * Linux: CI_DRM_12952 -> Patchwork_115975v1

  CI-20190529: 20190529
  CI_DRM_12952: 51cf6fb5e846c1adbe92debb7282d0dcc3934ecb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7231: 94188a1dc91b6ef1cf3e9df1440ff00b6ff25935 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115975v1: 51cf6fb5e846c1adbe92debb7282d0dcc3934ecb @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

4528960efd95 i915/guc/slpc: Provide sysfs for efficient freq

== Logs ==

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

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

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

* Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
                   ` (2 preceding siblings ...)
  2023-04-01  2:39 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2023-04-01  3:11 ` Dixit, Ashutosh
  2023-04-05 13:57   ` Rodrigo Vivi
  2023-04-02  0:02 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
  4 siblings, 1 reply; 10+ messages in thread
From: Dixit, Ashutosh @ 2023-04-01  3:11 UTC (permalink / raw)
  To: Vinay Belgaumkar; +Cc: intel-gfx, dri-devel, Rodrigo Vivi

On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
>

Hi Vinay,

> @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
>	    val > slpc->max_freq_softlimit)
>		return -EINVAL;
>
> +	/* Ignore efficient freq if lower min freq is requested */
> +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> +	if (ret)
> +		goto out;
> +

I don't agree with this. If we are now providing an interface explicitly to
ignore RPe, that should be /only/ way to ignore RPe. There should be no
other "under the hood" ignoring of RPe. In other words, ignoring RPe should
be minimized unless explicitly requested.

I don't clearly understand why this was done previously but it makes even
less sense to me now after this patch.

Thanks.
--
Ashutosh


>	/* Need a lock now since waitboost can be modifying min as well */
>	mutex_lock(&slpc->lock);
>	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>
> -	/* Ignore efficient freq if lower min freq is requested */
> -	ret = slpc_set_param(slpc,
> -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> -			     val < slpc->rp1_freq);
> -	if (ret) {
> -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> -				ERR_PTR(ret));
> -		goto out;
> -	}
> -
>	ret = slpc_set_param(slpc,
>			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
>			     val);

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

* [Intel-gfx] ✓ Fi.CI.IGT: success for i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
                   ` (3 preceding siblings ...)
  2023-04-01  3:11 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh
@ 2023-04-02  0:02 ` Patchwork
  4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-02  0:02 UTC (permalink / raw)
  To: Vinay Belgaumkar; +Cc: intel-gfx

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

== Series Details ==

Series: i915/guc/slpc: Provide sysfs for efficient freq
URL   : https://patchwork.freedesktop.org/series/115975/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12952_full -> Patchwork_115975v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-deadline:
    - shard-glk:          [PASS][1] -> [FAIL][2] ([i915#2846])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-glk6/igt@gem_exec_fair@basic-deadline.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-glk2/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-none-solo@rcs0:
    - shard-apl:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-apl6/igt@gem_exec_fair@basic-none-solo@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-apl1/igt@gem_exec_fair@basic-none-solo@rcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][5] -> [FAIL][6] ([i915#2842])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-glk4/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-glk7/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-apl:          [PASS][7] -> [ABORT][8] ([i915#5566])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-apl7/igt@gen9_exec_parse@allowed-all.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-apl4/igt@gen9_exec_parse@allowed-all.html

  
#### Possible fixes ####

  * igt@gem_eio@in-flight-contexts-1us:
    - {shard-tglu}:       [TIMEOUT][9] ([i915#3063]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-tglu-9/igt@gem_eio@in-flight-contexts-1us.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-tglu-8/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-tglu}:       [FAIL][11] ([i915#2842]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-tglu-6/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-tglu-5/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
    - shard-apl:          [FAIL][13] ([i915#2842]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-apl4/igt@gem_exec_fair@basic-pace-solo@rcs0.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-apl2/igt@gem_exec_fair@basic-pace-solo@rcs0.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-glk:          [DMESG-FAIL][15] ([i915#5334]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-glk9/igt@i915_selftest@live@gt_heartbeat.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-glk8/igt@i915_selftest@live@gt_heartbeat.html
    - shard-apl:          [DMESG-FAIL][17] ([i915#5334]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-apl6/igt@i915_selftest@live@gt_heartbeat.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-apl2/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2:
    - shard-glk:          [FAIL][19] ([i915#79]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12952/shard-glk7/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_115975v1/shard-glk6/igt@kms_flip@flip-vs-expired-vblank-interruptible@b-hdmi-a2.html

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

  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112283]: https://bugs.freedesktop.org/show_bug.cgi?id=112283
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#4036]: https://gitlab.freedesktop.org/drm/intel/issues/4036
  [i915#404]: https://gitlab.freedesktop.org/drm/intel/issues/404
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4879]: https://gitlab.freedesktop.org/drm/intel/issues/4879
  [i915#4885]: https://gitlab.freedesktop.org/drm/intel/issues/4885
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#5461]: https://gitlab.freedesktop.org/drm/intel/issues/5461
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8234]: https://gitlab.freedesktop.org/drm/intel/issues/8234


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

  * Linux: CI_DRM_12952 -> Patchwork_115975v1

  CI-20190529: 20190529
  CI_DRM_12952: 51cf6fb5e846c1adbe92debb7282d0dcc3934ecb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7231: 94188a1dc91b6ef1cf3e9df1440ff00b6ff25935 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_115975v1: 51cf6fb5e846c1adbe92debb7282d0dcc3934ecb @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

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

* Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-01  3:11 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh
@ 2023-04-05 13:57   ` Rodrigo Vivi
  2023-04-05 19:42     ` Dixit, Ashutosh
  0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Vivi @ 2023-04-05 13:57 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: intel-gfx, dri-devel

On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote:
> On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
> >
> 
> Hi Vinay,
> 
> > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
> >	    val > slpc->max_freq_softlimit)
> >		return -EINVAL;
> >
> > +	/* Ignore efficient freq if lower min freq is requested */
> > +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> > +	if (ret)
> > +		goto out;
> > +
> 
> I don't agree with this. If we are now providing an interface explicitly to
> ignore RPe, that should be /only/ way to ignore RPe. There should be no
> other "under the hood" ignoring of RPe. In other words, ignoring RPe should
> be minimized unless explicitly requested.
> 
> I don't clearly understand why this was done previously but it makes even
> less sense to me now after this patch.

well, I had suggested this previously. And just because without this we would
be breaking API expectations.

When user selects a minimal frequency it expect that to stick. But with the
efficient freq enabled in guc if minimal is less than the efficient one,
this request is likely ignored.

Well, even worse is that we are actually caching the request in the soft values.
So we show a minimal, but the hardware without any workload is operating at
efficient.

So, the thought process was: 'if user requested a very low minimal, we give them
the minimal requested, even if that means to disable the efficient freq.'

So, that was introduced to avoid API breakage. Removing it now would mean
breaking API. (Not sure if the IGT tests for the API got merged already,
but think that as the API contract).

But I do agree with you that having something selected from multiple places
also has the potential to cause some miss-expectations. So I was thinking
about multiple even orders where the user select the RP0 as minimal, then
enable the efficient or vice versa, but I couldn't think of a bad case.
Or at least not as bad as the user asking to get RP0 as minimal and only
getting RPe back.

With this in mind, and having checked the code:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

But I won't push this immediately because I'm still open to hear another
side/angle.

> 
> Thanks.
> --
> Ashutosh
> 
> 
> >	/* Need a lock now since waitboost can be modifying min as well */
> >	mutex_lock(&slpc->lock);
> >	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> >
> > -	/* Ignore efficient freq if lower min freq is requested */
> > -	ret = slpc_set_param(slpc,
> > -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > -			     val < slpc->rp1_freq);
> > -	if (ret) {
> > -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> > -				ERR_PTR(ret));
> > -		goto out;
> > -	}
> > -
> >	ret = slpc_set_param(slpc,
> >			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> >			     val);

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

* Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-05 13:57   ` Rodrigo Vivi
@ 2023-04-05 19:42     ` Dixit, Ashutosh
  2023-04-05 20:12       ` Rodrigo Vivi
  0 siblings, 1 reply; 10+ messages in thread
From: Dixit, Ashutosh @ 2023-04-05 19:42 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel

On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote:
>

Hi Rodrigo,

> On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote:
> > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
> > >
> >
> > Hi Vinay,
> >
> > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
> > >	    val > slpc->max_freq_softlimit)
> > >		return -EINVAL;
> > >
> > > +	/* Ignore efficient freq if lower min freq is requested */
> > > +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> > > +	if (ret)
> > > +		goto out;
> > > +
> >
> > I don't agree with this. If we are now providing an interface explicitly to
> > ignore RPe, that should be /only/ way to ignore RPe. There should be no
> > other "under the hood" ignoring of RPe. In other words, ignoring RPe should
> > be minimized unless explicitly requested.
> >
> > I don't clearly understand why this was done previously but it makes even
> > less sense to me now after this patch.
>
> well, I had suggested this previously. And just because without this we would
> be breaking API expectations.
>
> When user selects a minimal frequency it expect that to stick. But with the
> efficient freq enabled in guc if minimal is less than the efficient one,
> this request is likely ignored.
>
> Well, even worse is that we are actually caching the request in the soft values.
> So we show a minimal, but the hardware without any workload is operating at
> efficient.
>
> So, the thought process was: 'if user requested a very low minimal, we give them
> the minimal requested, even if that means to disable the efficient freq.'

Hmm, I understand this even less now :)

* Why is RPe ignored when min < RPe? Since the freq can be between min and
  max? Shouldn't the condition be min > RPe, that is turn RPe off if min
  higher that RPe is requested?

* Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD?

* Finally, we know that enabling RPe broke the kernel freq API because RPe
  could go over max_freq. So it is actually the max freq which is not
  obeyed after RPe is enabled.

So we ignore RPe in some select cases (which also I don't understand as
mentioned above but maybe I am missing something) to claim that we are
obeying the freq API, but let the freq API stay broken in other cases?

> So, that was introduced to avoid API breakage. Removing it now would mean
> breaking API. (Not sure if the IGT tests for the API got merged already,
> but think that as the API contract).

I think we should take this patch as an opportunity to fix this and give
the user a clean interface to ignore RPe and remove this other implicit way
to ignore RPe. All IGT changes are unmerged at present.

Thanks.
--
Ashutosh



>
> But I do agree with you that having something selected from multiple places
> also has the potential to cause some miss-expectations. So I was thinking
> about multiple even orders where the user select the RP0 as minimal, then
> enable the efficient or vice versa, but I couldn't think of a bad case.
> Or at least not as bad as the user asking to get RP0 as minimal and only
> getting RPe back.
>
> With this in mind, and having checked the code:
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> But I won't push this immediately because I'm still open to hear another
> side/angle.
>
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >
> > >	/* Need a lock now since waitboost can be modifying min as well */
> > >	mutex_lock(&slpc->lock);
> > >	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > >
> > > -	/* Ignore efficient freq if lower min freq is requested */
> > > -	ret = slpc_set_param(slpc,
> > > -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > > -			     val < slpc->rp1_freq);
> > > -	if (ret) {
> > > -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> > > -				ERR_PTR(ret));
> > > -		goto out;
> > > -	}
> > > -
> > >	ret = slpc_set_param(slpc,
> > >			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> > >			     val);

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

* Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-05 19:42     ` Dixit, Ashutosh
@ 2023-04-05 20:12       ` Rodrigo Vivi
  2023-04-06  5:08         ` Dixit, Ashutosh
  0 siblings, 1 reply; 10+ messages in thread
From: Rodrigo Vivi @ 2023-04-05 20:12 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: intel-gfx, dri-devel

On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote:
> On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote:
> >
> 
> Hi Rodrigo,
> 
> > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote:
> > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
> > > >
> > >
> > > Hi Vinay,
> > >
> > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
> > > >	    val > slpc->max_freq_softlimit)
> > > >		return -EINVAL;
> > > >
> > > > +	/* Ignore efficient freq if lower min freq is requested */
> > > > +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > >
> > > I don't agree with this. If we are now providing an interface explicitly to
> > > ignore RPe, that should be /only/ way to ignore RPe. There should be no
> > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should
> > > be minimized unless explicitly requested.
> > >
> > > I don't clearly understand why this was done previously but it makes even
> > > less sense to me now after this patch.
> >
> > well, I had suggested this previously. And just because without this we would
> > be breaking API expectations.
> >
> > When user selects a minimal frequency it expect that to stick. But with the
> > efficient freq enabled in guc if minimal is less than the efficient one,
> > this request is likely ignored.
> >
> > Well, even worse is that we are actually caching the request in the soft values.
> > So we show a minimal, but the hardware without any workload is operating at
> > efficient.
> >
> > So, the thought process was: 'if user requested a very low minimal, we give them
> > the minimal requested, even if that means to disable the efficient freq.'
> 
> Hmm, I understand this even less now :)
> 
> * Why is RPe ignored when min < RPe? Since the freq can be between min and
>   max? Shouldn't the condition be min > RPe, that is turn RPe off if min
>   higher that RPe is requested?

that is not how guc efficient freq selection works. (unless my memory is
tricking me right now.)

So, if we select a min that is between RPe and RP0, guc will respect and
use the selected min. So we don't need to disable guc selection of the
efficient.

This is not true when we select a very low min like RPn. If we select RPn
as min and guc efficient freq selection is enabled guc will simply ignore
our request. So the only way to give the user what is asked, is to also
disable guc's efficient freq selection. (I probably confused you in the
previous email because I used 'RP0' when I meant 'RPn'. I hope it gets
clear now).

> 
> * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD?

Oh... yeap, this is an issue indeed. Specially with i915 where we have
the soft values cached instead of asking guc everytime.

That's a good point. The variance is not big, but we will hit corner cases.
One way is to keep checking and updating everytime a sysfs is touched.
Other way is do what you are suggesting and let's just accept and deal
with the reality that is: "we cannot guarantee a min freq selection if user
doesn't disable the efficient freq selection".

> 
> * Finally, we know that enabling RPe broke the kernel freq API because RPe
>   could go over max_freq. So it is actually the max freq which is not
>   obeyed after RPe is enabled.

Oh! so it was my bad memory indeed and everything is the other way around?
But I just looked to Xe code, my most recent memory, and I just needed
to toggle the efficient freq off on the case that I mentioned, when min
selection is below the efficient one. With that all the API expectation
that I coded in IGT works neatly.

> 
> So we ignore RPe in some select cases (which also I don't understand as
> mentioned above but maybe I am missing something) to claim that we are
> obeying the freq API, but let the freq API stay broken in other cases?

what cases it stays broken? This is why we need the IGT tests for all the
API behavior in place.

> 
> > So, that was introduced to avoid API breakage. Removing it now would mean
> > breaking API. (Not sure if the IGT tests for the API got merged already,
> > but think that as the API contract).
> 
> I think we should take this patch as an opportunity to fix this and give
> the user a clean interface to ignore RPe and remove this other implicit way
> to ignore RPe. All IGT changes are unmerged at present.

Yeap, the IGT needs to come with whatever we concluded here and we need to
stick with that afterwards, so let's think with care.

Vinay, Ashutosh's strongest argument is the variable RPe. Do you have thoughts
on that?

> 
> Thanks.
> --
> Ashutosh
> 
> 
> 
> >
> > But I do agree with you that having something selected from multiple places
> > also has the potential to cause some miss-expectations. So I was thinking
> > about multiple even orders where the user select the RP0 as minimal, then
> > enable the efficient or vice versa, but I couldn't think of a bad case.
> > Or at least not as bad as the user asking to get RP0 as minimal and only
> > getting RPe back.

in case I let you confused here, what I meant was RPn, not RP0.

> >
> > With this in mind, and having checked the code:
> >
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > But I won't push this immediately because I'm still open to hear another
> > side/angle.
> >
> > >
> > > Thanks.
> > > --
> > > Ashutosh
> > >
> > >
> > > >	/* Need a lock now since waitboost can be modifying min as well */
> > > >	mutex_lock(&slpc->lock);
> > > >	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > >
> > > > -	/* Ignore efficient freq if lower min freq is requested */
> > > > -	ret = slpc_set_param(slpc,
> > > > -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > > > -			     val < slpc->rp1_freq);
> > > > -	if (ret) {
> > > > -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> > > > -				ERR_PTR(ret));
> > > > -		goto out;
> > > > -	}
> > > > -
> > > >	ret = slpc_set_param(slpc,
> > > >			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> > > >			     val);

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

* Re: [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq
  2023-04-05 20:12       ` Rodrigo Vivi
@ 2023-04-06  5:08         ` Dixit, Ashutosh
  0 siblings, 0 replies; 10+ messages in thread
From: Dixit, Ashutosh @ 2023-04-06  5:08 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel

On Wed, 05 Apr 2023 13:12:29 -0700, Rodrigo Vivi wrote:
>
> On Wed, Apr 05, 2023 at 12:42:30PM -0700, Dixit, Ashutosh wrote:
> > On Wed, 05 Apr 2023 06:57:42 -0700, Rodrigo Vivi wrote:
> > >

Hi Rodrigo,

> >
> > > On Fri, Mar 31, 2023 at 08:11:29PM -0700, Dixit, Ashutosh wrote:
> > > > On Fri, 31 Mar 2023 19:00:49 -0700, Vinay Belgaumkar wrote:
> > > > >
> > > >
> > > > Hi Vinay,
> > > >
> > > > > @@ -478,20 +507,15 @@ int intel_guc_slpc_set_min_freq(struct intel_guc_slpc *slpc, u32 val)
> > > > >	    val > slpc->max_freq_softlimit)
> > > > >		return -EINVAL;
> > > > >
> > > > > +	/* Ignore efficient freq if lower min freq is requested */
> > > > > +	ret = intel_guc_slpc_set_ignore_eff_freq(slpc, val < slpc->rp1_freq);
> > > > > +	if (ret)
> > > > > +		goto out;
> > > > > +
> > > >
> > > > I don't agree with this. If we are now providing an interface explicitly to
> > > > ignore RPe, that should be /only/ way to ignore RPe. There should be no
> > > > other "under the hood" ignoring of RPe. In other words, ignoring RPe should
> > > > be minimized unless explicitly requested.
> > > >
> > > > I don't clearly understand why this was done previously but it makes even
> > > > less sense to me now after this patch.
> > >
> > > well, I had suggested this previously. And just because without this we would
> > > be breaking API expectations.
> > >
> > > When user selects a minimal frequency it expect that to stick. But with the
> > > efficient freq enabled in guc if minimal is less than the efficient one,
> > > this request is likely ignored.
> > >
> > > Well, even worse is that we are actually caching the request in the soft values.
> > > So we show a minimal, but the hardware without any workload is operating at
> > > efficient.
> > >
> > > So, the thought process was: 'if user requested a very low minimal, we give them
> > > the minimal requested, even if that means to disable the efficient freq.'
> >
> > Hmm, I understand this even less now :)
> >
> > * Why is RPe ignored when min < RPe? Since the freq can be between min and
> >   max? Shouldn't the condition be min > RPe, that is turn RPe off if min
> >   higher that RPe is requested?
>
> that is not how guc efficient freq selection works. (unless my memory is
> tricking me right now.)
>
> So, if we select a min that is between RPe and RP0, guc will respect and
> use the selected min. So we don't need to disable guc selection of the
> efficient.
>
> This is not true when we select a very low min like RPn. If we select RPn
> as min and guc efficient freq selection is enabled guc will simply ignore
> our request. So the only way to give the user what is asked, is to also
> disable guc's efficient freq selection. (I probably confused you in the
> previous email because I used 'RP0' when I meant 'RPn'. I hope it gets
> clear now).
>
> >
> > * Also isn't RPe dynamic, so we can't say RPe == rp1 when using in KMD?
>
> Oh... yeap, this is an issue indeed. Specially with i915 where we have
> the soft values cached instead of asking guc everytime.
>
> That's a good point. The variance is not big, but we will hit corner cases.
> One way is to keep checking and updating everytime a sysfs is touched.

This I believe not possible in all cases. Say the freq's are set through
sysfs first and the workload starts later. In this case RPe will probably
start changing after the workload starts, not when freq's are set in sysfs.

> Other way is do what you are suggesting and let's just accept and deal
> with the reality that is: "we cannot guarantee a min freq selection if user
> doesn't disable the efficient freq selection".
>
> >
> > * Finally, we know that enabling RPe broke the kernel freq API because RPe
> >   could go over max_freq. So it is actually the max freq which is not
> >   obeyed after RPe is enabled.
>
> Oh! so it was my bad memory indeed and everything is the other way around?
> But I just looked to Xe code, my most recent memory, and I just needed
> to toggle the efficient freq off on the case that I mentioned, when min
> selection is below the efficient one. With that all the API expectation
> that I coded in IGT works neatly.

From what I saw the following bugs:

https://gitlab.freedesktop.org/drm/intel/-/issues/6806
https://gitlab.freedesktop.org/drm/intel/-/issues/6786

and the following patches in response to these bugs (as well as the
discussion on these patches):

https://patchwork.freedesktop.org/series/111282/
https://patchwork.freedesktop.org/series/110574/

were all due to the fact that the max freq is not obeyed after RPe is
enabled.

These patches were never merged but I will now try to submit them again
after this ignore efficient freq patch gets reviewed and merged.

Thanks.
--
Ashutosh

> >
> > So we ignore RPe in some select cases (which also I don't understand as
> > mentioned above but maybe I am missing something) to claim that we are
> > obeying the freq API, but let the freq API stay broken in other cases?
>
> what cases it stays broken? This is why we need the IGT tests for all the
> API behavior in place.
>
> > > So, that was introduced to avoid API breakage. Removing it now would mean
> > > breaking API. (Not sure if the IGT tests for the API got merged already,
> > > but think that as the API contract).
> >
> > I think we should take this patch as an opportunity to fix this and give
> > the user a clean interface to ignore RPe and remove this other implicit way
> > to ignore RPe. All IGT changes are unmerged at present.
>
> Yeap, the IGT needs to come with whatever we concluded here and we need to
> stick with that afterwards, so let's think with care.
>
> Vinay, Ashutosh's strongest argument is the variable RPe. Do you have thoughts
> on that?
>
> >
> > Thanks.
> > --
> > Ashutosh
> >
> >
> >
> > >
> > > But I do agree with you that having something selected from multiple places
> > > also has the potential to cause some miss-expectations. So I was thinking
> > > about multiple even orders where the user select the RP0 as minimal, then
> > > enable the efficient or vice versa, but I couldn't think of a bad case.
> > > Or at least not as bad as the user asking to get RP0 as minimal and only
> > > getting RPe back.
>
> in case I let you confused here, what I meant was RPn, not RP0.
>
> > >
> > > With this in mind, and having checked the code:
> > >
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >
> > > But I won't push this immediately because I'm still open to hear another
> > > side/angle.
> > >
> > > >
> > > > Thanks.
> > > > --
> > > > Ashutosh
> > > >
> > > >
> > > > >	/* Need a lock now since waitboost can be modifying min as well */
> > > > >	mutex_lock(&slpc->lock);
> > > > >	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > > > >
> > > > > -	/* Ignore efficient freq if lower min freq is requested */
> > > > > -	ret = slpc_set_param(slpc,
> > > > > -			     SLPC_PARAM_IGNORE_EFFICIENT_FREQUENCY,
> > > > > -			     val < slpc->rp1_freq);
> > > > > -	if (ret) {
> > > > > -		guc_probe_error(slpc_to_guc(slpc), "Failed to toggle efficient freq: %pe\n",
> > > > > -				ERR_PTR(ret));
> > > > > -		goto out;
> > > > > -	}
> > > > > -
> > > > >	ret = slpc_set_param(slpc,
> > > > >			     SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ,
> > > > >			     val);

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

end of thread, other threads:[~2023-04-06  5:08 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-01  2:00 [Intel-gfx] [PATCH] i915/guc/slpc: Provide sysfs for efficient freq Vinay Belgaumkar
2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2023-04-01  2:26 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-04-01  2:39 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-04-01  3:11 ` [Intel-gfx] [PATCH] " Dixit, Ashutosh
2023-04-05 13:57   ` Rodrigo Vivi
2023-04-05 19:42     ` Dixit, Ashutosh
2023-04-05 20:12       ` Rodrigo Vivi
2023-04-06  5:08         ` Dixit, Ashutosh
2023-04-02  0:02 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork

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).