[02/10] drm/i915: fix FDI lane calculation

Commit Message

Paulo Zanoni Nov. 20, 2012, 3:27 p.m. UTC

From: Paulo Zanoni <[e-mail]>

The previous code was making the bps value 5% higher than what the
spec says, which was enough to make certain VGA modes require 3 lanes
instead of 2, which breaks Haswell since it only has 2 FDI lanes. For
previous gens this was not a problem, since requiring more lanes than
the needed is ok, as long as you have all the lanes.

Notice that this might improve the case where we use pipes B and C on
Ivy Bridge since both pipes only have 4 lanes to share (see
ironlake_check_fdi_lanes).

We still need to code to refuse modes requiring more than 2 lanes on
Haswell.

Cc: Adam Jackson <[e-mail]>
Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Lespiau, Damien Nov. 20, 2012, 6:17 p.m. UTC | #1

> -                * Account for spread spectrum to avoid
> -                * oversubscribing the link. Max center spread
> -                * is 2.5%; use 5% for safety's sake.
> +                * The spec says:
> +                * Number of lanes >= INT(dot clock * bytes per pixel / ls_clk)

Right, so the real question is: "Is that ok to not get the spread rate
(maximum of how much we derive from the requested frequency) into
account?"

I believe it is, on average the frequency is what we set-up. Maybe
Adam can shed more light on why he thought it was necessary?

As a side note, the spec does not mention that at all.

Paulo Zanoni Nov. 20, 2012, 6:43 p.m. UTC | #2

Hi

2012/11/20 Damien Lespiau <[e-mail]>:
>> -                * Account for spread spectrum to avoid
>> -                * oversubscribing the link. Max center spread
>> -                * is 2.5%; use 5% for safety's sake.
>> +                * The spec says:
>> +                * Number of lanes >= INT(dot clock * bytes per pixel / ls_clk)
>
> Right, so the real question is: "Is that ok to not get the spread rate
> (maximum of how much we derive from the requested frequency) into
> account?"

Well, the spec does not say we need to do this. Also, I tested this
patch on SNB and some modes that were moved from 3 to 2 lanes still
work.

>
> I believe it is, on average the frequency is what we set-up. Maybe
> Adam can shed more light on why he thought it was necessary?
>
> As a side note, the spec does not mention that at all.
>
> --
> Damien

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0102931..9940765 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5272,14 +5272,15 @@  static void ironlake_set_m_n(struct drm_crtc *crtc,
 
 	if (!lane) {
 		/*
-		 * Account for spread spectrum to avoid
-		 * oversubscribing the link. Max center spread
-		 * is 2.5%; use 5% for safety's sake.
+		 * The spec says:
+		 * Number of lanes >= INT(dot clock * bytes per pixel / ls_clk)
 		 */
-		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
-		lane = bps / (link_bw * 8) + 1;
+		u32 bps = target_clock * intel_crtc->bpp;
+		lane = DIV_ROUND_UP(bps, (link_bw * 8));
 	}
 
+	DRM_DEBUG_KMS("Using %d FDI lanes on pipe %c\n", lane,
+		      pipe_name(intel_crtc->pipe));
 	intel_crtc->fdi_lanes = lane;
 
 	if (pixel_multiplier > 1)