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 >
