Quoting Tvrtko Ursulin (2018-08-30 17:23:43)
>
> On 30/08/2018 11:24, Chris Wilson wrote:
> > +static int steal_hw_id(struct drm_i915_private *i915)
> > +{
> > + struct i915_gem_context *ctx, *cn;
> > + LIST_HEAD(pinned);
> > + int id = -ENOSPC;
> > +
> > + lockdep_assert_held(&i915->contexts.mutex);
> > +
> > + list_for_each_entry_safe(ctx, cn,
> > + &i915->contexts.hw_id_list, hw_id_link) {
> > + if (atomic_read(&ctx->pin_hw_id)) {
> > + list_move_tail(&ctx->hw_id_link, &pinned);
> > + continue;
> > + }
> > +
> > + GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> > + list_del_init(&ctx->hw_id_link);
> > + id = ctx->hw_id;
> > + break;
> > + }
> > +
> > + list_splice_tail(&pinned, &i915->contexts.hw_id_list);
>
> Put a comment what is this code doing please. Trying to create some sort
> of LRU order?
LRSearched. Same as the shrinker, and eviction code if you would also
review that ;)
>
> > + return id;
> > +}
> > +
> > +static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> > +{
> > + int ret;
> > +
> > + lockdep_assert_held(&i915->contexts.mutex);
> > +
> > + ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> > + if (unlikely(ret < 0)) {
> > + ret = steal_hw_id(i915);
> > + if (ret < 0) /* once again for the correct erro code */
>
> errno
>
> > + ret = new_hw_id(i915, GFP_KERNEL);
>
> Hmm.. shouldn't you try GFP_KERNEL before attempting to steal? Actually
> I think you should branch based on -ENOSPC (steal) vs -ENOMEM (retry
> with GFP_KERNEL). Which would actually mean something like:
I was applying the same strategy as we use elsewhere. Penalise any
driver cache before hitting reclaim.
I think that is fair from an application of soft backpressure point of
view. (Lack of backpressure is probably a sore point for many.)
> > - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > - 0, max, GFP_KERNEL);
>
> Although now that I see this I am struggling not to say the change to
> try a lighter weight allocation strategy first (gfp may fail) needs to
> be split out to a separate patch.
Pardon? I appear to suddenly be hard of hearing.
The patch was all about the steal_hw_id().
> > - if (ret < 0) {
> > - /* Contexts are only released when no longer active.
> > - * Flush any pending retires to hopefully release some
> > - * stale contexts and try again.
> > - */
> > - i915_retire_requests(dev_priv);
> > - ret = ida_simple_get(&dev_priv->contexts.hw_ida,
> > - 0, max, GFP_KERNEL);
> > - if (ret < 0)
> > - return ret;
> > - }
> > -
> > - *out = ret;
> > - return 0;
> > -}
> > -
> > static u32 default_desc_template(const struct drm_i915_private *i915,
> > const struct i915_hw_ppgtt *ppgtt)
> > {
> > @@ -276,12 +324,6 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> > if (ctx == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > - ret = assign_hw_id(dev_priv, &ctx->hw_id);
> > - if (ret) {
> > - kfree(ctx);
> > - return ERR_PTR(ret);
> > - }
> > -
> > kref_init(&ctx->ref);
> > list_add_tail(&ctx->link, &dev_priv->contexts.list);
> > ctx->i915 = dev_priv;
> > @@ -295,6 +337,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> >
> > INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);
> > INIT_LIST_HEAD(&ctx->handles_list);
> > + INIT_LIST_HEAD(&ctx->hw_id_link);
> >
> > /* Default context will never have a file_priv */
> > ret = DEFAULT_CONTEXT_HANDLE;
> > @@ -421,15 +464,35 @@ i915_gem_context_create_gvt(struct drm_device *dev)
> > return ctx;
> > }
> >
> > +static void
> > +destroy_kernel_context(struct i915_gem_context **ctxp)
> > +{
> > + struct i915_gem_context *ctx;
> > +
> > + /* Keep the context ref so that we can free it immediately ourselves */
> > + ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > + GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > +
> > + context_close(ctx);
> > + i915_gem_context_free(ctx);
> > +}
> > +
> > struct i915_gem_context *
> > i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> > {
> > struct i915_gem_context *ctx;
> > + int err;
> >
> > ctx = i915_gem_create_context(i915, NULL);
> > if (IS_ERR(ctx))
> > return ctx;
> >
> > + err = i915_gem_context_pin_hw_id(ctx);
> > + if (err) {
> > + destroy_kernel_context(&ctx);
> > + return ERR_PTR(err);
> > + }
> > +
> > i915_gem_context_clear_bannable(ctx);
> > ctx->sched.priority = prio;
> > ctx->ring_size = PAGE_SIZE;
> > @@ -439,17 +502,19 @@ i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
> > return ctx;
> > }
> >
> > -static void
> > -destroy_kernel_context(struct i915_gem_context **ctxp)
> > +static void init_contexts(struct drm_i915_private *i915)
> > {
> > - struct i915_gem_context *ctx;
> > + mutex_init(&i915->contexts.mutex);
> > + INIT_LIST_HEAD(&i915->contexts.list);
> >
> > - /* Keep the context ref so that we can free it immediately ourselves */
> > - ctx = i915_gem_context_get(fetch_and_zero(ctxp));
> > - GEM_BUG_ON(!i915_gem_context_is_kernel(ctx));
> > + /* Using the simple ida interface, the max is limited by sizeof(int) */
> > + BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > + BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > + ida_init(&i915->contexts.hw_ida);
> > + INIT_LIST_HEAD(&i915->contexts.hw_id_list);
> >
> > - context_close(ctx);
> > - i915_gem_context_free(ctx);
> > + INIT_WORK(&i915->contexts.free_work, contexts_free_worker);
> > + init_llist_head(&i915->contexts.free_list);
>
> ugh diff.. :) looks like pure movement from perspective of
> destroy_kernel_context.
>
> > }
> >
> > static bool needs_preempt_context(struct drm_i915_private *i915)
> > @@ -470,14 +535,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> > if (ret)
> > return ret;
> >
> > - INIT_LIST_HEAD(&dev_priv->contexts.list);
> > - INIT_WORK(&dev_priv->contexts.free_work, contexts_free_worker);
> > - init_llist_head(&dev_priv->contexts.free_list);
> > -
> > - /* Using the simple ida interface, the max is limited by sizeof(int) */
> > - BUILD_BUG_ON(MAX_CONTEXT_HW_ID > INT_MAX);
> > - BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > INT_MAX);
> > - ida_init(&dev_priv->contexts.hw_ida);
> > + init_contexts(dev_priv);
> >
> > /* lowest priority; idle task */
> > ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
> > @@ -490,6 +548,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
> > * all user contexts will have non-zero hw_id.
> > */
> > GEM_BUG_ON(ctx->hw_id);
> > + GEM_BUG_ON(!atomic_read(&ctx->pin_hw_id));
>
> /* Kernel context is perma-pinned */
>
> > dev_priv->kernel_context = ctx;
> >
> > /* highest priority; preempting task */
> > @@ -527,6 +586,7 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
> > destroy_kernel_context(&i915->kernel_context);
> >
> > /* Must free all deferred contexts (via flush_workqueue) first */
> > + GEM_BUG_ON(!list_empty(&i915->contexts.hw_id_list));
> > ida_destroy(&i915->contexts.hw_ida);
> > }
> >
> > @@ -932,6 +992,33 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> > return ret;
> > }
> >
> > +int __i915_gem_context_pin_hw_id(struct i915_gem_context *ctx)
> > +{
> > + struct drm_i915_private *i915 = ctx->i915;
> > + int err = 0;
> > +
> > + mutex_lock(&i915->contexts.mutex);
> > +
> > + GEM_BUG_ON(i915_gem_context_is_closed(ctx));
> > +
> > + if (list_empty(&ctx->hw_id_link)) {
> > + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id));
> > +
> > + err = assign_hw_id(i915, &ctx->hw_id);
> > + if (err)
> > + goto out_unlock;
> > +
> > + list_add_tail(&ctx->hw_id_link, &i915->contexts.hw_id_list);
> > + }
> > +
> > + GEM_BUG_ON(atomic_read(&ctx->pin_hw_id) == ~0u);
> > + atomic_inc(&ctx->pin_hw_id);
> > +
> > +out_unlock:
> > + mutex_unlock(&i915->contexts.mutex);
> > + return err;
> > +}
> > +
> > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> > #include "selftests/mock_context.c"
> > #include "selftests/i915_gem_context.c"
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
> > index 851dad6decd7..c73ac614f58c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.h
> > @@ -136,6 +136,8 @@ struct i915_gem_context {
> > * id for the lifetime of the context.
> > */
> > unsigned int hw_id;
> > + atomic_t pin_hw_id;
>
> I think now we need short comments describing the difference between the
> two.
One is 32bits unsigned, unserialised. The other is 32bits signed, and
very loosely serialised :)
> > + struct list_head hw_id_link;
>
> And for this one.
[snip]
> So in essence there will be a little bit more cost when pinning in the
> normal case, or a bit bit more in the stealing/pathological case, but as
> long as we stay below over-subscription the cost is only on first pin.
> No complaints there. Debug also won't be confusing in the normal case
> since numbers will be stable.
Yup. Nice addition to the changelog, thanks.
> Does it have any negative connotations in the world of OA is the
> question for Lionel?
Lionel kept promising me this was ok, that he/gputop was quite ready for
shorter lived ctx id, and reuse.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
