On Wed, Apr 16, 2014 at 07:37:16PM +0300, Ville Syrjälä wrote: > 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. I've implemented it just now since it avoids piling more hacks on top to handle the unshared SPLL. Imo looks fairly ok-ish: http://cgit.freedesktop.org/~danvet/drm/log/?h=no-more-enc-mode_set Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
