[PATCH 7/7] drm/syncobj: use the timeline point in drm_syncobj_find_fence

[zhoucm1]

On 2018年11月23日 18:10, Koenig, Christian wrote:
> Am 23.11.18 um 03:36 schrieb zhoucm1:
>>
>> On 2018年11月22日 19:30, Christian König wrote:
>>> Am 22.11.18 um 07:52 schrieb zhoucm1:
>>>>
>>>> On 2018年11月15日 19:12, Christian König wrote:
>>>>> Implement finding the right timeline point in drm_syncobj_find_fence.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_syncobj.c | 10 +++++++++-
>>>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>>> index 589d884ccd58..d42c51520da4 100644
>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>> @@ -307,9 +307,17 @@ int drm_syncobj_find_fence(struct drm_file
>>>>> *file_private,
>>>>>            return -ENOENT;
>>>>>          *fence = drm_syncobj_fence_get(syncobj);
>>>>> -    if (!*fence) {
>>>>> +    if (!*fence)
>>>>>            ret = -EINVAL;
>>>>> +
>>>>> +    if (!ret && point) {
>>>>> +        dma_fence_chain_for_each(*fence) {
>>>>> +            if (!to_dma_fence_chain(*fence) ||
>>>>> +                (*fence)->seqno <= point)
>>>>> +                break;
>>>> This condition isn't enough to find proper point.
>>>> For two examples:
>>>> a. No garbage collection happens, the points in chain are
>>>> 1----3----6----9----12---18---20, if user wants to get point17, then
>>>> we should return node 18.
>>> And that is exactly what's wrong in the original logic. In this case
>>> we need to return 12, not 18 because point 17 could have already been
>>> garbage collected.
>> I don't think so, the 'a' case I already assume there isn't garbage
>> collection. If user wants to get point17, then we should return node 18.
>> timeline means point[N]  must be signaled later than point[N-1].
>> Point[12] just can make sure point[1] ~point[12] are signaled.
>> Point[18] signal can make sure point[17] is signaled.
>> So this case we need to return 18, not 12, which is key timeline concept.
> No, exactly that's incorrect. When we ask for 17 and can't find it then
> this means it either never existed or that it is signaled already.
>
> Returning a lower number in this case or even a stub fence is perfectly
> fine since we only need to wait for that one in this case.
>
> If we return 18 in this case then we add incorrect synchronization when
> there shouldn't be any.
No, That will make timeline not work at all and break timeline semantics 
totally.

If there aren't point18 and point20, the chain is 
1----3----6----9----12, if user wants to get point 17, you also return 
12? if yes, which absolutely is incorrect. The answer should be NO, 
right? point17 should be waited on there until a bigger point is coming.

For chain is 1----3----6----9----12---18---20, if user wants to wait on 
any one of points 13,14,15,16,17,18, we must wait for point 18, this is 
timeline semantic.

You can also check sw_sync.c for timeline meaning.

-David
>
> Christian.
>
>> -David
>>>> b. garbage collection happens on point6, chain would be updated to
>>>> 1---3---9---12---18---20, if user wants to get point5, then we
>>>> should return node 3, but if user wants to get point 7, then we
>>>> should return node 9.
>>> Why? That doesn't seem to make any sense to me.
>>>
>>>> I still have no idea how to satisfy all these requirements with your
>>>> current chain-fence. All these logic just are same we encountered
>>>> before, we're walking them again. After solving these problems, I
>>>> guess all design is similar as before.
>>>>
>>>> In fact, I don't know what problem previous design has, maybe there
>>>> are some bugs, can't we fix these bugs by time going? Who can make
>>>> sure his implementation never have bugs?
>>> Well there where numerous problems with the original design. For
>>> example we need to reject the requirement that timeline fences are in
>>> order because that doesn't make sense in the kernel.
>>>
>>> When userspace does something like submitting fences in the order 1,
>>> 5, 3 then it is broken and can keep the pieces. In other words the
>>> kernel should not care about that, but rather make sure that it never
>>> looses any synchronization no matter what.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> -David
>>>>> +        }
>>>>>        }
>>>>> +
>>>>>        drm_syncobj_put(syncobj);
>>>>>        return ret;
>>>>>    }