From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 5/6] [v3] drm/i915: Use the new vm [un]bind functions Date: Tue, 17 Sep 2013 17:02:03 -0700 Message-ID: <20130918000203.GF6112@bwidawsk.net> References: <20130916073705.GA27478@nuc-i3427.alporthouse.com> <1379437293-8258-1-git-send-email-benjamin.widawsky@intel.com> <20130917205535.GA9223@nuc-i3427.alporthouse.com> <20130917231442.GB6112@bwidawsk.net> <20130917233332.GE24051@nuc-i3427.alporthouse.com> <20130917234850.GC6112@bwidawsk.net> <20130917235720.GF24051@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 43671E772A for ; Tue, 17 Sep 2013 17:02:12 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130917235720.GF24051@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 Wed, Sep 18, 2013 at 12:57:20AM +0100, Chris Wilson wrote: > On Tue, Sep 17, 2013 at 04:48:50PM -0700, Ben Widawsky wrote: > > On Wed, Sep 18, 2013 at 12:33:32AM +0100, Chris Wilson wrote: > > > On Tue, Sep 17, 2013 at 04:14:43PM -0700, Ben Widawsky wrote: > > > > On Tue, Sep 17, 2013 at 09:55:35PM +0100, Chris Wilson wrote: > > > > > On Tue, Sep 17, 2013 at 10:01:33AM -0700, Ben Widawsky wrote: > > > > > > @@ -1117,8 +1109,13 @@ 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 && > > > > > > + !batch_obj->has_global_gtt_mapping) { > > > > > > + const struct i915_address_space *ggtt = obj_to_ggtt(batch_obj); > > > > > > + struct i915_vma *vma = i915_gem_obj_to_ggtt(batch_obj); > > > > > > + BUG_ON(!vma); > > > > > > + ggtt->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND); > > > > > > + } > > > > > > > > > > The issue here is that if we don't set the USE_PPGTT/USE_SECURE flag in > > > > > the dispatch, the CS will use the GGTT (hence our binding) but so we > > > > > then need to use the GGTT offset for the dispatch as well. > > > > > > > > > > Is that as concisely as we can write bind_to_ggtt? :( > > > > > -Chris > > > > > > > > > > > > > Resuming the conversation started on irc... what do you want from me? > > > > > > I think we need to pass the ggtt offset to dispatch for > > > I915_DISPATCH_SECURE -- which offset to use might even depend upon the > > > implementation and hw generation in intel_ringbuffer.c. But at the very > > > least, I think SNB/IVB will be executing the wrong address come full > > > ppgtt. > > > > > > dev_priv->ggtt.base.bind_vma(i915_gem_obj_to_ggtt(batch_obj), > > > batch_obj->cache_level, > > > GLOBAL_BIND); > > > > > > #define i915_vm_bind(vm__, vma__, cache_level__, flags__) \ > > > (vm__)->bind_vma((vma__), (cache_level__), (flags__)) > > > > > > i915_vm_bind(&dev_priv->ggtt.base, > > > i915_gem_obj_to_ggtt(batch_obj), > > > batch_obj->cache_level, > > > GLOBAL_BIND); > > > -Chris > > > > > > > I915_DISPATCH_SECURE is a special case. If we see the flag, we look up > > the GGTT offset as opposed to the offset in the VM being used at > > execbuf. We can either bind the batchbuffer into both the PPGTT, and > > GGTT, or it's only even in the GGTT - in either case, we'll have to have > > done the bind (and found space in the drm_mm). It just seems like this > > is duplicating the already existing i915_gem_object_bind_to_vm code > > that's in place. > > > > Sorry if I am not following what you're asking. I'm just failing to see > > a problem, or maybe you're just trying to solve problems that I haven't > > yet conceived; or solved in a different way. It's pretty darn hard to > > discuss this given the piecemeal nature of the thing. > > The code does > > exec_start = i915_gem_obj_offset(batch_obj, vm) + > args->batch_start_offset; > exec_len = args->batch_len; > ... > ret = ring->dispatch_execbuffer(ring, > exec_start, exec_len, > flags); > if (ret) > goto err; > > So we lookup the address of the batch buffer in the wrong vm for > I915_DISPATCH_SECURE. > -Chris > But this is very easily solved, no? http://cgit.freedesktop.org/~bwidawsk/drm-intel/tree/drivers/gpu/drm/i915/i915_gem_execbuffer.c?h=ppgtt#n1083 -- Ben Widawsky, Intel Open Source Technology Center