Re: RFC: i915 arch changes to better support new chipsets

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Mar 20, 2012 at 11:13:57AM -0700, Jesse Barnes wrote:
> I've been talking with some people offline about potential changes to
> our source structure to make it easier to add support for new chipsets,
> prevent breaking chipsets one doesn't intend to affect, and generally
> make the code more readable and maintainable.
> 
> I've got some new chipset code ready that's motivating some of this
> work, because it moved all the display regs around, added a new unit in
> low MMIO space, and generally affects gen7 paths with gen5 like
> behavior.
> 
> The question is, how far should we go?  At the very least, I'd like to
> propose the following:
>   intel_display.c -> pch_display.c, gmch_display.c, potentially with
>     additional sub-files for specific weirdness and power/drps
>     functionality

Yeah, that seems pretty sensible. I also think that splitting out some of
the less crtc_modeset related display functionality would be good, e.g.
pageflip, cursor stuff (like we've done for planes already). One big
monsters is the init code, which for added hilarity seems to be the place
to do all kinds of once-per-poweron workarounds. I don't have a clear idea
what we should do there.

>   i915_irq.c -> per chipset irq files (i9xx, ironlake, ivb at the very
>     least, with some additional duplication)

I'm not too sure about that one - we have i915_irq.c, i915_dma.c and
i915_drv.c with rather ill-defined use-cases for all kinds of things. And
tons of dri1 legacy stuff splattered all accross. We might want to pull
out a few functionalities out of these first before splitting it up
according to chipsets (e.g. pulling out the error handler and gpu reset
crap, which is currently splattered over a few files). After that, not
much might be left and it might make sense to just leave the irq stuff
together.

>   new, range specific i915_read/write routines, e.g. i915_read_gt,
>     i915_read_display to make forcewake and register block moves easier
>     to handle

I'm not convinced whether this is a great idea if applied all over the
code. For specific cases where a block moves it's imo better to just add a
mmio base for that (like we're doing with the ring ctrl regs, which move
around quite a bit over the various rings and generations). Obviously
adding a small helper is good, but imo we should name it a bit more
specific (if possible).

> I'm open to suggestions on how to fix i915_reg.h; it's becoming quite a
> beast.  Our goal to be to make it easy to add new definitions while
> also making it easy to not accidentally use old an incorrect
> definitions on a new platform.

Close your eyes and just keep on adding gunk. Imo i915_reg.h is pretty
much a write-once file, and cscope can still keep up with the definitions.
So not a pain point for me.

> Then obviously within those files there's lots of room for improvement,
> for example in i9xx mode setting we still have some pretty massive
> functions that need to be split (I have patches to do that).
> 
> Thoughts?  It may also make sense to split some of our port specific
> files where they differ enough from previous platforms.  E.g. g4x DP vs
> ironlake+...

I've just talked about this a bit with Eugeni in the context of Haswell,
and I think we might want to hold of for that code until we move output
stuff all over the place.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48