Re: [PATCH 14/26] drm/i915: use the new iterator in i915_sw_fence_await_reservation v3

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 20.09.21 um 10:47 schrieb Tvrtko Ursulin:
> On 20/09/2021 09:45, Tvrtko Ursulin wrote:
> > On 17/09/2021 13:35, Christian König wrote:
> > > Simplifying the code a bit.
> > >
> > > v2: use dma_resv_for_each_fence instead, according to Tvrtko the 
> > > lock is
> > >      held here anyway.
> > > v3: back to using dma_resv_for_each_fence_unlocked.
> >
> > It did not work out - what happened?
> Wait, my suggestion to try the locked iterator was against 
> i915_request_await_object. I haven't looked at this one at the time or 
> even now.

Exactly! I've mixed the two up and this one here is really called 
without holding a lock.

Regards,
Christian.

> Regards,
>
> Tvrtko
>
>
> > Regards,
> >
> > Tvrtko
> >
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/i915_sw_fence.c | 57 
> > > ++++++++--------------------
> > >   1 file changed, 15 insertions(+), 42 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c 
> > > b/drivers/gpu/drm/i915/i915_sw_fence.c
> > > index c589a681da77..7635b0478ea5 100644
> > > --- a/drivers/gpu/drm/i915/i915_sw_fence.c
> > > +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> > > @@ -572,56 +572,29 @@ int i915_sw_fence_await_reservation(struct 
> > > i915_sw_fence *fence,
> > >                       unsigned long timeout,
> > >                       gfp_t gfp)
> > >   {
> > > -    struct dma_fence *excl;
> > > +    struct dma_resv_iter cursor;
> > > +    struct dma_fence *f;
> > >       int ret = 0, pending;
> > >       debug_fence_assert(fence);
> > >       might_sleep_if(gfpflags_allow_blocking(gfp));
> > > -    if (write) {
> > > -        struct dma_fence **shared;
> > > -        unsigned int count, i;
> > > -
> > > -        ret = dma_resv_get_fences(resv, &excl, &count, &shared);
> > > -        if (ret)
> > > -            return ret;
> > > -
> > > -        for (i = 0; i < count; i++) {
> > > -            if (shared[i]->ops == exclude)
> > > -                continue;
> > > -
> > > -            pending = i915_sw_fence_await_dma_fence(fence,
> > > -                                shared[i],
> > > -                                timeout,
> > > -                                gfp);
> > > -            if (pending < 0) {
> > > -                ret = pending;
> > > -                break;
> > > -            }
> > > -
> > > -            ret |= pending;
> > > -        }
> > > -
> > > -        for (i = 0; i < count; i++)
> > > -            dma_fence_put(shared[i]);
> > > -        kfree(shared);
> > > -    } else {
> > > -        excl = dma_resv_get_excl_unlocked(resv);
> > > -    }
> > > -
> > > -    if (ret >= 0 && excl && excl->ops != exclude) {
> > > -        pending = i915_sw_fence_await_dma_fence(fence,
> > > -                            excl,
> > > -                            timeout,
> > > +    rcu_read_lock();
> > > +    dma_resv_iter_begin(&cursor, resv, write);
> > > +    dma_resv_for_each_fence_unlocked(&cursor, f) {
> > > +        rcu_read_unlock();
> > > +        pending = i915_sw_fence_await_dma_fence(fence, f, timeout,
> > >                               gfp);
> > > -        if (pending < 0)
> > > +        rcu_read_lock();
> > > +        if (pending < 0) {
> > >               ret = pending;
> > > -        else
> > > -            ret |= pending;
> > > -    }
> > > -
> > > -    dma_fence_put(excl);
> > > +            break;
> > > +        }
> > > +        ret |= pending;
> > > +    }
> > > +    dma_resv_iter_end(&cursor);
> > > +    rcu_read_unlock();
> > >       return ret;
> > >   }