[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
Koenig, Christian
Christian.Koenig at amd.com
Mon Apr 1 09:10:03 UTC 2019
Am 01.04.19 um 11:05 schrieb Lionel Landwerlin:
> 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.
Well, I've got commit rights as well.
Going to remove the warning, add your rb and push everything if nobody
objects in the next hour or so.
Christian.
>
>
>>
>>>
>>> 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 amd-gfx
mailing list