On Fri, 11 Apr 2014, Ben Widawsky <ben@xxxxxxxxxxxx> wrote: > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ben Widawsky, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
