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

[Jani Nikula <jani.nikula@linux.intel.com>]

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. ;)

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

BR,
Jani.




>
>
>> 
>> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center