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 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
