drm/i915: Fix issues caused from get_clock elimination

Commit Message

Ville Syrjälä Sept. 6, 2013, 1:24 p.m. UTC

From: Ville Syrjälä <[e-mail]>

For non-PCH encoders ironlake_crtc_clock_get() attempts to extract
adjusted_mode.clock from port_clock. But now that we call
ironlake_crtc_clock_get() before the encoders' get_config() that no
longer works.

To fix the problem also call ironlake_crtc_clock_get() before
get_config() for PCH encoders, and afterwards for non-PCH encoders.
Also be careful not to clobber port_clock when calling it afterwards.

The problem was introduced by:
"drm/i915: Fix port_clock readout for SDVO and HDMI 12bpc cases"

Signed-off-by: Ville Syrjälä <[e-mail]>
---

This could be squashed with the patch that caused the problem in the first
place, but I left it separate for now to solicit feedback on how ugly do
people think this is. I could of course leave the get_clock callback
in place, but only fill it in for ILK+ and add a has_pch_encoder check
there, otherwise we could keep the direct func call in get_pipe_config.

 drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Comments

Daniel Vetter Sept. 6, 2013, 4 p.m. UTC | #1

On Fri, Sep 06, 2013 at 04:24:17PM +0300, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> For non-PCH encoders ironlake_crtc_clock_get() attempts to extract
> adjusted_mode.clock from port_clock. But now that we call
> ironlake_crtc_clock_get() before the encoders' get_config() that no
> longer works.
> 
> To fix the problem also call ironlake_crtc_clock_get() before
> get_config() for PCH encoders, and afterwards for non-PCH encoders.
> Also be careful not to clobber port_clock when calling it afterwards.
> 
> The problem was introduced by:
> "drm/i915: Fix port_clock readout for SDVO and HDMI 12bpc cases"
> 
> Signed-off-by: Ville Syrjälä <[e-mail]>

Plan B, since this is only a special case for the cpu edp encoder where
the port clock is stored in the encoder. What about we move the full
responsibility for computing the clocks for the cpu edp port to the
encoder callback? ironlake_crtc_get_clock would then more correctly just
be a ironlake_get_pch_clocks or so.

Note that the exact same issue happens for hsw ddi ports where also the
port itself selects the port clock (and as usual for dp with the m/n
stuff then the dotclock). So we need a notch more generic solution here
than just a hack.

Cheers, Daniel

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5b8a437..3cfd99b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6040,7 +6040,14 @@  static void ironlake_crtc_clock_get(struct intel_crtc *crtc,
 	 * This value does not include pixel_multiplier. We will fix
 	 * port_clock in the encoder's get_config() function if necessary.
 	 */
-	pipe_config->port_clock = pipe_config->adjusted_mode.clock = clock;
+	pipe_config->adjusted_mode.clock = clock;
+
+	/*
+	 * We get called before encoder get_config() for pch encoders,
+	 * after for non-pch. Don't clobber port_clock in the latter case.
+	 */
+	if (!pipe_config->port_clock)
+		pipe_config->port_clock = clock;
 }
 
 static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
@@ -6105,6 +6112,8 @@  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 		pipe_config->pixel_multiplier =
 			((tmp & PLL_REF_SDVO_HDMI_MULTIPLIER_MASK)
 			 >> PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT) + 1;
+
+		ironlake_crtc_clock_get(crtc, pipe_config);
 	} else {
 		pipe_config->pixel_multiplier = 1;
 	}
@@ -6113,8 +6122,6 @@  static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 
 	ironlake_get_pfit_config(crtc, pipe_config);
 
-	ironlake_crtc_clock_get(crtc, pipe_config);
-
 	return true;
 }
 
@@ -8827,6 +8834,9 @@  check_crtc_state(struct drm_device *dev)
 				encoder->get_config(encoder, &pipe_config);
 		}
 
+		if (HAS_PCH_SPLIT(dev) && !pipe_config.has_pch_encoder)
+			ironlake_crtc_clock_get(crtc, &pipe_config);
+
 		WARN(crtc->active != active,
 		     "crtc active state doesn't match with hw state "
 		     "(expected %i, found %i)\n", crtc->active, active);
@@ -10479,6 +10489,13 @@  static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			      pipe);
 	}
 
+	if (HAS_PCH_SPLIT(dev)) {
+		list_for_each_entry(crtc, &dev->mode_config.crtc_list, base.head) {
+			if (!crtc->config.has_pch_encoder)
+				ironlake_crtc_clock_get(crtc, &crtc->config);
+		}
+	}
+
 	list_for_each_entry(connector, &dev->mode_config.connector_list,
 			    base.head) {
 		if (connector->get_hw_state(connector)) {