Re: [PATCH 07/36] drm/i915: move pipe bpp computation to pipe_config

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

On Thu, Feb 21, 2013 at 01:49:59AM +0100, Daniel Vetter wrote:
> The procedure has now 3 steps:
> 
> 1. Compute the bpp that the plane will output, this is done in
>    pipe_config_set_bpp and stored into pipe_config->pipe_bpp. Also,
>    this function clamps the pipe_bpp to whatever limit the EDID of any
>    connected output specifies.
> 2. Adjust the pipe_bpp in the encoder and crtc functions, according to
>    whatever constraints there are.
> 3. Decide whether to use dither by comparing the stored plane bpp with
>    computed pipe_bpp.
> 
> There are a few slight functional changes in this patch:
> - LVDS connector are now also going through the EDID clamping. But in
>   a 2nd change we now unconditionally force the lvds bpc value - this
>   shouldn't matter in reality when the panel setup is consistent, but
>   better safe than sorry.
> - HDMI now forces the pipe_bpp to the selected value - I think that's
>   what we actually want, since otherwise at least the pixelclock
>   computations are wrong (I'm not sure whether the port would accept
>   e.g. 10 bpc when in 12bpc mode). Contrary to the old code, we pick
>   the next higher bpc value, since otherwise there's no way to make
>   use of the 12 bpc mode (since the next patch will remove the 12bpc
>   plane format, it doesn't exist).
> 
> Both of these changes are due to the removal of the
> 
> 	pipe_bpp = min(display_bpp, plane_bpp);
> 
> statement.
> 
> Another slight change is the reworking of the dp bpc code:
> - For the mode_valid callback it's sufficient to only check whether
>   the mode would fit at the lowest bpc.
> - The bandwidth computation code is a bit restructured: It now walks
>   all available bpp values in an outer loop and the codeblock the
>   compute derived values (once a good configuration is found) has been
>   moved out of the for loop maze. This is prep work to allow us to
>   successively fall back on bpc values, and also correctly support bpc
>   values != 8 or 6.
> 
> If we ever get around to adding additional dithering (e.g. to squeeze
> a mode into 2 fdi lanes), we need to add a flag that tells the crtc
> configuration adjusting code that the bpp value selected by the output
> can't be lowered. Since we don't have such logic for now, let it be.
> 
> v2: Rebased on top of Paulo Zanoni's little refactoring to use more
> drm dp helper functions.
> 
> v3: Rebased on top of Jani's eDP bpp fix and Ville's limited color
> range work.
> 
> v4: Remove the INTEL_MODE_DP_FORCE_6BPC #define, no longer needed.
> 
> v5: Remove intel_crtc->bpp, too, and fix up the 12bpc check in the
> hdmi code. Also fixup the bpp check in intel_dp.c, it'll get reworked
> in a later patch though again.
> 
> v6: Fix spelling in a comment.
> 
> v7: Debug output improvements for the bpp computation.
> 
> v8: Fixup 6bpc lvds check - dual-link and 8bpc mode are different
> things!
> 
> v9: Reinstate the fix to properly ignore the firmware edp bpp ... this
> was lost in a rebase.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |  11 +-
>  drivers/gpu/drm/i915/intel_display.c | 223 ++++++++++++-----------------------
>  drivers/gpu/drm/i915/intel_dp.c      | 105 ++++++++---------
>  drivers/gpu/drm/i915/intel_drv.h     |   9 +-
>  drivers/gpu/drm/i915/intel_hdmi.c    |  17 ++-
>  drivers/gpu/drm/i915/intel_lvds.c    |  12 ++
>  6 files changed, 157 insertions(+), 220 deletions(-)
> 
<snip>
> @@ -7392,14 +7255,72 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
>  	}
>  }
>  
> +static int
> +pipe_config_set_bpp(struct drm_crtc *crtc,
> +		    struct drm_framebuffer *fb,
> +		    struct intel_crtc_config *pipe_config)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_connector *connector;
> +	int bpp;
> +
> +	switch (fb->depth) {
> +	case 8:
> +		bpp = 8*3; /* since we go through a colormap */
> +		break;
> +	case 15:
> +	case 16:
> +		bpp = 6*3; /* min is 18bpp */
> +		break;
> +	case 24:
> +		bpp = 8*3;
> +		break;
> +	case 30:
> +		bpp = 10*3;
> +		break;
> +	case 48:
> +		bpp = 12*3;
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("unsupported depth\n");
> +		return -EINVAL;
> +	}
> +
> +	if (fb->depth > 24 && !HAS_PCH_SPLIT(dev)) {
> +		DRM_DEBUG_KMS("high depth not supported on gmch platforms\n");
> +		return -EINVAL;
> +	}

AFAIK Gen4 does support 2:10:10:10 formats. This would fail there, no?

-- 
Ville Syrjälä
Intel OTC