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

[Christian König]

Am 04.09.2018 um 11:00 schrieb 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?

Well we of course keep what the last garbage collected number is, don't we?

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

Yeah, but the syncobj can live for a very very long time. Not having 
some form of shrinking it when fences are signaled is certainly not 
going to fly very far.

Regards,
Christian.

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