Message ID | 1410424786-22992-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Sep 11, 2014 at 11:39:46AM +0300, Mika Kuoppala wrote: > When PPGGT is in use, we need to bind secure batches also to ggtt. > We used to pin, exec and unpin. This trick worked as there was nothing > competing in ggtt space and the ppggt vma was used to tracking. > But this left the ggtt vma untracked and potentially it could vanish > during the batch execution. > > Track ggtt vma properly by piggypacking it into the eb->vmas list > just before batch submission is made. This ensures that vma gets > into the active list properly. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 1a0611b..06c71e9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -217,6 +217,10 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) > > entry = vma->exec_entry; > > + /* Check if piggypacked ggtt batch entry */ > + if (!entry) > + return; > + > if (entry->flags & __EXEC_OBJECT_HAS_FENCE) > i915_gem_object_unpin_fence(obj); > > @@ -224,6 +228,8 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) > vma->pin_count--; > > entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN); > + > + vma->exec_entry = NULL; > } > > static void eb_destroy(struct eb_vmas *eb) > @@ -970,7 +976,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, > /* update for the implicit flush after a batch */ > obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS; > } > - if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) { > + if (entry && entry->flags & EXEC_OBJECT_NEEDS_FENCE) { > obj->last_fenced_seqno = seqno; > if (entry->flags & __EXEC_OBJECT_HAS_FENCE) { > struct drm_i915_private *dev_priv = to_i915(ring->dev); > @@ -1385,6 +1391,7 @@ 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 bdw mucks it up again. */ > if (flags & I915_DISPATCH_SECURE) { > + struct i915_vma *vma; > /* > * So on first glance it looks freaky that we pin the batch here > * outside of the reservation loop. But: > @@ -1399,6 +1406,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > if (ret) > goto err; > > + vma = i915_gem_obj_to_ggtt(batch_obj); > + > + /* If there is no exec_entry in this stage, ggtt vma > + * is not in the eb->vmas list. Piggypack our ggtt > + * address to the list in order for it to be added as > + * an active vma in do_execbuf() > + */ > + if (vma->exec_entry == NULL) { > + drm_gem_object_reference(&batch_obj->base); > + list_add_tail(&vma->exec_list, &eb->vmas); vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry); Then you can kill the additional if (entry) checks. -Chris
On Thu, Sep 11, 2014 at 01:49:07AM -0700, Chris Wilson wrote: > On Thu, Sep 11, 2014 at 11:39:46AM +0300, Mika Kuoppala wrote: > > When PPGGT is in use, we need to bind secure batches also to ggtt. > > We used to pin, exec and unpin. This trick worked as there was nothing > > competing in ggtt space and the ppggt vma was used to tracking. > > But this left the ggtt vma untracked and potentially it could vanish > > during the batch execution. > > > > Track ggtt vma properly by piggypacking it into the eb->vmas list > > just before batch submission is made. This ensures that vma gets > > into the active list properly. > > > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 ++++++++++++++++++++--------- > > 1 file changed, 20 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 1a0611b..06c71e9 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -217,6 +217,10 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) > > > > entry = vma->exec_entry; > > > > + /* Check if piggypacked ggtt batch entry */ > > + if (!entry) > > + return; > > + > > if (entry->flags & __EXEC_OBJECT_HAS_FENCE) > > i915_gem_object_unpin_fence(obj); > > > > @@ -224,6 +228,8 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) > > vma->pin_count--; > > > > entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN); > > + > > + vma->exec_entry = NULL; > > } > > > > static void eb_destroy(struct eb_vmas *eb) > > @@ -970,7 +976,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, > > /* update for the implicit flush after a batch */ > > obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS; > > } > > - if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) { > > + if (entry && entry->flags & EXEC_OBJECT_NEEDS_FENCE) { > > obj->last_fenced_seqno = seqno; > > if (entry->flags & __EXEC_OBJECT_HAS_FENCE) { > > struct drm_i915_private *dev_priv = to_i915(ring->dev); > > @@ -1385,6 +1391,7 @@ 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 bdw mucks it up again. */ > > if (flags & I915_DISPATCH_SECURE) { > > + struct i915_vma *vma; > > /* > > * So on first glance it looks freaky that we pin the batch here > > * outside of the reservation loop. But: > > @@ -1399,6 +1406,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, > > if (ret) > > goto err; > > > > + vma = i915_gem_obj_to_ggtt(batch_obj); > > + > > + /* If there is no exec_entry in this stage, ggtt vma > > + * is not in the eb->vmas list. Piggypack our ggtt > > + * address to the list in order for it to be added as > > + * an active vma in do_execbuf() > > + */ > > + if (vma->exec_entry == NULL) { > > + drm_gem_object_reference(&batch_obj->base); > > + list_add_tail(&vma->exec_list, &eb->vmas); > > vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry); > > Then you can kill the additional if (entry) checks. Also, don't we need to set vma->exec_entry->flags = __EXEC_OBJECT_HAS_PIN ? Looking back at the last set of batch buffer copy patches I sent, I thought at that time that it was necessary when doing something similar for the shadow batch. Not sure yet how all the code has changed since then. http://lists.freedesktop.org/archives/intel-gfx/2014-July/048707.html Brad
On Thu, Sep 11, 2014 at 09:30:27AM -0700, Volkin, Bradley D wrote: > On Thu, Sep 11, 2014 at 01:49:07AM -0700, Chris Wilson wrote: > > vma->exec_entry = memset(&dummy_exec_entry, 0, sizeof(dummy_exec_entry); > > > > Then you can kill the additional if (entry) checks. > > Also, don't we need to set vma->exec_entry->flags = __EXEC_OBJECT_HAS_PIN ? Yes we do. I realised that and mentioned it to Mika on IRC right after sending :| All the more reason for using the dummy_exec_entry/shadow_exec_entry. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 1a0611b..06c71e9 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -217,6 +217,10 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) entry = vma->exec_entry; + /* Check if piggypacked ggtt batch entry */ + if (!entry) + return; + if (entry->flags & __EXEC_OBJECT_HAS_FENCE) i915_gem_object_unpin_fence(obj); @@ -224,6 +228,8 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma) vma->pin_count--; entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN); + + vma->exec_entry = NULL; } static void eb_destroy(struct eb_vmas *eb) @@ -970,7 +976,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas, /* update for the implicit flush after a batch */ obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS; } - if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) { + if (entry && entry->flags & EXEC_OBJECT_NEEDS_FENCE) { obj->last_fenced_seqno = seqno; if (entry->flags & __EXEC_OBJECT_HAS_FENCE) { struct drm_i915_private *dev_priv = to_i915(ring->dev); @@ -1385,6 +1391,7 @@ 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 bdw mucks it up again. */ if (flags & I915_DISPATCH_SECURE) { + struct i915_vma *vma; /* * So on first glance it looks freaky that we pin the batch here * outside of the reservation loop. But: @@ -1399,6 +1406,18 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, if (ret) goto err; + vma = i915_gem_obj_to_ggtt(batch_obj); + + /* If there is no exec_entry in this stage, ggtt vma + * is not in the eb->vmas list. Piggypack our ggtt + * address to the list in order for it to be added as + * an active vma in do_execbuf() + */ + if (vma->exec_entry == NULL) { + drm_gem_object_reference(&batch_obj->base); + list_add_tail(&vma->exec_list, &eb->vmas); + } + exec_start += i915_gem_obj_ggtt_offset(batch_obj); } else exec_start += i915_gem_obj_offset(batch_obj, vm); @@ -1406,14 +1425,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = dev_priv->gt.do_execbuf(dev, file, ring, ctx, args, &eb->vmas, batch_obj, exec_start, flags); - /* - * FIXME: We crucially rely upon the active tracking for the (ppgtt) - * batch vma for correctness. For less ugly and less fragility this - * needs to be adjusted to also track the ggtt batch vma properly as - * active. - */ - if (flags & I915_DISPATCH_SECURE) - i915_gem_object_ggtt_unpin(batch_obj); err: /* the request owns the ref now */ i915_gem_context_unreference(ctx);
When PPGGT is in use, we need to bind secure batches also to ggtt. We used to pin, exec and unpin. This trick worked as there was nothing competing in ggtt space and the ppggt vma was used to tracking. But this left the ggtt vma untracked and potentially it could vanish during the batch execution. Track ggtt vma properly by piggypacking it into the eb->vmas list just before batch submission is made. This ensures that vma gets into the active list properly. Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)