Re: [PATCH] drm/i915: implement high-bpc + pipeconf-dither support for g4x/vlv

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 19 Apr 2013 20:17:10 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8c36376..7f1ab8b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4605,22 +4605,29 @@ static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc)
>  			pipeconf &= ~PIPECONF_DOUBLE_WIDE;
>  	}
>  
> -	/* default to 8bpc */
> -	pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
> -	if (intel_crtc->config.has_dp_encoder) {
> -		if (intel_crtc->config.dither) {
> -			pipeconf |= PIPECONF_6BPC |
> -				    PIPECONF_DITHER_EN |
> +	/* only g4x and later have fancy bpc/dither controls */
> +	if (IS_G4X(dev) || IS_VALLEYVIEW(dev)) {
> +		pipeconf &= ~(PIPECONF_BPC_MASK | PIPECONF_DITHER_EN);
> +
> +		/* Bspec claims that we can't use dithering for 30bpp pipes. */
> +		if (intel_crtc->config.dither && intel_crtc->config.pipe_bpp != 30)
> +			pipeconf |= PIPECONF_DITHER_EN |
>  				    PIPECONF_DITHER_TYPE_SP;
> -		}
> -	}
>  
> -	if (IS_VALLEYVIEW(dev) && intel_pipe_has_type(&intel_crtc->base,
> -						      INTEL_OUTPUT_EDP)) {
> -		if (intel_crtc->config.dither) {
> -			pipeconf |= PIPECONF_6BPC |
> -					PIPECONF_ENABLE |
> -					I965_PIPECONF_ACTIVE;
> +		pipeconf &= ~PIPECONF_BPC_MASK;
> +		switch (intel_crtc->config.pipe_bpp) {
> +		case 18:
> +			pipeconf |= PIPECONF_6BPC;
> +			break;
> +		case 24:
> +			pipeconf |= PIPECONF_8BPC;
> +			break;
> +		case 30:
> +			pipeconf |= PIPECONF_10BPC;
> +			break;
> +		default:
> +			/* Case prevented by intel_choose_pipe_bpp_dither. */
> +			BUG();

Am I misreading here?  It looks like we may set the 10bpc pipeconf bit,
but never enable it.  Should there be another G4x check somewhere?  Or
should the 10bpc case be dropped from the switch?

>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 58a98ff..094f3c5 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -135,7 +135,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	/* Set the dithering flag on LVDS as needed, note that there is no
>  	 * special lvds dither control bit on pch-split platforms, dithering is
>  	 * only controlled through the PIPECONF reg. */
> -	if (INTEL_INFO(dev)->gen == 4) {
> +	if (INTEL_INFO(dev)->gen == 4 && !IS_G4X(dev)) {
>  		if (intel_crtc->config.dither)
>  			temp |= LVDS_ENABLE_DITHER;
>  		else

Looks ok otherwise, though I'd still like to see LVDS dither compared
with pipe dither.

And what happens now if a 30bpp config gets chosen for an LVDS or eDP
panel?  Do we just output it anyway and get junk on the screen?

-- 
Jesse Barnes, Intel Open Source Technology Center