Re: [PATCH] drm/i915: Unshare the idle-barrier from other kernel requests

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-07-25 14:19:22)
> 
> On 25/07/2019 10:38, Chris Wilson wrote:
> > @@ -352,22 +384,29 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >   
> >       GEM_BUG_ON(!engine->mask);
> >       for_each_engine_masked(engine, i915, engine->mask, tmp) {
> > -             struct intel_context *kctx = engine->kernel_context;
> >               struct active_node *node;
> >   
> > -             node = kmem_cache_alloc(global.slab_cache, GFP_KERNEL);
> > -             if (unlikely(!node)) {
> > -                     err = -ENOMEM;
> > -                     goto unwind;
> > +             node = idle_barrier(ref);
> > +             if (!node) {
> > +                     node = kmem_cache_alloc(global.slab_cache,
> > +                                             GFP_KERNEL |
> > +                                             __GFP_RETRY_MAYFAIL |
> > +                                             __GFP_NOWARN);
> > +                     if (unlikely(!node)) {
> > +                             err = -ENOMEM;
> > +                             goto unwind;
> > +                     }
> > +
> > +                     node->ref = ref;
> > +                     node->timeline = 0;
> > +                     node->base.retire = node_retire;
> >               }
> >   
> > -             i915_active_request_init(&node->base,
> > -                                      (void *)engine, node_retire);
> > -             node->timeline = kctx->ring->timeline->fence_context;
> > -             node->ref = ref;
> > +             intel_engine_pm_get(engine);
> 
> AFAIU this could comfortably be moved last instead of going even higher 
> with this patch.

Sure, the wakeref is pinned by the caller this acquires one for the
barrier. I moved it out of the way as I felt grouping the linkage of the
preallocated barrier was more interesting.
 
> > +
> > +             RCU_INIT_POINTER(node->base.request, (void *)engine);
> 
> What happens with the engine in request slot? Nothing if there is no 
> retire hook set? But who uses it then?

We're just stashing a pointer back to the engine in an unused location.

> 
> >               atomic_inc(&ref->count);
> >   
> > -             intel_engine_pm_get(engine);
> >               llist_add((struct llist_node *)&node->base.link,
> >                         &ref->barriers);
> >       }
> > @@ -402,6 +441,7 @@ void i915_active_acquire_barrier(struct i915_active *ref)
> >   
> >               node = container_of((struct list_head *)pos,
> >                                   typeof(*node), base.link);
> > +             GEM_BUG_ON(node->timeline);
> >   
> >               engine = (void *)rcu_access_pointer(node->base.request);
> >               RCU_INIT_POINTER(node->base.request, ERR_PTR(-EAGAIN));
> > @@ -410,12 +450,7 @@ void i915_active_acquire_barrier(struct i915_active *ref)
> >               p = &ref->tree.rb_node;
> >               while (*p) {
> >                       parent = *p;
> > -                     if (rb_entry(parent,
> > -                                  struct active_node,
> > -                                  node)->timeline < node->timeline)
> > -                             p = &parent->rb_right;
> > -                     else
> > -                             p = &parent->rb_left;
> > +                     p = &parent->rb_left;
> 
> I don't get this hunk at all - how can it be okay to ignore the timeline 
> value. Is this the buggy bit you mentioned on IRC?

We don't ignore, we know it's 0.

No, the bug is whether or not the ppGTT counts as part of the pinned
state for the legacy ringbuffer. Currently we do not and my selftest
required that we always insert an idle-barrier after use. Erring on the
side of safety says we should always utilise the idle-barriers.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx