[Intel-gfx] [PATCH 2/8] Review comments.

[Daniel Vetter]

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