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 at ffwll.ch Mobile: +41 (0)79 365 57 48
