[PATCH 0/7] Shared DPLL kernel doc and improvements

[PATCH 0/7] Shared DPLL kernel doc and improvements

[Ander Conselvan de Oliveira]

Hi,

This is a resend of some previous patches adding documentation to the
shared dpll stuff, which I didn't merge since the first patch still
lacks review, plus some more changes on top to make the interface
more self contained.

Thanks,
Ander

Ander Conselvan de Oliveira (7):
  drm/i915: Introduce intel_release_shared_dpll()
  drm/i915: Rename intel_shared_dpll_commit() to _swap_state()
  drm/i915: Rename intel_shared_dpll_config to intel_shared_dpll_state
  drm/i915: Rename intel_shared_dpll->mode_set() to prepare()
  drm/i915: Update kerneldoc for intel_dpll_mgr.c
  drm/i915: Add dpll entrypoint for dumpoing hw state
  drm/i915: Add entrypoints for mapping dplls to encoders and crtcs

 Documentation/gpu/i915.rst            |  12 +
 drivers/gpu/drm/i915/i915_debugfs.c   |  12 +-
 drivers/gpu/drm/i915/intel_atomic.c   |   6 +-
 drivers/gpu/drm/i915/intel_ddi.c      |  61 ++--
 drivers/gpu/drm/i915/intel_display.c  | 189 ++---------
 drivers/gpu/drm/i915/intel_dp_mst.c   |   4 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 581 ++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 184 +++++++++--
 drivers/gpu/drm/i915/intel_drv.h      |   8 +-
 9 files changed, 745 insertions(+), 312 deletions(-)

-- 
2.5.5

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

[PATCH 1/7] drm/i915: Introduce intel_release_shared_dpll()

[Ander Conselvan de Oliveira]

While the details of getting a shared dpll are wrapped by
intel_get_shared_dpll(), the release was still hand rolled into the
modeset code. Fix that by creating an entry point for releasing the
pll and move that code there.

v2: Take old_dpll from crtc->state instead of crtc_state. (CI)
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  |  6 +----
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 41 +++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 11 +++-------
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a366656..28d9d3e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13768,7 +13768,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_shared_dpll_config *shared_dpll = NULL;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
@@ -13789,10 +13788,7 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 		if (!old_dpll)
 			continue;
 
-		if (!shared_dpll)
-			shared_dpll = intel_atomic_get_shared_dpll_state(state);
-
-		intel_shared_dpll_config_put(shared_dpll, old_dpll, intel_crtc);
+		intel_release_shared_dpll(old_dpll, intel_crtc, state);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 1c59ca5..f1b3feb 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -79,28 +79,6 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 	return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
 }
 
-void
-intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
-
-	config[id].crtc_mask |= 1 << crtc->pipe;
-}
-
-void
-intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
-
-	config[id].crtc_mask &= ~(1 << crtc->pipe);
-}
-
 /* For ILK+ */
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll,
@@ -285,7 +263,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	intel_shared_dpll_config_get(shared_dpll, pll, crtc);
+	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
 }
 
 void intel_shared_dpll_commit(struct drm_atomic_state *state)
@@ -1900,3 +1878,20 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
 
 	return dpll_mgr->get_dpll(crtc, crtc_state, encoder);
 }
+
+/**
+ * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state
+ * @dpll: dpll in use by @crtc
+ * @crtc: crtc
+ * @state: atomic state
+ *
+ */
+void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
+			       struct intel_crtc *crtc,
+			       struct drm_atomic_state *state)
+{
+	struct intel_shared_dpll_config *shared_dpll_config;
+
+	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);
+	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
+}
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index f438535..99a82c9 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -138,14 +138,6 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
 enum intel_dpll_id
 intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 			 struct intel_shared_dpll *pll);
-void
-intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc);
-void
-intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll,
 			bool state);
@@ -154,6 +146,9 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 						struct intel_crtc_state *state,
 						struct intel_encoder *encoder);
+void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
+			       struct intel_crtc *crtc,
+			       struct drm_atomic_state *state);
 void intel_prepare_shared_dpll(struct intel_crtc *crtc);
 void intel_enable_shared_dpll(struct intel_crtc *crtc);
 void intel_disable_shared_dpll(struct intel_crtc *crtc);
-- 
2.5.5

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

[PATCH 2/7] drm/i915: Rename intel_shared_dpll_commit() to _swap_state()

[Ander Conselvan de Oliveira]

The function intel_shared_dpll_commit() performs the equivalent of
drm_atomic_helper_swap_state() for the shared dpll state, which is not
handled by the helpers. So rename it for consistency.

v2: Fix typo in the commit message. (Durga)
v3: Rebase.
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 2 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 28d9d3e..b8bfde0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14530,7 +14530,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_swap_state(state, true);
 	dev_priv->wm.distrust_bios_wm = false;
 	dev_priv->wm.skl_results = intel_state->wm_results;
-	intel_shared_dpll_commit(state);
+	intel_shared_dpll_swap_state(state);
 	intel_atomic_track_fbs(state);
 
 	if (nonblock)
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index f1b3feb..15bf462 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -266,7 +266,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
 }
 
-void intel_shared_dpll_commit(struct drm_atomic_state *state)
+void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
 	struct intel_shared_dpll_config *shared_dpll;
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 99a82c9..06d61c5 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -152,7 +152,7 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
 void intel_prepare_shared_dpll(struct intel_crtc *crtc);
 void intel_enable_shared_dpll(struct intel_crtc *crtc);
 void intel_disable_shared_dpll(struct intel_crtc *crtc);
-void intel_shared_dpll_commit(struct drm_atomic_state *state);
+void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
 void intel_shared_dpll_init(struct drm_device *dev);
 
 /* BXT dpll related functions */
-- 
2.5.5

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

[PATCH 3/7] drm/i915: Rename intel_shared_dpll_config to intel_shared_dpll_state

[Ander Conselvan de Oliveira]

Struct intel_shared_dpll_config is used to hold the state of the DPLL in
the "atomic" sense, so call it state like everything else atomic.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c   | 12 +++----
 drivers/gpu/drm/i915/intel_atomic.c   |  6 ++--
 drivers/gpu/drm/i915/intel_ddi.c      | 10 +++---
 drivers/gpu/drm/i915/intel_display.c  | 22 ++++++------
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 68 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  4 +--
 drivers/gpu/drm/i915/intel_drv.h      |  4 +--
 7 files changed, 63 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index da71413..12b97af 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3229,14 +3229,14 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 
 		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->name, pll->id);
 		seq_printf(m, " crtc_mask: 0x%08x, active: 0x%x, on: %s\n",
-			   pll->config.crtc_mask, pll->active_mask, yesno(pll->on));
+			   pll->state.crtc_mask, pll->active_mask, yesno(pll->on));
 		seq_printf(m, " tracked hardware state:\n");
-		seq_printf(m, " dpll:    0x%08x\n", pll->config.hw_state.dpll);
+		seq_printf(m, " dpll:    0x%08x\n", pll->state.hw_state.dpll);
 		seq_printf(m, " dpll_md: 0x%08x\n",
-			   pll->config.hw_state.dpll_md);
-		seq_printf(m, " fp0:     0x%08x\n", pll->config.hw_state.fp0);
-		seq_printf(m, " fp1:     0x%08x\n", pll->config.hw_state.fp1);
-		seq_printf(m, " wrpll:   0x%08x\n", pll->config.hw_state.wrpll);
+			   pll->state.hw_state.dpll_md);
+		seq_printf(m, " fp0:     0x%08x\n", pll->state.hw_state.fp0);
+		seq_printf(m, " fp1:     0x%08x\n", pll->state.hw_state.fp1);
+		seq_printf(m, " wrpll:   0x%08x\n", pll->state.hw_state.wrpll);
 	}
 	drm_modeset_unlock_all(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index c5a1667..fa6dc43 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -267,7 +267,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 
 static void
 intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
-				  struct intel_shared_dpll_config *shared_dpll)
+				  struct intel_shared_dpll_state *shared_dpll)
 {
 	enum intel_dpll_id i;
 
@@ -275,11 +275,11 @@ intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
-		shared_dpll[i] = pll->config;
+		shared_dpll[i] = pll->state;
 	}
 }
 
-struct intel_shared_dpll_config *
+struct intel_shared_dpll_state *
 intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s)
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(s);
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 35f0b7c..4077205 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -992,7 +992,7 @@ static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
 		return 0;
 
 	pll = &dev_priv->shared_dplls[dpll];
-	state = &pll->config.hw_state;
+	state = &pll->state.hw_state;
 
 	clock.m1 = 2;
 	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
@@ -2401,7 +2401,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct intel_shared_dpll *pll = NULL;
-	struct intel_shared_dpll_config tmp_pll_config;
+	struct intel_shared_dpll_state tmp_pll_state;
 	enum intel_dpll_id dpll_id;
 
 	if (IS_BROXTON(dev_priv)) {
@@ -2417,11 +2417,11 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
 				  pll->active_mask);
 			return NULL;
 		}
-		tmp_pll_config = pll->config;
+		tmp_pll_state = pll->state;
 		if (!bxt_ddi_dp_set_dpll_hw_state(clock,
-						  &pll->config.hw_state)) {
+						  &pll->state.hw_state)) {
 			DRM_ERROR("Could not setup DPLL\n");
-			pll->config = tmp_pll_config;
+			pll->state = tmp_pll_state;
 			return NULL;
 		}
 	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8bfde0..e6fe85b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13639,9 +13639,9 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
 	}
 
 	if (!crtc) {
-		I915_STATE_WARN(pll->active_mask & ~pll->config.crtc_mask,
+		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
 				"more active pll users than references: %x vs %x\n",
-				pll->active_mask, pll->config.crtc_mask);
+				pll->active_mask, pll->state.crtc_mask);
 
 		return;
 	}
@@ -13657,11 +13657,11 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
 				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
 				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
 
-	I915_STATE_WARN(!(pll->config.crtc_mask & crtc_mask),
+	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
 			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
-			crtc_mask, pll->config.crtc_mask);
+			crtc_mask, pll->state.crtc_mask);
 
-	I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state,
+	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
 					  &dpll_hw_state,
 					  sizeof(dpll_hw_state)),
 			"pll hw state mismatch\n");
@@ -13687,7 +13687,7 @@ verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
 		I915_STATE_WARN(pll->active_mask & crtc_mask,
 				"pll active mismatch (didn't expect pipe %c in active mask)\n",
 				pipe_name(drm_crtc_index(crtc)));
-		I915_STATE_WARN(pll->config.crtc_mask & crtc_mask,
+		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
 				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
 				pipe_name(drm_crtc_index(crtc)));
 	}
@@ -16759,16 +16759,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
 
 		pll->on = pll->funcs.get_hw_state(dev_priv, pll,
-						  &pll->config.hw_state);
-		pll->config.crtc_mask = 0;
+						  &pll->state.hw_state);
+		pll->state.crtc_mask = 0;
 		for_each_intel_crtc(dev, crtc) {
 			if (crtc->active && crtc->config->shared_dpll == pll)
-				pll->config.crtc_mask |= 1 << crtc->pipe;
+				pll->state.crtc_mask |= 1 << crtc->pipe;
 		}
-		pll->active_mask = pll->config.crtc_mask;
+		pll->active_mask = pll->state.crtc_mask;
 
 		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
-			      pll->name, pll->config.crtc_mask, pll->on);
+			      pll->name, pll->state.crtc_mask, pll->on);
 	}
 
 	for_each_intel_encoder(dev, encoder) {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 15bf462..c88fc07 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -38,11 +38,11 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
 		pll = &dev_priv->shared_dplls[i];
 
 		/* Only want to check enabled timings first */
-		if (pll->config.crtc_mask == 0)
+		if (pll->state.crtc_mask == 0)
 			continue;
 
-		if (memcmp(&dpll_hw_state, &pll->config.hw_state,
-			   sizeof(pll->config.hw_state)) == 0) {
+		if (memcmp(&dpll_hw_state, &pll->state.hw_state,
+			   sizeof(pll->state.hw_state)) == 0) {
 			found = true;
 			break;
 		}
@@ -52,8 +52,8 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
 	for (i = DPLL_ID_SKL_DPLL1;
 	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {
 		pll = &dev_priv->shared_dplls[i];
-		if (pll->config.crtc_mask == 0) {
-			pll->config.hw_state = dpll_hw_state;
+		if (pll->state.crtc_mask == 0) {
+			pll->state.hw_state = dpll_hw_state;
 			break;
 		}
 	}
@@ -106,7 +106,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 		return;
 
 	mutex_lock(&dev_priv->dpll_lock);
-	WARN_ON(!pll->config.crtc_mask);
+	WARN_ON(!pll->state.crtc_mask);
 	if (!pll->active_mask) {
 		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
 		WARN_ON(pll->on);
@@ -139,7 +139,7 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	mutex_lock(&dev_priv->dpll_lock);
 	old_mask = pll->active_mask;
 
-	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
+	if (WARN_ON(!(pll->state.crtc_mask & crtc_mask)) ||
 	    WARN_ON(pll->active_mask & crtc_mask))
 		goto out;
 
@@ -209,7 +209,7 @@ intel_find_shared_dpll(struct intel_crtc *crtc,
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	struct intel_shared_dpll *pll;
-	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_shared_dpll_state *shared_dpll;
 	enum intel_dpll_id i;
 
 	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
@@ -249,7 +249,7 @@ static void
 intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 			    struct intel_crtc_state *crtc_state)
 {
-	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_shared_dpll_state *shared_dpll;
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	enum intel_dpll_id i = pll->id;
 
@@ -269,7 +269,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
-	struct intel_shared_dpll_config *shared_dpll;
+	struct intel_shared_dpll_state *shared_dpll;
 	struct intel_shared_dpll *pll;
 	enum intel_dpll_id i;
 
@@ -279,7 +279,7 @@ void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
 	shared_dpll = to_intel_atomic_state(state)->shared_dpll;
 	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
 		pll = &dev_priv->shared_dplls[i];
-		pll->config = shared_dpll[i];
+		pll->state = shared_dpll[i];
 	}
 }
 
@@ -305,8 +305,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
 				  struct intel_shared_dpll *pll)
 {
-	I915_WRITE(PCH_FP0(pll->id), pll->config.hw_state.fp0);
-	I915_WRITE(PCH_FP1(pll->id), pll->config.hw_state.fp1);
+	I915_WRITE(PCH_FP0(pll->id), pll->state.hw_state.fp0);
+	I915_WRITE(PCH_FP1(pll->id), pll->state.hw_state.fp1);
 }
 
 static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
@@ -328,7 +328,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 	/* PCH refclock must be enabled first */
 	ibx_assert_pch_refclk_enabled(dev_priv);
 
-	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), pll->state.hw_state.dpll);
 
 	/* Wait for the clocks to stabilize. */
 	POSTING_READ(PCH_DPLL(pll->id));
@@ -339,7 +339,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
 	 *
 	 * So write it again.
 	 */
-	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
+	I915_WRITE(PCH_DPLL(pll->id), pll->state.hw_state.dpll);
 	POSTING_READ(PCH_DPLL(pll->id));
 	udelay(200);
 }
@@ -401,7 +401,7 @@ static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
 static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
 			       struct intel_shared_dpll *pll)
 {
-	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
+	I915_WRITE(WRPLL_CTL(pll->id), pll->state.hw_state.wrpll);
 	POSTING_READ(WRPLL_CTL(pll->id));
 	udelay(20);
 }
@@ -409,7 +409,7 @@ static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
 static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv,
 				struct intel_shared_dpll *pll)
 {
-	I915_WRITE(SPLL_CTL, pll->config.hw_state.spll);
+	I915_WRITE(SPLL_CTL, pll->state.hw_state.spll);
 	POSTING_READ(SPLL_CTL);
 	udelay(20);
 }
@@ -852,7 +852,7 @@ static void skl_ddi_pll_write_ctrl1(struct drm_i915_private *dev_priv,
 
 	val &= ~(DPLL_CTRL1_HDMI_MODE(pll->id) | DPLL_CTRL1_SSC(pll->id) |
 		 DPLL_CTRL1_LINK_RATE_MASK(pll->id));
-	val |= pll->config.hw_state.ctrl1 << (pll->id * 6);
+	val |= pll->state.hw_state.ctrl1 << (pll->id * 6);
 
 	I915_WRITE(DPLL_CTRL1, val);
 	POSTING_READ(DPLL_CTRL1);
@@ -865,8 +865,8 @@ static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
 
 	skl_ddi_pll_write_ctrl1(dev_priv, pll);
 
-	I915_WRITE(regs[pll->id].cfgcr1, pll->config.hw_state.cfgcr1);
-	I915_WRITE(regs[pll->id].cfgcr2, pll->config.hw_state.cfgcr2);
+	I915_WRITE(regs[pll->id].cfgcr1, pll->state.hw_state.cfgcr1);
+	I915_WRITE(regs[pll->id].cfgcr2, pll->state.hw_state.cfgcr2);
 	POSTING_READ(regs[pll->id].cfgcr1);
 	POSTING_READ(regs[pll->id].cfgcr2);
 
@@ -1363,31 +1363,31 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	/* Write P1 & P2 */
 	temp = I915_READ(BXT_PORT_PLL_EBB_0(port));
 	temp &= ~(PORT_PLL_P1_MASK | PORT_PLL_P2_MASK);
-	temp |= pll->config.hw_state.ebb0;
+	temp |= pll->state.hw_state.ebb0;
 	I915_WRITE(BXT_PORT_PLL_EBB_0(port), temp);
 
 	/* Write M2 integer */
 	temp = I915_READ(BXT_PORT_PLL(port, 0));
 	temp &= ~PORT_PLL_M2_MASK;
-	temp |= pll->config.hw_state.pll0;
+	temp |= pll->state.hw_state.pll0;
 	I915_WRITE(BXT_PORT_PLL(port, 0), temp);
 
 	/* Write N */
 	temp = I915_READ(BXT_PORT_PLL(port, 1));
 	temp &= ~PORT_PLL_N_MASK;
-	temp |= pll->config.hw_state.pll1;
+	temp |= pll->state.hw_state.pll1;
 	I915_WRITE(BXT_PORT_PLL(port, 1), temp);
 
 	/* Write M2 fraction */
 	temp = I915_READ(BXT_PORT_PLL(port, 2));
 	temp &= ~PORT_PLL_M2_FRAC_MASK;
-	temp |= pll->config.hw_state.pll2;
+	temp |= pll->state.hw_state.pll2;
 	I915_WRITE(BXT_PORT_PLL(port, 2), temp);
 
 	/* Write M2 fraction enable */
 	temp = I915_READ(BXT_PORT_PLL(port, 3));
 	temp &= ~PORT_PLL_M2_FRAC_ENABLE;
-	temp |= pll->config.hw_state.pll3;
+	temp |= pll->state.hw_state.pll3;
 	I915_WRITE(BXT_PORT_PLL(port, 3), temp);
 
 	/* Write coeff */
@@ -1395,24 +1395,24 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	temp &= ~PORT_PLL_PROP_COEFF_MASK;
 	temp &= ~PORT_PLL_INT_COEFF_MASK;
 	temp &= ~PORT_PLL_GAIN_CTL_MASK;
-	temp |= pll->config.hw_state.pll6;
+	temp |= pll->state.hw_state.pll6;
 	I915_WRITE(BXT_PORT_PLL(port, 6), temp);
 
 	/* Write calibration val */
 	temp = I915_READ(BXT_PORT_PLL(port, 8));
 	temp &= ~PORT_PLL_TARGET_CNT_MASK;
-	temp |= pll->config.hw_state.pll8;
+	temp |= pll->state.hw_state.pll8;
 	I915_WRITE(BXT_PORT_PLL(port, 8), temp);
 
 	temp = I915_READ(BXT_PORT_PLL(port, 9));
 	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
-	temp |= pll->config.hw_state.pll9;
+	temp |= pll->state.hw_state.pll9;
 	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
 
 	temp = I915_READ(BXT_PORT_PLL(port, 10));
 	temp &= ~PORT_PLL_DCO_AMP_OVR_EN_H;
 	temp &= ~PORT_PLL_DCO_AMP_MASK;
-	temp |= pll->config.hw_state.pll10;
+	temp |= pll->state.hw_state.pll10;
 	I915_WRITE(BXT_PORT_PLL(port, 10), temp);
 
 	/* Recalibrate with new settings */
@@ -1420,7 +1420,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	temp |= PORT_PLL_RECALIBRATE;
 	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
 	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
-	temp |= pll->config.hw_state.ebb4;
+	temp |= pll->state.hw_state.ebb4;
 	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
 
 	/* Enable PLL */
@@ -1440,7 +1440,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
 	temp = I915_READ(BXT_PORT_PCS_DW12_LN01(port));
 	temp &= ~LANE_STAGGER_MASK;
 	temp &= ~LANESTAGGER_STRAP_OVRD;
-	temp |= pll->config.hw_state.pcsdw12;
+	temp |= pll->state.hw_state.pcsdw12;
 	I915_WRITE(BXT_PORT_PCS_DW12_GRP(port), temp);
 }
 
@@ -1890,8 +1890,8 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
 			       struct intel_crtc *crtc,
 			       struct drm_atomic_state *state)
 {
-	struct intel_shared_dpll_config *shared_dpll_config;
+	struct intel_shared_dpll_state *shared_dpll_state;
 
-	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);
-	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
+	shared_dpll_state = intel_atomic_get_shared_dpll_state(state);
+	shared_dpll_state[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
 }
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 06d61c5..6e3a0f1 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -93,7 +93,7 @@ struct intel_dpll_hw_state {
 		 pcsdw12;
 };
 
-struct intel_shared_dpll_config {
+struct intel_shared_dpll_state {
 	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
 	struct intel_dpll_hw_state hw_state;
 };
@@ -113,7 +113,7 @@ struct intel_shared_dpll_funcs {
 };
 
 struct intel_shared_dpll {
-	struct intel_shared_dpll_config config;
+	struct intel_shared_dpll_state state;
 
 	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
 	bool on; /* is the PLL actually active? Disabled during modeset */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f48e79a..7874f66 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -355,7 +355,7 @@ struct intel_atomic_state {
 	/* SKL/KBL Only */
 	unsigned int cdclk_pll_vco;
 
-	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
+	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
 
 	/*
 	 * Current watermarks can't be trusted during hardware readout, so
@@ -1798,7 +1798,7 @@ void intel_crtc_destroy_state(struct drm_crtc *crtc,
 			       struct drm_crtc_state *state);
 struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
 void intel_atomic_state_clear(struct drm_atomic_state *);
-struct intel_shared_dpll_config *
+struct intel_shared_dpll_state *
 intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
 
 static inline struct intel_crtc_state *
-- 
2.5.5

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

[PATCH 4/7] drm/i915: Rename intel_shared_dpll->mode_set() to prepare()

[Ander Conselvan de Oliveira]

The hook is called from intel_prepare_shared_dpll(). The name doesn't
make sense after all the changes to modeset code. So just call it
prepare.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 8 ++++----
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index c88fc07..9446446 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -112,7 +112,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 		WARN_ON(pll->on);
 		assert_shared_dpll_disabled(dev_priv, pll);
 
-		pll->funcs.mode_set(dev_priv, pll);
+		pll->funcs.prepare(dev_priv, pll);
 	}
 	mutex_unlock(&dev_priv->dpll_lock);
 }
@@ -302,8 +302,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 	return val & DPLL_VCO_ENABLE;
 }
 
-static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
-				  struct intel_shared_dpll *pll)
+static void ibx_pch_dpll_prepare(struct drm_i915_private *dev_priv,
+				 struct intel_shared_dpll *pll)
 {
 	I915_WRITE(PCH_FP0(pll->id), pll->state.hw_state.fp0);
 	I915_WRITE(PCH_FP1(pll->id), pll->state.hw_state.fp1);
@@ -392,7 +392,7 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 }
 
 static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
-	.mode_set = ibx_pch_dpll_mode_set,
+	.prepare = ibx_pch_dpll_prepare,
 	.enable = ibx_pch_dpll_enable,
 	.disable = ibx_pch_dpll_disable,
 	.get_hw_state = ibx_pch_dpll_get_hw_state,
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 6e3a0f1..9a7db65 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -101,7 +101,7 @@ struct intel_shared_dpll_state {
 struct intel_shared_dpll_funcs {
 	/* The mode_set hook is optional and should be used together with the
 	 * intel_prepare_shared_dpll function. */
-	void (*mode_set)(struct drm_i915_private *dev_priv,
+	void (*prepare)(struct drm_i915_private *dev_priv,
 			 struct intel_shared_dpll *pll);
 	void (*enable)(struct drm_i915_private *dev_priv,
 		       struct intel_shared_dpll *pll);
-- 
2.5.5

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

[PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Ander Conselvan de Oliveira]

The documentation for most of the non-static members and structs were
missing. Fix that.

v2: Fix typos (Durga)

v3: Rebase.
    Fix make docs warnings.
    Document more.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 Documentation/gpu/i915.rst            |  12 +++
 drivers/gpu/drm/i915/intel_dpll_mgr.c |  90 ++++++++++++++++++--
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 155 ++++++++++++++++++++++++++++++----
 3 files changed, 237 insertions(+), 20 deletions(-)

diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
index 87aaffc..c19e437 100644
--- a/Documentation/gpu/i915.rst
+++ b/Documentation/gpu/i915.rst
@@ -204,6 +204,18 @@ Video BIOS Table (VBT)
 .. kernel-doc:: drivers/gpu/drm/i915/intel_vbt_defs.h
    :internal:
 
+Display PLLs
+------------
+
+.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
+   :doc: Display PLLs
+
+.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.h
+   :internal:
+
 Memory Management and Command Submission
 ========================================
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 9446446..8c4efa9 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -23,6 +23,25 @@
 
 #include "intel_drv.h"
 
+/**
+ * DOC: Display PLLs
+ *
+ * Display PLLs used for driving outputs vary by platform. While some have
+ * per-pipe or per-encoder dedicated PLLs, others allow the use of any PLL
+ * from a pool. In the latter scenario, it is possible that multiple pipes
+ * share a PLL if their configurations match.
+ *
+ * This file provides an abstraction over display PLLs. The function
+ * intel_shared_dpll_init() initializes the PLLs for the given platform.  The
+ * users of a PLL are tracked and that tracking is integrated with the atomic
+ * modest interface. During an atomic operation, a PLL can be requested for a
+ * given crtc and encoder configuration by calling intel_get_shared_dpll() and
+ * a previously used PLL can be released with intel_release_shared_dpll().
+ * Changes to the users are first staged in the atomic state, and then made
+ * effective by calling intel_shared_dpll_swap_state() during the atomic
+ * commit phase.
+ */
+
 struct intel_shared_dpll *
 skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
 {
@@ -61,6 +80,14 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
 	return pll;
 }
 
+/**
+ * intel_get_shared_dpll_by_id - get a DPLL given its id
+ * @dev_priv: i915 device instance
+ * @id: pll id
+ *
+ * Returns:
+ * A pointer to the DPLL with @id
+ */
 struct intel_shared_dpll *
 intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
 			    enum intel_dpll_id id)
@@ -68,6 +95,14 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
 	return &dev_priv->shared_dplls[id];
 }
 
+/**
+ * intel_get_shared_dpll_id - get the id of a DPLL
+ * @dev_priv: i915 device instance
+ * @pll: the DPLL
+ *
+ * Returns:
+ * The id of @pll
+ */
 enum intel_dpll_id
 intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 			 struct intel_shared_dpll *pll)
@@ -96,6 +131,13 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			pll->name, onoff(state), onoff(cur_state));
 }
 
+/**
+ * intel_prepare_shared_dpll - call a dpll's prepare hook
+ * @crtc: crtc which has a shared dpll
+ *
+ * This calls the PLL's prepare hook if it has one and if the PLL is not
+ * already enabled. The prepare hook is platform specific.
+ */
 void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -118,12 +160,10 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
 }
 
 /**
- * intel_enable_shared_dpll - enable PCH PLL
- * @dev_priv: i915 private structure
- * @pipe: pipe PLL to enable
+ * intel_enable_shared_dpll - enable a crtc's shared DPLL
+ * @crtc: crtc which has a shared DPLL
  *
- * The PCH PLL needs to be enabled before the PCH transcoder, since it
- * drives the transcoder clock.
+ * Enable the shared DPLL used by @crtc.
  */
 void intel_enable_shared_dpll(struct intel_crtc *crtc)
 {
@@ -164,6 +204,12 @@ out:
 	mutex_unlock(&dev_priv->dpll_lock);
 }
 
+/**
+ * intel_disable_shared_dpll - disable a crtc's shared DPLL
+ * @crtc: crtc which has a shared DPLL
+ *
+ * Disable the shared DPLL used by @crtc.
+ */
 void intel_disable_shared_dpll(struct intel_crtc *crtc)
 {
 	struct drm_device *dev = crtc->base.dev;
@@ -266,6 +312,16 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
 }
 
+/**
+ * intel_shared_dpll_swap_state - make atomic DPLL configuration effective
+ * @state: atomic state
+ *
+ * This is the dpll version of drm_atomic_helper_swap_state() since the
+ * helper does not handle driver-specific global state.
+ *
+ * Note that this doesn't actually swap states, the dpll configutation in
+ * @state is left unchanged.
+ */
 void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->dev);
@@ -1822,6 +1878,12 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
 	.get_dpll = bxt_get_dpll,
 };
 
+/**
+ * intel_shared_dpll_init - Initialize shared DPLLs
+ * @dev: drm device
+ *
+ * Initialize shared DPLLs for @dev.
+ */
 void intel_shared_dpll_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -1865,6 +1927,21 @@ void intel_shared_dpll_init(struct drm_device *dev)
 		intel_ddi_pll_init(dev);
 }
 
+/**
+ * intel_get_shared_dpll - get a shared DPLL for crtc and encoder combination
+ * @crtc: crtc
+ * @crtc_state: atomic state for @crtc
+ * @encoder: encoder
+ *
+ * Find an appropriate DPLL for the given crtc and encoder combination. A
+ * reference from the @crtc to the returned pll is registered in the atomic
+ * state. That configuration is made effective by calling
+ * intel_shared_dpll_swap_state(). The reference should be released by calling
+ * intel_release_shared_dpll().
+ *
+ * Returns:
+ * A shared DPLL to be used by @crtc and @encoder with the given @crtc_state.
+ */
 struct intel_shared_dpll *
 intel_get_shared_dpll(struct intel_crtc *crtc,
 		      struct intel_crtc_state *crtc_state,
@@ -1885,6 +1962,9 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
  * @crtc: crtc
  * @state: atomic state
  *
+ * This function releases the reference from @crtc to @dpll from the
+ * atomic @state. The new configuration is made effective by calling
+ * intel_shared_dpll_swap_state().
  */
 void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
 			       struct intel_crtc *crtc,
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 9a7db65..40f1a6f 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -40,32 +40,72 @@ struct intel_encoder;
 struct intel_shared_dpll;
 struct intel_dpll_mgr;
 
+/**
+ * enum intel_dpll_id - possible DPLL ids
+ *
+ * Enumeration of possible IDs for a DPLL. Real shared dpll ids must be >= 0.
+ */
 enum intel_dpll_id {
-	DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */
-	/* real shared dpll ids must be >= 0 */
+	/**
+	 * @DPLL_ID_PRIVATE: non-shared dpll in use
+	 */
+	DPLL_ID_PRIVATE = -1,
+
+	/**
+	 * @DPLL_ID_PCH_PLL_A: DPLL A in ILK, SNB and IVB
+	 */
 	DPLL_ID_PCH_PLL_A = 0,
+	/**
+	 * @DPLL_ID_PCH_PLL_B: DPLL B in ILK, SNB and IVB
+	 */
 	DPLL_ID_PCH_PLL_B = 1,
-	/* hsw/bdw */
+
+
+	/**
+	 * @DPLL_ID_WRPLL1: HSW and BDW WRPLL1
+	 */
 	DPLL_ID_WRPLL1 = 0,
+	/**
+	 * @DPLL_ID_WRPLL2: HSW and BDW WRPLL2
+	 */
 	DPLL_ID_WRPLL2 = 1,
+	/**
+	 * @DPLL_ID_SPLL: HSW and BDW SPLL
+	 */
 	DPLL_ID_SPLL = 2,
+	/**
+	 * @DPLL_ID_LCPLL_810: HSW and BDW 0.81 GHz LCPLL
+	 */
 	DPLL_ID_LCPLL_810 = 3,
+	/**
+	 * @DPLL_ID_LCPLL_1350: HSW and BDW 1.35 GHz LCPLL
+	 */
 	DPLL_ID_LCPLL_1350 = 4,
+	/**
+	 * @DPLL_ID_LCPLL_2700: HSW and BDW 2.7 GHz LCPLL
+	 */
 	DPLL_ID_LCPLL_2700 = 5,
 
-	/* skl */
+
+	/**
+	 * @DPLL_ID_SKL_DPLL0: SKL and later DPLL0
+	 */
 	DPLL_ID_SKL_DPLL0 = 0,
+	/**
+	 * @DPLL_ID_SKL_DPLL1: SKL and later DPLL1
+	 */
 	DPLL_ID_SKL_DPLL1 = 1,
+	/**
+	 * @DPLL_ID_SKL_DPLL2: SKL and later DPLL2
+	 */
 	DPLL_ID_SKL_DPLL2 = 2,
+	/**
+	 * @DPLL_ID_SKL_DPLL3: SKL and later DPLL3
+	 */
 	DPLL_ID_SKL_DPLL3 = 3,
 };
 #define I915_NUM_PLLS 6
 
-/** Inform the state checker that the DPLL is kept enabled even if not
- * in use by any crtc.
- */
-#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
-
 struct intel_dpll_hw_state {
 	/* i9xx, pch plls */
 	uint32_t dpll;
@@ -93,39 +133,124 @@ struct intel_dpll_hw_state {
 		 pcsdw12;
 };
 
+/**
+ * struct intel_shared_dpll_state - hold the DPLL atomic state
+ *
+ * This structure holds an atomic state for the DPLL, that can represent
+ * either its current state or a desired furture state which would be
+ * applied by an atomic mode set.
+ */
 struct intel_shared_dpll_state {
-	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
+	/**
+	 * @crtc_mask: mask of crtc using this DPLL, active or not
+	 */
+	unsigned crtc_mask;
+
+	/**
+	 * @hw_state: hardware configuration for the DPLL.
+	 */
 	struct intel_dpll_hw_state hw_state;
 };
 
+/**
+ * struct intel_shared_dpll_funcs - platform specific hooks for managing DPLLs
+ */
 struct intel_shared_dpll_funcs {
-	/* The mode_set hook is optional and should be used together with the
-	 * intel_prepare_shared_dpll function. */
+	/**
+	 * @prepare:
+	 *
+	 * Optional hook to perform operations prior to enabling the PLL.
+	 * Called from intel_prepare_shared_dpll() function.
+	 */
 	void (*prepare)(struct drm_i915_private *dev_priv,
 			 struct intel_shared_dpll *pll);
+
+	/**
+	 * @enable:
+	 *
+	 * Hook for enabling the pll, called from intel_enable_shared_dpll()
+	 * if the pll is not already enabled.
+	 */
 	void (*enable)(struct drm_i915_private *dev_priv,
 		       struct intel_shared_dpll *pll);
+
+	/**
+	 * @disable:
+	 *
+	 * Hook for disabling the pll, called from intel_disable_shared_dpll()
+	 * only when it is safe to disable the pll, i.e., there are no more
+	 * tracked users for it.
+	 */
 	void (*disable)(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll);
+
+	/**
+	 * @get_hw_state:
+	 *
+	 * Hook for reading the values currently programmed to the DPLL
+	 * registers. This is used for initial hw state readout and state
+	 * verification after a mode set.
+	 */
 	bool (*get_hw_state)(struct drm_i915_private *dev_priv,
 			     struct intel_shared_dpll *pll,
 			     struct intel_dpll_hw_state *hw_state);
 };
 
+/**
+ * struct intel_shared_dpll - display PLL with tracked state and users
+ */
 struct intel_shared_dpll {
+	/**
+	 * @state:
+	 *
+	 * Store the state for the pll, including the its hw state
+	 * and crtcs using it.
+	 */
 	struct intel_shared_dpll_state state;
 
-	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
-	bool on; /* is the PLL actually active? Disabled during modeset */
+	/**
+	 * @active_mask: mask of active CRTCs (i.e. DPMS on) using this DPLL
+	 */
+	unsigned active_mask;
+
+	/**
+	 * @on: is the PLL actually active? Disabled during modeset
+	 */
+	bool on;
+
+	/**
+	 * @name: DPLL name; used for logging
+	 */
 	const char *name;
-	/* should match the index in the dev_priv->shared_dplls array */
+
+	/**
+	 * @id: unique indentifier for this DPLL; should match the index in the
+	 * dev_priv->shared_dplls array
+	 */
 	enum intel_dpll_id id;
 
+	/**
+	 * @funcs: platform specific hooks
+	 */
 	struct intel_shared_dpll_funcs funcs;
 
+	/**
+	 * @flags:
+	 *
+	 * accepted flags: INTEL_DPLL_ALWAYS_ON
+	 */
 	uint32_t flags;
 };
 
+/**
+ * INTEL_DPLL_ALWAYS_ON
+ *
+ * Inform the state checker that the DPLL is kept enabled even if not
+ * in use by any crtc.
+ */
+#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
+
+
 #define SKL_DPLL0 0
 #define SKL_DPLL1 1
 #define SKL_DPLL2 2
-- 
2.5.5

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

[PATCH 6/7] drm/i915: Add dpll entrypoint for dumping hw state

[Ander Conselvan de Oliveira]

Remove the IS_PLATFORM() macros from intel_dump_pipe_config() and split
that logic in platform specific implementations inside the dpll code,
accessed through a platform independent interface.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c  | 35 +---------------
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 79 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.h |  3 ++
 3 files changed, 84 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e6fe85b..8ecaf18 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12744,6 +12744,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 				   const char *context)
 {
 	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_plane *plane;
 	struct intel_plane *intel_plane;
 	struct intel_plane_state *state;
@@ -12805,39 +12806,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
 	DRM_DEBUG_KMS("ips: %i\n", pipe_config->ips_enabled);
 	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
 
-	if (IS_BROXTON(dev)) {
-		DRM_DEBUG_KMS("dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
-			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
-			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
-			      pipe_config->dpll_hw_state.ebb0,
-			      pipe_config->dpll_hw_state.ebb4,
-			      pipe_config->dpll_hw_state.pll0,
-			      pipe_config->dpll_hw_state.pll1,
-			      pipe_config->dpll_hw_state.pll2,
-			      pipe_config->dpll_hw_state.pll3,
-			      pipe_config->dpll_hw_state.pll6,
-			      pipe_config->dpll_hw_state.pll8,
-			      pipe_config->dpll_hw_state.pll9,
-			      pipe_config->dpll_hw_state.pll10,
-			      pipe_config->dpll_hw_state.pcsdw12);
-	} else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
-		DRM_DEBUG_KMS("dpll_hw_state: "
-			      "ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
-			      pipe_config->dpll_hw_state.ctrl1,
-			      pipe_config->dpll_hw_state.cfgcr1,
-			      pipe_config->dpll_hw_state.cfgcr2);
-	} else if (HAS_DDI(dev)) {
-		DRM_DEBUG_KMS("dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
-			      pipe_config->dpll_hw_state.wrpll,
-			      pipe_config->dpll_hw_state.spll);
-	} else {
-		DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
-			      "fp0: 0x%x, fp1: 0x%x\n",
-			      pipe_config->dpll_hw_state.dpll,
-			      pipe_config->dpll_hw_state.dpll_md,
-			      pipe_config->dpll_hw_state.fp0,
-			      pipe_config->dpll_hw_state.fp1);
-	}
+	intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
 
 	DRM_DEBUG_KMS("planes on this crtc\n");
 	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 8c4efa9..9b02d9c 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -447,6 +447,17 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static void ibx_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state)
+{
+	DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
+		      "fp0: 0x%x, fp1: 0x%x\n",
+		      hw_state->dpll,
+		      hw_state->dpll_md,
+		      hw_state->fp0,
+		      hw_state->fp1);
+}
+
 static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
 	.prepare = ibx_pch_dpll_prepare,
 	.enable = ibx_pch_dpll_enable,
@@ -833,6 +844,13 @@ hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static void hsw_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state)
+{
+	DRM_DEBUG_KMS("dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
+		      hw_state->wrpll, hw_state->spll);
+}
+
 static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = {
 	.enable = hsw_ddi_wrpll_enable,
 	.disable = hsw_ddi_wrpll_disable,
@@ -1388,6 +1406,16 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static void skl_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state)
+{
+	DRM_DEBUG_KMS("dpll_hw_state: "
+		      "ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
+		      hw_state->ctrl1,
+		      hw_state->cfgcr1,
+		      hw_state->cfgcr2);
+}
+
 static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = {
 	.enable = skl_ddi_pll_enable,
 	.disable = skl_ddi_pll_disable,
@@ -1785,6 +1813,25 @@ bxt_get_dpll(struct intel_crtc *crtc,
 	return pll;
 }
 
+static void bxt_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state)
+{
+	DRM_DEBUG_KMS("dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
+		      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
+		      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
+		      hw_state->ebb0,
+		      hw_state->ebb4,
+		      hw_state->pll0,
+		      hw_state->pll1,
+		      hw_state->pll2,
+		      hw_state->pll3,
+		      hw_state->pll6,
+		      hw_state->pll8,
+		      hw_state->pll9,
+		      hw_state->pll10,
+		      hw_state->pcsdw12);
+}
+
 static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = {
 	.enable = bxt_ddi_pll_enable,
 	.disable = bxt_ddi_pll_disable,
@@ -1825,6 +1872,9 @@ struct intel_dpll_mgr {
 	struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc,
 					      struct intel_crtc_state *crtc_state,
 					      struct intel_encoder *encoder);
+
+	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state);
 };
 
 static const struct dpll_info pch_plls[] = {
@@ -1836,6 +1886,7 @@ static const struct dpll_info pch_plls[] = {
 static const struct intel_dpll_mgr pch_pll_mgr = {
 	.dpll_info = pch_plls,
 	.get_dpll = ibx_get_dpll,
+	.dump_hw_state = ibx_dump_hw_state,
 };
 
 static const struct dpll_info hsw_plls[] = {
@@ -1851,6 +1902,7 @@ static const struct dpll_info hsw_plls[] = {
 static const struct intel_dpll_mgr hsw_pll_mgr = {
 	.dpll_info = hsw_plls,
 	.get_dpll = hsw_get_dpll,
+	.dump_hw_state = hsw_dump_hw_state,
 };
 
 static const struct dpll_info skl_plls[] = {
@@ -1864,6 +1916,7 @@ static const struct dpll_info skl_plls[] = {
 static const struct intel_dpll_mgr skl_pll_mgr = {
 	.dpll_info = skl_plls,
 	.get_dpll = skl_get_dpll,
+	.dump_hw_state = skl_dump_hw_state,
 };
 
 static const struct dpll_info bxt_plls[] = {
@@ -1876,6 +1929,7 @@ static const struct dpll_info bxt_plls[] = {
 static const struct intel_dpll_mgr bxt_pll_mgr = {
 	.dpll_info = bxt_plls,
 	.get_dpll = bxt_get_dpll,
+	.dump_hw_state = bxt_dump_hw_state,
 };
 
 /**
@@ -1975,3 +2029,28 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
 	shared_dpll_state = intel_atomic_get_shared_dpll_state(state);
 	shared_dpll_state[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
 }
+
+/**
+ * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
+ * @dev_priv: i915 drm device
+ * @hw_state: hw state to be written to the log
+ *
+ * Write the relevant values in @hw_state to dmesg using DRM_DEBUG_KMS.
+ */
+void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state)
+{
+	if (dev_priv->dpll_mgr) {
+		dev_priv->dpll_mgr->dump_hw_state(dev_priv, hw_state);
+	} else {
+		/* fallback for platforms that don't use the shared dpll
+		 * infrastructure
+		 */
+		DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
+			      "fp0: 0x%x, fp1: 0x%x\n",
+			      hw_state->dpll,
+			      hw_state->dpll_md,
+			      hw_state->fp0,
+			      hw_state->fp1);
+	}
+}
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 40f1a6f..76111a4 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -280,6 +280,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
 void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
 void intel_shared_dpll_init(struct drm_device *dev);
 
+void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
+			      struct intel_dpll_hw_state *hw_state);
+
 /* BXT dpll related functions */
 bool bxt_ddi_dp_set_dpll_hw_state(int clock,
 			  struct intel_dpll_hw_state *dpll_hw_state);
-- 
2.5.5

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

[PATCH 7/7] drm/i915: Add entrypoints for mapping dplls to encoders and crtcs

[Ander Conselvan de Oliveira]

Abstract the platform specific bits of mapping the dplls under a
platform independ entrypoints so the differences between platforms are
contained in the dpll code. I.e., it removes IS_PLATFORM() macros from
other parts of the code.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c      |  51 ++----
 drivers/gpu/drm/i915/intel_display.c  | 124 +-------------
 drivers/gpu/drm/i915/intel_dp_mst.c   |   4 +-
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 299 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dpll_mgr.h |   7 +
 drivers/gpu/drm/i915/intel_drv.h      |   4 +-
 6 files changed, 334 insertions(+), 155 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 4077205..144fe5c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -319,6 +319,19 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
 	}
 }
 
+struct intel_encoder *
+intel_ddi_get_port_encoder(struct drm_i915_private *dev_priv, enum port port)
+{
+	struct intel_encoder *encoder;
+
+	for_each_intel_encoder(&dev_priv->drm, encoder) {
+		if (port == intel_ddi_get_encoder_port(encoder))
+			return encoder;
+	}
+
+	return NULL;
+}
+
 static const struct ddi_buf_trans *
 bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
 {
@@ -582,11 +595,12 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_encoder *encoder;
-	u32 temp, i, rx_ctl_val, ddi_pll_sel;
+	u32 temp, i, rx_ctl_val;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		WARN_ON(encoder->type != INTEL_OUTPUT_ANALOG);
 		intel_prepare_dp_ddi_buffers(encoder);
+		break;
 	}
 
 	/* Set the FDI_RX_MISC pwrdn lanes and the 2 workarounds listed at the
@@ -613,9 +627,7 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
 	I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
 
 	/* Configure Port Clock Select */
-	ddi_pll_sel = hsw_pll_to_ddi_pll_sel(intel_crtc->config->shared_dpll);
-	I915_WRITE(PORT_CLK_SEL(PORT_E), ddi_pll_sel);
-	WARN_ON(ddi_pll_sel != PORT_CLK_SEL_SPLL);
+	intel_dpll_map_to_encoder(intel_crtc->config->shared_dpll, encoder);
 
 	/* Start the training iterating through available voltages and emphasis,
 	 * testing each value twice. */
@@ -1607,33 +1619,6 @@ uint32_t ddi_signal_levels(struct intel_dp *intel_dp)
 	return DDI_BUF_TRANS_SELECT(level);
 }
 
-void intel_ddi_clk_select(struct intel_encoder *encoder,
-			  struct intel_shared_dpll *pll)
-{
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
-	enum port port = intel_ddi_get_encoder_port(encoder);
-
-	if (WARN_ON(!pll))
-		return;
-
-	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
-		uint32_t val;
-
-		/* DDI -> PLL mapping  */
-		val = I915_READ(DPLL_CTRL2);
-
-		val &= ~(DPLL_CTRL2_DDI_CLK_OFF(port) |
-			DPLL_CTRL2_DDI_CLK_SEL_MASK(port));
-		val |= (DPLL_CTRL2_DDI_CLK_SEL(pll->id, port) |
-			DPLL_CTRL2_DDI_SEL_OVERRIDE(port));
-
-		I915_WRITE(DPLL_CTRL2, val);
-
-	} else if (INTEL_INFO(dev_priv)->gen < 9) {
-		I915_WRITE(PORT_CLK_SEL(port), hsw_pll_to_ddi_pll_sel(pll));
-	}
-}
-
 static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 				    int link_rate, uint32_t lane_count,
 				    struct intel_shared_dpll *pll,
@@ -1648,7 +1633,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	if (encoder->type == INTEL_OUTPUT_EDP)
 		intel_edp_panel_on(intel_dp);
 
-	intel_ddi_clk_select(encoder, pll);
+	intel_dpll_map_to_encoder(pll, encoder);
 	intel_prepare_dp_ddi_buffers(encoder);
 	intel_ddi_init_dp_buf_reg(encoder);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
@@ -1669,7 +1654,7 @@ static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
 	int level = intel_ddi_hdmi_level(dev_priv, port);
 
 	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
-	intel_ddi_clk_select(encoder, pll);
+	intel_dpll_map_to_encoder(pll, encoder);
 	intel_prepare_hdmi_ddi_buffers(encoder);
 	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
 		skl_ddi_set_iboost(encoder, level);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8ecaf18..22e3c46 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4555,19 +4555,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
 
 	/* We need to program the right clock selection before writing the pixel
 	 * mutliplier into the DPLL. */
-	if (HAS_PCH_CPT(dev)) {
-		u32 sel;
-
-		temp = I915_READ(PCH_DPLL_SEL);
-		temp |= TRANS_DPLL_ENABLE(pipe);
-		sel = TRANS_DPLLB_SEL(pipe);
-		if (intel_crtc->config->shared_dpll ==
-		    intel_get_shared_dpll_by_id(dev_priv, DPLL_ID_PCH_PLL_B))
-			temp |= sel;
-		else
-			temp &= ~sel;
-		I915_WRITE(PCH_DPLL_SEL, temp);
-	}
+	intel_dpll_map_to_crtc(intel_crtc->config->shared_dpll, intel_crtc);
 
 	/* XXX: pch pll's can be enabled any time before we enable the PCH
 	 * transcoder, and we actually should do this to not upset any PCH
@@ -5574,9 +5562,7 @@ static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
 			I915_WRITE(reg, temp);
 
 			/* disable DPLL_SEL */
-			temp = I915_READ(PCH_DPLL_SEL);
-			temp &= ~(TRANS_DPLL_ENABLE(pipe) | TRANS_DPLLB_SEL(pipe));
-			I915_WRITE(PCH_DPLL_SEL, temp);
+			intel_dpll_map_to_crtc(NULL, intel_crtc);
 		}
 
 		ironlake_fdi_pll_disable(intel_crtc);
@@ -9952,7 +9938,6 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 
 	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
 		struct intel_shared_dpll *pll;
-		enum intel_dpll_id pll_id;
 
 		pipe_config->has_pch_encoder = true;
 
@@ -9962,23 +9947,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 
 		ironlake_get_fdi_m_n_config(crtc, pipe_config);
 
-		if (HAS_PCH_IBX(dev_priv)) {
-			/*
-			 * The pipe->pch transcoder and pch transcoder->pll
-			 * mapping is fixed.
-			 */
-			pll_id = (enum intel_dpll_id) crtc->pipe;
-		} else {
-			tmp = I915_READ(PCH_DPLL_SEL);
-			if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
-				pll_id = DPLL_ID_PCH_PLL_B;
-			else
-				pll_id= DPLL_ID_PCH_PLL_A;
-		}
-
-		pipe_config->shared_dpll =
-			intel_get_shared_dpll_by_id(dev_priv, pll_id);
-		pll = pipe_config->shared_dpll;
+		pipe_config->shared_dpll = pll = intel_get_crtc_dpll(crtc);
 
 		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
 						 &pipe_config->dpll_hw_state));
@@ -10458,82 +10427,6 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
 	return 0;
 }
 
-static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
-				enum port port,
-				struct intel_crtc_state *pipe_config)
-{
-	enum intel_dpll_id id;
-
-	switch (port) {
-	case PORT_A:
-		id = DPLL_ID_SKL_DPLL0;
-		break;
-	case PORT_B:
-		id = DPLL_ID_SKL_DPLL1;
-		break;
-	case PORT_C:
-		id = DPLL_ID_SKL_DPLL2;
-		break;
-	default:
-		DRM_ERROR("Incorrect port type\n");
-		return;
-	}
-
-	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
-}
-
-static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv,
-				enum port port,
-				struct intel_crtc_state *pipe_config)
-{
-	enum intel_dpll_id id;
-	u32 temp;
-
-	temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
-	id = temp >> (port * 3 + 1);
-
-	if (WARN_ON(id < SKL_DPLL0 || id > SKL_DPLL3))
-		return;
-
-	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
-}
-
-static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
-				enum port port,
-				struct intel_crtc_state *pipe_config)
-{
-	enum intel_dpll_id id;
-	uint32_t ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
-
-	switch (ddi_pll_sel) {
-	case PORT_CLK_SEL_WRPLL1:
-		id = DPLL_ID_WRPLL1;
-		break;
-	case PORT_CLK_SEL_WRPLL2:
-		id = DPLL_ID_WRPLL2;
-		break;
-	case PORT_CLK_SEL_SPLL:
-		id = DPLL_ID_SPLL;
-		break;
-	case PORT_CLK_SEL_LCPLL_810:
-		id = DPLL_ID_LCPLL_810;
-		break;
-	case PORT_CLK_SEL_LCPLL_1350:
-		id = DPLL_ID_LCPLL_1350;
-		break;
-	case PORT_CLK_SEL_LCPLL_2700:
-		id = DPLL_ID_LCPLL_2700;
-		break;
-	default:
-		MISSING_CASE(ddi_pll_sel);
-		/* fall through */
-	case PORT_CLK_SEL_NONE:
-		return;
-	}
-
-	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
-}
-
 static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
 				     struct intel_crtc_state *pipe_config,
 				     unsigned long *power_domain_mask)
@@ -10638,6 +10531,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_encoder *encoder;
 	struct intel_shared_dpll *pll;
 	enum port port;
 	uint32_t tmp;
@@ -10646,14 +10540,8 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
 
 	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
 
-	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
-		skylake_get_ddi_pll(dev_priv, port, pipe_config);
-	else if (IS_BROXTON(dev))
-		bxt_get_ddi_pll(dev_priv, port, pipe_config);
-	else
-		haswell_get_ddi_pll(dev_priv, port, pipe_config);
-
-	pll = pipe_config->shared_dpll;
+	encoder =  intel_ddi_get_port_encoder(dev_priv, port);
+	pipe_config->shared_dpll = pll = intel_get_encoder_dpll(encoder);
 	if (pll) {
 		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
 						 &pipe_config->dpll_hw_state));
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 3ffbd69..cb097fb 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -150,8 +150,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
 	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
 
 	if (intel_dp->active_mst_links == 0) {
-		intel_ddi_clk_select(&intel_dig_port->base,
-				     pipe_config->shared_dpll);
+		intel_dpll_map_to_encoder(pipe_config->shared_dpll,
+					  &intel_dig_port->base);
 
 		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
 		intel_dp_set_link_params(intel_dp,
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 9b02d9c..532236d 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -40,6 +40,12 @@
  * Changes to the users are first staged in the atomic state, and then made
  * effective by calling intel_shared_dpll_swap_state() during the atomic
  * commit phase.
+ *
+ * The functions intel_dpll_map_to_crtc(), intel_dpll_map_to_encoder(),
+ * intel_get_crtc_dpll() and intel_get_encoder_dpll() allows the mapping of
+ * dplls to crtcs and encoders to be queried and changed. Whether plls map
+ * to encoders or crtcs depends on the platform. The caller needs to know
+ * which of the functions to use.
  */
 
 struct intel_shared_dpll *
@@ -447,6 +453,54 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static struct intel_shared_dpll *
+ibx_get_crtc_dpll(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum intel_dpll_id pll_id;
+	u32 tmp;
+
+	if (HAS_PCH_IBX(dev_priv)) {
+		/*
+		 * The pipe->pch transcoder and pch transcoder->pll
+		 * mapping is fixed.
+		 */
+		pll_id = (enum intel_dpll_id) crtc->pipe;
+	} else {
+		tmp = I915_READ(PCH_DPLL_SEL);
+		if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
+			pll_id = DPLL_ID_PCH_PLL_B;
+		else
+			pll_id= DPLL_ID_PCH_PLL_A;
+	}
+
+	return intel_get_shared_dpll_by_id(dev_priv, pll_id);
+}
+
+static void
+ibx_dpll_map_to_crtc(struct intel_shared_dpll *dpll, struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	u32 temp, sel;
+
+	if (!HAS_PCH_CPT(dev_priv))
+		return;
+
+	temp = I915_READ(PCH_DPLL_SEL);
+	if (dpll) {
+		temp |= TRANS_DPLL_ENABLE(pipe);
+		sel = TRANS_DPLLB_SEL(pipe);
+		if (dpll->id ==  DPLL_ID_PCH_PLL_B)
+			temp |= sel;
+		else
+			temp &= ~sel;
+	} else {
+		temp &= ~(TRANS_DPLL_ENABLE(pipe) | TRANS_DPLLB_SEL(pipe));
+	}
+	I915_WRITE(PCH_DPLL_SEL, temp);
+}
+
 static void ibx_dump_hw_state(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state)
 {
@@ -844,6 +898,73 @@ hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static enum intel_dpll_id hsw_reg_to_pll_id(uint32_t ddi_pll_sel)
+{
+	switch (ddi_pll_sel) {
+	case PORT_CLK_SEL_WRPLL1:
+		return DPLL_ID_WRPLL1;
+	case PORT_CLK_SEL_WRPLL2:
+		return DPLL_ID_WRPLL2;
+	case PORT_CLK_SEL_SPLL:
+		return DPLL_ID_SPLL;
+	case PORT_CLK_SEL_LCPLL_810:
+		return DPLL_ID_LCPLL_810;
+	case PORT_CLK_SEL_LCPLL_1350:
+		return DPLL_ID_LCPLL_1350;
+	case PORT_CLK_SEL_LCPLL_2700:
+		return DPLL_ID_LCPLL_2700;
+	default:
+		MISSING_CASE(ddi_pll_sel);
+		/* fall through */
+	case PORT_CLK_SEL_NONE:
+		return DPLL_ID_PRIVATE;
+	}
+}
+
+static uint32_t hsw_pll_id_to_reg(enum intel_dpll_id id)
+{
+	switch (id) {
+	case DPLL_ID_WRPLL1:
+		return PORT_CLK_SEL_WRPLL1;
+	case DPLL_ID_WRPLL2:
+		return PORT_CLK_SEL_WRPLL2;
+	case DPLL_ID_SPLL:
+		return PORT_CLK_SEL_SPLL;
+	case DPLL_ID_LCPLL_810:
+		return PORT_CLK_SEL_LCPLL_810;
+	case DPLL_ID_LCPLL_1350:
+		return PORT_CLK_SEL_LCPLL_1350;
+	case DPLL_ID_LCPLL_2700:
+		return PORT_CLK_SEL_LCPLL_2700;
+	default:
+		MISSING_CASE(id);
+		/* fall through */
+	case DPLL_ID_PRIVATE:
+		return PORT_CLK_SEL_NONE;
+	}
+}
+
+static struct intel_shared_dpll *
+hsw_get_encoder_dpll(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = intel_ddi_get_encoder_port(encoder);
+	uint32_t ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
+
+	return intel_get_shared_dpll_by_id(dev_priv,
+					   hsw_reg_to_pll_id(ddi_pll_sel));
+}
+
+static void
+hsw_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
+			struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = intel_ddi_get_encoder_port(encoder);
+
+	I915_WRITE(PORT_CLK_SEL(port), hsw_pll_id_to_reg(dpll->id));
+}
+
 static void hsw_dump_hw_state(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state)
 {
@@ -1406,6 +1527,41 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
 	return pll;
 }
 
+static struct intel_shared_dpll *
+skl_get_encoder_dpll(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum intel_dpll_id id;
+	u32 temp;
+
+	temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
+	id = temp >> (port * 3 + 1);
+
+	if (WARN_ON(id < SKL_DPLL0 || id > SKL_DPLL3))
+		return NULL;
+
+	return intel_get_shared_dpll_by_id(dev_priv, id);
+}
+
+static void
+skl_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
+			struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = intel_ddi_get_encoder_port(encoder);
+	uint32_t val;
+
+	val = I915_READ(DPLL_CTRL2);
+
+	val &= ~(DPLL_CTRL2_DDI_CLK_OFF(port) |
+		DPLL_CTRL2_DDI_CLK_SEL_MASK(port));
+	val |= (DPLL_CTRL2_DDI_CLK_SEL(dpll->id, port) |
+		DPLL_CTRL2_DDI_SEL_OVERRIDE(port));
+
+	I915_WRITE(DPLL_CTRL2, val);
+}
+
 static void skl_dump_hw_state(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state)
 {
@@ -1812,6 +1968,38 @@ bxt_get_dpll(struct intel_crtc *crtc,
 
 	return pll;
 }
+static struct intel_shared_dpll *
+bxt_get_encoder_dpll(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = intel_ddi_get_encoder_port(encoder);
+	enum intel_dpll_id id;
+
+	switch (port) {
+	case PORT_A:
+		id = DPLL_ID_SKL_DPLL0;
+		break;
+	case PORT_B:
+		id = DPLL_ID_SKL_DPLL1;
+		break;
+	case PORT_C:
+		id = DPLL_ID_SKL_DPLL2;
+		break;
+	default:
+		DRM_ERROR("Incorrect port type\n");
+		return NULL;
+	}
+
+	return intel_get_shared_dpll_by_id(dev_priv, id);
+}
+
+static void
+bxt_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
+			struct intel_encoder *encoder)
+{
+	/* Fixed mapping, nothing to do */
+}
+
 
 static void bxt_dump_hw_state(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state)
@@ -1859,6 +2047,33 @@ static void intel_ddi_pll_init(struct drm_device *dev)
 	}
 }
 
+static void
+noop_dpll_map_to_crtc(struct intel_shared_dpll *dpll, struct intel_crtc *crtc)
+{
+	WARN(1, "Platform does not support mapping DPLLs to CRTCs.");
+}
+
+static struct intel_shared_dpll *
+noop_get_crtc_dpll(struct intel_crtc *crtc)
+{
+	WARN(1, "Platform does not support mapping DPLLs to CRTCs.");
+	return NULL;
+}
+
+static void
+noop_dpll_map_to_encoder(struct intel_shared_dpll *dpll, struct intel_encoder *encoder)
+{
+	WARN(1, "Platform does not support mapping DPLLs to encoders.");
+}
+
+static struct intel_shared_dpll *
+noop_get_encoder_dpll(struct intel_encoder *encoder)
+{
+	WARN(1, "Platform does not support mapping DPLLs to encoders.");
+	return NULL;
+}
+
+
 struct dpll_info {
 	const char *name;
 	const int id;
@@ -1873,6 +2088,14 @@ struct intel_dpll_mgr {
 					      struct intel_crtc_state *crtc_state,
 					      struct intel_encoder *encoder);
 
+	struct intel_shared_dpll *(*get_crtc_dpll)(struct intel_crtc *crtc);
+	struct intel_shared_dpll *(*get_encoder_dpll)(struct intel_encoder *encoder);
+
+	void (*map_to_crtc)(struct intel_shared_dpll *dpll,
+			    struct intel_crtc *crtc);
+	void (*map_to_encoder)(struct intel_shared_dpll *dpll,
+			       struct intel_encoder *encoder);
+
 	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state);
 };
