From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/3] drm/i915: Add bind/unbind object functions to VM Date: Tue, 16 Jul 2013 07:13:54 +0200 Message-ID: <20130716051354.GC5784@phenom.ffwll.local> References: <1373350122-5118-1-git-send-email-ben@bwidawsk.net> <1373690756-15156-1-git-send-email-ben@bwidawsk.net> <1373690756-15156-2-git-send-email-ben@bwidawsk.net> <20130713093322.GM6143@phenom.ffwll.local> <20130716033543.GB634@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f178.google.com (mail-ea0-f178.google.com [209.85.215.178]) by gabe.freedesktop.org (Postfix) with ESMTP id 4EC55E5CD6 for ; Mon, 15 Jul 2013 22:13:53 -0700 (PDT) Received: by mail-ea0-f178.google.com with SMTP id l15so100425eak.23 for ; Mon, 15 Jul 2013 22:13:52 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130716033543.GB634@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 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 > > > --- > > > 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