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

From: "Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: zhoucm1 <zhoucm1-5C7GfCeVMHo@public.gmane.org>,
	"Daniel Vetter" <daniel-/w4YWyX8dFk@public.gmane.org>,
	"Christian König" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
Cc: Chunming Zhou <David1.Zhou-5C7GfCeVMHo@public.gmane.org>,
	amd-gfx list
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	"Rakos, Daniel" <Daniel.Rakos-5C7GfCeVMHo@public.gmane.org>,
	dri-devel
	<dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>,
	Bas Nieuwenhuizen
	<bas-dldO88ZXqoXqqjsSq9zF6IRWq/SkRNHw@public.gmane.org>,
	Dave Airlie <airlied-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Subject: Re: [PATCH 2/7] drm: add syncobj timeline support v8
Date: Thu, 18 Oct 2018 13:50:18 +0200	[thread overview]
Message-ID: <bdb5bf27-b543-e308-a121-6bae35854e95@gmail.com> (raw)
In-Reply-To: <b08b1f23-d8dc-a294-2176-5125075c9acc-5C7GfCeVMHo@public.gmane.org>

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

The rest in the series looks good to me as well, but I certainly want 
the radv/anv developers to take a look as well as Daniel suggested.

Christian.

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