| Message ID | 1379437293-8258-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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?
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
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.
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
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
On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > 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 No, just because the batch once had a ggtt entry doesn't mean the CS is going to use the ggtt for this execution... -Chris
On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > 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 > > No, just because the batch once had a ggtt entry doesn't mean the CS is > going to use the ggtt for this execution... > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre I guess your use of the term, "CS" here is a bit confusing to me, since in my head the CS better do whatever we tell it to do. If you're trying to say, "just because batch_obj has a ggtt binding; that doesn't necessarily mean it's the one to use at dispatch." I think that statement is true, but it's still pretty simple to solve, just use the I915_DISPATCH_SECURE flag to check instead of obj->has_global_gtt_mapping. Right? I'm really sorry about being so dense here. As a side note, I tried really hard to think of how we could end up with a ggtt mapping for batch_obj, and not want to use that one. I'm not actually sure it's possible, but I can't prove it as such, so I'm willing to assume it is possible. Excluding SNB, so few objects actually will get a ggtt mapping, I don't believe any of them should be reused for a batch BO - however, IGT can probably make it happen.
On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote: > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > > 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 > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is > > going to use the ggtt for this execution... > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > I guess your use of the term, "CS" here is a bit confusing to me, since > in my head the CS better do whatever we tell it to do. Exactly, we tell the CS to use the ggtt for the SECURE batch (at least on some generations). > If you're trying to say, "just because batch_obj has a ggtt binding; > that doesn't necessarily mean it's the one to use at dispatch." I think > that statement is true, but it's still pretty simple to solve, just use > the I915_DISPATCH_SECURE flag to check instead of > obj->has_global_gtt_mapping. Right? Yes. With the same caveat that it may change. > I'm really sorry about being so dense here. > > As a side note, I tried really hard to think of how we could end up with > a ggtt mapping for batch_obj, and not want to use that one. I'm not > actually sure it's possible, but I can't prove it as such, so I'm > willing to assume it is possible. Excluding SNB, so few objects actually > will get a ggtt mapping, I don't believe any of them should be reused > for a batch BO - however, IGT can probably make it happen. It's trivial for a batch to end up with a ggtt entry - userspace can just access it through the GTT. Or any bo prior to reusing it as a batch. -Chris
On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote: > On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote: > > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > > > 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 > > > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is > > > going to use the ggtt for this execution... > > > -Chris > > > > > > -- > > > Chris Wilson, Intel Open Source Technology Centre > > > > I guess your use of the term, "CS" here is a bit confusing to me, since > > in my head the CS better do whatever we tell it to do. > > Exactly, we tell the CS to use the ggtt for the SECURE batch (at least > on some generations). > > > If you're trying to say, "just because batch_obj has a ggtt binding; > > that doesn't necessarily mean it's the one to use at dispatch." I think > > that statement is true, but it's still pretty simple to solve, just use > > the I915_DISPATCH_SECURE flag to check instead of > > obj->has_global_gtt_mapping. Right? > > Yes. With the same caveat that it may change. What may change? Dispatch secure always means use the GGTT offset, does it not? Or do you think we'll want privileged batches running from the PPGTT? If the latter is true, why ever use GGTT? > > > I'm really sorry about being so dense here. > > > > As a side note, I tried really hard to think of how we could end up with > > a ggtt mapping for batch_obj, and not want to use that one. I'm not > > actually sure it's possible, but I can't prove it as such, so I'm > > willing to assume it is possible. Excluding SNB, so few objects actually > > will get a ggtt mapping, I don't believe any of them should be reused > > for a batch BO - however, IGT can probably make it happen. > > It's trivial for a batch to end up with a ggtt entry - userspace can > just access it through the GTT. Or any bo prior to reusing it as a > batch. > -Chris Trivial, perhaps on the gtt mapping. It's not really relevant, but is any userspace currently doing that? As for BO reuse, that's a separate problem - are we handing back BOs with their mappings intact? That seems like a security problem. > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Sep 18, 2013 at 08:48:45AM -0700, Ben Widawsky wrote: > On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote: > > On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote: > > > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > > > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > > > > 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 > > > > > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is > > > > going to use the ggtt for this execution... > > > > -Chris > > > > > > > > -- > > > > Chris Wilson, Intel Open Source Technology Centre > > > > > > I guess your use of the term, "CS" here is a bit confusing to me, since > > > in my head the CS better do whatever we tell it to do. > > > > Exactly, we tell the CS to use the ggtt for the SECURE batch (at least > > on some generations). > > > > > If you're trying to say, "just because batch_obj has a ggtt binding; > > > that doesn't necessarily mean it's the one to use at dispatch." I think > > > that statement is true, but it's still pretty simple to solve, just use > > > the I915_DISPATCH_SECURE flag to check instead of > > > obj->has_global_gtt_mapping. Right? > > > > Yes. With the same caveat that it may change. > > What may change? Dispatch secure always means use the GGTT offset, does > it not? Or do you think we'll want privileged batches running from the > PPGTT? If the latter is true, why ever use GGTT? The security bit is already independent from the use-ppgtt bit. With Haswell it should be possible to execute a privileged batch buffer from a ppgtt address, right? In which case we would not need to allocate a GGTT entry. > > > I'm really sorry about being so dense here. > > > > > > As a side note, I tried really hard to think of how we could end up with > > > a ggtt mapping for batch_obj, and not want to use that one. I'm not > > > actually sure it's possible, but I can't prove it as such, so I'm > > > willing to assume it is possible. Excluding SNB, so few objects actually > > > will get a ggtt mapping, I don't believe any of them should be reused > > > for a batch BO - however, IGT can probably make it happen. > > > > It's trivial for a batch to end up with a ggtt entry - userspace can > > just access it through the GTT. Or any bo prior to reusing it as a > > batch. > > -Chris > > Trivial, perhaps on the gtt mapping. It's not really relevant, but is > any userspace currently doing that? As for BO reuse, that's a separate > problem - are we handing back BOs with their mappings intact? That seems > like a security problem. *Userspace* caches its bo with the mappings intact. -Chris
On Wed, Sep 18, 2013 at 04:59:01PM +0100, Chris Wilson wrote: > On Wed, Sep 18, 2013 at 08:48:45AM -0700, Ben Widawsky wrote: > > On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote: > > > On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote: > > > > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > > > > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > > > > > 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 > > > > > > > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is > > > > > going to use the ggtt for this execution... > > > > > -Chris > > > > > > > > > > -- > > > > > Chris Wilson, Intel Open Source Technology Centre > > > > > > > > I guess your use of the term, "CS" here is a bit confusing to me, since > > > > in my head the CS better do whatever we tell it to do. > > > > > > Exactly, we tell the CS to use the ggtt for the SECURE batch (at least > > > on some generations). > > > > > > > If you're trying to say, "just because batch_obj has a ggtt binding; > > > > that doesn't necessarily mean it's the one to use at dispatch." I think > > > > that statement is true, but it's still pretty simple to solve, just use > > > > the I915_DISPATCH_SECURE flag to check instead of > > > > obj->has_global_gtt_mapping. Right? > > > > > > Yes. With the same caveat that it may change. > > > > What may change? Dispatch secure always means use the GGTT offset, does > > it not? Or do you think we'll want privileged batches running from the > > PPGTT? If the latter is true, why ever use GGTT? > > The security bit is already independent from the use-ppgtt bit. With > Haswell it should be possible to execute a privileged batch buffer from > a ppgtt address, right? In which case we would not need to allocate a > GGTT entry. > Right, that was my point. But I *still* fail to see how your earlier request does anything to help this along. The decision can still easily be made at any given time with the I915_DISPATCH_SECURE flag, and per platform driver policy. Say, if you wanted HSW to always run privileged batches out of PPGTT. OTOH, if we need to pass a flag down to specify which address space to execute the batch out of, maybe some more hoops need to be jumped through. I don't see a reason to do this however, and if we want to support IVB, we have to support GGTT execution anyway - so I'm not really sure of a benefit to building in support for PPGTT privileged execution. > > > > I'm really sorry about being so dense here. > > > > > > > > As a side note, I tried really hard to think of how we could end up with > > > > a ggtt mapping for batch_obj, and not want to use that one. I'm not > > > > actually sure it's possible, but I can't prove it as such, so I'm > > > > willing to assume it is possible. Excluding SNB, so few objects actually > > > > will get a ggtt mapping, I don't believe any of them should be reused > > > > for a batch BO - however, IGT can probably make it happen. > > > > > > It's trivial for a batch to end up with a ggtt entry - userspace can > > > just access it through the GTT. Or any bo prior to reusing it as a > > > batch. > > > -Chris > > > > Trivial, perhaps on the gtt mapping. It's not really relevant, but is > > any userspace currently doing that? As for BO reuse, that's a separate > > problem - are we handing back BOs with their mappings intact? That seems > > like a security problem. > > *Userspace* caches its bo with the mappings intact. > -Chris Yes, this seems like a potential (albeit small) problem to me if we (the kernel) arbitrarily upgrade BOs to a GGTT mapping. I guess everything running privileged is trusted though, so we don't need to worry about the unintentional global BOs being snooped. It does sort of seem to circumvent real PPGTT to some extent though if the global mappings linger. Let me state that at this point of the thread, I am lost. Do you still want the original change you asked for? I still don't understand, or see a reason for not just quirking away with a quick check (which I'll state again doesn't even matter until patches which haven't yet been written are posted) - but I clearly haven't been able to convince you; and nobody else is stepping in.
On Wed, Sep 18, 2013 at 09:11:56AM -0700, Ben Widawsky wrote: > On Wed, Sep 18, 2013 at 04:59:01PM +0100, Chris Wilson wrote: > > On Wed, Sep 18, 2013 at 08:48:45AM -0700, Ben Widawsky wrote: > > > On Wed, Sep 18, 2013 at 03:53:37PM +0100, Chris Wilson wrote: > > > > On Wed, Sep 18, 2013 at 07:47:45AM -0700, Ben Widawsky wrote: > > > > > On Wed, Sep 18, 2013 at 09:30:17AM +0100, Chris Wilson wrote: > > > > > > On Tue, Sep 17, 2013 at 05:02:03PM -0700, Ben Widawsky wrote: > > > > > > > > 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 > > > > > > > > > > > > No, just because the batch once had a ggtt entry doesn't mean the CS is > > > > > > going to use the ggtt for this execution... > > > > > > -Chris > > > > > > > > > > > > -- > > > > > > Chris Wilson, Intel Open Source Technology Centre > > > > > > > > > > I guess your use of the term, "CS" here is a bit confusing to me, since > > > > > in my head the CS better do whatever we tell it to do. > > > > > > > > Exactly, we tell the CS to use the ggtt for the SECURE batch (at least > > > > on some generations). > > > > > > > > > If you're trying to say, "just because batch_obj has a ggtt binding; > > > > > that doesn't necessarily mean it's the one to use at dispatch." I think > > > > > that statement is true, but it's still pretty simple to solve, just use > > > > > the I915_DISPATCH_SECURE flag to check instead of > > > > > obj->has_global_gtt_mapping. Right? > > > > > > > > Yes. With the same caveat that it may change. > > > > > > What may change? Dispatch secure always means use the GGTT offset, does > > > it not? Or do you think we'll want privileged batches running from the > > > PPGTT? If the latter is true, why ever use GGTT? > > > > The security bit is already independent from the use-ppgtt bit. With > > Haswell it should be possible to execute a privileged batch buffer from > > a ppgtt address, right? In which case we would not need to allocate a > > GGTT entry. > > > > Right, that was my point. But I *still* fail to see how your earlier > request does anything to help this along. The decision can still easily > be made at any given time with the I915_DISPATCH_SECURE flag, and per > platform driver policy. Say, if you wanted HSW to always run privileged > batches out of PPGTT. OTOH, if we need to pass a flag down to specify > which address space to execute the batch out of, maybe some more hoops > need to be jumped through. I don't see a reason to do this however, and > if we want to support IVB, we have to support GGTT execution anyway - so > I'm not really sure of a benefit to building in support for PPGTT > privileged execution. > > > > > > > I'm really sorry about being so dense here. > > > > > > > > > > As a side note, I tried really hard to think of how we could end up with > > > > > a ggtt mapping for batch_obj, and not want to use that one. I'm not > > > > > actually sure it's possible, but I can't prove it as such, so I'm > > > > > willing to assume it is possible. Excluding SNB, so few objects actually > > > > > will get a ggtt mapping, I don't believe any of them should be reused > > > > > for a batch BO - however, IGT can probably make it happen. > > > > > > > > It's trivial for a batch to end up with a ggtt entry - userspace can > > > > just access it through the GTT. Or any bo prior to reusing it as a > > > > batch. > > > > -Chris > > > > > > Trivial, perhaps on the gtt mapping. It's not really relevant, but is > > > any userspace currently doing that? As for BO reuse, that's a separate > > > problem - are we handing back BOs with their mappings intact? That seems > > > like a security problem. > > > > *Userspace* caches its bo with the mappings intact. > > -Chris > > Yes, this seems like a potential (albeit small) problem to me if we (the > kernel) arbitrarily upgrade BOs to a GGTT mapping. I guess everything > running privileged is trusted though, so we don't need to worry about > the unintentional global BOs being snooped. It does sort of seem to > circumvent real PPGTT to some extent though if the global mappings > linger. > > Let me state that at this point of the thread, I am lost. Do you still > want the original change you asked for? I still don't understand, or see > a reason for not just quirking away with a quick check (which I'll state > again doesn't even matter until patches which haven't yet been written > are posted) - but I clearly haven't been able to convince you; and > nobody else is stepping in. Yes, I want the bug in the code fixed. -Chris
On Wed, Sep 18, 2013 at 6:15 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Yes, I want the bug in the code fixed.
I guess what Ben's trying to say is that right now we don't yet have a
bug (since we lack the ppgtt address space). But I agree that the fix
Ben pointed at in this thread of using obj->has_global_mapping won't
work, we need to pick the address space for the batch offset according
to the SECURE_DISPATCH flag. And we also need to make sure that we
actually have the global mapping around.
Aside: Batch security and ppgtt aren't even fully untangled on hsw,
afaik only on the render ring do we have seperate bits.
-Daniel
On Wed, Sep 18, 2013 at 06:20:23PM +0200, Daniel Vetter wrote: > On Wed, Sep 18, 2013 at 6:15 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Yes, I want the bug in the code fixed. > > I guess what Ben's trying to say is that right now we don't yet have a > bug (since we lack the ppgtt address space). But I agree that the fix > Ben pointed at in this thread of using obj->has_global_mapping won't > work, we need to pick the address space for the batch offset according > to the SECURE_DISPATCH flag. And we also need to make sure that we > actually have the global mapping around. > > Aside: Batch security and ppgtt aren't even fully untangled on hsw, > afaik only on the render ring do we have seperate bits. > -Daniel I see it now. He was trying to solve my trivial bug with the grander longer term solution. As he requested, I'll just fix the bug for now, and we can worry about multiple VM support later.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 686a66c..ab88b43 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2085,17 +2085,8 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, /* i915_gem_gtt.c */ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev); -void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level); -void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj); - void i915_gem_restore_gtt_mappings(struct drm_device *dev); int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj); -void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level); -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj); void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); void i915_gem_init_global_gtt(struct drm_device *dev); void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 651b91c..4e6f20a 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2693,12 +2693,8 @@ int i915_vma_unbind(struct i915_vma *vma) trace_i915_vma_unbind(vma); - if (obj->has_global_gtt_mapping) - i915_gem_gtt_unbind_object(obj); - if (obj->has_aliasing_ppgtt_mapping) { - i915_ppgtt_unbind_object(dev_priv->mm.aliasing_ppgtt, obj); - obj->has_aliasing_ppgtt_mapping = 0; - } + vma->vm->unbind_vma(vma); + i915_gem_gtt_finish_object(obj); i915_gem_object_unpin_pages(obj); @@ -3424,7 +3420,6 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, enum i915_cache_level cache_level) { struct drm_device *dev = obj->base.dev; - drm_i915_private_t *dev_priv = dev->dev_private; struct i915_vma *vma; int ret; @@ -3463,11 +3458,8 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, return ret; } - if (obj->has_global_gtt_mapping) - i915_gem_gtt_bind_object(obj, cache_level); - if (obj->has_aliasing_ppgtt_mapping) - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, - obj, cache_level); + list_for_each_entry(vma, &obj->vma_list, vma_link) + vma->vm->bind_vma(vma, cache_level, 0); } list_for_each_entry(vma, &obj->vma_list, vma_link) @@ -3795,6 +3787,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, bool map_and_fenceable, bool nonblocking) { + const u32 flags = map_and_fenceable ? GLOBAL_BIND : 0; struct i915_vma *vma; int ret; @@ -3823,20 +3816,22 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj, } if (!i915_gem_obj_bound(obj, vm)) { - struct drm_i915_private *dev_priv = obj->base.dev->dev_private; - ret = i915_gem_object_bind_to_vm(obj, vm, alignment, map_and_fenceable, nonblocking); if (ret) return ret; - if (!dev_priv->mm.aliasing_ppgtt) - i915_gem_gtt_bind_object(obj, obj->cache_level); - } + vma = i915_gem_obj_to_vma(obj, vm); + vm->bind_vma(vma, obj->cache_level, flags); + } else + vma = i915_gem_obj_to_vma(obj, vm); + /* Objects are created map and fenceable. If we bind an object + * the first time, and we had aliasing PPGTT (and didn't request + * GLOBAL), we'll need to do this on the second bind.*/ if (!obj->has_global_gtt_mapping && map_and_fenceable) - i915_gem_gtt_bind_object(obj, obj->cache_level); + vm->bind_vma(vma, obj->cache_level, GLOBAL_BIND); obj->pin_count++; obj->pin_mappable |= map_and_fenceable; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index cb3b7e8..a030739 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -391,6 +391,7 @@ mi_set_context(struct intel_ring_buffer *ring, static int do_switch(struct i915_hw_context *to) { struct intel_ring_buffer *ring = to->ring; + struct drm_i915_private *dev_priv = ring->dev->dev_private; struct i915_hw_context *from = ring->last_context; u32 hw_flags = 0; int ret; @@ -415,8 +416,11 @@ static int do_switch(struct i915_hw_context *to) return ret; } - if (!to->obj->has_global_gtt_mapping) - i915_gem_gtt_bind_object(to->obj, to->obj->cache_level); + if (!to->obj->has_global_gtt_mapping) { + struct i915_vma *vma = i915_gem_obj_to_vma(to->obj, + &dev_priv->gtt.base); + vma->vm->bind_vma(vma, to->obj->cache_level, GLOBAL_BIND); + } if (!to->is_initialized || is_default_context(to)) hw_flags |= MI_RESTORE_INHIBIT; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index b26d979..b85e2dc 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -286,8 +286,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, if (unlikely(IS_GEN6(dev) && reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION && !target_i915_obj->has_global_gtt_mapping)) { - i915_gem_gtt_bind_object(target_i915_obj, - target_i915_obj->cache_level); + struct i915_vma *vma = i915_gem_obj_to_vma(obj, vm); + vma->vm->bind_vma(vma, target_i915_obj->cache_level, + GLOBAL_BIND); } /* Validate that the target is in a valid r/w GPU domain */ @@ -464,11 +465,12 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, struct intel_ring_buffer *ring, bool *need_reloc) { - struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct drm_i915_gem_object *obj = vma->obj; struct drm_i915_gem_exec_object2 *entry = vma->exec_entry; bool has_fenced_gpu_access = INTEL_INFO(ring->dev)->gen < 4; bool need_fence, need_mappable; - struct drm_i915_gem_object *obj = vma->obj; + u32 flags = (entry->flags & EXEC_OBJECT_NEEDS_GTT) && + !vma->obj->has_global_gtt_mapping ? GLOBAL_BIND : 0; int ret; need_fence = @@ -497,14 +499,6 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, } } - /* Ensure ppgtt mapping exists if needed */ - if (dev_priv->mm.aliasing_ppgtt && !obj->has_aliasing_ppgtt_mapping) { - i915_ppgtt_bind_object(dev_priv->mm.aliasing_ppgtt, - obj, obj->cache_level); - - obj->has_aliasing_ppgtt_mapping = 1; - } - if (entry->offset != vma->node.start) { entry->offset = vma->node.start; *need_reloc = true; @@ -515,9 +509,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, obj->base.pending_write_domain = I915_GEM_DOMAIN_RENDER; } - if (entry->flags & EXEC_OBJECT_NEEDS_GTT && - !obj->has_global_gtt_mapping) - i915_gem_gtt_bind_object(obj, obj->cache_level); + vma->vm->bind_vma(vma, obj->cache_level, flags); return 0; } @@ -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); + } ret = i915_gem_execbuffer_move_to_gpu(ring, &eb->vmas); if (ret) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0ea40b3..7e4a308 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -437,15 +437,6 @@ void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev) dev_priv->mm.aliasing_ppgtt = NULL; } -void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level) -{ - ppgtt->base.insert_entries(&ppgtt->base, obj->pages, - i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT, - cache_level); -} - static void __always_unused gen6_ppgtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, @@ -458,14 +449,6 @@ gen6_ppgtt_bind_vma(struct i915_vma *vma, gen6_ppgtt_insert_entries(vma->vm, vma->obj->pages, entry, cache_level); } -void i915_ppgtt_unbind_object(struct i915_hw_ppgtt *ppgtt, - struct drm_i915_gem_object *obj) -{ - ppgtt->base.clear_range(&ppgtt->base, - i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT, - obj->base.size >> PAGE_SHIFT); -} - static void __always_unused gen6_ppgtt_unbind_vma(struct i915_vma *vma) { const unsigned long entry = vma->node.start >> PAGE_SHIFT; @@ -523,8 +506,10 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) dev_priv->gtt.base.total / PAGE_SIZE); list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) { + struct i915_vma *vma = i915_gem_obj_to_vma(obj, + &dev_priv->gtt.base); i915_gem_clflush_object(obj, obj->pin_display); - i915_gem_gtt_bind_object(obj, obj->cache_level); + vma->vm->bind_vma(vma, obj->cache_level, 0); } i915_gem_chipset_flush(dev); @@ -687,33 +672,6 @@ static void gen6_ggtt_bind_vma(struct i915_vma *vma, } } -void i915_gem_gtt_bind_object(struct drm_i915_gem_object *obj, - enum i915_cache_level cache_level) -{ - struct drm_device *dev = obj->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; - - dev_priv->gtt.base.insert_entries(&dev_priv->gtt.base, obj->pages, - entry, - cache_level); - - obj->has_global_gtt_mapping = 1; -} - -void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj) -{ - struct drm_device *dev = obj->base.dev; - struct drm_i915_private *dev_priv = dev->dev_private; - const unsigned long entry = i915_gem_obj_ggtt_offset(obj) >> PAGE_SHIFT; - - dev_priv->gtt.base.clear_range(&dev_priv->gtt.base, - entry, - obj->base.size >> PAGE_SHIFT); - - obj->has_global_gtt_mapping = 0; -} - static void gen6_ggtt_unbind_vma(struct i915_vma *vma) { struct drm_device *dev = vma->vm->dev;