Re: [PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 10 May 2011 14:23:18 -0700
Keith Packard <keithp@keithp.com> wrote:

> 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?

Ok fixed, just added _dither to the name.

> 
> > +{
> > +	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?

If the comment is confusing, I can remove it.  Or maybe I need to
expand it.

What we're trying to do is find a common bpc for all the displays
attached to the pipe.  But we need to use the minimum of all attached in
order to set the pipe's dithering correctly, since dithering occurs at
the pipe level, not the output (at least on ILK+).

> > +
> > +	/*
> > +	 * 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.

Ok, fixed.  New patches on their way.

-- 
Jesse Barnes, Intel Open Source Technology Center