All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
@ 2019-02-22 23:23 Lucas De Marchi
  2019-02-22 23:23 ` [PATCH 2/3] drm/i915: introduce platform_flags to use with dpll hooks Lucas De Marchi
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-22 23:23 UTC (permalink / raw)
  To: intel-gfx

Let the MG plls have their own hooks since it shares very little with
other PLL types. It's also better so the platform info contains the info
if the PLL is for MG PHY rather than relying on the PLL ids.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
 1 file changed, 74 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 0a42d11c4c33..e4ec73d415d9 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
 	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
 }
 
+static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
+				struct intel_shared_dpll *pll,
+				struct intel_dpll_hw_state *hw_state)
+{
+	const enum intel_dpll_id id = pll->info->id;
+	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
+	intel_wakeref_t wakeref;
+	bool ret = false;
+	u32 val;
+
+	wakeref = intel_display_power_get_if_enabled(dev_priv,
+						     POWER_DOMAIN_PLLS);
+	if (!wakeref)
+		return false;
+
+	val = I915_READ(MG_PLL_ENABLE(tc_port));
+	if (!(val & PLL_ENABLE))
+		goto out;
+
+	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
+	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
+
+	hw_state->mg_clktop2_coreclkctl1 =
+		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
+	hw_state->mg_clktop2_coreclkctl1 &=
+		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
+
+	hw_state->mg_clktop2_hsclkctl =
+		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
+	hw_state->mg_clktop2_hsclkctl &=
+		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
+		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
+		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
+		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
+
+	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
+	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
+	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
+	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
+	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
+
+	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
+	hw_state->mg_pll_tdc_coldst_bias =
+		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
+
+	if (dev_priv->cdclk.hw.ref == 38400) {
+		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
+		hw_state->mg_pll_bias_mask = 0;
+	} else {
+		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
+		hw_state->mg_pll_bias_mask = -1U;
+	}
+
+	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
+	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
+
+	ret = true;
+out:
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
+	return ret;
+}
+
 static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 				 struct intel_shared_dpll *pll,
 				 struct intel_dpll_hw_state *hw_state)
@@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	if (!(val & PLL_ENABLE))
 		goto out;
 
-	if (intel_dpll_is_combophy(id) ||
-	    id == DPLL_ID_ICL_TBTPLL) {
-		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
-		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
-	} else {
-		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
-
-		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
-		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
-
-		hw_state->mg_clktop2_coreclkctl1 =
-			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
-		hw_state->mg_clktop2_coreclkctl1 &=
-			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
-
-		hw_state->mg_clktop2_hsclkctl =
-			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
-		hw_state->mg_clktop2_hsclkctl &=
-			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
-			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
-			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
-			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
-
-		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
-		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
-		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
-		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
-		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
-
-		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
-		hw_state->mg_pll_tdc_coldst_bias =
-			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
-
-		if (dev_priv->cdclk.hw.ref == 38400) {
-			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
-			hw_state->mg_pll_bias_mask = 0;
-		} else {
-			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
-			hw_state->mg_pll_bias_mask = -1U;
-		}
-
-		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
-		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
-	}
+	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
+	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
 
 	ret = true;
 out:
@@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
 	.get_hw_state = icl_pll_get_hw_state,
 };
 
+static const struct intel_shared_dpll_funcs mg_pll_funcs = {
+	.enable = icl_pll_enable,
+	.disable = icl_pll_disable,
+	.get_hw_state = mg_pll_get_hw_state,
+};
+
 static const struct dpll_info icl_plls[] = {
 	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
 	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
 	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
-	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
-	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
-	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
-	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
+	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
+	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
+	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
+	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
 	{ },
 };
 
-- 
2.20.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 2/3] drm/i915: introduce platform_flags to use with dpll hooks
  2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
@ 2019-02-22 23:23 ` Lucas De Marchi
  2019-02-22 23:23 ` [PATCH 3/3] drm/i915/icl: decouple dpll ids from type Lucas De Marchi
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-22 23:23 UTC (permalink / raw)
  To: intel-gfx

Define platform flags taking the high 16 bits of the under-utilized
flags in dpll_info. This makes clear separation on what flags are used
by the core and what flags can be freely defined for use in the context
of the platforms sharing a set of hooks.

The initial motivation for this is the Ice Lake case that heavily relies
on the PLL id to make decisions on what type of PLL that is. This
hinders code sharing across platforms as the IDs may change easily.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 40e8391a92f2..b1fdce1be942 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -279,7 +279,14 @@ struct dpll_info {
 	 *     Inform the state checker that the DPLL is kept enabled even if
 	 *     not in use by any CRTC.
 	 */
-	u32 flags;
+	u16 flags;
+
+	/**
+	 * @platform_flags: like @flags, but these make are meant to use
+	 * within a single platform scope only (or just a few, when the hooks
+	 * are shared)
+	 */
+	u16 platform_flags;
 };
 
 /**
-- 
2.20.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
  2019-02-22 23:23 ` [PATCH 2/3] drm/i915: introduce platform_flags to use with dpll hooks Lucas De Marchi
@ 2019-02-22 23:23 ` Lucas De Marchi
  2019-02-25 20:45   ` Ville Syrjälä
  2019-02-23  0:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: move MG pll hw_state readout Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-22 23:23 UTC (permalink / raw)
  To: intel-gfx

Use the first 3 bits of dpll_info.platform_flags to mark the type of the
PLL instead of relying on the IDs. This is more future proof for
allowing the same set of functions to be reused, even if the IDs change.

The warning about PLL id not corresponding to a combo phy in intel_display
was removed as I don't think it should ever trigger.

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  3 --
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  1 -
 3 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1d63c32ca94..a2be35118dd5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
 		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
 		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
 		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
-
-		if (WARN_ON(!intel_dpll_is_combophy(id)))
-			return;
 	} else if (intel_port_is_tc(dev_priv, port)) {
 		id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index e4ec73d415d9..cdb4463bab5d 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
 	return link_clock;
 }
 
+enum icl_dpll_flags {
+	ICL_DPLL_TYPE_COMBOPHY,
+	ICL_DPLL_TYPE_TBT,
+	ICL_DPLL_TYPE_MG,
+	_ICL_DPLL_TYPE_MASK = 0x7,
+};
+
 static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
 {
 	return id - DPLL_ID_ICL_MGPLL1;
@@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
 	return tc_port + DPLL_ID_ICL_MGPLL1;
 }
 
-bool intel_dpll_is_combophy(enum intel_dpll_id id)
+static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
 {
-	return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
+	return info->platform_flags & _ICL_DPLL_TYPE_MASK;
 }
 
 static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
@@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
-static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
+static i915_reg_t
+icl_pll_info_to_enable_reg(const struct dpll_info *info)
 {
-	if (intel_dpll_is_combophy(id))
-		return CNL_DPLL_ENABLE(id);
-	else if (id == DPLL_ID_ICL_TBTPLL)
-		return TBT_PLL_ENABLE;
+	enum icl_dpll_flags type = icl_dpll_type(info);
 
-	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
+	switch (type) {
+	case ICL_DPLL_TYPE_COMBOPHY:
+		return CNL_DPLL_ENABLE(info->id);
+	case ICL_DPLL_TYPE_TBT:
+		return TBT_PLL_ENABLE;
+	case ICL_DPLL_TYPE_MG:
+		return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
+	default:
+		MISSING_CASE(type);
+		return CNL_DPLL_ENABLE(info->id);
+	}
 }
 
 static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
@@ -3042,7 +3057,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
 	if (!wakeref)
 		return false;
 
-	val = I915_READ(icl_pll_id_to_enable_reg(id));
+	val = I915_READ(icl_pll_info_to_enable_reg(pll->info));
 	if (!(val & PLL_ENABLE))
 		goto out;
 
@@ -3120,7 +3135,8 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
 			   struct intel_shared_dpll *pll)
 {
 	const enum intel_dpll_id id = pll->info->id;
-	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
+	enum icl_dpll_flags type = icl_dpll_type(pll->info);
+	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
 	u32 val;
 
 	val = I915_READ(enable_reg);
@@ -3135,7 +3151,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
 				    PLL_POWER_STATE, 1))
 		DRM_ERROR("PLL %d Power not enabled\n", id);
 
-	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
+	if (type == ICL_DPLL_TYPE_COMBOPHY || type == ICL_DPLL_TYPE_TBT)
 		icl_dpll_write(dev_priv, pll);
 	else
 		icl_mg_pll_write(dev_priv, pll);
@@ -3161,7 +3177,7 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
 			    struct intel_shared_dpll *pll)
 {
 	const enum intel_dpll_id id = pll->info->id;
-	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
+	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
 	u32 val;
 
 	/* The first steps are done by intel_ddi_post_disable(). */
@@ -3230,13 +3246,13 @@ static const struct intel_shared_dpll_funcs mg_pll_funcs = {
 };
 
 static const struct dpll_info icl_plls[] = {
-	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
-	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
-	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
-	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
-	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
-	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
-	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
+	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0, ICL_DPLL_TYPE_COMBOPHY },
+	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0, ICL_DPLL_TYPE_COMBOPHY },
+	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0, ICL_DPLL_TYPE_TBT },
+	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0, ICL_DPLL_TYPE_MG },
+	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0, ICL_DPLL_TYPE_MG },
+	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0, ICL_DPLL_TYPE_MG },
+	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0, ICL_DPLL_TYPE_MG },
 	{ },
 };
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index b1fdce1be942..12ffe83598aa 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -352,6 +352,5 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
 			       u32 pll_id);
 int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
 enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
-bool intel_dpll_is_combophy(enum intel_dpll_id id);
 
 #endif /* _INTEL_DPLL_MGR_H_ */
-- 
2.20.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
  2019-02-22 23:23 ` [PATCH 2/3] drm/i915: introduce platform_flags to use with dpll hooks Lucas De Marchi
  2019-02-22 23:23 ` [PATCH 3/3] drm/i915/icl: decouple dpll ids from type Lucas De Marchi
@ 2019-02-23  0:09 ` Patchwork
  2019-02-23  6:03 ` ✓ Fi.CI.IGT: " Patchwork
  2019-02-25 20:42 ` [PATCH 1/3] " Ville Syrjälä
  4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2019-02-23  0:09 UTC (permalink / raw)
  To: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: move MG pll hw_state readout
URL   : https://patchwork.freedesktop.org/series/57116/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5655 -> Patchwork_12289
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/57116/revisions/1/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       PASS -> SKIP [fdo#109271] +33

  
#### Possible fixes ####

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       FAIL [fdo#109485] -> PASS

  
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (45 -> 35)
------------------------------

  Missing    (10): fi-kbl-soraka fi-kbl-7567u fi-ilk-m540 fi-byt-squawks fi-icl-u2 fi-bsw-cyan fi-kbl-guc fi-gdg-551 fi-ivb-3770 fi-bdw-samus 


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

    * Linux: CI_DRM_5655 -> Patchwork_12289

  CI_DRM_5655: a40729237602fa7454aaf3355ad3058cad5c6ee9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4853: 8afdfd8fa9ce17043d9105dedca46ad4555fdcdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12289: 733d58810b2834d707806c3298348907ac6562ca @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

733d58810b28 drm/i915/icl: decouple dpll ids from type
0f1e84d98acc drm/i915: introduce platform_flags to use with dpll hooks
76aed92c6a4a drm/i915/icl: move MG pll hw_state readout

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12289/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
                   ` (2 preceding siblings ...)
  2019-02-23  0:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: move MG pll hw_state readout Patchwork
@ 2019-02-23  6:03 ` Patchwork
  2019-02-25 20:42 ` [PATCH 1/3] " Ville Syrjälä
  4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2019-02-23  6:03 UTC (permalink / raw)
  To: intel-gfx

== Series Details ==

Series: series starting with [1/3] drm/i915/icl: move MG pll hw_state readout
URL   : https://patchwork.freedesktop.org/series/57116/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5655_full -> Patchwork_12289_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_busy@extended-parallel-bsd1:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +7

  * igt@gem_ctx_param@invalid-param-set:
    - shard-kbl:          NOTRUN -> FAIL [fdo#109674]

  * igt@kms_busy@basic-flip-f:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@basic-modeset-e:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-kbl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_cursor_crc@cursor-64x21-sliding:
    - shard-apl:          PASS -> FAIL [fdo#103232] +3

  * igt@kms_cursor_crc@cursor-64x64-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-apl:          PASS -> FAIL [fdo#109350]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - shard-apl:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-glk:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +41

  * igt@kms_plane@pixel-format-pipe-c-planes:
    - shard-apl:          PASS -> FAIL [fdo#103166] +1

  * igt@kms_plane_alpha_blend@pipe-a-alpha-transparant-fb:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-glk:          PASS -> DMESG-FAIL [fdo#105763] / [fdo#106538]

  * igt@kms_setmode@basic:
    - shard-apl:          PASS -> FAIL [fdo#99912]

  * igt@testdisplay:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  
#### Possible fixes ####

  * igt@i915_pm_rc6_residency@rc6-accuracy:
    - shard-snb:          SKIP [fdo#109271] -> PASS

  * igt@kms_color@pipe-c-legacy-gamma:
    - shard-glk:          FAIL [fdo#104782] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-glk:          FAIL [fdo#109350] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-none:
    - shard-apl:          FAIL [fdo#103166] -> PASS

  
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#106538]: https://bugs.freedesktop.org/show_bug.cgi?id=106538
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
  [fdo#109674]: https://bugs.freedesktop.org/show_bug.cgi?id=109674
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (6 -> 5)
------------------------------

  Missing    (1): shard-skl 


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

    * Linux: CI_DRM_5655 -> Patchwork_12289

  CI_DRM_5655: a40729237602fa7454aaf3355ad3058cad5c6ee9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4853: 8afdfd8fa9ce17043d9105dedca46ad4555fdcdb @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12289: 733d58810b2834d707806c3298348907ac6562ca @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12289/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
                   ` (3 preceding siblings ...)
  2019-02-23  6:03 ` ✓ Fi.CI.IGT: " Patchwork
@ 2019-02-25 20:42 ` Ville Syrjälä
  2019-02-26  0:03   ` Lucas De Marchi
  4 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-25 20:42 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
> Let the MG plls have their own hooks since it shares very little with
> other PLL types. It's also better so the platform info contains the info
> if the PLL is for MG PHY rather than relying on the PLL ids.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

There's quite a bit more cleanup to be done in this area. As a start
https://patchwork.freedesktop.org/series/56354/ ;)

This patch looks good to me. It'll conflict with my series though, but
no biggie.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
>  1 file changed, 74 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 0a42d11c4c33..e4ec73d415d9 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>  	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>  }
>  
> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> +				struct intel_shared_dpll *pll,
> +				struct intel_dpll_hw_state *hw_state)
> +{
> +	const enum intel_dpll_id id = pll->info->id;
> +	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> +	intel_wakeref_t wakeref;
> +	bool ret = false;
> +	u32 val;
> +
> +	wakeref = intel_display_power_get_if_enabled(dev_priv,
> +						     POWER_DOMAIN_PLLS);
> +	if (!wakeref)
> +		return false;
> +
> +	val = I915_READ(MG_PLL_ENABLE(tc_port));
> +	if (!(val & PLL_ENABLE))
> +		goto out;
> +
> +	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> +	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> +
> +	hw_state->mg_clktop2_coreclkctl1 =
> +		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> +	hw_state->mg_clktop2_coreclkctl1 &=
> +		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> +
> +	hw_state->mg_clktop2_hsclkctl =
> +		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> +	hw_state->mg_clktop2_hsclkctl &=
> +		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> +		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> +
> +	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> +	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> +	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> +	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> +	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> +
> +	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> +	hw_state->mg_pll_tdc_coldst_bias =
> +		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> +
> +	if (dev_priv->cdclk.hw.ref == 38400) {
> +		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> +		hw_state->mg_pll_bias_mask = 0;
> +	} else {
> +		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> +		hw_state->mg_pll_bias_mask = -1U;
> +	}
> +
> +	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> +	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> +
> +	ret = true;
> +out:
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
> +	return ret;
> +}
> +
>  static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  				 struct intel_shared_dpll *pll,
>  				 struct intel_dpll_hw_state *hw_state)
> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	if (!(val & PLL_ENABLE))
>  		goto out;
>  
> -	if (intel_dpll_is_combophy(id) ||
> -	    id == DPLL_ID_ICL_TBTPLL) {
> -		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> -		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
> -	} else {
> -		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> -
> -		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> -		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> -
> -		hw_state->mg_clktop2_coreclkctl1 =
> -			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> -		hw_state->mg_clktop2_coreclkctl1 &=
> -			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> -
> -		hw_state->mg_clktop2_hsclkctl =
> -			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> -		hw_state->mg_clktop2_hsclkctl &=
> -			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> -			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> -			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> -			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> -
> -		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> -		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> -		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> -		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> -		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> -
> -		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> -		hw_state->mg_pll_tdc_coldst_bias =
> -			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> -
> -		if (dev_priv->cdclk.hw.ref == 38400) {
> -			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> -			hw_state->mg_pll_bias_mask = 0;
> -		} else {
> -			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> -			hw_state->mg_pll_bias_mask = -1U;
> -		}
> -
> -		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> -		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> -	}
> +	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> +	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>  
>  	ret = true;
>  out:
> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>  	.get_hw_state = icl_pll_get_hw_state,
>  };
>  
> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
> +	.enable = icl_pll_enable,
> +	.disable = icl_pll_disable,
> +	.get_hw_state = mg_pll_get_hw_state,
> +};
> +
>  static const struct dpll_info icl_plls[] = {
>  	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
>  	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
>  	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
> -	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> -	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> -	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> -	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>  	{ },
>  };
>  
> -- 
> 2.20.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-22 23:23 ` [PATCH 3/3] drm/i915/icl: decouple dpll ids from type Lucas De Marchi
@ 2019-02-25 20:45   ` Ville Syrjälä
  2019-02-25 21:28     ` Lucas De Marchi
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-25 20:45 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Fri, Feb 22, 2019 at 03:23:24PM -0800, Lucas De Marchi wrote:
> Use the first 3 bits of dpll_info.platform_flags to mark the type of the
> PLL instead of relying on the IDs. This is more future proof for
> allowing the same set of functions to be reused, even if the IDs change.
> 
> The warning about PLL id not corresponding to a combo phy in intel_display
> was removed as I don't think it should ever trigger.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  3 --
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  1 -
>  3 files changed, 35 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1d63c32ca94..a2be35118dd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
>  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> -
> -		if (WARN_ON(!intel_dpll_is_combophy(id)))
> -			return;
>  	} else if (intel_port_is_tc(dev_priv, port)) {
>  		id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
>  	} else {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index e4ec73d415d9..cdb4463bab5d 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  	return link_clock;
>  }
>  
> +enum icl_dpll_flags {
> +	ICL_DPLL_TYPE_COMBOPHY,
> +	ICL_DPLL_TYPE_TBT,
> +	ICL_DPLL_TYPE_MG,
> +	_ICL_DPLL_TYPE_MASK = 0x7,
> +};
> +
>  static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>  {
>  	return id - DPLL_ID_ICL_MGPLL1;
> @@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
>  	return tc_port + DPLL_ID_ICL_MGPLL1;
>  }
>  
> -bool intel_dpll_is_combophy(enum intel_dpll_id id)
> +static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
>  {
> -	return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
> +	return info->platform_flags & _ICL_DPLL_TYPE_MASK;
>  }
>  
>  static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
> @@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> +static i915_reg_t
> +icl_pll_info_to_enable_reg(const struct dpll_info *info)
>  {
> -	if (intel_dpll_is_combophy(id))
> -		return CNL_DPLL_ENABLE(id);
> -	else if (id == DPLL_ID_ICL_TBTPLL)
> -		return TBT_PLL_ENABLE;
> +	enum icl_dpll_flags type = icl_dpll_type(info);
>  
> -	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
> +	switch (type) {
> +	case ICL_DPLL_TYPE_COMBOPHY:
> +		return CNL_DPLL_ENABLE(info->id);
> +	case ICL_DPLL_TYPE_TBT:
> +		return TBT_PLL_ENABLE;
> +	case ICL_DPLL_TYPE_MG:
> +		return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
> +	default:
> +		MISSING_CASE(type);
> +		return CNL_DPLL_ENABLE(info->id);
> +	}
>  }

This seems a rather roundabout way of doing things when we already have
the vfuncs.

How about just:

mg_pll_enable()
{
	icl_pll_enable(MG_REG);
}

combo_pll_enable()
{
	icl_pll_enable(COMBO_REG);
}

or something along those lines.


>  
>  static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -3042,7 +3057,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>  	if (!wakeref)
>  		return false;
>  
> -	val = I915_READ(icl_pll_id_to_enable_reg(id));
> +	val = I915_READ(icl_pll_info_to_enable_reg(pll->info));
>  	if (!(val & PLL_ENABLE))
>  		goto out;
>  
> @@ -3120,7 +3135,8 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  			   struct intel_shared_dpll *pll)
>  {
>  	const enum intel_dpll_id id = pll->info->id;
> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> +	enum icl_dpll_flags type = icl_dpll_type(pll->info);
> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>  	u32 val;
>  
>  	val = I915_READ(enable_reg);
> @@ -3135,7 +3151,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>  				    PLL_POWER_STATE, 1))
>  		DRM_ERROR("PLL %d Power not enabled\n", id);
>  
> -	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
> +	if (type == ICL_DPLL_TYPE_COMBOPHY || type == ICL_DPLL_TYPE_TBT)
>  		icl_dpll_write(dev_priv, pll);
>  	else
>  		icl_mg_pll_write(dev_priv, pll);
> @@ -3161,7 +3177,7 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
>  			    struct intel_shared_dpll *pll)
>  {
>  	const enum intel_dpll_id id = pll->info->id;
> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>  	u32 val;
>  
>  	/* The first steps are done by intel_ddi_post_disable(). */
> @@ -3230,13 +3246,13 @@ static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>  };
>  
>  static const struct dpll_info icl_plls[] = {
> -	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
> -	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
> -	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
> -	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> -	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> -	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> -	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> +	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0, ICL_DPLL_TYPE_COMBOPHY },
> +	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0, ICL_DPLL_TYPE_COMBOPHY },
> +	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0, ICL_DPLL_TYPE_TBT },
> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0, ICL_DPLL_TYPE_MG },
> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0, ICL_DPLL_TYPE_MG },
>  	{ },
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index b1fdce1be942..12ffe83598aa 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -352,6 +352,5 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>  			       u32 pll_id);
>  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
>  enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
> -bool intel_dpll_is_combophy(enum intel_dpll_id id);
>  
>  #endif /* _INTEL_DPLL_MGR_H_ */
> -- 
> 2.20.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-25 20:45   ` Ville Syrjälä
@ 2019-02-25 21:28     ` Lucas De Marchi
  2019-02-26 14:48       ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-25 21:28 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Mon, Feb 25, 2019 at 10:45:34PM +0200, Ville Syrjälä wrote:
>On Fri, Feb 22, 2019 at 03:23:24PM -0800, Lucas De Marchi wrote:
>> Use the first 3 bits of dpll_info.platform_flags to mark the type of the
>> PLL instead of relying on the IDs. This is more future proof for
>> allowing the same set of functions to be reused, even if the IDs change.
>>
>> The warning about PLL id not corresponding to a combo phy in intel_display
>> was removed as I don't think it should ever trigger.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c  |  3 --
>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  1 -
>>  3 files changed, 35 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b1d63c32ca94..a2be35118dd5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
>>  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
>>  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
>>  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
>> -
>> -		if (WARN_ON(!intel_dpll_is_combophy(id)))
>> -			return;
>>  	} else if (intel_port_is_tc(dev_priv, port)) {
>>  		id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
>>  	} else {
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index e4ec73d415d9..cdb4463bab5d 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>>  	return link_clock;
>>  }
>>
>> +enum icl_dpll_flags {
>> +	ICL_DPLL_TYPE_COMBOPHY,
>> +	ICL_DPLL_TYPE_TBT,
>> +	ICL_DPLL_TYPE_MG,
>> +	_ICL_DPLL_TYPE_MASK = 0x7,
>> +};
>> +
>>  static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
>>  {
>>  	return id - DPLL_ID_ICL_MGPLL1;
>> @@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
>>  	return tc_port + DPLL_ID_ICL_MGPLL1;
>>  }
>>
>> -bool intel_dpll_is_combophy(enum intel_dpll_id id)
>> +static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
>>  {
>> -	return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
>> +	return info->platform_flags & _ICL_DPLL_TYPE_MASK;
>>  }
>>
>>  static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
>> @@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>>  	return pll;
>>  }
>>
>> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>> +static i915_reg_t
>> +icl_pll_info_to_enable_reg(const struct dpll_info *info)
>>  {
>> -	if (intel_dpll_is_combophy(id))
>> -		return CNL_DPLL_ENABLE(id);
>> -	else if (id == DPLL_ID_ICL_TBTPLL)
>> -		return TBT_PLL_ENABLE;
>> +	enum icl_dpll_flags type = icl_dpll_type(info);
>>
>> -	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>> +	switch (type) {
>> +	case ICL_DPLL_TYPE_COMBOPHY:
>> +		return CNL_DPLL_ENABLE(info->id);
>> +	case ICL_DPLL_TYPE_TBT:
>> +		return TBT_PLL_ENABLE;
>> +	case ICL_DPLL_TYPE_MG:
>> +		return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
>> +	default:
>> +		MISSING_CASE(type);
>> +		return CNL_DPLL_ENABLE(info->id);
>> +	}
>>  }
>
>This seems a rather roundabout way of doing things when we already have
>the vfuncs.
>
>How about just:
>
>mg_pll_enable()
>{
>	icl_pll_enable(MG_REG);
>}
>
>combo_pll_enable()
>{
>	icl_pll_enable(COMBO_REG);
>}
>
>or something along those lines.

humn... I thought about this approach as an intermediate step to the
full blown "add another vfunc struct and pass that instead".  Platforms
are free to use this for small differences that don't justify going that
route.

In your approach you go the route of "always use vfunc and add
additional arguments to the common function".
Compared to the approach here, it's not subjective on what to do in
similar cases, but has its downsides as well.

1) The function can't be inlined so there's and extra hop when calling
the vfunc
2) if the callee is inlined we basically duplicate .text, but I doubt
any compiler would do that
3) reg as the argument is probably not a good choice as it may change
in the next platform - so we would need to add a "type" nonetheless

Since flags is already there and under-utilized I don't see much
advantage on the vfunc-only approach. I'm not strongly opposed though:
IMO both are better than the current state.

Lucas De Marchi

>
>
>>
>>  static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> @@ -3042,7 +3057,7 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>  	if (!wakeref)
>>  		return false;
>>
>> -	val = I915_READ(icl_pll_id_to_enable_reg(id));
>> +	val = I915_READ(icl_pll_info_to_enable_reg(pll->info));
>>  	if (!(val & PLL_ENABLE))
>>  		goto out;
>>
>> @@ -3120,7 +3135,8 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>>  			   struct intel_shared_dpll *pll)
>>  {
>>  	const enum intel_dpll_id id = pll->info->id;
>> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> +	enum icl_dpll_flags type = icl_dpll_type(pll->info);
>> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>>  	u32 val;
>>
>>  	val = I915_READ(enable_reg);
>> @@ -3135,7 +3151,7 @@ static void icl_pll_enable(struct drm_i915_private *dev_priv,
>>  				    PLL_POWER_STATE, 1))
>>  		DRM_ERROR("PLL %d Power not enabled\n", id);
>>
>> -	if (intel_dpll_is_combophy(id) || id == DPLL_ID_ICL_TBTPLL)
>> +	if (type == ICL_DPLL_TYPE_COMBOPHY || type == ICL_DPLL_TYPE_TBT)
>>  		icl_dpll_write(dev_priv, pll);
>>  	else
>>  		icl_mg_pll_write(dev_priv, pll);
>> @@ -3161,7 +3177,7 @@ static void icl_pll_disable(struct drm_i915_private *dev_priv,
>>  			    struct intel_shared_dpll *pll)
>>  {
>>  	const enum intel_dpll_id id = pll->info->id;
>> -	i915_reg_t enable_reg = icl_pll_id_to_enable_reg(id);
>> +	i915_reg_t enable_reg = icl_pll_info_to_enable_reg(pll->info);
>>  	u32 val;
>>
>>  	/* The first steps are done by intel_ddi_post_disable(). */
>> @@ -3230,13 +3246,13 @@ static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>>  };
>>
>>  static const struct dpll_info icl_plls[] = {
>> -	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
>> -	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
>> -	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
>> -	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> -	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> -	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> -	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> +	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0, 0, ICL_DPLL_TYPE_COMBOPHY },
>> +	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1, 0, ICL_DPLL_TYPE_COMBOPHY },
>> +	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0, ICL_DPLL_TYPE_TBT },
>> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0, ICL_DPLL_TYPE_MG },
>> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0, ICL_DPLL_TYPE_MG },
>> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0, ICL_DPLL_TYPE_MG },
>> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0, ICL_DPLL_TYPE_MG },
>>  	{ },
>>  };
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> index b1fdce1be942..12ffe83598aa 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
>> @@ -352,6 +352,5 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
>>  			       u32 pll_id);
>>  int cnl_hdmi_pll_ref_clock(struct drm_i915_private *dev_priv);
>>  enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port);
>> -bool intel_dpll_is_combophy(enum intel_dpll_id id);
>>
>>  #endif /* _INTEL_DPLL_MGR_H_ */
>> --
>> 2.20.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-25 20:42 ` [PATCH 1/3] " Ville Syrjälä
@ 2019-02-26  0:03   ` Lucas De Marchi
  2019-02-26 14:21     ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-26  0:03 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Mon, Feb 25, 2019 at 10:42:12PM +0200, Ville Syrjälä wrote:
>On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
>> Let the MG plls have their own hooks since it shares very little with
>> other PLL types. It's also better so the platform info contains the info
>> if the PLL is for MG PHY rather than relying on the PLL ids.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>
>There's quite a bit more cleanup to be done in this area. As a start
>https://patchwork.freedesktop.org/series/56354/ ;)

I started reviewing that and even gave my r-b in some patches - first
patches cause several conflicts with in-flight patches though. Not sure
how beneficial they are. IMO last 3 patches could be standalone
(particularly the one that is a bug fix, and doesn't depend on the
previous ones)

>This patch looks good to me. It'll conflict with my series though, but
>no biggie.
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

thanks
Lucas De Marchi

>
>> ---
>>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
>>  1 file changed, 74 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> index 0a42d11c4c33..e4ec73d415d9 100644
>> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>>  	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>>  }
>>
>> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> +				struct intel_shared_dpll *pll,
>> +				struct intel_dpll_hw_state *hw_state)
>> +{
>> +	const enum intel_dpll_id id = pll->info->id;
>> +	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> +	intel_wakeref_t wakeref;
>> +	bool ret = false;
>> +	u32 val;
>> +
>> +	wakeref = intel_display_power_get_if_enabled(dev_priv,
>> +						     POWER_DOMAIN_PLLS);
>> +	if (!wakeref)
>> +		return false;
>> +
>> +	val = I915_READ(MG_PLL_ENABLE(tc_port));
>> +	if (!(val & PLL_ENABLE))
>> +		goto out;
>> +
>> +	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> +	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> +
>> +	hw_state->mg_clktop2_coreclkctl1 =
>> +		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> +	hw_state->mg_clktop2_coreclkctl1 &=
>> +		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> +
>> +	hw_state->mg_clktop2_hsclkctl =
>> +		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> +	hw_state->mg_clktop2_hsclkctl &=
>> +		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> +		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> +
>> +	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> +	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> +	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> +	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> +	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> +
>> +	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> +	hw_state->mg_pll_tdc_coldst_bias =
>> +		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> +
>> +	if (dev_priv->cdclk.hw.ref == 38400) {
>> +		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> +		hw_state->mg_pll_bias_mask = 0;
>> +	} else {
>> +		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> +		hw_state->mg_pll_bias_mask = -1U;
>> +	}
>> +
>> +	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> +	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> +
>> +	ret = true;
>> +out:
>> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
>> +	return ret;
>> +}
>> +
>>  static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>  				 struct intel_shared_dpll *pll,
>>  				 struct intel_dpll_hw_state *hw_state)
>> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>>  	if (!(val & PLL_ENABLE))
>>  		goto out;
>>
>> -	if (intel_dpll_is_combophy(id) ||
>> -	    id == DPLL_ID_ICL_TBTPLL) {
>> -		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> -		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>> -	} else {
>> -		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> -
>> -		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> -		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> -
>> -		hw_state->mg_clktop2_coreclkctl1 =
>> -			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> -		hw_state->mg_clktop2_coreclkctl1 &=
>> -			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> -
>> -		hw_state->mg_clktop2_hsclkctl =
>> -			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> -		hw_state->mg_clktop2_hsclkctl &=
>> -			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> -			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> -			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> -			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> -
>> -		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> -		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> -		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> -		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> -		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> -
>> -		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> -		hw_state->mg_pll_tdc_coldst_bias =
>> -			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> -
>> -		if (dev_priv->cdclk.hw.ref == 38400) {
>> -			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> -			hw_state->mg_pll_bias_mask = 0;
>> -		} else {
>> -			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> -			hw_state->mg_pll_bias_mask = -1U;
>> -		}
>> -
>> -		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> -		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> -	}
>> +	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> +	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>>
>>  	ret = true;
>>  out:
>> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>>  	.get_hw_state = icl_pll_get_hw_state,
>>  };
>>
>> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>> +	.enable = icl_pll_enable,
>> +	.disable = icl_pll_disable,
>> +	.get_hw_state = mg_pll_get_hw_state,
>> +};
>> +
>>  static const struct dpll_info icl_plls[] = {
>>  	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
>>  	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
>>  	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
>> -	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> -	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> -	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> -	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>>  	{ },
>>  };
>>
>> --
>> 2.20.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-26  0:03   ` Lucas De Marchi
@ 2019-02-26 14:21     ` Ville Syrjälä
  2019-02-26 19:15       ` Lucas De Marchi
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-26 14:21 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Mon, Feb 25, 2019 at 04:03:05PM -0800, Lucas De Marchi wrote:
> On Mon, Feb 25, 2019 at 10:42:12PM +0200, Ville Syrjälä wrote:
> >On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
> >> Let the MG plls have their own hooks since it shares very little with
> >> other PLL types. It's also better so the platform info contains the info
> >> if the PLL is for MG PHY rather than relying on the PLL ids.
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >
> >There's quite a bit more cleanup to be done in this area. As a start
> >https://patchwork.freedesktop.org/series/56354/ ;)
> 
> I started reviewing that and even gave my r-b in some patches - first
> patches cause several conflicts with in-flight patches though. Not sure
> how beneficial they are.

The current code is quite messy, and thus hard to follow.
It should be cleaned up before we pile even more stuff on top.
People had already cargo culted all the crud from the older
implementations to the later ones.

We can't let the dirt accumulate indefinitely because eventually
the point comes where the thing either collapses under its own weight
or someone just decides to not even look at the previous code and 
does the new thing totally differently. And that approach does not
scale because then everyone has to keep in mind N different ways
of doing the exact same thing. And also all the learnings from the 
old code are forgotten and we probably get more bugs as a result.

> IMO last 3 patches could be standalone
> (particularly the one that is a bug fix, and doesn't depend on the
> previous ones)

Nothing prevents the patches from being reviewed/merged out of order.

> 
> >This patch looks good to me. It'll conflict with my series though, but
> >no biggie.
> >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> thanks
> Lucas De Marchi
> 
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
> >>  1 file changed, 74 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index 0a42d11c4c33..e4ec73d415d9 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> >>  	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
> >>  }
> >>
> >> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >> +				struct intel_shared_dpll *pll,
> >> +				struct intel_dpll_hw_state *hw_state)
> >> +{
> >> +	const enum intel_dpll_id id = pll->info->id;
> >> +	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> >> +	intel_wakeref_t wakeref;
> >> +	bool ret = false;
> >> +	u32 val;
> >> +
> >> +	wakeref = intel_display_power_get_if_enabled(dev_priv,
> >> +						     POWER_DOMAIN_PLLS);
> >> +	if (!wakeref)
> >> +		return false;
> >> +
> >> +	val = I915_READ(MG_PLL_ENABLE(tc_port));
> >> +	if (!(val & PLL_ENABLE))
> >> +		goto out;
> >> +
> >> +	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> >> +	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> >> +
> >> +	hw_state->mg_clktop2_coreclkctl1 =
> >> +		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> >> +	hw_state->mg_clktop2_coreclkctl1 &=
> >> +		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> >> +
> >> +	hw_state->mg_clktop2_hsclkctl =
> >> +		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> >> +	hw_state->mg_clktop2_hsclkctl &=
> >> +		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> >> +		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> >> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> >> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> >> +
> >> +	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> >> +	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> >> +	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> >> +	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> >> +	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> >> +
> >> +	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> >> +	hw_state->mg_pll_tdc_coldst_bias =
> >> +		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> >> +
> >> +	if (dev_priv->cdclk.hw.ref == 38400) {
> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> >> +		hw_state->mg_pll_bias_mask = 0;
> >> +	} else {
> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> >> +		hw_state->mg_pll_bias_mask = -1U;
> >> +	}
> >> +
> >> +	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> >> +	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> >> +
> >> +	ret = true;
> >> +out:
> >> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
> >> +	return ret;
> >> +}
> >> +
> >>  static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >>  				 struct intel_shared_dpll *pll,
> >>  				 struct intel_dpll_hw_state *hw_state)
> >> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >>  	if (!(val & PLL_ENABLE))
> >>  		goto out;
> >>
> >> -	if (intel_dpll_is_combophy(id) ||
> >> -	    id == DPLL_ID_ICL_TBTPLL) {
> >> -		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> >> -		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
> >> -	} else {
> >> -		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> >> -
> >> -		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> >> -		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> >> -
> >> -		hw_state->mg_clktop2_coreclkctl1 =
> >> -			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> >> -		hw_state->mg_clktop2_coreclkctl1 &=
> >> -			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> >> -
> >> -		hw_state->mg_clktop2_hsclkctl =
> >> -			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> >> -		hw_state->mg_clktop2_hsclkctl &=
> >> -			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> >> -			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> >> -			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> >> -			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> >> -
> >> -		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> >> -		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> >> -		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> >> -		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> >> -		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> >> -
> >> -		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> >> -		hw_state->mg_pll_tdc_coldst_bias =
> >> -			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> >> -
> >> -		if (dev_priv->cdclk.hw.ref == 38400) {
> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> >> -			hw_state->mg_pll_bias_mask = 0;
> >> -		} else {
> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> >> -			hw_state->mg_pll_bias_mask = -1U;
> >> -		}
> >> -
> >> -		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> >> -		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> >> -	}
> >> +	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> >> +	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
> >>
> >>  	ret = true;
> >>  out:
> >> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
> >>  	.get_hw_state = icl_pll_get_hw_state,
> >>  };
> >>
> >> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
> >> +	.enable = icl_pll_enable,
> >> +	.disable = icl_pll_disable,
> >> +	.get_hw_state = mg_pll_get_hw_state,
> >> +};
> >> +
> >>  static const struct dpll_info icl_plls[] = {
> >>  	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
> >>  	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
> >>  	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
> >> -	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> >> -	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> >> -	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> >> -	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> >> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> >> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> >> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> >> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> >>  	{ },
> >>  };
> >>
> >> --
> >> 2.20.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >-- 
> >Ville Syrjälä
> >Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-25 21:28     ` Lucas De Marchi
@ 2019-02-26 14:48       ` Ville Syrjälä
  2019-02-26 19:02         ` Lucas De Marchi
  0 siblings, 1 reply; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-26 14:48 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Mon, Feb 25, 2019 at 01:28:23PM -0800, Lucas De Marchi wrote:
> On Mon, Feb 25, 2019 at 10:45:34PM +0200, Ville Syrjälä wrote:
> >On Fri, Feb 22, 2019 at 03:23:24PM -0800, Lucas De Marchi wrote:
> >> Use the first 3 bits of dpll_info.platform_flags to mark the type of the
> >> PLL instead of relying on the IDs. This is more future proof for
> >> allowing the same set of functions to be reused, even if the IDs change.
> >>
> >> The warning about PLL id not corresponding to a combo phy in intel_display
> >> was removed as I don't think it should ever trigger.
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c  |  3 --
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 54 +++++++++++++++++----------
> >>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  1 -
> >>  3 files changed, 35 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index b1d63c32ca94..a2be35118dd5 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -9592,9 +9592,6 @@ static void icelake_get_ddi_pll(struct drm_i915_private *dev_priv,
> >>  		temp = I915_READ(DPCLKA_CFGCR0_ICL) &
> >>  		       DPCLKA_CFGCR0_DDI_CLK_SEL_MASK(port);
> >>  		id = temp >> DPCLKA_CFGCR0_DDI_CLK_SEL_SHIFT(port);
> >> -
> >> -		if (WARN_ON(!intel_dpll_is_combophy(id)))
> >> -			return;
> >>  	} else if (intel_port_is_tc(dev_priv, port)) {
> >>  		id = icl_tc_port_to_pll_id(intel_port_to_tc(dev_priv, port));
> >>  	} else {
> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> index e4ec73d415d9..cdb4463bab5d 100644
> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> @@ -2639,6 +2639,13 @@ int icl_calc_dp_combo_pll_link(struct drm_i915_private *dev_priv,
> >>  	return link_clock;
> >>  }
> >>
> >> +enum icl_dpll_flags {
> >> +	ICL_DPLL_TYPE_COMBOPHY,
> >> +	ICL_DPLL_TYPE_TBT,
> >> +	ICL_DPLL_TYPE_MG,
> >> +	_ICL_DPLL_TYPE_MASK = 0x7,
> >> +};
> >> +
> >>  static enum tc_port icl_pll_id_to_tc_port(enum intel_dpll_id id)
> >>  {
> >>  	return id - DPLL_ID_ICL_MGPLL1;
> >> @@ -2649,9 +2656,9 @@ enum intel_dpll_id icl_tc_port_to_pll_id(enum tc_port tc_port)
> >>  	return tc_port + DPLL_ID_ICL_MGPLL1;
> >>  }
> >>
> >> -bool intel_dpll_is_combophy(enum intel_dpll_id id)
> >> +static enum icl_dpll_flags icl_dpll_type(const struct dpll_info *info)
> >>  {
> >> -	return id == DPLL_ID_ICL_DPLL0 || id == DPLL_ID_ICL_DPLL1;
> >> +	return info->platform_flags & _ICL_DPLL_TYPE_MASK;
> >>  }
> >>
> >>  static bool icl_mg_pll_find_divisors(int clock_khz, bool is_dp, bool use_ssc,
> >> @@ -2956,14 +2963,22 @@ icl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
> >>  	return pll;
> >>  }
> >>
> >> -static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> >> +static i915_reg_t
> >> +icl_pll_info_to_enable_reg(const struct dpll_info *info)
> >>  {
> >> -	if (intel_dpll_is_combophy(id))
> >> -		return CNL_DPLL_ENABLE(id);
> >> -	else if (id == DPLL_ID_ICL_TBTPLL)
> >> -		return TBT_PLL_ENABLE;
> >> +	enum icl_dpll_flags type = icl_dpll_type(info);
> >>
> >> -	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
> >> +	switch (type) {
> >> +	case ICL_DPLL_TYPE_COMBOPHY:
> >> +		return CNL_DPLL_ENABLE(info->id);
> >> +	case ICL_DPLL_TYPE_TBT:
> >> +		return TBT_PLL_ENABLE;
> >> +	case ICL_DPLL_TYPE_MG:
> >> +		return MG_PLL_ENABLE(icl_pll_id_to_tc_port(info->id));
> >> +	default:
> >> +		MISSING_CASE(type);
> >> +		return CNL_DPLL_ENABLE(info->id);
> >> +	}
> >>  }
> >
> >This seems a rather roundabout way of doing things when we already have
> >the vfuncs.
> >
> >How about just:
> >
> >mg_pll_enable()
> >{
> >	icl_pll_enable(MG_REG);
> >}
> >
> >combo_pll_enable()
> >{
> >	icl_pll_enable(COMBO_REG);
> >}
> >
> >or something along those lines.
> 
> humn... I thought about this approach as an intermediate step to the
> full blown "add another vfunc struct and pass that instead".  Platforms
> are free to use this for small differences that don't justify going that
> route.
> 
> In your approach you go the route of "always use vfunc and add
> additional arguments to the common function".
> Compared to the approach here, it's not subjective on what to do in
> similar cases, but has its downsides as well.
> 
> 1) The function can't be inlined so there's and extra hop when calling
> the vfunc

We already have the vfunc. And even if we didn't, an extra vfunc in
modesetting code is probably in the noise.

> 2) if the callee is inlined we basically duplicate .text, but I doubt
> any compiler would do that

Either it inlines or not. Why should we care in this particular case?

> 3) reg as the argument is probably not a good choice as it may change
> in the next platform - so we would need to add a "type" nonetheless

Not sure what you mean. If the reg changes you pass in a different reg.
If other things differ significantly you write a new function.

> 
> Since flags is already there
> and under-utilized I don't see much
> advantage on the vfunc-only approach. I'm not strongly opposed though:
> IMO both are better than the current state.

If the existing mechanism is sufficient to the task then we should
generally use it rather than adding yet another mechanism. This
keeps the code more uniform and thus easier for everyone to
understand.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-26 14:48       ` Ville Syrjälä
@ 2019-02-26 19:02         ` Lucas De Marchi
  2019-02-27 18:15           ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-26 19:02 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, Feb 26, 2019 at 04:48:23PM +0200, Ville Syrjälä wrote:
>> >This seems a rather roundabout way of doing things when we already have
>> >the vfuncs.
>> >
>> >How about just:
>> >
>> >mg_pll_enable()
>> >{
>> >	icl_pll_enable(MG_REG);
>> >}
>> >
>> >combo_pll_enable()
>> >{
>> >	icl_pll_enable(COMBO_REG);
>> >}
>> >
>> >or something along those lines.
>>
>> humn... I thought about this approach as an intermediate step to the
>> full blown "add another vfunc struct and pass that instead".  Platforms
>> are free to use this for small differences that don't justify going that
>> route.
>>
>> In your approach you go the route of "always use vfunc and add
>> additional arguments to the common function".
>> Compared to the approach here, it's not subjective on what to do in
>> similar cases, but has its downsides as well.
>>
>> 1) The function can't be inlined so there's and extra hop when calling
>> the vfunc
>
>We already have the vfunc. And even if we didn't, an extra vfunc in
>modesetting code is probably in the noise.

I'm talking about the extra function you added here. The vfunc will call
this, which then calls the real common function.

>> 2) if the callee is inlined we basically duplicate .text, but I doubt
>> any compiler would do that
>
>Either it inlines or not. Why should we care in this particular case?

In this case I'm referring to icl_pll_enable() being inlined inside
mg_pll_enable() and combo_pll_enable().

But let's leave alone the inlines and extra function calls and talk
about the organization below.

>> 3) reg as the argument is probably not a good choice as it may change
>> in the next platform - so we would need to add a "type" nonetheless
>
>Not sure what you mean. If the reg changes you pass in a different reg.
>If other things differ significantly you write a new function.

because here the function can share more when I consider the *type* of
the pll, not if it's reg 0x10, 0x30 or 0x40.

>>
>> Since flags is already there
>> and under-utilized I don't see much
>> advantage on the vfunc-only approach. I'm not strongly opposed though:
>> IMO both are better than the current state.
>
>If the existing mechanism is sufficient to the task then we should
>generally use it rather than adding yet another mechanism. This
>keeps the code more uniform and thus easier for everyone to
>understand.


both of them already exist - flags is already there. If I handle the
*types* differently with your approach I would basically have:

    enum pll_type {
        A,
        B,
        C,
    }

    pll_enable()
    {
        ...

        if (type == A)
        else if (type == B)
        else

        ...
    }

    a_pll_enable()
    {
        return pll_enable(A);
    }

    b_pll_enable()
    {
        return pll_enable(B);
    }

    c_pll_enable()
    {
        return pll_enable(C);
    }

    static const struct funcs a_funcs = {
        .enable = a_pll_enable(),
    };
    static const struct funcs b_funcs = {
        .eanble = b_pll_enable(),
    };
    static const struct funcs c_funcs = {
        .enable = c_pll_enable(),
    };

    static const struct plls[] = {
        { a_funcs, ... },
        { b_funcs, ... },
        { c_funcs, ... },
    };


This approach has its value when the implementations are completely
different - e.g. the mg vs combo approach in patch 1. When the
implementation is very similar, what I'm proposing is to be able to do:

    enum pll_type {
        A,
        B,
        C,
    }

    pll_enable()
    {
        ...

        if (type == A)
        else if (type == B)
        else

        ...
    }

    static const struct funcs funcs = {
        .enable = pll_enable(),
    };
 
    static const struct plls[] = {
        { funcs, A, ... }
        { funcs, B, ... }
        { funcs, C, ... }
    }

We have less boilerplate code and the information is in the table rather
than spread across multiple functions. Don't get me wrong, the other
approach has its place and is even used in patch 1 because the impl
is totally different.

In the ICL case, the type in the table is used to tweak the behavior for
MG vs TBT, because they reuse most of the same calls. Combo vs MG is
handled in patch 1, not here.

Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-26 14:21     ` Ville Syrjälä
@ 2019-02-26 19:15       ` Lucas De Marchi
  2019-02-27 18:13         ` Ville Syrjälä
  0 siblings, 1 reply; 15+ messages in thread
From: Lucas De Marchi @ 2019-02-26 19:15 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On Tue, Feb 26, 2019 at 04:21:01PM +0200, Ville Syrjälä wrote:
>On Mon, Feb 25, 2019 at 04:03:05PM -0800, Lucas De Marchi wrote:
>> On Mon, Feb 25, 2019 at 10:42:12PM +0200, Ville Syrjälä wrote:
>> >On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
>> >> Let the MG plls have their own hooks since it shares very little with
>> >> other PLL types. It's also better so the platform info contains the info
>> >> if the PLL is for MG PHY rather than relying on the PLL ids.
>> >>
>> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> >
>> >There's quite a bit more cleanup to be done in this area. As a start
>> >https://patchwork.freedesktop.org/series/56354/ ;)
>>
>> I started reviewing that and even gave my r-b in some patches - first
>> patches cause several conflicts with in-flight patches though. Not sure
>> how beneficial they are.
>
>The current code is quite messy, and thus hard to follow.
>It should be cleaned up before we pile even more stuff on top.
>People had already cargo culted all the crud from the older
>implementations to the later ones.
>
>We can't let the dirt accumulate indefinitely because eventually
>the point comes where the thing either collapses under its own weight
>or someone just decides to not even look at the previous code and
>does the new thing totally differently. And that approach does not
>scale because then everyone has to keep in mind N different ways
>of doing the exact same thing. And also all the learnings from the
>old code are forgotten and we probably get more bugs as a result.

I agree with all of that. I'm talking about those particular changes and
the timing with the pending work going upstream, not generically.

>> IMO last 3 patches could be standalone
>> (particularly the one that is a bug fix, and doesn't depend on the
>> previous ones)
>
>Nothing prevents the patches from being reviewed/merged out of order.

CI running on them and they taking more time to be merged waiting for
the other patches to be reviewed.

Lucas De Marchi

>
>>
>> >This patch looks good to me. It'll conflict with my series though, but
>> >no biggie.
>> >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> thanks
>> Lucas De Marchi
>>
>> >
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
>> >>  1 file changed, 74 insertions(+), 48 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> index 0a42d11c4c33..e4ec73d415d9 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
>> >> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
>> >>  	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
>> >>  }
>> >>
>> >> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >> +				struct intel_shared_dpll *pll,
>> >> +				struct intel_dpll_hw_state *hw_state)
>> >> +{
>> >> +	const enum intel_dpll_id id = pll->info->id;
>> >> +	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> >> +	intel_wakeref_t wakeref;
>> >> +	bool ret = false;
>> >> +	u32 val;
>> >> +
>> >> +	wakeref = intel_display_power_get_if_enabled(dev_priv,
>> >> +						     POWER_DOMAIN_PLLS);
>> >> +	if (!wakeref)
>> >> +		return false;
>> >> +
>> >> +	val = I915_READ(MG_PLL_ENABLE(tc_port));
>> >> +	if (!(val & PLL_ENABLE))
>> >> +		goto out;
>> >> +
>> >> +	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> >> +	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> >> +
>> >> +	hw_state->mg_clktop2_coreclkctl1 =
>> >> +		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> >> +	hw_state->mg_clktop2_coreclkctl1 &=
>> >> +		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> >> +
>> >> +	hw_state->mg_clktop2_hsclkctl =
>> >> +		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> >> +	hw_state->mg_clktop2_hsclkctl &=
>> >> +		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> >> +		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> >> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> >> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> >> +
>> >> +	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> >> +	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> >> +	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> >> +	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> >> +	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> >> +
>> >> +	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> >> +	hw_state->mg_pll_tdc_coldst_bias =
>> >> +		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> >> +
>> >> +	if (dev_priv->cdclk.hw.ref == 38400) {
>> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> >> +		hw_state->mg_pll_bias_mask = 0;
>> >> +	} else {
>> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> >> +		hw_state->mg_pll_bias_mask = -1U;
>> >> +	}
>> >> +
>> >> +	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> >> +	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> >> +
>> >> +	ret = true;
>> >> +out:
>> >> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
>> >> +	return ret;
>> >> +}
>> >> +
>> >>  static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >>  				 struct intel_shared_dpll *pll,
>> >>  				 struct intel_dpll_hw_state *hw_state)
>> >> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
>> >>  	if (!(val & PLL_ENABLE))
>> >>  		goto out;
>> >>
>> >> -	if (intel_dpll_is_combophy(id) ||
>> >> -	    id == DPLL_ID_ICL_TBTPLL) {
>> >> -		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> >> -		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>> >> -	} else {
>> >> -		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
>> >> -
>> >> -		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
>> >> -		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
>> >> -
>> >> -		hw_state->mg_clktop2_coreclkctl1 =
>> >> -			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
>> >> -		hw_state->mg_clktop2_coreclkctl1 &=
>> >> -			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
>> >> -
>> >> -		hw_state->mg_clktop2_hsclkctl =
>> >> -			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
>> >> -		hw_state->mg_clktop2_hsclkctl &=
>> >> -			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
>> >> -			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
>> >> -			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
>> >> -			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
>> >> -
>> >> -		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
>> >> -		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
>> >> -		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
>> >> -		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
>> >> -		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
>> >> -
>> >> -		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
>> >> -		hw_state->mg_pll_tdc_coldst_bias =
>> >> -			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
>> >> -
>> >> -		if (dev_priv->cdclk.hw.ref == 38400) {
>> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
>> >> -			hw_state->mg_pll_bias_mask = 0;
>> >> -		} else {
>> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
>> >> -			hw_state->mg_pll_bias_mask = -1U;
>> >> -		}
>> >> -
>> >> -		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
>> >> -		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
>> >> -	}
>> >> +	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
>> >> +	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
>> >>
>> >>  	ret = true;
>> >>  out:
>> >> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
>> >>  	.get_hw_state = icl_pll_get_hw_state,
>> >>  };
>> >>
>> >> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
>> >> +	.enable = icl_pll_enable,
>> >> +	.disable = icl_pll_disable,
>> >> +	.get_hw_state = mg_pll_get_hw_state,
>> >> +};
>> >> +
>> >>  static const struct dpll_info icl_plls[] = {
>> >>  	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
>> >>  	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
>> >>  	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
>> >> -	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> >> -	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> >> -	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> >> -	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> >> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
>> >> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
>> >> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
>> >> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
>> >>  	{ },
>> >>  };
>> >>
>> >> --
>> >> 2.20.0
>> >>
>> >> _______________________________________________
>> >> Intel-gfx mailing list
>> >> Intel-gfx@lists.freedesktop.org
>> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> >
>> >--
>> >Ville Syrjälä
>> >Intel
>
>-- 
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout
  2019-02-26 19:15       ` Lucas De Marchi
@ 2019-02-27 18:13         ` Ville Syrjälä
  0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-27 18:13 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Tue, Feb 26, 2019 at 11:15:44AM -0800, Lucas De Marchi wrote:
> On Tue, Feb 26, 2019 at 04:21:01PM +0200, Ville Syrjälä wrote:
> >On Mon, Feb 25, 2019 at 04:03:05PM -0800, Lucas De Marchi wrote:
> >> On Mon, Feb 25, 2019 at 10:42:12PM +0200, Ville Syrjälä wrote:
> >> >On Fri, Feb 22, 2019 at 03:23:22PM -0800, Lucas De Marchi wrote:
> >> >> Let the MG plls have their own hooks since it shares very little with
> >> >> other PLL types. It's also better so the platform info contains the info
> >> >> if the PLL is for MG PHY rather than relying on the PLL ids.
> >> >>
> >> >> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> >> >
> >> >There's quite a bit more cleanup to be done in this area. As a start
> >> >https://patchwork.freedesktop.org/series/56354/ ;)
> >>
> >> I started reviewing that and even gave my r-b in some patches - first
> >> patches cause several conflicts with in-flight patches though. Not sure
> >> how beneficial they are.
> >
> >The current code is quite messy, and thus hard to follow.
> >It should be cleaned up before we pile even more stuff on top.
> >People had already cargo culted all the crud from the older
> >implementations to the later ones.
> >
> >We can't let the dirt accumulate indefinitely because eventually
> >the point comes where the thing either collapses under its own weight
> >or someone just decides to not even look at the previous code and
> >does the new thing totally differently. And that approach does not
> >scale because then everyone has to keep in mind N different ways
> >of doing the exact same thing. And also all the learnings from the
> >old code are forgotten and we probably get more bugs as a result.
> 
> I agree with all of that. I'm talking about those particular changes and
> the timing with the pending work going upstream, not generically.

