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

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

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@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@amd.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@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.


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,
>>
>>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel