Re: [Intel-gfx] [PATCH 01/26] dma-buf: add dma_resv_for_each_fence_unlocked v4

Tvrtko Ursulin
2021-09-22T15:09:28Z

On 22/09/2021 15:50, Christian König wrote:
> Am 22.09.21 um 16:36 schrieb Tvrtko Ursulin:
>>> +
>>> +/**
>>> + * dma_resv_iter_first_unlocked - first fence in an unlocked
>>> dma_resv obj.
>>> + * @cursor: the cursor with the current position
>>> + *
>>> + * Returns the first fence from an unlocked dma_resv obj.
>>> + */
>>> +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter
>>> *cursor)
>>> +{
>>> +    rcu_read_lock();
>>> +    do {
>>> +        dma_resv_iter_restart_unlocked(cursor);
>>> +        dma_resv_iter_walk_unlocked(cursor);
>>> +    } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq));
>>> +    rcu_read_unlock();
>>> +
>>> +    return cursor->fence;
>>> +}
>>> +EXPORT_SYMBOL(dma_resv_iter_first_unlocked);
>>
>> Why is this one split from dma_resv_iter_begin and even exported?
>
> I've split it to be able to use dma_resv_iter_begin in both the unlocked
> and locked iterator.

Ok.

>
>> I couldn't find any users in the series.
>
> This is used in the dma_resv_for_each_fence_unlocked() macro to return
> the first fence.

Doh!

>>> +
>>> +/**
>>> + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv
>>> obj.
>>> + * @cursor: the cursor with the current position
>>> + *
>>> + * Returns the next fence from an unlocked dma_resv obj.
>>> + */
>>> +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter
>>> *cursor)
>>> +{
>>> +    bool restart;
>>> +
>>> +    rcu_read_lock();
>>> +    cursor->is_restarted = false;
>>> +    restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq);
>>> +    do {
>>> +        if (restart)
>>> +            dma_resv_iter_restart_unlocked(cursor);
>>> +        dma_resv_iter_walk_unlocked(cursor);
>>> +        restart = true;
>>> +    } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq));
>>> +    rcu_read_unlock();
>>> +
>>> +    return cursor->fence;
>>> +}
>>> +EXPORT_SYMBOL(dma_resv_iter_next_unlocked);
>>
>> Couldn't dma_resv_iter_first_unlocked and dma_resv_iter_next_unlocked
>> share the same implementation? Especially if you are able to replace
>> cursor->is_restarted with cursor->index == -1.
>
> That's what I had initially, but Daniel disliked it for some reason. You
> then need a centralized walk function instead of first/next.

I had some ideas to only consolidate "first" and "next" helpers but never mind, yours is fine as well.

Regards,

Tvrtko

>
> Thanks,
> Christian.
>
>> Regards,
>>
>> Tvrtko
>