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

[zhoucm1]

On 2018年09月04日 17:20, Christian König wrote:
> 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.
I will try to fix this problem.
btw, when I try your suggestion, I find it will be difficult to 
implement drm_syncobj_array_wait_timeout by your idea, since it needs 
first_signaled. if there is un-signaled syncobj, we will still register 
cb to timeline value change, then still back to need enble_signaling.

Thanks,
David Zhou
>
> 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
>>>>
>>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx