On Mon, Jul 15, 2013 at 08:35:43PM -0700, Ben Widawsky wrote:
> On Sat, Jul 13, 2013 at 11:33:22AM +0200, Daniel Vetter wrote:
> > On Fri, Jul 12, 2013 at 09:45:54PM -0700, Ben Widawsky wrote:
> > > As we plumb the code with more VM information, it has become more
> > > obvious that the easiest way to deal with bind and unbind is to simply
> > > put the function pointers in the vm, and let those choose the correct
> > > way to handle the page table updates. This change allows many places in
> > > the code to simply be vm->bind, and not have to worry about
> > > distinguishing PPGTT vs GGTT.
> > >
> > > NOTE: At some point in the future, brining back insert_entries may in
> > > fact be desirable in order to use 1 bind/unbind for multiple generations
> > > of PPGTT. For now however, it's just not necessary.
> >
> > I need to check the -internal tree again, but I'm rather sure that we need
> > ->insert_entries. In that case I don't want to remove it here in the
> > upstream tree since I have no intention to carry the re-add patch in
> > -internal ;-)
>
> We do use it for i915_ppgtt_bind_object(), however it should be easily
> fixable since the mini-series is exactly about removing
> i915_ppgtt_bind_object, and making into vm->bind_object. I think it's
> fair if you ask me to fix this up on -internal as well, before merging
> it, but with that one exception - I still believe this is the right
> direction to go in.
My idea behind ->bind was that we could us this to hide the aliasing ppgtt
stuff a bit, and otherwise keep things exactly as-is. I haven't actually
looked at -internal to check whether it's as ugly as I expect ;-)
So if you promise to fix up -internal if I come screaming around due to
rebase breakage I'm ok with either option.
> > >
> > > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 9 +++++
> > > drivers/gpu/drm/i915/i915_gem_gtt.c | 72 +++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 81 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index e6694ae..c2a9c98 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -484,9 +484,18 @@ struct i915_address_space {
> > > /* FIXME: Need a more generic return type */
> > > gen6_gtt_pte_t (*pte_encode)(dma_addr_t addr,
> > > enum i915_cache_level level);
> > > +
> > > + /** Unmap an object from an address space. This usually consists of
> > > + * setting the valid PTE entries to a reserved scratch page. */
> > > + void (*unbind_object)(struct i915_address_space *vm,
> > > + struct drm_i915_gem_object *obj);
> >
> > void (*unbind_vma)(struct i915_vma *vma);
> > void (*bind_vma)(struct i915_vma *vma,
> > enum i915_cache_level cache_level);
> >
> > I think if you do this as a follow-up we might as well bikeshed the
> > interface a bit. Again (I know, broken record) for me it feels
> > semantically much cleaner to talk about binding/unbindinig a vma instead
> > of an (obj, vm) pair ...
> >
>
> So as mentioned (and I haven't yet responded to the other email, but
> I'll be broken record there also) - I don't disagree with you. My
> argument is the performance difference should be negligible, and the code
> as is, is decently tested. Changing this requires changing so much, I'd
> rather do the conversion on top. See the other mail thread for more...
Yeah, I agree with the testing argument, sometimes I just want the Perfect
Patch a bit too much ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
