[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4

[Lionel Landwerlin]

On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
>
>> -----Original Message-----
>> From: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Sent: Saturday, March 30, 2019 10:09 PM
>> To: Koenig, Christian <Christian.Koenig at amd.com>; Zhou, David(ChunMing)
>> <David1.Zhou at amd.com>; dri-devel at lists.freedesktop.org; amd-
>> gfx at lists.freedesktop.org; jason at jlekstrand.net; Hector, Tobias
>> <Tobias.Hector at amd.com>
>> Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
>> interface v4
>>
>> On 28/03/2019 15:18, Christian König wrote:
>>> Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
>>>> On 25/03/2019 08:32, Chunming Zhou wrote:
>>>>> From: Christian König <ckoenig.leichtzumerken at gmail.com>
>>>>>
>>>>> Use the dma_fence_chain object to create a timeline of fence objects
>>>>> instead of just replacing the existing fence.
>>>>>
>>>>> v2: rebase and cleanup
>>>>> v3: fix garbage collection parameters
>>>>> v4: add unorder point check, print a warn calltrace
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/drm_syncobj.c | 39
>>>>> +++++++++++++++++++++++++++++++++++
>>>>>    include/drm/drm_syncobj.h     |  5 +++++
>>>>>    2 files changed, 44 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>>> b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
>>>>> 100644
>>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>>> @@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
>>>>> drm_syncobj *syncobj,
>>>>>        spin_unlock(&syncobj->lock);
>>>>>    }
>>>>>    +/**
>>>>> + * drm_syncobj_add_point - add new timeline point to the syncobj
>>>>> + * @syncobj: sync object to add timeline point do
>>>>> + * @chain: chain node to use to add the point
>>>>> + * @fence: fence to encapsulate in the chain node
>>>>> + * @point: sequence number to use for the point
>>>>> + *
>>>>> + * Add the chain node as new timeline point to the syncobj.
>>>>> + */
>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>> +               struct dma_fence_chain *chain,
>>>>> +               struct dma_fence *fence,
>>>>> +               uint64_t point)
>>>>> +{
>>>>> +    struct syncobj_wait_entry *cur, *tmp;
>>>>> +    struct dma_fence *prev;
>>>>> +
>>>>> +    dma_fence_get(fence);
>>>>> +
>>>>> +    spin_lock(&syncobj->lock);
>>>>> +
>>>>> +    prev = drm_syncobj_fence_get(syncobj);
>>>>> +    /* You are adding an unorder point to timeline, which could
>>>>> cause payload returned from query_ioctl is 0! */
>>>>> +    WARN_ON_ONCE(prev && prev->seqno >= point);
>>>>
>>>> I think the WARN/BUG macros should only fire when there is an issue
>>>> with programming from within the kernel.
>>>>
>>>> But this particular warning can be triggered by an application.
>>>>
>>>>
>>>> Probably best to just remove it?
>>> Yeah, that was also my argument against it.
>>>
>>> Key point here is that we still want to note somehow that userspace
>>> did something wrong and returning an error is not an option.
>>>
>>> Maybe just use DRM_ERROR with a static variable to print the message
>>> only once.
>>>
>>> Christian.
>> I don't really see any point in printing an error once. If you run your
>> application twice you end up thinking there was an issue just on the first run
>> but it's actually always wrong.
>>
> Except this nitpick, is there any other concern to push whole patch set? Is that time to push whole patch set?
>
> -David


Looks good to me.

I have an additional change to make drm_syncobj_find_fence() also return 
the drm_syncobj : 
https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e

This is needed in i915 to avoid looking up the drm_syncobj handle twice.

Our driver allows to wait on the syncobj's dma_fence that we're then 
going to replace so we need to get bot the fence & syncobj at the same time.

I guess it can go in a follow up series.


-Lionel


>
>> Unless we're willing to take the syncobj lock for longer periods of time when
>> adding points, I guess we'll have to defer validation to validation layers.
>>
>>
>> -Lionel
>>
>>>>
>>>> -Lionel
>>>>
>>>>
>>>>> +    dma_fence_chain_init(chain, prev, fence, point);
>>>>> +    rcu_assign_pointer(syncobj->fence, &chain->base);
>>>>> +
>>>>> +    list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
>>>>> +        list_del_init(&cur->node);
>>>>> +        syncobj_wait_syncobj_func(syncobj, cur);
>>>>> +    }
>>>>> +    spin_unlock(&syncobj->lock);
>>>>> +
>>>>> +    /* Walk the chain once to trigger garbage collection */
>>>>> +    dma_fence_chain_for_each(fence, prev);
>>>>> +    dma_fence_put(prev);
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_syncobj_add_point);
>>>>> +
>>>>>    /**
>>>>>     * drm_syncobj_replace_fence - replace fence in a sync object.
>>>>>     * @syncobj: Sync object to replace fence in diff --git
>>>>> a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
>>>>> 0311c9fdbd2f..6cf7243a1dc5 100644
>>>>> --- a/include/drm/drm_syncobj.h
>>>>> +++ b/include/drm/drm_syncobj.h
>>>>> @@ -27,6 +27,7 @@
>>>>>    #define __DRM_SYNCOBJ_H__
>>>>>      #include <linux/dma-fence.h>
>>>>> +#include <linux/dma-fence-chain.h>
>>>>>      struct drm_file;
>>>>>    @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
>>>>> *syncobj)
>>>>>      struct drm_syncobj *drm_syncobj_find(struct drm_file
>>>>> *file_private,
>>>>>                         u32 handle);
>>>>> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
>>>>> +               struct dma_fence_chain *chain,
>>>>> +               struct dma_fence *fence,
>>>>> +               uint64_t point);
>>>>>    void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>>>>>                       struct dma_fence *fence);
>>>>>    int drm_syncobj_find_fence(struct drm_file *file_private,
>>>>
>>>