Re: [Intel-gfx] [PATCH 15/26] drm/i915: use the new iterator in i915_request_await_object

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 14/09/2021 11:39, Christian König wrote:
> Am 14.09.21 um 12:26 schrieb Tvrtko Ursulin:
> > On 13/09/2021 14:16, Christian König wrote:
> > > Simplifying the code a bit.
> > >
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/i915/i915_request.c | 36 ++++++-----------------------
> > >   1 file changed, 7 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_request.c 
> > > b/drivers/gpu/drm/i915/i915_request.c
> > > index 37aef1308573..b81045ceb619 100644
> > > --- a/drivers/gpu/drm/i915/i915_request.c
> > > +++ b/drivers/gpu/drm/i915/i915_request.c
> > > @@ -1583,38 +1583,16 @@ i915_request_await_object(struct i915_request 
> > > *to,
> > >                 struct drm_i915_gem_object *obj,
> > >                 bool write)
> > >   {
> > > -    struct dma_fence *excl;
> > > +    struct dma_resv_cursor cursor;
> > > +    struct dma_fence *fence;
> > >       int ret = 0;
> > >   -    if (write) {
> > > -        struct dma_fence **shared;
> > > -        unsigned int count, i;
> > > -
> > > -        ret = dma_resv_get_fences(obj->base.resv, &excl, &count,
> > > -                      &shared);
> > > -        if (ret)
> > > -            return ret;
> > > -
> > > -        for (i = 0; i < count; i++) {
> > > -            ret = i915_request_await_dma_fence(to, shared[i]);
> > > -            if (ret)
> > > -                break;
> > > -
> > > -            dma_fence_put(shared[i]);
> > > +    dma_resv_for_each_fence_unlocked(obj->base.resv, &cursor, write, 
> > > fence) {
> >
> > I think callers have the object locked for this one. At least if you 
> > haven't tried it it's worth asking CI (you have the assert already so 
> > it will tell you). But I think it's important to have an atomic 
> > snapshot here.
>
> Thanks for the info. In this case I'm just going to use the locked 
> variant of the iterator here for the next round.
>
> Could you point me to the place where the lock is grabed/released for 
> reference?

There is quite a few callers and I haven't audited all of them. But I 
think, given the function is used for setting up tracking of implicit 
dependencies, that it has to be true.

In the case of execbuf for instance the flow is relatively complicated:

i915_gem_do_execbuffer
  eb_relocate_parse
    eb_validate_vmas
      eb_lock_vmas
        i915_gem_object_lock
  eb_submit
    eb_move_to_gpu
      i915_request_await_object
  i915_gem_ww_ctx_fini
    i915_gem_ww_ctx_unlock_all
      i915_gem_object_unlock

Other call sites have simpler flows but there is a lot of them so I 
think using CI is easiest.

Regards,

Tvrtko

> Thanks,
> Christian.
>
> > Regards,
> >
> > Tvrtko
> >
> > > +        ret = i915_request_await_dma_fence(to, fence);
> > > +        if (ret) {
> > > +            dma_fence_put(fence);
> > > +            break;
> > >           }
> > > -
> > > -        for (; i < count; i++)
> > > -            dma_fence_put(shared[i]);
> > > -        kfree(shared);
> > > -    } else {
> > > -        excl = dma_resv_get_excl_unlocked(obj->base.resv);
> > > -    }
> > > -
> > > -    if (excl) {
> > > -        if (ret == 0)
> > > -            ret = i915_request_await_dma_fence(to, excl);
> > > -
> > > -        dma_fence_put(excl);
> > >       }
> > >         return ret;