Re: [PATCH v6] drm/i915: Individualize fences before adding to dma_resv obj

[Matthew Auld <matthew.auld@xxxxxxxxx>]

On 27/05/2022 10:46, Das, Nirmoy wrote:
> On 5/26/2022 11:27 AM, Matthew Auld wrote:
> > On 25/05/2022 10:59, Nirmoy Das wrote:
> > > _i915_vma_move_to_active() can receive > 1 fences for
> > > multiple batch buffers submission. Because dma_resv_add_fence()
> > > can only accept one fence at a time, change _i915_vma_move_to_active()
> > > to be aware of multiple fences so that it can add individual
> > > fences to the dma resv object.
> > >
> > > v6: fix multi-line comment.
> > > v5: remove double fence reservation for batch VMAs.
> > > v4: Reserve fences for composite_fence on multi-batch contexts and
> > >      also reserve fence slots to composite_fence for each VMAs.
> > > v3: dma_resv_reserve_fences is not cumulative so pass num_fences.
> > > v2: make sure to reserve enough fence slots before adding.
> > >
> > > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5614
> > > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> >
> > Do we need Fixes: ?
>
>
> We can add:
>
> Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")

Ok, so needs:

Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
Cc: <stable@xxxxxxxxxxxxxxx> # v5.16+

Should I go ahead and push this to gt-next?

> Regards,
>
> Nirmoy
>
>
> > > ---
> > >   .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |  3 +-
> > >   drivers/gpu/drm/i915/i915_vma.c               | 48 +++++++++++--------
> > >   2 files changed, 30 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
> > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > index b279588c0672..8880d38d36b6 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > @@ -1010,7 +1010,8 @@ static int eb_validate_vmas(struct 
> > > i915_execbuffer *eb)
> > >               }
> > >           }
> > >   -        err = dma_resv_reserve_fences(vma->obj->base.resv, 1);
> > > +        /* Reserve enough slots to accommodate composite fences */
> > > +        err = dma_resv_reserve_fences(vma->obj->base.resv, 
> > > eb->num_batches);
> > >           if (err)
> > >               return err;
> > >   diff --git a/drivers/gpu/drm/i915/i915_vma.c 
> > > b/drivers/gpu/drm/i915/i915_vma.c
> > > index 4f6db539571a..0bffb70b3c5f 100644
> > > --- a/drivers/gpu/drm/i915/i915_vma.c
> > > +++ b/drivers/gpu/drm/i915/i915_vma.c
> > > @@ -23,6 +23,7 @@
> > >    */
> > >     #include <linux/sched/mm.h>
> > > +#include <linux/dma-fence-array.h>
> > >   #include <drm/drm_gem.h>
> > >     #include "display/intel_frontbuffer.h"
> > > @@ -1823,6 +1824,21 @@ int _i915_vma_move_to_active(struct i915_vma 
> > > *vma,
> > >       if (unlikely(err))
> > >           return err;
> > >   +    /*
> > > +     * Reserve fences slot early to prevent an allocation after 
> > > preparing
> > > +     * the workload and associating fences with dma_resv.
> > > +     */
> > > +    if (fence && !(flags & __EXEC_OBJECT_NO_RESERVE)) {
> > > +        struct dma_fence *curr;
> > > +        int idx;
> > > +
> > > +        dma_fence_array_for_each(curr, idx, fence)
> > > +            ;
> > > +        err = dma_resv_reserve_fences(vma->obj->base.resv, idx);
> > > +        if (unlikely(err))
> > > +            return err;
> > > +    }
> > > +
> > >       if (flags & EXEC_OBJECT_WRITE) {
> > >           struct intel_frontbuffer *front;
> > >   @@ -1832,31 +1848,23 @@ int _i915_vma_move_to_active(struct 
> > > i915_vma *vma,
> > >                   i915_active_add_request(&front->write, rq);
> > >               intel_frontbuffer_put(front);
> > >           }
> > > +    }
> > >   -        if (!(flags & __EXEC_OBJECT_NO_RESERVE)) {
> > > -            err = dma_resv_reserve_fences(vma->obj->base.resv, 1);
> > > -            if (unlikely(err))
> > > -                return err;
> > > -        }
> > > +    if (fence) {
> > > +        struct dma_fence *curr;
> > > +        enum dma_resv_usage usage;
> > > +        int idx;
> > >   -        if (fence) {
> > > -            dma_resv_add_fence(vma->obj->base.resv, fence,
> > > -                       DMA_RESV_USAGE_WRITE);
> > > +        obj->read_domains = 0;
> > > +        if (flags & EXEC_OBJECT_WRITE) {
> > > +            usage = DMA_RESV_USAGE_WRITE;
> > >               obj->write_domain = I915_GEM_DOMAIN_RENDER;
> > > -            obj->read_domains = 0;
> > > -        }
> > > -    } else {
> > > -        if (!(flags & __EXEC_OBJECT_NO_RESERVE)) {
> > > -            err = dma_resv_reserve_fences(vma->obj->base.resv, 1);
> > > -            if (unlikely(err))
> > > -                return err;
> > > +        } else {
> > > +            usage = DMA_RESV_USAGE_READ;
> > >           }
> > >   -        if (fence) {
> > > -            dma_resv_add_fence(vma->obj->base.resv, fence,
> > > -                       DMA_RESV_USAGE_READ);
> > > -            obj->write_domain = 0;
> > > -        }
> > > +        dma_fence_array_for_each(curr, idx, fence)
> > > +            dma_resv_add_fence(vma->obj->base.resv, curr, usage);
> > >       }
> > >         if (flags & EXEC_OBJECT_NEEDS_FENCE && vma->fence)