From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/12] drm/i915: Use the new vm [un]bind functions Date: Fri, 26 Jul 2013 23:56:15 +0200 Message-ID: <20130726215615.GQ6084@phenom.ffwll.local> References: <1374458899-8635-1-git-send-email-ben@bwidawsk.net> <1374458899-8635-7-git-send-email-ben@bwidawsk.net> <20130723165442.GK5939@phenom.ffwll.local> <20130726214832.GC20047@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id AFAC9E6CA2 for ; Fri, 26 Jul 2013 14:56:16 -0700 (PDT) Received: by mail-ea0-f177.google.com with SMTP id f10so1214671eak.22 for ; Fri, 26 Jul 2013 14:56:16 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130726214832.GC20047@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Fri, Jul 26, 2013 at 02:48:32PM -0700, Ben Widawsky wrote: > On Tue, Jul 23, 2013 at 06:54:43PM +0200, Daniel Vetter wrote: > > On Sun, Jul 21, 2013 at 07:08:13PM -0700, Ben Widawsky wrote: > > > Building on the last patch which created the new function pointers in > > > the VM for bind/unbind, here we actually put those new function pointers > > > to use. > > > > > > Split out as a separate patch to aid in review. I'm fine with squashing > > > into the previous patch if people request it. > > > > > > v2: Updated to address the smart ggtt which can do aliasing as needed > > > Make sure we bind to global gtt when mappable and fenceable. I thought > > > we could get away without this initialy, but we cannot. > > > > > > Signed-off-by: Ben Widawsky > > > > Meta review on the patch split: If you create new functions in a prep > > patch, then switch and then kill the old functions it's much harder to > > review whether any unwanted functional changes have been introduced. > > Reviewers have to essentially keep both the old and new code open and > > compare by hand. And generally the really hard regression in gem have > > been due to such deeply-hidden accidental changes, and we frankly don't > > yet have the test coverage to just gloss over this. > > > > If you instead first prepare the existing functions by changing the > > arguments and logic, and then once everything is in place switch over to > > vfuncs in the 2nd patch changes will be in-place. In-place changes are > > much easier to review since diff compresses away unchanged parts. > > > > Second reason for this approach is that the functions stay at the same > > place in the source code file, which reduces the amount of spurious > > conflicts when rebasing a large set of patches around such changes ... > > > > I need to ponder this more. > > -Daniel > > ping Keep it in mind for next time around. I think my general approach is easier on reviewers ... but hey, vacation! -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch