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@xxxxxxxx>
> > Signed-off-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
