Re: [PATCH 00/19] [RFC] introduce struct intel_pipe_config

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 14, 2013 at 02:28:19PM -0800, Jesse Barnes wrote:
> On Wed, 13 Feb 2013 12:32:03 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Hi all
> > 
> > So this is my old attempt to push our modeset infrastructure a bit further,
> > rebased on top of latest dinq. Originally I wanted to push this a bit further
> > still before submitting for rfc, but with fastboot picking up I think it's
> > better to throw it out there now.
> > 
> > Roughly this adds a new intel_pipe_config struct which will (eventually) contain
> > all the configuration parameters of an output pipe. With this I aim to solve a
> > bunch of issues with the current code:
> > 
> > - We have a lot of encoder/output specific logic in our crtc code. Having a
> >   struct to conveniently store flags and other special cases allows us to move
> >   that logic into encoder callbacks. A few examples are bpp selection/dithering,
> >   limited range selection, pixel clock computations, ... To have a few examples,
> >   this series converts all users of the mode->flag and the bpp selection.
> 
> Yeah, I like this better than the mode flag stuff.  The new pipe bpp
> chooser is definitely simpler, though now some bits are sprinkled to
> the outputs.  I guess that's a bit better.

Being able to sprinkle output specific logic to the outputs was one of the
goals ;-) I realize that this will make the pipe config computation more
opaque since now a given crtc value isn't computed in one place any more.
Otoh this allows us to shovel a lot of the output specific details (e.g.
dp only has fixed frequencies, the single/dual-link split for lvds, ...)
into output files and group them together there.

In the end I'd like to have most of the encoder checks removed from crtc
code, replaced by fancy values in the pipe_config struct. That way the
crtc enable/disable code is a simple "punch new values into regs" thing
and all the magic happens when computing the pipe_config.

> > - Shared resources are currently not checked or only after we've started to
> >   change the hw state already. Examples are shared plls, shared fdi b/c lanes,
> >   but also memory bw (we don't bother about this yet) and similar stuff. If the
> >   code is restructured to compute the pipe config first and only then start
> >   touching the hw, we can fix this. Fixing this is a requirement to get a
> >   possible "check mode" of atomic modesetting working correctly.
> > 
> > - Intermingling of configuration selection and hw setup leads to inflexible
> >   code. Best example is probably the bpp selection - some of the constraints
> >   like fdi link bw are computed way too late, after e.g. the DP output has
> >   computed link clock values already. So we can't adjust it any more and are
> >   left with the only option to fail the modeset.
> > 
> >   Originally I've wanted to implement better handling of fdi b/c 2 lane
> >   configurations as a proof-of-concept of this, hence just rfc. I'll try to
> >   amend this over the next few days.
> > 
> > - Lacking infrastructure for fastboot hw state readout. I'm firmly of the
> >   opinion that if we want fastboot to work on all the insane configuraions out
> >   there, we need really solid hw state readout support. Hence this series also
> >   adds pipe_config readout support and starts with some very minimal support for
> >   comparing different such states.
> 
> Well, I think we'll have to punt in quite a few configs due to hw
> limitations about what can be changed without a full pipe shutdown, but
> we can definitely do better than we do today, and may be able to take
> some shortcuts (provided we get lots of testing on the given hw).

Yeah, as discussed on irc my idea (doesn't exists as code yet) is to add a
few special modes to the pipe_config compare function:
- strict: Used after a modeset to check whether the new hw state matches
  up with what we wanted to set.
- modeset: More permissive for fastboot to decide whether the new
  configuration is essentially the same as the old one. Things like fuzzy
  clock matching and things like that would imo fit here.
- plane-update-only: Even more permissive to figure out whether we only
  have different plane/pfit/whatever settings. For a new fastboot plane
  update mode that everyone seems to dream about.
- Feel free to go nuts ;-)

> > Comments, flames and ideas for the patches themselves, but also for the above
> > issues in general highly welcome.
> 
> Overall I like this approach, it should make fastboot state tracking a
> lot easier.
> 
> We'll also want to track gamma enable; unfortunately that can only be
> changed with the plane off (according to docs), but we should check for
> it regardless.

Hey, that's a new one! I've we're lucky we could subsume this with the
"update plane only" mode to get rid of the pfit and similar stuff we don't
want ...

A similar issue is all the various bits&pieces to support hdmi/audio. I
don't know yet how we should exactly track this. Might be that fastboot
for external screens is a long way off still.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch