[Intel-gfx] [PATCH 1/3] drm/i915: Add bind/unbind object functions to VM

[Daniel Vetter]

On Mon, Jul 15, 2013 at 09:00:54PM -0700, Ben Widawsky wrote:
> On Mon, Jul 15, 2013 at 08:35:43PM -0700, Ben Widawsky wrote:
> > To me, aliasing ppgtt is just a wart that doesn't fit well with
> > anything. As such, my plan was to hide as much of it as possible in ggtt
> > functions. Using some kind of flag on ggtt_bind() we can determine if
> > the user actually wants ggtt, and if so bind to both, else just use
> > aliasing ppgtt. None of that code appears here because I want to make
> > the diff churn as small as possible, and hadn't completely thought it
> > all through.
> > 
> > Now after typing that (and this really did happen), I just looked at
> > your function, and it seems to be more or less exactly what I just
> > typed. Cool! The GPU/CPU naming scheme seems off to me, and I think you
> > really just want one flag which specifies "bind it in the global gtt,
> > sucka"

Feel free to bikeshed the names, I tend to not be really good at that ;-)
I guess a flag to switch between binding to ppgtt or ggtt would also work,
I just slowly started to dislike make bool arguments and favour explicitly
named flags/enums more. Better self-documenting code ...

> > Now having just typed /that/, it was indeed my plan. So as long as
> > nothing really bothers you with the bind/unbind() stuff, I can move
> > forward with a patch on top to fix it.
> > 
> 
> I changed my mind already. A patch on top doesn't make sense. I'll try
> to fix this one up as is.

Yeah, overall the thing that irks me is that you've added ppgtt bind
functions despite that we don't yet bind anything into any real ppgtt
address space. I'd recommend to just implement the ggtt bind functions
(with the aliasing crap added) and then add the ppgtt bind functions once
we get nearer to actually using them for real.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch