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

Wed Sep 5 06:55:09 UTC 2018

Am 05.09.2018 um 05:36 schrieb 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.

I don't full understand the problem here. When we need to wait for a 
fence it is obvious that we need to enable signaling. So what exactly is 
the issue?

BTW: I'm going to commit patches #1-#4 to drm-misc-next now if nobody is 
going to object.

Christian.

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