Re: [PATCH 12/13] drm/i915: clean up pipe bpp confusion

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 27 Mar 2013 23:41:55 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Wed, Mar 27, 2013 at 10:28 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > I had to double check this against 9/13... I guess the order will be
> > clearer with the actual code as opposed to patches.
> >
> > But won't these override the pipe_config bpp we set in
> > pipe_config_set_bpp()?  Why bother setting it there if each of the
> > encoders will set it themselves, and the real comparison is against
> > the plane bpp?  And doesn't that mean we'd need to set pipe_config->bpp
> > in the DP version too?
> 
> The pipe_bpp we set from the planes is the proposed one, used when
> nothing else overrides it. Then encoders can come around and add in
> their opinion about what's possible. Note that encoders want to know
> which pipe_bpp is the proposed one (from looking just at the plane) to
> make their own decision. E.g. hdmi wants to updither 10bpc to 12bpc
> (if possible) since it doesn't support 10bpc natively. Whereas DP only
> ever down-dithers.
> 
> This way we gain a notch more flexibility in handling bpp.
> 
> My auto-fdi dither work which is based on top of this goes one step
> further and checks (after plane/pipe/encoder all had their say)
> whether it actually fits into the fdi link. If it doesn't fit it tries
> to dither down. If that's possible we'll restart the pipe_config
> arbitrage, but with the new proposed pipe_bpp plus a flag telling
> everyone that they'll get shot if they try to increase bw
> requirements.
> 
> > Maybe I've been looking at this too hard and I've just confused
> > myself...
> 
> Maybe it's a bit overdesigned ;-)

Ok it makes some sense... though maybe if we passed down the plane bpp
directly we'd be able to avoid some of the re-calc stuff in your FDI
dither patch.

We can always improve it after it lands and becomes clearer.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center