[Intel-gfx] [PATCH 06/11] drm/i915: plumb VM into object operations

[Daniel Vetter]

On Thu, Jul 11, 2013 at 12:23 AM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Wed, Jul 10, 2013 at 07:05:52PM +0200, Daniel Vetter wrote:
>> On Wed, Jul 10, 2013 at 09:37:10AM -0700, Ben Widawsky wrote:
>> > On Tue, Jul 09, 2013 at 09:15:01AM +0200, Daniel Vetter wrote:
>> > > On Mon, Jul 08, 2013 at 11:08:37PM -0700, Ben Widawsky wrote:
>> > > > This patch was formerly known as:
>> > > > "drm/i915: Create VMAs (part 3) - plumbing"
>> > > >
>> > > > This patch adds a VM argument, bind/unbind, and the object
>> > > > offset/size/color getters/setters. It preserves the old ggtt helper
>> > > > functions because things still need, and will continue to need them.
>> > > >
>> > > > Some code will still need to be ported over after this.
>> > > >
>> > > > v2: Fix purge to pick an object and unbind all vmas
>> > > > This was doable because of the global bound list change.
>> > > >
>> > > > v3: With the commit to actually pin/unpin pages in place, there is no
>> > > > longer a need to check if unbind succeeded before calling put_pages().
>> > > > Make put_pages only BUG() after checking pin count.
>> > > >
>> > > > v4: Rebased on top of the new hangcheck work by Mika
>> > > > plumbed eb_destroy also
>> > > > Many checkpatch related fixes
>> > > >
>> > > > v5: Very large rebase
>> > > >
>> > > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> > >
>> > > This one is a rather large beast. Any chance we could split it into
>> > > topics, e.g. convert execbuf code, convert shrinker code? Or does that get
>> > > messy, fast?
>> > >
>> >
>> > I've thought of this...
>> >
>> > The one solution I came up with is to have two bind/unbind functions
>> > (similar to what I did with pin, and indeed it was my original plan with
>> > pin), and do the set_caching one separately.
>> >
>> > I think it won't be too messy, just a lot of typing, as Keith likes to
>> > say.
>> >
>> > However, my opinion was, since it's early in the merge cycle, we don't
>> > yet have multiple VMs, and it's /mostly/ a copypasta kind of patch, it's
>> > not a big deal. At a functional level too, I felt this made more sense.
>> >
>> > So I'll defer to your request on this and start splitting it up, unless
>> > my email has changed your mind ;-).
>>
>> Well, my concern is mostly in reviewing since we need to think about each
>> case and whether it makes sense to talk in therms of vma or objects in
>> that function. And what exactly to test.
>>
>> If you've played around and concluded it'll be a mess then I don't think
>> it'll help in reviewing. So pointless.
>
> I said I don't think it will be a mess, though I feel it won't really
> help review too much. Can you take a crack and review and poke me if you
> want me to try it. I'd rather not do it if I can avoid it, so I can try
> to go back to my 15 patch maximum rule.
>
>>
>> Still, there's a bunch of questions on this patch that we need to discuss
>> ;-)
>
> Ready whenever.

It's waiting for you in my first reply, just scroll down a bit ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch