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

[Christian König]

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.

The key is that as soon as a signal point is added adding a previous 
point is no longer allowed.

>>>>
>>>> 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
>>
>