[Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Stanislav Lisovskiy]

From: Jigar Bhatt <jigar.bhatt@intel.com>

Display to communicate "display 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 power well
will disable or enable.

v2: - Fixed identation(Stanislav Lisovskiy)
    - Made conditions more specific(in the commit we declare that
      we do this for DG2 only, however that commit changes >= to
      == for many other platforms.(Stanislav Lisovskiy)

Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c   | 89 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_cdclk.h   |  2 +
 drivers/gpu/drm/i915/display/intel_display.c |  6 ++
 drivers/gpu/drm/i915/i915_reg.h              |  4 +
 4 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 82da76b586ed..22ba0303ea28 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1908,10 +1908,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.
@@ -1922,7 +1922,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",
@@ -2218,6 +2218,34 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
 		    cdclk_config->voltage_level);
 }
 
+/**
+ * intel_display_to_pcode- inform pcode for display config
+ * @cdclk: current cdclk as per new or old state
+ * @voltage_level: current voltage_level send to Pcode
+ * @active_pipes: active pipes for more accurate power accounting
+ */
+static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
+				   unsigned int cdclk, u8 voltage_level,
+				   u8 active_pipes)
+{
+	int ret;
+
+	if (DISPLAY_VER(dev_priv) >= 12) {
+		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
+					SKL_CDCLK_PREPARE_FOR_CHANGE |
+					DISPLAY_TO_PCODE_MASK
+					(cdclk, active_pipes, voltage_level),
+					SKL_CDCLK_READY_FOR_CHANGE,
+					SKL_CDCLK_READY_FOR_CHANGE, 3);
+		if (ret) {
+			drm_err(&dev_priv->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
@@ -2287,6 +2315,61 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
 	}
 }
 
+/**
+ * intel_display_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_display_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);
+	if (!intel_cdclk_changed(&old_cdclk_state->actual,
+				&new_cdclk_state->actual) &&
+				(new_cdclk_state->active_pipes ==
+				old_cdclk_state->active_pipes))
+		return;
+	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
+		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
+					new_cdclk_state->actual.voltage_level,
+					new_cdclk_state->active_pipes);
+		return;
+	}
+	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
+		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
+					old_cdclk_state->actual.voltage_level,
+					old_cdclk_state->active_pipes);
+		return;
+	}
+	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
+		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
+					new_cdclk_state->actual.voltage_level,
+					new_cdclk_state->active_pipes);
+		return;
+	}
+	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
+				new_cdclk_state->actual.voltage_level,
+				new_cdclk_state->active_pipes);
+}
+
+/*intel_display_to_pcode_post_notification: after frequency change sending
+ * voltage_level, active pipes, current CDCLK frequency.
+ * @state: intel atomic state
+ */
+void intel_display_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);
+		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
+					new_cdclk_state->actual.voltage_level,
+					new_cdclk_state->active_pipes);
+}
+
 /**
  * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
  * @state: intel atomic state
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
index 51e2f6a11ce4..95d9d39d63be 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_display_to_pcode_pre_notification(struct intel_atomic_state *state);
+void intel_display_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/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 166662ade593..af0f0840e4b8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7462,6 +7462,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 
 	intel_atomic_prepare_plane_clear_colors(state);
 
+	if (IS_DG2(dev_priv))
+		intel_display_to_pcode_pre_notification(state);
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (intel_crtc_needs_modeset(new_crtc_state) ||
@@ -7483,6 +7486,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		intel_modeset_verify_disabled(dev_priv, state);
 	}
 
+	if (IS_DG2(dev_priv))
+		intel_display_to_pcode_post_notification(state);
+
 	intel_sagv_pre_plane_update(state);
 
 	/* Complete the events for pipes that have now been disabled */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 596efc940ee7..b90c31862083 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6498,6 +6498,10 @@
 #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_MASK(cdclk, num_pipes, voltage_level) \
+		((1 << 31) | (num_pipes << 28) | \
+		(cdclk << 16) | (1 << 27) | 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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/display: Communicate display configuration to pcode (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/display: Communicate display configuration to pcode (rev2)
URL   : https://patchwork.freedesktop.org/series/102678/
State : warning

== Summary ==

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

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Andi Shyti]

Hi Stanislav,

[...]

> +/**
> + * intel_display_to_pcode- inform pcode for display config
> + * @cdclk: current cdclk as per new or old state
> + * @voltage_level: current voltage_level send to Pcode
> + * @active_pipes: active pipes for more accurate power accounting
> + */
> +static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
> +				   unsigned int cdclk, u8 voltage_level,
> +				   u8 active_pipes)
> +{
> +	int ret;
> +

if (DISPLAY_VER(dev_priv) < 12)
	return;

the rest can go outside the if and we save one indentation leve.

BTW... /dev_priv/i915/ ?

> +	if (DISPLAY_VER(dev_priv) >= 12) {
> +		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> +					SKL_CDCLK_PREPARE_FOR_CHANGE |
> +					DISPLAY_TO_PCODE_MASK
> +					(cdclk, active_pipes, voltage_level),
> +					SKL_CDCLK_READY_FOR_CHANGE,
> +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> +		if (ret) {
> +			drm_err(&dev_priv->drm,
> +					"Failed to inform PCU about display config (err %d)\n",
> +					ret);
> +			return;
> +		}
> +	}
> +}

[...]

> +void intel_display_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);
> +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> +				&new_cdclk_state->actual) &&
> +				(new_cdclk_state->active_pipes ==
> +				old_cdclk_state->active_pipes))
> +		return;
> +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
> +					old_cdclk_state->actual.voltage_level,
> +					old_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> +				new_cdclk_state->actual.voltage_level,
> +				new_cdclk_state->active_pipes);

if you replace all the ifs with "else if"s you can have only one
return and remove all the brackets.

> +}
> +
> +/*intel_display_to_pcode_post_notification: after frequency change sending
> + * voltage_level, active pipes, current CDCLK frequency.
> + * @state: intel atomic state
> + */
> +void intel_display_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);
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);

the indentation here is off

> +}

Andi

[...]

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Jani Nikula]

