Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

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.