[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
zhoucm1
zhoucm1 at amd.com
Mon Apr 1 08:50:12 UTC 2019
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?
>
> 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
>
>
> -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 amd-gfx
mailing list