[Intel-gfx] [PATCH 24/28] drm: use new iterator in drm_gem_plane_helper_prepare_fb

[Christian König]

Am 05.10.21 um 12:47 schrieb Tvrtko Ursulin:
>
> On 05/10/2021 11:27, Christian König wrote:
>> Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin:
>>>
>>> On 01/10/2021 11:06, Christian König wrote:
>>>> Makes the handling a bit more complex, but avoids the use of
>>>> dma_resv_get_excl_unlocked().
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++--
>>>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c 
>>>> b/drivers/gpu/drm/drm_gem_atomic_helper.c
>>>> index e570398abd78..21ed930042b8 100644
>>>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>>>> @@ -143,6 +143,7 @@
>>>>    */
>>>>   int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, 
>>>> struct drm_plane_state *state)
>>>>   {
>>>> +    struct dma_resv_iter cursor;
>>>>       struct drm_gem_object *obj;
>>>>       struct dma_fence *fence;
>>>>   @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct 
>>>> drm_plane *plane, struct drm_plane_st
>>>>           return 0;
>>>>         obj = drm_gem_fb_get_obj(state->fb, 0);
>>>> -    fence = dma_resv_get_excl_unlocked(obj->resv);
>>>> -    drm_atomic_set_fence_for_plane(state, fence);
>>>> +    dma_resv_iter_begin(&cursor, obj->resv, false);
>>>> +    dma_resv_for_each_fence_unlocked(&cursor, fence) {
>>>> +        dma_fence_get(fence);
>>>> +        dma_resv_iter_end(&cursor);
>>>> +        /* TODO: We only use the first write fence here */
>>>
>>> What is the TODO? NB instead?
>>
>> The drm atomic API can unfortunately handle only one fence and we can 
>> certainly have more than that.
>>
>>>
>>>> + drm_atomic_set_fence_for_plane(state, fence);
>>>> +        return 0;
>>>> +    }
>>>> +    dma_resv_iter_end(&cursor);
>>>>   +    drm_atomic_set_fence_for_plane(state, NULL);
>>>
>>>     dma_resv_iter_begin(&cursor, obj->resv, false);
>>>     dma_resv_for_each_fence_unlocked(&cursor, fence) {
>>>         dma_fence_get(fence);
>>>         break;
>>>     }
>>>     dma_resv_iter_end(&cursor);
>>>
>>>     drm_atomic_set_fence_for_plane(state, fence);
>>>
>>> Does this work?
>>
>> Yeah that should work as well.
>>
>>>
>>> But overall I am not sure this is nicer. Is the goal to remove 
>>> dma_resv_get_excl_unlocked as API it just does not happen in this 
>>> series?
>>
>> Yes, the only user left is the i915_gem_object_last_write_engine() 
>> function and that one you already removed in i915.
>
> To me the above feels clumsier than dma_resv_get_excl_unlocked and you 
> can even view it as open coding that helper. So don't know, someone 
> else can have a casting vote.
>
> I guess if support for more than one fence is coming soon(-ish) do drm 
> atomic api then I could be convinced the iterator here makes sense today.

Well Daniel noted that the drm atomic API needs some more work here 
because we don't handle different fences for different planes correctly 
either.

We could as well postpone this change and fix the atomic functions first.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>       return 0;
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
>>>>
>>