On Tue, Apr 15, 2014 at 09:25:44PM +0200, Daniel Vetter wrote: > On Fri, Apr 11, 2014 at 10:21:59AM +0300, Jani Nikula wrote: > > On Fri, 11 Apr 2014, Ben Widawsky <ben@xxxxxxxxxxxx> wrote: > > > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: > > >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > >> > > >> Hi > > >> > > >> We always talk about how intel_display.c is a giant file and how we would like > > >> to reduce it, so this is my attempt. Currently the file has 12090 lines, and > > >> after my patch series it has 8850 lines. > > >> > > >> I don't know if right now is the appropriate time to merge patches like this. I > > >> don't remember seeing too many patches on the list touching cursor/fdi/eld/pll > > >> functions, but I know there is never an appropriate time for huge changes. > > >> > > >> Also, this change will obviously make the lives of people who backport our > > >> patches more complicated. So if we don't want this series at all, feel free to > > >> NACK it. > > > > > > I am only responding because it seems nobody else really said much. I > > > never touch this code, and I shouldn't be the authority. I really > > > quickly glanced at the patches. > > > > > > 1. +LOC: It sucks that you ended up adding 220 lines. I assume half of it is the > > > copyright header, but still, considering there are no actual refactors, > > > cleanups, or functional changes - adding lines makes me unhappy. > > > > > > 2. necessary? I personally haven't heard from anyone that we need to shrink > > > intel_display.c (again, I am the furthest from being an expert). I doubt > > > anyone isn't using some form of tags, or grep to navigate anyway. My > > > problem has never been the file size itself, but just the structure of > > > the display code interacting with the core KMS was hard to follow. > > > > > > 3. conflicts: Like you said, it's likely nobody touches this code, but we should > > > keep in mind we do have several people working on older branches, and > > > this kind of thing makes any sort of backport hard. > > > > > > On the other hand: > > > 1. If more than one person finds the results more readable/consumable, I > > > think it's worth it, and probably mostly justifies doing it. You've also > > > shrunk the file by quite a bit, so it's somewhat useful churn. > > > > > > 2. intel_pll.c sounds like a good idea > > > > I'm in favour of reducing the size of intel_display.c. I did not look at > > the patches though, because I've sent a few patches to this effect in > > the past (limits/pll and quirks at least), but they were stalled because > > they were in the collision course with the Grand Plans Daniel has. So > > Daniel, I expect you to chime in on this one too. ;) > > Chiming in now ;-) > > I've seen a few "extract stuff" patches float by and occasionally also > merged some, but thus far I' haven't been terribly convinced. I don't mind > the conflicts these patches cause, but if we want to reorg the driver the > goal shouldn't be to just make files smaller (cscope can cope) but > actually improve the organization of all this. > > Often these patches just grab a bag of functions with related names, throw > them all into a new file and then add forward and header declaration until > the damn thing compiles again. What we want instead are real code modules > where interactions within one file are high and the number of exported > functions fairly low. > > Two examples: > - Extracting the pageflip helpers and related code would mean that we also > should extract a new pageflip_init functions, so that all the platform > functions don't need to be exported. We've done similar things when > creating intel_unocore.c. > > - I've just stumbled around in the haswell code and noticed that pretty > much all the lpt and hsw fdi code could be moved into intel_crt.c with a > new hsw specific crt encoder. In the pre_enable and post_disable hooks > we could then do the ddi setup, fdi link training. And all the ltp > handling code could mostly be moved away too. With this we could also > remove a lot (if not all) of the has_pch_encoder checks and I think also > many type == INTEL_OUTPUT_ANALOG checks in the haswell code. > > I haven't actually checked whether it'll all nicely work out, but my gut > feeling says it'll be fewer forward declarations than just shoveling all > the fdi related functions (including the lpt stuff) into a new > intel_fdi.c. I'm not sure I'd like the code for one pch platform to have a totally different structure than the other platforms. I guess I can't really say w/o seeing the result. And this is hsw after all where the code likes be different just because, which usually makes it the last platform I think about. And my last hsw related patches got stuck in limbo anyway, so I now prefer to keep my distance. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
