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

[zhoucm1]

On 2018年10月19日 17:20, zhoucm1 wrote:
>
>
> On 2018年10月19日 16:55, Daniel Vetter wrote:
>> 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 at 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?
> Sorry for bug, please.
In fact, the bug is already caught and fixed, just the fix part isn't in 
patch#1, but in patch#2:

Have you reverted? If not, I can send that fix in one minute.


Regards,
David Zhou
>>
>> Also, can we please have igts for this stuff so that intel-gfx-ci could
>> test this properly before it's all fireworks?
> Seems we cannot avoid igt now and vulkan CTS isn't enough, I will find 
> some time next week to lean IGT, looks v10 is need.
>
> Regards,
> David Zhou
>>
>> 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 at lists.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx