[Intel-gfx] [PATCH 0/4] Reduce intel_display.c

[Daniel Vetter]

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 at bwidawsk.net> wrote:
> > > > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote:
> > > >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > >> 
> > > >> 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