On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp at 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. -- Damien