Re: [PATCH 2/7] drm: add syncobj timeline support v8

From: Daniel Vetter <daniel@ffwll.ch>
To: zhoucm1 <zhoucm1@amd.com>
Cc: Jason Ekstrand <jason@jlekstrand.net>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Rakos, Daniel" <Daniel.Rakos@amd.com>,
	amd-gfx list <amd-gfx@lists.freedesktop.org>,
	Dave Airlie <airlied@redhat.com>,
	christian.koenig@amd.com
Subject: Re: [PATCH 2/7] drm: add syncobj timeline support v8
Date: Fri, 19 Oct 2018 10:55:09 +0200	[thread overview]
Message-ID: <20181019085509.GZ31561@phenom.ffwll.local> (raw)
In-Reply-To: <544bca0a-977a-9900-d6dd-92c91ef9337e@amd.com>

On Fri, Oct 19, 2018 at 10:29:55AM +0800, zhoucm1 wrote:
> 
> 
> On 2018年10月18日 19:50, Christian König wrote:
> > Am 18.10.18 um 05:11 schrieb zhoucm1:
> > > 
> > > 
> > > On 2018年10月17日 18:24, Daniel Vetter wrote:
> > > > On Wed, Oct 17, 2018 at 11:29 AM Koenig, Christian
> > > > <Christian.Koenig@amd.com> wrote:
> > > > > Am 17.10.18 um 11:17 schrieb zhoucm1:
> > > > > > [SNIP]
> > > > > > > >    +struct drm_syncobj_signal_pt {
> > > > > > > > +    struct dma_fence_array *base;
> > > > > > > Out of curiosity, why the pointer and not embedding? base is kinda
> > > > > > > misleading for a pointer.
> > > > > > Yeah, Christian doesn't like signal_pt lifecycle same as fence, so
> > > > > > it's a pointer.
> > > > > > If you don't like 'base' name, I can change it.
> > > > > Well I never said that you can't embed the fence array into
> > > > > the signal_pt.
> > > > > 
> > > > > You just need to make sure that we don't affect the drm_syncobj
> > > > > lilecycle as well, e.g. that we don't also need to keep that around.
> > > > I don't see a problem with that, as long as drm_syncobj keeps a
> > > > reference to the fence while it's on the timeline list. Which it
> > > > already does. And embedding would avoid that 2nd separate allocation,
> > > > aside from making base less confusing.
> > > That's indeed my initial implementation for signal_pt/wait_pt with
> > > fence based, but after long and many discussions, we get current
> > > solution, as you see, the version is up to v8 :).
> > > 
> > > For here  why the pointer and not embedding?
> > > Two reasons:
> > > 1. their lifecycles are not same.
> > > 2. It is a fence array usage, which always needs separate
> > > allocation, seems which is mandatory.
> > > So it is a pointer.
> > > 
> > > But the name is historical from initial, and indeed be kinda
> > > misleading for a pointer, I will change it to fence_array instead in
> > > coming v9.
> > 
> > To avoid running into a v10 I've just pushed this version upstream :)
> Thanks a lot.

(This time reply to the right patch, silly me)

Went boom:

https://bugs.freedesktop.org/show_bug.cgi?id=108490

Can we revert pls?

Also, can we please have igts for this stuff so that intel-gfx-ci could
test this properly before it's all fireworks?

Thanks, Daniel

> > 
> > The rest in the series looks good to me as well,
> Can I get your RB on them first?
> 
> > but I certainly want the radv/anv developers to take a look as well as
> > Daniel suggested.
> Ping @Dave/Bas/Jason or other radv/anv developers, Could anyone of you take
> a look the rest of series for u/k interface? So that we can move to next
> step for libdrm patches?
> 
> Thanks,
> David
> > 
> > Christian.
> > 
> > > 
> > > Thanks,
> > > David Zhou
> > > 
> > > > -Daniel
> > > 
> > > _______________________________________________
> > > amd-gfx mailing list
> > > amd-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> > 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel