Intel-GFX Archive on lore.kernel.org
 help / color / Atom feed
From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Keith Packard <keithp@keithp.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code
Date: Wed, 11 May 2011 10:23:14 -0700
Message-ID: <20110511102314.09d136a6@jbarnes-desktop> (raw)
In-Reply-To: <yun39kmb67t.fsf@aiko.keithp.com>

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

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-19 19:12 [RFC] Deep color support Jesse Barnes
2011-04-19 19:12 ` [PATCH 1/7] drm/i915: color range only works on pre-ILK Jesse Barnes
2011-05-10 21:12   ` Keith Packard
2011-05-10 21:16     ` Jesse Barnes
2011-05-10 21:22       ` Jesse Barnes
2011-05-10 22:14       ` Keith Packard
2011-04-19 19:12 ` [PATCH 2/7] drm/i915: don't set transcoder bpc on CougarPoint Jesse Barnes
2011-04-19 19:12 ` [PATCH 3/7] drm/i915: set bpc for DP transcoder Jesse Barnes
2011-04-19 19:12 ` [PATCH 4/7] drm/i915: split out Ironlake pipe bpp picking code Jesse Barnes
2011-05-10 21:23   ` Keith Packard
2011-05-11 17:23     ` Jesse Barnes [this message]
2011-04-19 19:12 ` [PATCH 5/7] drm/i915: split out plane update code Jesse Barnes
2011-04-19 19:12 ` [PATCH 6/7] drm/i915: use pipe bpp in DP link bandwidth calculations Jesse Barnes
2011-04-19 19:16   ` Jesse Barnes
2011-04-19 19:12 ` [PATCH 7/7] drm/i915: use pipe bpp when setting HDMI bpc Jesse Barnes
2011-04-19 19:50 ` [ANCIENT PATCH] Enable 30-bit depth Andy Lutomirski
2011-04-19 19:56   ` Jesse Barnes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110511102314.09d136a6@jbarnes-desktop \
    --to=jbarnes@virtuousgeek.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=keithp@keithp.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Intel-GFX Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/intel-gfx/0 intel-gfx/git/0.git
	git clone --mirror https://lore.kernel.org/intel-gfx/1 intel-gfx/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 intel-gfx intel-gfx/ https://lore.kernel.org/intel-gfx \
		intel-gfx@lists.freedesktop.org
	public-inbox-index intel-gfx

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.intel-gfx


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git