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

[Tvrtko Ursulin]

On 19/06/2017 13:22, Chris Wilson wrote:
> 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.
> :)

No ideas I'm afraid, maybe it is just to hot. :)

>>> @@ -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.

Cool.

>>> @@ -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.

Can't spot any issues anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko