[PATCH 1/3] [RFC]drm: add syncobj timeline support v4
Zhou, David(ChunMing)
David1.Zhou at amd.com
Fri Sep 14 07:46:21 UTC 2018
> -----Original Message-----
> From: Koenig, Christian
> Sent: Friday, September 14, 2018 3:27 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; Daniel Vetter
> <daniel at 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 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 :)
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.
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 at lists.freedesktop.org
> >>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> >>>
> >>
> >
More information about the amd-gfx
mailing list