[Intel-gfx] [RFC 0/3] drm/i915/dmc: allocate dmc struct dynamically

[Intel-gfx] [RFC 0/3] drm/i915/dmc: allocate dmc struct dynamically

[Jani Nikula]

Allocate the >1k dmc struct dynamically, only for platforms that need
it.

Jani Nikula (3):
  drm/i915/power: move dc state members to struct i915_power_domains
  drm/i915/dmc: drop "ucode" from function names
  drm/i915/dmc: allocate dmc structure dynamically

 drivers/gpu/drm/i915/display/intel_display.c  |   6 +-
 .../gpu/drm/i915/display/intel_display_core.h |   8 +-
 .../drm/i915/display/intel_display_power.c    |  25 +--
 .../drm/i915/display/intel_display_power.h    |   4 +
 .../i915/display/intel_display_power_well.c   |  31 ++--
 drivers/gpu/drm/i915/display/intel_dmc.c      | 159 ++++++++++++------
 drivers/gpu/drm/i915/display/intel_dmc.h      |  44 +----
 .../drm/i915/display/intel_modeset_setup.c    |   1 +
 drivers/gpu/drm/i915/display/intel_psr.c      |   3 +-
 drivers/gpu/drm/i915/i915_driver.c            |   6 +-
 10 files changed, 164 insertions(+), 123 deletions(-)

-- 
2.34.1

[Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Jani Nikula]

There's only one reference to the struct intel_dmc members dc_state,
target_dc_state, and allowed_dc_mask within intel_dmc.c, begging the
question why they are under struct intel_dmc to begin with.

Moreover, the only references to i915->display.dmc outside of
intel_dmc.c are to these members.

They don't belong. Move them from struct intel_dmc to struct
i915_power_domains, which seems like a more suitable place.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 25 ++++++++-------
 .../drm/i915/display/intel_display_power.h    |  4 +++
 .../i915/display/intel_display_power_well.c   | 31 +++++++++++--------
 drivers/gpu/drm/i915/display/intel_dmc.c      |  3 +-
 drivers/gpu/drm/i915/display/intel_dmc.h      |  3 --
 drivers/gpu/drm/i915/display/intel_psr.c      |  3 +-
 6 files changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 7222502a760c..4ed7e50e1c21 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -264,9 +264,10 @@ bool intel_display_power_is_enabled(struct drm_i915_private *dev_priv,
 }
 
 static u32
