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

["Michel Dänzer" <michel@daenzer.net>]

On 2019-03-28 4:18 p.m., 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.

How about DRM_WARN_ONCE ?


-- 
Earthling Michel Dänzer               |              https://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel