| Message ID | 1344918891-6283-3-git-send-email-keithp@keithp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
"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!
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 --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)
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(-)