From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 0/4] Reduce intel_display.c Date: Wed, 16 Apr 2014 19:37:16 +0300 Message-ID: <20140416163716.GM18465@intel.com> References: <1397079873-18257-1-git-send-email-przanoni@gmail.com> <20140411065921.GA15641@bwidawsk.net> <87vbug72lk.fsf@intel.com> <20140415192544.GE1023@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id A57246EABC for ; Wed, 16 Apr 2014 09:37:25 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140415192544.GE1023@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Ben Widawsky , intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Tue, Apr 15, 2014 at 09:25:44PM +0200, Daniel Vetter wrote: > On Fri, Apr 11, 2014 at 10:21:59AM +0300, Jani Nikula wrote: > > On Fri, 11 Apr 2014, Ben Widawsky wrote: > > > On Wed, Apr 09, 2014 at 06:44:29PM -0300, Paulo Zanoni wrote: > > >> From: Paulo Zanoni > > >> = > > >> 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 li= nes, and > > >> after my patch series it has 8850 lines. > > >> = > > >> I don't know if right now is the appropriate time to merge patches l= ike this. I > > >> don't remember seeing too many patches on the list touching cursor/f= di/eld/pll > > >> functions, but I know there is never an appropriate time for huge ch= anges. > > >> = > > >> Also, this change will obviously make the lives of people who backpo= rt our > > >> patches more complicated. So if we don't want this series at all, fe= el 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 o= f it is the > > > copyright header, but still, considering there are no actual refactor= s, > > > 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 do= ubt > > > 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, bu= t 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 a= lso > > > 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 =3D=3D 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. I'm not sure I'd like the code for one pch platform to have a totally different structure than the other platforms. I guess I can't really say w/o seeing the result. And this is hsw after all where the code likes be different just because, which usually makes it the last platform I think about. And my last hsw related patches got stuck in limbo anyway, so I now prefer to keep my distance. -- = Ville Syrj=E4l=E4 Intel OTC