[Intel-gfx] [PATCH 1/3] drm/i915: Convert execbuf to use struct-of-array packing for critical fields

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-06-19 13:06:18)
> 
> On 16/06/2017 17:02, Chris Wilson wrote:
> > @@ -520,7 +511,8 @@ static inline int use_cpu_reloc(const struct reloc_cache *cache,
> >   static int eb_reserve_vma(const struct i915_execbuffer *eb,
> >                         struct i915_vma *vma)
> >   {
> > -     struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
> > +     struct drm_i915_gem_exec_object2 *entry =
> > +             &eb->exec[vma->exec_flags - eb->flags];
> 
> There are three instances of this so maybe "entry = exec_entry(eb, 
> vma)"? Added benefit that exec_entry implementation could site a comment 
> explaining the magic.

You were meant to jump in and suggest some way I would store the index.
:)

> > @@ -870,23 +864,20 @@ static void eb_release_vmas(const struct i915_execbuffer *eb)
> >       unsigned int i;
> >   
> >       for (i = 0; i < count; i++) {
> > -             struct drm_i915_gem_exec_object2 *entry = &eb->exec[i];
> > -             struct i915_vma *vma = exec_to_vma(entry);
> > +             struct i915_vma *vma = eb->vma[i];
> > +             unsigned int flags = eb->flags[i];
> >   
> >               if (!vma)
> >                       continue;
> >   
> > -             GEM_BUG_ON(vma->exec_entry != entry);
> > -             vma->exec_entry = NULL;
> > +             GEM_BUG_ON(vma->exec_flags != &eb->flags[i]);
> > +             vma->exec_flags = NULL;
> >   
> > -             if (entry->flags & __EXEC_OBJECT_HAS_PIN)
> > -                     __eb_unreserve_vma(vma, entry);
> > +             if (flags & __EXEC_OBJECT_HAS_PIN)
> > +                     __eb_unreserve_vma(vma, flags);
> >   
> > -             if (entry->flags & __EXEC_OBJECT_HAS_REF)
> > +             if (flags & __EXEC_OBJECT_HAS_REF)
> >                       i915_vma_put(vma);
> > -
> > -             entry->flags &=
> > -                     ~(__EXEC_OBJECT_RESERVED | __EXEC_OBJECT_HAS_REF);
> 
> No need to clear these from eb->flags[i] to ensure no double free?

No, the other path that may drop the vma prevents the double-free by
setting count to 0. Along the error path we don't have the vma set for
incomplete loads.

> > @@ -1830,33 +1824,34 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
> >                               return -ENOMEM;
> >   
> >                       capture->next = eb->request->capture_list;
> > -                     capture->vma = vma;
> > +                     capture->vma = eb->vma[i];
> >                       eb->request->capture_list = capture;
> >               }
> >   
> > -             if (entry->flags & EXEC_OBJECT_ASYNC)
> > -                     goto skip_flushes;
> > +             if (flags & EXEC_OBJECT_ASYNC)
> > +                     continue;
> >   
> > +             obj = eb->vma[i]->obj;
> >               if (unlikely(obj->cache_dirty && !obj->cache_coherent))
> >                       i915_gem_clflush_object(obj, 0);
> >   
> >               err = i915_gem_request_await_object
> > -                     (eb->request, obj, entry->flags & EXEC_OBJECT_WRITE);
> > +                     (eb->request, obj, flags & EXEC_OBJECT_WRITE);
> >               if (err)
> >                       return err;
> > -
> > -skip_flushes:
> > -             i915_vma_move_to_active(vma, eb->request, entry->flags);
> > -             __eb_unreserve_vma(vma, entry);
> > -             vma->exec_entry = NULL;
> 
> Grumble, it would have been better if this change was in a separate patch.

Grumble. It was more meaningful at some earlier version of the patch
where there was a dance here with flags.
-Chris