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

[zhoucm1 <zhoucm1-5C7GfCeVMHo@public.gmane.org>]

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@amd.com>; dri-
>>> devel@lists.freedesktop.org
>>> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
>>> <Daniel.Rakos@amd.com>; amd-gfx@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@gmail.com>
>>>>> Sent: Thursday, September 13, 2018 4:50 PM
>>>>> To: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Koenig, Christian
>>>>> <Christian.Koenig@amd.com>; dri-devel@lists.freedesktop.org
>>>>> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
>>>>> <Daniel.Rakos@amd.com>; amd-gfx@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@amd.com>; Zhou,
>>>>>>> David(ChunMing) <David1.Zhou@amd.com>; dri-
>>>>>>> devel@lists.freedesktop.org
>>>>>>> Cc: Dave Airlie <airlied@redhat.com>; Rakos, Daniel
>>>>>>> <Daniel.Rakos@amd.com>; amd-gfx@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.

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@lists.freedesktop.org
>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx