From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH] drm/i915: Use the new vm [un]bind functions Date: Fri, 20 Sep 2013 14:08:34 -0700 Message-ID: <20130920210833.GD22399@bwidawsk.net> References: <1379601919-22742-1-git-send-email-benjamin.widawsky@intel.com> <1379649999-16923-1-git-send-email-benjamin.widawsky@intel.com> <20130920104348.GD24200@nuc-i3427.alporthouse.com> <20130920204422.GB22399@bwidawsk.net> <20130920205551.GA16830@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail.bwidawsk.net (bwidawsk.net [166.78.191.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 91215E5DC4 for ; Fri, 20 Sep 2013 14:08:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130920205551.GA16830@nuc-i3427.alporthouse.com> 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: Chris Wilson , Ben Widawsky , Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Fri, Sep 20, 2013 at 09:55:51PM +0100, Chris Wilson wrote: > On Fri, Sep 20, 2013 at 01:44:23PM -0700, Ben Widawsky wrote: > > On Fri, Sep 20, 2013 at 11:43:48AM +0100, Chris Wilson wrote: > > > On Thu, Sep 19, 2013 at 09:06:39PM -0700, Ben Widawsky wrote: > > > > @@ -1117,8 +1114,25 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > > > * batch" bit. Hence we need to pin secure batches into the global gtt. > > > > * hsw should have this fixed, but let's be paranoid and do it > > > > * unconditionally for now. */ > > > > - if (flags & I915_DISPATCH_SECURE && !batch_obj->has_global_gtt_mapping) > > > > - i915_gem_gtt_bind_object(batch_obj, batch_obj->cache_level); > > > > + if (flags & I915_DISPATCH_SECURE) { > > > > + struct i915_address_space *ggtt = obj_to_ggtt(batch_obj); > > > > + /* Assuming all privileged batches are in the global GTT means > > > > + * we need to make sure we have a global gtt offset, as well as > > > > + * the PTEs mapped. As mentioned above, we can forego this on > > > > + * HSW, but don't. > > > > + */ > > > > + ret = i915_gem_object_bind_to_vm(batch_obj, ggtt, 0, false, > > > > + false); > > > > + if (ret) > > > > + goto err; > > > > > > bind_to_vm() has unwanted side-effects here - notably always allocating > > > a node and corrupting lists. > > > > > > Just pin, ggtt->bind_vma, unpin. Hmmm, except that we also need a > > > move_to_active (as we are not presuming vm == ggtt). > > > > > > pin, ggtt->bind_vma, move_to_active(ggtt), unpin. > > > > > > And then hope we have the correct flushes in place for that to be > > > retired if nothing else is going on with that ggtt. > > > > Yes, you're right, and a particular nice catch on the move to active; I > > completely forgot. I think ggtt->bind_vma is redundant though. Shouldn't > > it just be: > > pin, move_to_active, unpin? > > Since we will ask for a !map_and_fenceable pin, pin() will not > automatically bind into the global GTT, so I think we still need the > ggtt->bind_vma(). > pin gets passed a VM, which will be GGTT. > > > Furthermore, the actually pinning (pin count increment) should be > > unnecessary, but I assume you were just trying to save me some typing. > > Yes, the pin-count adjustments should be unnecessary - but not a huge > burden, and I was thinking it may help in the future as we may want to > explicitly hold the pin until move-to-active for all objects. That > future being where we strive to reduce hold times on struct_mutex. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- Ben Widawsky, Intel Open Source Technology Center