Imo extracting vlv_enable_pll should be done now, the other stuff
is imo ok as follow-up (you could keep the comments as FIXME sections).
---
drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 26ef98d..c124dba 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3701,6 +3701,14 @@ static void vlv_pll_pre_enable(struct drm_crtc *crtc)
WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
/* Assume eDP on port C and HDMI/DP on port B */
+
+ /*
+ * Presuming the port macro argument below is indeed the same port the
+ * above comment claims, the code does not match up with reality.
+ *
+ * Second, if you move this into encoder->pre_pll_enable you'll have
+ * access to the port number directly and can dtrt.
+ */
if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) {
/* Program Tx lane resets to default */
intel_dpio_write(dev_priv, DPIO_PCS_TX(0),
@@ -3751,6 +3759,11 @@ static void vlv_pll_post_enable(struct drm_crtc *crtc)
WARN_ON(!mutex_is_locked(&dev_priv->dpio_lock));
/* Assume eDP on port C and HDMI/DP on port B */
+
+ /*
+ * Same comment as for pll_pre_enable about the port confusion. Again I
+ * think this can be moved to the encoder->pre_enable.
+ */
if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) {
/* Enable clock channels for this port */
u32 val;
@@ -3822,6 +3835,19 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
intel_crtc->active = true;
intel_update_watermarks(dev);
+ /*
+ * Two issues with these hunks here:
+ * - it screams for a vlv_enable_pll function
+ * - you call vlv_pll_post/pre_enable twice. On latest dinq we don't
+ * enable anything anymore in i9xx_crtc_mode_set, so there's no reason
+ * any longer to duplicate the pll setup, at least for vlv. i9xx/i8xx
+ * is a different story, since there we still need to deal with the
+ * lvds pre_pll_enable hook. So I'm thinking of moving all the crap in
+ * vlv_update_pll to vlv_enable_pll and only call it from here, plus
+ * adding a call to encoder->pre_pll_enable to vlv_enable_pll, which
+ * would do the vlv_pll_pre_enable stuff (but with the right port
+ * numbers)
+ */
if (IS_VALLEYVIEW(dev)) {
mutex_lock(&dev_priv->dpio_lock);
vlv_pll_pre_enable(crtc);
@@ -4450,6 +4476,8 @@ static void vlv_update_pll(struct intel_crtc *crtc)
mdiv |= ((bestp1 << DPIO_P1_SHIFT) | (bestp2 << DPIO_P2_SHIFT));
mdiv |= ((bestn << DPIO_N_SHIFT));
mdiv |= (1 << DPIO_K_SHIFT);
+ /* You could replace the EDP || DISPLAYPORT checks here and below with
+ * crtc->config.has_dp_encoder. */
if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI) ||
intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_EDP) ||
intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
--
1.7.10.4
