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

Fri Sep 14 07:26:41 UTC 2018

Am 14.09.2018 um 05:59 schrieb 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?

Well when the signal pt is already garbage collected you know that it is 
already signaled. So you can just return a dummy fence.

I actually thought that this was the intention of the dummy fence rename :)

Christian.

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