[PATCH 1/3] [RFC]drm: add syncobj timeline support v4

[zhoucm1]

On 2018年09月14日 11:14, zhoucm1 wrote:
>
>
> On 2018年09月13日 18:22, Christian König wrote:
>> Am 13.09.2018 um 11:35 schrieb Zhou, David(ChunMing):
>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian
>>>> Sent: Thursday, September 13, 2018 5:20 PM
>>>> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; dri-
>>>> devel at lists.freedesktop.org
>>>> Cc: Dave Airlie <airlied at redhat.com>; Rakos, Daniel
>>>> <Daniel.Rakos at amd.com>; amd-gfx at lists.freedesktop.org
>>>> Subject: Re: [PATCH 1/3] [RFC]drm: add syncobj timeline support v4
>>>>
>>>> Am 13.09.2018 um 11:11 schrieb Zhou, David(ChunMing):
>>>>>> -----Original Message-----
>>>>>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>>>>>> Sent: Thursday, September 13, 2018 4:50 PM
>>>>>> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; Koenig, Christian
>>>>>> <Christian.Koenig at amd.com>; dri-devel at lists.freedesktop.org
>>>>>> Cc: Dave Airlie <airlied at redhat.com>; Rakos, Daniel
>>>>>> <Daniel.Rakos at amd.com>; amd-gfx at lists.freedesktop.org
>>>>>> Subject: Re: [PATCH 1/3] [RFC]drm: add syncobj timeline support v4
>>>>>>
>>>>>> Am 13.09.2018 um 09:43 schrieb Zhou, David(ChunMing):
>>>>>>>> -----Original Message-----
>>>>>>>> From: Koenig, Christian
>>>>>>>> Sent: Thursday, September 13, 2018 2:56 PM
>>>>>>>> To: Zhou, David(ChunMing) <David1.Zhou at amd.com>; Zhou,
>>>>>>>> David(ChunMing) <David1.Zhou at amd.com>; dri-
>>>>>>>> devel at lists.freedesktop.org
>>>>>>>> Cc: Dave Airlie <airlied at redhat.com>; Rakos, Daniel
>>>>>>>> <Daniel.Rakos at amd.com>; amd-gfx at lists.freedesktop.org
>>>>>>>> Subject: Re: [PATCH 1/3] [RFC]drm: add syncobj timeline support v4
>>>>>>>>
>>>>>>>> Am 13.09.2018 um 04:15 schrieb zhoucm1:
>>>>>>>>> On 2018年09月12日 19:05, Christian König wrote:
>>>>>>>>>>>>>> [SNIP]
>>>>>>>>>>>>>> +static void drm_syncobj_find_signal_pt_for_wait_pt(struct
>>>>>>>>>>>>>> drm_syncobj *syncobj,
>>>>>>>>>>>>>> +                           struct drm_syncobj_wait_pt
>>>>>>>>>>>>>> +*wait_pt) {
>>>>>>>>>>>>> That whole approach still looks horrible complicated to me.
>>>>>>>>>>> It's already very close to what you said before.
>>>>>>>>>>>
>>>>>>>>>>>>> Especially the separation of signal and wait pt is completely
>>>>>>>>>>>>> unnecessary as far as I can see.
>>>>>>>>>>>>> When a wait pt is requested we just need to search for the
>>>>>>>>>>>>> signal point which it will trigger.
>>>>>>>>>>> Yeah, I tried this, but when I implement cpu wait ioctl on
>>>>>>>>>>> specific point, we need a advanced wait pt fence, otherwise, we
>>>>>>>>>>> could still need old syncobj cb.
>>>>>>>>>> Why? I mean you just need to call drm_syncobj_find_fence() and
>>>>>> when
>>>>>>>>>> that one returns NULL you use wait_event_*() to wait for a 
>>>>>>>>>> signal
>>>>>>>>>> point >= your wait point to appear and try again.
>>>>>>>>> e.g. when there are 3 syncobjs(A,B,C) to wait, all syncobjABC 
>>>>>>>>> have
>>>>>>>>> no fence yet, as you said, during drm_syncobj_find_fence(A) is
>>>>>>>>> working on wait_event, syncobjB and syncobjC could already be
>>>>>>>>> signaled, then we don't know which one is first signaled, 
>>>>>>>>> which is
>>>>>>>>> need when wait ioctl returns.
>>>>>>>> I don't really see a problem with that. When you wait for the 
>>>>>>>> first
>>>>>>>> one you need to wait for A,B,C at the same time anyway.
>>>>>>>>
>>>>>>>> So what you do is to register a fence callback on the fences you
>>>>>>>> already have and for the syncobj which doesn't yet have a fence 
>>>>>>>> you
>>>>>>>> make sure that they wake up your thread when they get one.
>>>>>>>>
>>>>>>>> So essentially exactly what 
>>>>>>>> drm_syncobj_fence_get_or_add_callback()
>>>>>>>> already does today.
>>>>>>> So do you mean we need still use old syncobj CB for that?
>>>>>> Yes, as far as I can see it should work.
>>>>>>
>>>>>>>     Advanced wait pt is bad?
>>>>>> Well it isn't bad, I just don't see any advantage in it.
>>>>> The advantage is to replace old syncobj cb.
>>>>>
>>>>>> The existing mechanism
>>>>>> should already be able to handle that.
>>>>> I thought more a bit, we don't that mechanism at all, if use 
>>>>> advanced wait
>>>> pt, we can easily use fence array to achieve it for wait ioctl, we 
>>>> should use
>>>> kernel existing feature as much as possible, not invent another, 
>>>> shouldn't we?
>>>> I remember  you said  it before.
>>>>
>>>> Yeah, but the syncobj cb is an existing feature.
>>> This is obviously a workaround when doing for wait ioctl, Do you see 
>>> it used in other place?
>>>
>>>> And I absolutely don't see a
>>>> need to modify that and replace it with something far more complex.
>>> The wait ioctl is simplified much more by fence array, not complex, 
>>> and we just need  to allocate a wait pt.  If keeping old syncobj cb 
>>> workaround, all wait pt logic still is there, just save allocation 
>>> and wait pt handling, in fact, which part isn't complex at all. But 
>>> compare with ugly syncobj cb, which is simpler.
>>
>> I strongly disagree on that. You just need to extend the syncobj cb 
>> with the sequence number and you are done.
>>
>> We could clean that up in the long term by adding some wait_multi 
>> event macro, but for now just adding the sequence number should do 
>> the trick.
>
> Quote from Daniel Vetter comment when v1, "
>
> Specifically for this stuff here having unified future fence semantics
> will allow drivers to do clever stuff with them.
>
> "
> I think the advanced wait pt is a similar concept as 'future fence' 
> what Daniel Vetter said before, which obviously a right direction.
>
>
> Anyway, I will change the patch as you like if no other comment, so 
> that the patch can pass soon.
When I try to remove wait pt future fence, I encounter another problem, 
drm_syncobj_find_fence cannot get a fence if signal pt already is 
collected as garbage, then CS will report error, any idea for that?
I still think the future fence is right thing, Could you give futher 
thought on it again? Otherwise, we could need various workarounds.

Thanks,
David Zhou
>
> Thanks,
> David Zhou
>>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks,
>>> David Zhou
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Thanks,
>>>>> David Zhou
>>>>>> Christian.
>>>>>>
>>>>>>> Thanks,
>>>>>>> David Zhou
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> Back to my implementation, it already fixes all your concerns
>>>>>>>>> before, and can be able to easily used in wait_ioctl. When you
>>>>>>>>> feel that is complicated, I guess that is because we merged all
>>>>>>>>> logic to that and much clean up in one patch. In fact, it already
>>>>>>>>> is very simple, timeline_init/fini, create signal/wait_pt, find
>>>>>>>>> signal_pt for wait_pt, garbage collection, just them.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> David Zhou
>>>>>>>>>> Regards,
>>>>>>>>>> Christian.
>>>>>>> _______________________________________________
>>>>>>> amd-gfx mailing list
>>>>>>> amd-gfx at lists.freedesktop.org
>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>