Re: [Intel-gfx] [PATCH 02/28] dma-buf: add dma_resv_for_each_fence v2

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/10/2021 09:40, Tvrtko Ursulin wrote:
> 
> On 05/10/2021 12:37, Christian König wrote:
>> A simpler version of the iterator to be used when the dma_resv object is
>> locked.
>>
>> v2: fix index check here as well
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-resv.c | 49 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/dma-resv.h   | 19 +++++++++++++++
>>   2 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
>> index 3cbcf66a137e..231bae173ef1 100644
>> --- a/drivers/dma-buf/dma-resv.c
>> +++ b/drivers/dma-buf/dma-resv.c
>> @@ -423,6 +423,55 @@ struct dma_fence 
>> *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor)
>>   }
>>   EXPORT_SYMBOL(dma_resv_iter_next_unlocked);
>> +/**
>> + * dma_resv_iter_first - first fence from a locked dma_resv object
>> + * @cursor: cursor to record the current position
>> + *
>> + * Return all the fences in the dma_resv object while holding the
>> + * &dma_resv.lock.
>> + */
>> +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor)
>> +{
>> +    struct dma_fence *fence;
>> +
>> +    dma_resv_assert_held(cursor->obj);
>> +
>> +    cursor->index = 0;
>> +    cursor->fences = dma_resv_shared_list(cursor->obj);
>> +
>> +    fence = dma_resv_excl_fence(cursor->obj);
>> +    if (!fence)
>> +        fence = dma_resv_iter_next(cursor);
> 
> "Is restarted" probably does not matter hugely for the locked iterator 
> but I think if it hits this path (no exclusive fence, returns first 
> shared) then it will show it as false. Which is not consistent with the 
> unlocked iterator.

Sorry I was blind or I don't know which version of which patch I was 
looking at.. It is set to true a few lines below. :)

Regards,

Tvrtko

> 
> Bonus points if you make a debug build assert that makes querying "is 
> restarted" warn when used with the locked iterator.
> 
>> +
>> +    cursor->is_restarted = true;
>> +    return fence;
>> +}
>> +EXPORT_SYMBOL_GPL(dma_resv_iter_first);
>> +
>> +/**
>> + * dma_resv_iter_next - next fence from a locked dma_resv object
>> + * @cursor: cursor to record the current position
>> + *
>> + * Return all the fences in the dma_resv object while holding the
>> + * &dma_resv.lock.
> 
> You probably want to replace "all the fences" with first and next, 
> respectively, in here and in dma_resv_iter_first kerneldoc.
> 
>> + */
>> +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor)
>> +{
>> +    unsigned int idx;
>> +
>> +    dma_resv_assert_held(cursor->obj);
>> +
>> +    cursor->is_restarted = false;
>> +    if (!cursor->all_fences || !cursor->fences ||
>> +        cursor->index >= cursor->fences->shared_count)
>> +        return NULL;
> 
> Theoretically you could store the shared count in the cursor and so 
> could have a single condition here (assuming initialized to zero when 
> !all_fences and !cursor->fences). For some value of optimisation. :) 
> Probably not worth it.
> 
> But you could only assign cursor->fences if all_fences, in 
> dma_resv_iter_first, so wouldn't have to duplicate the all_fences check 
> here.
> 
>> +
>> +    idx = cursor->index++;
>> +    return rcu_dereference_protected(cursor->fences->shared[idx],
>> +                     dma_resv_held(cursor->obj));
>> +}
>> +EXPORT_SYMBOL_GPL(dma_resv_iter_next);
>> +
>>   /**
>>    * dma_resv_copy_fences - Copy all fences from src to dst.
>>    * @dst: the destination reservation object
>> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
>> index 764138ad8583..3df7ef23712d 100644
>> --- a/include/linux/dma-resv.h
>> +++ b/include/linux/dma-resv.h
>> @@ -179,6 +179,8 @@ struct dma_resv_iter {
>>   struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter 
>> *cursor);
>>   struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter 
>> *cursor);
>> +struct dma_fence *dma_resv_iter_first(struct dma_resv_iter *cursor);
>> +struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor);
>>   /**
>>    * dma_resv_iter_begin - initialize a dma_resv_iter object
>> @@ -244,6 +246,23 @@ static inline bool 
>> dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
>>       for (fence = dma_resv_iter_first_unlocked(cursor);        \
>>            fence; fence = dma_resv_iter_next_unlocked(cursor))
>> +/**
>> + * dma_resv_for_each_fence - fence iterator
>> + * @cursor: a struct dma_resv_iter pointer
>> + * @obj: a dma_resv object pointer
>> + * @all_fences: true if all fences should be returned
>> + * @fence: the current fence
>> + *
>> + * Iterate over the fences in a struct dma_resv object while holding the
>> + * &dma_resv.lock. @all_fences controls if the shared fences are 
>> returned as
>> + * well. The cursor initialisation is part of the iterator and the 
>> fence stays
>> + * valid as long as the lock is held.
> 
> I'd be super cautious and explicitly spell out that reference is not 
> held in contrast to the unlocked iterator.
> 
>> + */
>> +#define dma_resv_for_each_fence(cursor, obj, all_fences, fence)    \
>> +    for (dma_resv_iter_begin(cursor, obj, all_fences),    \
>> +         fence = dma_resv_iter_first(cursor); fence;    \
>> +         fence = dma_resv_iter_next(cursor))
>> +
>>   #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base)
>>   #define dma_resv_assert_held(obj) 
>> lockdep_assert_held(&(obj)->lock.base)
>>
> 
> Regards,
> 
> Tvrtko