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

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

Am 14.09.2018 um 09:46 schrieb Zhou, David(ChunMing):
>
>> -----Original Message-----
>> From: Koenig, Christian
>> Sent: Friday, September 14, 2018 3:27 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; Daniel Vetter
>> <daniel@ffwll.ch>
>> Subject: Re: [PATCH 1/3] [RFC]drm: add syncobj timeline support v4
>>
>> 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@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.
>>> 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 :)
> In fact, this is one of future fence functionality, then why not we unify them? Daniel Vetter also commented that before v1.
> Future fence can unify both your dummy fence and old syncobj cb.
> If you have no objection, I will prepare a patch to let us  see how simple wait_ioctl using fence array.

I do have objections, please keep the existing wait as it is.

Regards,
Christian.

>
>
> Thanks,
> David Zhou
>> 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@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