[Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Souza, Jose]

On Wed, 2020-01-29 at 23:33 +0000, Chris Wilson wrote:
> Quoting José Roberto de Souza (2020-01-29 23:23:45)
> > Only the first and the last nodes were being added to
> > ref->preallocated_barriers.
> > 
> > I'm not familiar with this part of the code but if that is the
> > expected behavior it is leaking the nodes in between.
> > 
> > Renaming variables to make it more easy to read.
> > 
> > Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between
> > context pin/unpin")
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_active.c
> > b/drivers/gpu/drm/i915/i915_active.c
> > index 9d6830885d2e..3d2e7cf55e52 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -607,7 +607,7 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >                                             struct intel_engine_cs
> > *engine)
> >  {
> >         intel_engine_mask_t tmp, mask = engine->mask;
> > -       struct llist_node *pos = NULL, *next;
> > +       struct llist_node *first = NULL, *last = NULL;
> 
> last cannot be NULL at the end.

last will be set in the first iteration and it will always interate at
least once because the mask will at least match with the engine in
i915_active_acquire_preallocate_barrier() parameter.

> 
> >         struct intel_gt *gt = engine->gt;
> >         int err;
> >  
> > @@ -626,6 +626,7 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >         GEM_BUG_ON(!mask);
> >         for_each_engine_masked(engine, gt, mask, tmp) {
> >                 u64 idx = engine->kernel_context->timeline-
> > >fence_context;
> > +               struct llist_node *prev = first;
> >                 struct active_node *node;
> >  
> >                 node = reuse_idle_barrier(ref, idx);
> > @@ -659,23 +660,23 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >                 GEM_BUG_ON(rcu_access_pointer(node->base.fence) !=
> > ERR_PTR(-EAGAIN));
> >  
> >                 GEM_BUG_ON(barrier_to_engine(node) != engine);
> > -               next = barrier_to_ll(node);
> > -               next->next = pos;
> > -               if (!pos)
> > -                       pos = next;
> 
> Yeah, that was a bit of nonsense.
> 
> Minus the spurious NULL,
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris