Re: [PATCH 10/10] drm/i915: clean up pipe bpp confusion

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 22 Feb 2013 00:56:54 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> - gen4 and earlier (save for g4x) only really have a 8bpc pipe, with
>   the possibility to dither to 6bpc using the panel fitter
> - g4x has hdmi, but no 12 bpc pipe ... !? Clamp hdmi accordingly.
> - TV/SDVO out are the only connectors available on platforms with
>   a pipe bpp != 8, add code to force the pipe to 8bpc unconditionally.
> 
> <rant>
> The dither handling on gmch platforms is one giant disaster. I'm hoping
> somewhat that vlv enabling will fix this up, but given that the 6bpc
> handling for edp was simply added with another quick hack, I don't have
> high hopes ...
> </rant>
> 
> v2: Neither vlv nor g4x have 12bpc pipes. Still set pipe_bpp to 12*3,
> but let the crtc code clamp things down to 10bpc on these platforms.
> 
> v3: Fix a bpc vs. bpp mixup in the gen4 and earlier pipe_bpp limiter
> code.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_hdmi.c    |  4 +++-
>  drivers/gpu/drm/i915/intel_sdvo.c    |  3 +++
>  drivers/gpu/drm/i915/intel_tv.c      | 14 ++++++++------
>  4 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1258883..f1d47dd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3914,6 +3914,14 @@ static bool intel_crtc_compute_config(struct drm_crtc *crtc,
>  		adjusted_mode->hsync_start == adjusted_mode->hdisplay)
>  		return false;
>  
> +	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) && pipe_config->pipe_bpp > 10) {
> +		pipe_config->pipe_bpp = 10*3; /* 12bpc is gen5+ */
> +	} else if (INTEL_INFO(dev)->gen <= 4 && pipe_config->pipe_bpp > 8) {
> +		/* only a 8bpc pipe, with 6bpc dither through the panel fitter
> +		 * for lvds. */
> +		pipe_config->pipe_bpp = 8*3;
> +	}
> +
>  	return true;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 47d3a21..0e51b2b 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -797,7 +797,9 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	/*
>  	 * HDMI is either 12 or 8, so if the display lets 10bpc sneak
> -	 * through, clamp it down.
> +	 * through, clamp it down. Note that only pch_split platforms have 12bpc
> +	 * pipes, g4x and vlv only has a 10bpc pipe, all earlier platforms have
> +	 * only 8bpc.
>  	 */
>  	if (pipe_config->pipe_bpp > 8*3) {
>  		DRM_DEBUG_KMS("forcing bpc to 12 for HDMI\n");
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 59a6781..a443086 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1046,6 +1046,9 @@ static bool intel_sdvo_compute_config(struct intel_encoder *encoder,
>  	struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>  	struct drm_display_mode *mode = &pipe_config->requested_mode;
>  
> +	DRM_DEBUG_KMS("forcing bpc to 8 for SDVO\n");
> +	pipe_config->pipe_bpp = 8*3;
> +
>  	if (HAS_PCH_SPLIT(encoder->base.dev))
>  		pipe_config->has_pch_encoder = true;
>  
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index d808421..6673726 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -905,11 +905,10 @@ intel_tv_mode_valid(struct drm_connector *connector,
>  
>  
>  static bool
> -intel_tv_mode_fixup(struct drm_encoder *encoder,
> -		    const struct drm_display_mode *mode,
> -		    struct drm_display_mode *adjusted_mode)
> +intel_tv_compute_config(struct intel_encoder *encoder,
> +			struct intel_crtc_config *pipe_config)
>  {
> -	struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
> +	struct intel_tv *intel_tv = enc_to_intel_tv(&encoder->base);
>  	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
>  
>  	if (!tv_mode)
> @@ -918,7 +917,10 @@ intel_tv_mode_fixup(struct drm_encoder *encoder,
>  	if (intel_encoder_check_is_cloned(&intel_tv->base))
>  		return false;
>  
> -	adjusted_mode->clock = tv_mode->clock;
> +	pipe_config->adjusted_mode.clock = tv_mode->clock;
> +	DRM_DEBUG_KMS("forcing bpc to 8 for TV\n");
> +	pipe_config->pipe_bpp = 8*3;
> +
>  	return true;
>  }
>  
> @@ -1485,7 +1487,6 @@ out:
>  }
>  
>  static const struct drm_encoder_helper_funcs intel_tv_helper_funcs = {
> -	.mode_fixup = intel_tv_mode_fixup,
>  	.mode_set = intel_tv_mode_set,
>  };
>  
> @@ -1620,6 +1621,7 @@ intel_tv_init(struct drm_device *dev)
>  	drm_encoder_init(dev, &intel_encoder->base, &intel_tv_enc_funcs,
>  			 DRM_MODE_ENCODER_TVDAC);
>  
> +	intel_encoder->compute_config = intel_tv_compute_config;
>  	intel_encoder->enable = intel_enable_tv;
>  	intel_encoder->disable = intel_disable_tv;
>  	intel_encoder->get_hw_state = intel_tv_get_hw_state;

Yeah looks good.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center