Re: [PATCH 08/10] drm/i915: move pipe bpp computation to pipe_config

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 26, 2013 at 02:12:39PM -0700, Jesse Barnes wrote:
> On Fri, 22 Feb 2013 00:56:52 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> 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).
> > 
> 
> This is a big patch; maybe there's a reasonable way to split it up?
> Maybe by leaving the _FORCE_6BPC flag in then removing in a second
> patch?  May as well squash in 9/10 while you're at it, since it mainly
> affects the new function.

FORCE_6BPC is just the DP encoder's way to communicate bpp requirements,
so I've figured I might as well include it. Looking at the patch again I
admit it's pretty big, so I'll try to smash it into pieces. Should be
doable from a quick look.

> I wonder if we want a pipe_dither and a connector_dither flag too?
> On Cantiga, we can dither either at LVDS or in the pipe for example.
> Not sure which is better...

I've thought that the lvds dither flag in the port disappeared with the
appearance of the pipe dither modes. Might be wrong though, but I think
I've completely check all docs covering that range (i.e. gen3-cantiga).

The only place where we have 2 dither bits is on pch ports (one on the cpu
transcoder, one in the pch transcoder). But imo it doesn't make much sense
to dither down/up in two steps, so we should be fine with just one flag.

The only case I've come up with is a 8bpc panel which _requires_ 8bpc (the
apple retina are like that iirc), and we try to feed it a 16bpp plane.
Then it would make sense to shovel 6bpc over the fdi link and up-dither to
8bpc on the pch.

But since that'll only happen with a desktop/all-in-one eDP config, who
cares. And if your driving your expensive 8bpc edp panel at 6bpc, you're
doing it wrong anyway.

> And as usual, we need better tests for this stuff.  The color ramps in
> testdisplay should show artifacts if we get the dithering wrong, but we
> could probably improve it.

My self-imposed merge criterion for the bpp fixes (an entire pile of
follow-up patches) is that I want to improve testdisplay to also test the
different pixel depths. And it needs testing (10bpc screen is on order).

But that part of the pipe_config rework is pretty orthogonal (imo the dp
unconfusion part is more important to get things going), so can wait.

> Otherwise this mostly looks good.  I definitely like the direction.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch