On Fri, 2013-04-19 at 11:24 +0200, Daniel Vetter wrote:
> So on a bunch of setups we only have 2 fdi lanes available, e.g. hsw
> VGA or 3 pipes on ivb. And seemingly a lot of modes don't quite fit
> into this, among them the default 1080p mode.
>
> The solution is to dither down the pipe a bit so that everything fits,
> which this patch implements.
>
> But ports compute their state under the assumption that the bpp they
> pick will be the one selected, e.g. the display port bw computations
> won't work otherwise. Now we could adjust our code to again up-dither
> to the computed DP link parameters, but that's pointless.
>
> So instead when the pipe needs to adjust parameters we need to retry
> the pipe_config computation at the encoder stage. Furthermore we need
> to inform encoders that they should not increase bandwidth
> requirements if possible. This is required for the hdmi code, which
> prefers the pipe to up-dither to either of the two possible hdmi bpc
> values.
>
> LVDS has a similar requirement, although that's probably only
> theoretical in nature: It's unlikely that we'll ever see an 8bpc
> high-res lvds panel (which is required to hit the 2 fdi lane limit).
>
> v2: Rebased on top of a bikeshed from Paulo.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> ---
> drivers/gpu/drm/i915/intel_display.c | 56 ++++++++++++++++++++++++++++--------
> drivers/gpu/drm/i915/intel_drv.h | 7 +++++
> drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++---
> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
> 4 files changed, 62 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c25dbdd..6d35ccd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4021,13 +4021,16 @@ static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
> }
> }
>
> -static bool ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> - struct intel_crtc_config *pipe_config)
> +#define RETRY 1
> +static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> + struct intel_crtc_config *pipe_config)
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> int target_clock, lane, link_bw;
> + bool setup_ok, needs_recompute = false;
>
> +retry:
> /* 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
> @@ -4052,12 +4055,26 @@ static bool ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
> link_bw, &pipe_config->fdi_m_n);
>
> - return ironlake_check_fdi_lanes(intel_crtc->base.dev,
> - intel_crtc->pipe, pipe_config);
> + setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> + intel_crtc->pipe, pipe_config);
> + if (!setup_ok && pipe_config->pipe_bpp > 6*3) {
> + pipe_config->pipe_bpp -= 2*3;
> + DRM_DEBUG_KMS("fdi link bw constraint, reducing pipe bpp to %i\n",
> + pipe_config->pipe_bpp);
> + needs_recompute = true;
> + pipe_config->bw_constrained = true;
> +
> + goto retry;
> + }
> +
> + if (needs_recompute)
> + return RETRY;
> +
> + return setup_ok ? 0 : -EINVAL;
> }
>
> -static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> - struct intel_crtc_config *pipe_config)
> +static int intel_crtc_compute_config(struct drm_crtc *crtc,
> + struct intel_crtc_config *pipe_config)
> {
> struct drm_device *dev = crtc->dev;
> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> @@ -4066,7 +4083,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> /* FDI link clock is fixed at 2.7G */
> if (pipe_config->requested_mode.clock * 3
> > IRONLAKE_FDI_FREQ * 4)
> - return false;
> + return -EINVAL;
> }
>
> /* All interlaced capable intel hw wants timings in frames. Note though
> @@ -4080,7 +4097,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> */
> if ((INTEL_INFO(dev)->gen > 4 || IS_G4X(dev)) &&
> adjusted_mode->hsync_start == adjusted_mode->hdisplay)
> - return false;
> + return -EINVAL;
>
> if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10*3) {
> pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
> @@ -4093,7 +4110,7 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
> if (pipe_config->has_pch_encoder)
> return ironlake_fdi_compute_config(to_intel_crtc(crtc), pipe_config);
>
> - return true;
> + return 0;
> }
>
> static int valleyview_get_display_clock_speed(struct drm_device *dev)
> @@ -7673,7 +7690,8 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> struct drm_encoder_helper_funcs *encoder_funcs;
> struct intel_encoder *encoder;
> struct intel_crtc_config *pipe_config;
> - int plane_bpp;
> + int plane_bpp, ret = -EINVAL;
> + bool retry = true;
>
> pipe_config = kzalloc(sizeof(*pipe_config), GFP_KERNEL);
> if (!pipe_config)
> @@ -7686,6 +7704,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> if (plane_bpp < 0)
> goto fail;
>
> +encoder_retry:
> /* Pass our mode to the connectors and the CRTC to give them a chance to
> * adjust it according to limitations or connector properties, and also
> * a chance to reject the mode entirely.
> @@ -7714,10 +7733,23 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> }
> }
>
> - if (!(intel_crtc_compute_config(crtc, pipe_config))) {
> + ret = intel_crtc_compute_config(crtc, pipe_config);
> + if (ret < 0) {
> DRM_DEBUG_KMS("CRTC fixup failed\n");
> goto fail;
> }
> +
> + if (ret == RETRY) {
> + if (WARN(!retry, "loop in pipe configuration computation\n")) {
> + ret = -EINVAL;
> + goto fail;
Isn't it possible that intel_dp_compute_config increases pipe_bpp when
it forces pipe_bpp to what the firmware has set? In that case could hit
this WARN.
> + }
> +
> + DRM_DEBUG_KMS("CRTC bw constrained, retrying\n");
> + retry = false;
> + goto encoder_retry;
> + }
> +
> DRM_DEBUG_KMS("[CRTC:%d]\n", crtc->base.id);
>
> pipe_config->dither = pipe_config->pipe_bpp != plane_bpp;
> @@ -7727,7 +7759,7 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> return pipe_config;
> fail:
> kfree(pipe_config);
> - return ERR_PTR(-EINVAL);
> + return ERR_PTR(ret);
> }
>
> /* Computes which crtcs are affected and sets the relevant bits in the mask. For
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f40b43f..c14afc6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -211,6 +211,13 @@ struct intel_crtc_config {
> /* Controls for the clock computation, to override various stages. */
> bool clock_set;
>
> + /*
> + * crtc bandwidth limit, don't increase pipe bpp or clock if not really
> + * required. This is set in the 2nd loop of calling encoder's
> + * ->compute_config if the first pick doesn't work out.
> + */
> + bool bw_constrained;
> +
> /* Settings for the intel dpll used on pretty much everything but
> * haswell. */
> struct dpll {
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index a8273c7..3942041 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -784,6 +784,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> struct drm_device *dev = encoder->base.dev;
> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> int clock_12bpc = pipe_config->requested_mode.clock * 3 / 2;
> + int desired_bpp;
>
> if (intel_hdmi->color_range_auto) {
> /* See CEA-861-E - 5.1 Default Encoding Parameters */
> @@ -808,16 +809,21 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> */
> if (pipe_config->pipe_bpp > 8*3 && clock_12bpc < 225000
> && HAS_PCH_SPLIT(dev)) {
> - DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
> - pipe_config->pipe_bpp = 12*3;
> + DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> + desired_bpp = 12*3;
>
> /* Need to adjust the port link by 1.5x for 12bpc. */
> adjusted_mode->clock = clock_12bpc;
> pipe_config->pixel_target_clock =
> pipe_config->requested_mode.clock;
> } else {
> - DRM_DEBUG_KMS("forcing bpc to 8 for HDMI\n");
> - pipe_config->pipe_bpp = 8*3;
> + DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> + desired_bpp = 8*3;
> + }
> +
> + if (!pipe_config->bw_constrained) {
> + DRM_DEBUG_KMS("forcing pipe bpc to %i for HDMI\n", desired_bpp);
> + pipe_config->pipe_bpp = desired_bpp;
> }
>
> if (adjusted_mode->clock > 225000) {
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 094f3c5..c426581 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -331,7 +331,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> else
> lvds_bpp = 6*3;
>
> - if (lvds_bpp != pipe_config->pipe_bpp) {
> + if (lvds_bpp != pipe_config->pipe_bpp && !pipe_config->bw_constrained) {
> DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
> pipe_config->pipe_bpp, lvds_bpp);
> pipe_config->pipe_bpp = lvds_bpp;
