[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Mon Apr 1 09:05:45 UTC 2019
On 01/04/2019 11:50, zhoucm1 wrote:
>
>
> On 2019年04月01日 16:19, Lionel Landwerlin wrote:
>> 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.
> Does that mean we can add your RB on patch set so that we can submit
> the patch set to drm-misc-next branch?
Yes :
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
Not too sure about the process for drm-misc-next. Sounds like Sumit
Semwal <sumit.semwal at linaro.org> is the go to person according to
MAINTAINERS.
>
>>
>> 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.
> Yes, agree.
>
> Thanks for your effort as well,
> -David
Thanks to you!
-Lionel
>>
>>
>> -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,
>>>>>>
>>>>>
>>
>
>
More information about the dri-devel
mailing list