From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 4/3] drm/i915: pass intel_crtc as argument for intel_enable_pipe Date: Tue, 11 Feb 2014 22:54:28 +0100 Message-ID: References: <1389973873-2005-1-git-send-email-przanoni@gmail.com> <20140210141703.GE9282@strange.amr.corp.intel.com> <20140210172314.GZ17001@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ie0-f170.google.com (mail-ie0-f170.google.com [209.85.223.170]) by gabe.freedesktop.org (Postfix) with ESMTP id 2771CFA3A7 for ; Tue, 11 Feb 2014 13:54:29 -0800 (PST) Received: by mail-ie0-f170.google.com with SMTP id lx4so970673iec.29 for ; Tue, 11 Feb 2014 13:54:28 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Tue, Feb 11, 2014 at 6:20 PM, Paulo Zanoni wrote: > 2014-02-11 15:09 GMT-02:00 Paulo Zanoni : >> 2014-02-11 13:44 GMT-02:00 Daniel Vetter : >>> On Tue, Feb 11, 2014 at 4:23 PM, Paulo Zanoni wrote: >>>> >>>> 2014-02-10 15:23 GMT-02:00 Daniel Vetter : >>>>> On Mon, Feb 10, 2014 at 02:17:03PM +0000, Damien Lespiau wrote: >>>>>> On Fri, Jan 17, 2014 at 01:51:09PM -0200, Paulo Zanoni wrote: >>>>>> > From: Paulo Zanoni >>>>>> > >>>>>> > We want to remove those 3 boolean arguments. This is the first step. >>>>>> > The "pipe" passed as the argument is always intel_crtc->pipe. >>>>>> > >>>>>> > Also adjust the function documentation. >>>>>> > >>>>>> > Signed-off-by: Paulo Zanoni >>>>>> >>>>>> Reviewed-by: Damien Lespiau >>>>> >>>>> Ok, I've pulled in the entire series, but a bunch of things changed so had >>>>> to resolve some (minor) conflicts. Please double-check that I didn't botch >>>>> things up too badly. >>>> >>>> You forgot to apply patch 2, and this is probably the reason why every >>>> subsequent patch gave you a conflict. >>> >>> The conflicts where actually with one of Ville's patches to move the >>> plane enabling around. I've fixed those up but apparently then missed >>> the other conflict hidden underneath those. >>> >>>> You also applied patch 3 twice: once for Ironlake and once for >>>> Haswell. You shouldn't change the Ironlake function. >>>> >>>> Do you plan to rebase or do I need to submit patches on top? >>> >>> I've applied the missing patched and dropped the ironlake patch of the >>> double-merged one. So if the new tree looks ok no need to resend >>> anything. > > Doesn't look ok yet. Oh dear ... I've tried again. Not sure whether I should have though :( -Daniel > So previously I had "[PATCH 2/8] drm/i915: don't wait for vblank after > enabling pipe on HSW", which removes a wait_for_vblank on HSW. > > Then on "[PATCH 7/8] drm/i915: remove wait_for_vblank argument form > intel_enable_pipe" we just change the function parameters without > changing the function behavior. > > With this, if we bisect something to patch 2 we know the problem is > that we stopped waiting for a vblank, and if we bisect to patch 7 we > know the problem is something else. > > But since you just skipped patch 2, patch 7 is now more than just a > coding style change: it actually does what patch 2 was supposed to do. > So in a way, we can say patch 2 is not really necessary, but it was > written weeks before patch 7, and patch 7 should be just a result of > the review comments. > > And the version of "drm/i915: don't wait for vblank after enabling > pipe on HSW" which you just committed as a last patch instead of > second patch (the one that changes the argument to > intel_crtc_update_cursor) is just plain wrong. That needs to be > reverted. So either we add the original patch 2 at the right place, or > we completely discard it... > > I know it's common to change the patch ordering when applying to our > trees, but it can be quite dangerous... > > Thanks, > Paulo > >>> >>>> IMHO if a series starts getting messy to apply, I think you should >>>> probably just ask the author to rebase and resend the final stuff. >>>> Maybe with this we would be able to reduce the amount of bad merges, >>>> which is becoming a very common problem, at least for my patches. >>> >>> The problem is that small conflicts are really common, both because I >>> want people to submit against drm-intel-nightly (so that I can do the >>> backmerging and branch shuffling correctly) and because of our >>> development speed. I don't think me asking for rebases in all these >>> cases is the better option. I tend to poke people to double-check when >>> I screw things up, but I guess it's just been bad luck that recently >>> the conflict fallout has always hit your patches :( >>> >>> Cheers, Daniel >>> -- >>> Daniel Vetter >>> Software Engineer, Intel Corporation >>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch >> >> >> >> -- >> Paulo Zanoni > > > > -- > Paulo Zanoni -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch