On Mon, May 04, 2015 at 10:49:30AM +0200, Daniel Vetter wrote:
> On Fri, Apr 24, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > On Fri, Apr 24, 2015 at 12:14:17PM +0100, Chris Wilson wrote:
> > > On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> > > > Currently we have the problem that the decision whether ptes need to
> > > > be (re)written is splattered all over the codebase. Move all that into
> > > > i915_vma_bind. This needs a few changes:
> > > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > > > to vma->bound in there to avoid duplicating the conversion code all
> > > > over.
> > > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > > > around) explicit, add PIN_USER for that.
> > > > - Two callers want to update ptes, give them a PIN_UPDATE for that.
> > > >
> > > > Of course we still want to avoid double-binding, but that should be
> > > > taken care of:
> > > > - A ppgtt vma will only ever see PIN_USER, so no issue with
> > > > double-binding.
> > > > - A ggtt vma with aliasing ppgtt needs both types of binding, and we
> > > > track that properly now.
> > > > - A ggtt vma without aliasing ppgtt could be bound twice. In the
> > > > lower-level ->bind_vma functions hence unconditionally set
> > > > GLOBAL_BIND when writing the ggtt ptes.
> > > >
> > > > There's still a bit room for cleanup, but that's for follow-up
> > > > patches.
> > > >
> > > > v2: Fixup fumbles.
> > > >
> > > > v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> > > >
> > > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > >
> > > I'm getting lots of GPU hangs from this patch...
>
> Oops, I've considered the vma_unbind case but then totally forgot that a
> vma can survive over execbuf. Can you please wrap this into a proper
> commit message with sob?
Mika has and already been applied :)
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 91aade7..e3841f8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3066,8 +3066,8 @@ int i915_vma_unbind(struct i915_vma *vma)
> > }
> >
> > trace_i915_vma_unbind(vma);
> > -
> > vma->vm->unbind_vma(vma);
> > + vma->bound = 0;
> >
> > list_del_init(&vma->mm_list);
> > if (i915_is_ggtt(vma->vm)) {
> > @@ -3588,7 +3588,6 @@ search_free:
> > if (ret)
> > goto err_remove_node;
> >
> > - trace_i915_vma_bind(vma, flags);
> > ret = i915_vma_bind(vma, obj->cache_level, flags);
> > if (ret)
> > goto err_finish_gtt;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index cfdc8c6..45d74da 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1563,9 +1563,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > * be in ggtt still end up in the aliasing ppgtt. remove
> > * this check when that is fixed.
> > */
> > - if (USES_FULL_PPGTT(dev))
> > - dispatch_flags |= I915_DISPATCH_SECURE;
> > -
> > + dispatch_flags |= I915_DISPATCH_SECURE;
>
> Unrelated hunk?
No, two bugs, this being quite fun and an intentional artifact of your
patches :)
> > exec_start = 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 9e06180..bbeb6c3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
> >
> > BUG_ON(!i915_is_ggtt(vma->vm));
> > intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
> > -
> > - vma->bound |= GLOBAL_BIND;
>
> Hm, why remove this? I added this to avoid double-binding on platforms
> where one implies the other binding type. Maybe should have been a bit
> more consistent with bound |= GLOBAL_BIND | LOCAL_BIND.
Hmm, didn't we end with doing vma->bound = bind_flags? But I think if
you did if (!HAS_PPGTT) bind_flags |= GLOBAL_BIND in i915_vma_bind, then
we have all the vma->bound processing in the one place.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
