On Tue, 19 Apr 2011 12:12:38 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Figuring out which pipe bpp to use is a bit painful.  It depends on both
> the encoder and display configuration attached to a pipe.  For instance,
> to drive a 24bpp framebuffer out to an 18bpp panel, we need to use 6bpc
> on the pipe but also enable dithering.  But driving that same
> framebuffer to a DisplayPort output on another pipe means using 8bpc and
> no dithering.
> 
> So split out and enhance the code to handle the various cases, returning
> an appropriate pipe bpp as well as whether dithering should be enabled.
> 
> Save the resulting pipe bpp in the intel_crtc struct for use by encoders
> in calculating bandwidth requirements (defaults to 24bpp on pre-ILK).
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  181 ++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  2 files changed, 140 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8ef0c39..e81e418 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4232,6 +4232,120 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  	return dev_priv->lvds_use_ssc && i915_panel_use_ssc;
>  }
>  
> +/**
> + * intel_choose_pipe_bpp - figure out what color depth the pipe should send
> + * @crtc: CRTC structure
> + *
> + * A pipe may be connected to one or more outputs.  Based on the depth of the
> + * attached framebuffer, choose a good color depth to use on the pipe.
> + *
> + * If possible, match the pipe depth to the fb depth.  In some cases, this
> + * isn't ideal, because the connected output supports a lesser or restricted
> + * set of depths.  Resolve that here:
> + *    LVDS typically supports only 6bpc, so clamp down in that case
> + *    HDMI supports only 8bpc or 12bpc, so clamp to 8bpc with dither for 10bpc
> + *    Displays may support a restricted set as well, check EDID and clamp as
> + *      appropriate.
> + *
> + * RETURNS:
> + * Dithering requirement (i.e. false if display bpc and pipe bpc match,
> + * true if they don't match).
> + */
> +static bool intel_choose_pipe_bpp(struct drm_crtc *crtc, unsigned int
> *pipe_bpp)

Should mention dithering in the name, perhaps
choose_pipe_bpp_and_dithering or some such? Perhaps returning the
dithering in another by-reference parameter?

> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_encoder *encoder;
> +	struct drm_connector *connector;
> +	unsigned int display_bpc = UINT_MAX, fb_bpc, bpc;
> +
> +	fb_bpc = crtc->fb->depth / 3;
> +
> +	/* Walk the encoders & connectors on this crtc, get min bpc */

Don't you want the max bpc?

> +
> +	/*
> +	 * We could just drive the pipe at the highest bpc all the time and
> +	 * enable dithering as needed, but that costs bandwidth.  So choose
> +	 * the minimum value that expresses the full color range of the fb but
> +	 * also stays within the max display bpc discovered above.
> +	 */
> +
> +	switch (crtc->fb->depth) {
> +	case 8:
> +	case 15:
> +	case 16:
> +		bpc = 6; /* min is 18bpp */
> +		break;
> +	case 24:
> +		bpc = min((unsigned int)8, display_bpc);
> +		break;
> +	case 30:
> +		bpc = min((unsigned int)10, display_bpc);
> +		break;
> +	case 48:
> +		bpc = min((unsigned int)12, display_bpc);
> +		break;
> +	default:
> +		DRM_DEBUG("unsupported depth, assuming 24 bits\n");
> +		bpc = min((unsigned int)8, display_bpc);
> +		break;
> +	}

Hrm. 8-bit through a colormap should probably use 8 bpc? We can probably
ignore 15/16-bit direct color (which might want 8bpc as well).

The simple fix would be to use 8bpc on 8bpp and otherwise leave things alone.

-- 
keith.packard@intel.com