Re: [PATCH 15/15] drm/i915: implement fdi auto-dithering

[Imre Deak <imre.deak@intel.com>]

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@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;