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

[Daniel Vetter]

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.

So from my pov to make extractions into new files really useful we need to
put a bit of time into first polishing the interfaces a bit and maybe
reorganizing the code structure to have real modules. In my experience
trying to write abi docs for those interfaces helps a lot.

> To reduce the conflict/backport pains, might be good to do this spread
> out over a few releases instead of everything at once. *shrug*.

tbh it'll always hurt, and due to our rolling -next model there's never
really a time where it's convenient. I'm willing to merge such reorg
patches pretty much always.

Taking all the above into account I propose the following approach:

1. Each patch series only does one extraction.

2. The series also does any necessary interface polish where the
extraction requires tons of header declarations.

3. To make it really worth it the series also needs to add kerneldoc for
all exported functions in the new file and a small DOC: section with an
(potentially very short) overview section. All that should then be pulled
into the drm DocBook template somewhere suitable in the i915 book.

Comments, thoughs?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch