Re: [PATCH 0/4] Reduce intel_display.c

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC