[PATCH 5/5] [RFC]drm: add syncobj timeline support v3
zhoucm1
zhoucm1 at amd.com
Tue Sep 4 04:04:48 UTC 2018
On 2018年09月03日 19:19, Christian König wrote:
> Am 03.09.2018 um 12:07 schrieb Chunming Zhou:
>>
>>
>> 在 2018/9/3 16:50, Christian König 写道:
>>> Am 03.09.2018 um 06:13 schrieb Chunming Zhou:
>>>>
>>>>
>>>> 在 2018/8/30 19:32, Christian König 写道:
>>>>> [SNIP]
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +struct drm_syncobj_wait_pt {
>>>>>>>>>> + struct drm_syncobj_stub_fence base;
>>>>>>>>>> + u64 value;
>>>>>>>>>> + struct rb_node node;
>>>>>>>>>> +};
>>>>>>>>>> +struct drm_syncobj_signal_pt {
>>>>>>>>>> + struct drm_syncobj_stub_fence base;
>>>>>>>>>> + struct dma_fence *signal_fence;
>>>>>>>>>> + struct dma_fence *pre_pt_base;
>>>>>>>>>> + struct dma_fence_cb signal_cb;
>>>>>>>>>> + struct dma_fence_cb pre_pt_cb;
>>>>>>>>>> + struct drm_syncobj *syncobj;
>>>>>>>>>> + u64 value;
>>>>>>>>>> + struct list_head list;
>>>>>>>>>> +};
>>>>>>>>>
>>>>>>>>> What are those two structures good for
>>>>>>>> They are used to record wait ops points and signal ops points.
>>>>>>>> For timeline, they are connected by timeline value, works like:
>>>>>>>> a. signal pt increase timeline value to signal_pt value
>>>>>>>> when signal_pt is signaled. signal_pt is depending on previous
>>>>>>>> pt fence and itself signal fence from signal ops.
>>>>>>>> b. wait pt tree checks if timeline value over itself value.
>>>>>>>>
>>>>>>>> For normal, works like:
>>>>>>>> a. signal pt increase 1 for syncobj_timeline->signal_point
>>>>>>>> every time when signal ops is performed.
>>>>>>>> b. when wait ops is comming, wait pt will fetch above last
>>>>>>>> signal pt value as its wait point. wait pt will be only
>>>>>>>> signaled by equal point value signal_pt.
>>>>>>>>
>>>>>>>>
>>>>>>>>> and why is the stub fence their base?
>>>>>>>> Good question, I tried to kzalloc for them as well when I debug
>>>>>>>> them, I encountered a problem:
>>>>>>>> I lookup/find wait_pt or signal_pt successfully, but when I
>>>>>>>> tried to use them, they are freed sometimes, and results in
>>>>>>>> NULL point.
>>>>>>>> and generally, when lookup them, we often need their stub fence
>>>>>>>> as well, and in the meantime, their lifecycle are same.
>>>>>>>> Above reason, I make stub fence as their base.
>>>>>>>
>>>>>>> That sounds like you only did this because you messed up the
>>>>>>> lifecycle.
>>>>>>>
>>>>>>> Additional to that I don't think you correctly considered the
>>>>>>> lifecycle of the waits and the sync object itself. E.g. blocking
>>>>>>> in drm_syncobj_timeline_fini() until all waits are done is not a
>>>>>>> good idea.
>>>>>>>
>>>>>>> What you should do instead is to create a fence_array object
>>>>>>> with all the fence we need to wait for when a wait point is
>>>>>>> requested.
>>>>>> Yeah, this is our initial discussion result, but when I tried to
>>>>>> do that, I found that cannot meet the advance signal requirement:
>>>>>> a. We need to consider the wait and signal pt value are not
>>>>>> one-to-one match, it's difficult to find dependent point, at
>>>>>> least, there is some overhead.
>>>>>
>>>>> As far as I can see that is independent of using a fence array
>>>>> here. See you can either use a ring buffer or an rb-tree, but when
>>>>> you want to wait for a specific point we need to condense the not
>>>>> yet signaled fences into an array.
>>>> again, need to find the range of where the specific point in, we
>>>> should close to timeline semantics, I also refered the sw_sync.c
>>>> timeline, usally wait_pt is signalled by timeline point. And I
>>>> agree we can implement it with several methods, but I don't think
>>>> there are basical differences.
>>>
>>> The problem is that with your current approach you need the sync_obj
>>> alive for the synchronization to work. That is most likely not a
>>> good idea.
>> Indeed, I will fix that. How abount only creating fence array for
>> every wait pt when syncobj release? when syncobj release, wait pt
>> must have waited the signal opertation, then we can easily condense
>> fences for every wait pt. And meantime, we can take timeline based
>> wait pt advantage.
>
> That could work, but I don't see how you want to replace the already
> issued fence with a fence_array when the sync object is destroyed.
>
> Additional to that I would rather prefer a consistent handling, e.g.
> without extra rarely used code paths.
Ah, I find a easy way, we just need to make syncobj_timeline structure
as a reference. This way syncobj itself can be released first,
wait_pt/signal_pt don't need syncobj at all.
every wait_pt/signal_pt keep a reference of syncobj_timeline.
>
>>
>>>
>>> Additional to that you enable signaling without a need from the
>>> waiting side. That is rather bad for implementations which need that
>>> optimization.
>> Do you mean increasing timeline based on signal fence is not better?
>> only update timeline value when requested by a wait pt?
>
> Yes, exactly.
>
>> This way, we will not update timeline value immidiately and cannot
>> free signal pt immidiately, and we also need to consider it to CPU
>> query and wait.
>
> That is actually the better coding style. We usually try to avoid
> doing things in interrupt handlers as much as possible.
OK, I see your concern, how about to delay handling to a workqueue? this
way, we only increase timeline value and wake up workqueue in fence cb,
is that acceptable?
>
> 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.
>
> 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.
>
> 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.
Thanks,
David Zhou
>
> Regards,
> Christian.
>
>>
>> Thanks,
>> David Zhou
>>
>>>
>>> I suggest to either condense all the fences you need to wait for in
>>> an array during the wait operation, or reference count the sync_obj
>>> and only enable the signaling on the fences when requested by a wait.
>>>
>>>>
>>>>>
>>>>>> b. because we allowed "wait-before-signal", if
>>>>>> "wait-before-signal" happens, there isn't signal fence which can
>>>>>> be used to create fence array.
>>>>>
>>>>> Well, again we DON'T support wait-before-signal here. I will
>>>>> certainly NAK any implementation which tries to do this until we
>>>>> haven't figured out all the resource management constraints and I
>>>>> still don't see how we can do this.
>>>> yes, we don't support real wait-before-signal in patch now, just a
>>>> fake wait-before-signal, which still wait on CS submission until
>>>> signal operation coming through wait_event, which is the conclusion
>>>> we disscussed before.
>>>
>>> Well in this case we should call it wait-for-signal and not
>>> wait-before-signal :)
>>>
>>>>
>>>>>
>>>>>> So timeline value is good to resolve that.
>>>>>>
>>>>>>>
>>>>>>> Otherwise somebody can easily construct a situation where
>>>>>>> timeline sync obj A waits on B and B waits on A.
>>>>>> Same situation also can happens with fence-array, we only can see
>>>>>> this is a programming bug with incorrect use.
>>>>>
>>>>> No, fence-array is initialized only once with a static list of
>>>>> fences. This way it is impossible to add the fence-array to itself
>>>>> for example.
>>>>>
>>>>> E.g. you can't build circle dependencies with that.
>>>> we already wait for signal operation event, so never build circle
>>>> dependencies with that. The theory is same.
>>>
>>> Yeah, ok that is certainly true.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>>
>>>>>
>>>>>> Better use rbtree_postorder_for_each_entry_safe() for this.
>>>>>>>> From the comments, seems we cannot use it here, we need erase
>>>>>>>> node here.
>>>>>>>> Comments:
>>>>>>>> * Note, however, that it cannot handle other modifications
>>>>>>>> that re-order the
>>>>>>>> * rbtree it is iterating over. This includes calling
>>>>>>>> rb_erase() on @pos, as
>>>>>>>> * rb_erase() may rebalance the tree, causing us to miss some
>>>>>>>> nodes.
>>>>>>>> */
>>>>>>>
>>>>>>> Yeah, but your current implementation has the same problem :)
>>>>>>>
>>>>>>> You need to use something like "while (node = rb_first(...))"
>>>>>>> instead if you want to destruct the whole tree.
>>>> OK, got it, I will change it in v4.
>>>>
>>>> Thanks,
>>>> David Zhou
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
More information about the amd-gfx
mailing list