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

[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? Since there is no timeline as a 
line, I think this is not right direction.

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