diff mbox

[2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

Message ID 1344918891-6283-3-git-send-email-keithp@keithp.com
State New, archived
Headers show

Commit Message

Keith Packard Aug. 14, 2012, 4:34 a.m. UTC
IVB shares 4 lanes between FDI B and FDI C. When sharing, compute the
maximum BPC based on the available bandwidth.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |  101 +++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 7 deletions(-)

Comments

Lespiau, Damien Aug. 17, 2012, 2:45 p.m. UTC | #1
On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:

@@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
drm_i915_private *dev_priv)
  */
 static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
                                         unsigned int *pipe_bpp,
-                                        struct drm_display_mode *mode)
+                                        struct drm_display_mode *mode,
+                                        int max_fdi_bpp)

There's some kernel-doc for this function, maybe add a @max_fdi_bpp there?


> @@ -3800,6 +3801,15 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                 display_bpc = 6;
>         }
>
> +       if (display_bpc * 3 > max_fdi_bpp) {
> +               if (max_fdi_bpp < 24)
> +                       display_bpc = 6;
> +               else if (max_fdi_bpp < 30)
> +                       display_bpc = 8;
> +               else if (max_fdi_bpp < 36)
> +                       display_bpc = 10;
> +               DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
> +       }

This chunk is being moved around in a later patch in the series,
merging the two patches in one looks like a good idea?


> +       /* If the other FDI link is using too many lanes, we can't have
> +        * any
> +        */
> +       if (other_intel_crtc->fdi_lanes > 2)
> +               return 0;
> +
> +       /* When both are running, we only get 2 lanes at most
> +        */
> +       return 2;
> +}

I guess this does not cover the case of pipe B using 3 lanes meaning
pipe C can use 1?

> @@ -4672,7 +4731,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>            according to current link config */
>         if (is_cpu_edp) {
>                 intel_edp_link_config(edp_encoder, &lane, &link_bw);
> +               max_fdi_bpp = 0;
> +               max_lane = lane;
>         } else {
> +               u32     fdi_bw;
> +
> +               /* [e]DP over FDI requires target mode clock
> +                  instead of link clock */
> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;
> +

This duplicates the code just that is just a few lines away, instead
how about moving the logic to set target_clock up in front of this
whole if()?

>                 /* FDI is a binary signal running at ~2.7GHz, encoding
>                  * each output octet as 10 bits. The actual frequency
>                  * is stored as a divider into a 100MHz clock, and the
> @@ -4681,6 +4751,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                  * is:
>                  */
>                 link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +
> +               max_lane = 4;
> +               if (IS_IVYBRIDGE(dev))
> +                       max_lane = ivb_fdi_max_lanes(crtc);
> +
> +               /*
> +                * Compute the available FDI bandwidth, use that
> +                * to compute the maximum supported BPP
> +                */
> +               fdi_bw = link_bw * max_lane * 19 / 20;
> +               max_fdi_bpp = fdi_bw / target_clock;
> +               DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
>         }

This chunk is also reworked in a later commit in this series.
Keith Packard Aug. 17, 2012, 3 p.m. UTC | #2
"Lespiau, Damien" <damien.lespiau@intel.com> writes:

> On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
>
> @@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
> drm_i915_private *dev_priv)
>   */
>  static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                                          unsigned int *pipe_bpp,
> -                                        struct drm_display_mode *mode)
> +                                        struct drm_display_mode *mode,
> +                                        int max_fdi_bpp)
>
> There's some kernel-doc for this function, maybe add a @max_fdi_bpp
> there?

Will do

> This chunk is being moved around in a later patch in the series,
> merging the two patches in one looks like a good idea?

Or at least move this into its final position in this patch.

> I guess this does not cover the case of pipe B using 3 lanes meaning
> pipe C can use 1?

It didn't look like that was a supported mode from the docs.

> This duplicates the code just that is just a few lines away, instead
> how about moving the logic to set target_clock up in front of this
> whole if()?

It's not the same, it's the inverse -- computing bpp from lanes+clock
clock instead of computing lanes from bpp+clock. But, yeah, it would be
nice to have these merged somehow. I couldn't figure out a good way though.

> This chunk is also reworked in a later commit in this series.

I'll see if I can't avoid that as it's confusing. Thanks for your review!
Lespiau, Damien Aug. 17, 2012, 3:12 p.m. UTC | #3
On Fri, Aug 17, 2012 at 4:00 PM, Keith Packard <keithp@keithp.com> wrote:
>> I guess this does not cover the case of pipe B using 3 lanes meaning
>> pipe C can use 1?
>
> It didn't look like that was a supported mode from the docs.

Ah yes, found it now "FDI B maximum port width is 4 lanes when FDI C
is disabled, 2 lanes when FDI C is enabled."

>> This duplicates the code just that is just a few lines away, instead
>> how about moving the logic to set target_clock up in front of this
>> whole if()?
>
> It's not the same, it's the inverse -- computing bpp from lanes+clock
> clock instead of computing lanes from bpp+clock. But, yeah, it would be
> nice to have these merged somehow. I couldn't figure out a good way though.

I meant the

> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;

Just after that else block you have

        /* [e]DP over FDI requires target mode clock instead of link clock. */
        if (edp_encoder)
                target_clock = intel_edp_target_clock(edp_encoder, mode);
        else if (is_dp)
                target_clock = mode->clock;
        else
                target_clock = adjusted_mode->clock;

Those look strangely similar to me. The later could be moved up.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70d30fc..7106807 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3575,7 +3575,7 @@  void intel_encoder_destroy(struct drm_encoder *encoder)
 }
 
 static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
-				  struct drm_display_mode *mode,
+				  const struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3728,7 +3728,8 @@  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
  */
 static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 					 unsigned int *pipe_bpp,
-					 struct drm_display_mode *mode)
+					 struct drm_display_mode *mode,
+					 int max_fdi_bpp)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3800,6 +3801,15 @@  static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 		display_bpc = 6;
 	}
 
+	if (display_bpc * 3 > max_fdi_bpp) {
+		if (max_fdi_bpp < 24)
+			display_bpc = 6;
+		else if (max_fdi_bpp < 30)
+			display_bpc = 8;
+		else if (max_fdi_bpp < 36)
+			display_bpc = 10;
+		DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
+	}
 	/*
 	 * We could just drive the pipe at the highest bpc all the time and
 	 * enable dithering as needed, but that costs bandwidth.  So choose
@@ -4570,6 +4580,53 @@  static int ironlake_get_refclk(struct drm_crtc *crtc)
 	return 120000;
 }
 
+/*
+ * FDI C can only have 2 lanes, borrowed from FDI B
+ */
+
+static int ivb_fdi_max_lanes(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe other_pipe;
+	struct drm_crtc *other_crtc;
+	struct intel_crtc *other_intel_crtc;
+	int max_lanes;
+
+	/* FDI links B and C share 4 lanes */
+	switch (intel_crtc->pipe) {
+	case PIPE_B:
+		other_pipe = PIPE_C;
+		max_lanes = 4;
+		break;
+	case PIPE_C:
+		other_pipe = PIPE_B;
+		max_lanes = 2;
+		break;
+	default:
+		return 4;
+	}
+	other_crtc = dev_priv->pipe_to_crtc_mapping[other_pipe];
+	other_intel_crtc = to_intel_crtc(other_crtc);
+
+	/* If the other FDI link isn't running, we can use all of the
+	 * available lanes
+	 */
+	if (!other_intel_crtc->active)
+		return max_lanes;
+
+	/* If the other FDI link is using too many lanes, we can't have
+	 * any
+	 */
+	if (other_intel_crtc->fdi_lanes > 2)
+		return 0;
+
+	/* When both are running, we only get 2 lanes at most
+	 */
+	return 2;
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4595,6 +4652,8 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	unsigned int pipe_bpp;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
+	int max_fdi_bpp;
+	int max_lane;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -4672,7 +4731,18 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	   according to current link config */
 	if (is_cpu_edp) {
 		intel_edp_link_config(edp_encoder, &lane, &link_bw);
+		max_fdi_bpp = 0;
+		max_lane = lane;
 	} else {
+		u32	fdi_bw;
+
+		/* [e]DP over FDI requires target mode clock
+		   instead of link clock */
+		if (is_dp)
+			target_clock = mode->clock;
+		else
+			target_clock = adjusted_mode->clock;
+
 		/* FDI is a binary signal running at ~2.7GHz, encoding
 		 * each output octet as 10 bits. The actual frequency
 		 * is stored as a divider into a 100MHz clock, and the
@@ -4681,6 +4751,18 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		 * is:
 		 */
 		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
+
+		max_lane = 4;
+		if (IS_IVYBRIDGE(dev))
+			max_lane = ivb_fdi_max_lanes(crtc);
+
+		/*
+		 * Compute the available FDI bandwidth, use that
+		 * to compute the maximum supported BPP
+		 */
+		fdi_bw = link_bw * max_lane * 19 / 20;
+		max_fdi_bpp = fdi_bw / target_clock;
+		DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
 	}
 
 	/* [e]DP over FDI requires target mode clock instead of link clock. */
@@ -4694,7 +4776,7 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* determine panel color depth */
 	temp = I915_READ(PIPECONF(pipe));
 	temp &= ~PIPE_BPC_MASK;
-	dither = intel_choose_pipe_bpp_dither(crtc, &pipe_bpp, mode);
+	dither = intel_choose_pipe_bpp_dither(crtc, &pipe_bpp, mode, max_fdi_bpp);
 	switch (pipe_bpp) {
 	case 18:
 		temp |= PIPE_6BPC;
@@ -4716,19 +4798,24 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		break;
 	}
 
-	intel_crtc->bpp = pipe_bpp;
-	I915_WRITE(PIPECONF(pipe), temp);
-
 	if (!lane) {
 		/*
 		 * Account for spread spectrum to avoid
 		 * oversubscribing the link. Max center spread
 		 * is 2.5%; use 5% for safety's sake.
 		 */
-		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
+		u32 bps = target_clock * pipe_bpp * 21 / 20;
 		lane = bps / (link_bw * 8) + 1;
+		if (lane > max_lane) {
+			DRM_ERROR("Not enough lanes available for mode! (want %d have %d)\n",
+				  lane, max_lane);
+			return -EINVAL;
+		}
 	}
 
+	intel_crtc->bpp = pipe_bpp;
+	I915_WRITE(PIPECONF(pipe), temp);
+
 	intel_crtc->fdi_lanes = lane;
 
 	if (pixel_multiplier > 1)