On Wed, 08 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> From: Jigar Bhatt <jigar.bhatt@intel.com>
>
> Display to communicate "display 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 power well
> will disable or enable.
>
> v2: - Fixed identation(Stanislav Lisovskiy)
>     - Made conditions more specific(in the commit we declare that
>       we do this for DG2 only, however that commit changes >= to
>       == for many other platforms.(Stanislav Lisovskiy)
>
> Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c   | 89 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_cdclk.h   |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c |  6 ++
>  drivers/gpu/drm/i915/i915_reg.h              |  4 +
>  4 files changed, 98 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 82da76b586ed..22ba0303ea28 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1908,10 +1908,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.
> @@ -1922,7 +1922,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",
> @@ -2218,6 +2218,34 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
>  		    cdclk_config->voltage_level);
>  }
>  
> +/**
> + * intel_display_to_pcode- inform pcode for display config
> + * @cdclk: current cdclk as per new or old state
> + * @voltage_level: current voltage_level send to Pcode
> + * @active_pipes: active pipes for more accurate power accounting
> + */

kernel-doc is usually discoraged for internal functions.

> +static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
> +				   unsigned int cdclk, u8 voltage_level,
> +				   u8 active_pipes)
> +{
> +	int ret;
> +
> +	if (DISPLAY_VER(dev_priv) >= 12) {
> +		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> +					SKL_CDCLK_PREPARE_FOR_CHANGE |
> +					DISPLAY_TO_PCODE_MASK
> +					(cdclk, active_pipes, voltage_level),
> +					SKL_CDCLK_READY_FOR_CHANGE,
> +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> +		if (ret) {
> +			drm_err(&dev_priv->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
> @@ -2287,6 +2315,61 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +/**
> + * intel_display_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_display_to_pcode_pre_notification(struct intel_atomic_state *state)

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_*.

> +{
> +	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);
> +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> +				&new_cdclk_state->actual) &&
> +				(new_cdclk_state->active_pipes ==
> +				old_cdclk_state->active_pipes))
> +		return;
> +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
> +					old_cdclk_state->actual.voltage_level,
> +					old_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> +				new_cdclk_state->actual.voltage_level,
> +				new_cdclk_state->active_pipes);
> +}
> +
> +/*intel_display_to_pcode_post_notification: after frequency change sending
> + * voltage_level, active pipes, current CDCLK frequency.
> + * @state: intel atomic state
> + */
> +void intel_display_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);
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +}
> +
>  /**
>   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
>   * @state: intel atomic state
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 51e2f6a11ce4..95d9d39d63be 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_display_to_pcode_pre_notification(struct intel_atomic_state *state);
> +void intel_display_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/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 166662ade593..af0f0840e4b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7462,6 +7462,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  
>  	intel_atomic_prepare_plane_clear_colors(state);
>  
> +	if (IS_DG2(dev_priv))
> +		intel_display_to_pcode_pre_notification(state);
> +

This is very high level code and platform checks don't belong
here. Let's not start now.

>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (intel_crtc_needs_modeset(new_crtc_state) ||
> @@ -7483,6 +7486,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		intel_modeset_verify_disabled(dev_priv, state);
>  	}
>  
> +	if (IS_DG2(dev_priv))
> +		intel_display_to_pcode_post_notification(state);
> +
>  	intel_sagv_pre_plane_update(state);
>  
>  	/* Complete the events for pipes that have now been disabled */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 596efc940ee7..b90c31862083 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6498,6 +6498,10 @@
>  #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_MASK(cdclk, num_pipes, voltage_level) \
> +		((1 << 31) | (num_pipes << 28) | \
> +		(cdclk << 16) | (1 << 27) | 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

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Lisovskiy, Stanislav]

On Wed, Feb 08, 2023 at 01:46:40PM +0200, Jani Nikula wrote:
> On Wed, 08 Feb 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote:
> > From: Jigar Bhatt <jigar.bhatt@intel.com>
> >
> > Display to communicate "display 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 power well
> > will disable or enable.
> >
> > v2: - Fixed identation(Stanislav Lisovskiy)
> >     - Made conditions more specific(in the commit we declare that
> >       we do this for DG2 only, however that commit changes >= to
> >       == for many other platforms.(Stanislav Lisovskiy)
> >
> > Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c   | 89 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_cdclk.h   |  2 +
> >  drivers/gpu/drm/i915/display/intel_display.c |  6 ++
> >  drivers/gpu/drm/i915/i915_reg.h              |  4 +
> >  4 files changed, 98 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 82da76b586ed..22ba0303ea28 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1908,10 +1908,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.
> > @@ -1922,7 +1922,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",
> > @@ -2218,6 +2218,34 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >  		    cdclk_config->voltage_level);
> >  }
> >  
> > +/**
> > + * intel_display_to_pcode- inform pcode for display config
> > + * @cdclk: current cdclk as per new or old state
> > + * @voltage_level: current voltage_level send to Pcode
> > + * @active_pipes: active pipes for more accurate power accounting
> > + */
> 
> kernel-doc is usually discoraged for internal functions.
> 
> > +static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
> > +				   unsigned int cdclk, u8 voltage_level,
> > +				   u8 active_pipes)
> > +{
> > +	int ret;
> > +
> > +	if (DISPLAY_VER(dev_priv) >= 12) {
> > +		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> > +					SKL_CDCLK_PREPARE_FOR_CHANGE |
> > +					DISPLAY_TO_PCODE_MASK
> > +					(cdclk, active_pipes, voltage_level),
> > +					SKL_CDCLK_READY_FOR_CHANGE,
> > +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> > +		if (ret) {
> > +			drm_err(&dev_priv->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
> > @@ -2287,6 +2315,61 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +/**
> > + * intel_display_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_display_to_pcode_pre_notification(struct intel_atomic_state *state)
> 
> 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_*.
> 
> > +{
> > +	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);
> > +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > +				&new_cdclk_state->actual) &&
> > +				(new_cdclk_state->active_pipes ==
> > +				old_cdclk_state->active_pipes))
> > +		return;
> > +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
> > +		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
> > +					old_cdclk_state->actual.voltage_level,
> > +					old_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> > +				new_cdclk_state->actual.voltage_level,
> > +				new_cdclk_state->active_pipes);
> > +}
> > +
> > +/*intel_display_to_pcode_post_notification: after frequency change sending
> > + * voltage_level, active pipes, current CDCLK frequency.
> > + * @state: intel atomic state
> > + */
> > +void intel_display_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);
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +}
> > +
> >  /**
> >   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> >   * @state: intel atomic state
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index 51e2f6a11ce4..95d9d39d63be 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_display_to_pcode_pre_notification(struct intel_atomic_state *state);
> > +void intel_display_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/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 166662ade593..af0f0840e4b8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7462,6 +7462,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >  
> >  	intel_atomic_prepare_plane_clear_colors(state);
> >  
> > +	if (IS_DG2(dev_priv))
> > +		intel_display_to_pcode_pre_notification(state);
> > +
> 
> This is very high level code and platform checks don't belong
> here. Let's not start now.

Ohhh. Totally agree. That is one of those cases, when you wonder
"why I didn't spot this myself(((". 
Looks like I was a bit too soon with resending Jigar's patch, I took
over. Just looked briefly for some obvious things.
Will fix that.

Stan

> 
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (intel_crtc_needs_modeset(new_crtc_state) ||
> > @@ -7483,6 +7486,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >  		intel_modeset_verify_disabled(dev_priv, state);
> >  	}
> >  
> > +	if (IS_DG2(dev_priv))
> > +		intel_display_to_pcode_post_notification(state);
> > +
> >  	intel_sagv_pre_plane_update(state);
> >  
> >  	/* Complete the events for pipes that have now been disabled */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 596efc940ee7..b90c31862083 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6498,6 +6498,10 @@
> >  #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_MASK(cdclk, num_pipes, voltage_level) \
> > +		((1 << 31) | (num_pipes << 28) | \
> > +		(cdclk << 16) | (1 << 27) | 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

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Rodrigo Vivi]

On Wed, Feb 08, 2023 at 11:45:50AM +0200, Stanislav Lisovskiy wrote:
> From: Jigar Bhatt <jigar.bhatt@intel.com>
> 
> Display to communicate "display 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 power well
> will disable or enable.
> 
> v2: - Fixed identation(Stanislav Lisovskiy)
>     - Made conditions more specific(in the commit we declare that
>       we do this for DG2 only, however that commit changes >= to
>       == for many other platforms.(Stanislav Lisovskiy)


Is this a DG2 only thing? or more like DGFX in general?
what about DG1?

> 
> Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c   | 89 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_cdclk.h   |  2 +
>  drivers/gpu/drm/i915/display/intel_display.c |  6 ++
>  drivers/gpu/drm/i915/i915_reg.h              |  4 +
>  4 files changed, 98 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 82da76b586ed..22ba0303ea28 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1908,10 +1908,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.
> @@ -1922,7 +1922,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",
> @@ -2218,6 +2218,34 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
>  		    cdclk_config->voltage_level);
>  }
>  
> +/**
> + * intel_display_to_pcode- inform pcode for display config
> + * @cdclk: current cdclk as per new or old state
> + * @voltage_level: current voltage_level send to Pcode
> + * @active_pipes: active pipes for more accurate power accounting
> + */
> +static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
> +				   unsigned int cdclk, u8 voltage_level,
> +				   u8 active_pipes)
> +{
> +	int ret;
> +
> +	if (DISPLAY_VER(dev_priv) >= 12) {
> +		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> +					SKL_CDCLK_PREPARE_FOR_CHANGE |
> +					DISPLAY_TO_PCODE_MASK
> +					(cdclk, active_pipes, voltage_level),
> +					SKL_CDCLK_READY_FOR_CHANGE,
> +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> +		if (ret) {
> +			drm_err(&dev_priv->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
> @@ -2287,6 +2315,61 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +/**
> + * intel_display_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_display_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);
> +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> +				&new_cdclk_state->actual) &&
> +				(new_cdclk_state->active_pipes ==
> +				old_cdclk_state->active_pipes))
> +		return;
> +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
> +		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
> +					old_cdclk_state->actual.voltage_level,
> +					old_cdclk_state->active_pipes);
> +		return;
> +	}
> +	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +		return;
> +	}
> +	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> +				new_cdclk_state->actual.voltage_level,
> +				new_cdclk_state->active_pipes);
> +}
> +
> +/*intel_display_to_pcode_post_notification: after frequency change sending
> + * voltage_level, active pipes, current CDCLK frequency.
> + * @state: intel atomic state
> + */
> +void intel_display_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);
> +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> +					new_cdclk_state->actual.voltage_level,
> +					new_cdclk_state->active_pipes);
> +}
> +
>  /**
>   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
>   * @state: intel atomic state
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> index 51e2f6a11ce4..95d9d39d63be 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_display_to_pcode_pre_notification(struct intel_atomic_state *state);
> +void intel_display_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/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 166662ade593..af0f0840e4b8 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -7462,6 +7462,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  
>  	intel_atomic_prepare_plane_clear_colors(state);
>  
> +	if (IS_DG2(dev_priv))
> +		intel_display_to_pcode_pre_notification(state);
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (intel_crtc_needs_modeset(new_crtc_state) ||
> @@ -7483,6 +7486,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>  		intel_modeset_verify_disabled(dev_priv, state);
>  	}
>  
> +	if (IS_DG2(dev_priv))
> +		intel_display_to_pcode_post_notification(state);
> +
>  	intel_sagv_pre_plane_update(state);
>  
>  	/* Complete the events for pipes that have now been disabled */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 596efc940ee7..b90c31862083 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6498,6 +6498,10 @@
>  #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_MASK(cdclk, num_pipes, voltage_level) \
> +		((1 << 31) | (num_pipes << 28) | \
> +		(cdclk << 16) | (1 << 27) | 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
> 

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Lisovskiy, Stanislav]

On Wed, Feb 08, 2023 at 08:15:23AM -0500, Rodrigo Vivi wrote:
> On Wed, Feb 08, 2023 at 11:45:50AM +0200, Stanislav Lisovskiy wrote:
> > From: Jigar Bhatt <jigar.bhatt@intel.com>
> > 
> > Display to communicate "display 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 power well
> > will disable or enable.
> > 
> > v2: - Fixed identation(Stanislav Lisovskiy)
> >     - Made conditions more specific(in the commit we declare that
> >       we do this for DG2 only, however that commit changes >= to
> >       == for many other platforms.(Stanislav Lisovskiy)
> 
> 
> Is this a DG2 only thing? or more like DGFX in general?
> what about DG1?

Need to check this, took over this task from Jigar, so not fully
aware currently. At least my understanding and what commit says
is that this is for DG2. Agree need to check if we can extend this
to other platforms..

Stan

> 
> > 
> > Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c   | 89 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_cdclk.h   |  2 +
> >  drivers/gpu/drm/i915/display/intel_display.c |  6 ++
> >  drivers/gpu/drm/i915/i915_reg.h              |  4 +
> >  4 files changed, 98 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 82da76b586ed..22ba0303ea28 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1908,10 +1908,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.
> > @@ -1922,7 +1922,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",
> > @@ -2218,6 +2218,34 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >  		    cdclk_config->voltage_level);
> >  }
> >  
> > +/**
> > + * intel_display_to_pcode- inform pcode for display config
> > + * @cdclk: current cdclk as per new or old state
> > + * @voltage_level: current voltage_level send to Pcode
> > + * @active_pipes: active pipes for more accurate power accounting
> > + */
> > +static void intel_display_to_pcode(struct drm_i915_private *dev_priv,
> > +				   unsigned int cdclk, u8 voltage_level,
> > +				   u8 active_pipes)
> > +{
> > +	int ret;
> > +
> > +	if (DISPLAY_VER(dev_priv) >= 12) {
> > +		ret = skl_pcode_request(&dev_priv->uncore, SKL_PCODE_CDCLK_CONTROL,
> > +					SKL_CDCLK_PREPARE_FOR_CHANGE |
> > +					DISPLAY_TO_PCODE_MASK
> > +					(cdclk, active_pipes, voltage_level),
> > +					SKL_CDCLK_READY_FOR_CHANGE,
> > +					SKL_CDCLK_READY_FOR_CHANGE, 3);
> > +		if (ret) {
> > +			drm_err(&dev_priv->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
> > @@ -2287,6 +2315,61 @@ static void intel_set_cdclk(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +/**
> > + * intel_display_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_display_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);
> > +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > +				&new_cdclk_state->actual) &&
> > +				(new_cdclk_state->active_pipes ==
> > +				old_cdclk_state->active_pipes))
> > +		return;
> > +	if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk) {
> > +		intel_display_to_pcode(dev_priv, old_cdclk_state->actual.cdclk,
> > +					old_cdclk_state->actual.voltage_level,
> > +					old_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes) {
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +		return;
> > +	}
> > +	intel_display_to_pcode(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> > +				new_cdclk_state->actual.voltage_level,
> > +				new_cdclk_state->active_pipes);
> > +}
> > +
> > +/*intel_display_to_pcode_post_notification: after frequency change sending
> > + * voltage_level, active pipes, current CDCLK frequency.
> > + * @state: intel atomic state
> > + */
> > +void intel_display_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);
> > +		intel_display_to_pcode(dev_priv, new_cdclk_state->actual.cdclk,
> > +					new_cdclk_state->actual.voltage_level,
> > +					new_cdclk_state->active_pipes);
> > +}
> > +
> >  /**
> >   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> >   * @state: intel atomic state
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h
> > index 51e2f6a11ce4..95d9d39d63be 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_display_to_pcode_pre_notification(struct intel_atomic_state *state);
> > +void intel_display_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/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 166662ade593..af0f0840e4b8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7462,6 +7462,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >  
> >  	intel_atomic_prepare_plane_clear_colors(state);
> >  
> > +	if (IS_DG2(dev_priv))
> > +		intel_display_to_pcode_pre_notification(state);
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (intel_crtc_needs_modeset(new_crtc_state) ||
> > @@ -7483,6 +7486,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
> >  		intel_modeset_verify_disabled(dev_priv, state);
> >  	}
> >  
> > +	if (IS_DG2(dev_priv))
> > +		intel_display_to_pcode_post_notification(state);
> > +
> >  	intel_sagv_pre_plane_update(state);
> >  
> >  	/* Complete the events for pipes that have now been disabled */
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 596efc940ee7..b90c31862083 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6498,6 +6498,10 @@
> >  #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_MASK(cdclk, num_pipes, voltage_level) \
> > +		((1 << 31) | (num_pipes << 28) | \
> > +		(cdclk << 16) | (1 << 27) | 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
> > 

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/display: Communicate display configuration to pcode (rev2)

[Patchwork]

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

== Series Details ==

Series: drm/i915/display: Communicate display configuration to pcode (rev2)
URL   : https://patchwork.freedesktop.org/series/102678/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12712 -> Patchwork_102678v2
====================================================

Summary
-------

  **FAILURE**

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

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

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

  Additional (1): fi-kbl-soraka 
  Missing    (2): bat-atsm-1 fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-cfl-8700k:       [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/fi-cfl-8700k/igt@i915_suspend@basic-s2idle-without-i915.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-cfl-8700k/igt@i915_suspend@basic-s2idle-without-i915.html

  
#### Suppressed ####

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

  * igt@i915_module_load@load:
    - {bat-dg2-11}:       [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/bat-dg2-11/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/bat-dg2-11/igt@i915_module_load@load.html
    - {bat-dg2-9}:        [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/bat-dg2-9/igt@i915_module_load@load.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/bat-dg2-9/igt@i915_module_load@load.html
    - {bat-dg2-8}:        [PASS][7] -> [ABORT][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/bat-dg2-8/igt@i915_module_load@load.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/bat-dg2-8/igt@i915_module_load@load.html

  
New tests
---------

  New tests have been introduced between CI_DRM_12712 and Patchwork_102678v2:

### New IGT tests (4) ###

  * igt@gem_exec_fence@basic-wait:
    - Statuses :
    - Exec time: [None] s

  * igt@gem_exec_fence@nb-await:
    - Statuses :
    - Exec time: [None] s

  * igt@i915_pm_rpm@module-reload:
    - Statuses : 29 pass(s) 5 skip(s)
    - Exec time: [0.0] s

  * igt@i915_selftest@live:
    - Statuses :
    - Exec time: [None] s

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#2190])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-kbl-soraka/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#4613]) +3 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-kbl-soraka/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@execlists:
    - fi-kbl-soraka:      NOTRUN -> [INCOMPLETE][11] ([i915#7156] / [i915#7913])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-kbl-soraka/igt@i915_selftest@live@execlists.html

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

  * igt@i915_selftest@live@gt_pm:
    - fi-kbl-soraka:      NOTRUN -> [DMESG-FAIL][14] ([i915#1886])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-kbl-soraka/igt@i915_selftest@live@gt_pm.html

  * igt@kms_chamelium_frames@hdmi-crc-fast:
    - fi-kbl-soraka:      NOTRUN -> [SKIP][15] ([fdo#109271]) +15 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-kbl-soraka/igt@kms_chamelium_frames@hdmi-crc-fast.html

  
#### Possible fixes ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [FAIL][16] ([i915#7229]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  * igt@i915_selftest@live@reset:
    - {bat-rpls-2}:       [ABORT][18] ([i915#4983]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12712/bat-rpls-2/igt@i915_selftest@live@reset.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_102678v2/bat-rpls-2/igt@i915_selftest@live@reset.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1886]: https://gitlab.freedesktop.org/drm/intel/issues/1886
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7156]: https://gitlab.freedesktop.org/drm/intel/issues/7156
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981


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

  * Linux: CI_DRM_12712 -> Patchwork_102678v2

  CI-20190529: 20190529
  CI_DRM_12712: 579f8b992ea3f5cd11bd803dd0585a7ddb006a13 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7153: f47f859f13376958a2bd199423b1f0ff53dddbe0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_102678v2: 579f8b992ea3f5cd11bd803dd0585a7ddb006a13 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

34b7e79ad857 drm/i915/display: Communicate display configuration to pcode

== Logs ==

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

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

[Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Stanislav Lisovskiy]

From: Jigar Bhatt <jigar.bhatt@intel.com>

Display to communicate "display 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 power well
will disable or enable.

v2: - Fixed identation(Stanislav Lisovskiy)
    - Made conditions more specific(in the commit we declare that
      we do this for DG2 only, however that commit changes >= to
      == for many other platforms.(Stanislav Lisovskiy)

v3: - Refactored code for proper identation and smaller conditions
      (Andi Shyti)
    - Switched to proper function naming, removed platform specific
      code from intel_atomic_commit_tail(Jani Nikula)
    - Moved intel_cdclk_power_usage_to_pcode_pre/post_notification
      to proper places, before and after setting CDCLK(Stanislav Lisovskiy)

Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 97 ++++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_cdclk.h |  2 +
 drivers/gpu/drm/i915/i915_reg.h            |  4 +
 3 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
index 82da76b586ed..4f8bcc0b51e8 100644
--- a/drivers/gpu/drm/i915/display/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
@@ -1908,10 +1908,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.
@@ -1922,7 +1922,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",
@@ -2218,6 +2218,29 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
 		    cdclk_config->voltage_level);
 }
 
+static void intel_pcode_notify(struct drm_i915_private *i915,
+			       unsigned int cdclk, u8 voltage_level,
+			       u8 active_pipes)
+{
+	int ret;
+
+	if (DISPLAY_VER(i915) < 12)
+		return;
+
+	ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
+				SKL_CDCLK_PREPARE_FOR_CHANGE |
+				DISPLAY_TO_PCODE_MASK
+				(cdclk, active_pipes, voltage_level),
+				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
@@ -2287,6 +2310,56 @@ 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);
+	if (!intel_cdclk_changed(&old_cdclk_state->actual,
+				 &new_cdclk_state->actual) &&
+				 (new_cdclk_state->active_pipes ==
+				 old_cdclk_state->active_pipes))
+		return;
+	else if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk)
+		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
+				   new_cdclk_state->actual.voltage_level,
+				   new_cdclk_state->active_pipes);
+	else if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk)
+		intel_pcode_notify(dev_priv, old_cdclk_state->actual.cdclk,
+				   old_cdclk_state->actual.voltage_level,
+				   old_cdclk_state->active_pipes);
+	else if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes)
+		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
+				   new_cdclk_state->actual.voltage_level,
+				   new_cdclk_state->active_pipes);
+
+	intel_pcode_notify(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
+			   new_cdclk_state->actual.voltage_level,
+			   new_cdclk_state->active_pipes);
+}
+
+/* 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);
+		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
+				   new_cdclk_state->actual.voltage_level,
+				   new_cdclk_state->active_pipes);
+}
+
 /**
  * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
  * @state: intel atomic state
@@ -2297,7 +2370,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 =
@@ -2308,11 +2381,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);
 	}
 }
 
@@ -2326,7 +2402,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 =
@@ -2337,11 +2413,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);
 	}
 }
 
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 596efc940ee7..b90c31862083 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6498,6 +6498,10 @@
 #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_MASK(cdclk, num_pipes, voltage_level) \
+		((1 << 31) | (num_pipes << 28) | \
+		(cdclk << 16) | (1 << 27) | 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

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Ville Syrjälä]

On Tue, Feb 14, 2023 at 12:01:32PM +0200, Stanislav Lisovskiy wrote:
> From: Jigar Bhatt <jigar.bhatt@intel.com>
> 
> Display to communicate "display 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 power well
> will disable or enable.
> 
> v2: - Fixed identation(Stanislav Lisovskiy)
>     - Made conditions more specific(in the commit we declare that
>       we do this for DG2 only, however that commit changes >= to
>       == for many other platforms.(Stanislav Lisovskiy)
> 
> v3: - Refactored code for proper identation and smaller conditions
>       (Andi Shyti)
>     - Switched to proper function naming, removed platform specific
>       code from intel_atomic_commit_tail(Jani Nikula)
>     - Moved intel_cdclk_power_usage_to_pcode_pre/post_notification
>       to proper places, before and after setting CDCLK(Stanislav Lisovskiy)
> 
> Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cdclk.c | 97 ++++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_cdclk.h |  2 +
>  drivers/gpu/drm/i915/i915_reg.h            |  4 +
>  3 files changed, 94 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 82da76b586ed..4f8bcc0b51e8 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -1908,10 +1908,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.
> @@ -1922,7 +1922,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",
> @@ -2218,6 +2218,29 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
>  		    cdclk_config->voltage_level);
>  }
>  
> +static void intel_pcode_notify(struct drm_i915_private *i915,
> +			       unsigned int cdclk, u8 voltage_level,
> +			       u8 active_pipes)
> +{
> +	int ret;
> +
> +	if (DISPLAY_VER(i915) < 12)
> +		return;
> +
> +	ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> +				SKL_CDCLK_PREPARE_FOR_CHANGE |

Isn't that something we're supposed to set only *before*
the change? Here it looks like we're setting also for the
post call.

> +				DISPLAY_TO_PCODE_MASK
> +				(cdclk, active_pipes, voltage_level),
> +				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
> @@ -2287,6 +2310,56 @@ 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);
> +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> +				 &new_cdclk_state->actual) &&
> +				 (new_cdclk_state->active_pipes ==
> +				 old_cdclk_state->active_pipes))
> +		return;
> +	else if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk)
> +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> +				   new_cdclk_state->actual.voltage_level,
> +				   new_cdclk_state->active_pipes);
> +	else if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk)
> +		intel_pcode_notify(dev_priv, old_cdclk_state->actual.cdclk,
> +				   old_cdclk_state->actual.voltage_level,
> +				   old_cdclk_state->active_pipes);
> +	else if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes)
> +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> +				   new_cdclk_state->actual.voltage_level,
> +				   new_cdclk_state->active_pipes);
> +
> +	intel_pcode_notify(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> +			   new_cdclk_state->actual.voltage_level,
> +			   new_cdclk_state->active_pipes);

I don't understand what is going on here. Are we trying to
say something like

intel_pcode_notify(...,
		   max(old_cdclk, new_cdclk),
		   max(old_voltage_level, new_voltage_level),
		   old_active_pipes | new_active_pipes);
?

Also the inclusion of the pipes here would imply that
we need to think about serialize vs. lock.

> +}
> +
> +/* 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);
> +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> +				   new_cdclk_state->actual.voltage_level,
> +				   new_cdclk_state->active_pipes);
> +}
> +
>  /**
>   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
>   * @state: intel atomic state
> @@ -2297,7 +2370,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 =
> @@ -2308,11 +2381,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);
>  	}
>  }
>  
> @@ -2326,7 +2402,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 =
> @@ -2337,11 +2413,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);
>  	}
>  }
>  
> 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 596efc940ee7..b90c31862083 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6498,6 +6498,10 @@
>  #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_MASK(cdclk, num_pipes, voltage_level) \
> +		((1 << 31) | (num_pipes << 28) | \
> +		(cdclk << 16) | (1 << 27) | 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

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Lisovskiy, Stanislav]

On Tue, Feb 14, 2023 at 04:04:05PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 14, 2023 at 12:01:32PM +0200, Stanislav Lisovskiy wrote:
> > From: Jigar Bhatt <jigar.bhatt@intel.com>
> > 
> > Display to communicate "display 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 power well
> > will disable or enable.
> > 
> > v2: - Fixed identation(Stanislav Lisovskiy)
> >     - Made conditions more specific(in the commit we declare that
> >       we do this for DG2 only, however that commit changes >= to
> >       == for many other platforms.(Stanislav Lisovskiy)
> > 
> > v3: - Refactored code for proper identation and smaller conditions
> >       (Andi Shyti)
> >     - Switched to proper function naming, removed platform specific
> >       code from intel_atomic_commit_tail(Jani Nikula)
> >     - Moved intel_cdclk_power_usage_to_pcode_pre/post_notification
> >       to proper places, before and after setting CDCLK(Stanislav Lisovskiy)
> > 
> > Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 97 ++++++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_cdclk.h |  2 +
> >  drivers/gpu/drm/i915/i915_reg.h            |  4 +
> >  3 files changed, 94 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 82da76b586ed..4f8bcc0b51e8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -1908,10 +1908,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.
> > @@ -1922,7 +1922,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",
> > @@ -2218,6 +2218,29 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> >  		    cdclk_config->voltage_level);
> >  }
> >  
> > +static void intel_pcode_notify(struct drm_i915_private *i915,
> > +			       unsigned int cdclk, u8 voltage_level,
> > +			       u8 active_pipes)
> > +{
> > +	int ret;
> > +
> > +	if (DISPLAY_VER(i915) < 12)
> > +		return;
> > +
> > +	ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> > +				SKL_CDCLK_PREPARE_FOR_CHANGE |
> 
> Isn't that something we're supposed to set only *before*
> the change? Here it looks like we're setting also for the
> post call.

According to BSpec, we are supposed to send notifications both
before and after setting CDCLK:

1) Before the change we need to set voltage bits to the max(bits 1:0 set to 3)
2) If CDCLK is increasing set to increased value or leave the same
3) If enabling pipe write upcoming pipe count

After the change 

1) we set required voltage level
2) write CDCLK bits if CDCLK is disabled
3) If pipe is disabled write the current pipe count

I agree, current code in fact doesn't do it exactly as described.

> 
> > +				DISPLAY_TO_PCODE_MASK
> > +				(cdclk, active_pipes, voltage_level),
> > +				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
> > @@ -2287,6 +2310,56 @@ 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);
> > +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > +				 &new_cdclk_state->actual) &&
> > +				 (new_cdclk_state->active_pipes ==
> > +				 old_cdclk_state->active_pipes))
> > +		return;
> > +	else if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk)
> > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > +				   new_cdclk_state->actual.voltage_level,
> > +				   new_cdclk_state->active_pipes);
> > +	else if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk)
> > +		intel_pcode_notify(dev_priv, old_cdclk_state->actual.cdclk,
> > +				   old_cdclk_state->actual.voltage_level,
> > +				   old_cdclk_state->active_pipes);
> > +	else if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes)
> > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > +				   new_cdclk_state->actual.voltage_level,
> > +				   new_cdclk_state->active_pipes);
> > +
> > +	intel_pcode_notify(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> > +			   new_cdclk_state->actual.voltage_level,
> > +			   new_cdclk_state->active_pipes);
> 
> I don't understand what is going on here. Are we trying to
> say something like
> 
> intel_pcode_notify(...,
> 		   max(old_cdclk, new_cdclk),
> 		   max(old_voltage_level, new_voltage_level),
> 		   old_active_pipes | new_active_pipes);

Regarding CDCLK yes, for voltage level, just read out from BSpec 
that we are supposed to set voltage level to to max(3) always before
the change. Dunno why this was implemented that way initially(BSpec got updated?)


> ?
> 
> Also the inclusion of the pipes here would imply that
> we need to think about serialize vs. lock.

Should we already had locked/serialized global state in intel_modeset_calc_cdclk,
if active_pipes/cdclk had changed?

Stan

> 
> > +}
> > +
> > +/* 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);
> > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > +				   new_cdclk_state->actual.voltage_level,
> > +				   new_cdclk_state->active_pipes);
> > +}
> > +
> >  /**
> >   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> >   * @state: intel atomic state
> > @@ -2297,7 +2370,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 =
> > @@ -2308,11 +2381,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);
> >  	}
> >  }
> >  
> > @@ -2326,7 +2402,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 =
> > @@ -2337,11 +2413,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);
> >  	}
> >  }
> >  
> > 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 596efc940ee7..b90c31862083 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6498,6 +6498,10 @@
> >  #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_MASK(cdclk, num_pipes, voltage_level) \
> > +		((1 << 31) | (num_pipes << 28) | \
> > +		(cdclk << 16) | (1 << 27) | 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
> 
> -- 
> Ville Syrjälä
> Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Ville Syrjälä]

On Mon, Feb 20, 2023 at 05:45:45PM +0200, Lisovskiy, Stanislav wrote:
> On Tue, Feb 14, 2023 at 04:04:05PM +0200, Ville Syrjälä wrote:
> > On Tue, Feb 14, 2023 at 12:01:32PM +0200, Stanislav Lisovskiy wrote:
> > > From: Jigar Bhatt <jigar.bhatt@intel.com>
> > > 
> > > Display to communicate "display 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 power well
> > > will disable or enable.
> > > 
> > > v2: - Fixed identation(Stanislav Lisovskiy)
> > >     - Made conditions more specific(in the commit we declare that
> > >       we do this for DG2 only, however that commit changes >= to
> > >       == for many other platforms.(Stanislav Lisovskiy)
> > > 
> > > v3: - Refactored code for proper identation and smaller conditions
> > >       (Andi Shyti)
> > >     - Switched to proper function naming, removed platform specific
> > >       code from intel_atomic_commit_tail(Jani Nikula)
> > >     - Moved intel_cdclk_power_usage_to_pcode_pre/post_notification
> > >       to proper places, before and after setting CDCLK(Stanislav Lisovskiy)
> > > 
> > > Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 97 ++++++++++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_cdclk.h |  2 +
> > >  drivers/gpu/drm/i915/i915_reg.h            |  4 +
> > >  3 files changed, 94 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > index 82da76b586ed..4f8bcc0b51e8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > @@ -1908,10 +1908,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.
> > > @@ -1922,7 +1922,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",
> > > @@ -2218,6 +2218,29 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> > >  		    cdclk_config->voltage_level);
> > >  }
> > >  
> > > +static void intel_pcode_notify(struct drm_i915_private *i915,
> > > +			       unsigned int cdclk, u8 voltage_level,
> > > +			       u8 active_pipes)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (DISPLAY_VER(i915) < 12)
> > > +		return;
> > > +
> > > +	ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> > > +				SKL_CDCLK_PREPARE_FOR_CHANGE |
> > 
> > Isn't that something we're supposed to set only *before*
> > the change? Here it looks like we're setting also for the
> > post call.
> 
> According to BSpec, we are supposed to send notifications both
> before and after setting CDCLK:
> 
> 1) Before the change we need to set voltage bits to the max(bits 1:0 set to 3)

You are doing that for both pre and post now.

> 2) If CDCLK is increasing set to increased value or leave the same
> 3) If enabling pipe write upcoming pipe count
> 
> After the change 
> 
> 1) we set required voltage level
> 2) write CDCLK bits if CDCLK is disabled
> 3) If pipe is disabled write the current pipe count
> 
> I agree, current code in fact doesn't do it exactly as described.
> 
> > 
> > > +				DISPLAY_TO_PCODE_MASK
> > > +				(cdclk, active_pipes, voltage_level),
> > > +				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
> > > @@ -2287,6 +2310,56 @@ 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);
> > > +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > > +				 &new_cdclk_state->actual) &&
> > > +				 (new_cdclk_state->active_pipes ==
> > > +				 old_cdclk_state->active_pipes))
> > > +		return;
> > > +	else if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk)
> > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > +				   new_cdclk_state->actual.voltage_level,
> > > +				   new_cdclk_state->active_pipes);
> > > +	else if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk)
> > > +		intel_pcode_notify(dev_priv, old_cdclk_state->actual.cdclk,
> > > +				   old_cdclk_state->actual.voltage_level,
> > > +				   old_cdclk_state->active_pipes);
> > > +	else if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes)
> > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > +				   new_cdclk_state->actual.voltage_level,
> > > +				   new_cdclk_state->active_pipes);
> > > +
> > > +	intel_pcode_notify(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> > > +			   new_cdclk_state->actual.voltage_level,
> > > +			   new_cdclk_state->active_pipes);
> > 
> > I don't understand what is going on here. Are we trying to
> > say something like
> > 
> > intel_pcode_notify(...,
> > 		   max(old_cdclk, new_cdclk),
> > 		   max(old_voltage_level, new_voltage_level),
> > 		   old_active_pipes | new_active_pipes);
> 
> Regarding CDCLK yes, for voltage level, just read out from BSpec 
> that we are supposed to set voltage level to to max(3) always before
> the change. Dunno why this was implemented that way initially(BSpec got updated?)

Pretty sure it's always been like that for all platforms. Dunno why.

> 
> 
> > ?
> > 
> > Also the inclusion of the pipes here would imply that
> > we need to think about serialize vs. lock.
> 
> Should we already had locked/serialized global state in intel_modeset_calc_cdclk,
> if active_pipes/cdclk had changed?

For active_pipes in particular, locked yes, serialized no since no other
platform needs us to poke the hardware state on active_pipes changes.
The lock is what protects the software state, serialize is what
guarantees the hardware state updates happen in the correct order.

> 
> Stan
> 
> > 
> > > +}
> > > +
> > > +/* 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);
> > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > +				   new_cdclk_state->actual.voltage_level,
> > > +				   new_cdclk_state->active_pipes);
> > > +}
> > > +
> > >  /**
> > >   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> > >   * @state: intel atomic state
> > > @@ -2297,7 +2370,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 =
> > > @@ -2308,11 +2381,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);
> > >  	}
> > >  }
> > >  
> > > @@ -2326,7 +2402,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 =
> > > @@ -2337,11 +2413,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);
> > >  	}
> > >  }
> > >  
> > > 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 596efc940ee7..b90c31862083 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6498,6 +6498,10 @@
> > >  #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_MASK(cdclk, num_pipes, voltage_level) \
> > > +		((1 << 31) | (num_pipes << 28) | \
> > > +		(cdclk << 16) | (1 << 27) | 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
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH] drm/i915/display: Communicate display configuration to pcode

[Lisovskiy, Stanislav]

On Mon, Feb 20, 2023 at 06:14:58PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 20, 2023 at 05:45:45PM +0200, Lisovskiy, Stanislav wrote:
> > On Tue, Feb 14, 2023 at 04:04:05PM +0200, Ville Syrjälä wrote:
> > > On Tue, Feb 14, 2023 at 12:01:32PM +0200, Stanislav Lisovskiy wrote:
> > > > From: Jigar Bhatt <jigar.bhatt@intel.com>
> > > > 
> > > > Display to communicate "display 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 power well
> > > > will disable or enable.
> > > > 
> > > > v2: - Fixed identation(Stanislav Lisovskiy)
> > > >     - Made conditions more specific(in the commit we declare that
> > > >       we do this for DG2 only, however that commit changes >= to
> > > >       == for many other platforms.(Stanislav Lisovskiy)
> > > > 
> > > > v3: - Refactored code for proper identation and smaller conditions
> > > >       (Andi Shyti)
> > > >     - Switched to proper function naming, removed platform specific
> > > >       code from intel_atomic_commit_tail(Jani Nikula)
> > > >     - Moved intel_cdclk_power_usage_to_pcode_pre/post_notification
> > > >       to proper places, before and after setting CDCLK(Stanislav Lisovskiy)
> > > > 
> > > > Signed-off-by: Jigar Bhatt <jigar.bhatt@intel.com>
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 97 ++++++++++++++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.h |  2 +
> > > >  drivers/gpu/drm/i915/i915_reg.h            |  4 +
> > > >  3 files changed, 94 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 82da76b586ed..4f8bcc0b51e8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -1908,10 +1908,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.
> > > > @@ -1922,7 +1922,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",
> > > > @@ -2218,6 +2218,29 @@ void intel_cdclk_dump_config(struct drm_i915_private *i915,
> > > >  		    cdclk_config->voltage_level);
> > > >  }
> > > >  
> > > > +static void intel_pcode_notify(struct drm_i915_private *i915,
> > > > +			       unsigned int cdclk, u8 voltage_level,
> > > > +			       u8 active_pipes)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	if (DISPLAY_VER(i915) < 12)
> > > > +		return;
> > > > +
> > > > +	ret = skl_pcode_request(&i915->uncore, SKL_PCODE_CDCLK_CONTROL,
> > > > +				SKL_CDCLK_PREPARE_FOR_CHANGE |
> > > 
> > > Isn't that something we're supposed to set only *before*
> > > the change? Here it looks like we're setting also for the
> > > post call.
> > 
> > According to BSpec, we are supposed to send notifications both
> > before and after setting CDCLK:
> > 
> > 1) Before the change we need to set voltage bits to the max(bits 1:0 set to 3)
> 
> You are doing that for both pre and post now.
> 
> > 2) If CDCLK is increasing set to increased value or leave the same
> > 3) If enabling pipe write upcoming pipe count
> > 
> > After the change 
> > 
> > 1) we set required voltage level
> > 2) write CDCLK bits if CDCLK is disabled
> > 3) If pipe is disabled write the current pipe count
> > 
> > I agree, current code in fact doesn't do it exactly as described.
> > 
> > > 
> > > > +				DISPLAY_TO_PCODE_MASK
> > > > +				(cdclk, active_pipes, voltage_level),
> > > > +				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
> > > > @@ -2287,6 +2310,56 @@ 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);
> > > > +	if (!intel_cdclk_changed(&old_cdclk_state->actual,
> > > > +				 &new_cdclk_state->actual) &&
> > > > +				 (new_cdclk_state->active_pipes ==
> > > > +				 old_cdclk_state->active_pipes))
> > > > +		return;
> > > > +	else if (old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk)
> > > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > > +				   new_cdclk_state->actual.voltage_level,
> > > > +				   new_cdclk_state->active_pipes);
> > > > +	else if (old_cdclk_state->actual.cdclk >= new_cdclk_state->actual.cdclk)
> > > > +		intel_pcode_notify(dev_priv, old_cdclk_state->actual.cdclk,
> > > > +				   old_cdclk_state->actual.voltage_level,
> > > > +				   old_cdclk_state->active_pipes);
> > > > +	else if (old_cdclk_state->active_pipes != new_cdclk_state->active_pipes)
> > > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > > +				   new_cdclk_state->actual.voltage_level,
> > > > +				   new_cdclk_state->active_pipes);
> > > > +
> > > > +	intel_pcode_notify(dev_priv, DISPLAY_TO_PCODE_CDCLK_MAX,
> > > > +			   new_cdclk_state->actual.voltage_level,
> > > > +			   new_cdclk_state->active_pipes);
> > > 
> > > I don't understand what is going on here. Are we trying to
> > > say something like
> > > 
> > > intel_pcode_notify(...,
> > > 		   max(old_cdclk, new_cdclk),
> > > 		   max(old_voltage_level, new_voltage_level),
> > > 		   old_active_pipes | new_active_pipes);
> > 
> > Regarding CDCLK yes, for voltage level, just read out from BSpec 
> > that we are supposed to set voltage level to to max(3) always before
> > the change. Dunno why this was implemented that way initially(BSpec got updated?)
> 
> Pretty sure it's always been like that for all platforms. Dunno why.
> 
> > 
> > 
> > > ?
> > > 
> > > Also the inclusion of the pipes here would imply that
> > > we need to think about serialize vs. lock.
> > 
> > Should we already had locked/serialized global state in intel_modeset_calc_cdclk,
> > if active_pipes/cdclk had changed?
> 
> For active_pipes in particular, locked yes, serialized no since no other
> platform needs us to poke the hardware state on active_pipes changes.
> The lock is what protects the software state, serialize is what
> guarantees the hardware state updates happen in the correct order.

Okay, then I need to extend serializing also active_pipes change in
intel_modeset_calc_cdclk.

As a side note, wonder if we could make some kind of automated way to determine when we 
need to serialize or lock global state, i.e smth like
cdclk_state_is_serialize_needed(new_cdclk_state, old_cdclk_state) 
or cdclk_state_is_global_lock_needed. Have similar helpers to compare bw_state and other
states also.
So that we don't have to hardcode all this logic, also will make this look more intuitively
understandable.

> 
> > 
> > Stan
> > 
> > > 
> > > > +}
> > > > +
> > > > +/* 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);
> > > > +		intel_pcode_notify(dev_priv, new_cdclk_state->actual.cdclk,
> > > > +				   new_cdclk_state->actual.voltage_level,
> > > > +				   new_cdclk_state->active_pipes);
> > > > +}
> > > > +
> > > >  /**
> > > >   * intel_set_cdclk_pre_plane_update - Push the CDCLK state to the hardware
> > > >   * @state: intel atomic state
> > > > @@ -2297,7 +2370,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 =
> > > > @@ -2308,11 +2381,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);
> > > >  	}
> > > >  }
> > > >  
> > > > @@ -2326,7 +2402,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 =
> > > > @@ -2337,11 +2413,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);
> > > >  	}
> > > >  }
> > > >  
> > > > 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 596efc940ee7..b90c31862083 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6498,6 +6498,10 @@
> > > >  #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_MASK(cdclk, num_pipes, voltage_level) \
> > > > +		((1 << 31) | (num_pipes << 28) | \
> > > > +		(cdclk << 16) | (1 << 27) | 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
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel