[PATCH 5/5] [RFC]drm: add syncobj timeline support v3

[zhoucm1]

On 2018年09月04日 16:42, Christian König wrote:
> Am 04.09.2018 um 10:27 schrieb zhoucm1:
>>
>>
>> On 2018年09月04日 16:05, Christian König wrote:
>>> Am 04.09.2018 um 09:53 schrieb zhoucm1:
>>>> [SNIP]
>>>>>>>
>>>>>>> How about this idea:
>>>>>>> 1. Each signaling point is a fence implementation with an rb node.
>>>>>>> 2. Each node keeps a reference to the last previously inserted 
>>>>>>> node.
>>>>>>> 3. Each node is referenced by the sync object itself.
>>>>>>> 4. Before each signal/wait operation we do a garbage collection 
>>>>>>> and remove the first node from the tree as long as it is signaled.
>>>>>>>
>>>>>>> 5. When enable_signaling is requested for a node we cascade that 
>>>>>>> to the left using rb_prev.
>>>>>>>     This ensures that signaling is enabled for the current fence 
>>>>>>> as well as all previous fences.
>>>>>>>
>>>>>>> 6. A wait just looks into the tree for the signal point lower or 
>>>>>>> equal of the requested sequence number.
>>>> After re-thought your idea, I think it doesn't work since there is 
>>>> no timeline value as a line:
>>>> signal pt value doesn't must be continues, which can be jump by 
>>>> signal operation, like 1, 4, 8, 15, 19, e.g. there are five 
>>>> singal_pt, 
>>>> signal_pt1->signal_pt4->signal_pt8->signal_pt15->signal_pt19, if a 
>>>> wait pt is 7, do you mean this wait only needs signal_pt1 and 
>>>> signal_pt4???  That's certainly not right, we need to make sure the 
>>>> timeline value is bigger than wait pt value, that means signal_pt8 
>>>> is need for wait_pt7.
>>>
>>> That can be defined as we like it, e.g. when a wait operation asks 
>>> for 7 we can return 8 as well.
>> If defined this, then problem is coming again, if 8 is removed when 
>> garbage collection, you will return 15?
>
> The garbage collection is only done for signaled nodes. So when 8 is 
> already garbage collected and 7 is asked we know that we don't need to 
> return anything.
8 is a signaled node, waitA/signal operation do garbage collection, how 
waitB(7) know the garbage history?

>
>> Since there is no timeline as a line, I think this is not right 
>> direction.
>
> That is actually intended. There is no infinite timeline here, just a 
> windows of the last not yet signaled fences.
No one said the it's a infinite timeline, timeline will stop increasing 
when syncobj is released.

Anyway kref is a good way to solve the 'free' problem, I will try to use 
it improve my patch, of course, will refer your idea.:)

Thanks,
David Zhou
>
> Otherwise you will never be able to release nodes from the tree since 
> you always need to keep them around just in case somebody asks for a 
> lower number.
>
> Regards,
> Christian.
>
>>
>>>
>>> The key is that as soon as a signal point is added adding a previous 
>>> point is no longer allowed.
>> That's intention.
>>
>> Regards,
>> David Zhou
>>>
>>>>>>>
>>>>>>> 7. When the sync object is released we use 
>>>>>>> rbtree_postorder_for_each_entry_safe() and drop the extra 
>>>>>>> reference to each node, but never call rb_erase!
>>>>>>>     This way the rb_tree stays in memory, but without a root 
>>>>>>> (e.g. the sync object). It only destructs itself when the looked 
>>>>>>> up references to the nodes are dropped.
>>>> And here, who will destroy rb node since no one do 
>>>> enable_signaling, and there is no callback to free themselves.
>>>
>>> The node will be destroyed when the last reference drops, not when 
>>> enable_signaling is called.
>>>
>>> In other words the sync_obj keeps the references to each tree object 
>>> to provide the wait operation, as soon as the sync_obj is destroyed 
>>> we don't need that functionality any more.
>>>
>>> We don't even need to wait for anything to be signaled, this way we 
>>> can drop all unused signal points as soon as the sync_obj is destroyed.
>>>
>>> Only the used ones will stay alive and provide the necessary 
>>> functionality to provide the signal for each wait operation.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Regards,
>>>> David Zhou
>>>>>>>
>>>>>>> Well that is quite a bunch of logic, but I think that should 
>>>>>>> work fine.
>>>>>> Yeah, it could work, simple timeline reference also can solve 
>>>>>> 'free' problem.
>>>>>
>>>>> I think this approach is still quite a bit better, 
>>>>
>>>>> e.g. you don't run into circle dependency problems, it needs less 
>>>>> memory and each node has always the same size which means we can 
>>>>> use a kmem_cache for it.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David Zhou
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>