Quoting Tvrtko Ursulin (2019-02-12 11:18:16)
>
> On 06/02/2019 13:03, Chris Wilson wrote:
> > In preparation to making the ppGTT binding for a context explicit (to
> > facilitate reusing the same ppGTT between different contexts), allow the
> > user to create and destroy named ppGTT.
> >
> > v2: Replace global barrier for swapping over the ppgtt and tlbs with a
> > local context barrier (Tvrtko)
> > v3: serialise with struct_mutex; it's lazy but required dammit
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.c | 2 +
> > drivers/gpu/drm/i915/i915_drv.h | 3 +
> > drivers/gpu/drm/i915/i915_gem_context.c | 254 +++++++++++++++++-
> > drivers/gpu/drm/i915/i915_gem_context.h | 5 +
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.h | 16 +-
> > drivers/gpu/drm/i915/selftests/huge_pages.c | 1 -
> > .../gpu/drm/i915/selftests/i915_gem_context.c | 239 ++++++++++++----
> > drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 1 -
> > drivers/gpu/drm/i915/selftests/mock_context.c | 8 +-
> > include/uapi/drm/i915_drm.h | 35 +++
> > 11 files changed, 510 insertions(+), 71 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 36da8ab1e7ce..487e78094e93 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -3005,6 +3005,8 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
> > DRM_IOCTL_DEF_DRV(I915_PERF_ADD_CONFIG, i915_perf_add_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(I915_PERF_REMOVE_CONFIG, i915_perf_remove_config_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > DRM_IOCTL_DEF_DRV(I915_QUERY, i915_query_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(I915_GEM_VM_CREATE, i915_gem_vm_create_ioctl, DRM_RENDER_ALLOW),
> > + DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl, DRM_RENDER_ALLOW),
> > };
> >
> > static struct drm_driver driver = {
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index e554691304dc..523de3644570 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -217,6 +217,9 @@ struct drm_i915_file_private {
> > } mm;
> > struct idr context_idr;
> >
> > + struct mutex vm_lock;
> > + struct idr vm_idr;
> > +
> > unsigned int bsd_engine;
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index c3f41f501276..dd49b1ef3ff2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -110,6 +110,8 @@ static void lut_close(struct i915_gem_context *ctx)
> > struct i915_vma *vma = rcu_dereference_raw(*slot);
> >
> > radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> > +
> > + vma->open_count--;
>
> I am finding vma->open_count handling seriously confusing. So maybe that
> means there should be a comment here as minimum.
>
> What is the path in the current code which brings vma->open_count back
> to zero? If it is not done from lut_close, but object is removed from
> the lut_list, then the only current decrement in i915_gem_close_object
> won't run. Surely I am missing something..
In ppgtt, it is single instance, so we just know at this point it hits
zero. The open_count was originally to handle the reuse via the shared
GGTT.
> > static struct i915_gem_context *
> > i915_gem_create_context(struct drm_i915_private *dev_priv,
> > struct drm_i915_file_private *file_priv)
> > @@ -451,8 +479,8 @@ i915_gem_create_context(struct drm_i915_private *dev_priv,
> > return ERR_CAST(ppgtt);
> > }
> >
> > - ctx->ppgtt = ppgtt;
> > - ctx->desc_template = default_desc_template(dev_priv, ppgtt);
> > + __assign_ppgtt(ctx, ppgtt);
> > + i915_ppgtt_put(ppgtt);
>
> Looks strange and one realizes it is dropping the ref __assign_ppgtt
> takes. Not sure if it wouldn't be better to just open code what this
> site needs.
Don't tempt me; you know you'll regret the amount of code I'll copy
around just because it's easier :)
I think having assign add a ref and not borrow the callers is simpler in
the long run.
> > @@ -662,6 +700,89 @@ void i915_gem_context_close(struct drm_file *file)
> >
> > idr_for_each(&file_priv->context_idr, context_idr_cleanup, NULL);
> > idr_destroy(&file_priv->context_idr);
> > +
> > + idr_for_each(&file_priv->vm_idr, vm_idr_cleanup, NULL);
> > + idr_destroy(&file_priv->vm_idr);
>
> The name of this function always confuses me. Should we rename it to
> i915_gem_close_contexts or something?
Can do. Though not in this patch, so go right ahead ;)
> > +static int get_ppgtt(struct i915_gem_context *ctx,
> > + struct drm_i915_gem_context_param *args)
> > +{
> > + struct drm_i915_file_private *file_priv = ctx->file_priv;
> > + struct i915_hw_ppgtt *ppgtt;
> > + int ret;
> > +
> > + if (!ctx->ppgtt)
> > + return -ENODEV;
> > +
> > + /* XXX rcu acquire? */
> > + ret = mutex_lock_interruptible(&ctx->i915->drm.struct_mutex);
>
> Only to serialize threads working on the same ctx? Why not do that under
> the new vm->lock instead?
> > + err = mutex_lock_interruptible(&file_priv->vm_lock);
> > + if (err)
> > + return err;
> > +
> > + ppgtt = idr_find(&file_priv->vm_idr, args->value);
> > + if (ppgtt) {
> > + GEM_BUG_ON(ppgtt->user_handle != args->value);
> > + i915_ppgtt_get(ppgtt);
> > + }
> > + mutex_unlock(&file_priv->vm_lock);
> > + if (!ppgtt)
> > + return -ENOENT;
> > +
> > + err = mutex_lock_interruptible(&ctx->i915->drm.struct_mutex);
> > + if (err)
> > + goto out;
> > +
> > + if (ppgtt == ctx->ppgtt)
> > + goto unlock;
> > +
> > + /* Teardown the existing obj:vma cache, it will have to be rebuilt. */
> > + lut_close(ctx);
>
> Nesting issues I guess, the answer to my previous question.
I'll take another look, I think my thinking was that file_priv->vm
didn't have wide enough scope. But ppgtt are definitely restricted to
being inside a single fd.
I've also a new lock for you to play with here, ctx->pin_mutex (solves
ce->sseu locking requirements as well as pinning in general).
> > + old = __set_ppgtt(ctx, ppgtt);
> > +
> > + /*
> > + * We need to flush any requests using the current ppgtt before
> > + * we release it as the requests do not hold a reference themselves,
> > + * only indirectly through the context.
> > + */
> > + err = context_barrier_task(ctx, -1, set_ppgtt_barrier, old);
>
> But barrier can be retired on user interrupt with context save still
> running, no?
User interrupt while building the barrier requests? That results in the
barrier being cancelled with err = EINTR. The whole point is that the
context barrier is retired on the completion (maybe you meant
MI_USER_INTERRUPT) after the context has been run with the new ppgtt.
The context barrier itself is using the new ctx->ppgtt.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
