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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 24, 2013 at 12:30:14AM +0200, Daniel Vetter wrote:
> The current code is rather ... ugly. The only thing it managed to pull
> off is getting 6bpc on DP working on g4x. Then someone added another
> custom hack for 6bpc eDP on vlv. Fix up this entire mess by properly
> implementing the PIPECONF-based dither/bpc controls on g4x/vlv.
> 
> Note that compared to pch based platforms g4x/vlv don't support 12bpc
> modes. g4x is already caught, extend the check for vlv.
> 
> The other fixup is to restrict the lvds-specific dithering to early
> gen4 devices - g4x should use the pipeconf dither controls. Note that
> on gen2/3 the dither control is in the panel fitter even.
> 
> v2: Don't enable dithering when the pipe is in 10 bpc mode. Quoting
> from Bspec "PIPEACONF - Pipe A Configuration Register, bit 4":
> 
> "Programming note: Dithering should only be enabled for 8 bpc or 6
> bpc."
> 
> v3: Actually drop the old ugly dither code.
> 
> v4: Explain in a short comment why g4x/vlv shouldn't dither for 30 bpp
> pipes (Jesse).
> 
> v5: Also clear the dither type correctly as spotted by Ville.
> 
> v6: As Ville pointed out we need to indeed set the dithering both in
> the pipeconf register (for DP outputs) and in the LVDS port register
> (for LVDS ouputs). Otherwise LVDS panel will not get properly
> dithered. The old patch got away with this since it forgot to clear
> the LVDS dither bit ...
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 505c2fc..3cdb921 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4591,22 +4591,30 @@ 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 | PIPECONF_DITHER_TYPE_MASK);
> +
> +		/* 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;

bpc bits were already cleared. This line can be dropped.

Otherwise:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +		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();
>  		}
>  	}
>  
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC