* [PATCH] drm/i915: Fix secure dispatch with full ppgtt
@ 2014-08-11 10:05 Daniel Vetter
2014-08-11 10:08 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-08-11 10:05 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
Based upon a hunk from a patch from Chris Wilson, but augmented to:
- Process the batch in the full ppgtt vm so that self-relocations
match again with userspace's expectations..
- Add a comment why plain pin for the global gtt binding is safe at
that point.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 38 ++++++++++++++----------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6320a385841b..697b3d5f161a 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -138,13 +138,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
goto err;
}
- /* If we have secure dispatch, or the userspace assures us that
- * they know what they're doing, use the GGTT VM.
- */
- if (((args->flags & I915_EXEC_SECURE) &&
- (i == (args->buffer_count - 1))))
- bind_vm = &dev_priv->gtt.base;
-
obj = list_first_entry(&objects,
struct drm_i915_gem_object,
obj_exec_link);
@@ -1387,25 +1380,30 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
* batch" bit. Hence we need to pin secure batches into the global gtt.
* hsw should have this fixed, but bdw mucks it up again. */
- if (flags & I915_DISPATCH_SECURE &&
- !batch_obj->has_global_gtt_mapping) {
- /* When we have multiple VMs, we'll need to make sure that we
- * allocate space first */
- struct i915_vma *vma = i915_gem_obj_to_ggtt(batch_obj);
- BUG_ON(!vma);
- vma->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND);
- }
+ if (flags & I915_DISPATCH_SECURE) {
+ /*
+ * So on first glance it looks freaky that we pin the batch here
+ * outside of the reservation loop. But:
+ * - The batch is already pinned into the relevant ppgtt, so we
+ * already have the backing storage fully allocated.
+ * - No other BO uses the global gtt (well contexts, but meh),
+ * so we don't really have issues with mutliple objects not
+ * fitting due to fragmentation.
+ * So this is actually safe.
+ */
+ ret = i915_gem_obj_ggtt_pin(batch_obj, 0, 0);
+ if (ret)
+ goto err;
- if (flags & I915_DISPATCH_SECURE)
exec_start += i915_gem_obj_ggtt_offset(batch_obj);
- else
+ } else
exec_start += i915_gem_obj_offset(batch_obj, vm);
ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
- args, &eb->vmas, batch_obj, exec_start, flags);
- if (ret)
- goto err;
+ args, &eb->vmas, batch_obj, exec_start, flags);
+ if (flags & I915_DISPATCH_SECURE)
+ i915_gem_object_ggtt_unpin(batch_obj);
err:
/* the request owns the ref now */
i915_gem_context_unreference(ctx);
--
2.0.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Fix secure dispatch with full ppgtt
2014-08-11 10:05 [PATCH] drm/i915: Fix secure dispatch with full ppgtt Daniel Vetter
@ 2014-08-11 10:08 ` Daniel Vetter
2014-08-11 10:17 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-08-11 10:08 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Ben Widawsky
Based upon a hunk from a patch from Chris Wilson, but augmented to:
- Process the batch in the full ppgtt vm so that self-relocations
match again with userspace's expectations..
- Add a comment why plain pin for the global gtt binding is safe at
that point.
v2: Drop local bind_vm variable (Chris).
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 42 ++++++++++++++----------------
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6320a385841b..01fa9aa2b90e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -95,7 +95,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
struct i915_address_space *vm,
struct drm_file *file)
{
- struct drm_i915_private *dev_priv = vm->dev->dev_private;
struct drm_i915_gem_object *obj;
struct list_head objects;
int i, ret;
@@ -130,7 +129,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
i = 0;
while (!list_empty(&objects)) {
struct i915_vma *vma;
- struct i915_address_space *bind_vm = vm;
if (exec[i].flags & EXEC_OBJECT_NEEDS_GTT &&
USES_FULL_PPGTT(vm->dev)) {
@@ -138,13 +136,6 @@ eb_lookup_vmas(struct eb_vmas *eb,
goto err;
}
- /* If we have secure dispatch, or the userspace assures us that
- * they know what they're doing, use the GGTT VM.
- */
- if (((args->flags & I915_EXEC_SECURE) &&
- (i == (args->buffer_count - 1))))
- bind_vm = &dev_priv->gtt.base;
-
obj = list_first_entry(&objects,
struct drm_i915_gem_object,
obj_exec_link);
@@ -157,7 +148,7 @@ eb_lookup_vmas(struct eb_vmas *eb,
* from the (obj, vm) we don't run the risk of creating
* duplicated vmas for the same vm.
*/
- vma = i915_gem_obj_lookup_or_create_vma(obj, bind_vm);
+ vma = i915_gem_obj_lookup_or_create_vma(obj, vm);
if (IS_ERR(vma)) {
DRM_DEBUG("Failed to lookup VMA\n");
ret = PTR_ERR(vma);
@@ -1387,25 +1378,30 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
/* snb/ivb/vlv conflate the "batch in ppgtt" bit with the "non-secure
* batch" bit. Hence we need to pin secure batches into the global gtt.
* hsw should have this fixed, but bdw mucks it up again. */
- if (flags & I915_DISPATCH_SECURE &&
- !batch_obj->has_global_gtt_mapping) {
- /* When we have multiple VMs, we'll need to make sure that we
- * allocate space first */
- struct i915_vma *vma = i915_gem_obj_to_ggtt(batch_obj);
- BUG_ON(!vma);
- vma->bind_vma(vma, batch_obj->cache_level, GLOBAL_BIND);
- }
+ if (flags & I915_DISPATCH_SECURE) {
+ /*
+ * So on first glance it looks freaky that we pin the batch here
+ * outside of the reservation loop. But:
+ * - The batch is already pinned into the relevant ppgtt, so we
+ * already have the backing storage fully allocated.
+ * - No other BO uses the global gtt (well contexts, but meh),
+ * so we don't really have issues with mutliple objects not
+ * fitting due to fragmentation.
+ * So this is actually safe.
+ */
+ ret = i915_gem_obj_ggtt_pin(batch_obj, 0, 0);
+ if (ret)
+ goto err;
- if (flags & I915_DISPATCH_SECURE)
exec_start += i915_gem_obj_ggtt_offset(batch_obj);
- else
+ } else
exec_start += i915_gem_obj_offset(batch_obj, vm);
ret = legacy_ringbuffer_submission(dev, file, ring, ctx,
- args, &eb->vmas, batch_obj, exec_start, flags);
- if (ret)
- goto err;
+ args, &eb->vmas, batch_obj, exec_start, flags);
+ if (flags & I915_DISPATCH_SECURE)
+ i915_gem_object_ggtt_unpin(batch_obj);
err:
/* the request owns the ref now */
i915_gem_context_unreference(ctx);
--
2.0.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix secure dispatch with full ppgtt
2014-08-11 10:08 ` Daniel Vetter
@ 2014-08-11 10:17 ` Chris Wilson
2014-08-11 11:22 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-08-11 10:17 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Ben Widawsky
On Mon, Aug 11, 2014 at 12:08:58PM +0200, Daniel Vetter wrote:
> Based upon a hunk from a patch from Chris Wilson, but augmented to:
> - Process the batch in the full ppgtt vm so that self-relocations
> match again with userspace's expectations..
> - Add a comment why plain pin for the global gtt binding is safe at
> that point.
>
> v2: Drop local bind_vm variable (Chris).
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
/me jaw drops
Oh nice, yes you are right that does fix the issue with the relocations
being in one address space whilst the exec needs to be done from gGTT.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix secure dispatch with full ppgtt
2014-08-11 10:17 ` Chris Wilson
@ 2014-08-11 11:22 ` Daniel Vetter
2014-08-11 11:35 ` Chris Wilson
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-08-11 11:22 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Intel Graphics Development,
Ben Widawsky
On Mon, Aug 11, 2014 at 11:17:40AM +0100, Chris Wilson wrote:
> On Mon, Aug 11, 2014 at 12:08:58PM +0200, Daniel Vetter wrote:
> > Based upon a hunk from a patch from Chris Wilson, but augmented to:
> > - Process the batch in the full ppgtt vm so that self-relocations
> > match again with userspace's expectations..
> > - Add a comment why plain pin for the global gtt binding is safe at
> > that point.
> >
> > v2: Drop local bind_vm variable (Chris).
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> /me jaw drops
>
> Oh nice, yes you are right that does fix the issue with the relocations
> being in one address space whilst the exec needs to be done from gGTT.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Actually it's not that simple - with this patch we don't add the ggtt vma
for the batch to the active list, which we probably should. So needs to be
revised a bit more I think ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix secure dispatch with full ppgtt
2014-08-11 11:22 ` Daniel Vetter
@ 2014-08-11 11:35 ` Chris Wilson
2014-08-11 11:51 ` Daniel Vetter
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2014-08-11 11:35 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, Ben Widawsky
On Mon, Aug 11, 2014 at 01:22:19PM +0200, Daniel Vetter wrote:
> On Mon, Aug 11, 2014 at 11:17:40AM +0100, Chris Wilson wrote:
> > On Mon, Aug 11, 2014 at 12:08:58PM +0200, Daniel Vetter wrote:
> > > Based upon a hunk from a patch from Chris Wilson, but augmented to:
> > > - Process the batch in the full ppgtt vm so that self-relocations
> > > match again with userspace's expectations..
> > > - Add a comment why plain pin for the global gtt binding is safe at
> > > that point.
> > >
> > > v2: Drop local bind_vm variable (Chris).
> > >
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > /me jaw drops
> >
> > Oh nice, yes you are right that does fix the issue with the relocations
> > being in one address space whilst the exec needs to be done from gGTT.
> >
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Actually it's not that simple - with this patch we don't add the ggtt vma
> for the batch to the active list, which we probably should. So needs to be
> revised a bit more I think ...
The only thing that comes unstuck is eviction, and even there that we
keep them all in order of last GTT access, should prevent most random
stalls. As the batch object itself is still being tracked for gpu
busyness, it is only a level of finese required rather than being
outright broken wrt to serialisation.
It is still a massive leap forward in our understanding of the
potential solution, and not one step back.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Fix secure dispatch with full ppgtt
2014-08-11 11:35 ` Chris Wilson
@ 2014-08-11 11:51 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-08-11 11:51 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Daniel Vetter,
Intel Graphics Development, Ben Widawsky
On Mon, Aug 11, 2014 at 12:35:42PM +0100, Chris Wilson wrote:
> On Mon, Aug 11, 2014 at 01:22:19PM +0200, Daniel Vetter wrote:
> > On Mon, Aug 11, 2014 at 11:17:40AM +0100, Chris Wilson wrote:
> > > On Mon, Aug 11, 2014 at 12:08:58PM +0200, Daniel Vetter wrote:
> > > > Based upon a hunk from a patch from Chris Wilson, but augmented to:
> > > > - Process the batch in the full ppgtt vm so that self-relocations
> > > > match again with userspace's expectations..
> > > > - Add a comment why plain pin for the global gtt binding is safe at
> > > > that point.
> > > >
> > > > v2: Drop local bind_vm variable (Chris).
> > > >
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >
> > > /me jaw drops
> > >
> > > Oh nice, yes you are right that does fix the issue with the relocations
> > > being in one address space whilst the exec needs to be done from gGTT.
> > >
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Actually it's not that simple - with this patch we don't add the ggtt vma
> > for the batch to the active list, which we probably should. So needs to be
> > revised a bit more I think ...
>
> The only thing that comes unstuck is eviction, and even there that we
> keep them all in order of last GTT access, should prevent most random
> stalls. As the batch object itself is still being tracked for gpu
> busyness, it is only a level of finese required rather than being
> outright broken wrt to serialisation.
>
> It is still a massive leap forward in our understanding of the
> potential solution, and not one step back.
Yeah, you're right. Added a FIXME comment as discussed on irc and pulled
it in. I guess in the end we should still smash the ggtt batch vma onto
the eb list somehow, to make sure that it goes through all the proper
move_to_active processing _before_ we call add_request.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-11 11:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-11 10:05 [PATCH] drm/i915: Fix secure dispatch with full ppgtt Daniel Vetter
2014-08-11 10:08 ` Daniel Vetter
2014-08-11 10:17 ` Chris Wilson
2014-08-11 11:22 ` Daniel Vetter
2014-08-11 11:35 ` Chris Wilson
2014-08-11 11:51 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox