[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode
Display to communicate display pipe count/CDCLK/voltage configuration
to Pcode for more accurate power accounting for DG2.
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)
v4: - Fixed some checkpatch warnings
v5: - According to HW team comments that change should affect only DG2,
fix correspodent platform check to account this.
v6: - Added one more missing IS_DG2 check(Vinod Govindapillai)
Signed-off-by: Stanislav Lisovskiy
Reviewed-by: Vinod Govindapillai
---
drivers/gpu/drm/i915/display/intel_cdclk.c | 158 +++--
drivers/gpu/drm/i915/i915_reg.h| 14 ++
2 files changed, 160 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c
b/drivers/gpu/drm/i915/display/intel_cdclk.c
index f6223d8f13b8..a11092deaba6 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1898,7 +1898,7 @@ static void bxt_set_cdclk(struct drm_i915_private
*dev_priv,
*/
if (DISPLAY_VER(dev_priv) >= 14)
/* NOOP */;
- else if (DISPLAY_VER(dev_priv) >= 11)
+ else if (DISPLAY_VER(dev_priv) >= 11 && !IS_DG2(dev_priv))
ret = skl_pcode_request(_priv->uncore,
SKL_PCODE_CDCLK_CONTROL,
SKL_CDCLK_PREPARE_FOR_CHANGE,
SKL_CDCLK_READY_FOR_CHANGE,
@@ -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(_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(_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 (!IS_DG2(i915))
+ 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(>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(>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 =
+
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode
Hi Stan
On Tue, 2023-04-25 at 11:13 +0300, Stanislav Lisovskiy wrote:
> Display to communicate display pipe count/CDCLK/voltage configuration
> to Pcode for more accurate power accounting for DG2.
> 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)
> v4: - Fixed some checkpatch warnings
> v5: - According to HW team comments that change should affect only DG2,
> fix correspodent platform check to account this.
>
> Signed-off-by: Stanislav Lisovskiy
> ---
> 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 f6223d8f13b8..1b70c7be92fe 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(_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);
> -
> + }
After this, there is this below code snippet to update pcode for the current
voltage_level.
else if (DISPLAY_VER(dev_priv) >= 11)
ret = snb_pcode_write(_priv->uncore,
SKL_PCODE_CDCLK_CONTROL,
cdclk_config->voltage_level);
else
Don't you need to handle this part for DG2 as well?
With that,
Reviewed-by: Vinod Govindapillai
> if (ret) {
> drm_err(_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 (!IS_DG2(i915))
> + 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(>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(>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 =
[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode
Display to communicate display pipe count/CDCLK/voltage configuration
to Pcode for more accurate power accounting for DG2.
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)
v4: - Fixed some checkpatch warnings
v5: - According to HW team comments that change should affect only DG2,
fix correspodent platform check to account this.
Signed-off-by: Stanislav Lisovskiy
---
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 f6223d8f13b8..1b70c7be92fe 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(_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(_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 (!IS_DG2(i915))
+ 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(>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(>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(_cdclk_state->actual,
+_cdclk_state->actual) &&
+new_cdclk_state->active_pipes ==
+old_cdclk_state->active_pipes)
+ return;
+
+ /* According to "Sequence Before Frequency
[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode
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)
v4: - Fixed some checkpatch warnings
Signed-off-by: Stanislav Lisovskiy
---
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..4d4ebe5fbee1 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(_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(_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(>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(>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(_cdclk_state->actual,
+_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 =
[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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
---
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(_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(_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(>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(>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(_cdclk_state->actual,
+_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 =
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
On Tue, 07 Mar 2023, "Lisovskiy, Stanislav"
wrote:
> On Tue, Mar 07, 2023 at 12:23:59PM +0200, Jani Nikula wrote:
>> On Tue, 07 Mar 2023, "Lisovskiy, Stanislav"
>> wrote:
>> > On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
>> >> On Mon, 27 Feb 2023, Stanislav Lisovskiy
>> >> 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
>> >> > ---
>> >> > 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(_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(_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(>uncore, SKL_PCODE_CDCLK_CONTROL,
>> >> > +
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
On Tue, Mar 07, 2023 at 12:23:59PM +0200, Jani Nikula wrote:
> On Tue, 07 Mar 2023, "Lisovskiy, Stanislav"
> wrote:
> > On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
> >> On Mon, 27 Feb 2023, Stanislav Lisovskiy
> >> 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
> >> > ---
> >> > 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(_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(_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(>uncore, SKL_PCODE_CDCLK_CONTROL,
> >> > +SKL_CDCLK_PREPARE_FOR_CHANGE |
> >> > +update_mask,
> >> > +
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
On Tue, 07 Mar 2023, "Lisovskiy, Stanislav"
wrote:
> On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
>> On Mon, 27 Feb 2023, Stanislav Lisovskiy
>> 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
>> > ---
>> > 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(_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(_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(>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(>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?
>
>>
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
On Mon, Mar 06, 2023 at 08:14:35PM +0200, Jani Nikula wrote:
> On Mon, 27 Feb 2023, Stanislav Lisovskiy
> 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
> > ---
> > 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(_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(_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(>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(>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(_cdclk_state->actual,
> > +
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
On Mon, 27 Feb 2023, Stanislav Lisovskiy 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
> ---
> 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(_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(_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(>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(>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(_cdclk_state->actual,
> + _cdclk_state->actual) &&
> + (new_cdclk_state->active_pipes ==
> + old_cdclk_state->active_pipes))
> + return;
> +
> + /* According to "Sequence Before
[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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
---
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(_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(_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(>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(>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(_cdclk_state->actual,
+_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);
+
+
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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-...@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
| Link:
https://lore.kernel.org/oe-kbuild-all/202302272052.zvlmgyjl-...@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(_cdclk_state->actual,
3015 _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
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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-...@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
| Link:
https://lore.kernel.org/oe-kbuild-all/202302272028.ppiibz9v-...@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(_cdclk_state->actual,
3015 _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
Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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-...@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
| Link:
https://lore.kernel.org/oe-kbuild-all/202302272011.1ilxtakr-...@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(_cdclk_state->actual,
3015 _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
[Intel-gfx] [PATCH] drm/i915/display: Communicate display power demands to pcode more accurately
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
---
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(_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(_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(>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(>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(_cdclk_state->actual,
+_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",
+