-sanitize_target_dc_state(struct drm_i915_private *dev_priv,
+sanitize_target_dc_state(struct drm_i915_private *i915,
 			 u32 target_dc_state)
 {
+	struct i915_power_domains *power_domains = &i915->display.power.domains;
 	static const u32 states[] = {
 		DC_STATE_EN_UPTO_DC6,
 		DC_STATE_EN_UPTO_DC5,
@@ -279,7 +280,7 @@ sanitize_target_dc_state(struct drm_i915_private *dev_priv,
 		if (target_dc_state != states[i])
 			continue;
 
-		if (dev_priv->display.dmc.allowed_dc_mask & target_dc_state)
+		if (power_domains->allowed_dc_mask & target_dc_state)
 			break;
 
 		target_dc_state = states[i + 1];
@@ -312,7 +313,7 @@ void intel_display_power_set_target_dc_state(struct drm_i915_private *dev_priv,
 
 	state = sanitize_target_dc_state(dev_priv, state);
 
-	if (state == dev_priv->display.dmc.target_dc_state)
+	if (state == power_domains->target_dc_state)
 		goto unlock;
 
 	dc_off_enabled = intel_power_well_is_enabled(dev_priv, power_well);
@@ -323,7 +324,7 @@ void intel_display_power_set_target_dc_state(struct drm_i915_private *dev_priv,
 	if (!dc_off_enabled)
 		intel_power_well_enable(dev_priv, power_well);
 
-	dev_priv->display.dmc.target_dc_state = state;
+	power_domains->target_dc_state = state;
 
 	if (!dc_off_enabled)
 		intel_power_well_disable(dev_priv, power_well);
@@ -992,10 +993,10 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
 	dev_priv->params.disable_power_well =
 		sanitize_disable_power_well_option(dev_priv,
 						   dev_priv->params.disable_power_well);
-	dev_priv->display.dmc.allowed_dc_mask =
+	power_domains->allowed_dc_mask =
 		get_allowed_dc_mask(dev_priv, dev_priv->params.enable_dc);
 
-	dev_priv->display.dmc.target_dc_state =
+	power_domains->target_dc_state =
 		sanitize_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
 
 	mutex_init(&power_domains->lock);
@@ -2053,7 +2054,7 @@ void intel_power_domains_suspend(struct drm_i915_private *i915,
 	 * resources as required and also enable deeper system power states
 	 * that would be blocked if the firmware was inactive.
 	 */
-	if (!(i915->display.dmc.allowed_dc_mask & DC_STATE_EN_DC9) &&
+	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC9) &&
 	    suspend_mode == I915_DRM_SUSPEND_IDLE &&
 	    intel_dmc_has_payload(i915)) {
 		intel_display_power_flush_work(i915);
@@ -2242,22 +2243,22 @@ void intel_display_power_suspend(struct drm_i915_private *i915)
 
 void intel_display_power_resume(struct drm_i915_private *i915)
 {
+	struct i915_power_domains *power_domains = &i915->display.power.domains;
+
 	if (DISPLAY_VER(i915) >= 11) {
 		bxt_disable_dc9(i915);
 		icl_display_core_init(i915, true);
 		if (intel_dmc_has_payload(i915)) {
-			if (i915->display.dmc.allowed_dc_mask &
-			    DC_STATE_EN_UPTO_DC6)
+			if (power_domains->allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
 				skl_enable_dc6(i915);
-			else if (i915->display.dmc.allowed_dc_mask &
-				 DC_STATE_EN_UPTO_DC5)
+			else if (power_domains->allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
 				gen9_enable_dc5(i915);
 		}
 	} else if (IS_GEMINILAKE(i915) || IS_BROXTON(i915)) {
 		bxt_disable_dc9(i915);
 		bxt_display_core_init(i915, true);
 		if (intel_dmc_has_payload(i915) &&
-		    (i915->display.dmc.allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
+		    (power_domains->allowed_dc_mask & DC_STATE_EN_UPTO_DC5))
 			gen9_enable_dc5(i915);
 	} else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) {
 		hsw_disable_pc8(i915);
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 2154d900b1aa..8e96be8e6330 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -137,6 +137,10 @@ struct i915_power_domains {
 	bool display_core_suspended;
 	int power_well_count;
 
+	u32 dc_state;
+	u32 target_dc_state;
+	u32 allowed_dc_mask;
+
 	intel_wakeref_t init_wakeref;
 	intel_wakeref_t disable_wakeref;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
index 8710dd41ffd4..04498934b41a 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
@@ -700,19 +700,20 @@ static u32 gen9_dc_mask(struct drm_i915_private *dev_priv)
 	return mask;
 }
 
-void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
+void gen9_sanitize_dc_state(struct drm_i915_private *i915)
 {
+	struct i915_power_domains *power_domains = &i915->display.power.domains;
 	u32 val;
 
-	if (!HAS_DISPLAY(dev_priv))
+	if (!HAS_DISPLAY(i915))
 		return;
 
-	val = intel_de_read(dev_priv, DC_STATE_EN) & gen9_dc_mask(dev_priv);
+	val = intel_de_read(i915, DC_STATE_EN) & gen9_dc_mask(i915);
 
-	drm_dbg_kms(&dev_priv->drm,
+	drm_dbg_kms(&i915->drm,
 		    "Resetting DC state tracking from %02x to %02x\n",
-		    dev_priv->display.dmc.dc_state, val);
-	dev_priv->display.dmc.dc_state = val;
+		    power_domains->dc_state, val);
+	power_domains->dc_state = val;
 }
 
 /**
@@ -740,6 +741,7 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
  */
 void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
 {
+	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
 	u32 val;
 	u32 mask;
 
@@ -747,8 +749,8 @@ void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
 		return;
 
 	if (drm_WARN_ON_ONCE(&dev_priv->drm,
-			     state & ~dev_priv->display.dmc.allowed_dc_mask))
-		state &= dev_priv->display.dmc.allowed_dc_mask;
+			     state & ~power_domains->allowed_dc_mask))
+		state &= power_domains->allowed_dc_mask;
 
 	val = intel_de_read(dev_priv, DC_STATE_EN);
 	mask = gen9_dc_mask(dev_priv);
@@ -756,16 +758,16 @@ void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
 		    val & mask, state);
 
 	/* Check if DMC is ignoring our DC state requests */
-	if ((val & mask) != dev_priv->display.dmc.dc_state)
+	if ((val & mask) != power_domains->dc_state)
 		drm_err(&dev_priv->drm, "DC state mismatch (0x%x -> 0x%x)\n",
-			dev_priv->display.dmc.dc_state, val & mask);
+			power_domains->dc_state, val & mask);
 
 	val &= ~mask;
 	val |= state;
 
 	gen9_write_dc_state(dev_priv, val);
 
-	dev_priv->display.dmc.dc_state = val & mask;
+	power_domains->dc_state = val & mask;
 }
 
 static void tgl_enable_dc3co(struct drm_i915_private *dev_priv)
@@ -957,9 +959,10 @@ static void gen9_assert_dbuf_enabled(struct drm_i915_private *dev_priv)
 
 void gen9_disable_dc_states(struct drm_i915_private *dev_priv)
 {
+	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
 	struct intel_cdclk_config cdclk_config = {};
 
-	if (dev_priv->display.dmc.target_dc_state == DC_STATE_EN_DC3CO) {
+	if (power_domains->target_dc_state == DC_STATE_EN_DC3CO) {
 		tgl_disable_dc3co(dev_priv);
 		return;
 	}
@@ -998,10 +1001,12 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
 static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
 					   struct i915_power_well *power_well)
 {
+	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
+
 	if (!intel_dmc_has_payload(dev_priv))
 		return;
 
-	switch (dev_priv->display.dmc.target_dc_state) {
+	switch (power_domains->target_dc_state) {
 	case DC_STATE_EN_DC3CO:
 		tgl_enable_dc3co(dev_priv);
 		break;
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 3b8e8193d042..30a2d0e677d3 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -449,6 +449,7 @@ void intel_dmc_disable_pipe(struct drm_i915_private *i915, enum pipe pipe)
  */
 void intel_dmc_load_program(struct drm_i915_private *dev_priv)
 {
+	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
 	struct intel_dmc *dmc = &dev_priv->display.dmc;
 	enum intel_dmc_id dmc_id;
 	u32 i;
@@ -481,7 +482,7 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	dev_priv->display.dmc.dc_state = 0;
+	power_domains->dc_state = 0;
 
 	gen9_set_dc_state_debugmask(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
index 88eae74dbcf2..da8ba246013e 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.h
+++ b/drivers/gpu/drm/i915/display/intel_dmc.h
@@ -40,9 +40,6 @@ struct intel_dmc {
 		bool present;
 	} dmc_info[DMC_FW_MAX];
 
-	u32 dc_state;
-	u32 target_dc_state;
-	u32 allowed_dc_mask;
 	intel_wakeref_t wakeref;
 };
 
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
index 2954759e9d12..cf13580af34a 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
 {
 	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
 	u32 exit_scanlines;
 
 	/*
@@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
 	if (crtc_state->enable_psr2_sel_fetch)
 		return;
 
-	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
+	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
 		return;
 
 	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
-- 
2.34.1

[Intel-gfx] [RFC 2/3] drm/i915/dmc: drop "ucode" from function names

[Jani Nikula]

The ucode part in the init, fini, suspend and resume function names is
just unnecessary. Drop it.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c |  6 +++---
 drivers/gpu/drm/i915/display/intel_dmc.c     | 20 ++++++++++----------
 drivers/gpu/drm/i915/display/intel_dmc.h     |  8 ++++----
 drivers/gpu/drm/i915/i915_driver.c           |  6 +++---
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 166662ade593..a8c91fda40a8 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -8639,7 +8639,7 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
 	if (!HAS_DISPLAY(i915))
 		return 0;
 
-	intel_dmc_ucode_init(i915);
+	intel_dmc_init(i915);
 
 	i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
 	i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
@@ -8674,7 +8674,7 @@ int intel_modeset_init_noirq(struct drm_i915_private *i915)
 	return 0;
 
 cleanup_vga_client_pw_domain_dmc:
-	intel_dmc_ucode_fini(i915);
+	intel_dmc_fini(i915);
 	intel_power_domains_driver_remove(i915);
 	intel_vga_unregister(i915);
 cleanup_bios:
@@ -9000,7 +9000,7 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915)
 /* part #3: call after gem init */
 void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915)
 {
-	intel_dmc_ucode_fini(i915);
+	intel_dmc_fini(i915);
 
 	intel_power_domains_driver_remove(i915);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index 30a2d0e677d3..ab4fdedd4c5f 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -919,13 +919,13 @@ static void dmc_load_work_fn(struct work_struct *work)
 }
 
 /**
- * intel_dmc_ucode_init() - initialize the firmware loading.
+ * intel_dmc_init() - initialize the firmware loading.
  * @dev_priv: i915 drm device.
  *
  * This function is called at the time of loading the display driver to read
  * firmware from a .bin file and copied into a internal memory.
  */
-void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
+void intel_dmc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_dmc *dmc = &dev_priv->display.dmc;
 
@@ -1003,14 +1003,14 @@ void intel_dmc_ucode_init(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_dmc_ucode_suspend() - prepare DMC firmware before system suspend
+ * intel_dmc_suspend() - prepare DMC firmware before system suspend
  * @dev_priv: i915 drm device
  *
  * Prepare the DMC firmware before entering system suspend. This includes
  * flushing pending work items and releasing any resources acquired during
  * init.
  */
-void intel_dmc_ucode_suspend(struct drm_i915_private *dev_priv)
+void intel_dmc_suspend(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DMC(dev_priv))
 		return;
@@ -1023,13 +1023,13 @@ void intel_dmc_ucode_suspend(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_dmc_ucode_resume() - init DMC firmware during system resume
+ * intel_dmc_resume() - init DMC firmware during system resume
  * @dev_priv: i915 drm device
  *
  * Reinitialize the DMC firmware during system resume, reacquiring any
- * resources released in intel_dmc_ucode_suspend().
+ * resources released in intel_dmc_suspend().
  */
-void intel_dmc_ucode_resume(struct drm_i915_private *dev_priv)
+void intel_dmc_resume(struct drm_i915_private *dev_priv)
 {
 	if (!HAS_DMC(dev_priv))
 		return;
@@ -1043,20 +1043,20 @@ void intel_dmc_ucode_resume(struct drm_i915_private *dev_priv)
 }
 
 /**
- * intel_dmc_ucode_fini() - unload the DMC firmware.
+ * intel_dmc_fini() - unload the DMC firmware.
  * @dev_priv: i915 drm device.
  *
  * Firmmware unloading includes freeing the internal memory and reset the
  * firmware loading status.
  */
-void intel_dmc_ucode_fini(struct drm_i915_private *dev_priv)
+void intel_dmc_fini(struct drm_i915_private *dev_priv)
 {
 	enum intel_dmc_id dmc_id;
 
 	if (!HAS_DMC(dev_priv))
 		return;
 
-	intel_dmc_ucode_suspend(dev_priv);
+	intel_dmc_suspend(dev_priv);
 	drm_WARN_ON(&dev_priv->drm, dev_priv->display.dmc.wakeref);
 
 	for_each_dmc_id(dmc_id)
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
index da8ba246013e..90910cecc2f6 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.h
+++ b/drivers/gpu/drm/i915/display/intel_dmc.h
@@ -43,14 +43,14 @@ struct intel_dmc {
 	intel_wakeref_t wakeref;
 };
 
-void intel_dmc_ucode_init(struct drm_i915_private *i915);
+void intel_dmc_init(struct drm_i915_private *i915);
 void intel_dmc_load_program(struct drm_i915_private *i915);
 void intel_dmc_disable_program(struct drm_i915_private *i915);
 void intel_dmc_enable_pipe(struct drm_i915_private *i915, enum pipe pipe);
 void intel_dmc_disable_pipe(struct drm_i915_private *i915, enum pipe pipe);
-void intel_dmc_ucode_fini(struct drm_i915_private *i915);
-void intel_dmc_ucode_suspend(struct drm_i915_private *i915);
-void intel_dmc_ucode_resume(struct drm_i915_private *i915);
+void intel_dmc_fini(struct drm_i915_private *i915);
+void intel_dmc_suspend(struct drm_i915_private *i915);
+void intel_dmc_resume(struct drm_i915_private *i915);
 bool intel_dmc_has_payload(struct drm_i915_private *i915);
 void intel_dmc_debugfs_register(struct drm_i915_private *i915);
 void intel_dmc_print_error_state(struct drm_i915_error_state_buf *m,
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 13bf4fe52f9f..57305bb00dbc 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1004,7 +1004,7 @@ void i915_driver_shutdown(struct drm_i915_private *i915)
 	intel_suspend_encoders(i915);
 	intel_shutdown_encoders(i915);
 
-	intel_dmc_ucode_suspend(i915);
+	intel_dmc_suspend(i915);
 
 	i915_gem_suspend(i915);
 
@@ -1096,7 +1096,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	dev_priv->suspend_count++;
 
-	intel_dmc_ucode_suspend(dev_priv);
+	intel_dmc_suspend(dev_priv);
 
 	enable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
 
@@ -1217,7 +1217,7 @@ static int i915_drm_resume(struct drm_device *dev)
 	/* Must be called after GGTT is resumed. */
 	intel_dpt_resume(dev_priv);
 
-	intel_dmc_ucode_resume(dev_priv);
+	intel_dmc_resume(dev_priv);
 
 	i915_restore_display(dev_priv);
 	intel_pps_unlock_regs_wa(dev_priv);
-- 
2.34.1

[Intel-gfx] [RFC 3/3] drm/i915/dmc: allocate dmc structure dynamically

[Jani Nikula]

sizeof(struct intel_dmc) > 1024 bytes, allocated on all platforms as
part of struct drm_i915_private, whether they have DMC or not.

Allocate struct intel_dmc dynamically, and hide all the dmc details
behind an opaque pointer in intel_dmc.c.

Care must be taken to take into account all cases: DMC not supported on
the platform, DMC supported but not initialized, and DMC initialized but
not loaded. For the second case, we need to move the wakeref out of
struct intel_dmc.

Cc: Imre Deak <imre.deak@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 .../gpu/drm/i915/display/intel_display_core.h |   8 +-
 drivers/gpu/drm/i915/display/intel_dmc.c      | 136 +++++++++++++-----
 drivers/gpu/drm/i915/display/intel_dmc.h      |  33 +----
 .../drm/i915/display/intel_modeset_setup.c    |   1 +
 4 files changed, 105 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
index fb8670aa2932..e517e06d76a0 100644
--- a/drivers/gpu/drm/i915/display/intel_display_core.h
+++ b/drivers/gpu/drm/i915/display/intel_display_core.h
@@ -19,7 +19,6 @@
 #include "intel_cdclk.h"
 #include "intel_display_limits.h"
 #include "intel_display_power.h"
-#include "intel_dmc.h"
 #include "intel_dpll_mgr.h"
 #include "intel_fbc.h"
 #include "intel_global_state.h"
@@ -40,6 +39,7 @@ struct intel_cdclk_vals;
 struct intel_color_funcs;
 struct intel_crtc;
 struct intel_crtc_state;
+struct intel_dmc;
 struct intel_dpll_funcs;
 struct intel_dpll_mgr;
 struct intel_fbdev;
@@ -339,6 +339,11 @@ struct intel_display {
 		spinlock_t phy_lock;
 	} dkl;
 
+	struct {
+		struct intel_dmc *dmc;
+		intel_wakeref_t wakeref;
+	} dmc;
+
 	struct {
 		/* VLV/CHV/BXT/GLK DSI MMIO register base address */
 		u32 mmio_base;
@@ -466,7 +471,6 @@ struct intel_display {
 
 	/* Grouping using named structs. Keep sorted. */
 	struct intel_audio audio;
-	struct intel_dmc dmc;
 	struct intel_dpll dpll;
 	struct intel_fbc *fbc[I915_MAX_FBCS];
 	struct intel_frontbuffer_tracking fb_tracking;
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
index ab4fdedd4c5f..8428d08e0c3d 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.c
+++ b/drivers/gpu/drm/i915/display/intel_dmc.c
@@ -38,6 +38,39 @@
  * low-power state and comes back to normal.
  */
 
+enum intel_dmc_id {
+	DMC_FW_MAIN = 0,
+	DMC_FW_PIPEA,
+	DMC_FW_PIPEB,
+	DMC_FW_PIPEC,
+	DMC_FW_PIPED,
+	DMC_FW_MAX
+};
+
+struct intel_dmc {
+	struct drm_i915_private *i915;
+	struct work_struct work;
+	const char *fw_path;
+	u32 max_fw_size; /* bytes */
+	u32 version;
+	struct dmc_fw_info {
+		u32 mmio_count;
+		i915_reg_t mmioaddr[20];
+		u32 mmiodata[20];
+		u32 dmc_offset;
+		u32 start_mmioaddr;
+		u32 dmc_fw_size; /*dwords */
+		u32 *payload;
+		bool present;
+	} dmc_info[DMC_FW_MAX];
+};
+
+/* Note: This may be NULL. */
+static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
+{
+	return i915->display.dmc.dmc;
+}
+
 #define DMC_VERSION(major, minor)	((major) << 16 | (minor))
 #define DMC_VERSION_MAJOR(version)	((version) >> 16)
 #define DMC_VERSION_MINOR(version)	((version) & 0xffff)
@@ -259,7 +292,9 @@ static bool is_valid_dmc_id(enum intel_dmc_id dmc_id)
 
 static bool has_dmc_id_fw(struct drm_i915_private *i915, enum intel_dmc_id dmc_id)
 {
-	return i915->display.dmc.dmc_info[dmc_id].payload;
+	struct intel_dmc *dmc = i915_to_dmc(i915);
+
+	return dmc && dmc->dmc_info[dmc_id].payload;
 }
 
 bool intel_dmc_has_payload(struct drm_i915_private *i915)
@@ -450,7 +485,7 @@ void intel_dmc_disable_pipe(struct drm_i915_private *i915, enum pipe pipe)
 void intel_dmc_load_program(struct drm_i915_private *dev_priv)
 {
 	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
-	struct intel_dmc *dmc = &dev_priv->display.dmc;
+	struct intel_dmc *dmc = i915_to_dmc(dev_priv);
 	enum intel_dmc_id dmc_id;
 	u32 i;
 
@@ -515,8 +550,11 @@ void intel_dmc_disable_program(struct drm_i915_private *i915)
 
 void assert_dmc_loaded(struct drm_i915_private *i915)
 {
-	drm_WARN_ONCE(&i915->drm,
-		      !intel_de_read(i915, DMC_PROGRAM(i915->display.dmc.dmc_info[DMC_FW_MAIN].start_mmioaddr, 0)),
+	struct intel_dmc *dmc = i915_to_dmc(i915);
+
+	drm_WARN_ONCE(&i915->drm, !dmc, "DMC not initialized\n");
+	drm_WARN_ONCE(&i915->drm, dmc &&
+		      !intel_de_read(i915, DMC_PROGRAM(dmc->dmc_info[DMC_FW_MAIN].start_mmioaddr, 0)),
 		      "DMC program storage start is NULL\n");
 	drm_WARN_ONCE(&i915->drm, !intel_de_read(i915, DMC_SSP_BASE),
 		      "DMC SSP Base Not fine\n");
@@ -551,11 +589,10 @@ static void dmc_set_fw_offset(struct intel_dmc *dmc,
 			      const struct stepping_info *si,
 			      u8 package_ver)
 {
+	struct drm_i915_private *i915 = dmc->i915;
 	enum intel_dmc_id dmc_id;
 	unsigned int i;
 
-	struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), display.dmc);
-
 	for (i = 0; i < num_entries; i++) {
 		dmc_id = package_ver <= 1 ? DMC_FW_MAIN : fw_info[i].dmc_id;
 
@@ -582,7 +619,7 @@ static bool dmc_mmio_addr_sanity_check(struct intel_dmc *dmc,
 				       const u32 *mmioaddr, u32 mmio_count,
 				       int header_ver, enum intel_dmc_id dmc_id)
 {
-	struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), display.dmc);
+	struct drm_i915_private *i915 = dmc->i915;
 	u32 start_range, end_range;
 	int i;
 
@@ -615,7 +652,7 @@ static u32 parse_dmc_fw_header(struct intel_dmc *dmc,
 			       const struct intel_dmc_header_base *dmc_header,
 			       size_t rem_size, enum intel_dmc_id dmc_id)
 {
-	struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), display.dmc);
+	struct drm_i915_private *i915 = dmc->i915;
 	struct dmc_fw_info *dmc_info = &dmc->dmc_info[dmc_id];
 	unsigned int header_len_bytes, dmc_header_size, payload_size, i;
 	const u32 *mmioaddr, *mmiodata;
@@ -726,7 +763,7 @@ parse_dmc_fw_package(struct intel_dmc *dmc,
 		     const struct stepping_info *si,
 		     size_t rem_size)
 {
-	struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), display.dmc);
+	struct drm_i915_private *i915 = dmc->i915;
 	u32 package_size = sizeof(struct intel_package_header);
 	u32 num_entries, max_entries;
 	const struct intel_fw_info *fw_info;
@@ -780,7 +817,7 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
 			    struct intel_css_header *css_header,
 			    size_t rem_size)
 {
-	struct drm_i915_private *i915 = container_of(dmc, typeof(*i915), display.dmc);
+	struct drm_i915_private *i915 = dmc->i915;
 
 	if (rem_size < sizeof(struct intel_css_header)) {
 		drm_err(&i915->drm, "Truncated DMC firmware, refusing.\n");
@@ -800,13 +837,12 @@ static u32 parse_dmc_fw_css(struct intel_dmc *dmc,
 	return sizeof(struct intel_css_header);
 }
 
-static void parse_dmc_fw(struct drm_i915_private *dev_priv,
-			 const struct firmware *fw)
+static void parse_dmc_fw(struct intel_dmc *dmc, const struct firmware *fw)
 {
+	struct drm_i915_private *dev_priv = dmc->i915;
 	struct intel_css_header *css_header;
 	struct intel_package_header *package_header;
 	struct intel_dmc_header_base *dmc_header;
-	struct intel_dmc *dmc = &dev_priv->display.dmc;
 	struct stepping_info display_info = { '*', '*'};
 	const struct stepping_info *si = intel_get_stepping_info(dev_priv, &display_info);
 	enum intel_dmc_id dmc_id;
@@ -833,7 +869,7 @@ static void parse_dmc_fw(struct drm_i915_private *dev_priv,
 	readcount += r;
 
 	for_each_dmc_id(dmc_id) {
-		if (!dev_priv->display.dmc.dmc_info[dmc_id].present)
+		if (!dmc->dmc_info[dmc_id].present)
 			continue;
 
 		offset = readcount + dmc->dmc_info[dmc_id].dmc_offset * 4;
@@ -872,16 +908,13 @@ static const char *dmc_fallback_path(struct drm_i915_private *i915)
 
 static void dmc_load_work_fn(struct work_struct *work)
 {
-	struct drm_i915_private *dev_priv;
-	struct intel_dmc *dmc;
+	struct intel_dmc *dmc = container_of(work, typeof(*dmc), work);
+	struct drm_i915_private *dev_priv = dmc->i915;
 	const struct firmware *fw = NULL;
 	const char *fallback_path;
 	int err;
 
-	dev_priv = container_of(work, typeof(*dev_priv), display.dmc.work);
-	dmc = &dev_priv->display.dmc;
-
-	err = request_firmware(&fw, dev_priv->display.dmc.fw_path, dev_priv->drm.dev);
+	err = request_firmware(&fw, dmc->fw_path, dev_priv->drm.dev);
 
 	if (err == -ENOENT && !dev_priv->params.dmc_firmware_path) {
 		fallback_path = dmc_fallback_path(dev_priv);
@@ -892,11 +925,11 @@ static void dmc_load_work_fn(struct work_struct *work)
 				    fallback_path);
 			err = request_firmware(&fw, fallback_path, dev_priv->drm.dev);
 			if (err == 0)
-				dev_priv->display.dmc.fw_path = fallback_path;
+				dmc->fw_path = fallback_path;
 		}
 	}
 
-	parse_dmc_fw(dev_priv, fw);
+	parse_dmc_fw(dmc, fw);
 
 	if (intel_dmc_has_payload(dev_priv)) {
 		intel_dmc_load_program(dev_priv);
@@ -904,7 +937,7 @@ static void dmc_load_work_fn(struct work_struct *work)
 
 		drm_info(&dev_priv->drm,
 			 "Finished loading DMC firmware %s (v%u.%u)\n",
-			 dev_priv->display.dmc.fw_path, DMC_VERSION_MAJOR(dmc->version),
+			 dmc->fw_path, DMC_VERSION_MAJOR(dmc->version),
 			 DMC_VERSION_MINOR(dmc->version));
 	} else {
 		drm_notice(&dev_priv->drm,
@@ -927,13 +960,19 @@ static void dmc_load_work_fn(struct work_struct *work)
  */
 void intel_dmc_init(struct drm_i915_private *dev_priv)
 {
-	struct intel_dmc *dmc = &dev_priv->display.dmc;
-
-	INIT_WORK(&dev_priv->display.dmc.work, dmc_load_work_fn);
+	struct intel_dmc *dmc;
 
 	if (!HAS_DMC(dev_priv))
 		return;
 
+	dmc = kzalloc(sizeof(*dmc), GFP_KERNEL);
+	if (!dmc)
+		return;
+
+	dmc->i915 = dev_priv;
+
+	INIT_WORK(&dmc->work, dmc_load_work_fn);
+
 	/*
 	 * Obtain a runtime pm reference, until DMC is loaded, to avoid entering
 	 * runtime-suspend.
@@ -983,10 +1022,9 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
 
 	if (dev_priv->params.dmc_firmware_path) {
 		if (strlen(dev_priv->params.dmc_firmware_path) == 0) {
-			dmc->fw_path = NULL;
 			drm_info(&dev_priv->drm,
 				 "Disabling DMC firmware and runtime PM\n");
-			return;
+			goto out;
 		}
 
 		dmc->fw_path = dev_priv->params.dmc_firmware_path;
@@ -995,11 +1033,18 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
 	if (!dmc->fw_path) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "No known DMC firmware for platform, disabling runtime PM\n");
-		return;
+		goto out;
 	}
 
+	dev_priv->display.dmc.dmc = dmc;
+
 	drm_dbg_kms(&dev_priv->drm, "Loading %s\n", dmc->fw_path);
-	schedule_work(&dev_priv->display.dmc.work);
+	schedule_work(&dmc->work);
+
+	return;
+
+out:
+	kfree(dmc);
 }
 
 /**
@@ -1012,10 +1057,13 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
  */
 void intel_dmc_suspend(struct drm_i915_private *dev_priv)
 {
+	struct intel_dmc *dmc = i915_to_dmc(dev_priv);
+
 	if (!HAS_DMC(dev_priv))
 		return;
 
-	flush_work(&dev_priv->display.dmc.work);
+	if (dmc)
+		flush_work(&dmc->work);
 
 	/* Drop the reference held in case DMC isn't loaded. */
 	if (!intel_dmc_has_payload(dev_priv))
@@ -1051,6 +1099,7 @@ void intel_dmc_resume(struct drm_i915_private *dev_priv)
  */
 void intel_dmc_fini(struct drm_i915_private *dev_priv)
 {
+	struct intel_dmc *dmc = i915_to_dmc(dev_priv);
 	enum intel_dmc_id dmc_id;
 
 	if (!HAS_DMC(dev_priv))
@@ -1059,36 +1108,45 @@ void intel_dmc_fini(struct drm_i915_private *dev_priv)
 	intel_dmc_suspend(dev_priv);
 	drm_WARN_ON(&dev_priv->drm, dev_priv->display.dmc.wakeref);
 
-	for_each_dmc_id(dmc_id)
-		kfree(dev_priv->display.dmc.dmc_info[dmc_id].payload);
+	if (dmc) {
+		for_each_dmc_id(dmc_id)
+			kfree(dmc->dmc_info[dmc_id].payload);
+
+		kfree(dmc);
+		dev_priv->display.dmc.dmc = NULL;
+	}
 }
 
 void intel_dmc_print_error_state(struct drm_i915_error_state_buf *m,
 				 struct drm_i915_private *i915)
 {
-	struct intel_dmc *dmc = &i915->display.dmc;
+	struct intel_dmc *dmc = i915_to_dmc(i915);
 
 	if (!HAS_DMC(i915))
 		return;
 
+	i915_error_printf(m, "DMC initialized: %s\n", str_yes_no(dmc));
 	i915_error_printf(m, "DMC loaded: %s\n",
 			  str_yes_no(intel_dmc_has_payload(i915)));
-	i915_error_printf(m, "DMC fw version: %d.%d\n",
-			  DMC_VERSION_MAJOR(dmc->version),
-			  DMC_VERSION_MINOR(dmc->version));
+	if (dmc)
+		i915_error_printf(m, "DMC fw version: %d.%d\n",
+				  DMC_VERSION_MAJOR(dmc->version),
+				  DMC_VERSION_MINOR(dmc->version));
 }
 
 static int intel_dmc_debugfs_status_show(struct seq_file *m, void *unused)
 {
 	struct drm_i915_private *i915 = m->private;
+	struct intel_dmc *dmc = i915_to_dmc(i915);
 	intel_wakeref_t wakeref;
-	struct intel_dmc *dmc;
 	i915_reg_t dc5_reg, dc6_reg = INVALID_MMIO_REG;
 
 	if (!HAS_DMC(i915))
 		return -ENODEV;
 
-	dmc = &i915->display.dmc;
+	seq_printf(m, "DMC initialized: %s\n", str_yes_no(dmc));
+	if (!dmc)
+		return 0;
 
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
index 90910cecc2f6..fd607afff2ef 100644
--- a/drivers/gpu/drm/i915/display/intel_dmc.h
+++ b/drivers/gpu/drm/i915/display/intel_dmc.h
@@ -6,43 +6,12 @@
 #ifndef __INTEL_DMC_H__
 #define __INTEL_DMC_H__
 
-#include "i915_reg_defs.h"
-#include "intel_wakeref.h"
-#include <linux/workqueue.h>
+#include <linux/types.h>
 
 struct drm_i915_error_state_buf;
 struct drm_i915_private;
-
 enum pipe;
 
-enum intel_dmc_id {
-	DMC_FW_MAIN = 0,
-	DMC_FW_PIPEA,
-	DMC_FW_PIPEB,
-	DMC_FW_PIPEC,
-	DMC_FW_PIPED,
-	DMC_FW_MAX
-};
-
-struct intel_dmc {
-	struct work_struct work;
-	const char *fw_path;
-	u32 max_fw_size; /* bytes */
-	u32 version;
-	struct dmc_fw_info {
-		u32 mmio_count;
-		i915_reg_t mmioaddr[20];
-		u32 mmiodata[20];
-		u32 dmc_offset;
-		u32 start_mmioaddr;
-		u32 dmc_fw_size; /*dwords */
-		u32 *payload;
-		bool present;
-	} dmc_info[DMC_FW_MAX];
-
-	intel_wakeref_t wakeref;
-};
-
 void intel_dmc_init(struct drm_i915_private *i915);
 void intel_dmc_load_program(struct drm_i915_private *i915);
 void intel_dmc_disable_program(struct drm_i915_private *i915);
diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
index 52cdbd4fc2fa..8926fdcc00f5 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
@@ -21,6 +21,7 @@
 #include "intel_display.h"
 #include "intel_display_power.h"
 #include "intel_display_types.h"
+#include "intel_dmc.h"
 #include "intel_modeset_setup.h"
 #include "intel_pch_display.h"
 #include "intel_pm.h"
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/dmc: allocate dmc struct dynamically

[Patchwork]

== Series Details ==

Series: drm/i915/dmc: allocate dmc struct dynamically
URL   : https://patchwork.freedesktop.org/series/113622/
State : warning

== Summary ==

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/dmc: allocate dmc struct dynamically

[Patchwork]

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

== Series Details ==

Series: drm/i915/dmc: allocate dmc struct dynamically
URL   : https://patchwork.freedesktop.org/series/113622/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12686 -> Patchwork_113622v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_113622v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_113622v1, 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_113622v1/index.html

Participating hosts (26 -> 25)
------------------------------

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - bat-dg1-6:          [PASS][1] -> [SKIP][2] +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-dg1-6/igt@i915_pm_rpm@basic-pci-d3-state.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-dg1-6/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@module-reload:
    - bat-dg1-5:          [PASS][3] -> [SKIP][4] +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-dg1-5/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-dg1-5/igt@i915_pm_rpm@module-reload.html

  
#### Suppressed ####

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

  * igt@dmabuf@all-tests@dma_fence:
    - {bat-adln-1}:       [PASS][5] -> [DMESG-FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adln-1/igt@dmabuf@all-tests@dma_fence.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adln-1/igt@dmabuf@all-tests@dma_fence.html

  * igt@dmabuf@all-tests@sanitycheck:
    - {bat-adln-1}:       [PASS][7] -> [ABORT][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adln-1/igt@dmabuf@all-tests@sanitycheck.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adln-1/igt@dmabuf@all-tests@sanitycheck.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - {bat-adlm-1}:       [PASS][9] -> [SKIP][10] +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adlm-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adlm-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-jsl-1}:        [PASS][11] -> [SKIP][12] +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-jsl-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-rpls-1}:       [PASS][13] -> [SKIP][14] +2 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-rpls-1/igt@i915_pm_rpm@basic-pci-d3-state.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-rpls-1/igt@i915_pm_rpm@basic-pci-d3-state.html
    - {bat-adlp-9}:       [PASS][15] -> [SKIP][16] +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adlp-9/igt@i915_pm_rpm@basic-pci-d3-state.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adlp-9/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@basic-rte:
    - {bat-rplp-1}:       [PASS][17] -> [SKIP][18] +2 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-rplp-1/igt@i915_pm_rpm@basic-rte.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-rplp-1/igt@i915_pm_rpm@basic-rte.html
    - {bat-jsl-3}:        [PASS][19] -> [SKIP][20] +2 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-jsl-3/igt@i915_pm_rpm@basic-rte.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-jsl-3/igt@i915_pm_rpm@basic-rte.html
    - {bat-rpls-2}:       [PASS][21] -> [SKIP][22] +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-rpls-2/igt@i915_pm_rpm@basic-rte.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-rpls-2/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rpm@module-reload:
    - {bat-adlp-6}:       [PASS][23] -> [SKIP][24] +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adlp-6/igt@i915_pm_rpm@module-reload.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adlp-6/igt@i915_pm_rpm@module-reload.html
    - {bat-dg1-7}:        [PASS][25] -> [SKIP][26] +2 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-dg1-7/igt@i915_pm_rpm@module-reload.html
    - {bat-adln-1}:       [PASS][27] -> [SKIP][28] +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/bat-adln-1/igt@i915_pm_rpm@module-reload.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/bat-adln-1/igt@i915_pm_rpm@module-reload.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-kbl-guc:         [PASS][29] -> [SKIP][30] ([fdo#109271]) +2 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-kbl-guc/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-skl-6600u:       [PASS][31] -> [SKIP][32] ([fdo#109271]) +2 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-skl-6600u/igt@i915_pm_rpm@basic-pci-d3-state.html
    - fi-glk-j4005:       [PASS][33] -> [SKIP][34] ([fdo#109271]) +2 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-glk-j4005/igt@i915_pm_rpm@basic-pci-d3-state.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-glk-j4005/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@i915_pm_rpm@basic-rte:
    - fi-cfl-8109u:       [PASS][35] -> [SKIP][36] ([fdo#109271]) +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-cfl-8109u/igt@i915_pm_rpm@basic-rte.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-cfl-8109u/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rpm@module-reload:
    - fi-apl-guc:         [PASS][37] -> [SKIP][38] ([fdo#109271]) +2 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-apl-guc/igt@i915_pm_rpm@module-reload.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-apl-guc/igt@i915_pm_rpm@module-reload.html
    - fi-kbl-7567u:       [PASS][39] -> [SKIP][40] ([fdo#109271]) +2 similar issues
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12686/fi-kbl-7567u/igt@i915_pm_rpm@module-reload.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113622v1/fi-kbl-7567u/igt@i915_pm_rpm@module-reload.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#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311
  [i915#6794]: https://gitlab.freedesktop.org/drm/intel/issues/6794
  [i915#7359]: https://gitlab.freedesktop.org/drm/intel/issues/7359
  [i915#7714]: https://gitlab.freedesktop.org/drm/intel/issues/7714


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

  * Linux: CI_DRM_12686 -> Patchwork_113622v1

  CI-20190529: 20190529
  CI_DRM_12686: 0b50be56bb4863e926efa2d89754d654fad828b9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7144: cda71bf809b981a646270963d6b1ccee4fd4643b @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113622v1: 0b50be56bb4863e926efa2d89754d654fad828b9 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

5b50147904e5 drm/i915/dmc: allocate dmc structure dynamically
276afb42bb4e drm/i915/dmc: drop "ucode" from function names
a3261fbce807 drm/i915/power: move dc state members to struct i915_power_domains

== Logs ==

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

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

Re: [Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Imre Deak]

On Thu, Feb 02, 2023 at 10:47:46PM +0200, Jani Nikula wrote:
> There's only one reference to the struct intel_dmc members dc_state,
> target_dc_state, and allowed_dc_mask within intel_dmc.c, begging the
> question why they are under struct intel_dmc to begin with.
> 
> Moreover, the only references to i915->display.dmc outside of
> intel_dmc.c are to these members.
> 
> They don't belong. Move them from struct intel_dmc to struct
> i915_power_domains, which seems like a more suitable place.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> [...]
>
> @@ -481,7 +482,7 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
>  		}
>  	}
>  
> -	dev_priv->display.dmc.dc_state = 0;
> +	power_domains->dc_state = 0;

This could be dropped as well, as it's already inited by the time the
function is called.

I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
state is a functionality provided by the firmware (except of DC9), an
alternative would be to move/add get/set/assert etc. DC state functions
to intel_dmc.c instead and call these from intel_display_power*.c.

>  
>  	gen9_set_dc_state_debugmask(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> index 88eae74dbcf2..da8ba246013e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> @@ -40,9 +40,6 @@ struct intel_dmc {
>  		bool present;
>  	} dmc_info[DMC_FW_MAX];
>  
> -	u32 dc_state;
> -	u32 target_dc_state;
> -	u32 allowed_dc_mask;
>  	intel_wakeref_t wakeref;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 2954759e9d12..cf13580af34a 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>  {
>  	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
>  	u32 exit_scanlines;
>  
>  	/*
> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>  	if (crtc_state->enable_psr2_sel_fetch)
>  		return;
>  
> -	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
> +	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
>  		return;
>  
>  	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
> -- 
> 2.34.1
> 

Re: [Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Jani Nikula]

On Fri, 03 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, Feb 02, 2023 at 10:47:46PM +0200, Jani Nikula wrote:
>> There's only one reference to the struct intel_dmc members dc_state,
>> target_dc_state, and allowed_dc_mask within intel_dmc.c, begging the
>> question why they are under struct intel_dmc to begin with.
>> 
>> Moreover, the only references to i915->display.dmc outside of
>> intel_dmc.c are to these members.
>> 
>> They don't belong. Move them from struct intel_dmc to struct
>> i915_power_domains, which seems like a more suitable place.
>> 
>> Cc: Imre Deak <imre.deak@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>> [...]
>>
>> @@ -481,7 +482,7 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
>>  		}
>>  	}
>>  
>> -	dev_priv->display.dmc.dc_state = 0;
>> +	power_domains->dc_state = 0;
>
> This could be dropped as well, as it's already inited by the time the
> function is called.

The whole dc_state thing originates to 832dba889e27 ("drm/i915/gen9:
Check for DC state mismatch"), and it's all about detecting
mismatches. I'm not sure how that works and if it's still useful.

> I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
> state is a functionality provided by the firmware (except of DC9), an
> alternative would be to move/add get/set/assert etc. DC state functions
> to intel_dmc.c instead and call these from intel_display_power*.c.

That was actually the first patch I wrote but didn't send. There were a
few reasons I switched to this one:

First, it adds a dependency between power well and dmc initialization.

Second, it's a bunch of boilerplate, six get/set functions altogether,
and all of them checking for dmc init in patch 3.

Third, it still seems funny to have these members stored in intel_dmc,
accessed via intel_dmc.c, but intel_dmc.c not actually using them for
anything internally. It's only the power well code that uses
them. Should more of the DC state handling be moved to intel_dmc.c then?


BR,
Jani.


>
>>  
>>  	gen9_set_dc_state_debugmask(dev_priv);
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
>> index 88eae74dbcf2..da8ba246013e 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
>> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
>> @@ -40,9 +40,6 @@ struct intel_dmc {
>>  		bool present;
>>  	} dmc_info[DMC_FW_MAX];
>>  
>> -	u32 dc_state;
>> -	u32 target_dc_state;
>> -	u32 allowed_dc_mask;
>>  	intel_wakeref_t wakeref;
>>  };
>>  
>> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> index 2954759e9d12..cf13580af34a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>>  {
>>  	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
>>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> +	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
>>  	u32 exit_scanlines;
>>  
>>  	/*
>> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>>  	if (crtc_state->enable_psr2_sel_fetch)
>>  		return;
>>  
>> -	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
>> +	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
>>  		return;
>>  
>>  	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
>> -- 
>> 2.34.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Imre Deak]

On Mon, Feb 06, 2023 at 06:21:11PM +0200, Jani Nikula wrote:
> On Fri, 03 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, Feb 02, 2023 at 10:47:46PM +0200, Jani Nikula wrote:
> >> There's only one reference to the struct intel_dmc members dc_state,
> >> target_dc_state, and allowed_dc_mask within intel_dmc.c, begging the
> >> question why they are under struct intel_dmc to begin with.
> >> 
> >> Moreover, the only references to i915->display.dmc outside of
> >> intel_dmc.c are to these members.
> >> 
> >> They don't belong. Move them from struct intel_dmc to struct
> >> i915_power_domains, which seems like a more suitable place.
> >> 
> >> Cc: Imre Deak <imre.deak@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >> [...]
> >>
> >> @@ -481,7 +482,7 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
> >>  		}
> >>  	}
> >>  
> >> -	dev_priv->display.dmc.dc_state = 0;
> >> +	power_domains->dc_state = 0;
> >
> > This could be dropped as well, as it's already inited by the time the
> > function is called.
> 
> The whole dc_state thing originates to 832dba889e27 ("drm/i915/gen9:
> Check for DC state mismatch"), and it's all about detecting
> mismatches. I'm not sure how that works and if it's still useful.

On SKL writing the DC_STATE_EN register didn't take effect, so a
workaround was added to reread/verify the write. Yes, on newer platforms
we should probably revisit this and try to remove at least the part
rereading the register 5 times.

> > I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
> > state is a functionality provided by the firmware (except of DC9), an
> > alternative would be to move/add get/set/assert etc. DC state functions
> > to intel_dmc.c instead and call these from intel_display_power*.c.
> 
> That was actually the first patch I wrote but didn't send. There were a
> few reasons I switched to this one:
> 
> First, it adds a dependency between power well and dmc initialization.

Do you mean the dependency that is there already?: taking some power ref
and keeping the whole runtime PM functionality disabled until the
firmware load completes. This is based on an earlier decision not to
support runtime PM w/o DMC.

> Second, it's a bunch of boilerplate, six get/set functions altogether,
> and all of them checking for dmc init in patch 3.
> 
> Third, it still seems funny to have these members stored in intel_dmc,
> accessed via intel_dmc.c, but intel_dmc.c not actually using them for
> anything internally. It's only the power well code that uses
> them. Should more of the DC state handling be moved to intel_dmc.c then?

I thought that any functions accessing the DC_STATE_EN register would be
moved as well (besides the state checkers you refer to). I wasn't sure
if my suggestion makes sense without actually seeing the end result; I
think we can also regard the DC_STATE_EN register as a more general
display PM HW interface leaving that in intel_display_power* (since DC9
which isn't a DMC thing is also toggled via it) and leave only the
firmware loading/initialization in intel_dmc.c as in your RFC.

> BR,
> Jani.
> 
> >>  	gen9_set_dc_state_debugmask(dev_priv);
> >>  
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> index 88eae74dbcf2..da8ba246013e 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> @@ -40,9 +40,6 @@ struct intel_dmc {
> >>  		bool present;
> >>  	} dmc_info[DMC_FW_MAX];
> >>  
> >> -	u32 dc_state;
> >> -	u32 target_dc_state;
> >> -	u32 allowed_dc_mask;
> >>  	intel_wakeref_t wakeref;
> >>  };
> >>  
> >> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> >> index 2954759e9d12..cf13580af34a 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> >> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >>  {
> >>  	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
> >>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> +	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
> >>  	u32 exit_scanlines;
> >>  
> >>  	/*
> >> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >>  	if (crtc_state->enable_psr2_sel_fetch)
> >>  		return;
> >>  
> >> -	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
> >> +	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
> >>  		return;
> >>  
> >>  	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
> >> -- 
> >> 2.34.1
> >> 
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Jani Nikula]

On Mon, 06 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
> On Mon, Feb 06, 2023 at 06:21:11PM +0200, Jani Nikula wrote:
>> On Fri, 03 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
>> > On Thu, Feb 02, 2023 at 10:47:46PM +0200, Jani Nikula wrote:
>> >> There's only one reference to the struct intel_dmc members dc_state,
>> >> target_dc_state, and allowed_dc_mask within intel_dmc.c, begging the
>> >> question why they are under struct intel_dmc to begin with.
>> >> 
>> >> Moreover, the only references to i915->display.dmc outside of
>> >> intel_dmc.c are to these members.
>> >> 
>> >> They don't belong. Move them from struct intel_dmc to struct
>> >> i915_power_domains, which seems like a more suitable place.
>> >> 
>> >> Cc: Imre Deak <imre.deak@intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >> [...]
>> >>
>> >> @@ -481,7 +482,7 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
>> >>  		}
>> >>  	}
>> >>  
>> >> -	dev_priv->display.dmc.dc_state = 0;
>> >> +	power_domains->dc_state = 0;
>> >
>> > This could be dropped as well, as it's already inited by the time the
>> > function is called.
>> 
>> The whole dc_state thing originates to 832dba889e27 ("drm/i915/gen9:
>> Check for DC state mismatch"), and it's all about detecting
>> mismatches. I'm not sure how that works and if it's still useful.
>
> On SKL writing the DC_STATE_EN register didn't take effect, so a
> workaround was added to reread/verify the write. Yes, on newer platforms
> we should probably revisit this and try to remove at least the part
> rereading the register 5 times.
>
>> > I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
>> > state is a functionality provided by the firmware (except of DC9), an
>> > alternative would be to move/add get/set/assert etc. DC state functions
>> > to intel_dmc.c instead and call these from intel_display_power*.c.
>> 
>> That was actually the first patch I wrote but didn't send. There were a
>> few reasons I switched to this one:
>> 
>> First, it adds a dependency between power well and dmc initialization.
>
> Do you mean the dependency that is there already?: taking some power ref
> and keeping the whole runtime PM functionality disabled until the
> firmware load completes. This is based on an earlier decision not to
> support runtime PM w/o DMC.

intel_power_domains_init() is called very early. It adjusts
allowed_dc_mask and target_dc_state.

intel_dmc_ucode_init() is called later, and depends on
intel_power_domains_init() in the way you describe above.

If intel_dmc_ucode_init() starts allocating intel_dmc dynamically, it
needs to exist before intel_power_domains_init() modifies
allowed_dc_mask and target_dc_state.

It's an interdependency that would need to be broken up somehow.

>
>> Second, it's a bunch of boilerplate, six get/set functions altogether,
>> and all of them checking for dmc init in patch 3.
>> 
>> Third, it still seems funny to have these members stored in intel_dmc,
>> accessed via intel_dmc.c, but intel_dmc.c not actually using them for
>> anything internally. It's only the power well code that uses
>> them. Should more of the DC state handling be moved to intel_dmc.c then?
>
> I thought that any functions accessing the DC_STATE_EN register would be
> moved as well (besides the state checkers you refer to). I wasn't sure
> if my suggestion makes sense without actually seeing the end result; I
> think we can also regard the DC_STATE_EN register as a more general
> display PM HW interface leaving that in intel_display_power* (since DC9
> which isn't a DMC thing is also toggled via it) and leave only the
> firmware loading/initialization in intel_dmc.c as in your RFC.

Yeah, I don't know. *lots of shrugging* :)

>
>> BR,
>> Jani.
>> 
>> >>  	gen9_set_dc_state_debugmask(dev_priv);
>> >>  
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> index 88eae74dbcf2..da8ba246013e 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> @@ -40,9 +40,6 @@ struct intel_dmc {
>> >>  		bool present;
>> >>  	} dmc_info[DMC_FW_MAX];
>> >>  
>> >> -	u32 dc_state;
>> >> -	u32 target_dc_state;
>> >> -	u32 allowed_dc_mask;
>> >>  	intel_wakeref_t wakeref;
>> >>  };
>> >>  
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
>> >> index 2954759e9d12..cf13580af34a 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> >> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>> >>  {
>> >>  	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
>> >>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >> +	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
>> >>  	u32 exit_scanlines;
>> >>  
>> >>  	/*
>> >> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
>> >>  	if (crtc_state->enable_psr2_sel_fetch)
>> >>  		return;
>> >>  
>> >> -	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
>> >> +	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
>> >>  		return;
>> >>  
>> >>  	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
>> >> -- 
>> >> 2.34.1
>> >> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RFC 1/3] drm/i915/power: move dc state members to struct i915_power_domains

[Imre Deak]

On Tue, Feb 07, 2023 at 12:05:43PM +0200, Jani Nikula wrote:
> [...]
> >> > I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
> >> > state is a functionality provided by the firmware (except of DC9), an
> >> > alternative would be to move/add get/set/assert etc. DC state functions
> >> > to intel_dmc.c instead and call these from intel_display_power*.c.
> >> 
> >> That was actually the first patch I wrote but didn't send. There were a
> >> few reasons I switched to this one:
> >> 
> >> First, it adds a dependency between power well and dmc initialization.
> >
> > Do you mean the dependency that is there already?: taking some power ref
> > and keeping the whole runtime PM functionality disabled until the
> > firmware load completes. This is based on an earlier decision not to
> > support runtime PM w/o DMC.
> 
> intel_power_domains_init() is called very early. It adjusts
> allowed_dc_mask and target_dc_state.
> 
> intel_dmc_ucode_init() is called later, and depends on
> intel_power_domains_init() in the way you describe above.
> 
> If intel_dmc_ucode_init() starts allocating intel_dmc dynamically, it
> needs to exist before intel_power_domains_init() modifies
> allowed_dc_mask and target_dc_state.

Ok, I missed all the above.

> It's an interdependency that would need to be broken up somehow.
> 
> >> Second, it's a bunch of boilerplate, six get/set functions altogether,
> >> and all of them checking for dmc init in patch 3.
> >> 
> >> Third, it still seems funny to have these members stored in intel_dmc,
> >> accessed via intel_dmc.c, but intel_dmc.c not actually using them for
> >> anything internally. It's only the power well code that uses
> >> them. Should more of the DC state handling be moved to intel_dmc.c then?
> >
> > I thought that any functions accessing the DC_STATE_EN register would be
> > moved as well (besides the state checkers you refer to). I wasn't sure
> > if my suggestion makes sense without actually seeing the end result; I
> > think we can also regard the DC_STATE_EN register as a more general
> > display PM HW interface leaving that in intel_display_power* (since DC9
> > which isn't a DMC thing is also toggled via it) and leave only the
> > firmware loading/initialization in intel_dmc.c as in your RFC.
> 
> Yeah, I don't know. *lots of shrugging* :)

In the end I think it was wrong on my side to regard the DC_STATE_EN HW
i/f as a DMC internal thing. Rather it is for a display wide runtime PM
control. So yes, moving the DC state fields to intel_display_power.c
makes sense, as well as making the DMC FW internals local to
intel_dmc.c.

Could you follow up with the actual patches?

> >> BR,
> >> Jani.
> >> 
> >> >>  	gen9_set_dc_state_debugmask(dev_priv);
> >> >>  
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> index 88eae74dbcf2..da8ba246013e 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> @@ -40,9 +40,6 @@ struct intel_dmc {
> >> >>  		bool present;
> >> >>  	} dmc_info[DMC_FW_MAX];
> >> >>  
> >> >> -	u32 dc_state;
> >> >> -	u32 target_dc_state;
> >> >> -	u32 allowed_dc_mask;
> >> >>  	intel_wakeref_t wakeref;
> >> >>  };
> >> >>  
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> index 2954759e9d12..cf13580af34a 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >> >>  {
> >> >>  	const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
> >> >>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> >> +	struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
> >> >>  	u32 exit_scanlines;
> >> >>  
> >> >>  	/*
> >> >> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >> >>  	if (crtc_state->enable_psr2_sel_fetch)
> >> >>  		return;
> >> >>  
> >> >> -	if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
> >> >> +	if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
> >> >>  		return;
> >> >>  
> >> >>  	if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
> >> >> -- 
> >> >> 2.34.1
> >> >> 
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center