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

[Ben Widawsky <ben@bwidawsk.net>]

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 also didn't really know what kind of changes I needed to do to the file
> headers, so I just copied the header from intel_display.c, kept Eric's name and
> added a "2014" to Intel's copyright. I am not a lawyer and this may be not the
> best thing to do, so please tell me the correct approach here :)
> 
> There are also some things that we might want to migrate from intel_ddi.c to
> intel_pll.c, but I'll leave this to another patch.
> 
> Also, feel free to propose better ways to split intel_display.c.
> 
> Thanks,
> Paulo
> 
> Paulo Zanoni (4):
>   drm/i915: extract intel_eld.c from intel_display.c
>   drm/i915: extract intel_cursor.c from intel_display.c
>   drm/i915: extract intel_fdi.c from intel_display.c
>   drm/i915: extract intel_pll.c from intel_display.c
> 
>  drivers/gpu/drm/i915/Makefile        |    4 +
>  drivers/gpu/drm/i915/intel_cursor.c  |  357 ++++
>  drivers/gpu/drm/i915/intel_ddi.c     |  142 +-
>  drivers/gpu/drm/i915/intel_display.c | 3622 ++--------------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  143 +-
>  drivers/gpu/drm/i915/intel_eld.c     |  355 ++++
>  drivers/gpu/drm/i915/intel_fdi.c     |  959 +++++++++
>  drivers/gpu/drm/i915/intel_panel.c   |   36 +
>  drivers/gpu/drm/i915/intel_pll.c     | 1779 +++++++++++++++++
>  9 files changed, 3808 insertions(+), 3589 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_cursor.c
>  create mode 100644 drivers/gpu/drm/i915/intel_eld.c
>  create mode 100644 drivers/gpu/drm/i915/intel_fdi.c
>  create mode 100644 drivers/gpu/drm/i915/intel_pll.c
> 
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center