There is always pending work somewhere. We can't stop for that. Also
if there's more of this stuff about to be submitted upstream we 
should clean it up before it lands.

> 
> >> IMO last 3 patches could be standalone
> >> (particularly the one that is a bug fix, and doesn't depend on the
> >> previous ones)
> >
> >Nothing prevents the patches from being reviewed/merged out of order.
> 
> CI running on them and they taking more time to be merged waiting for
> the other patches to be reviewed.

The first part is already sorted by CI checking the whole thing.

The second part should be easy to fix.

> 
> Lucas De Marchi
> 
> >
> >>
> >> >This patch looks good to me. It'll conflict with my series though, but
> >> >no biggie.
> >> >Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> thanks
> >> Lucas De Marchi
> >>
> >> >
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 122 ++++++++++++++++----------
> >> >>  1 file changed, 74 insertions(+), 48 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> >> index 0a42d11c4c33..e4ec73d415d9 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> >> >> @@ -2966,6 +2966,68 @@ static i915_reg_t icl_pll_id_to_enable_reg(enum intel_dpll_id id)
> >> >>  	return MG_PLL_ENABLE(icl_pll_id_to_tc_port(id));
> >> >>  }
> >> >>
> >> >> +static bool mg_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >> >> +				struct intel_shared_dpll *pll,
> >> >> +				struct intel_dpll_hw_state *hw_state)
> >> >> +{
> >> >> +	const enum intel_dpll_id id = pll->info->id;
> >> >> +	enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> >> >> +	intel_wakeref_t wakeref;
> >> >> +	bool ret = false;
> >> >> +	u32 val;
> >> >> +
> >> >> +	wakeref = intel_display_power_get_if_enabled(dev_priv,
> >> >> +						     POWER_DOMAIN_PLLS);
> >> >> +	if (!wakeref)
> >> >> +		return false;
> >> >> +
> >> >> +	val = I915_READ(MG_PLL_ENABLE(tc_port));
> >> >> +	if (!(val & PLL_ENABLE))
> >> >> +		goto out;
> >> >> +
> >> >> +	hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> >> >> +	hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> >> >> +
> >> >> +	hw_state->mg_clktop2_coreclkctl1 =
> >> >> +		I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> >> >> +	hw_state->mg_clktop2_coreclkctl1 &=
> >> >> +		MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> >> >> +
> >> >> +	hw_state->mg_clktop2_hsclkctl =
> >> >> +		I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> >> >> +	hw_state->mg_clktop2_hsclkctl &=
> >> >> +		MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> >> >> +		MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> >> >> +		MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> >> >> +		MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> >> >> +
> >> >> +	hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> >> >> +	hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> >> >> +	hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> >> >> +	hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> >> >> +	hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> >> >> +
> >> >> +	hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> >> >> +	hw_state->mg_pll_tdc_coldst_bias =
> >> >> +		I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> >> >> +
> >> >> +	if (dev_priv->cdclk.hw.ref == 38400) {
> >> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> >> >> +		hw_state->mg_pll_bias_mask = 0;
> >> >> +	} else {
> >> >> +		hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> >> >> +		hw_state->mg_pll_bias_mask = -1U;
> >> >> +	}
> >> >> +
> >> >> +	hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> >> >> +	hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> >> >> +
> >> >> +	ret = true;
> >> >> +out:
> >> >> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS, wakeref);
> >> >> +	return ret;
> >> >> +}
> >> >> +
> >> >>  static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >> >>  				 struct intel_shared_dpll *pll,
> >> >>  				 struct intel_dpll_hw_state *hw_state)
> >> >> @@ -2984,50 +3046,8 @@ static bool icl_pll_get_hw_state(struct drm_i915_private *dev_priv,
> >> >>  	if (!(val & PLL_ENABLE))
> >> >>  		goto out;
> >> >>
> >> >> -	if (intel_dpll_is_combophy(id) ||
> >> >> -	    id == DPLL_ID_ICL_TBTPLL) {
> >> >> -		hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> >> >> -		hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
> >> >> -	} else {
> >> >> -		enum tc_port tc_port = icl_pll_id_to_tc_port(id);
> >> >> -
> >> >> -		hw_state->mg_refclkin_ctl = I915_READ(MG_REFCLKIN_CTL(tc_port));
> >> >> -		hw_state->mg_refclkin_ctl &= MG_REFCLKIN_CTL_OD_2_MUX_MASK;
> >> >> -
> >> >> -		hw_state->mg_clktop2_coreclkctl1 =
> >> >> -			I915_READ(MG_CLKTOP2_CORECLKCTL1(tc_port));
> >> >> -		hw_state->mg_clktop2_coreclkctl1 &=
> >> >> -			MG_CLKTOP2_CORECLKCTL1_A_DIVRATIO_MASK;
> >> >> -
> >> >> -		hw_state->mg_clktop2_hsclkctl =
> >> >> -			I915_READ(MG_CLKTOP2_HSCLKCTL(tc_port));
> >> >> -		hw_state->mg_clktop2_hsclkctl &=
> >> >> -			MG_CLKTOP2_HSCLKCTL_TLINEDRV_CLKSEL_MASK |
> >> >> -			MG_CLKTOP2_HSCLKCTL_CORE_INPUTSEL_MASK |
> >> >> -			MG_CLKTOP2_HSCLKCTL_HSDIV_RATIO_MASK |
> >> >> -			MG_CLKTOP2_HSCLKCTL_DSDIV_RATIO_MASK;
> >> >> -
> >> >> -		hw_state->mg_pll_div0 = I915_READ(MG_PLL_DIV0(tc_port));
> >> >> -		hw_state->mg_pll_div1 = I915_READ(MG_PLL_DIV1(tc_port));
> >> >> -		hw_state->mg_pll_lf = I915_READ(MG_PLL_LF(tc_port));
> >> >> -		hw_state->mg_pll_frac_lock = I915_READ(MG_PLL_FRAC_LOCK(tc_port));
> >> >> -		hw_state->mg_pll_ssc = I915_READ(MG_PLL_SSC(tc_port));
> >> >> -
> >> >> -		hw_state->mg_pll_bias = I915_READ(MG_PLL_BIAS(tc_port));
> >> >> -		hw_state->mg_pll_tdc_coldst_bias =
> >> >> -			I915_READ(MG_PLL_TDC_COLDST_BIAS(tc_port));
> >> >> -
> >> >> -		if (dev_priv->cdclk.hw.ref == 38400) {
> >> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = MG_PLL_TDC_COLDST_COLDSTART;
> >> >> -			hw_state->mg_pll_bias_mask = 0;
> >> >> -		} else {
> >> >> -			hw_state->mg_pll_tdc_coldst_bias_mask = -1U;
> >> >> -			hw_state->mg_pll_bias_mask = -1U;
> >> >> -		}
> >> >> -
> >> >> -		hw_state->mg_pll_tdc_coldst_bias &= hw_state->mg_pll_tdc_coldst_bias_mask;
> >> >> -		hw_state->mg_pll_bias &= hw_state->mg_pll_bias_mask;
> >> >> -	}
> >> >> +	hw_state->cfgcr0 = I915_READ(ICL_DPLL_CFGCR0(id));
> >> >> +	hw_state->cfgcr1 = I915_READ(ICL_DPLL_CFGCR1(id));
> >> >>
> >> >>  	ret = true;
> >> >>  out:
> >> >> @@ -3203,14 +3223,20 @@ static const struct intel_shared_dpll_funcs icl_pll_funcs = {
> >> >>  	.get_hw_state = icl_pll_get_hw_state,
> >> >>  };
> >> >>
> >> >> +static const struct intel_shared_dpll_funcs mg_pll_funcs = {
> >> >> +	.enable = icl_pll_enable,
> >> >> +	.disable = icl_pll_disable,
> >> >> +	.get_hw_state = mg_pll_get_hw_state,
> >> >> +};
> >> >> +
> >> >>  static const struct dpll_info icl_plls[] = {
> >> >>  	{ "DPLL 0",   &icl_pll_funcs, DPLL_ID_ICL_DPLL0,  0 },
> >> >>  	{ "DPLL 1",   &icl_pll_funcs, DPLL_ID_ICL_DPLL1,  0 },
> >> >>  	{ "TBT PLL",  &icl_pll_funcs, DPLL_ID_ICL_TBTPLL, 0 },
> >> >> -	{ "MG PLL 1", &icl_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> >> >> -	{ "MG PLL 2", &icl_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> >> >> -	{ "MG PLL 3", &icl_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> >> >> -	{ "MG PLL 4", &icl_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> >> >> +	{ "MG PLL 1", &mg_pll_funcs, DPLL_ID_ICL_MGPLL1, 0 },
> >> >> +	{ "MG PLL 2", &mg_pll_funcs, DPLL_ID_ICL_MGPLL2, 0 },
> >> >> +	{ "MG PLL 3", &mg_pll_funcs, DPLL_ID_ICL_MGPLL3, 0 },
> >> >> +	{ "MG PLL 4", &mg_pll_funcs, DPLL_ID_ICL_MGPLL4, 0 },
> >> >>  	{ },
> >> >>  };
> >> >>
> >> >> --
> >> >> 2.20.0
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx@lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> >--
> >> >Ville Syrjälä
> >> >Intel
> >
> >-- 
> >Ville Syrjälä
> >Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] drm/i915/icl: decouple dpll ids from type
  2019-02-26 19:02         ` Lucas De Marchi
@ 2019-02-27 18:15           ` Ville Syrjälä
  0 siblings, 0 replies; 15+ messages in thread
From: Ville Syrjälä @ 2019-02-27 18:15 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: intel-gfx

On Tue, Feb 26, 2019 at 11:02:58AM -0800, Lucas De Marchi wrote:
> On Tue, Feb 26, 2019 at 04:48:23PM +0200, Ville Syrjälä wrote:
> >> >This seems a rather roundabout way of doing things when we already have
> >> >the vfuncs.
> >> >
> >> >How about just:
> >> >
> >> >mg_pll_enable()
> >> >{
> >> >	icl_pll_enable(MG_REG);
> >> >}
> >> >
> >> >combo_pll_enable()
> >> >{
> >> >	icl_pll_enable(COMBO_REG);
> >> >}
> >> >
> >> >or something along those lines.
> >>
> >> humn... I thought about this approach as an intermediate step to the
> >> full blown "add another vfunc struct and pass that instead".  Platforms
> >> are free to use this for small differences that don't justify going that
> >> route.
> >>
> >> In your approach you go the route of "always use vfunc and add
> >> additional arguments to the common function".
> >> Compared to the approach here, it's not subjective on what to do in
> >> similar cases, but has its downsides as well.
> >>
> >> 1) The function can't be inlined so there's and extra hop when calling
> >> the vfunc
> >
> >We already have the vfunc. And even if we didn't, an extra vfunc in
> >modesetting code is probably in the noise.
> 
> I'm talking about the extra function you added here. The vfunc will call
> this, which then calls the real common function.
> 
> >> 2) if the callee is inlined we basically duplicate .text, but I doubt
> >> any compiler would do that
> >
> >Either it inlines or not. Why should we care in this particular case?
> 
> In this case I'm referring to icl_pll_enable() being inlined inside
> mg_pll_enable() and combo_pll_enable().
> 
> But let's leave alone the inlines and extra function calls and talk
> about the organization below.
> 
> >> 3) reg as the argument is probably not a good choice as it may change
> >> in the next platform - so we would need to add a "type" nonetheless
> >
> >Not sure what you mean. If the reg changes you pass in a different reg.
> >If other things differ significantly you write a new function.
> 
> because here the function can share more when I consider the *type* of
> the pll, not if it's reg 0x10, 0x30 or 0x40.
> 
> >>
> >> Since flags is already there
> >> and under-utilized I don't see much
> >> advantage on the vfunc-only approach. I'm not strongly opposed though:
> >> IMO both are better than the current state.
> >
> >If the existing mechanism is sufficient to the task then we should
> >generally use it rather than adding yet another mechanism. This
> >keeps the code more uniform and thus easier for everyone to
> >understand.
> 
> 
> both of them already exist - flags is already there. If I handle the
> *types* differently with your approach I would basically have:
> 
>     enum pll_type {
>         A,
>         B,
>         C,
>     }
> 
>     pll_enable()
>     {
>         ...
> 
>         if (type == A)
>         else if (type == B)
>         else

This thing shouldn't have any ifs in it if this is done right.

The more ifs you have the harder it is to follow the code.
Ideally we'd have none.

> 
>         ...
>     }
> 
>     a_pll_enable()
>     {
>         return pll_enable(A);
>     }
> 
>     b_pll_enable()
>     {
>         return pll_enable(B);
>     }
> 
>     c_pll_enable()
>     {
>         return pll_enable(C);
>     }
> 
>     static const struct funcs a_funcs = {
>         .enable = a_pll_enable(),
>     };
>     static const struct funcs b_funcs = {
>         .eanble = b_pll_enable(),
>     };
>     static const struct funcs c_funcs = {
>         .enable = c_pll_enable(),
>     };
> 
>     static const struct plls[] = {
>         { a_funcs, ... },
>         { b_funcs, ... },
>         { c_funcs, ... },
>     };
> 
> 
> This approach has its value when the implementations are completely
> different - e.g. the mg vs combo approach in patch 1. When the
> implementation is very similar, what I'm proposing is to be able to do:
> 
>     enum pll_type {
>         A,
>         B,
>         C,
>     }
> 
>     pll_enable()
>     {
>         ...
> 
>         if (type == A)
>         else if (type == B)
>         else
> 
>         ...
>     }
> 
>     static const struct funcs funcs = {
>         .enable = pll_enable(),
>     };
>  
>     static const struct plls[] = {
>         { funcs, A, ... }
>         { funcs, B, ... }
>         { funcs, C, ... }
>     }
> 
> We have less boilerplate code and the information is in the table rather
> than spread across multiple functions. Don't get me wrong, the other
> approach has its place and is even used in patch 1 because the impl
> is totally different.
> 
> In the ICL case, the type in the table is used to tweak the behavior for
> MG vs TBT, because they reuse most of the same calls. Combo vs MG is
> handled in patch 1, not here.
> 
> Lucas De Marchi

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-02-27 18:15 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-22 23:23 [PATCH 1/3] drm/i915/icl: move MG pll hw_state readout Lucas De Marchi
2019-02-22 23:23 ` [PATCH 2/3] drm/i915: introduce platform_flags to use with dpll hooks Lucas De Marchi
2019-02-22 23:23 ` [PATCH 3/3] drm/i915/icl: decouple dpll ids from type Lucas De Marchi
2019-02-25 20:45   ` Ville Syrjälä
2019-02-25 21:28     ` Lucas De Marchi
2019-02-26 14:48       ` Ville Syrjälä
2019-02-26 19:02         ` Lucas De Marchi
2019-02-27 18:15           ` Ville Syrjälä
2019-02-23  0:09 ` ✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915/icl: move MG pll hw_state readout Patchwork
2019-02-23  6:03 ` ✓ Fi.CI.IGT: " Patchwork
2019-02-25 20:42 ` [PATCH 1/3] " Ville Syrjälä
2019-02-26  0:03   ` Lucas De Marchi
2019-02-26 14:21     ` Ville Syrjälä
2019-02-26 19:15       ` Lucas De Marchi
2019-02-27 18:13         ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.