From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 06/12] drm/i915: Use the new vm [un]bind functions
Date: Fri, 26 Jul 2013 23:56:15 +0200 [thread overview]
Message-ID: <20130726215615.GQ6084@phenom.ffwll.local> (raw)
In-Reply-To: <20130726214832.GC20047@bwidawsk.net>
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 <ben@bwidawsk.net>
> >
> > 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
next prev parent reply other threads:[~2013-07-26 21:56 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-07-22 2:08 [PATCH 00/12] Completion of i915 VMAs Ben Widawsky
2013-07-22 2:08 ` [PATCH 01/12] drm/i915: plumb VM into object operations Ben Widawsky
2013-07-23 16:37 ` Daniel Vetter
2013-07-26 9:51 ` Maintainer-review fluff (was: Re: [PATCH 01/12] drm/i915: plumb VM into object operations) Daniel Vetter
2013-07-26 16:59 ` Jesse Barnes
2013-07-26 17:08 ` Chris Wilson
2013-07-26 17:12 ` Jesse Barnes
2013-08-04 20:31 ` Daniel Vetter
2013-07-26 17:40 ` Daniel Vetter
2013-07-26 20:15 ` Ben Widawsky
2013-07-26 20:43 ` Daniel Vetter
2013-07-26 23:13 ` Dave Airlie
2013-07-27 0:05 ` Ben Widawsky
2013-07-27 8:52 ` Dave Airlie
2013-08-04 19:55 ` Daniel Vetter
2013-07-29 22:35 ` Jesse Barnes
2013-07-29 23:50 ` Dave Airlie
2013-08-04 20:17 ` Daniel Vetter
2013-08-05 21:33 ` Jesse Barnes
2013-08-05 22:19 ` Daniel Vetter
2013-08-05 23:34 ` Jesse Barnes
2013-08-06 6:29 ` Daniel Vetter
2013-08-06 14:50 ` Paulo Zanoni
2013-08-06 17:06 ` Daniel Vetter
2013-08-06 23:28 ` Dave Airlie
2013-07-22 2:08 ` [PATCH 02/12] drm/i915: Fix up map and fenceable for VMA Ben Widawsky
2013-07-23 16:42 ` Daniel Vetter
2013-07-23 18:14 ` Ben Widawsky
2013-07-22 2:08 ` [PATCH 03/12] drm/i915: Update error capture for VMs Ben Widawsky
2013-07-22 2:08 ` [PATCH 04/12] drm/i915: Track active by VMA instead of object Ben Widawsky
2013-07-23 16:48 ` Daniel Vetter
2013-07-26 21:48 ` Ben Widawsky
2013-07-22 2:08 ` [PATCH 05/12] drm/i915: Add map/unmap object functions to VM Ben Widawsky
2013-07-22 2:08 ` [PATCH 06/12] drm/i915: Use the new vm [un]bind functions Ben Widawsky
2013-07-23 16:54 ` Daniel Vetter
2013-07-26 21:48 ` Ben Widawsky
2013-07-26 21:56 ` Daniel Vetter [this message]
2013-07-22 2:08 ` [PATCH 07/12] drm/i915: eliminate vm->insert_entries() Ben Widawsky
2013-07-23 16:57 ` Daniel Vetter
2013-07-22 2:08 ` [PATCH 08/12] drm/i915: Add vma to list at creation Ben Widawsky
2013-07-22 2:08 ` [PATCH 09/12] drm/i915: create vmas at execbuf Ben Widawsky
2013-07-22 13:32 ` Chris Wilson
2013-07-22 2:08 ` [PATCH 10/12] drm/i915: Convert execbuf code to use vmas Ben Widawsky
2013-07-22 2:08 ` [PATCH 11/12] drm/i915: Convert object coloring to VMA Ben Widawsky
2013-07-23 17:07 ` Daniel Vetter
2013-07-22 2:08 ` [PATCH 12/12] drm/i915: Convert active API " Ben Widawsky
2013-07-22 10:42 ` [PATCH 00/12] Completion of i915 VMAs Chris Wilson
2013-07-22 16:35 ` Ben Widawsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130726215615.GQ6084@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=ben@bwidawsk.net \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox