* [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
@ 2023-03-17 10:24 Stanislav Lisovskiy
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3) Patchwork
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Stanislav Lisovskiy @ 2023-03-17 10:24 UTC (permalink / raw)
To: intel-gfx; +Cc: rodrigo.vivi
Display to communicate display pipe count/CDCLK/voltage configuration
to Pcode for more accurate power accounting for gen >= 12.
Existing sequence is only sending the voltage value to the Pcode.
Adding new sequence with current cdclk associate with voltage value masking.
Adding pcode request when any pipe power well will disable or enable.
v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
v3: - Removed redundant return(Jani Nikula)
- Changed intel_cdclk_power_usage_to_pcode_(pre|post)_notification to be
static and also naming to intel_cdclk_pcode_(pre|post)_notify(Jani Nikula)
- Changed u8 to be u16 for cdclk parameter in intel_pcode_notify function,
as according to BSpec it requires 10 bits(Jani Nikula)
- Replaced dev_priv's with i915's(Jani Nikula)
- Simplified expression in intel_cdclk_need_serialize(Jani Nikula)
- Removed redundant kernel-doc and indentation(Jani Nikula)
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 156 +++++++++++++++++++--
drivers/gpu/drm/i915/i915_reg.h | 14 ++
2 files changed, 159 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 084a483f9776..fa739796c701 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
* NOOP - No Pcode communication needed for
* Display versions 14 and beyond
*/;
- else if (DISPLAY_VER(dev_priv) >= 11)
+ else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
cdclk_config->voltage_level);
- else
+ if (DISPLAY_VER(dev_priv) < 11) {
/*
* The timeout isn't specified, the 2ms used here is based on
* experiment.
@@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
HSW_PCODE_DE_WRITE_FREQ_REQ,
cdclk_config->voltage_level,
150, 2);
-
+ }
if (ret) {
drm_err(&dev_priv->drm,
"PCode CDCLK freq set failed, (err %d, freq %d)\n",
@@ -2242,6 +2242,38 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
cdclk_config->voltage_level);
}
+static void intel_pcode_notify(struct drm_i915_private *i915,
+ u8 voltage_level,
+ u8 active_pipe_count,
+ u16 cdclk,
+ bool cdclk_update_valid,
+ bool pipe_count_update_valid)
+{
+ int ret;
+ u32 update_mask = 0;
+
+ if (DISPLAY_VER(i915) < 12)
+ return;
+
+ update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
+
+ if (cdclk_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
+
+ if (pipe_count_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
+
+ ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
+ SKL_CDCLK_PREPARE_FOR_CHANGE |
+ update_mask,
+ SKL_CDCLK_READY_FOR_CHANGE,
+ SKL_CDCLK_READY_FOR_CHANGE, 3);
+ if (ret)
+ drm_err(&i915->drm,
+ "Failed to inform PCU about display config (err %d)\n",
+ ret);
+}
+
/**
* intel_set_cdclk - Push the CDCLK configuration to the hardware
* @dev_priv: i915 device
@@ -2311,6 +2343,88 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
}
}
+static void intel_cdclk_pcode_pre_notify(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool change_cdclk, update_pipe_count;
+
+ if (!intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) &&
+ (new_cdclk_state->active_pipes ==
+ old_cdclk_state->active_pipes))
+ return;
+
+ /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
+ voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
+
+ change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence Before Frequency Change",
+ * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
+ * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
+ * which basically means we choose the maximum of old and new CDCLK, if we know both
+ */
+ if (change_cdclk)
+ cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is increasing, set bits 25:16 to upcoming pipe count
+ * (power well is enabled)
+ * no action if it is decreasing, before the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(i915, voltage_level, num_active_pipes, cdclk,
+ change_cdclk, update_pipe_count);
+}
+
+static void intel_cdclk_pcode_post_notify(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool update_cdclk, update_pipe_count;
+
+ /* According to "Sequence After Frequency Change", set voltage to used level */
+ voltage_level = new_cdclk_state->actual.voltage_level;
+
+ update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence After Frequency Change",
+ * set bits 25:16 to current CDCLK
+ */
+ if (update_cdclk)
+ cdclk = new_cdclk_state->actual.cdclk;
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is decreasing, set bits 25:16 to current pipe count,
+ * after the change(power well is disabled)
+ * no action if it is increasing, after the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(i915, voltage_level, num_active_pipes, cdclk,
+ update_cdclk, update_pipe_count);
+}
+
/**
* intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
* @state: intel atomic state
@@ -2321,7 +2435,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
void
intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2332,11 +2446,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_pcode_pre_notify(state);
+
if (pipe == INVALID_PIPE ||
old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2350,7 +2467,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
void
intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2361,11 +2478,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_pcode_post_notify(state);
+
if (pipe != INVALID_PIPE &&
old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2871,6 +2991,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
return 0;
}
+static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
+ const struct intel_cdclk_state *old_cdclk_state,
+ const struct intel_cdclk_state *new_cdclk_state)
+{
+ bool power_well_cnt_changed = hweight8(old_cdclk_state->active_pipes) !=
+ hweight8(new_cdclk_state->active_pipes);
+ bool cdclk_changed = intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual);
+ /*
+ * We need to poke hw for gen >= 12, because we notify PCode if
+ * pipe power well count changes.
+ */
+ return cdclk_changed || (DISPLAY_VER(i915) >= 12 && power_well_cnt_changed);
+}
+
int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
@@ -2892,8 +3027,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
if (ret)
return ret;
- if (intel_cdclk_changed(&old_cdclk_state->actual,
- &new_cdclk_state->actual)) {
+ if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
/*
* Also serialize commits across all crtcs
* if the actual hw needs to be poked.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9db6b3f06a74..acf8d297605a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6413,6 +6413,20 @@
#define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
#define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
#define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
+#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
+#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
+#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
+#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
+#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
+#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
+#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
+#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
+#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
+ (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
+ (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
+ (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
#define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
#define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
#define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
@ 2023-03-17 16:46 ` Patchwork
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: " Patchwork
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2023-03-17 16:46 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
URL : https://patchwork.freedesktop.org/series/114401/
State : warning
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/114401/revisions/3/mbox/ not applied
Committer identity unknown
*** Please tell me who you are.
Run
git config --global user.email "you@example.com"
git config --global user.name "Your Name"
to set your account's default identity.
Omit --global to set the identity only in this repository.
fatal: unable to auto-detect email address (got 'kbuild2@gfx-ci.(none)')
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3) Patchwork
@ 2023-03-17 16:46 ` Patchwork
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2023-03-17 16:46 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
URL : https://patchwork.freedesktop.org/series/114401/
State : warning
== Summary ==
Error: dim checkpatch failed
da82682afba7 drm/i915/display: Communicate display power demands to pcode more accurately
-:10: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10:
Adding new sequence with current cdclk associate with voltage value masking.
-:83: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#83: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:2273:
+ drm_err(&i915->drm,
+ "Failed to inform PCU about display config (err %d)\n",
-:104: CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'new_cdclk_state->active_pipes ==
old_cdclk_state->active_pipes'
#104: FILE: drivers/gpu/drm/i915/display/intel_cdclk.c:2356:
+ if (!intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) &&
+ (new_cdclk_state->active_pipes ==
+ old_cdclk_state->active_pipes))
-:279: WARNING:LONG_LINE: line length of 101 exceeds 100 columns
#279: FILE: drivers/gpu/drm/i915/i915_reg.h:6424:
+#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
-:281: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parentheses
#281: FILE: drivers/gpu/drm/i915/i915_reg.h:6426:
+#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
+ (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
+ (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
+ (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
total: 1 errors, 2 warnings, 2 checks, 244 lines checked
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3) Patchwork
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: " Patchwork
@ 2023-03-17 16:46 ` Patchwork
2023-03-17 17:12 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-03-17 20:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
4 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2023-03-17 16:46 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
URL : https://patchwork.freedesktop.org/series/114401/
State : warning
== Summary ==
Error: patch https://patchwork.freedesktop.org/api/1.0/series/114401/revisions/3/mbox/ not applied
Committer identity unknown
*** Please tell me who you are.
Run
git config --global user.email "you@example.com"
git config --global user.name "Your Name"
to set your account's default identity.
Omit --global to set the identity only in this repository.
fatal: unable to auto-detect email address (got 'kbuild2@gfx-ci.(none)')
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
` (2 preceding siblings ...)
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
@ 2023-03-17 17:12 ` Patchwork
2023-03-17 20:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
4 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2023-03-17 17:12 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 10650 bytes --]
== Series Details ==
Series: drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
URL : https://patchwork.freedesktop.org/series/114401/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12873 -> Patchwork_114401v3
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/index.html
Participating hosts (35 -> 35)
------------------------------
Additional (2): bat-atsm-1 bat-dg1-6
Missing (2): bat-adlm-1 fi-snb-2520m
Known issues
------------
Here are the changes found in Patchwork_114401v3 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@fbdev@eof:
- bat-atsm-1: NOTRUN -> [SKIP][1] ([i915#2582]) +4 similar issues
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@fbdev@eof.html
* igt@gem_mmap@basic:
- bat-atsm-1: NOTRUN -> [SKIP][2] ([i915#4083])
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@gem_mmap@basic.html
- bat-dg1-6: NOTRUN -> [SKIP][3] ([i915#4083])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@gem_mmap@basic.html
* igt@gem_render_tiled_blits@basic:
- bat-dg1-6: NOTRUN -> [SKIP][4] ([i915#4079]) +1 similar issue
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@gem_render_tiled_blits@basic.html
* igt@gem_sync@basic-each:
- bat-atsm-1: NOTRUN -> [FAIL][5] ([i915#8062]) +1 similar issue
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@gem_sync@basic-each.html
* igt@gem_tiled_fence_blits@basic:
- bat-dg1-6: NOTRUN -> [SKIP][6] ([i915#4077]) +2 similar issues
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@gem_tiled_fence_blits@basic.html
- bat-atsm-1: NOTRUN -> [SKIP][7] ([i915#4077]) +2 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@gem_tiled_fence_blits@basic.html
* igt@gem_tiled_pread_basic:
- bat-atsm-1: NOTRUN -> [SKIP][8] ([i915#4079]) +1 similar issue
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@gem_tiled_pread_basic.html
* igt@i915_hangman@error-state-basic:
- bat-atsm-1: NOTRUN -> [ABORT][9] ([i915#8060])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-atsm-1/igt@i915_hangman@error-state-basic.html
* igt@i915_pm_backlight@basic-brightness:
- bat-dg1-6: NOTRUN -> [SKIP][10] ([i915#7561])
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html
* igt@i915_pm_rps@basic-api:
- bat-dg1-6: NOTRUN -> [SKIP][11] ([i915#6621])
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@i915_pm_rps@basic-api.html
* igt@kms_addfb_basic@basic-y-tiled-legacy:
- bat-dg1-6: NOTRUN -> [SKIP][12] ([i915#4215])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_addfb_basic@basic-y-tiled-legacy.html
* igt@kms_addfb_basic@tile-pitch-mismatch:
- bat-dg1-6: NOTRUN -> [SKIP][13] ([i915#4212]) +7 similar issues
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_addfb_basic@tile-pitch-mismatch.html
* igt@kms_chamelium_hpd@common-hpd-after-suspend:
- bat-dg1-6: NOTRUN -> [SKIP][14] ([i915#7828]) +8 similar issues
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
- fi-bsw-nick: NOTRUN -> [SKIP][15] ([fdo#109271]) +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/fi-bsw-nick/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
- bat-jsl-3: NOTRUN -> [SKIP][16] ([i915#7828])
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-jsl-3/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
- bat-rpls-1: NOTRUN -> [SKIP][17] ([i915#7828])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- bat-dg1-6: NOTRUN -> [SKIP][18] ([i915#4103] / [i915#4213]) +1 similar issue
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_force_connector_basic@force-load-detect:
- bat-dg1-6: NOTRUN -> [SKIP][19] ([fdo#109285])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_force_connector_basic@force-load-detect.html
* igt@kms_pipe_crc_basic@suspend-read-crc:
- bat-rpls-1: NOTRUN -> [SKIP][20] ([i915#1845])
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html
* igt@kms_psr@sprite_plane_onoff:
- bat-dg1-6: NOTRUN -> [SKIP][21] ([i915#1072] / [i915#4078]) +3 similar issues
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_psr@sprite_plane_onoff.html
* igt@kms_setmode@basic-clone-single-crtc:
- bat-dg1-6: NOTRUN -> [SKIP][22] ([i915#3555])
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@kms_setmode@basic-clone-single-crtc.html
* igt@prime_vgem@basic-gtt:
- bat-dg1-6: NOTRUN -> [SKIP][23] ([i915#3708] / [i915#4077]) +1 similar issue
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@prime_vgem@basic-gtt.html
* igt@prime_vgem@basic-read:
- bat-dg1-6: NOTRUN -> [SKIP][24] ([i915#3708]) +3 similar issues
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@prime_vgem@basic-read.html
* igt@prime_vgem@basic-userptr:
- bat-dg1-6: NOTRUN -> [SKIP][25] ([i915#3708] / [i915#4873])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-dg1-6/igt@prime_vgem@basic-userptr.html
#### Possible fixes ####
* igt@i915_selftest@live@execlists:
- fi-bsw-nick: [ABORT][26] ([i915#7911] / [i915#7913]) -> [PASS][27]
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/fi-bsw-nick/igt@i915_selftest@live@execlists.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/fi-bsw-nick/igt@i915_selftest@live@execlists.html
* igt@i915_selftest@live@gem_contexts:
- bat-jsl-3: [INCOMPLETE][28] -> [PASS][29]
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/bat-jsl-3/igt@i915_selftest@live@gem_contexts.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-jsl-3/igt@i915_selftest@live@gem_contexts.html
* igt@i915_selftest@live@hangcheck:
- fi-skl-guc: [DMESG-WARN][30] ([i915#8073]) -> [PASS][31]
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
* igt@i915_selftest@live@reset:
- bat-rpls-1: [ABORT][32] ([i915#4983]) -> [PASS][33]
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/bat-rpls-1/igt@i915_selftest@live@reset.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/bat-rpls-1/igt@i915_selftest@live@reset.html
#### Warnings ####
* igt@i915_selftest@live@execlists:
- fi-bsw-n3050: [ABORT][34] ([i915#7911]) -> [ABORT][35] ([i915#7911] / [i915#7913])
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/fi-bsw-n3050/igt@i915_selftest@live@execlists.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
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
[i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872
[i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
[i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
[i915#8060]: https://gitlab.freedesktop.org/drm/intel/issues/8060
[i915#8062]: https://gitlab.freedesktop.org/drm/intel/issues/8062
[i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073
Build changes
-------------
* Linux: CI_DRM_12873 -> Patchwork_114401v3
CI-20190529: 20190529
CI_DRM_12873: b97925f47e2a20e1b79bc7c8cc236ded1bd431df @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7202: b4ec7dac375eed2dda89c64d4de94c4c9205b601 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_114401v3: b97925f47e2a20e1b79bc7c8cc236ded1bd431df @ git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
f86f0e0015e0 drm/i915/display: Communicate display power demands to pcode more accurately
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/index.html
[-- Attachment #2: Type: text/html, Size: 12649 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
` (3 preceding siblings ...)
2023-03-17 17:12 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2023-03-17 20:03 ` Patchwork
4 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2023-03-17 20:03 UTC (permalink / raw)
To: Stanislav Lisovskiy; +Cc: intel-gfx
[-- Attachment #1: Type: text/plain, Size: 23179 bytes --]
== Series Details ==
Series: drm/i915/display: Communicate display power demands to pcode more accurately (rev3)
URL : https://patchwork.freedesktop.org/series/114401/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_12873_full -> Patchwork_114401v3_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Participating hosts (7 -> 8)
------------------------------
Additional (1): shard-rkl0
Known issues
------------
Here are the changes found in Patchwork_114401v3_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_fair@basic-none-rrul@rcs0:
- shard-glk: [PASS][1] -> [FAIL][2] ([i915#2842])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-glk7/igt@gem_exec_fair@basic-none-rrul@rcs0.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-glk2/igt@gem_exec_fair@basic-none-rrul@rcs0.html
* igt@gen9_exec_parse@allowed-single:
- shard-apl: [PASS][3] -> [ABORT][4] ([i915#5566])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-apl6/igt@gen9_exec_parse@allowed-single.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-apl7/igt@gen9_exec_parse@allowed-single.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
- shard-glk: [PASS][5] -> [FAIL][6] ([i915#2346])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-glk5/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-glk1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
- shard-apl: [PASS][7] -> [FAIL][8] ([i915#2346]) +1 similar issue
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-apl3/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
* igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1:
- shard-glk: [PASS][9] -> [FAIL][10] ([i915#79])
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-glk6/igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-glk8/igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1.html
#### Possible fixes ####
* igt@fbdev@eof:
- {shard-rkl}: [SKIP][11] ([i915#2582]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-2/igt@fbdev@eof.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@fbdev@eof.html
* igt@fbdev@info:
- {shard-tglu}: [SKIP][13] ([i915#2582]) -> [PASS][14] +1 similar issue
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@fbdev@info.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@fbdev@info.html
* igt@gem_eio@in-flight-suspend:
- {shard-rkl}: [FAIL][15] ([fdo#103375]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-3/igt@gem_eio@in-flight-suspend.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-2/igt@gem_eio@in-flight-suspend.html
* igt@gem_exec_fair@basic-deadline:
- shard-glk: [FAIL][17] ([i915#2846]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-glk4/igt@gem_exec_fair@basic-deadline.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-glk3/igt@gem_exec_fair@basic-deadline.html
* igt@gem_exec_fair@basic-none-rrul@rcs0:
- {shard-rkl}: [FAIL][19] ([i915#2842]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-3/igt@gem_exec_fair@basic-none-rrul@rcs0.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-5/igt@gem_exec_fair@basic-none-rrul@rcs0.html
* igt@gem_exec_fair@basic-throttle@rcs0:
- shard-glk: [FAIL][21] ([i915#2842]) -> [PASS][22] +1 similar issue
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-glk3/igt@gem_exec_fair@basic-throttle@rcs0.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-glk6/igt@gem_exec_fair@basic-throttle@rcs0.html
* igt@gem_exec_reloc@basic-concurrent16:
- {shard-rkl}: [SKIP][23] ([i915#3281]) -> [PASS][24]
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-3/igt@gem_exec_reloc@basic-concurrent16.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-5/igt@gem_exec_reloc@basic-concurrent16.html
* igt@gem_lmem_swapping@heavy-verify-multi@lmem0:
- {shard-dg1}: [DMESG-WARN][25] ([i915#4391]) -> [PASS][26]
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-dg1-13/igt@gem_lmem_swapping@heavy-verify-multi@lmem0.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-dg1-17/igt@gem_lmem_swapping@heavy-verify-multi@lmem0.html
* igt@gem_lmem_swapping@verify@lmem0:
- {shard-dg1}: [DMESG-WARN][27] -> [PASS][28]
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-dg1-13/igt@gem_lmem_swapping@verify@lmem0.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-dg1-17/igt@gem_lmem_swapping@verify@lmem0.html
* igt@i915_hangman@engine-engine-error@bcs0:
- {shard-rkl}: [SKIP][29] ([i915#6258]) -> [PASS][30]
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-5/igt@i915_hangman@engine-engine-error@bcs0.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-3/igt@i915_hangman@engine-engine-error@bcs0.html
* igt@i915_module_load@reload-with-fault-injection:
- {shard-tglu}: [DMESG-WARN][31] ([i915#2867]) -> [PASS][32]
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-2/igt@i915_module_load@reload-with-fault-injection.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-10/igt@i915_module_load@reload-with-fault-injection.html
* igt@i915_pm_dc@dc9-dpms:
- {shard-rkl}: [SKIP][33] ([i915#3361]) -> [PASS][34]
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-5/igt@i915_pm_dc@dc9-dpms.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-3/igt@i915_pm_dc@dc9-dpms.html
* igt@i915_pm_rpm@dpms-mode-unset-lpsp:
- {shard-rkl}: [SKIP][35] ([i915#1397]) -> [PASS][36] +1 similar issue
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-4/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@i915_pm_rpm@dpms-mode-unset-lpsp.html
* igt@i915_pm_rpm@drm-resources-equal:
- {shard-tglu}: [SKIP][37] ([i915#3547]) -> [PASS][38]
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@i915_pm_rpm@drm-resources-equal.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@i915_pm_rpm@drm-resources-equal.html
* igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait:
- {shard-dg1}: [SKIP][39] ([i915#1397]) -> [PASS][40]
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-dg1-14/igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-dg1-15/igt@i915_pm_rpm@modeset-non-lpsp-stress-no-wait.html
* igt@i915_selftest@live@gt_heartbeat:
- shard-apl: [DMESG-FAIL][41] ([i915#5334]) -> [PASS][42]
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-apl2/igt@i915_selftest@live@gt_heartbeat.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-apl4/igt@i915_selftest@live@gt_heartbeat.html
* igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-async-flip:
- {shard-rkl}: [SKIP][43] ([i915#1845] / [i915#4098]) -> [PASS][44] +17 similar issues
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-2/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@kms_big_fb@x-tiled-max-hw-stride-32bpp-rotate-180-async-flip.html
* igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs:
- {shard-tglu}: [SKIP][45] ([i915#1845]) -> [PASS][46] +34 similar issues
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-10/igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-3/igt@kms_ccs@pipe-c-crc-primary-rotation-180-y_tiled_gen12_rc_ccs.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible@a-vga1:
- shard-snb: [FAIL][47] ([i915#79]) -> [PASS][48]
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-snb7/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-vga1.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-snb2/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-vga1.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt:
- {shard-tglu}: [SKIP][49] ([i915#1849]) -> [PASS][50] +14 similar issues
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-plflip-blt.html
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt:
- {shard-rkl}: [SKIP][51] ([i915#1849] / [i915#4098]) -> [PASS][52] +8 similar issues
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-2/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt.html
* igt@kms_plane@plane-position-hole-dpms@pipe-a-planes:
- {shard-tglu}: [SKIP][53] ([i915#1849] / [i915#3558]) -> [PASS][54] +3 similar issues
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-10/igt@kms_plane@plane-position-hole-dpms@pipe-a-planes.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-3/igt@kms_plane@plane-position-hole-dpms@pipe-a-planes.html
* igt@kms_psr@dpms:
- {shard-rkl}: [SKIP][55] ([i915#1072]) -> [PASS][56]
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-4/igt@kms_psr@dpms.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@kms_psr@dpms.html
* igt@kms_universal_plane@disable-primary-vs-flip-pipe-a:
- {shard-tglu}: [SKIP][57] ([fdo#109274]) -> [PASS][58] +3 similar issues
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html
- {shard-rkl}: [SKIP][59] ([i915#1845] / [i915#4070] / [i915#4098]) -> [PASS][60]
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-2/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@kms_universal_plane@disable-primary-vs-flip-pipe-a.html
* igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b:
- {shard-rkl}: [SKIP][61] ([i915#4098]) -> [PASS][62]
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-4/igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@kms_universal_plane@universal-plane-pageflip-windowed-pipe-b.html
* igt@kms_vblank@pipe-d-wait-forked-busy:
- {shard-tglu}: [SKIP][63] ([i915#1845] / [i915#7651]) -> [PASS][64] +21 similar issues
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@kms_vblank@pipe-d-wait-forked-busy.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@kms_vblank@pipe-d-wait-forked-busy.html
* igt@perf_pmu@idle@rcs0:
- {shard-rkl}: [FAIL][65] ([i915#4349]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-1/igt@perf_pmu@idle@rcs0.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-3/igt@perf_pmu@idle@rcs0.html
* igt@prime_vgem@basic-fence-flip:
- {shard-tglu}: [SKIP][67] ([fdo#109295]) -> [PASS][68]
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-tglu-9/igt@prime_vgem@basic-fence-flip.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-tglu-2/igt@prime_vgem@basic-fence-flip.html
- {shard-rkl}: [SKIP][69] ([fdo#109295] / [i915#3708] / [i915#4098]) -> [PASS][70]
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-2/igt@prime_vgem@basic-fence-flip.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-6/igt@prime_vgem@basic-fence-flip.html
* igt@prime_vgem@basic-write:
- {shard-rkl}: [SKIP][71] ([fdo#109295] / [i915#3291] / [i915#3708]) -> [PASS][72]
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12873/shard-rkl-3/igt@prime_vgem@basic-write.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114401v3/shard-rkl-5/igt@prime_vgem@basic-write.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
[fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
[fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
[fdo#109291]: https://bugs.freedesktop.org/show_bug.cgi?id=109291
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#109303]: https://bugs.freedesktop.org/show_bug.cgi?id=109303
[fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
[fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
[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#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
[i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
[i915#132]: https://gitlab.freedesktop.org/drm/intel/issues/132
[i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
[i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
[i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
[i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
[i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
[i915#1850]: https://gitlab.freedesktop.org/drm/intel/issues/1850
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2433]: https://gitlab.freedesktop.org/drm/intel/issues/2433
[i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437
[i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
[i915#2532]: https://gitlab.freedesktop.org/drm/intel/issues/2532
[i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
[i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
[i915#2658]: https://gitlab.freedesktop.org/drm/intel/issues/2658
[i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
[i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
[i915#2705]: https://gitlab.freedesktop.org/drm/intel/issues/2705
[i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
[i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
[i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
[i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
[i915#2920]: https://gitlab.freedesktop.org/drm/intel/issues/2920
[i915#3116]: https://gitlab.freedesktop.org/drm/intel/issues/3116
[i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
[i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
[i915#3299]: https://gitlab.freedesktop.org/drm/intel/issues/3299
[i915#3318]: https://gitlab.freedesktop.org/drm/intel/issues/3318
[i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
[i915#3361]: https://gitlab.freedesktop.org/drm/intel/issues/3361
[i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
[i915#3528]: https://gitlab.freedesktop.org/drm/intel/issues/3528
[i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
[i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
[i915#3547]: https://gitlab.freedesktop.org/drm/intel/issues/3547
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3558]: https://gitlab.freedesktop.org/drm/intel/issues/3558
[i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
[i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
[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#3734]: https://gitlab.freedesktop.org/drm/intel/issues/3734
[i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
[i915#3840]: https://gitlab.freedesktop.org/drm/intel/issues/3840
[i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
[i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952
[i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
[i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
[i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
[i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
[i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
[i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565
[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#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
[i915#4885]: https://gitlab.freedesktop.org/drm/intel/issues/4885
[i915#5115]: https://gitlab.freedesktop.org/drm/intel/issues/5115
[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#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
[i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
[i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
[i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
[i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
[i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
[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#6247]: https://gitlab.freedesktop.org/drm/intel/issues/6247
[i915#6248]: https://gitlab.freedesktop.org/drm/intel/issues/6248
[i915#6258]: https://gitlab.freedesktop.org/drm/intel/issues/6258
[i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
[i915#6497]: https://gitlab.freedesktop.org/drm/intel/issues/6497
[i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
[i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
[i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
[i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
[i915#7037]: https://gitlab.freedesktop.org/drm/intel/issues/7037
[i915#7052]: https://gitlab.freedesktop.org/drm/intel/issues/7052
[i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
[i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
[i915#7128]: https://gitlab.freedesktop.org/drm/intel/issues/7128
[i915#7294]: https://gitlab.freedesktop.org/drm/intel/issues/7294
[i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
[i915#7651]: https://gitlab.freedesktop.org/drm/intel/issues/7651
[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#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
[i915#7811]: https://gitlab.freedesktop.org/drm/intel/issues/7811
[i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#8152]: https://gitlab.freedesktop.org/drm/intel/issues/8152
[i915#8228]: https://gitlab.freedesktop.org/drm/intel/issues/8228
[i915#8282]: https://gitlab.freedesktop.org/drm/intel/issues/8282
Build changes
-------------
* Linux: CI_DRM_12873 -> Patchwork_114401v3
CI-20190529: 20190529
CI_DRM_12873: b97925f47e2a20e1b79bc7c8cc236ded1bd431df @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_7202: b4ec7dac375eed2dda89c64d4de94c4c9205b601 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_114401v3: b97925f47e2a20e1b79bc7c8cc236ded1bd431df @ 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_114401v3/index.html
[-- Attachment #2: Type: text/html, Size: 19269 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-03-07 11:15 ` Lisovskiy, Stanislav
@ 2023-03-07 11:52 ` Jani Nikula
0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2023-03-07 11:52 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: intel-gfx, rodrigo.vivi
On Tue, 07 Mar 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote:
> On Tue, Mar 07, 2023 at 12:23:59PM +0200, Jani Nikula wrote:
>> On Tue, 07 Mar 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote:
>> > On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
>> >> On Mon, 27 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
>> >> > Display to communicate display pipe count/CDCLK/voltage configuration
>> >> > to Pcode for more accurate power accounting for gen >= 12.
>> >> > Existing sequence is only sending the voltage value to the Pcode.
>> >> > Adding new sequence with current cdclk associate with voltage value masking.
>> >> > Adding pcode request when any pipe power well will disable or enable.
>> >> >
>> >> > v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
>> >> >
>> >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> >> > ---
>> >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
>> >> > drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
>> >> > drivers/gpu/drm/i915/i915_reg.h | 14 ++
>> >> > 3 files changed, 176 insertions(+), 14 deletions(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> > index 084a483f9776..df5a21f6a393 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> >> > @@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> >> > * NOOP - No Pcode communication needed for
>> >> > * Display versions 14 and beyond
>> >> > */;
>> >> > - else if (DISPLAY_VER(dev_priv) >= 11)
>> >> > + else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
>> >> > ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
>> >> > cdclk_config->voltage_level);
>> >> > - else
>> >> > + if (DISPLAY_VER(dev_priv) < 11) {
>> >> > /*
>> >> > * The timeout isn't specified, the 2ms used here is based on
>> >> > * experiment.
>> >> > @@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> >> > HSW_PCODE_DE_WRITE_FREQ_REQ,
>> >> > cdclk_config->voltage_level,
>> >> > 150, 2);
>> >> > -
>> >> > + }
>> >> > if (ret) {
>> >> > drm_err(&dev_priv->drm,
>> >> > "PCode CDCLK freq set failed, (err %d, freq %d)\n",
>> >> > @@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
>> >> > cdclk_config->voltage_level);
>> >> > }
>> >> >
>> >> > +static void intel_pcode_notify(struct drm_i915_private *i915,
>> >> > + u8 voltage_level,
>> >> > + u8 active_pipe_count,
>> >> > + u8 cdclk,
>>
>> How can that be u8?
>
> Agree, cdclk uses 10 bits according to BSpec, for the rest u8 is enough.
> Thanks for noticing.
>
>>
>> >> > + bool cdclk_update_valid,
>> >> > + bool pipe_count_update_valid)
>>
>> Also not super happy about the flags arguments... could have cdclk == 0
>> means not valid and int active_pipe_count == -1 means not valid. Maybe.
>
> Those are actually called that way, because BSpec calls those bits so.
> In fact it is more like "should we update this" flag.
> As I understand if it is not set PCode will simply ignore those bits in
> notification.
>
> The calling site should determine whether this bit need to be set or not.
> We set the to true only if correspodent state got changed.
> I think those are validated already, so we shouldn't expect any bogus values
> there(otherwise we are screwed in quite many other places), however cdclk == 0
> actually would be valid and this bit would be then set.
> Active pipe count is always amount of "1"'s in cdclk_state->active_pipes mask, so it shouldn't
> ever become -1. Or if it becomes, once again, this function would be the least
> of our problems.
>
>>
>> >> > +{
>> >> > + int ret;
>> >> > + u32 update_mask = 0;
>> >> > +
>> >> > + if (DISPLAY_VER(i915) < 12)
>> >> > + return;
>> >> > +
>> >> > + update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
>> >> > +
>> >> > + if (cdclk_update_valid)
>> >> > + update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
>> >> > +
>> >> > + if (pipe_count_update_valid)
>> >> > + update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
>> >> > +
>> >> > + ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
>> >> > + SKL_CDCLK_PREPARE_FOR_CHANGE |
>> >> > + update_mask,
>> >> > + SKL_CDCLK_READY_FOR_CHANGE,
>> >> > + SKL_CDCLK_READY_FOR_CHANGE, 3);
>> >> > + if (ret) {
>> >> > + drm_err(&i915->drm,
>> >> > + "Failed to inform PCU about display config (err %d)\n",
>> >> > + ret);
>> >> > + return;
>> >>
>> >> Superfluous return.
>> >>
>> >> > + }
>> >> > +}
>> >> > +
>> >> > /**
>> >> > * intel_set_cdclk - Push the CDCLK configuration to the hardware
>> >> > * @dev_priv: i915 device
>> >> > @@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>> >> > }
>> >> > }
>> >> >
>> >> > +/**
>> >> > + * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
>>
>> This doesn't need to be a kernel-doc comment (and it's a malformed one
>> at that). Ditto for the others.
>>
>> >> > + * before the enabling power wells.
>> >> > + * send notification with cdclk, number of pipes, voltage_level.
>> >> > + * @state: intel atomic state
>> >> > + */
>> >> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
>> >>
>> >> This could be static?
>> >>
>> >> Not happy with the name, really, but at least it could use a
>> >> verb. Notify instead of notification.
>> >
>> > Any hint for better naming?
>>
>> Maybe just intel_cdclk_pcode_pre_notify(),
>> intel_cdclk_pcode_post_notify(), and then intel_cdclk_pcode_notify() as
>> the shared thing?
>
> Sorry Jani, but I think we would be going in circles then, because initially you were stating that:
>
> "The function name should indicate what is being
> notified. Pre-notification of what? Post-notification of what?
>
> Functions in this file should be prefixed intel_cdclk_*."
>
> At this point it was called "intel_display_to_pcode_pre_notification", the prefix comment
> of course is now changed.
>
> So if I change it back to "pcode_pre_notify" thing, that would lose information
> about what its doing once again.
>
> I can for sure do that, but then what is the guarantee it won't get a comment about
> "lack of informativeness" once again? :)
*shrug* :)
Compared to the original, there's now "cdclk" in the name.
BR,
Jani.
>
> Stan
>
>>
>> >
>> >>
>> >> > +{
>> >> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >>
>> >> No new dev_priv please.
>> >>
>> >> > + const struct intel_cdclk_state *old_cdclk_state =
>> >> > + intel_atomic_get_old_cdclk_state(state);
>> >> > + const struct intel_cdclk_state *new_cdclk_state =
>> >> > + intel_atomic_get_new_cdclk_state(state);
>> >> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
>> >> > + bool change_cdclk, update_pipe_count;
>> >> > +
>> >> > + if (!intel_cdclk_changed(&old_cdclk_state->actual,
>> >> > + &new_cdclk_state->actual) &&
>> >> > + (new_cdclk_state->active_pipes ==
>> >> > + old_cdclk_state->active_pipes))
>> >> > + return;
>> >> > +
>> >> > + /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
>> >> > + voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
>> >> > +
>> >> > + change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
>> >> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
>> >> > + hweight8(old_cdclk_state->active_pipes);
>> >> > +
>> >> > + /*
>> >> > + * According to "Sequence Before Frequency Change",
>> >> > + * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
>> >> > + * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
>> >> > + * which basically means we choose the maximum of old and new CDCLK, if we know both
>> >> > + */
>> >> > + if (change_cdclk)
>> >> > + cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
>> >> > +
>> >> > + /*
>> >> > + * According to "Sequence For Pipe Count Change",
>> >> > + * if pipe count is increasing, set bits 25:16 to upcoming pipe count
>> >> > + * (power well is enabled)
>> >> > + * no action if it is decreasing, before the change
>> >> > + */
>> >> > + if (update_pipe_count)
>> >> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
>> >> > +
>> >> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
>> >> > + change_cdclk, update_pipe_count);
>> >> > +}
>> >> > +
>> >> > +/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
>> >> > + * voltage_level, active pipes, current CDCLK frequency.
>> >> > + * @state: intel atomic state
>> >> > + */
>> >> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
>> >>
>> >> Ditto about static & naming.
>> >>
>> >> > +{
>> >> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >>
>> >> No new dev_priv please.
>> >>
>> >> > + const struct intel_cdclk_state *new_cdclk_state =
>> >> > + intel_atomic_get_new_cdclk_state(state);
>> >> > + const struct intel_cdclk_state *old_cdclk_state =
>> >> > + intel_atomic_get_old_cdclk_state(state);
>> >> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
>> >> > + bool update_cdclk, update_pipe_count;
>> >> > +
>> >> > + /* According to "Sequence After Frequency Change", set voltage to used level */
>> >> > + voltage_level = new_cdclk_state->actual.voltage_level;
>> >> > +
>> >> > + update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
>> >> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
>> >> > + hweight8(old_cdclk_state->active_pipes);
>> >> > +
>> >> > + /*
>> >> > + * According to "Sequence After Frequency Change",
>> >> > + * set bits 25:16 to current CDCLK
>> >> > + */
>> >> > + if (update_cdclk)
>> >> > + cdclk = new_cdclk_state->actual.cdclk;
>> >> > +
>> >> > + /*
>> >> > + * According to "Sequence For Pipe Count Change",
>> >> > + * if pipe count is decreasing, set bits 25:16 to current pipe count,
>> >> > + * after the change(power well is disabled)
>> >> > + * no action if it is increasing, after the change
>> >> > + */
>> >> > + if (update_pipe_count)
>> >> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
>> >> > +
>> >> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
>> >> > + update_cdclk, update_pipe_count);
>> >> > +}
>> >> > +
>> >> > /**
>> >> > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
>> >> > * @state: intel atomic state
>> >> > @@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>> >> > void
>> >> > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> >> > {
>> >> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
>> >> > const struct intel_cdclk_state *old_cdclk_state =
>> >> > intel_atomic_get_old_cdclk_state(state);
>> >> > const struct intel_cdclk_state *new_cdclk_state =
>> >> > @@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> >> > &new_cdclk_state->actual))
>> >> > return;
>> >> >
>> >> > + if (DISPLAY_VER(i915) >= 12)
>> >> > + intel_cdclk_power_usage_to_pcode_pre_notification(state);
>> >> > +
>> >> > if (pipe == INVALID_PIPE ||
>> >> > old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>> >> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
>> >> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>> >> >
>> >> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
>> >> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>> >> > }
>> >> > }
>> >> >
>> >> > @@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> >> > void
>> >> > intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>> >> > {
>> >> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
>> >> > const struct intel_cdclk_state *old_cdclk_state =
>> >> > intel_atomic_get_old_cdclk_state(state);
>> >> > const struct intel_cdclk_state *new_cdclk_state =
>> >> > @@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>> >> > &new_cdclk_state->actual))
>> >> > return;
>> >> >
>> >> > + if (DISPLAY_VER(i915) >= 12)
>> >> > + intel_cdclk_power_usage_to_pcode_post_notification(state);
>> >> > +
>> >> > if (pipe != INVALID_PIPE &&
>> >> > old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>> >> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
>> >> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>> >> >
>> >> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
>> >> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>> >> > }
>> >> > }
>> >> >
>> >> > @@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
>> >> > return 0;
>> >> > }
>> >> >
>> >> > +static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
>> >> > + const struct intel_cdclk_state *old_cdclk_state,
>> >> > + const struct intel_cdclk_state *new_cdclk_state)
>> >> > +{
>> >> > + /*
>> >> > + * We need to poke hw for gen >= 12, because we notify PCode if
>> >> > + * pipe power well count changes.
>> >> > + */
>> >> > + return intel_cdclk_changed(&old_cdclk_state->actual,
>> >> > + &new_cdclk_state->actual) ||
>> >> > + ((DISPLAY_VER(i915) >= 12 &&
>> >> > + hweight8(old_cdclk_state->active_pipes) !=
>> >> > + hweight8(new_cdclk_state->active_pipes)));
>> >>
>> >> That's getting a bit unweildy for one expression.
>> >>
>> >> > +}
>> >> > +
>> >> > int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> >> > {
>> >> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> >> > @@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> >> > if (ret)
>> >> > return ret;
>> >> >
>> >> > - if (intel_cdclk_changed(&old_cdclk_state->actual,
>> >> > - &new_cdclk_state->actual)) {
>> >> > + if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
>> >> > /*
>> >> > * Also serialize commits across all crtcs
>> >> > * if the actual hw needs to be poked.
>> >> > @@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> >> > old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
>> >> > intel_cdclk_changed(&old_cdclk_state->logical,
>> >> > &new_cdclk_state->logical)) {
>> >> > - ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
>> >> > - if (ret)
>> >> > - return ret;
>> >> > + ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
>> >> > + if (ret)
>> >> > + return ret;
>>
>> Accidental indentation change?
>>
>> >> > } else {
>> >> > return 0;
>> >> > }
>> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> >> > index 51e2f6a11ce4..fa356adc61d9 100644
>> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
>> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> >> > @@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> >> > const struct intel_cdclk_config *b);
>> >> > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
>> >> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
>> >> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
>> >> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
>> >> > void intel_cdclk_dump_config(struct drm_i915_private *i915,
>> >> > const struct intel_cdclk_config *cdclk_config,
>> >> > const char *context);
>> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> >> > index c1efa655fb68..9f786952585b 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> > @@ -6526,6 +6526,20 @@
>> >> > #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
>> >> > #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
>> >> > #define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
>> >> > +#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
>> >> > +#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
>> >> > +#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
>> >> > +#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
>> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
>> >> > +#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
>> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
>> >> > +#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
>> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
>> >> > +#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
>> >> > +#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
>> >> > + (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
>> >> > + (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
>> >> > + (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
>> >> > #define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
>> >> > #define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
>> >> > #define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Graphics Center
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-03-07 10:23 ` Jani Nikula
@ 2023-03-07 11:15 ` Lisovskiy, Stanislav
2023-03-07 11:52 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Lisovskiy, Stanislav @ 2023-03-07 11:15 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, rodrigo.vivi
On Tue, Mar 07, 2023 at 12:23:59PM +0200, Jani Nikula wrote:
> On Tue, 07 Mar 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote:
> > On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
> >> On Mon, 27 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> >> > Display to communicate display pipe count/CDCLK/voltage configuration
> >> > to Pcode for more accurate power accounting for gen >= 12.
> >> > Existing sequence is only sending the voltage value to the Pcode.
> >> > Adding new sequence with current cdclk associate with voltage value masking.
> >> > Adding pcode request when any pipe power well will disable or enable.
> >> >
> >> > v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
> >> >
> >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >> > ---
> >> > drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
> >> > drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
> >> > drivers/gpu/drm/i915/i915_reg.h | 14 ++
> >> > 3 files changed, 176 insertions(+), 14 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > index 084a483f9776..df5a21f6a393 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> > @@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >> > * NOOP - No Pcode communication needed for
> >> > * Display versions 14 and beyond
> >> > */;
> >> > - else if (DISPLAY_VER(dev_priv) >= 11)
> >> > + else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
> >> > ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> >> > cdclk_config->voltage_level);
> >> > - else
> >> > + if (DISPLAY_VER(dev_priv) < 11) {
> >> > /*
> >> > * The timeout isn't specified, the 2ms used here is based on
> >> > * experiment.
> >> > @@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> >> > HSW_PCODE_DE_WRITE_FREQ_REQ,
> >> > cdclk_config->voltage_level,
> >> > 150, 2);
> >> > -
> >> > + }
> >> > if (ret) {
> >> > drm_err(&dev_priv->drm,
> >> > "PCode CDCLK freq set failed, (err %d, freq %d)\n",
> >> > @@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >> > cdclk_config->voltage_level);
> >> > }
> >> >
> >> > +static void intel_pcode_notify(struct drm_i915_private *i915,
> >> > + u8 voltage_level,
> >> > + u8 active_pipe_count,
> >> > + u8 cdclk,
>
> How can that be u8?
Agree, cdclk uses 10 bits according to BSpec, for the rest u8 is enough.
Thanks for noticing.
>
> >> > + bool cdclk_update_valid,
> >> > + bool pipe_count_update_valid)
>
> Also not super happy about the flags arguments... could have cdclk == 0
> means not valid and int active_pipe_count == -1 means not valid. Maybe.
Those are actually called that way, because BSpec calls those bits so.
In fact it is more like "should we update this" flag.
As I understand if it is not set PCode will simply ignore those bits in
notification.
The calling site should determine whether this bit need to be set or not.
We set the to true only if correspodent state got changed.
I think those are validated already, so we shouldn't expect any bogus values
there(otherwise we are screwed in quite many other places), however cdclk == 0
actually would be valid and this bit would be then set.
Active pipe count is always amount of "1"'s in cdclk_state->active_pipes mask, so it shouldn't
ever become -1. Or if it becomes, once again, this function would be the least
of our problems.
>
> >> > +{
> >> > + int ret;
> >> > + u32 update_mask = 0;
> >> > +
> >> > + if (DISPLAY_VER(i915) < 12)
> >> > + return;
> >> > +
> >> > + update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
> >> > +
> >> > + if (cdclk_update_valid)
> >> > + update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
> >> > +
> >> > + if (pipe_count_update_valid)
> >> > + update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
> >> > +
> >> > + ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> >> > + SKL_CDCLK_PREPARE_FOR_CHANGE |
> >> > + update_mask,
> >> > + SKL_CDCLK_READY_FOR_CHANGE,
> >> > + SKL_CDCLK_READY_FOR_CHANGE, 3);
> >> > + if (ret) {
> >> > + drm_err(&i915->drm,
> >> > + "Failed to inform PCU about display config (err %d)\n",
> >> > + ret);
> >> > + return;
> >>
> >> Superfluous return.
> >>
> >> > + }
> >> > +}
> >> > +
> >> > /**
> >> > * intel_set_cdclk - Push the CDCLK configuration to the hardware
> >> > * @dev_priv: i915 device
> >> > @@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >> > }
> >> > }
> >> >
> >> > +/**
> >> > + * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
>
> This doesn't need to be a kernel-doc comment (and it's a malformed one
> at that). Ditto for the others.
>
> >> > + * before the enabling power wells.
> >> > + * send notification with cdclk, number of pipes, voltage_level.
> >> > + * @state: intel atomic state
> >> > + */
> >> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
> >>
> >> This could be static?
> >>
> >> Not happy with the name, really, but at least it could use a
> >> verb. Notify instead of notification.
> >
> > Any hint for better naming?
>
> Maybe just intel_cdclk_pcode_pre_notify(),
> intel_cdclk_pcode_post_notify(), and then intel_cdclk_pcode_notify() as
> the shared thing?
Sorry Jani, but I think we would be going in circles then, because initially you were stating that:
"The function name should indicate what is being
notified. Pre-notification of what? Post-notification of what?
Functions in this file should be prefixed intel_cdclk_*."
At this point it was called "intel_display_to_pcode_pre_notification", the prefix comment
of course is now changed.
So if I change it back to "pcode_pre_notify" thing, that would lose information
about what its doing once again.
I can for sure do that, but then what is the guarantee it won't get a comment about
"lack of informativeness" once again? :)
Stan
>
> >
> >>
> >> > +{
> >> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >>
> >> No new dev_priv please.
> >>
> >> > + const struct intel_cdclk_state *old_cdclk_state =
> >> > + intel_atomic_get_old_cdclk_state(state);
> >> > + const struct intel_cdclk_state *new_cdclk_state =
> >> > + intel_atomic_get_new_cdclk_state(state);
> >> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> >> > + bool change_cdclk, update_pipe_count;
> >> > +
> >> > + if (!intel_cdclk_changed(&old_cdclk_state->actual,
> >> > + &new_cdclk_state->actual) &&
> >> > + (new_cdclk_state->active_pipes ==
> >> > + old_cdclk_state->active_pipes))
> >> > + return;
> >> > +
> >> > + /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
> >> > + voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
> >> > +
> >> > + change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> >> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
> >> > + hweight8(old_cdclk_state->active_pipes);
> >> > +
> >> > + /*
> >> > + * According to "Sequence Before Frequency Change",
> >> > + * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
> >> > + * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
> >> > + * which basically means we choose the maximum of old and new CDCLK, if we know both
> >> > + */
> >> > + if (change_cdclk)
> >> > + cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
> >> > +
> >> > + /*
> >> > + * According to "Sequence For Pipe Count Change",
> >> > + * if pipe count is increasing, set bits 25:16 to upcoming pipe count
> >> > + * (power well is enabled)
> >> > + * no action if it is decreasing, before the change
> >> > + */
> >> > + if (update_pipe_count)
> >> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> >> > +
> >> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> >> > + change_cdclk, update_pipe_count);
> >> > +}
> >> > +
> >> > +/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
> >> > + * voltage_level, active pipes, current CDCLK frequency.
> >> > + * @state: intel atomic state
> >> > + */
> >> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
> >>
> >> Ditto about static & naming.
> >>
> >> > +{
> >> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >>
> >> No new dev_priv please.
> >>
> >> > + const struct intel_cdclk_state *new_cdclk_state =
> >> > + intel_atomic_get_new_cdclk_state(state);
> >> > + const struct intel_cdclk_state *old_cdclk_state =
> >> > + intel_atomic_get_old_cdclk_state(state);
> >> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> >> > + bool update_cdclk, update_pipe_count;
> >> > +
> >> > + /* According to "Sequence After Frequency Change", set voltage to used level */
> >> > + voltage_level = new_cdclk_state->actual.voltage_level;
> >> > +
> >> > + update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> >> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
> >> > + hweight8(old_cdclk_state->active_pipes);
> >> > +
> >> > + /*
> >> > + * According to "Sequence After Frequency Change",
> >> > + * set bits 25:16 to current CDCLK
> >> > + */
> >> > + if (update_cdclk)
> >> > + cdclk = new_cdclk_state->actual.cdclk;
> >> > +
> >> > + /*
> >> > + * According to "Sequence For Pipe Count Change",
> >> > + * if pipe count is decreasing, set bits 25:16 to current pipe count,
> >> > + * after the change(power well is disabled)
> >> > + * no action if it is increasing, after the change
> >> > + */
> >> > + if (update_pipe_count)
> >> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> >> > +
> >> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> >> > + update_cdclk, update_pipe_count);
> >> > +}
> >> > +
> >> > /**
> >> > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> >> > * @state: intel atomic state
> >> > @@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >> > void
> >> > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >> > {
> >> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> >> > const struct intel_cdclk_state *old_cdclk_state =
> >> > intel_atomic_get_old_cdclk_state(state);
> >> > const struct intel_cdclk_state *new_cdclk_state =
> >> > @@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >> > &new_cdclk_state->actual))
> >> > return;
> >> >
> >> > + if (DISPLAY_VER(i915) >= 12)
> >> > + intel_cdclk_power_usage_to_pcode_pre_notification(state);
> >> > +
> >> > if (pipe == INVALID_PIPE ||
> >> > old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> >> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> >> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >> >
> >> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> >> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> >> > }
> >> > }
> >> >
> >> > @@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >> > void
> >> > intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> >> > {
> >> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> >> > const struct intel_cdclk_state *old_cdclk_state =
> >> > intel_atomic_get_old_cdclk_state(state);
> >> > const struct intel_cdclk_state *new_cdclk_state =
> >> > @@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> >> > &new_cdclk_state->actual))
> >> > return;
> >> >
> >> > + if (DISPLAY_VER(i915) >= 12)
> >> > + intel_cdclk_power_usage_to_pcode_post_notification(state);
> >> > +
> >> > if (pipe != INVALID_PIPE &&
> >> > old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> >> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> >> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >> >
> >> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> >> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> >> > }
> >> > }
> >> >
> >> > @@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
> >> > return 0;
> >> > }
> >> >
> >> > +static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
> >> > + const struct intel_cdclk_state *old_cdclk_state,
> >> > + const struct intel_cdclk_state *new_cdclk_state)
> >> > +{
> >> > + /*
> >> > + * We need to poke hw for gen >= 12, because we notify PCode if
> >> > + * pipe power well count changes.
> >> > + */
> >> > + return intel_cdclk_changed(&old_cdclk_state->actual,
> >> > + &new_cdclk_state->actual) ||
> >> > + ((DISPLAY_VER(i915) >= 12 &&
> >> > + hweight8(old_cdclk_state->active_pipes) !=
> >> > + hweight8(new_cdclk_state->active_pipes)));
> >>
> >> That's getting a bit unweildy for one expression.
> >>
> >> > +}
> >> > +
> >> > int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >> > {
> >> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> >> > @@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >> > if (ret)
> >> > return ret;
> >> >
> >> > - if (intel_cdclk_changed(&old_cdclk_state->actual,
> >> > - &new_cdclk_state->actual)) {
> >> > + if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
> >> > /*
> >> > * Also serialize commits across all crtcs
> >> > * if the actual hw needs to be poked.
> >> > @@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> >> > old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
> >> > intel_cdclk_changed(&old_cdclk_state->logical,
> >> > &new_cdclk_state->logical)) {
> >> > - ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> >> > - if (ret)
> >> > - return ret;
> >> > + ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> >> > + if (ret)
> >> > + return ret;
>
> Accidental indentation change?
>
> >> > } else {
> >> > return 0;
> >> > }
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> > index 51e2f6a11ce4..fa356adc61d9 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> >> > @@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> >> > const struct intel_cdclk_config *b);
> >> > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
> >> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
> >> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
> >> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
> >> > void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >> > const struct intel_cdclk_config *cdclk_config,
> >> > const char *context);
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> > index c1efa655fb68..9f786952585b 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -6526,6 +6526,20 @@
> >> > #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
> >> > #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
> >> > #define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
> >> > +#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
> >> > +#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
> >> > +#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
> >> > +#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
> >> > +#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
> >> > +#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
> >> > +#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
> >> > +#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
> >> > +#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
> >> > + (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> >> > + (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
> >> > + (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
> >> > #define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
> >> > #define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
> >> > #define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-03-07 9:41 ` Lisovskiy, Stanislav
@ 2023-03-07 10:23 ` Jani Nikula
2023-03-07 11:15 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2023-03-07 10:23 UTC (permalink / raw)
To: Lisovskiy, Stanislav; +Cc: intel-gfx, rodrigo.vivi
On Tue, 07 Mar 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote:
> On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
>> On Mon, 27 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
>> > Display to communicate display pipe count/CDCLK/voltage configuration
>> > to Pcode for more accurate power accounting for gen >= 12.
>> > Existing sequence is only sending the voltage value to the Pcode.
>> > Adding new sequence with current cdclk associate with voltage value masking.
>> > Adding pcode request when any pipe power well will disable or enable.
>> >
>> > v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
>> >
>> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
>> > drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
>> > drivers/gpu/drm/i915/i915_reg.h | 14 ++
>> > 3 files changed, 176 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > index 084a483f9776..df5a21f6a393 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
>> > @@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> > * NOOP - No Pcode communication needed for
>> > * Display versions 14 and beyond
>> > */;
>> > - else if (DISPLAY_VER(dev_priv) >= 11)
>> > + else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
>> > ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
>> > cdclk_config->voltage_level);
>> > - else
>> > + if (DISPLAY_VER(dev_priv) < 11) {
>> > /*
>> > * The timeout isn't specified, the 2ms used here is based on
>> > * experiment.
>> > @@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
>> > HSW_PCODE_DE_WRITE_FREQ_REQ,
>> > cdclk_config->voltage_level,
>> > 150, 2);
>> > -
>> > + }
>> > if (ret) {
>> > drm_err(&dev_priv->drm,
>> > "PCode CDCLK freq set failed, (err %d, freq %d)\n",
>> > @@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
>> > cdclk_config->voltage_level);
>> > }
>> >
>> > +static void intel_pcode_notify(struct drm_i915_private *i915,
>> > + u8 voltage_level,
>> > + u8 active_pipe_count,
>> > + u8 cdclk,
How can that be u8?
>> > + bool cdclk_update_valid,
>> > + bool pipe_count_update_valid)
Also not super happy about the flags arguments... could have cdclk == 0
means not valid and int active_pipe_count == -1 means not valid. Maybe.
>> > +{
>> > + int ret;
>> > + u32 update_mask = 0;
>> > +
>> > + if (DISPLAY_VER(i915) < 12)
>> > + return;
>> > +
>> > + update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
>> > +
>> > + if (cdclk_update_valid)
>> > + update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
>> > +
>> > + if (pipe_count_update_valid)
>> > + update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
>> > +
>> > + ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
>> > + SKL_CDCLK_PREPARE_FOR_CHANGE |
>> > + update_mask,
>> > + SKL_CDCLK_READY_FOR_CHANGE,
>> > + SKL_CDCLK_READY_FOR_CHANGE, 3);
>> > + if (ret) {
>> > + drm_err(&i915->drm,
>> > + "Failed to inform PCU about display config (err %d)\n",
>> > + ret);
>> > + return;
>>
>> Superfluous return.
>>
>> > + }
>> > +}
>> > +
>> > /**
>> > * intel_set_cdclk - Push the CDCLK configuration to the hardware
>> > * @dev_priv: i915 device
>> > @@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>> > }
>> > }
>> >
>> > +/**
>> > + * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
This doesn't need to be a kernel-doc comment (and it's a malformed one
at that). Ditto for the others.
>> > + * before the enabling power wells.
>> > + * send notification with cdclk, number of pipes, voltage_level.
>> > + * @state: intel atomic state
>> > + */
>> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
>>
>> This could be static?
>>
>> Not happy with the name, really, but at least it could use a
>> verb. Notify instead of notification.
>
> Any hint for better naming?
Maybe just intel_cdclk_pcode_pre_notify(),
intel_cdclk_pcode_post_notify(), and then intel_cdclk_pcode_notify() as
the shared thing?
>
>>
>> > +{
>> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>>
>> No new dev_priv please.
>>
>> > + const struct intel_cdclk_state *old_cdclk_state =
>> > + intel_atomic_get_old_cdclk_state(state);
>> > + const struct intel_cdclk_state *new_cdclk_state =
>> > + intel_atomic_get_new_cdclk_state(state);
>> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
>> > + bool change_cdclk, update_pipe_count;
>> > +
>> > + if (!intel_cdclk_changed(&old_cdclk_state->actual,
>> > + &new_cdclk_state->actual) &&
>> > + (new_cdclk_state->active_pipes ==
>> > + old_cdclk_state->active_pipes))
>> > + return;
>> > +
>> > + /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
>> > + voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
>> > +
>> > + change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
>> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
>> > + hweight8(old_cdclk_state->active_pipes);
>> > +
>> > + /*
>> > + * According to "Sequence Before Frequency Change",
>> > + * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
>> > + * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
>> > + * which basically means we choose the maximum of old and new CDCLK, if we know both
>> > + */
>> > + if (change_cdclk)
>> > + cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
>> > +
>> > + /*
>> > + * According to "Sequence For Pipe Count Change",
>> > + * if pipe count is increasing, set bits 25:16 to upcoming pipe count
>> > + * (power well is enabled)
>> > + * no action if it is decreasing, before the change
>> > + */
>> > + if (update_pipe_count)
>> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
>> > +
>> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
>> > + change_cdclk, update_pipe_count);
>> > +}
>> > +
>> > +/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
>> > + * voltage_level, active pipes, current CDCLK frequency.
>> > + * @state: intel atomic state
>> > + */
>> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
>>
>> Ditto about static & naming.
>>
>> > +{
>> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>>
>> No new dev_priv please.
>>
>> > + const struct intel_cdclk_state *new_cdclk_state =
>> > + intel_atomic_get_new_cdclk_state(state);
>> > + const struct intel_cdclk_state *old_cdclk_state =
>> > + intel_atomic_get_old_cdclk_state(state);
>> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
>> > + bool update_cdclk, update_pipe_count;
>> > +
>> > + /* According to "Sequence After Frequency Change", set voltage to used level */
>> > + voltage_level = new_cdclk_state->actual.voltage_level;
>> > +
>> > + update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
>> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
>> > + hweight8(old_cdclk_state->active_pipes);
>> > +
>> > + /*
>> > + * According to "Sequence After Frequency Change",
>> > + * set bits 25:16 to current CDCLK
>> > + */
>> > + if (update_cdclk)
>> > + cdclk = new_cdclk_state->actual.cdclk;
>> > +
>> > + /*
>> > + * According to "Sequence For Pipe Count Change",
>> > + * if pipe count is decreasing, set bits 25:16 to current pipe count,
>> > + * after the change(power well is disabled)
>> > + * no action if it is increasing, after the change
>> > + */
>> > + if (update_pipe_count)
>> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
>> > +
>> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
>> > + update_cdclk, update_pipe_count);
>> > +}
>> > +
>> > /**
>> > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
>> > * @state: intel atomic state
>> > @@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>> > void
>> > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> > {
>> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
>> > const struct intel_cdclk_state *old_cdclk_state =
>> > intel_atomic_get_old_cdclk_state(state);
>> > const struct intel_cdclk_state *new_cdclk_state =
>> > @@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> > &new_cdclk_state->actual))
>> > return;
>> >
>> > + if (DISPLAY_VER(i915) >= 12)
>> > + intel_cdclk_power_usage_to_pcode_pre_notification(state);
>> > +
>> > if (pipe == INVALID_PIPE ||
>> > old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
>> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
>> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>> >
>> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
>> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>> > }
>> > }
>> >
>> > @@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
>> > void
>> > intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>> > {
>> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
>> > const struct intel_cdclk_state *old_cdclk_state =
>> > intel_atomic_get_old_cdclk_state(state);
>> > const struct intel_cdclk_state *new_cdclk_state =
>> > @@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
>> > &new_cdclk_state->actual))
>> > return;
>> >
>> > + if (DISPLAY_VER(i915) >= 12)
>> > + intel_cdclk_power_usage_to_pcode_post_notification(state);
>> > +
>> > if (pipe != INVALID_PIPE &&
>> > old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
>> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
>> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>> >
>> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
>> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
>> > }
>> > }
>> >
>> > @@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
>> > return 0;
>> > }
>> >
>> > +static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
>> > + const struct intel_cdclk_state *old_cdclk_state,
>> > + const struct intel_cdclk_state *new_cdclk_state)
>> > +{
>> > + /*
>> > + * We need to poke hw for gen >= 12, because we notify PCode if
>> > + * pipe power well count changes.
>> > + */
>> > + return intel_cdclk_changed(&old_cdclk_state->actual,
>> > + &new_cdclk_state->actual) ||
>> > + ((DISPLAY_VER(i915) >= 12 &&
>> > + hweight8(old_cdclk_state->active_pipes) !=
>> > + hweight8(new_cdclk_state->active_pipes)));
>>
>> That's getting a bit unweildy for one expression.
>>
>> > +}
>> > +
>> > int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> > {
>> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>> > @@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> > if (ret)
>> > return ret;
>> >
>> > - if (intel_cdclk_changed(&old_cdclk_state->actual,
>> > - &new_cdclk_state->actual)) {
>> > + if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
>> > /*
>> > * Also serialize commits across all crtcs
>> > * if the actual hw needs to be poked.
>> > @@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
>> > old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
>> > intel_cdclk_changed(&old_cdclk_state->logical,
>> > &new_cdclk_state->logical)) {
>> > - ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
>> > - if (ret)
>> > - return ret;
>> > + ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
>> > + if (ret)
>> > + return ret;
Accidental indentation change?
>> > } else {
>> > return 0;
>> > }
>> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> > index 51e2f6a11ce4..fa356adc61d9 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
>> > @@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
>> > const struct intel_cdclk_config *b);
>> > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
>> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
>> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
>> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
>> > void intel_cdclk_dump_config(struct drm_i915_private *i915,
>> > const struct intel_cdclk_config *cdclk_config,
>> > const char *context);
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index c1efa655fb68..9f786952585b 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -6526,6 +6526,20 @@
>> > #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
>> > #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
>> > #define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
>> > +#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
>> > +#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
>> > +#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
>> > +#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
>> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
>> > +#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
>> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
>> > +#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
>> > +#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
>> > +#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
>> > +#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
>> > + (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
>> > + (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
>> > + (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
>> > #define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
>> > #define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
>> > #define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-03-06 18:14 ` Jani Nikula
@ 2023-03-07 9:41 ` Lisovskiy, Stanislav
2023-03-07 10:23 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Lisovskiy, Stanislav @ 2023-03-07 9:41 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, rodrigo.vivi
On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> > Display to communicate display pipe count/CDCLK/voltage configuration
> > to Pcode for more accurate power accounting for gen >= 12.
> > Existing sequence is only sending the voltage value to the Pcode.
> > Adding new sequence with current cdclk associate with voltage value masking.
> > Adding pcode request when any pipe power well will disable or enable.
> >
> > v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
> > drivers/gpu/drm/i915/i915_reg.h | 14 ++
> > 3 files changed, 176 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 084a483f9776..df5a21f6a393 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > * NOOP - No Pcode communication needed for
> > * Display versions 14 and beyond
> > */;
> > - else if (DISPLAY_VER(dev_priv) >= 11)
> > + else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
> > ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> > cdclk_config->voltage_level);
> > - else
> > + if (DISPLAY_VER(dev_priv) < 11) {
> > /*
> > * The timeout isn't specified, the 2ms used here is based on
> > * experiment.
> > @@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> > HSW_PCODE_DE_WRITE_FREQ_REQ,
> > cdclk_config->voltage_level,
> > 150, 2);
> > -
> > + }
> > if (ret) {
> > drm_err(&dev_priv->drm,
> > "PCode CDCLK freq set failed, (err %d, freq %d)\n",
> > @@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> > cdclk_config->voltage_level);
> > }
> >
> > +static void intel_pcode_notify(struct drm_i915_private *i915,
> > + u8 voltage_level,
> > + u8 active_pipe_count,
> > + u8 cdclk,
> > + bool cdclk_update_valid,
> > + bool pipe_count_update_valid)
> > +{
> > + int ret;
> > + u32 update_mask = 0;
> > +
> > + if (DISPLAY_VER(i915) < 12)
> > + return;
> > +
> > + update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
> > +
> > + if (cdclk_update_valid)
> > + update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
> > +
> > + if (pipe_count_update_valid)
> > + update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
> > +
> > + ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> > + SKL_CDCLK_PREPARE_FOR_CHANGE |
> > + update_mask,
> > + SKL_CDCLK_READY_FOR_CHANGE,
> > + SKL_CDCLK_READY_FOR_CHANGE, 3);
> > + if (ret) {
> > + drm_err(&i915->drm,
> > + "Failed to inform PCU about display config (err %d)\n",
> > + ret);
> > + return;
>
> Superfluous return.
>
> > + }
> > +}
> > +
> > /**
> > * intel_set_cdclk - Push the CDCLK configuration to the hardware
> > * @dev_priv: i915 device
> > @@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > }
> > }
> >
> > +/**
> > + * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
> > + * before the enabling power wells.
> > + * send notification with cdclk, number of pipes, voltage_level.
> > + * @state: intel atomic state
> > + */
> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
>
> This could be static?
>
> Not happy with the name, really, but at least it could use a
> verb. Notify instead of notification.
Any hint for better naming?
>
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>
> No new dev_priv please.
>
> > + const struct intel_cdclk_state *old_cdclk_state =
> > + intel_atomic_get_old_cdclk_state(state);
> > + const struct intel_cdclk_state *new_cdclk_state =
> > + intel_atomic_get_new_cdclk_state(state);
> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> > + bool change_cdclk, update_pipe_count;
> > +
> > + if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > + &new_cdclk_state->actual) &&
> > + (new_cdclk_state->active_pipes ==
> > + old_cdclk_state->active_pipes))
> > + return;
> > +
> > + /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
> > + voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
> > +
> > + change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
> > + hweight8(old_cdclk_state->active_pipes);
> > +
> > + /*
> > + * According to "Sequence Before Frequency Change",
> > + * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
> > + * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
> > + * which basically means we choose the maximum of old and new CDCLK, if we know both
> > + */
> > + if (change_cdclk)
> > + cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
> > +
> > + /*
> > + * According to "Sequence For Pipe Count Change",
> > + * if pipe count is increasing, set bits 25:16 to upcoming pipe count
> > + * (power well is enabled)
> > + * no action if it is decreasing, before the change
> > + */
> > + if (update_pipe_count)
> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> > +
> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> > + change_cdclk, update_pipe_count);
> > +}
> > +
> > +/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
> > + * voltage_level, active pipes, current CDCLK frequency.
> > + * @state: intel atomic state
> > + */
> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
>
> Ditto about static & naming.
>
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
>
> No new dev_priv please.
>
> > + const struct intel_cdclk_state *new_cdclk_state =
> > + intel_atomic_get_new_cdclk_state(state);
> > + const struct intel_cdclk_state *old_cdclk_state =
> > + intel_atomic_get_old_cdclk_state(state);
> > + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> > + bool update_cdclk, update_pipe_count;
> > +
> > + /* According to "Sequence After Frequency Change", set voltage to used level */
> > + voltage_level = new_cdclk_state->actual.voltage_level;
> > +
> > + update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> > + update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
> > + hweight8(old_cdclk_state->active_pipes);
> > +
> > + /*
> > + * According to "Sequence After Frequency Change",
> > + * set bits 25:16 to current CDCLK
> > + */
> > + if (update_cdclk)
> > + cdclk = new_cdclk_state->actual.cdclk;
> > +
> > + /*
> > + * According to "Sequence For Pipe Count Change",
> > + * if pipe count is decreasing, set bits 25:16 to current pipe count,
> > + * after the change(power well is disabled)
> > + * no action if it is increasing, after the change
> > + */
> > + if (update_pipe_count)
> > + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> > +
> > + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> > + update_cdclk, update_pipe_count);
> > +}
> > +
> > /**
> > * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> > * @state: intel atomic state
> > @@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > void
> > intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > const struct intel_cdclk_state *old_cdclk_state =
> > intel_atomic_get_old_cdclk_state(state);
> > const struct intel_cdclk_state *new_cdclk_state =
> > @@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > &new_cdclk_state->actual))
> > return;
> >
> > + if (DISPLAY_VER(i915) >= 12)
> > + intel_cdclk_power_usage_to_pcode_pre_notification(state);
> > +
> > if (pipe == INVALID_PIPE ||
> > old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >
> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > }
> > }
> >
> > @@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> > void
> > intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > + struct drm_i915_private *i915 = to_i915(state->base.dev);
> > const struct intel_cdclk_state *old_cdclk_state =
> > intel_atomic_get_old_cdclk_state(state);
> > const struct intel_cdclk_state *new_cdclk_state =
> > @@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> > &new_cdclk_state->actual))
> > return;
> >
> > + if (DISPLAY_VER(i915) >= 12)
> > + intel_cdclk_power_usage_to_pcode_post_notification(state);
> > +
> > if (pipe != INVALID_PIPE &&
> > old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> > - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> > + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >
> > - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> > + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> > }
> > }
> >
> > @@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
> > return 0;
> > }
> >
> > +static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
> > + const struct intel_cdclk_state *old_cdclk_state,
> > + const struct intel_cdclk_state *new_cdclk_state)
> > +{
> > + /*
> > + * We need to poke hw for gen >= 12, because we notify PCode if
> > + * pipe power well count changes.
> > + */
> > + return intel_cdclk_changed(&old_cdclk_state->actual,
> > + &new_cdclk_state->actual) ||
> > + ((DISPLAY_VER(i915) >= 12 &&
> > + hweight8(old_cdclk_state->active_pipes) !=
> > + hweight8(new_cdclk_state->active_pipes)));
>
> That's getting a bit unweildy for one expression.
>
> > +}
> > +
> > int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > @@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> > if (ret)
> > return ret;
> >
> > - if (intel_cdclk_changed(&old_cdclk_state->actual,
> > - &new_cdclk_state->actual)) {
> > + if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
> > /*
> > * Also serialize commits across all crtcs
> > * if the actual hw needs to be poked.
> > @@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> > old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
> > intel_cdclk_changed(&old_cdclk_state->logical,
> > &new_cdclk_state->logical)) {
> > - ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> > - if (ret)
> > - return ret;
> > + ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> > + if (ret)
> > + return ret;
> > } else {
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index 51e2f6a11ce4..fa356adc61d9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > @@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> > const struct intel_cdclk_config *b);
> > void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
> > void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
> > +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
> > +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
> > void intel_cdclk_dump_config(struct drm_i915_private *i915,
> > const struct intel_cdclk_config *cdclk_config,
> > const char *context);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index c1efa655fb68..9f786952585b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6526,6 +6526,20 @@
> > #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
> > #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
> > #define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
> > +#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
> > +#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
> > +#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
> > +#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
> > +#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
> > +#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
> > +#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
> > +#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
> > +#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
> > +#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
> > + (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> > + (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
> > + (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
> > #define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
> > #define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
> > #define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
>
> --
> Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-02-27 13:26 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
@ 2023-03-06 18:14 ` Jani Nikula
2023-03-07 9:41 ` Lisovskiy, Stanislav
0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2023-03-06 18:14 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: rodrigo.vivi
On Mon, 27 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> Display to communicate display pipe count/CDCLK/voltage configuration
> to Pcode for more accurate power accounting for gen >= 12.
> Existing sequence is only sending the voltage value to the Pcode.
> Adding new sequence with current cdclk associate with voltage value masking.
> Adding pcode request when any pipe power well will disable or enable.
>
> v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
> drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
> drivers/gpu/drm/i915/i915_reg.h | 14 ++
> 3 files changed, 176 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 084a483f9776..df5a21f6a393 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> * NOOP - No Pcode communication needed for
> * Display versions 14 and beyond
> */;
> - else if (DISPLAY_VER(dev_priv) >= 11)
> + else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
> ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> cdclk_config->voltage_level);
> - else
> + if (DISPLAY_VER(dev_priv) < 11) {
> /*
> * The timeout isn't specified, the 2ms used here is based on
> * experiment.
> @@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
> HSW_PCODE_DE_WRITE_FREQ_REQ,
> cdclk_config->voltage_level,
> 150, 2);
> -
> + }
> if (ret) {
> drm_err(&dev_priv->drm,
> "PCode CDCLK freq set failed, (err %d, freq %d)\n",
> @@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> cdclk_config->voltage_level);
> }
>
> +static void intel_pcode_notify(struct drm_i915_private *i915,
> + u8 voltage_level,
> + u8 active_pipe_count,
> + u8 cdclk,
> + bool cdclk_update_valid,
> + bool pipe_count_update_valid)
> +{
> + int ret;
> + u32 update_mask = 0;
> +
> + if (DISPLAY_VER(i915) < 12)
> + return;
> +
> + update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
> +
> + if (cdclk_update_valid)
> + update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
> +
> + if (pipe_count_update_valid)
> + update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
> +
> + ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> + SKL_CDCLK_PREPARE_FOR_CHANGE |
> + update_mask,
> + SKL_CDCLK_READY_FOR_CHANGE,
> + SKL_CDCLK_READY_FOR_CHANGE, 3);
> + if (ret) {
> + drm_err(&i915->drm,
> + "Failed to inform PCU about display config (err %d)\n",
> + ret);
> + return;
Superfluous return.
> + }
> +}
> +
> /**
> * intel_set_cdclk - Push the CDCLK configuration to the hardware
> * @dev_priv: i915 device
> @@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> }
> }
>
> +/**
> + * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
> + * before the enabling power wells.
> + * send notification with cdclk, number of pipes, voltage_level.
> + * @state: intel atomic state
> + */
> +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
This could be static?
Not happy with the name, really, but at least it could use a
verb. Notify instead of notification.
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
No new dev_priv please.
> + const struct intel_cdclk_state *old_cdclk_state =
> + intel_atomic_get_old_cdclk_state(state);
> + const struct intel_cdclk_state *new_cdclk_state =
> + intel_atomic_get_new_cdclk_state(state);
> + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> + bool change_cdclk, update_pipe_count;
> +
> + if (!intel_cdclk_changed(&old_cdclk_state->actual,
> + &new_cdclk_state->actual) &&
> + (new_cdclk_state->active_pipes ==
> + old_cdclk_state->active_pipes))
> + return;
> +
> + /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
> + voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
> +
> + change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> + update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
> + hweight8(old_cdclk_state->active_pipes);
> +
> + /*
> + * According to "Sequence Before Frequency Change",
> + * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
> + * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
> + * which basically means we choose the maximum of old and new CDCLK, if we know both
> + */
> + if (change_cdclk)
> + cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
> +
> + /*
> + * According to "Sequence For Pipe Count Change",
> + * if pipe count is increasing, set bits 25:16 to upcoming pipe count
> + * (power well is enabled)
> + * no action if it is decreasing, before the change
> + */
> + if (update_pipe_count)
> + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> +
> + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> + change_cdclk, update_pipe_count);
> +}
> +
> +/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
> + * voltage_level, active pipes, current CDCLK frequency.
> + * @state: intel atomic state
> + */
> +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
Ditto about static & naming.
> +{
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
No new dev_priv please.
> + const struct intel_cdclk_state *new_cdclk_state =
> + intel_atomic_get_new_cdclk_state(state);
> + const struct intel_cdclk_state *old_cdclk_state =
> + intel_atomic_get_old_cdclk_state(state);
> + unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
> + bool update_cdclk, update_pipe_count;
> +
> + /* According to "Sequence After Frequency Change", set voltage to used level */
> + voltage_level = new_cdclk_state->actual.voltage_level;
> +
> + update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
> + update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
> + hweight8(old_cdclk_state->active_pipes);
> +
> + /*
> + * According to "Sequence After Frequency Change",
> + * set bits 25:16 to current CDCLK
> + */
> + if (update_cdclk)
> + cdclk = new_cdclk_state->actual.cdclk;
> +
> + /*
> + * According to "Sequence For Pipe Count Change",
> + * if pipe count is decreasing, set bits 25:16 to current pipe count,
> + * after the change(power well is disabled)
> + * no action if it is increasing, after the change
> + */
> + if (update_pipe_count)
> + num_active_pipes = hweight8(new_cdclk_state->active_pipes);
> +
> + intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
> + update_cdclk, update_pipe_count);
> +}
> +
> /**
> * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> * @state: intel atomic state
> @@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> void
> intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> const struct intel_cdclk_state *old_cdclk_state =
> intel_atomic_get_old_cdclk_state(state);
> const struct intel_cdclk_state *new_cdclk_state =
> @@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> &new_cdclk_state->actual))
> return;
>
> + if (DISPLAY_VER(i915) >= 12)
> + intel_cdclk_power_usage_to_pcode_pre_notification(state);
> +
> if (pipe == INVALID_PIPE ||
> old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>
> - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> }
> }
>
> @@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> void
> intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct drm_i915_private *i915 = to_i915(state->base.dev);
> const struct intel_cdclk_state *old_cdclk_state =
> intel_atomic_get_old_cdclk_state(state);
> const struct intel_cdclk_state *new_cdclk_state =
> @@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
> &new_cdclk_state->actual))
> return;
>
> + if (DISPLAY_VER(i915) >= 12)
> + intel_cdclk_power_usage_to_pcode_post_notification(state);
> +
> if (pipe != INVALID_PIPE &&
> old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
> - drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
> + drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
>
> - intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
> + intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> }
> }
>
> @@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> +static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
> + const struct intel_cdclk_state *old_cdclk_state,
> + const struct intel_cdclk_state *new_cdclk_state)
> +{
> + /*
> + * We need to poke hw for gen >= 12, because we notify PCode if
> + * pipe power well count changes.
> + */
> + return intel_cdclk_changed(&old_cdclk_state->actual,
> + &new_cdclk_state->actual) ||
> + ((DISPLAY_VER(i915) >= 12 &&
> + hweight8(old_cdclk_state->active_pipes) !=
> + hweight8(new_cdclk_state->active_pipes)));
That's getting a bit unweildy for one expression.
> +}
> +
> int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> @@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> if (ret)
> return ret;
>
> - if (intel_cdclk_changed(&old_cdclk_state->actual,
> - &new_cdclk_state->actual)) {
> + if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
> /*
> * Also serialize commits across all crtcs
> * if the actual hw needs to be poked.
> @@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
> intel_cdclk_changed(&old_cdclk_state->logical,
> &new_cdclk_state->logical)) {
> - ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> - if (ret)
> - return ret;
> + ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
> + if (ret)
> + return ret;
> } else {
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 51e2f6a11ce4..fa356adc61d9 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
> @@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
> const struct intel_cdclk_config *b);
> void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
> void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
> +void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
> +void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
> void intel_cdclk_dump_config(struct drm_i915_private *i915,
> const struct intel_cdclk_config *cdclk_config,
> const char *context);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c1efa655fb68..9f786952585b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6526,6 +6526,20 @@
> #define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
> #define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
> #define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
> +#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
> +#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
> +#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
> +#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
> +#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
> +#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
> +#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
> +#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
> +#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
> +#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
> +#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
> + (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
> + (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
> + (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
> #define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
> #define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
> #define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
@ 2023-02-27 13:26 Stanislav Lisovskiy
2023-03-06 18:14 ` Jani Nikula
0 siblings, 1 reply; 16+ messages in thread
From: Stanislav Lisovskiy @ 2023-02-27 13:26 UTC (permalink / raw)
To: intel-gfx; +Cc: rodrigo.vivi
Display to communicate display pipe count/CDCLK/voltage configuration
to Pcode for more accurate power accounting for gen >= 12.
Existing sequence is only sending the voltage value to the Pcode.
Adding new sequence with current cdclk associate with voltage value masking.
Adding pcode request when any pipe power well will disable or enable.
v2: - Make intel_cdclk_need_serialize static to make CI compiler happy.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
drivers/gpu/drm/i915/i915_reg.h | 14 ++
3 files changed, 176 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 084a483f9776..df5a21f6a393 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
* NOOP - No Pcode communication needed for
* Display versions 14 and beyond
*/;
- else if (DISPLAY_VER(dev_priv) >= 11)
+ else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
cdclk_config->voltage_level);
- else
+ if (DISPLAY_VER(dev_priv) < 11) {
/*
* The timeout isn't specified, the 2ms used here is based on
* experiment.
@@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
HSW_PCODE_DE_WRITE_FREQ_REQ,
cdclk_config->voltage_level,
150, 2);
-
+ }
if (ret) {
drm_err(&dev_priv->drm,
"PCode CDCLK freq set failed, (err %d, freq %d)\n",
@@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
cdclk_config->voltage_level);
}
+static void intel_pcode_notify(struct drm_i915_private *i915,
+ u8 voltage_level,
+ u8 active_pipe_count,
+ u8 cdclk,
+ bool cdclk_update_valid,
+ bool pipe_count_update_valid)
+{
+ int ret;
+ u32 update_mask = 0;
+
+ if (DISPLAY_VER(i915) < 12)
+ return;
+
+ update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
+
+ if (cdclk_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
+
+ if (pipe_count_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
+
+ ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
+ SKL_CDCLK_PREPARE_FOR_CHANGE |
+ update_mask,
+ SKL_CDCLK_READY_FOR_CHANGE,
+ SKL_CDCLK_READY_FOR_CHANGE, 3);
+ if (ret) {
+ drm_err(&i915->drm,
+ "Failed to inform PCU about display config (err %d)\n",
+ ret);
+ return;
+ }
+}
+
/**
* intel_set_cdclk - Push the CDCLK configuration to the hardware
* @dev_priv: i915 device
@@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
}
}
+/**
+ * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
+ * before the enabling power wells.
+ * send notification with cdclk, number of pipes, voltage_level.
+ * @state: intel atomic state
+ */
+void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool change_cdclk, update_pipe_count;
+
+ if (!intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) &&
+ (new_cdclk_state->active_pipes ==
+ old_cdclk_state->active_pipes))
+ return;
+
+ /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
+ voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
+
+ change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence Before Frequency Change",
+ * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
+ * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
+ * which basically means we choose the maximum of old and new CDCLK, if we know both
+ */
+ if (change_cdclk)
+ cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is increasing, set bits 25:16 to upcoming pipe count
+ * (power well is enabled)
+ * no action if it is decreasing, before the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
+ change_cdclk, update_pipe_count);
+}
+
+/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
+ * voltage_level, active pipes, current CDCLK frequency.
+ * @state: intel atomic state
+ */
+void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool update_cdclk, update_pipe_count;
+
+ /* According to "Sequence After Frequency Change", set voltage to used level */
+ voltage_level = new_cdclk_state->actual.voltage_level;
+
+ update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence After Frequency Change",
+ * set bits 25:16 to current CDCLK
+ */
+ if (update_cdclk)
+ cdclk = new_cdclk_state->actual.cdclk;
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is decreasing, set bits 25:16 to current pipe count,
+ * after the change(power well is disabled)
+ * no action if it is increasing, after the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
+ update_cdclk, update_pipe_count);
+}
+
/**
* intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
* @state: intel atomic state
@@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
void
intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_power_usage_to_pcode_pre_notification(state);
+
if (pipe == INVALID_PIPE ||
old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
void
intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_power_usage_to_pcode_post_notification(state);
+
if (pipe != INVALID_PIPE &&
old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
return 0;
}
+static bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
+ const struct intel_cdclk_state *old_cdclk_state,
+ const struct intel_cdclk_state *new_cdclk_state)
+{
+ /*
+ * We need to poke hw for gen >= 12, because we notify PCode if
+ * pipe power well count changes.
+ */
+ return intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) ||
+ ((DISPLAY_VER(i915) >= 12 &&
+ hweight8(old_cdclk_state->active_pipes) !=
+ hweight8(new_cdclk_state->active_pipes)));
+}
+
int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
@@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
if (ret)
return ret;
- if (intel_cdclk_changed(&old_cdclk_state->actual,
- &new_cdclk_state->actual)) {
+ if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
/*
* Also serialize commits across all crtcs
* if the actual hw needs to be poked.
@@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
intel_cdclk_changed(&old_cdclk_state->logical,
&new_cdclk_state->logical)) {
- ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
- if (ret)
- return ret;
+ ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
+ if (ret)
+ return ret;
} else {
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 51e2f6a11ce4..fa356adc61d9 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
const struct intel_cdclk_config *b);
void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
+void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
+void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
void intel_cdclk_dump_config(struct drm_i915_private *i915,
const struct intel_cdclk_config *cdclk_config,
const char *context);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c1efa655fb68..9f786952585b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6526,6 +6526,20 @@
#define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
#define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
#define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
+#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
+#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
+#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
+#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
+#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
+#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
+#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
+#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
+#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
+ (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
+ (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
+ (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
#define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
#define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
#define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-02-27 9:52 Stanislav Lisovskiy
2023-02-27 12:20 ` kernel test robot
2023-02-27 12:30 ` kernel test robot
@ 2023-02-27 12:51 ` kernel test robot
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2023-02-27 12:51 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: rodrigo.vivi, llvm, oe-kbuild-all
Hi Stanislav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20230227095253.22415-1-stanislav.lisovskiy%40intel.com
patch subject: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
config: i386-randconfig-r033-20230227 (https://download.01.org/0day-ci/archive/20230227/202302272052.ZvLMGyJl-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/bc4de3b25bbf043f68ef862c45f975b79af938be
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
git checkout bc4de3b25bbf043f68ef862c45f975b79af938be
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302272052.ZvLMGyJl-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_cdclk.c:3006:6: warning: no previous prototype for function 'intel_cdclk_need_serialize' [-Wmissing-prototypes]
bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
^
drivers/gpu/drm/i915/display/intel_cdclk.c:3006:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
^
static
1 warning generated.
vim +/intel_cdclk_need_serialize +3006 drivers/gpu/drm/i915/display/intel_cdclk.c
3005
> 3006 bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
3007 const struct intel_cdclk_state *old_cdclk_state,
3008 const struct intel_cdclk_state *new_cdclk_state)
3009 {
3010 /*
3011 * We need to poke hw for gen >= 12, because we notify PCode if
3012 * pipe power well count changes.
3013 */
3014 return intel_cdclk_changed(&old_cdclk_state->actual,
3015 &new_cdclk_state->actual) ||
3016 ((DISPLAY_VER(i915) >= 12 &&
3017 hweight8(old_cdclk_state->active_pipes) !=
3018 hweight8(new_cdclk_state->active_pipes)));
3019 }
3020
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-02-27 9:52 Stanislav Lisovskiy
2023-02-27 12:20 ` kernel test robot
@ 2023-02-27 12:30 ` kernel test robot
2023-02-27 12:51 ` kernel test robot
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2023-02-27 12:30 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: rodrigo.vivi, oe-kbuild-all
Hi Stanislav,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20230227095253.22415-1-stanislav.lisovskiy%40intel.com
patch subject: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230227/202302272028.pPiIBz9V-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/bc4de3b25bbf043f68ef862c45f975b79af938be
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
git checkout bc4de3b25bbf043f68ef862c45f975b79af938be
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302272028.pPiIBz9V-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_cdclk.c:3006:6: error: no previous prototype for 'intel_cdclk_need_serialize' [-Werror=missing-prototypes]
3006 | bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/intel_cdclk_need_serialize +3006 drivers/gpu/drm/i915/display/intel_cdclk.c
3005
> 3006 bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
3007 const struct intel_cdclk_state *old_cdclk_state,
3008 const struct intel_cdclk_state *new_cdclk_state)
3009 {
3010 /*
3011 * We need to poke hw for gen >= 12, because we notify PCode if
3012 * pipe power well count changes.
3013 */
3014 return intel_cdclk_changed(&old_cdclk_state->actual,
3015 &new_cdclk_state->actual) ||
3016 ((DISPLAY_VER(i915) >= 12 &&
3017 hweight8(old_cdclk_state->active_pipes) !=
3018 hweight8(new_cdclk_state->active_pipes)));
3019 }
3020
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
2023-02-27 9:52 Stanislav Lisovskiy
@ 2023-02-27 12:20 ` kernel test robot
2023-02-27 12:30 ` kernel test robot
2023-02-27 12:51 ` kernel test robot
2 siblings, 0 replies; 16+ messages in thread
From: kernel test robot @ 2023-02-27 12:20 UTC (permalink / raw)
To: Stanislav Lisovskiy, intel-gfx; +Cc: rodrigo.vivi, oe-kbuild-all
Hi Stanislav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20230227095253.22415-1-stanislav.lisovskiy%40intel.com
patch subject: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20230227/202302272011.1iLXtakR-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/bc4de3b25bbf043f68ef862c45f975b79af938be
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Stanislav-Lisovskiy/drm-i915-display-Communicate-display-power-demands-to-pcode-more-accurately/20230227-175404
git checkout bc4de3b25bbf043f68ef862c45f975b79af938be
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302272011.1iLXtakR-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_cdclk.c:3006:6: warning: no previous prototype for 'intel_cdclk_need_serialize' [-Wmissing-prototypes]
3006 | bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/intel_cdclk_need_serialize +3006 drivers/gpu/drm/i915/display/intel_cdclk.c
3005
> 3006 bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
3007 const struct intel_cdclk_state *old_cdclk_state,
3008 const struct intel_cdclk_state *new_cdclk_state)
3009 {
3010 /*
3011 * We need to poke hw for gen >= 12, because we notify PCode if
3012 * pipe power well count changes.
3013 */
3014 return intel_cdclk_changed(&old_cdclk_state->actual,
3015 &new_cdclk_state->actual) ||
3016 ((DISPLAY_VER(i915) >= 12 &&
3017 hweight8(old_cdclk_state->active_pipes) !=
3018 hweight8(new_cdclk_state->active_pipes)));
3019 }
3020
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
@ 2023-02-27 9:52 Stanislav Lisovskiy
2023-02-27 12:20 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Stanislav Lisovskiy @ 2023-02-27 9:52 UTC (permalink / raw)
To: intel-gfx; +Cc: rodrigo.vivi
Display to communicate display pipe count/CDCLK/voltage configuration
to Pcode for more accurate power accounting for gen >= 12.
Existing sequence is only sending the voltage value to the Pcode.
Adding new sequence with current cdclk associate with voltage value masking.
Adding pcode request when any pipe power well will disable or enable.
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 174 +++++++++++++++++++--
drivers/gpu/drm/i915/display/intel_cdclk.h | 2 +
drivers/gpu/drm/i915/i915_reg.h | 14 ++
3 files changed, 176 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 084a483f9776..fa7846942194 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1932,10 +1932,10 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
* NOOP - No Pcode communication needed for
* Display versions 14 and beyond
*/;
- else if (DISPLAY_VER(dev_priv) >= 11)
+ else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
ret = snb_pcode_write(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
cdclk_config->voltage_level);
- else
+ if (DISPLAY_VER(dev_priv) < 11) {
/*
* The timeout isn't specified, the 2ms used here is based on
* experiment.
@@ -1946,7 +1946,7 @@ static void bxt_set_cdclk(struct drm_i915_private *dev_priv,
HSW_PCODE_DE_WRITE_FREQ_REQ,
cdclk_config->voltage_level,
150, 2);
-
+ }
if (ret) {
drm_err(&dev_priv->drm,
"PCode CDCLK freq set failed, (err %d, freq %d)\n",
@@ -2242,6 +2242,40 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
cdclk_config->voltage_level);
}
+static void intel_pcode_notify(struct drm_i915_private *i915,
+ u8 voltage_level,
+ u8 active_pipe_count,
+ u8 cdclk,
+ bool cdclk_update_valid,
+ bool pipe_count_update_valid)
+{
+ int ret;
+ u32 update_mask = 0;
+
+ if (DISPLAY_VER(i915) < 12)
+ return;
+
+ update_mask = DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, active_pipe_count, voltage_level);
+
+ if (cdclk_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_CDCLK_VALID;
+
+ if (pipe_count_update_valid)
+ update_mask |= DISPLAY_TO_PCODE_PIPE_COUNT_VALID;
+
+ ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
+ SKL_CDCLK_PREPARE_FOR_CHANGE |
+ update_mask,
+ SKL_CDCLK_READY_FOR_CHANGE,
+ SKL_CDCLK_READY_FOR_CHANGE, 3);
+ if (ret) {
+ drm_err(&i915->drm,
+ "Failed to inform PCU about display config (err %d)\n",
+ ret);
+ return;
+ }
+}
+
/**
* intel_set_cdclk - Push the CDCLK configuration to the hardware
* @dev_priv: i915 device
@@ -2311,6 +2345,98 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
}
}
+/**
+ * intel_cdclk_power_usage_to_pcode_pre_notification: display to pcode notification
+ * before the enabling power wells.
+ * send notification with cdclk, number of pipes, voltage_level.
+ * @state: intel atomic state
+ */
+void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool change_cdclk, update_pipe_count;
+
+ if (!intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) &&
+ (new_cdclk_state->active_pipes ==
+ old_cdclk_state->active_pipes))
+ return;
+
+ /* According to "Sequence Before Frequency Change", voltage level set to 0x3 */
+ voltage_level = DISPLAY_TO_PCODE_VOLTAGE_MAX;
+
+ change_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) >
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence Before Frequency Change",
+ * if CDCLK is increasing, set bits 25:16 to upcoming CDCLK,
+ * if CDCLK is decreasing or not changing, set bits 25:16 to current CDCLK,
+ * which basically means we choose the maximum of old and new CDCLK, if we know both
+ */
+ if (change_cdclk)
+ cdclk = max(new_cdclk_state->actual.cdclk, old_cdclk_state->actual.cdclk);
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is increasing, set bits 25:16 to upcoming pipe count
+ * (power well is enabled)
+ * no action if it is decreasing, before the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
+ change_cdclk, update_pipe_count);
+}
+
+/* intel_cdclk_power_usage_to_pcode_post_notification: after frequency change sending
+ * voltage_level, active pipes, current CDCLK frequency.
+ * @state: intel atomic state
+ */
+void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state)
+{
+ struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ const struct intel_cdclk_state *new_cdclk_state =
+ intel_atomic_get_new_cdclk_state(state);
+ const struct intel_cdclk_state *old_cdclk_state =
+ intel_atomic_get_old_cdclk_state(state);
+ unsigned int cdclk = 0; u8 voltage_level, num_active_pipes = 0;
+ bool update_cdclk, update_pipe_count;
+
+ /* According to "Sequence After Frequency Change", set voltage to used level */
+ voltage_level = new_cdclk_state->actual.voltage_level;
+
+ update_cdclk = new_cdclk_state->actual.cdclk != old_cdclk_state->actual.cdclk;
+ update_pipe_count = hweight8(new_cdclk_state->active_pipes) <
+ hweight8(old_cdclk_state->active_pipes);
+
+ /*
+ * According to "Sequence After Frequency Change",
+ * set bits 25:16 to current CDCLK
+ */
+ if (update_cdclk)
+ cdclk = new_cdclk_state->actual.cdclk;
+
+ /*
+ * According to "Sequence For Pipe Count Change",
+ * if pipe count is decreasing, set bits 25:16 to current pipe count,
+ * after the change(power well is disabled)
+ * no action if it is increasing, after the change
+ */
+ if (update_pipe_count)
+ num_active_pipes = hweight8(new_cdclk_state->active_pipes);
+
+ intel_pcode_notify(dev_priv, voltage_level, num_active_pipes, cdclk,
+ update_cdclk, update_pipe_count);
+}
+
/**
* intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
* @state: intel atomic state
@@ -2321,7 +2447,7 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
void
intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2332,11 +2458,14 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_power_usage_to_pcode_pre_notification(state);
+
if (pipe == INVALID_PIPE ||
old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2350,7 +2479,7 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
void
intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+ struct drm_i915_private *i915 = to_i915(state->base.dev);
const struct intel_cdclk_state *old_cdclk_state =
intel_atomic_get_old_cdclk_state(state);
const struct intel_cdclk_state *new_cdclk_state =
@@ -2361,11 +2490,14 @@ intel_set_cdclk_post_plane_update(struct intel_atomic_state *state)
&new_cdclk_state->actual))
return;
+ if (DISPLAY_VER(i915) >= 12)
+ intel_cdclk_power_usage_to_pcode_post_notification(state);
+
if (pipe != INVALID_PIPE &&
old_cdclk_state->actual.cdclk > new_cdclk_state->actual.cdclk) {
- drm_WARN_ON(&dev_priv->drm, !new_cdclk_state->base.changed);
+ drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
- intel_set_cdclk(dev_priv, &new_cdclk_state->actual, pipe);
+ intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
}
}
@@ -2871,6 +3003,21 @@ int intel_cdclk_init(struct drm_i915_private *dev_priv)
return 0;
}
+bool intel_cdclk_need_serialize(struct drm_i915_private *i915,
+ const struct intel_cdclk_state *old_cdclk_state,
+ const struct intel_cdclk_state *new_cdclk_state)
+{
+ /*
+ * We need to poke hw for gen >= 12, because we notify PCode if
+ * pipe power well count changes.
+ */
+ return intel_cdclk_changed(&old_cdclk_state->actual,
+ &new_cdclk_state->actual) ||
+ ((DISPLAY_VER(i915) >= 12 &&
+ hweight8(old_cdclk_state->active_pipes) !=
+ hweight8(new_cdclk_state->active_pipes)));
+}
+
int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->base.dev);
@@ -2892,8 +3039,7 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
if (ret)
return ret;
- if (intel_cdclk_changed(&old_cdclk_state->actual,
- &new_cdclk_state->actual)) {
+ if (intel_cdclk_need_serialize(dev_priv, old_cdclk_state, new_cdclk_state)) {
/*
* Also serialize commits across all crtcs
* if the actual hw needs to be poked.
@@ -2905,9 +3051,9 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk ||
intel_cdclk_changed(&old_cdclk_state->logical,
&new_cdclk_state->logical)) {
- ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
- if (ret)
- return ret;
+ ret = intel_atomic_lock_global_state(&new_cdclk_state->base);
+ if (ret)
+ return ret;
} else {
return 0;
}
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 51e2f6a11ce4..fa356adc61d9 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.h
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.h
@@ -64,6 +64,8 @@ bool intel_cdclk_needs_modeset(const struct intel_cdclk_config *a,
const struct intel_cdclk_config *b);
void intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state);
void intel_set_cdclk_post_plane_update(struct intel_atomic_state *state);
+void intel_cdclk_power_usage_to_pcode_pre_notification(struct intel_atomic_state *state);
+void intel_cdclk_power_usage_to_pcode_post_notification(struct intel_atomic_state *state);
void intel_cdclk_dump_config(struct drm_i915_private *i915,
const struct intel_cdclk_config *cdclk_config,
const char *context);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c1efa655fb68..9f786952585b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6526,6 +6526,20 @@
#define ICL_PCODE_MEM_SS_READ_GLOBAL_INFO (0x0 << 8)
#define ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point) (((point) << 16) | (0x1 << 8))
#define ADL_PCODE_MEM_SS_READ_PSF_GV_INFO ((0) | (0x2 << 8))
+#define DISPLAY_TO_PCODE_CDCLK_MAX 0x28D
+#define DISPLAY_TO_PCODE_VOLTAGE_MASK REG_GENMASK(1, 0)
+#define DISPLAY_TO_PCODE_VOLTAGE_MAX DISPLAY_TO_PCODE_VOLTAGE_MASK
+#define DISPLAY_TO_PCODE_CDCLK_VALID REG_BIT(27)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_VALID REG_BIT(31)
+#define DISPLAY_TO_PCODE_CDCLK_MASK REG_GENMASK(25, 16)
+#define DISPLAY_TO_PCODE_PIPE_COUNT_MASK REG_GENMASK(30, 28)
+#define DISPLAY_TO_PCODE_CDCLK(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_CDCLK_MASK, (x))
+#define DISPLAY_TO_PCODE_PIPE_COUNT(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_PIPE_COUNT_MASK, (x))
+#define DISPLAY_TO_PCODE_VOLTAGE(x) REG_FIELD_PREP(DISPLAY_TO_PCODE_VOLTAGE_MASK, (x))
+#define DISPLAY_TO_PCODE_UPDATE_MASK(cdclk, num_pipes, voltage_level) \
+ (DISPLAY_TO_PCODE_CDCLK(cdclk)) | \
+ (DISPLAY_TO_PCODE_PIPE_COUNT(num_pipes)) | \
+ (DISPLAY_TO_PCODE_VOLTAGE(voltage_level))
#define ICL_PCODE_SAGV_DE_MEM_SS_CONFIG 0xe
#define ICL_PCODE_REP_QGV_MASK REG_GENMASK(1, 0)
#define ICL_PCODE_REP_QGV_SAFE REG_FIELD_PREP(ICL_PCODE_REP_QGV_MASK, 0)
--
2.37.3
^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-03-17 20:03 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-17 10:24 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.BUILD: warning for drm/i915/display: Communicate display power demands to pcode more accurately (rev3) Patchwork
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: " Patchwork
2023-03-17 16:46 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-03-17 17:12 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-03-17 20:03 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
-- strict thread matches above, loose matches on Subject: below --
2023-02-27 13:26 [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately Stanislav Lisovskiy
2023-03-06 18:14 ` Jani Nikula
2023-03-07 9:41 ` Lisovskiy, Stanislav
2023-03-07 10:23 ` Jani Nikula
2023-03-07 11:15 ` Lisovskiy, Stanislav
2023-03-07 11:52 ` Jani Nikula
2023-02-27 9:52 Stanislav Lisovskiy
2023-02-27 12:20 ` kernel test robot
2023-02-27 12:30 ` kernel test robot
2023-02-27 12:51 ` kernel test robot
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).