@@ -1886,6 +2109,10 @@ static const struct dpll_info pch_plls[] = {
 static const struct intel_dpll_mgr pch_pll_mgr = {
 	.dpll_info = pch_plls,
 	.get_dpll = ibx_get_dpll,
+	.get_crtc_dpll = ibx_get_crtc_dpll,
+	.map_to_crtc = ibx_dpll_map_to_crtc,
+	.get_encoder_dpll = noop_get_encoder_dpll,
+	.map_to_encoder = noop_dpll_map_to_encoder,
 	.dump_hw_state = ibx_dump_hw_state,
 };
 
@@ -1902,6 +2129,10 @@ static const struct dpll_info hsw_plls[] = {
 static const struct intel_dpll_mgr hsw_pll_mgr = {
 	.dpll_info = hsw_plls,
 	.get_dpll = hsw_get_dpll,
+	.get_crtc_dpll = noop_get_crtc_dpll,
+	.map_to_crtc = noop_dpll_map_to_crtc,
+	.get_encoder_dpll = hsw_get_encoder_dpll,
+	.map_to_encoder = hsw_dpll_map_to_encoder,
 	.dump_hw_state = hsw_dump_hw_state,
 };
 
@@ -1916,6 +2147,10 @@ static const struct dpll_info skl_plls[] = {
 static const struct intel_dpll_mgr skl_pll_mgr = {
 	.dpll_info = skl_plls,
 	.get_dpll = skl_get_dpll,
+	.get_crtc_dpll = noop_get_crtc_dpll,
+	.map_to_crtc = noop_dpll_map_to_crtc,
+	.get_encoder_dpll = skl_get_encoder_dpll,
+	.map_to_encoder = skl_dpll_map_to_encoder,
 	.dump_hw_state = skl_dump_hw_state,
 };
 
@@ -1929,6 +2164,10 @@ static const struct dpll_info bxt_plls[] = {
 static const struct intel_dpll_mgr bxt_pll_mgr = {
 	.dpll_info = bxt_plls,
 	.get_dpll = bxt_get_dpll,
+	.get_crtc_dpll = noop_get_crtc_dpll,
+	.map_to_crtc = noop_dpll_map_to_crtc,
+	.get_encoder_dpll = bxt_get_encoder_dpll,
+	.map_to_encoder = bxt_dpll_map_to_encoder,
 	.dump_hw_state = bxt_dump_hw_state,
 };
 
@@ -2031,6 +2270,66 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
 }
 
 /**
+ * intel_get_crtc_dpll - Get which pll is mapped to a pipe
+ * @crtc: pipe
+ *
+ * Returns the DPLL is mapped to @crtc. NULL indicates no DPLL is mapped to
+ * @crtc.
+ */
+struct intel_shared_dpll *intel_get_crtc_dpll(struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	return dev_priv->dpll_mgr->get_crtc_dpll(crtc);
+}
+
+/**
+ * intel_get_encoder_dpll - Get which pll is mapped to an encoder
+ * @encoder: encoder
+ *
+ * Returns the DPLL is mapped to @encoder. NULL indicates no DPLL is mapped to
+ * @encoder.
+ */
+struct intel_shared_dpll *intel_get_encoder_dpll(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+
+	return dev_priv->dpll_mgr->get_encoder_dpll(encoder);
+}
+
+/**
+ * intel_dpll_map_to_crtc - Map DPLL to pipe
+ * @dpll: dpll to map
+ * @crtc: pipe to map the @dpll to
+ *
+ * For platforms where DPLLs are mapped to crtcs, program the appropriate
+ * bits so that @dpll is mapped to @crtc.
+ */
+void intel_dpll_map_to_crtc(struct intel_shared_dpll *dpll,
+			    struct intel_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+
+	dev_priv->dpll_mgr->map_to_crtc(dpll, crtc);
+}
+
+/**
+ * intel_dpll_map_to_encoder - Map DPLL to encoder
+ * @dpll: dpll to map
+ * @encoder: encoder to map the @dpll to
+ *
+ * For platforms where DPLLs are mapped to encoders, program the appropriate
+ * bits so that @dpll is mapped to @encoder.
+ */
+void intel_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
+			       struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+
+	dev_priv->dpll_mgr->map_to_encoder(dpll, encoder);
+}
+
+/**
  * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
  * @dev_priv: i915 drm device
  * @hw_state: hw state to be written to the log
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index 76111a4..7b46c88 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -280,6 +280,13 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
 void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
 void intel_shared_dpll_init(struct drm_device *dev);
 
+struct intel_shared_dpll *intel_get_crtc_dpll(struct intel_crtc *crtc);
+struct intel_shared_dpll *intel_get_encoder_dpll(struct intel_encoder *encoder);
+void intel_dpll_map_to_crtc(struct intel_shared_dpll *dpll,
+			    struct intel_crtc *crtc);
+void intel_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
+			       struct intel_encoder *encoder);
+
 void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
 			      struct intel_dpll_hw_state *hw_state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7874f66..a04d917 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1136,8 +1136,6 @@ void intel_crt_init(struct drm_device *dev);
 void intel_crt_reset(struct drm_encoder *encoder);
 
 /* intel_ddi.c */
-void intel_ddi_clk_select(struct intel_encoder *encoder,
-			  struct intel_shared_dpll *pll);
 void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder,
 				struct intel_crtc_state *old_crtc_state,
 				struct drm_connector_state *old_conn_state);
@@ -1145,6 +1143,8 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
 void hsw_fdi_link_train(struct drm_crtc *crtc);
 void intel_ddi_init(struct drm_device *dev, enum port port);
 enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
+struct intel_encoder *
+intel_ddi_get_port_encoder(struct drm_i915_private *dev_priv, enum port port);
 bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
 void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
 void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
-- 
2.5.5

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

✗ Fi.CI.BAT: warning for Shared DPLL kernel doc and improvements

[Patchwork]

== Series Details ==

Series: Shared DPLL kernel doc and improvements
URL   : https://patchwork.freedesktop.org/series/13277/
State : warning

== Summary ==

Series 13277v1 Shared DPLL kernel doc and improvements
https://patchwork.freedesktop.org/api/1.0/series/13277/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (fi-skl-6770hq)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-byt-j1900)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-byt-j1900)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:244  pass:214  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:244  pass:211  dwarn:1   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:244  pass:208  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:221  dwarn:1   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:218  dwarn:2   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:227  dwarn:2   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2621/

cedbdecff4c878309133e066f696ea63d41cee73 drm-intel-nightly: 2016y-10m-04d-10h-53m-20s UTC integration manifest
1ffe86b drm/i915: Add entrypoints for mapping dplls to encoders and crtcs
1d399a4 drm/i915: Add dpll entrypoint for dumping hw state
60e1afa drm/i915: Update kerneldoc for intel_dpll_mgr.c
d45c74f drm/i915: Rename intel_shared_dpll->mode_set() to prepare()
33dd291 drm/i915: Rename intel_shared_dpll_config to intel_shared_dpll_state
bb39d86 drm/i915: Rename intel_shared_dpll_commit() to _swap_state()
ddec815 drm/i915: Introduce intel_release_shared_dpll()

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

Re: [PATCH 1/7] drm/i915: Introduce intel_release_shared_dpll()

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:11PM +0300, Ander Conselvan de Oliveira wrote:
> While the details of getting a shared dpll are wrapped by
> intel_get_shared_dpll(), the release was still hand rolled into the
> modeset code. Fix that by creating an entry point for releasing the
> pll and move that code there.
> 
> v2: Take old_dpll from crtc->state instead of crtc_state. (CI)
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c  |  6 +----
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 41 +++++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 11 +++-------
>  3 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a366656..28d9d3e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13768,7 +13768,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_shared_dpll_config *shared_dpll = NULL;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int i;
> @@ -13789,10 +13788,7 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
>  		if (!old_dpll)
>  			continue;
>  
> -		if (!shared_dpll)
> -			shared_dpll = intel_atomic_get_shared_dpll_state(state);
> -
> -		intel_shared_dpll_config_put(shared_dpll, old_dpll, intel_crtc);
> +		intel_release_shared_dpll(old_dpll, intel_crtc, state);
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 1c59ca5..f1b3feb 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -79,28 +79,6 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
>  	return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
>  }
>  
> -void
> -intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
> -
> -	config[id].crtc_mask |= 1 << crtc->pipe;
> -}
> -
> -void
> -intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
> -
> -	config[id].crtc_mask &= ~(1 << crtc->pipe);
> -}
> -
>  /* For ILK+ */
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			struct intel_shared_dpll *pll,
> @@ -285,7 +263,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>  			 pipe_name(crtc->pipe));
>  
> -	intel_shared_dpll_config_get(shared_dpll, pll, crtc);
> +	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
>  }
>  
>  void intel_shared_dpll_commit(struct drm_atomic_state *state)
> @@ -1900,3 +1878,20 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
>  
>  	return dpll_mgr->get_dpll(crtc, crtc_state, encoder);
>  }
> +
> +/**
> + * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state
> + * @dpll: dpll in use by @crtc
> + * @crtc: crtc
> + * @state: atomic state
> + *
> + */
> +void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> +			       struct intel_crtc *crtc,
> +			       struct drm_atomic_state *state)
> +{
> +	struct intel_shared_dpll_config *shared_dpll_config;
> +
> +	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);

I wonder whether we shouldn't move intel_atomic_get_shared_dpll_state into
intel_dpll_mgr.c too. That would further reduce the interfaces exported
from intel_dpll_mgr.[hc].

On this patch: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> +	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index f438535..99a82c9 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -138,14 +138,6 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
>  enum intel_dpll_id
>  intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
>  			 struct intel_shared_dpll *pll);
> -void
> -intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc);
> -void
> -intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
> -			     struct intel_shared_dpll *pll,
> -			     struct intel_crtc *crtc);
>  void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			struct intel_shared_dpll *pll,
>  			bool state);
> @@ -154,6 +146,9 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
>  						struct intel_crtc_state *state,
>  						struct intel_encoder *encoder);
> +void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> +			       struct intel_crtc *crtc,
> +			       struct drm_atomic_state *state);
>  void intel_prepare_shared_dpll(struct intel_crtc *crtc);
>  void intel_enable_shared_dpll(struct intel_crtc *crtc);
>  void intel_disable_shared_dpll(struct intel_crtc *crtc);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Rename intel_shared_dpll_commit() to _swap_state()

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:12PM +0300, Ander Conselvan de Oliveira wrote:
> The function intel_shared_dpll_commit() performs the equivalent of
> drm_atomic_helper_swap_state() for the shared dpll state, which is not
> handled by the helpers. So rename it for consistency.
> 
> v2: Fix typo in the commit message. (Durga)
> v3: Rebase.
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c  | 2 +-
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 2 +-
>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 28d9d3e..b8bfde0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14530,7 +14530,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	drm_atomic_helper_swap_state(state, true);
>  	dev_priv->wm.distrust_bios_wm = false;
>  	dev_priv->wm.skl_results = intel_state->wm_results;
> -	intel_shared_dpll_commit(state);
> +	intel_shared_dpll_swap_state(state);
>  	intel_atomic_track_fbs(state);
>  
>  	if (nonblock)
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index f1b3feb..15bf462 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -266,7 +266,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
>  }
>  
> -void intel_shared_dpll_commit(struct drm_atomic_state *state)
> +void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
>  	struct intel_shared_dpll_config *shared_dpll;
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 99a82c9..06d61c5 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -152,7 +152,7 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
>  void intel_prepare_shared_dpll(struct intel_crtc *crtc);
>  void intel_enable_shared_dpll(struct intel_crtc *crtc);
>  void intel_disable_shared_dpll(struct intel_crtc *crtc);
> -void intel_shared_dpll_commit(struct drm_atomic_state *state);
> +void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
>  void intel_shared_dpll_init(struct drm_device *dev);
>  
>  /* BXT dpll related functions */
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Rename intel_shared_dpll_config to intel_shared_dpll_state

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:13PM +0300, Ander Conselvan de Oliveira wrote:
> Struct intel_shared_dpll_config is used to hold the state of the DPLL in
> the "atomic" sense, so call it state like everything else atomic.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   | 12 +++----
>  drivers/gpu/drm/i915/intel_atomic.c   |  6 ++--
>  drivers/gpu/drm/i915/intel_ddi.c      | 10 +++---
>  drivers/gpu/drm/i915/intel_display.c  | 22 ++++++------
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 68 +++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  4 +--
>  drivers/gpu/drm/i915/intel_drv.h      |  4 +--
>  7 files changed, 63 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index da71413..12b97af 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3229,14 +3229,14 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>  
>  		seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->name, pll->id);
>  		seq_printf(m, " crtc_mask: 0x%08x, active: 0x%x, on: %s\n",
> -			   pll->config.crtc_mask, pll->active_mask, yesno(pll->on));
> +			   pll->state.crtc_mask, pll->active_mask, yesno(pll->on));
>  		seq_printf(m, " tracked hardware state:\n");
> -		seq_printf(m, " dpll:    0x%08x\n", pll->config.hw_state.dpll);
> +		seq_printf(m, " dpll:    0x%08x\n", pll->state.hw_state.dpll);
>  		seq_printf(m, " dpll_md: 0x%08x\n",
> -			   pll->config.hw_state.dpll_md);
> -		seq_printf(m, " fp0:     0x%08x\n", pll->config.hw_state.fp0);
> -		seq_printf(m, " fp1:     0x%08x\n", pll->config.hw_state.fp1);
> -		seq_printf(m, " wrpll:   0x%08x\n", pll->config.hw_state.wrpll);
> +			   pll->state.hw_state.dpll_md);
> +		seq_printf(m, " fp0:     0x%08x\n", pll->state.hw_state.fp0);
> +		seq_printf(m, " fp1:     0x%08x\n", pll->state.hw_state.fp1);
> +		seq_printf(m, " wrpll:   0x%08x\n", pll->state.hw_state.wrpll);
>  	}
>  	drm_modeset_unlock_all(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index c5a1667..fa6dc43 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -267,7 +267,7 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  
>  static void
>  intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
> -				  struct intel_shared_dpll_config *shared_dpll)
> +				  struct intel_shared_dpll_state *shared_dpll)
>  {
>  	enum intel_dpll_id i;
>  
> @@ -275,11 +275,11 @@ intel_atomic_duplicate_dpll_state(struct drm_i915_private *dev_priv,
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>  
> -		shared_dpll[i] = pll->config;
> +		shared_dpll[i] = pll->state;
>  	}
>  }
>  
> -struct intel_shared_dpll_config *
> +struct intel_shared_dpll_state *
>  intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s)
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(s);
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 35f0b7c..4077205 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -992,7 +992,7 @@ static int bxt_calc_pll_link(struct drm_i915_private *dev_priv,
>  		return 0;
>  
>  	pll = &dev_priv->shared_dplls[dpll];
> -	state = &pll->config.hw_state;
> +	state = &pll->state.hw_state;
>  
>  	clock.m1 = 2;
>  	clock.m2 = (state->pll0 & PORT_PLL_M2_MASK) << 22;
> @@ -2401,7 +2401,7 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_shared_dpll *pll = NULL;
> -	struct intel_shared_dpll_config tmp_pll_config;
> +	struct intel_shared_dpll_state tmp_pll_state;
>  	enum intel_dpll_id dpll_id;
>  
>  	if (IS_BROXTON(dev_priv)) {
> @@ -2417,11 +2417,11 @@ intel_ddi_get_link_dpll(struct intel_dp *intel_dp, int clock)
>  				  pll->active_mask);
>  			return NULL;
>  		}
> -		tmp_pll_config = pll->config;
> +		tmp_pll_state = pll->state;
>  		if (!bxt_ddi_dp_set_dpll_hw_state(clock,
> -						  &pll->config.hw_state)) {
> +						  &pll->state.hw_state)) {
>  			DRM_ERROR("Could not setup DPLL\n");
> -			pll->config = tmp_pll_config;
> +			pll->state = tmp_pll_state;
>  			return NULL;
>  		}
>  	} else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b8bfde0..e6fe85b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13639,9 +13639,9 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
>  	}
>  
>  	if (!crtc) {
> -		I915_STATE_WARN(pll->active_mask & ~pll->config.crtc_mask,
> +		I915_STATE_WARN(pll->active_mask & ~pll->state.crtc_mask,
>  				"more active pll users than references: %x vs %x\n",
> -				pll->active_mask, pll->config.crtc_mask);
> +				pll->active_mask, pll->state.crtc_mask);
>  
>  		return;
>  	}
> @@ -13657,11 +13657,11 @@ verify_single_dpll_state(struct drm_i915_private *dev_priv,
>  				"pll active mismatch (didn't expect pipe %c in active mask 0x%02x)\n",
>  				pipe_name(drm_crtc_index(crtc)), pll->active_mask);
>  
> -	I915_STATE_WARN(!(pll->config.crtc_mask & crtc_mask),
> +	I915_STATE_WARN(!(pll->state.crtc_mask & crtc_mask),
>  			"pll enabled crtcs mismatch (expected 0x%x in 0x%02x)\n",
> -			crtc_mask, pll->config.crtc_mask);
> +			crtc_mask, pll->state.crtc_mask);
>  
> -	I915_STATE_WARN(pll->on && memcmp(&pll->config.hw_state,
> +	I915_STATE_WARN(pll->on && memcmp(&pll->state.hw_state,
>  					  &dpll_hw_state,
>  					  sizeof(dpll_hw_state)),
>  			"pll hw state mismatch\n");
> @@ -13687,7 +13687,7 @@ verify_shared_dpll_state(struct drm_device *dev, struct drm_crtc *crtc,
>  		I915_STATE_WARN(pll->active_mask & crtc_mask,
>  				"pll active mismatch (didn't expect pipe %c in active mask)\n",
>  				pipe_name(drm_crtc_index(crtc)));
> -		I915_STATE_WARN(pll->config.crtc_mask & crtc_mask,
> +		I915_STATE_WARN(pll->state.crtc_mask & crtc_mask,
>  				"pll enabled crtcs mismatch (found %x in enabled mask)\n",
>  				pipe_name(drm_crtc_index(crtc)));
>  	}
> @@ -16759,16 +16759,16 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  		struct intel_shared_dpll *pll = &dev_priv->shared_dplls[i];
>  
>  		pll->on = pll->funcs.get_hw_state(dev_priv, pll,
> -						  &pll->config.hw_state);
> -		pll->config.crtc_mask = 0;
> +						  &pll->state.hw_state);
> +		pll->state.crtc_mask = 0;
>  		for_each_intel_crtc(dev, crtc) {
>  			if (crtc->active && crtc->config->shared_dpll == pll)
> -				pll->config.crtc_mask |= 1 << crtc->pipe;
> +				pll->state.crtc_mask |= 1 << crtc->pipe;
>  		}
> -		pll->active_mask = pll->config.crtc_mask;
> +		pll->active_mask = pll->state.crtc_mask;
>  
>  		DRM_DEBUG_KMS("%s hw state readout: crtc_mask 0x%08x, on %i\n",
> -			      pll->name, pll->config.crtc_mask, pll->on);
> +			      pll->name, pll->state.crtc_mask, pll->on);
>  	}
>  
>  	for_each_intel_encoder(dev, encoder) {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 15bf462..c88fc07 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -38,11 +38,11 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
>  		pll = &dev_priv->shared_dplls[i];
>  
>  		/* Only want to check enabled timings first */
> -		if (pll->config.crtc_mask == 0)
> +		if (pll->state.crtc_mask == 0)
>  			continue;
>  
> -		if (memcmp(&dpll_hw_state, &pll->config.hw_state,
> -			   sizeof(pll->config.hw_state)) == 0) {
> +		if (memcmp(&dpll_hw_state, &pll->state.hw_state,
> +			   sizeof(pll->state.hw_state)) == 0) {
>  			found = true;
>  			break;
>  		}
> @@ -52,8 +52,8 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
>  	for (i = DPLL_ID_SKL_DPLL1;
>  	     ((found == false) && (i <= DPLL_ID_SKL_DPLL3)); i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		if (pll->config.crtc_mask == 0) {
> -			pll->config.hw_state = dpll_hw_state;
> +		if (pll->state.crtc_mask == 0) {
> +			pll->state.hw_state = dpll_hw_state;
>  			break;
>  		}
>  	}
> @@ -106,7 +106,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  		return;
>  
>  	mutex_lock(&dev_priv->dpll_lock);
> -	WARN_ON(!pll->config.crtc_mask);
> +	WARN_ON(!pll->state.crtc_mask);
>  	if (!pll->active_mask) {
>  		DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
>  		WARN_ON(pll->on);
> @@ -139,7 +139,7 @@ void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	mutex_lock(&dev_priv->dpll_lock);
>  	old_mask = pll->active_mask;
>  
> -	if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)) ||
> +	if (WARN_ON(!(pll->state.crtc_mask & crtc_mask)) ||
>  	    WARN_ON(pll->active_mask & crtc_mask))
>  		goto out;
>  
> @@ -209,7 +209,7 @@ intel_find_shared_dpll(struct intel_crtc *crtc,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	struct intel_shared_dpll *pll;
> -	struct intel_shared_dpll_config *shared_dpll;
> +	struct intel_shared_dpll_state *shared_dpll;
>  	enum intel_dpll_id i;
>  
>  	shared_dpll = intel_atomic_get_shared_dpll_state(crtc_state->base.state);
> @@ -249,7 +249,7 @@ static void
>  intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  			    struct intel_crtc_state *crtc_state)
>  {
> -	struct intel_shared_dpll_config *shared_dpll;
> +	struct intel_shared_dpll_state *shared_dpll;
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	enum intel_dpll_id i = pll->id;
>  
> @@ -269,7 +269,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> -	struct intel_shared_dpll_config *shared_dpll;
> +	struct intel_shared_dpll_state *shared_dpll;
>  	struct intel_shared_dpll *pll;
>  	enum intel_dpll_id i;
>  
> @@ -279,7 +279,7 @@ void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
>  	shared_dpll = to_intel_atomic_state(state)->shared_dpll;
>  	for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>  		pll = &dev_priv->shared_dplls[i];
> -		pll->config = shared_dpll[i];
> +		pll->state = shared_dpll[i];
>  	}
>  }
>  
> @@ -305,8 +305,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
>  				  struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(PCH_FP0(pll->id), pll->config.hw_state.fp0);
> -	I915_WRITE(PCH_FP1(pll->id), pll->config.hw_state.fp1);
> +	I915_WRITE(PCH_FP0(pll->id), pll->state.hw_state.fp0);
> +	I915_WRITE(PCH_FP1(pll->id), pll->state.hw_state.fp1);
>  }
>  
>  static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
> @@ -328,7 +328,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	/* PCH refclock must be enabled first */
>  	ibx_assert_pch_refclk_enabled(dev_priv);
>  
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), pll->state.hw_state.dpll);
>  
>  	/* Wait for the clocks to stabilize. */
>  	POSTING_READ(PCH_DPLL(pll->id));
> @@ -339,7 +339,7 @@ static void ibx_pch_dpll_enable(struct drm_i915_private *dev_priv,
>  	 *
>  	 * So write it again.
>  	 */
> -	I915_WRITE(PCH_DPLL(pll->id), pll->config.hw_state.dpll);
> +	I915_WRITE(PCH_DPLL(pll->id), pll->state.hw_state.dpll);
>  	POSTING_READ(PCH_DPLL(pll->id));
>  	udelay(200);
>  }
> @@ -401,7 +401,7 @@ static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
>  static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
>  			       struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(WRPLL_CTL(pll->id), pll->config.hw_state.wrpll);
> +	I915_WRITE(WRPLL_CTL(pll->id), pll->state.hw_state.wrpll);
>  	POSTING_READ(WRPLL_CTL(pll->id));
>  	udelay(20);
>  }
> @@ -409,7 +409,7 @@ static void hsw_ddi_wrpll_enable(struct drm_i915_private *dev_priv,
>  static void hsw_ddi_spll_enable(struct drm_i915_private *dev_priv,
>  				struct intel_shared_dpll *pll)
>  {
> -	I915_WRITE(SPLL_CTL, pll->config.hw_state.spll);
> +	I915_WRITE(SPLL_CTL, pll->state.hw_state.spll);
>  	POSTING_READ(SPLL_CTL);
>  	udelay(20);
>  }
> @@ -852,7 +852,7 @@ static void skl_ddi_pll_write_ctrl1(struct drm_i915_private *dev_priv,
>  
>  	val &= ~(DPLL_CTRL1_HDMI_MODE(pll->id) | DPLL_CTRL1_SSC(pll->id) |
>  		 DPLL_CTRL1_LINK_RATE_MASK(pll->id));
> -	val |= pll->config.hw_state.ctrl1 << (pll->id * 6);
> +	val |= pll->state.hw_state.ctrl1 << (pll->id * 6);
>  
>  	I915_WRITE(DPLL_CTRL1, val);
>  	POSTING_READ(DPLL_CTRL1);
> @@ -865,8 +865,8 @@ static void skl_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  
>  	skl_ddi_pll_write_ctrl1(dev_priv, pll);
>  
> -	I915_WRITE(regs[pll->id].cfgcr1, pll->config.hw_state.cfgcr1);
> -	I915_WRITE(regs[pll->id].cfgcr2, pll->config.hw_state.cfgcr2);
> +	I915_WRITE(regs[pll->id].cfgcr1, pll->state.hw_state.cfgcr1);
> +	I915_WRITE(regs[pll->id].cfgcr2, pll->state.hw_state.cfgcr2);
>  	POSTING_READ(regs[pll->id].cfgcr1);
>  	POSTING_READ(regs[pll->id].cfgcr2);
>  
> @@ -1363,31 +1363,31 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	/* Write P1 & P2 */
>  	temp = I915_READ(BXT_PORT_PLL_EBB_0(port));
>  	temp &= ~(PORT_PLL_P1_MASK | PORT_PLL_P2_MASK);
> -	temp |= pll->config.hw_state.ebb0;
> +	temp |= pll->state.hw_state.ebb0;
>  	I915_WRITE(BXT_PORT_PLL_EBB_0(port), temp);
>  
>  	/* Write M2 integer */
>  	temp = I915_READ(BXT_PORT_PLL(port, 0));
>  	temp &= ~PORT_PLL_M2_MASK;
> -	temp |= pll->config.hw_state.pll0;
> +	temp |= pll->state.hw_state.pll0;
>  	I915_WRITE(BXT_PORT_PLL(port, 0), temp);
>  
>  	/* Write N */
>  	temp = I915_READ(BXT_PORT_PLL(port, 1));
>  	temp &= ~PORT_PLL_N_MASK;
> -	temp |= pll->config.hw_state.pll1;
> +	temp |= pll->state.hw_state.pll1;
>  	I915_WRITE(BXT_PORT_PLL(port, 1), temp);
>  
>  	/* Write M2 fraction */
>  	temp = I915_READ(BXT_PORT_PLL(port, 2));
>  	temp &= ~PORT_PLL_M2_FRAC_MASK;
> -	temp |= pll->config.hw_state.pll2;
> +	temp |= pll->state.hw_state.pll2;
>  	I915_WRITE(BXT_PORT_PLL(port, 2), temp);
>  
>  	/* Write M2 fraction enable */
>  	temp = I915_READ(BXT_PORT_PLL(port, 3));
>  	temp &= ~PORT_PLL_M2_FRAC_ENABLE;
> -	temp |= pll->config.hw_state.pll3;
> +	temp |= pll->state.hw_state.pll3;
>  	I915_WRITE(BXT_PORT_PLL(port, 3), temp);
>  
>  	/* Write coeff */
> @@ -1395,24 +1395,24 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	temp &= ~PORT_PLL_PROP_COEFF_MASK;
>  	temp &= ~PORT_PLL_INT_COEFF_MASK;
>  	temp &= ~PORT_PLL_GAIN_CTL_MASK;
> -	temp |= pll->config.hw_state.pll6;
> +	temp |= pll->state.hw_state.pll6;
>  	I915_WRITE(BXT_PORT_PLL(port, 6), temp);
>  
>  	/* Write calibration val */
>  	temp = I915_READ(BXT_PORT_PLL(port, 8));
>  	temp &= ~PORT_PLL_TARGET_CNT_MASK;
> -	temp |= pll->config.hw_state.pll8;
> +	temp |= pll->state.hw_state.pll8;
>  	I915_WRITE(BXT_PORT_PLL(port, 8), temp);
>  
>  	temp = I915_READ(BXT_PORT_PLL(port, 9));
>  	temp &= ~PORT_PLL_LOCK_THRESHOLD_MASK;
> -	temp |= pll->config.hw_state.pll9;
> +	temp |= pll->state.hw_state.pll9;
>  	I915_WRITE(BXT_PORT_PLL(port, 9), temp);
>  
>  	temp = I915_READ(BXT_PORT_PLL(port, 10));
>  	temp &= ~PORT_PLL_DCO_AMP_OVR_EN_H;
>  	temp &= ~PORT_PLL_DCO_AMP_MASK;
> -	temp |= pll->config.hw_state.pll10;
> +	temp |= pll->state.hw_state.pll10;
>  	I915_WRITE(BXT_PORT_PLL(port, 10), temp);
>  
>  	/* Recalibrate with new settings */
> @@ -1420,7 +1420,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	temp |= PORT_PLL_RECALIBRATE;
>  	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
>  	temp &= ~PORT_PLL_10BIT_CLK_ENABLE;
> -	temp |= pll->config.hw_state.ebb4;
> +	temp |= pll->state.hw_state.ebb4;
>  	I915_WRITE(BXT_PORT_PLL_EBB_4(port), temp);
>  
>  	/* Enable PLL */
> @@ -1440,7 +1440,7 @@ static void bxt_ddi_pll_enable(struct drm_i915_private *dev_priv,
>  	temp = I915_READ(BXT_PORT_PCS_DW12_LN01(port));
>  	temp &= ~LANE_STAGGER_MASK;
>  	temp &= ~LANESTAGGER_STRAP_OVRD;
> -	temp |= pll->config.hw_state.pcsdw12;
> +	temp |= pll->state.hw_state.pcsdw12;
>  	I915_WRITE(BXT_PORT_PCS_DW12_GRP(port), temp);
>  }
>  
> @@ -1890,8 +1890,8 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
>  			       struct intel_crtc *crtc,
>  			       struct drm_atomic_state *state)
>  {
> -	struct intel_shared_dpll_config *shared_dpll_config;
> +	struct intel_shared_dpll_state *shared_dpll_state;
>  
> -	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);
> -	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
> +	shared_dpll_state = intel_atomic_get_shared_dpll_state(state);
> +	shared_dpll_state[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 06d61c5..6e3a0f1 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -93,7 +93,7 @@ struct intel_dpll_hw_state {
>  		 pcsdw12;
>  };
>  
> -struct intel_shared_dpll_config {
> +struct intel_shared_dpll_state {
>  	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
>  	struct intel_dpll_hw_state hw_state;
>  };
> @@ -113,7 +113,7 @@ struct intel_shared_dpll_funcs {
>  };
>  
>  struct intel_shared_dpll {
> -	struct intel_shared_dpll_config config;
> +	struct intel_shared_dpll_state state;
>  
>  	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
>  	bool on; /* is the PLL actually active? Disabled during modeset */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f48e79a..7874f66 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -355,7 +355,7 @@ struct intel_atomic_state {
>  	/* SKL/KBL Only */
>  	unsigned int cdclk_pll_vco;
>  
> -	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> +	struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
>  
>  	/*
>  	 * Current watermarks can't be trusted during hardware readout, so
> @@ -1798,7 +1798,7 @@ void intel_crtc_destroy_state(struct drm_crtc *crtc,
>  			       struct drm_crtc_state *state);
>  struct drm_atomic_state *intel_atomic_state_alloc(struct drm_device *dev);
>  void intel_atomic_state_clear(struct drm_atomic_state *);
> -struct intel_shared_dpll_config *
> +struct intel_shared_dpll_state *
>  intel_atomic_get_shared_dpll_state(struct drm_atomic_state *s);
>  
>  static inline struct intel_crtc_state *
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915: Rename intel_shared_dpll->mode_set() to prepare()

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:14PM +0300, Ander Conselvan de Oliveira wrote:
> The hook is called from intel_prepare_shared_dpll(). The name doesn't
> make sense after all the changes to modeset code. So just call it
> prepare.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 8 ++++----
>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index c88fc07..9446446 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -112,7 +112,7 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  		WARN_ON(pll->on);
>  		assert_shared_dpll_disabled(dev_priv, pll);
>  
> -		pll->funcs.mode_set(dev_priv, pll);
> +		pll->funcs.prepare(dev_priv, pll);
>  	}
>  	mutex_unlock(&dev_priv->dpll_lock);
>  }
> @@ -302,8 +302,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  	return val & DPLL_VCO_ENABLE;
>  }
>  
> -static void ibx_pch_dpll_mode_set(struct drm_i915_private *dev_priv,
> -				  struct intel_shared_dpll *pll)
> +static void ibx_pch_dpll_prepare(struct drm_i915_private *dev_priv,
> +				 struct intel_shared_dpll *pll)
>  {
>  	I915_WRITE(PCH_FP0(pll->id), pll->state.hw_state.fp0);
>  	I915_WRITE(PCH_FP1(pll->id), pll->state.hw_state.fp1);
> @@ -392,7 +392,7 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  }
>  
>  static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
> -	.mode_set = ibx_pch_dpll_mode_set,
> +	.prepare = ibx_pch_dpll_prepare,
>  	.enable = ibx_pch_dpll_enable,
>  	.disable = ibx_pch_dpll_disable,
>  	.get_hw_state = ibx_pch_dpll_get_hw_state,
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 6e3a0f1..9a7db65 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -101,7 +101,7 @@ struct intel_shared_dpll_state {
>  struct intel_shared_dpll_funcs {
>  	/* The mode_set hook is optional and should be used together with the
>  	 * intel_prepare_shared_dpll function. */
> -	void (*mode_set)(struct drm_i915_private *dev_priv,
> +	void (*prepare)(struct drm_i915_private *dev_priv,
>  			 struct intel_shared_dpll *pll);
>  	void (*enable)(struct drm_i915_private *dev_priv,
>  		       struct intel_shared_dpll *pll);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:15PM +0300, Ander Conselvan de Oliveira wrote:
> The documentation for most of the non-static members and structs were
> missing. Fix that.
> 
> v2: Fix typos (Durga)
> 
> v3: Rebase.
>     Fix make docs warnings.
>     Document more.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

As mentioned in an earlier patch, I think we should also move
intel_atomic_get_shared_dpll_state from intel_atomic.c into
intel_dpll_mgr.c. Grouping functions by the data structures they touch
makes imo much more sense, than grouping them by topic. That way all the
dpll stuff is in one place.

> ---
>  Documentation/gpu/i915.rst            |  12 +++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c |  90 ++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_dpll_mgr.h | 155 ++++++++++++++++++++++++++++++----
>  3 files changed, 237 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> index 87aaffc..c19e437 100644
> --- a/Documentation/gpu/i915.rst
> +++ b/Documentation/gpu/i915.rst
> @@ -204,6 +204,18 @@ Video BIOS Table (VBT)
>  .. kernel-doc:: drivers/gpu/drm/i915/intel_vbt_defs.h
>     :internal:
>  
> +Display PLLs
> +------------
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> +   :doc: Display PLLs
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.h
> +   :internal:
> +
>  Memory Management and Command Submission
>  ========================================
>  
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 9446446..8c4efa9 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -23,6 +23,25 @@
>  
>  #include "intel_drv.h"
>  
> +/**
> + * DOC: Display PLLs
> + *
> + * Display PLLs used for driving outputs vary by platform. While some have
> + * per-pipe or per-encoder dedicated PLLs, others allow the use of any PLL
> + * from a pool. In the latter scenario, it is possible that multiple pipes
> + * share a PLL if their configurations match.
> + *
> + * This file provides an abstraction over display PLLs. The function
> + * intel_shared_dpll_init() initializes the PLLs for the given platform.  The
> + * users of a PLL are tracked and that tracking is integrated with the atomic
> + * modest interface. During an atomic operation, a PLL can be requested for a
> + * given crtc and encoder configuration by calling intel_get_shared_dpll() and

s/crtc/CRTC/ for consistency (abbreviations are all upercase), pls do that
on the entire doc.

> + * a previously used PLL can be released with intel_release_shared_dpll().
> + * Changes to the users are first staged in the atomic state, and then made
> + * effective by calling intel_shared_dpll_swap_state() during the atomic
> + * commit phase.
> + */
> +
>  struct intel_shared_dpll *
>  skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
>  {
> @@ -61,6 +80,14 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
>  	return pll;
>  }
>  
> +/**
> + * intel_get_shared_dpll_by_id - get a DPLL given its id
> + * @dev_priv: i915 device instance
> + * @id: pll id
> + *
> + * Returns:
> + * A pointer to the DPLL with @id
> + */
>  struct intel_shared_dpll *
>  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
>  			    enum intel_dpll_id id)
> @@ -68,6 +95,14 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
>  	return &dev_priv->shared_dplls[id];
>  }
>  
> +/**
> + * intel_get_shared_dpll_id - get the id of a DPLL
> + * @dev_priv: i915 device instance
> + * @pll: the DPLL
> + *
> + * Returns:
> + * The id of @pll
> + */
>  enum intel_dpll_id
>  intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
>  			 struct intel_shared_dpll *pll)
> @@ -96,6 +131,13 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
>  			pll->name, onoff(state), onoff(cur_state));
>  }
>  
> +/**
> + * intel_prepare_shared_dpll - call a dpll's prepare hook
> + * @crtc: crtc which has a shared dpll
> + *
> + * This calls the PLL's prepare hook if it has one and if the PLL is not
> + * already enabled. The prepare hook is platform specific.
> + */
>  void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -118,12 +160,10 @@ void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>  }
>  
>  /**
> - * intel_enable_shared_dpll - enable PCH PLL
> - * @dev_priv: i915 private structure
> - * @pipe: pipe PLL to enable
> + * intel_enable_shared_dpll - enable a crtc's shared DPLL
> + * @crtc: crtc which has a shared DPLL
>   *
> - * The PCH PLL needs to be enabled before the PCH transcoder, since it
> - * drives the transcoder clock.
> + * Enable the shared DPLL used by @crtc.
>   */
>  void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  {
> @@ -164,6 +204,12 @@ out:
>  	mutex_unlock(&dev_priv->dpll_lock);
>  }
>  
> +/**
> + * intel_disable_shared_dpll - disable a crtc's shared DPLL
> + * @crtc: crtc which has a shared DPLL
> + *
> + * Disable the shared DPLL used by @crtc.
> + */
>  void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -266,6 +312,16 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
>  	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
>  }
>  
> +/**
> + * intel_shared_dpll_swap_state - make atomic DPLL configuration effective
> + * @state: atomic state
> + *
> + * This is the dpll version of drm_atomic_helper_swap_state() since the
> + * helper does not handle driver-specific global state.
> + *
> + * Note that this doesn't actually swap states, the dpll configutation in
> + * @state is left unchanged.

Hm, what do you mean with that? I guess you mean that it only puts the
state from drm_atomic_state into dev_priv, and not the other way round.
Tbh that's a bit unexpected (yes atm we don't have a reason for that), so
instead of documenting this oddity I'd just fix it. And then maybe mention
that "For consistency with atomic helpers this also puts the current state
into @state, i.e. it does a complete swap, even though right now that's
not needed."

> + */
>  void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> @@ -1822,6 +1878,12 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.get_dpll = bxt_get_dpll,
>  };
>  
> +/**
> + * intel_shared_dpll_init - Initialize shared DPLLs
> + * @dev: drm device
> + *
> + * Initialize shared DPLLs for @dev.
> + */
>  void intel_shared_dpll_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -1865,6 +1927,21 @@ void intel_shared_dpll_init(struct drm_device *dev)
>  		intel_ddi_pll_init(dev);
>  }
>  
> +/**
> + * intel_get_shared_dpll - get a shared DPLL for crtc and encoder combination
> + * @crtc: crtc
> + * @crtc_state: atomic state for @crtc
> + * @encoder: encoder
> + *
> + * Find an appropriate DPLL for the given crtc and encoder combination. A
> + * reference from the @crtc to the returned pll is registered in the atomic
> + * state. That configuration is made effective by calling
> + * intel_shared_dpll_swap_state(). The reference should be released by calling
> + * intel_release_shared_dpll().
> + *
> + * Returns:
> + * A shared DPLL to be used by @crtc and @encoder with the given @crtc_state.
> + */
>  struct intel_shared_dpll *
>  intel_get_shared_dpll(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *crtc_state,
> @@ -1885,6 +1962,9 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
>   * @crtc: crtc
>   * @state: atomic state
>   *
> + * This function releases the reference from @crtc to @dpll from the
> + * atomic @state. The new configuration is made effective by calling
> + * intel_shared_dpll_swap_state().
>   */
>  void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
>  			       struct intel_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 9a7db65..40f1a6f 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -40,32 +40,72 @@ struct intel_encoder;
>  struct intel_shared_dpll;
>  struct intel_dpll_mgr;
>  
> +/**
> + * enum intel_dpll_id - possible DPLL ids
> + *
> + * Enumeration of possible IDs for a DPLL. Real shared dpll ids must be >= 0.
> + */
>  enum intel_dpll_id {
> -	DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */
> -	/* real shared dpll ids must be >= 0 */
> +	/**
> +	 * @DPLL_ID_PRIVATE: non-shared dpll in use
> +	 */
> +	DPLL_ID_PRIVATE = -1,
> +
> +	/**
> +	 * @DPLL_ID_PCH_PLL_A: DPLL A in ILK, SNB and IVB
> +	 */
>  	DPLL_ID_PCH_PLL_A = 0,
> +	/**
> +	 * @DPLL_ID_PCH_PLL_B: DPLL B in ILK, SNB and IVB
> +	 */
>  	DPLL_ID_PCH_PLL_B = 1,
> -	/* hsw/bdw */
> +
> +
> +	/**
> +	 * @DPLL_ID_WRPLL1: HSW and BDW WRPLL1
> +	 */
>  	DPLL_ID_WRPLL1 = 0,
> +	/**
> +	 * @DPLL_ID_WRPLL2: HSW and BDW WRPLL2
> +	 */
>  	DPLL_ID_WRPLL2 = 1,
> +	/**
> +	 * @DPLL_ID_SPLL: HSW and BDW SPLL
> +	 */
>  	DPLL_ID_SPLL = 2,
> +	/**
> +	 * @DPLL_ID_LCPLL_810: HSW and BDW 0.81 GHz LCPLL
> +	 */
>  	DPLL_ID_LCPLL_810 = 3,
> +	/**
> +	 * @DPLL_ID_LCPLL_1350: HSW and BDW 1.35 GHz LCPLL
> +	 */
>  	DPLL_ID_LCPLL_1350 = 4,
> +	/**
> +	 * @DPLL_ID_LCPLL_2700: HSW and BDW 2.7 GHz LCPLL
> +	 */
>  	DPLL_ID_LCPLL_2700 = 5,
>  
> -	/* skl */
> +
> +	/**
> +	 * @DPLL_ID_SKL_DPLL0: SKL and later DPLL0
> +	 */
>  	DPLL_ID_SKL_DPLL0 = 0,
> +	/**
> +	 * @DPLL_ID_SKL_DPLL1: SKL and later DPLL1
> +	 */
>  	DPLL_ID_SKL_DPLL1 = 1,
> +	/**
> +	 * @DPLL_ID_SKL_DPLL2: SKL and later DPLL2
> +	 */
>  	DPLL_ID_SKL_DPLL2 = 2,
> +	/**
> +	 * @DPLL_ID_SKL_DPLL3: SKL and later DPLL3
> +	 */
>  	DPLL_ID_SKL_DPLL3 = 3,
>  };
>  #define I915_NUM_PLLS 6
>  
> -/** Inform the state checker that the DPLL is kept enabled even if not
> - * in use by any crtc.
> - */
> -#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> -
>  struct intel_dpll_hw_state {
>  	/* i9xx, pch plls */
>  	uint32_t dpll;
> @@ -93,39 +133,124 @@ struct intel_dpll_hw_state {
>  		 pcsdw12;
>  };
>  
> +/**
> + * struct intel_shared_dpll_state - hold the DPLL atomic state
> + *
> + * This structure holds an atomic state for the DPLL, that can represent
> + * either its current state or a desired furture state which would be
> + * applied by an atomic mode set.

I think it'd be good to reference where we store pointers to that, i.e.
where in intel_atomic_state and intel_shared_dpll it sits. Best if you do that
using the struct &intel_atomic_state reference style (so that it becomes a
hyperlink once that's documented).

Also links to the main functions used to manage this would be good I
think, i.e. release and get.

> + */
>  struct intel_shared_dpll_state {
> -	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
> +	/**
> +	 * @crtc_mask: mask of crtc using this DPLL, active or not

s/crtc/CRTCs/

> +	 */
> +	unsigned crtc_mask;
> +
> +	/**
> +	 * @hw_state: hardware configuration for the DPLL.

"... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)

> +	 */
>  	struct intel_dpll_hw_state hw_state;
>  };
>  
> +/**
> + * struct intel_shared_dpll_funcs - platform specific hooks for managing DPLLs
> + */
>  struct intel_shared_dpll_funcs {
> -	/* The mode_set hook is optional and should be used together with the
> -	 * intel_prepare_shared_dpll function. */
> +	/**
> +	 * @prepare:
> +	 *
> +	 * Optional hook to perform operations prior to enabling the PLL.
> +	 * Called from intel_prepare_shared_dpll() function.

Missing the language about "if the pll is not already enabled."

> +	 */
>  	void (*prepare)(struct drm_i915_private *dev_priv,
>  			 struct intel_shared_dpll *pll);
> +
> +	/**
> +	 * @enable:
> +	 *
> +	 * Hook for enabling the pll, called from intel_enable_shared_dpll()
> +	 * if the pll is not already enabled.
> +	 */
>  	void (*enable)(struct drm_i915_private *dev_priv,
>  		       struct intel_shared_dpll *pll);
> +
> +	/**
> +	 * @disable:
> +	 *
> +	 * Hook for disabling the pll, called from intel_disable_shared_dpll()
> +	 * only when it is safe to disable the pll, i.e., there are no more
> +	 * tracked users for it.
> +	 */
>  	void (*disable)(struct drm_i915_private *dev_priv,
>  			struct intel_shared_dpll *pll);
> +
> +	/**
> +	 * @get_hw_state:
> +	 *
> +	 * Hook for reading the values currently programmed to the DPLL
> +	 * registers. This is used for initial hw state readout and state
> +	 * verification after a mode set.
> +	 */
>  	bool (*get_hw_state)(struct drm_i915_private *dev_priv,
>  			     struct intel_shared_dpll *pll,
>  			     struct intel_dpll_hw_state *hw_state);
>  };
>  
> +/**
> + * struct intel_shared_dpll - display PLL with tracked state and users
> + */
>  struct intel_shared_dpll {
> +	/**
> +	 * @state:
> +	 *
> +	 * Store the state for the pll, including the its hw state
> +	 * and crtcs using it.
> +	 */
>  	struct intel_shared_dpll_state state;
>  
> -	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
> -	bool on; /* is the PLL actually active? Disabled during modeset */
> +	/**
> +	 * @active_mask: mask of active CRTCs (i.e. DPMS on) using this DPLL
> +	 */
> +	unsigned active_mask;
> +
> +	/**
> +	 * @on: is the PLL actually active? Disabled during modeset
> +	 */
> +	bool on;
> +
> +	/**
> +	 * @name: DPLL name; used for logging
> +	 */
>  	const char *name;
> -	/* should match the index in the dev_priv->shared_dplls array */
> +
> +	/**
> +	 * @id: unique indentifier for this DPLL; should match the index in the
> +	 * dev_priv->shared_dplls array
> +	 */
>  	enum intel_dpll_id id;
>  
> +	/**
> +	 * @funcs: platform specific hooks
> +	 */
>  	struct intel_shared_dpll_funcs funcs;
>  
> +	/**
> +	 * @flags:
> +	 *
> +	 * accepted flags: INTEL_DPLL_ALWAYS_ON

Hm, I've started to just document flags in-line as an enumeration, to keep
things tightly grouped. And then also place the #define right in front of
the kernel-doc for @flags. See e.g. @flags in drm_property.h for a really
long example of that (but there the flags are in an uabi header, so a bit
special).

With the nitpicks addressed somehow:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	 */
>  	uint32_t flags;
>  };
>  
> +/**
> + * INTEL_DPLL_ALWAYS_ON
> + *
> + * Inform the state checker that the DPLL is kept enabled even if not
> + * in use by any crtc.
> + */
> +#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> +
> +
>  #define SKL_DPLL0 0
>  #define SKL_DPLL1 1
>  #define SKL_DPLL2 2
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: Add dpll entrypoint for dumping hw state

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:16PM +0300, Ander Conselvan de Oliveira wrote:
> Remove the IS_PLATFORM() macros from intel_dump_pipe_config() and split
> that logic in platform specific implementations inside the dpll code,
> accessed through a platform independent interface.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Real pretty ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c  | 35 +---------------
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 79 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |  3 ++
>  3 files changed, 84 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e6fe85b..8ecaf18 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12744,6 +12744,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  				   const char *context)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct drm_plane *plane;
>  	struct intel_plane *intel_plane;
>  	struct intel_plane_state *state;
> @@ -12805,39 +12806,7 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc,
>  	DRM_DEBUG_KMS("ips: %i\n", pipe_config->ips_enabled);
>  	DRM_DEBUG_KMS("double wide: %i\n", pipe_config->double_wide);
>  
> -	if (IS_BROXTON(dev)) {
> -		DRM_DEBUG_KMS("dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> -			      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> -			      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
> -			      pipe_config->dpll_hw_state.ebb0,
> -			      pipe_config->dpll_hw_state.ebb4,
> -			      pipe_config->dpll_hw_state.pll0,
> -			      pipe_config->dpll_hw_state.pll1,
> -			      pipe_config->dpll_hw_state.pll2,
> -			      pipe_config->dpll_hw_state.pll3,
> -			      pipe_config->dpll_hw_state.pll6,
> -			      pipe_config->dpll_hw_state.pll8,
> -			      pipe_config->dpll_hw_state.pll9,
> -			      pipe_config->dpll_hw_state.pll10,
> -			      pipe_config->dpll_hw_state.pcsdw12);
> -	} else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev)) {
> -		DRM_DEBUG_KMS("dpll_hw_state: "
> -			      "ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
> -			      pipe_config->dpll_hw_state.ctrl1,
> -			      pipe_config->dpll_hw_state.cfgcr1,
> -			      pipe_config->dpll_hw_state.cfgcr2);
> -	} else if (HAS_DDI(dev)) {
> -		DRM_DEBUG_KMS("dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
> -			      pipe_config->dpll_hw_state.wrpll,
> -			      pipe_config->dpll_hw_state.spll);
> -	} else {
> -		DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> -			      "fp0: 0x%x, fp1: 0x%x\n",
> -			      pipe_config->dpll_hw_state.dpll,
> -			      pipe_config->dpll_hw_state.dpll_md,
> -			      pipe_config->dpll_hw_state.fp0,
> -			      pipe_config->dpll_hw_state.fp1);
> -	}
> +	intel_dpll_dump_hw_state(dev_priv, &pipe_config->dpll_hw_state);
>  
>  	DRM_DEBUG_KMS("planes on this crtc\n");
>  	list_for_each_entry(plane, &dev->mode_config.plane_list, head) {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 8c4efa9..9b02d9c 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -447,6 +447,17 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static void ibx_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state)
> +{
> +	DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> +		      "fp0: 0x%x, fp1: 0x%x\n",
> +		      hw_state->dpll,
> +		      hw_state->dpll_md,
> +		      hw_state->fp0,
> +		      hw_state->fp1);
> +}
> +
>  static const struct intel_shared_dpll_funcs ibx_pch_dpll_funcs = {
>  	.prepare = ibx_pch_dpll_prepare,
>  	.enable = ibx_pch_dpll_enable,
> @@ -833,6 +844,13 @@ hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static void hsw_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state)
> +{
> +	DRM_DEBUG_KMS("dpll_hw_state: wrpll: 0x%x spll: 0x%x\n",
> +		      hw_state->wrpll, hw_state->spll);
> +}
> +
>  static const struct intel_shared_dpll_funcs hsw_ddi_wrpll_funcs = {
>  	.enable = hsw_ddi_wrpll_enable,
>  	.disable = hsw_ddi_wrpll_disable,
> @@ -1388,6 +1406,16 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static void skl_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state)
> +{
> +	DRM_DEBUG_KMS("dpll_hw_state: "
> +		      "ctrl1: 0x%x, cfgcr1: 0x%x, cfgcr2: 0x%x\n",
> +		      hw_state->ctrl1,
> +		      hw_state->cfgcr1,
> +		      hw_state->cfgcr2);
> +}
> +
>  static const struct intel_shared_dpll_funcs skl_ddi_pll_funcs = {
>  	.enable = skl_ddi_pll_enable,
>  	.disable = skl_ddi_pll_disable,
> @@ -1785,6 +1813,25 @@ bxt_get_dpll(struct intel_crtc *crtc,
>  	return pll;
>  }
>  
> +static void bxt_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state)
> +{
> +	DRM_DEBUG_KMS("dpll_hw_state: ebb0: 0x%x, ebb4: 0x%x,"
> +		      "pll0: 0x%x, pll1: 0x%x, pll2: 0x%x, pll3: 0x%x, "
> +		      "pll6: 0x%x, pll8: 0x%x, pll9: 0x%x, pll10: 0x%x, pcsdw12: 0x%x\n",
> +		      hw_state->ebb0,
> +		      hw_state->ebb4,
> +		      hw_state->pll0,
> +		      hw_state->pll1,
> +		      hw_state->pll2,
> +		      hw_state->pll3,
> +		      hw_state->pll6,
> +		      hw_state->pll8,
> +		      hw_state->pll9,
> +		      hw_state->pll10,
> +		      hw_state->pcsdw12);
> +}
> +
>  static const struct intel_shared_dpll_funcs bxt_ddi_pll_funcs = {
>  	.enable = bxt_ddi_pll_enable,
>  	.disable = bxt_ddi_pll_disable,
> @@ -1825,6 +1872,9 @@ struct intel_dpll_mgr {
>  	struct intel_shared_dpll *(*get_dpll)(struct intel_crtc *crtc,
>  					      struct intel_crtc_state *crtc_state,
>  					      struct intel_encoder *encoder);
> +
> +	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state);
>  };
>  
>  static const struct dpll_info pch_plls[] = {
> @@ -1836,6 +1886,7 @@ static const struct dpll_info pch_plls[] = {
>  static const struct intel_dpll_mgr pch_pll_mgr = {
>  	.dpll_info = pch_plls,
>  	.get_dpll = ibx_get_dpll,
> +	.dump_hw_state = ibx_dump_hw_state,
>  };
>  
>  static const struct dpll_info hsw_plls[] = {
> @@ -1851,6 +1902,7 @@ static const struct dpll_info hsw_plls[] = {
>  static const struct intel_dpll_mgr hsw_pll_mgr = {
>  	.dpll_info = hsw_plls,
>  	.get_dpll = hsw_get_dpll,
> +	.dump_hw_state = hsw_dump_hw_state,
>  };
>  
>  static const struct dpll_info skl_plls[] = {
> @@ -1864,6 +1916,7 @@ static const struct dpll_info skl_plls[] = {
>  static const struct intel_dpll_mgr skl_pll_mgr = {
>  	.dpll_info = skl_plls,
>  	.get_dpll = skl_get_dpll,
> +	.dump_hw_state = skl_dump_hw_state,
>  };
>  
>  static const struct dpll_info bxt_plls[] = {
> @@ -1876,6 +1929,7 @@ static const struct dpll_info bxt_plls[] = {
>  static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.dpll_info = bxt_plls,
>  	.get_dpll = bxt_get_dpll,
> +	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
>  /**
> @@ -1975,3 +2029,28 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
>  	shared_dpll_state = intel_atomic_get_shared_dpll_state(state);
>  	shared_dpll_state[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
>  }
> +
> +/**
> + * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
> + * @dev_priv: i915 drm device
> + * @hw_state: hw state to be written to the log
> + *
> + * Write the relevant values in @hw_state to dmesg using DRM_DEBUG_KMS.
> + */
> +void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state)
> +{
> +	if (dev_priv->dpll_mgr) {
> +		dev_priv->dpll_mgr->dump_hw_state(dev_priv, hw_state);
> +	} else {
> +		/* fallback for platforms that don't use the shared dpll
> +		 * infrastructure
> +		 */
> +		DRM_DEBUG_KMS("dpll_hw_state: dpll: 0x%x, dpll_md: 0x%x, "
> +			      "fp0: 0x%x, fp1: 0x%x\n",
> +			      hw_state->dpll,
> +			      hw_state->dpll_md,
> +			      hw_state->fp0,
> +			      hw_state->fp1);
> +	}
> +}
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 40f1a6f..76111a4 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -280,6 +280,9 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
>  void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
>  void intel_shared_dpll_init(struct drm_device *dev);
>  
> +void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
> +			      struct intel_dpll_hw_state *hw_state);
> +
>  /* BXT dpll related functions */
>  bool bxt_ddi_dp_set_dpll_hw_state(int clock,
>  			  struct intel_dpll_hw_state *dpll_hw_state);
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Add entrypoints for mapping dplls to encoders and crtcs

[Daniel Vetter]

On Tue, Oct 04, 2016 at 03:32:17PM +0300, Ander Conselvan de Oliveira wrote:
> Abstract the platform specific bits of mapping the dplls under a
> platform independ entrypoints so the differences between platforms are
> contained in the dpll code. I.e., it removes IS_PLATFORM() macros from
> other parts of the code.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

Not really sure this is worth it. This is all highly platform specific,
adding abstraction just means even more hoops to jump through. But I
spotted one thing while reading this. I know you're trying to consolidate
the code here, but it feels a bit too much. At least I feel like the
interactions between dpll and other code (i.e. how they link to
encoders/crtc) is best left to that code. After all it's also that code
that decides what kind of dpll setting it wants, the dpll mgr just figures
out whether one is still available.
-Daniel



> ---
>  drivers/gpu/drm/i915/intel_ddi.c      |  51 ++----
>  drivers/gpu/drm/i915/intel_display.c  | 124 +-------------
>  drivers/gpu/drm/i915/intel_dp_mst.c   |   4 +-
>  drivers/gpu/drm/i915/intel_dpll_mgr.c | 299 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dpll_mgr.h |   7 +
>  drivers/gpu/drm/i915/intel_drv.h      |   4 +-
>  6 files changed, 334 insertions(+), 155 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 4077205..144fe5c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -319,6 +319,19 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *encoder)
>  	}
>  }
>  
> +struct intel_encoder *
> +intel_ddi_get_port_encoder(struct drm_i915_private *dev_priv, enum port port)
> +{
> +	struct intel_encoder *encoder;
> +
> +	for_each_intel_encoder(&dev_priv->drm, encoder) {
> +		if (port == intel_ddi_get_encoder_port(encoder))
> +			return encoder;
> +	}
> +
> +	return NULL;
> +}
> +
>  static const struct ddi_buf_trans *
>  bdw_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> @@ -582,11 +595,12 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_encoder *encoder;
> -	u32 temp, i, rx_ctl_val, ddi_pll_sel;
> +	u32 temp, i, rx_ctl_val;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		WARN_ON(encoder->type != INTEL_OUTPUT_ANALOG);
>  		intel_prepare_dp_ddi_buffers(encoder);
> +		break;
>  	}
>  
>  	/* Set the FDI_RX_MISC pwrdn lanes and the 2 workarounds listed at the
> @@ -613,9 +627,7 @@ void hsw_fdi_link_train(struct drm_crtc *crtc)
>  	I915_WRITE(FDI_RX_CTL(PIPE_A), rx_ctl_val);
>  
>  	/* Configure Port Clock Select */
> -	ddi_pll_sel = hsw_pll_to_ddi_pll_sel(intel_crtc->config->shared_dpll);
> -	I915_WRITE(PORT_CLK_SEL(PORT_E), ddi_pll_sel);
> -	WARN_ON(ddi_pll_sel != PORT_CLK_SEL_SPLL);
> +	intel_dpll_map_to_encoder(intel_crtc->config->shared_dpll, encoder);
>  
>  	/* Start the training iterating through available voltages and emphasis,
>  	 * testing each value twice. */
> @@ -1607,33 +1619,6 @@ uint32_t ddi_signal_levels(struct intel_dp *intel_dp)
>  	return DDI_BUF_TRANS_SELECT(level);
>  }
>  
> -void intel_ddi_clk_select(struct intel_encoder *encoder,
> -			  struct intel_shared_dpll *pll)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	enum port port = intel_ddi_get_encoder_port(encoder);
> -
> -	if (WARN_ON(!pll))
> -		return;
> -
> -	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) {
> -		uint32_t val;
> -
> -		/* DDI -> PLL mapping  */
> -		val = I915_READ(DPLL_CTRL2);
> -
> -		val &= ~(DPLL_CTRL2_DDI_CLK_OFF(port) |
> -			DPLL_CTRL2_DDI_CLK_SEL_MASK(port));
> -		val |= (DPLL_CTRL2_DDI_CLK_SEL(pll->id, port) |
> -			DPLL_CTRL2_DDI_SEL_OVERRIDE(port));
> -
> -		I915_WRITE(DPLL_CTRL2, val);
> -
> -	} else if (INTEL_INFO(dev_priv)->gen < 9) {
> -		I915_WRITE(PORT_CLK_SEL(port), hsw_pll_to_ddi_pll_sel(pll));
> -	}
> -}
> -
>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  				    int link_rate, uint32_t lane_count,
>  				    struct intel_shared_dpll *pll,
> @@ -1648,7 +1633,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  	if (encoder->type == INTEL_OUTPUT_EDP)
>  		intel_edp_panel_on(intel_dp);
>  
> -	intel_ddi_clk_select(encoder, pll);
> +	intel_dpll_map_to_encoder(pll, encoder);
>  	intel_prepare_dp_ddi_buffers(encoder);
>  	intel_ddi_init_dp_buf_reg(encoder);
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> @@ -1669,7 +1654,7 @@ static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
>  	int level = intel_ddi_hdmi_level(dev_priv, port);
>  
>  	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
> -	intel_ddi_clk_select(encoder, pll);
> +	intel_dpll_map_to_encoder(pll, encoder);
>  	intel_prepare_hdmi_ddi_buffers(encoder);
>  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
>  		skl_ddi_set_iboost(encoder, level);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ecaf18..22e3c46 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4555,19 +4555,7 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>  
>  	/* We need to program the right clock selection before writing the pixel
>  	 * mutliplier into the DPLL. */
> -	if (HAS_PCH_CPT(dev)) {
> -		u32 sel;
> -
> -		temp = I915_READ(PCH_DPLL_SEL);
> -		temp |= TRANS_DPLL_ENABLE(pipe);
> -		sel = TRANS_DPLLB_SEL(pipe);
> -		if (intel_crtc->config->shared_dpll ==
> -		    intel_get_shared_dpll_by_id(dev_priv, DPLL_ID_PCH_PLL_B))
> -			temp |= sel;
> -		else
> -			temp &= ~sel;
> -		I915_WRITE(PCH_DPLL_SEL, temp);
> -	}
> +	intel_dpll_map_to_crtc(intel_crtc->config->shared_dpll, intel_crtc);
>  
>  	/* XXX: pch pll's can be enabled any time before we enable the PCH
>  	 * transcoder, and we actually should do this to not upset any PCH
> @@ -5574,9 +5562,7 @@ static void ironlake_crtc_disable(struct intel_crtc_state *old_crtc_state,
>  			I915_WRITE(reg, temp);
>  
>  			/* disable DPLL_SEL */
> -			temp = I915_READ(PCH_DPLL_SEL);
> -			temp &= ~(TRANS_DPLL_ENABLE(pipe) | TRANS_DPLLB_SEL(pipe));
> -			I915_WRITE(PCH_DPLL_SEL, temp);
> +			intel_dpll_map_to_crtc(NULL, intel_crtc);
>  		}
>  
>  		ironlake_fdi_pll_disable(intel_crtc);
> @@ -9952,7 +9938,6 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  	if (I915_READ(PCH_TRANSCONF(crtc->pipe)) & TRANS_ENABLE) {
>  		struct intel_shared_dpll *pll;
> -		enum intel_dpll_id pll_id;
>  
>  		pipe_config->has_pch_encoder = true;
>  
> @@ -9962,23 +9947,7 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  
>  		ironlake_get_fdi_m_n_config(crtc, pipe_config);
>  
> -		if (HAS_PCH_IBX(dev_priv)) {
> -			/*
> -			 * The pipe->pch transcoder and pch transcoder->pll
> -			 * mapping is fixed.
> -			 */
> -			pll_id = (enum intel_dpll_id) crtc->pipe;
> -		} else {
> -			tmp = I915_READ(PCH_DPLL_SEL);
> -			if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
> -				pll_id = DPLL_ID_PCH_PLL_B;
> -			else
> -				pll_id= DPLL_ID_PCH_PLL_A;
> -		}
> -
> -		pipe_config->shared_dpll =
> -			intel_get_shared_dpll_by_id(dev_priv, pll_id);
> -		pll = pipe_config->shared_dpll;
> +		pipe_config->shared_dpll = pll = intel_get_crtc_dpll(crtc);
>  
>  		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
>  						 &pipe_config->dpll_hw_state));
> @@ -10458,82 +10427,6 @@ static int haswell_crtc_compute_clock(struct intel_crtc *crtc,
>  	return 0;
>  }
>  
> -static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv,
> -				enum port port,
> -				struct intel_crtc_state *pipe_config)
> -{
> -	enum intel_dpll_id id;
> -
> -	switch (port) {
> -	case PORT_A:
> -		id = DPLL_ID_SKL_DPLL0;
> -		break;
> -	case PORT_B:
> -		id = DPLL_ID_SKL_DPLL1;
> -		break;
> -	case PORT_C:
> -		id = DPLL_ID_SKL_DPLL2;
> -		break;
> -	default:
> -		DRM_ERROR("Incorrect port type\n");
> -		return;
> -	}
> -
> -	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> -}
> -
> -static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv,
> -				enum port port,
> -				struct intel_crtc_state *pipe_config)
> -{
> -	enum intel_dpll_id id;
> -	u32 temp;
> -
> -	temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
> -	id = temp >> (port * 3 + 1);
> -
> -	if (WARN_ON(id < SKL_DPLL0 || id > SKL_DPLL3))
> -		return;
> -
> -	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> -}
> -
> -static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
> -				enum port port,
> -				struct intel_crtc_state *pipe_config)
> -{
> -	enum intel_dpll_id id;
> -	uint32_t ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
> -
> -	switch (ddi_pll_sel) {
> -	case PORT_CLK_SEL_WRPLL1:
> -		id = DPLL_ID_WRPLL1;
> -		break;
> -	case PORT_CLK_SEL_WRPLL2:
> -		id = DPLL_ID_WRPLL2;
> -		break;
> -	case PORT_CLK_SEL_SPLL:
> -		id = DPLL_ID_SPLL;
> -		break;
> -	case PORT_CLK_SEL_LCPLL_810:
> -		id = DPLL_ID_LCPLL_810;
> -		break;
> -	case PORT_CLK_SEL_LCPLL_1350:
> -		id = DPLL_ID_LCPLL_1350;
> -		break;
> -	case PORT_CLK_SEL_LCPLL_2700:
> -		id = DPLL_ID_LCPLL_2700;
> -		break;
> -	default:
> -		MISSING_CASE(ddi_pll_sel);
> -		/* fall through */
> -	case PORT_CLK_SEL_NONE:
> -		return;
> -	}
> -
> -	pipe_config->shared_dpll = intel_get_shared_dpll_by_id(dev_priv, id);
> -}
> -
>  static bool hsw_get_transcoder_state(struct intel_crtc *crtc,
>  				     struct intel_crtc_state *pipe_config,
>  				     unsigned long *power_domain_mask)
> @@ -10638,6 +10531,7 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  {
>  	struct drm_device *dev = crtc->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_encoder *encoder;
>  	struct intel_shared_dpll *pll;
>  	enum port port;
>  	uint32_t tmp;
> @@ -10646,14 +10540,8 @@ static void haswell_get_ddi_port_state(struct intel_crtc *crtc,
>  
>  	port = (tmp & TRANS_DDI_PORT_MASK) >> TRANS_DDI_PORT_SHIFT;
>  
> -	if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
> -		skylake_get_ddi_pll(dev_priv, port, pipe_config);
> -	else if (IS_BROXTON(dev))
> -		bxt_get_ddi_pll(dev_priv, port, pipe_config);
> -	else
> -		haswell_get_ddi_pll(dev_priv, port, pipe_config);
> -
> -	pll = pipe_config->shared_dpll;
> +	encoder =  intel_ddi_get_port_encoder(dev_priv, port);
> +	pipe_config->shared_dpll = pll = intel_get_encoder_dpll(encoder);
>  	if (pll) {
>  		WARN_ON(!pll->funcs.get_hw_state(dev_priv, pll,
>  						 &pipe_config->dpll_hw_state));
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 3ffbd69..cb097fb 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -150,8 +150,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
>  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
>  
>  	if (intel_dp->active_mst_links == 0) {
> -		intel_ddi_clk_select(&intel_dig_port->base,
> -				     pipe_config->shared_dpll);
> +		intel_dpll_map_to_encoder(pipe_config->shared_dpll,
> +					  &intel_dig_port->base);
>  
>  		intel_prepare_dp_ddi_buffers(&intel_dig_port->base);
>  		intel_dp_set_link_params(intel_dp,
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index 9b02d9c..532236d 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -40,6 +40,12 @@
>   * Changes to the users are first staged in the atomic state, and then made
>   * effective by calling intel_shared_dpll_swap_state() during the atomic
>   * commit phase.
> + *
> + * The functions intel_dpll_map_to_crtc(), intel_dpll_map_to_encoder(),
> + * intel_get_crtc_dpll() and intel_get_encoder_dpll() allows the mapping of
> + * dplls to crtcs and encoders to be queried and changed. Whether plls map
> + * to encoders or crtcs depends on the platform. The caller needs to know
> + * which of the functions to use.
>   */
>  
>  struct intel_shared_dpll *
> @@ -447,6 +453,54 @@ ibx_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static struct intel_shared_dpll *
> +ibx_get_crtc_dpll(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum intel_dpll_id pll_id;
> +	u32 tmp;
> +
> +	if (HAS_PCH_IBX(dev_priv)) {
> +		/*
> +		 * The pipe->pch transcoder and pch transcoder->pll
> +		 * mapping is fixed.
> +		 */
> +		pll_id = (enum intel_dpll_id) crtc->pipe;
> +	} else {
> +		tmp = I915_READ(PCH_DPLL_SEL);
> +		if (tmp & TRANS_DPLLB_SEL(crtc->pipe))
> +			pll_id = DPLL_ID_PCH_PLL_B;
> +		else
> +			pll_id= DPLL_ID_PCH_PLL_A;
> +	}
> +
> +	return intel_get_shared_dpll_by_id(dev_priv, pll_id);
> +}
> +
> +static void
> +ibx_dpll_map_to_crtc(struct intel_shared_dpll *dpll, struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	u32 temp, sel;
> +
> +	if (!HAS_PCH_CPT(dev_priv))
> +		return;
> +
> +	temp = I915_READ(PCH_DPLL_SEL);
> +	if (dpll) {
> +		temp |= TRANS_DPLL_ENABLE(pipe);
> +		sel = TRANS_DPLLB_SEL(pipe);
> +		if (dpll->id ==  DPLL_ID_PCH_PLL_B)
> +			temp |= sel;
> +		else
> +			temp &= ~sel;
> +	} else {
> +		temp &= ~(TRANS_DPLL_ENABLE(pipe) | TRANS_DPLLB_SEL(pipe));
> +	}
> +	I915_WRITE(PCH_DPLL_SEL, temp);
> +}
> +
>  static void ibx_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state)
>  {
> @@ -844,6 +898,73 @@ hsw_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static enum intel_dpll_id hsw_reg_to_pll_id(uint32_t ddi_pll_sel)
> +{
> +	switch (ddi_pll_sel) {
> +	case PORT_CLK_SEL_WRPLL1:
> +		return DPLL_ID_WRPLL1;
> +	case PORT_CLK_SEL_WRPLL2:
> +		return DPLL_ID_WRPLL2;
> +	case PORT_CLK_SEL_SPLL:
> +		return DPLL_ID_SPLL;
> +	case PORT_CLK_SEL_LCPLL_810:
> +		return DPLL_ID_LCPLL_810;
> +	case PORT_CLK_SEL_LCPLL_1350:
> +		return DPLL_ID_LCPLL_1350;
> +	case PORT_CLK_SEL_LCPLL_2700:
> +		return DPLL_ID_LCPLL_2700;
> +	default:
> +		MISSING_CASE(ddi_pll_sel);
> +		/* fall through */
> +	case PORT_CLK_SEL_NONE:
> +		return DPLL_ID_PRIVATE;
> +	}
> +}
> +
> +static uint32_t hsw_pll_id_to_reg(enum intel_dpll_id id)
> +{
> +	switch (id) {
> +	case DPLL_ID_WRPLL1:
> +		return PORT_CLK_SEL_WRPLL1;
> +	case DPLL_ID_WRPLL2:
> +		return PORT_CLK_SEL_WRPLL2;
> +	case DPLL_ID_SPLL:
> +		return PORT_CLK_SEL_SPLL;
> +	case DPLL_ID_LCPLL_810:
> +		return PORT_CLK_SEL_LCPLL_810;
> +	case DPLL_ID_LCPLL_1350:
> +		return PORT_CLK_SEL_LCPLL_1350;
> +	case DPLL_ID_LCPLL_2700:
> +		return PORT_CLK_SEL_LCPLL_2700;
> +	default:
> +		MISSING_CASE(id);
> +		/* fall through */
> +	case DPLL_ID_PRIVATE:
> +		return PORT_CLK_SEL_NONE;
> +	}
> +}
> +
> +static struct intel_shared_dpll *
> +hsw_get_encoder_dpll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +	uint32_t ddi_pll_sel = I915_READ(PORT_CLK_SEL(port));
> +
> +	return intel_get_shared_dpll_by_id(dev_priv,
> +					   hsw_reg_to_pll_id(ddi_pll_sel));
> +}
> +
> +static void
> +hsw_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
> +			struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +
> +	I915_WRITE(PORT_CLK_SEL(port), hsw_pll_id_to_reg(dpll->id));
> +}
> +
>  static void hsw_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state)
>  {
> @@ -1406,6 +1527,41 @@ skl_get_dpll(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
>  	return pll;
>  }
>  
> +static struct intel_shared_dpll *
> +skl_get_encoder_dpll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum intel_dpll_id id;
> +	u32 temp;
> +
> +	temp = I915_READ(DPLL_CTRL2) & DPLL_CTRL2_DDI_CLK_SEL_MASK(port);
> +	id = temp >> (port * 3 + 1);
> +
> +	if (WARN_ON(id < SKL_DPLL0 || id > SKL_DPLL3))
> +		return NULL;
> +
> +	return intel_get_shared_dpll_by_id(dev_priv, id);
> +}
> +
> +static void
> +skl_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
> +			struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +	uint32_t val;
> +
> +	val = I915_READ(DPLL_CTRL2);
> +
> +	val &= ~(DPLL_CTRL2_DDI_CLK_OFF(port) |
> +		DPLL_CTRL2_DDI_CLK_SEL_MASK(port));
> +	val |= (DPLL_CTRL2_DDI_CLK_SEL(dpll->id, port) |
> +		DPLL_CTRL2_DDI_SEL_OVERRIDE(port));
> +
> +	I915_WRITE(DPLL_CTRL2, val);
> +}
> +
>  static void skl_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state)
>  {
> @@ -1812,6 +1968,38 @@ bxt_get_dpll(struct intel_crtc *crtc,
>  
>  	return pll;
>  }
> +static struct intel_shared_dpll *
> +bxt_get_encoder_dpll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum port port = intel_ddi_get_encoder_port(encoder);
> +	enum intel_dpll_id id;
> +
> +	switch (port) {
> +	case PORT_A:
> +		id = DPLL_ID_SKL_DPLL0;
> +		break;
> +	case PORT_B:
> +		id = DPLL_ID_SKL_DPLL1;
> +		break;
> +	case PORT_C:
> +		id = DPLL_ID_SKL_DPLL2;
> +		break;
> +	default:
> +		DRM_ERROR("Incorrect port type\n");
> +		return NULL;
> +	}
> +
> +	return intel_get_shared_dpll_by_id(dev_priv, id);
> +}
> +
> +static void
> +bxt_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
> +			struct intel_encoder *encoder)
> +{
> +	/* Fixed mapping, nothing to do */
> +}
> +
>  
>  static void bxt_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state)
> @@ -1859,6 +2047,33 @@ static void intel_ddi_pll_init(struct drm_device *dev)
>  	}
>  }
>  
> +static void
> +noop_dpll_map_to_crtc(struct intel_shared_dpll *dpll, struct intel_crtc *crtc)
> +{
> +	WARN(1, "Platform does not support mapping DPLLs to CRTCs.");
> +}
> +
> +static struct intel_shared_dpll *
> +noop_get_crtc_dpll(struct intel_crtc *crtc)
> +{
> +	WARN(1, "Platform does not support mapping DPLLs to CRTCs.");
> +	return NULL;
> +}
> +
> +static void
> +noop_dpll_map_to_encoder(struct intel_shared_dpll *dpll, struct intel_encoder *encoder)
> +{
> +	WARN(1, "Platform does not support mapping DPLLs to encoders.");
> +}
> +
> +static struct intel_shared_dpll *
> +noop_get_encoder_dpll(struct intel_encoder *encoder)
> +{
> +	WARN(1, "Platform does not support mapping DPLLs to encoders.");
> +	return NULL;
> +}
> +
> +
>  struct dpll_info {
>  	const char *name;
>  	const int id;
> @@ -1873,6 +2088,14 @@ struct intel_dpll_mgr {
>  					      struct intel_crtc_state *crtc_state,
>  					      struct intel_encoder *encoder);
>  
> +	struct intel_shared_dpll *(*get_crtc_dpll)(struct intel_crtc *crtc);
> +	struct intel_shared_dpll *(*get_encoder_dpll)(struct intel_encoder *encoder);
> +
> +	void (*map_to_crtc)(struct intel_shared_dpll *dpll,
> +			    struct intel_crtc *crtc);
> +	void (*map_to_encoder)(struct intel_shared_dpll *dpll,
> +			       struct intel_encoder *encoder);
> +
>  	void (*dump_hw_state)(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state);
>  };
> @@ -1886,6 +2109,10 @@ static const struct dpll_info pch_plls[] = {
>  static const struct intel_dpll_mgr pch_pll_mgr = {
>  	.dpll_info = pch_plls,
>  	.get_dpll = ibx_get_dpll,
> +	.get_crtc_dpll = ibx_get_crtc_dpll,
> +	.map_to_crtc = ibx_dpll_map_to_crtc,
> +	.get_encoder_dpll = noop_get_encoder_dpll,
> +	.map_to_encoder = noop_dpll_map_to_encoder,
>  	.dump_hw_state = ibx_dump_hw_state,
>  };
>  
> @@ -1902,6 +2129,10 @@ static const struct dpll_info hsw_plls[] = {
>  static const struct intel_dpll_mgr hsw_pll_mgr = {
>  	.dpll_info = hsw_plls,
>  	.get_dpll = hsw_get_dpll,
> +	.get_crtc_dpll = noop_get_crtc_dpll,
> +	.map_to_crtc = noop_dpll_map_to_crtc,
> +	.get_encoder_dpll = hsw_get_encoder_dpll,
> +	.map_to_encoder = hsw_dpll_map_to_encoder,
>  	.dump_hw_state = hsw_dump_hw_state,
>  };
>  
> @@ -1916,6 +2147,10 @@ static const struct dpll_info skl_plls[] = {
>  static const struct intel_dpll_mgr skl_pll_mgr = {
>  	.dpll_info = skl_plls,
>  	.get_dpll = skl_get_dpll,
> +	.get_crtc_dpll = noop_get_crtc_dpll,
> +	.map_to_crtc = noop_dpll_map_to_crtc,
> +	.get_encoder_dpll = skl_get_encoder_dpll,
> +	.map_to_encoder = skl_dpll_map_to_encoder,
>  	.dump_hw_state = skl_dump_hw_state,
>  };
>  
> @@ -1929,6 +2164,10 @@ static const struct dpll_info bxt_plls[] = {
>  static const struct intel_dpll_mgr bxt_pll_mgr = {
>  	.dpll_info = bxt_plls,
>  	.get_dpll = bxt_get_dpll,
> +	.get_crtc_dpll = noop_get_crtc_dpll,
> +	.map_to_crtc = noop_dpll_map_to_crtc,
> +	.get_encoder_dpll = bxt_get_encoder_dpll,
> +	.map_to_encoder = bxt_dpll_map_to_encoder,
>  	.dump_hw_state = bxt_dump_hw_state,
>  };
>  
> @@ -2031,6 +2270,66 @@ void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
>  }
>  
>  /**
> + * intel_get_crtc_dpll - Get which pll is mapped to a pipe
> + * @crtc: pipe
> + *
> + * Returns the DPLL is mapped to @crtc. NULL indicates no DPLL is mapped to
> + * @crtc.
> + */
> +struct intel_shared_dpll *intel_get_crtc_dpll(struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	return dev_priv->dpll_mgr->get_crtc_dpll(crtc);
> +}
> +
> +/**
> + * intel_get_encoder_dpll - Get which pll is mapped to an encoder
> + * @encoder: encoder
> + *
> + * Returns the DPLL is mapped to @encoder. NULL indicates no DPLL is mapped to
> + * @encoder.
> + */
> +struct intel_shared_dpll *intel_get_encoder_dpll(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +
> +	return dev_priv->dpll_mgr->get_encoder_dpll(encoder);
> +}
> +
> +/**
> + * intel_dpll_map_to_crtc - Map DPLL to pipe
> + * @dpll: dpll to map
> + * @crtc: pipe to map the @dpll to
> + *
> + * For platforms where DPLLs are mapped to crtcs, program the appropriate
> + * bits so that @dpll is mapped to @crtc.
> + */
> +void intel_dpll_map_to_crtc(struct intel_shared_dpll *dpll,
> +			    struct intel_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> +	dev_priv->dpll_mgr->map_to_crtc(dpll, crtc);
> +}
> +
> +/**
> + * intel_dpll_map_to_encoder - Map DPLL to encoder
> + * @dpll: dpll to map
> + * @encoder: encoder to map the @dpll to
> + *
> + * For platforms where DPLLs are mapped to encoders, program the appropriate
> + * bits so that @dpll is mapped to @encoder.
> + */
> +void intel_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
> +			       struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +
> +	dev_priv->dpll_mgr->map_to_encoder(dpll, encoder);
> +}
> +
> +/**
>   * intel_shared_dpll_dump_hw_state - write hw_state to dmesg
>   * @dev_priv: i915 drm device
>   * @hw_state: hw state to be written to the log
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> index 76111a4..7b46c88 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> @@ -280,6 +280,13 @@ void intel_disable_shared_dpll(struct intel_crtc *crtc);
>  void intel_shared_dpll_swap_state(struct drm_atomic_state *state);
>  void intel_shared_dpll_init(struct drm_device *dev);
>  
> +struct intel_shared_dpll *intel_get_crtc_dpll(struct intel_crtc *crtc);
> +struct intel_shared_dpll *intel_get_encoder_dpll(struct intel_encoder *encoder);
> +void intel_dpll_map_to_crtc(struct intel_shared_dpll *dpll,
> +			    struct intel_crtc *crtc);
> +void intel_dpll_map_to_encoder(struct intel_shared_dpll *dpll,
> +			       struct intel_encoder *encoder);
> +
>  void intel_dpll_dump_hw_state(struct drm_i915_private *dev_priv,
>  			      struct intel_dpll_hw_state *hw_state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7874f66..a04d917 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1136,8 +1136,6 @@ void intel_crt_init(struct drm_device *dev);
>  void intel_crt_reset(struct drm_encoder *encoder);
>  
>  /* intel_ddi.c */
> -void intel_ddi_clk_select(struct intel_encoder *encoder,
> -			  struct intel_shared_dpll *pll);
>  void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder,
>  				struct intel_crtc_state *old_crtc_state,
>  				struct drm_connector_state *old_conn_state);
> @@ -1145,6 +1143,8 @@ void intel_prepare_dp_ddi_buffers(struct intel_encoder *encoder);
>  void hsw_fdi_link_train(struct drm_crtc *crtc);
>  void intel_ddi_init(struct drm_device *dev, enum port port);
>  enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder);
> +struct intel_encoder *
> +intel_ddi_get_port_encoder(struct drm_i915_private *dev_priv, enum port port);
>  bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe);
>  void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc);
>  void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Ander Conselvan De Oliveira]

On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
> On Tue, Oct 04, 2016 at 03:32:15PM +0300, Ander Conselvan de Oliveira wrote:
> > 
> > The documentation for most of the non-static members and structs were
> > missing. Fix that.
> > 
> > v2: Fix typos (Durga)
> > 
> > v3: Rebase.
> >     Fix make docs warnings.
> >     Document more.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> As mentioned in an earlier patch, I think we should also move
> intel_atomic_get_shared_dpll_state from intel_atomic.c into
> intel_dpll_mgr.c. Grouping functions by the data structures they touch
> makes imo much more sense, than grouping them by topic. That way all the
> dpll stuff is in one place.
> 
> > 
> > ---
> >  Documentation/gpu/i915.rst            |  12 +++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  90 ++++++++++++++++++--
> >  drivers/gpu/drm/i915/intel_dpll_mgr.h | 155 ++++++++++++++++++++++++++++++-
> > ---
> >  3 files changed, 237 insertions(+), 20 deletions(-)
> > 
> > diff --git a/Documentation/gpu/i915.rst b/Documentation/gpu/i915.rst
> > index 87aaffc..c19e437 100644
> > --- a/Documentation/gpu/i915.rst
> > +++ b/Documentation/gpu/i915.rst
> > @@ -204,6 +204,18 @@ Video BIOS Table (VBT)
> >  .. kernel-doc:: drivers/gpu/drm/i915/intel_vbt_defs.h
> >     :internal:
> >  
> > +Display PLLs
> > +------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +   :doc: Display PLLs
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +   :internal:
> > +
> >  Memory Management and Command Submission
> >  ========================================
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 9446446..8c4efa9 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -23,6 +23,25 @@
> >  
> >  #include "intel_drv.h"
> >  
> > +/**
> > + * DOC: Display PLLs
> > + *
> > + * Display PLLs used for driving outputs vary by platform. While some have
> > + * per-pipe or per-encoder dedicated PLLs, others allow the use of any PLL
> > + * from a pool. In the latter scenario, it is possible that multiple pipes
> > + * share a PLL if their configurations match.
> > + *
> > + * This file provides an abstraction over display PLLs. The function
> > + * intel_shared_dpll_init() initializes the PLLs for the given
> > platform.  The
> > + * users of a PLL are tracked and that tracking is integrated with the
> > atomic
> > + * modest interface. During an atomic operation, a PLL can be requested for
> > a
> > + * given crtc and encoder configuration by calling intel_get_shared_dpll()
> > and
> s/crtc/CRTC/ for consistency (abbreviations are all upercase), pls do that
> on the entire doc.
> 
> > 
> > + * a previously used PLL can be released with intel_release_shared_dpll().
> > + * Changes to the users are first staged in the atomic state, and then made
> > + * effective by calling intel_shared_dpll_swap_state() during the atomic
> > + * commit phase.
> > + */
> > +
> >  struct intel_shared_dpll *
> >  skl_find_link_pll(struct drm_i915_private *dev_priv, int clock)
> >  {
> > @@ -61,6 +80,14 @@ skl_find_link_pll(struct drm_i915_private *dev_priv, int
> > clock)
> >  	return pll;
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll_by_id - get a DPLL given its id
> > + * @dev_priv: i915 device instance
> > + * @id: pll id
> > + *
> > + * Returns:
> > + * A pointer to the DPLL with @id
> > + */
> >  struct intel_shared_dpll *
> >  intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
> >  			    enum intel_dpll_id id)
> > @@ -68,6 +95,14 @@ intel_get_shared_dpll_by_id(struct drm_i915_private
> > *dev_priv,
> >  	return &dev_priv->shared_dplls[id];
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll_id - get the id of a DPLL
> > + * @dev_priv: i915 device instance
> > + * @pll: the DPLL
> > + *
> > + * Returns:
> > + * The id of @pll
> > + */
> >  enum intel_dpll_id
> >  intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
> >  			 struct intel_shared_dpll *pll)
> > @@ -96,6 +131,13 @@ void assert_shared_dpll(struct drm_i915_private
> > *dev_priv,
> >  			pll->name, onoff(state), onoff(cur_state));
> >  }
> >  
> > +/**
> > + * intel_prepare_shared_dpll - call a dpll's prepare hook
> > + * @crtc: crtc which has a shared dpll
> > + *
> > + * This calls the PLL's prepare hook if it has one and if the PLL is not
> > + * already enabled. The prepare hook is platform specific.
> > + */
> >  void intel_prepare_shared_dpll(struct intel_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > @@ -118,12 +160,10 @@ void intel_prepare_shared_dpll(struct intel_crtc
> > *crtc)
> >  }
> >  
> >  /**
> > - * intel_enable_shared_dpll - enable PCH PLL
> > - * @dev_priv: i915 private structure
> > - * @pipe: pipe PLL to enable
> > + * intel_enable_shared_dpll - enable a crtc's shared DPLL
> > + * @crtc: crtc which has a shared DPLL
> >   *
> > - * The PCH PLL needs to be enabled before the PCH transcoder, since it
> > - * drives the transcoder clock.
> > + * Enable the shared DPLL used by @crtc.
> >   */
> >  void intel_enable_shared_dpll(struct intel_crtc *crtc)
> >  {
> > @@ -164,6 +204,12 @@ out:
> >  	mutex_unlock(&dev_priv->dpll_lock);
> >  }
> >  
> > +/**
> > + * intel_disable_shared_dpll - disable a crtc's shared DPLL
> > + * @crtc: crtc which has a shared DPLL
> > + *
> > + * Disable the shared DPLL used by @crtc.
> > + */
> >  void intel_disable_shared_dpll(struct intel_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > @@ -266,6 +312,16 @@ intel_reference_shared_dpll(struct intel_shared_dpll
> > *pll,
> >  	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
> >  }
> >  
> > +/**
> > + * intel_shared_dpll_swap_state - make atomic DPLL configuration effective
> > + * @state: atomic state
> > + *
> > + * This is the dpll version of drm_atomic_helper_swap_state() since the
> > + * helper does not handle driver-specific global state.
> > + *
> > + * Note that this doesn't actually swap states, the dpll configutation in
> > + * @state is left unchanged.
> Hm, what do you mean with that? I guess you mean that it only puts the
> state from drm_atomic_state into dev_priv, and not the other way round.
> Tbh that's a bit unexpected (yes atm we don't have a reason for that), so
> instead of documenting this oddity I'd just fix it. And then maybe mention
> that "For consistency with atomic helpers this also puts the current state
> into @state, i.e. it does a complete swap, even though right now that's
> not needed."
> 
> > 
> > + */
> >  void intel_shared_dpll_swap_state(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > @@ -1822,6 +1878,12 @@ static const struct intel_dpll_mgr bxt_pll_mgr = {
> >  	.get_dpll = bxt_get_dpll,
> >  };
> >  
> > +/**
> > + * intel_shared_dpll_init - Initialize shared DPLLs
> > + * @dev: drm device
> > + *
> > + * Initialize shared DPLLs for @dev.
> > + */
> >  void intel_shared_dpll_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -1865,6 +1927,21 @@ void intel_shared_dpll_init(struct drm_device *dev)
> >  		intel_ddi_pll_init(dev);
> >  }
> >  
> > +/**
> > + * intel_get_shared_dpll - get a shared DPLL for crtc and encoder
> > combination
> > + * @crtc: crtc
> > + * @crtc_state: atomic state for @crtc
> > + * @encoder: encoder
> > + *
> > + * Find an appropriate DPLL for the given crtc and encoder combination. A
> > + * reference from the @crtc to the returned pll is registered in the atomic
> > + * state. That configuration is made effective by calling
> > + * intel_shared_dpll_swap_state(). The reference should be released by
> > calling
> > + * intel_release_shared_dpll().
> > + *
> > + * Returns:
> > + * A shared DPLL to be used by @crtc and @encoder with the given
> > @crtc_state.
> > + */
> >  struct intel_shared_dpll *
> >  intel_get_shared_dpll(struct intel_crtc *crtc,
> >  		      struct intel_crtc_state *crtc_state,
> > @@ -1885,6 +1962,9 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
> >   * @crtc: crtc
> >   * @state: atomic state
> >   *
> > + * This function releases the reference from @crtc to @dpll from the
> > + * atomic @state. The new configuration is made effective by calling
> > + * intel_shared_dpll_swap_state().
> >   */
> >  void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
> >  			       struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > index 9a7db65..40f1a6f 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
> > @@ -40,32 +40,72 @@ struct intel_encoder;
> >  struct intel_shared_dpll;
> >  struct intel_dpll_mgr;
> >  
> > +/**
> > + * enum intel_dpll_id - possible DPLL ids
> > + *
> > + * Enumeration of possible IDs for a DPLL. Real shared dpll ids must be >=
> > 0.
> > + */
> >  enum intel_dpll_id {
> > -	DPLL_ID_PRIVATE = -1, /* non-shared dpll in use */
> > -	/* real shared dpll ids must be >= 0 */
> > +	/**
> > +	 * @DPLL_ID_PRIVATE: non-shared dpll in use
> > +	 */
> > +	DPLL_ID_PRIVATE = -1,
> > +
> > +	/**
> > +	 * @DPLL_ID_PCH_PLL_A: DPLL A in ILK, SNB and IVB
> > +	 */
> >  	DPLL_ID_PCH_PLL_A = 0,
> > +	/**
> > +	 * @DPLL_ID_PCH_PLL_B: DPLL B in ILK, SNB and IVB
> > +	 */
> >  	DPLL_ID_PCH_PLL_B = 1,
> > -	/* hsw/bdw */
> > +
> > +
> > +	/**
> > +	 * @DPLL_ID_WRPLL1: HSW and BDW WRPLL1
> > +	 */
> >  	DPLL_ID_WRPLL1 = 0,
> > +	/**
> > +	 * @DPLL_ID_WRPLL2: HSW and BDW WRPLL2
> > +	 */
> >  	DPLL_ID_WRPLL2 = 1,
> > +	/**
> > +	 * @DPLL_ID_SPLL: HSW and BDW SPLL
> > +	 */
> >  	DPLL_ID_SPLL = 2,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_810: HSW and BDW 0.81 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_810 = 3,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_1350: HSW and BDW 1.35 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_1350 = 4,
> > +	/**
> > +	 * @DPLL_ID_LCPLL_2700: HSW and BDW 2.7 GHz LCPLL
> > +	 */
> >  	DPLL_ID_LCPLL_2700 = 5,
> >  
> > -	/* skl */
> > +
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL0: SKL and later DPLL0
> > +	 */
> >  	DPLL_ID_SKL_DPLL0 = 0,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL1: SKL and later DPLL1
> > +	 */
> >  	DPLL_ID_SKL_DPLL1 = 1,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL2: SKL and later DPLL2
> > +	 */
> >  	DPLL_ID_SKL_DPLL2 = 2,
> > +	/**
> > +	 * @DPLL_ID_SKL_DPLL3: SKL and later DPLL3
> > +	 */
> >  	DPLL_ID_SKL_DPLL3 = 3,
> >  };
> >  #define I915_NUM_PLLS 6
> >  
> > -/** Inform the state checker that the DPLL is kept enabled even if not
> > - * in use by any crtc.
> > - */
> > -#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> > -
> >  struct intel_dpll_hw_state {
> >  	/* i9xx, pch plls */
> >  	uint32_t dpll;
> > @@ -93,39 +133,124 @@ struct intel_dpll_hw_state {
> >  		 pcsdw12;
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll_state - hold the DPLL atomic state
> > + *
> > + * This structure holds an atomic state for the DPLL, that can represent
> > + * either its current state or a desired furture state which would be
> > + * applied by an atomic mode set.
> I think it'd be good to reference where we store pointers to that, i.e.
> where in intel_atomic_state and intel_shared_dpll it sits. Best if you do that
> using the struct &intel_atomic_state reference style (so that it becomes a
> hyperlink once that's documented).
> 
> Also links to the main functions used to manage this would be good I
> think, i.e. release and get.
> 
> > 
> > + */
> >  struct intel_shared_dpll_state {
> > -	unsigned crtc_mask; /* mask of CRTCs sharing this PLL */
> > +	/**
> > +	 * @crtc_mask: mask of crtc using this DPLL, active or not
> s/crtc/CRTCs/
> 
> > 
> > +	 */
> > +	unsigned crtc_mask;
> > +
> > +	/**
> > +	 * @hw_state: hardware configuration for the DPLL.
> "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)

I'll add that, but I think it's silly. The type of the field is struct
intel_dpll_hw_state, so I think it would be more natural if the documentation
tool would add that link automatically.


> > 
> > +	 */
> >  	struct intel_dpll_hw_state hw_state;
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll_funcs - platform specific hooks for managing
> > DPLLs
> > + */
> >  struct intel_shared_dpll_funcs {
> > -	/* The mode_set hook is optional and should be used together with
> > the
> > -	 * intel_prepare_shared_dpll function. */
> > +	/**
> > +	 * @prepare:
> > +	 *
> > +	 * Optional hook to perform operations prior to enabling the PLL.
> > +	 * Called from intel_prepare_shared_dpll() function.
> Missing the language about "if the pll is not already enabled."
> 
> > 
> > +	 */
> >  	void (*prepare)(struct drm_i915_private *dev_priv,
> >  			 struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @enable:
> > +	 *
> > +	 * Hook for enabling the pll, called from
> > intel_enable_shared_dpll()
> > +	 * if the pll is not already enabled.
> > +	 */
> >  	void (*enable)(struct drm_i915_private *dev_priv,
> >  		       struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @disable:
> > +	 *
> > +	 * Hook for disabling the pll, called from
> > intel_disable_shared_dpll()
> > +	 * only when it is safe to disable the pll, i.e., there are no more
> > +	 * tracked users for it.
> > +	 */
> >  	void (*disable)(struct drm_i915_private *dev_priv,
> >  			struct intel_shared_dpll *pll);
> > +
> > +	/**
> > +	 * @get_hw_state:
> > +	 *
> > +	 * Hook for reading the values currently programmed to the DPLL
> > +	 * registers. This is used for initial hw state readout and state
> > +	 * verification after a mode set.
> > +	 */
> >  	bool (*get_hw_state)(struct drm_i915_private *dev_priv,
> >  			     struct intel_shared_dpll *pll,
> >  			     struct intel_dpll_hw_state *hw_state);
> >  };
> >  
> > +/**
> > + * struct intel_shared_dpll - display PLL with tracked state and users
> > + */
> >  struct intel_shared_dpll {
> > +	/**
> > +	 * @state:
> > +	 *
> > +	 * Store the state for the pll, including the its hw state
> > +	 * and crtcs using it.
> > +	 */
> >  	struct intel_shared_dpll_state state;
> >  
> > -	unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
> > -	bool on; /* is the PLL actually active? Disabled during modeset */
> > +	/**
> > +	 * @active_mask: mask of active CRTCs (i.e. DPMS on) using this
> > DPLL
> > +	 */
> > +	unsigned active_mask;
> > +
> > +	/**
> > +	 * @on: is the PLL actually active? Disabled during modeset
> > +	 */
> > +	bool on;
> > +
> > +	/**
> > +	 * @name: DPLL name; used for logging
> > +	 */
> >  	const char *name;
> > -	/* should match the index in the dev_priv->shared_dplls array */
> > +
> > +	/**
> > +	 * @id: unique indentifier for this DPLL; should match the index in
> > the
> > +	 * dev_priv->shared_dplls array
> > +	 */
> >  	enum intel_dpll_id id;
> >  
> > +	/**
> > +	 * @funcs: platform specific hooks
> > +	 */
> >  	struct intel_shared_dpll_funcs funcs;
> >  
> > +	/**
> > +	 * @flags:
> > +	 *
> > +	 * accepted flags: INTEL_DPLL_ALWAYS_ON
> Hm, I've started to just document flags in-line as an enumeration, to keep
> things tightly grouped. And then also place the #define right in front of
> the kernel-doc for @flags. See e.g. @flags in drm_property.h for a really
> long example of that (but there the flags are in an uabi header, so a bit
> special).

Like this?

struct intel_shared_dpll {

...

#define INTEL_DPLL_ALWAYS_ON    (1 << 0)
        /**
         * @flags:
         *
         * INTEL_DPLL_ALWAYS_ON
         *     Inform the state checker that the DPLL is kept enabled even if
         *     not in use by any CRTC.
         */
        uint32_t flags;
};


Ander

> 
> With the nitpicks addressed somehow:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > 
> > +	 */
> >  	uint32_t flags;
> >  };
> >  
> > +/**
> > + * INTEL_DPLL_ALWAYS_ON
> > + *
> > + * Inform the state checker that the DPLL is kept enabled even if not
> > + * in use by any crtc.
> > + */
> > +#define INTEL_DPLL_ALWAYS_ON	(1 << 0)
> > +
> > +
> >  #define SKL_DPLL0 0
> >  #define SKL_DPLL1 1
> >  #define SKL_DPLL2 2
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Jani Nikula]

On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
>> > +	/**
>> > +	 * @hw_state: hardware configuration for the DPLL.
>> "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)
>
> I'll add that, but I think it's silly. The type of the field is struct
> intel_dpll_hw_state, so I think it would be more natural if the documentation
> tool would add that link automatically.

Agreed.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Daniel Vetter]

On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote:
> On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
> > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
> >> > +	/**
> >> > +	 * @hw_state: hardware configuration for the DPLL.
> >> "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)
> >
> > I'll add that, but I think it's silly. The type of the field is struct
> > intel_dpll_hw_state, so I think it would be more natural if the documentation
> > tool would add that link automatically.
> 
> Agreed.

Someone volunteering? I'd hope it would be at most a bit of shuffling with
the generator, we should have the type and all that handy already. Except
maybe lots of corner-cases ...

And ack on the struct layout bikeshed I had.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Jani Nikula]

On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote:
>> On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote:
>> > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
>> >> > +	/**
>> >> > +	 * @hw_state: hardware configuration for the DPLL.
>> >> "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-)
>> >
>> > I'll add that, but I think it's silly. The type of the field is struct
>> > intel_dpll_hw_state, so I think it would be more natural if the documentation
>> > tool would add that link automatically.
>> 
>> Agreed.
>
> Someone volunteering? I'd hope it would be at most a bit of shuffling with
> the generator, we should have the type and all that handy already. Except
> maybe lots of corner-cases ...

I think the problem was that I couldn't figure out how to make Sphinx do
cross references within inline preformatted text. And I thought that was
less important than fixing up the struct presentation that I'm not all
too happy about currently. See [1] first. I don't think we need to have
both the definition and members. It's just wasted vertical space.

I'd suggest we drop the definition altogether, and have the members list
contain the member types, ideally with cross-references. If that means
having to use normal font instead of monospace, I'd go with it anyway.

Thoughts?

BR,
Jani.


[1] https://01.org/linuxgraphics/gfx-docs/drm/driver-api/infrastructure.html#c.device_driver

>
> And ack on the struct layout bikeshed I had.
> -Daniel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Ander Conselvan De Oliveira]

On Thu, 2016-10-20 at 11:19 +0300, Jani Nikula wrote:
> On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote:
> > > 
> > > On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com>
> > > wrote:
> > > > 
> > > > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > > 
> > > > > > +	/**
> > > > > > +	 * @hw_state: hardware configuration for the DPLL.
> > > > > "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-
> > > > > )
> > > > I'll add that, but I think it's silly. The type of the field is struct
> > > > intel_dpll_hw_state, so I think it would be more natural if the
> > > > documentation
> > > > tool would add that link automatically.
> > > Agreed.
> > Someone volunteering? I'd hope it would be at most a bit of shuffling with
> > the generator, we should have the type and all that handy already. Except
> > maybe lots of corner-cases ...
> I think the problem was that I couldn't figure out how to make Sphinx do
> cross references within inline preformatted text. And I thought that was
> less important than fixing up the struct presentation that I'm not all
> too happy about currently. See [1] first. I don't think we need to have
> both the definition and members. It's just wasted vertical space.
> 
> I'd suggest we drop the definition altogether, and have the members list
> contain the member types, ideally with cross-references. If that means
> having to use normal font instead of monospace, I'd go with it anyway.
> 
> Thoughts?

I think that makes sense. There's no extra information there (and if you forget
to add a kerneldoc tag to a field, it isn't even listed). The definition is in
the code anyway.

I was actually a bit surprised to see the definition in the doc the first time.

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

Re: [PATCH 5/7] drm/i915: Update kerneldoc for intel_dpll_mgr.c

[Daniel Vetter]

On Thu, Oct 20, 2016 at 11:56:07AM +0300, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-10-20 at 11:19 +0300, Jani Nikula wrote:
> > On Thu, 20 Oct 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > 
> > > On Wed, Oct 19, 2016 at 06:29:13PM +0300, Jani Nikula wrote:
> > > > 
> > > > On Wed, 19 Oct 2016, Ander Conselvan De Oliveira <conselvan2@gmail.com>
> > > > wrote:
> > > > > 
> > > > > On Thu, 2016-10-13 at 15:46 +0200, Daniel Vetter wrote:
> > > > > > 
> > > > > > > 
> > > > > > > +	/**
> > > > > > > +	 * @hw_state: hardware configuration for the DPLL.
> > > > > > "... stored in struct &intel_dpll_hw_state." - I love my hyperlinks ;-
> > > > > > )
> > > > > I'll add that, but I think it's silly. The type of the field is struct
> > > > > intel_dpll_hw_state, so I think it would be more natural if the
> > > > > documentation
> > > > > tool would add that link automatically.
> > > > Agreed.
> > > Someone volunteering? I'd hope it would be at most a bit of shuffling with
> > > the generator, we should have the type and all that handy already. Except
> > > maybe lots of corner-cases ...
> > I think the problem was that I couldn't figure out how to make Sphinx do
> > cross references within inline preformatted text. And I thought that was
> > less important than fixing up the struct presentation that I'm not all
> > too happy about currently. See [1] first. I don't think we need to have
> > both the definition and members. It's just wasted vertical space.
> > 
> > I'd suggest we drop the definition altogether, and have the members list
> > contain the member types, ideally with cross-references. If that means
> > having to use normal font instead of monospace, I'd go with it anyway.
> > 
> > Thoughts?
> 
> I think that makes sense. There's no extra information there (and if you forget
> to add a kerneldoc tag to a field, it isn't even listed). The definition is in
> the code anyway.
> 
> I was actually a bit surprised to see the definition in the doc the first time.

Assuming we're talking just about structs here: +1. For functions we
already have the signature in the heading, and that also already
hyperlinks.

There's also the difference that the detailed list has the full type+name,
whereas structs only have the name. I like the style used in functions
much more (and then we could just nuke the definition I think), and then
hyperlink them properly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915: Introduce intel_release_shared_dpll()

[Ander Conselvan de Oliveira]

While the details of getting a shared dpll are wrapped by
intel_get_shared_dpll(), the release was still hand rolled into the
modeset code. Fix that by creating an entry point for releasing the
pll and move that code there.

v2: Take old_dpll from crtc->state instead of crtc_state. (CI)
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c  |  6 +----
 drivers/gpu/drm/i915/intel_dpll_mgr.c | 41 +++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_dpll_mgr.h | 11 +++-------
 3 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d8effd4..55e1e88 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13874,7 +13874,6 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_shared_dpll_config *shared_dpll = NULL;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
@@ -13895,10 +13894,7 @@ static void intel_modeset_clear_plls(struct drm_atomic_state *state)
 		if (!old_dpll)
 			continue;
 
-		if (!shared_dpll)
-			shared_dpll = intel_atomic_get_shared_dpll_state(state);
-
-		intel_shared_dpll_config_put(shared_dpll, old_dpll, intel_crtc);
+		intel_release_shared_dpll(old_dpll, intel_crtc, state);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index 97f7cc9..1fa32f2 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -79,28 +79,6 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 	return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
 }
 
-void
-intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
-
-	config[id].crtc_mask |= 1 << crtc->pipe;
-}
-
-void
-intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	enum intel_dpll_id id = intel_get_shared_dpll_id(dev_priv, pll);
-
-	config[id].crtc_mask &= ~(1 << crtc->pipe);
-}
-
 /* For ILK+ */
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll,
@@ -284,7 +262,7 @@ intel_reference_shared_dpll(struct intel_shared_dpll *pll,
 	DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
 			 pipe_name(crtc->pipe));
 
-	intel_shared_dpll_config_get(shared_dpll, pll, crtc);
+	shared_dpll[pll->id].crtc_mask |= 1 << crtc->pipe;
 }
 
 void intel_shared_dpll_commit(struct drm_atomic_state *state)
@@ -1933,3 +1911,20 @@ intel_get_shared_dpll(struct intel_crtc *crtc,
 
 	return dpll_mgr->get_dpll(crtc, crtc_state, encoder);
 }
+
+/**
+ * intel_release_shared_dpll - end use of DPLL by CRTC in atomic state
+ * @dpll: dpll in use by @crtc
+ * @crtc: crtc
+ * @state: atomic state
+ *
+ */
+void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
+			       struct intel_crtc *crtc,
+			       struct drm_atomic_state *state)
+{
+	struct intel_shared_dpll_config *shared_dpll_config;
+
+	shared_dpll_config = intel_atomic_get_shared_dpll_state(state);
+	shared_dpll_config[dpll->id].crtc_mask &= ~(1 << crtc->pipe);
+}
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.h b/drivers/gpu/drm/i915/intel_dpll_mgr.h
index f438535..99a82c9 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.h
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.h
@@ -138,14 +138,6 @@ intel_get_shared_dpll_by_id(struct drm_i915_private *dev_priv,
 enum intel_dpll_id
 intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 			 struct intel_shared_dpll *pll);
-void
-intel_shared_dpll_config_get(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc);
-void
-intel_shared_dpll_config_put(struct intel_shared_dpll_config *config,
-			     struct intel_shared_dpll *pll,
-			     struct intel_crtc *crtc);
 void assert_shared_dpll(struct drm_i915_private *dev_priv,
 			struct intel_shared_dpll *pll,
 			bool state);
@@ -154,6 +146,9 @@ void assert_shared_dpll(struct drm_i915_private *dev_priv,
 struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc,
 						struct intel_crtc_state *state,
 						struct intel_encoder *encoder);
+void intel_release_shared_dpll(struct intel_shared_dpll *dpll,
+			       struct intel_crtc *crtc,
+			       struct drm_atomic_state *state);
 void intel_prepare_shared_dpll(struct intel_crtc *crtc);
 void intel_enable_shared_dpll(struct intel_crtc *crtc);
 void intel_disable_shared_dpll(struct intel_crtc *crtc);
-- 
2.5.5

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