[PATCH 1/2] drm: rename null fence to stub fence in syncobj

[zhoucm1@xxxxxxx (zhoucm1)]

On 2018å¹´08æ??22æ?¥ 17:05, Christian König wrote:
> Am 22.08.2018 um 11:02 schrieb zhoucm1:
>>
>>
>> On 2018å¹´08æ??22æ?¥ 16:52, Christian König wrote:
>>> Am 22.08.2018 um 10:38 schrieb Chunming Zhou:
>>>> stub fence will be used by timeline syncobj as well.
>>>>
>>>> Change-Id: Ia4252f03c07a8105491d2791dc7c8c6976682285
>>>> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
>>>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>>>> ---
>>>> Â  drivers/gpu/drm/drm_syncobj.c | 28 ++--------------------------
>>>>   include/drm/drm_syncobj.h     | 24 ++++++++++++++++++++++++
>>>> Â  2 files changed, 26 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>> index d4f4ce484529..70af32d0def1 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -187,39 +187,15 @@ void drm_syncobj_replace_fence(struct 
>>>> drm_syncobj *syncobj,
>>>> Â  }
>>>> Â  EXPORT_SYMBOL(drm_syncobj_replace_fence);
>>>> Â  -struct drm_syncobj_null_fence {
>>>> -Â Â Â  struct dma_fence base;
>>>> -Â Â Â  spinlock_t lock;
>>>> -};
>>>> -
>>>> -static const char *drm_syncobj_null_fence_get_name(struct 
>>>> dma_fence *fence)
>>>> -{
>>>> -Â Â Â Â Â Â Â  return "syncobjnull";
>>>> -}
>>>> -
>>>> -static bool drm_syncobj_null_fence_enable_signaling(struct 
>>>> dma_fence *fence)
>>>> -{
>>>> -Â Â Â  dma_fence_enable_sw_signaling(fence);
>>>> -Â Â Â  return !dma_fence_is_signaled(fence);
>>>> -}
>>>> -
>>>> -static const struct dma_fence_ops drm_syncobj_null_fence_ops = {
>>>> -Â Â Â  .get_driver_name = drm_syncobj_null_fence_get_name,
>>>> -Â Â Â  .get_timeline_name = drm_syncobj_null_fence_get_name,
>>>> -Â Â Â  .enable_signaling = drm_syncobj_null_fence_enable_signaling,
>>>> -Â Â Â  .wait = dma_fence_default_wait,
>>>> -Â Â Â  .release = NULL,
>>>> -};
>>>> -
>>>> Â  static int drm_syncobj_assign_null_handle(struct drm_syncobj 
>>>> *syncobj)
>>>> Â  {
>>>> -Â Â Â  struct drm_syncobj_null_fence *fence;
>>>> +Â Â Â  struct drm_syncobj_stub_fence *fence;
>>>> Â Â Â Â Â  fence = kzalloc(sizeof(*fence), GFP_KERNEL);
>>>> Â Â Â Â Â  if (fence == NULL)
>>>> Â Â Â Â Â Â Â Â Â  return -ENOMEM;
>>>> Â  Â Â Â Â Â  spin_lock_init(&fence->lock);
>>>> -Â Â Â  dma_fence_init(&fence->base, &drm_syncobj_null_fence_ops,
>>>> +Â Â Â  dma_fence_init(&fence->base, &drm_syncobj_stub_fence_ops,
>>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  &fence->lock, 0, 0);
>>>> Â Â Â Â Â  dma_fence_signal(&fence->base);
>>>> Â  diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
>>>> index 3980602472c0..b04c492ddbb5 100644
>>>> --- a/include/drm/drm_syncobj.h
>>>> +++ b/include/drm/drm_syncobj.h
>>>> @@ -30,6 +30,30 @@
>>>> Â  Â  struct drm_syncobj_cb;
>>>> Â  +struct drm_syncobj_stub_fence {
>>>> +Â Â Â  struct dma_fence base;
>>>> +Â Â Â  spinlock_t lock;
>>>> +};
>>>> +
>>>> +const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence)
>>>> +{
>>>> +Â Â Â Â Â Â Â  return "syncobjstub";
>>>> +}
>>>> +
>>>> +bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence)
>>>> +{
>>>> +Â Â Â  dma_fence_enable_sw_signaling(fence);
>>>
>>> Copy&pasted from the old implementation, but that is certainly 
>>> totally nonsense.
>>>
>>> dma_fence_enable_sw_signaling() is the function who is calling this 
>>> callback.
>> indeed, will fix.
>>>
>>>> +Â Â Â  return !dma_fence_is_signaled(fence);
>>>> +}
>>>> +
>>>> +const struct dma_fence_ops drm_syncobj_stub_fence_ops = {
>>>> +Â Â Â  .get_driver_name = drm_syncobj_stub_fence_get_name,
>>>> +Â Â Â  .get_timeline_name = drm_syncobj_stub_fence_get_name,
>>>> +Â Â Â  .enable_signaling = drm_syncobj_stub_fence_enable_signaling,
>>>> +Â Â Â  .wait = dma_fence_default_wait,
>>>
>>> The .wait callback should be dropped.
>> why?
>>
>> fence->ops->wait(fence, intr, timeout) is called by dma_fence_wait(). 
>> If dropped, how does dma_fence_wait() work?
>
> You are working on an older code base, fence->ops->wait is optional by 
> now.
Sorry, I still don't get it. My code is synced today from 
amd-staging-drm-next, and it's 4.18-rc1.
I still see the dma_fence_wait_timeout is :
signed long
dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long 
timeout)
{
 Â Â Â Â Â Â Â  signed long ret;

 Â Â Â Â Â Â Â  if (WARN_ON(timeout < 0))
 Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return -EINVAL;

 Â Â Â Â Â Â Â  trace_dma_fence_wait_start(fence);
 Â Â Â Â Â Â Â  ret = fence->ops->wait(fence, intr, timeout);
 Â Â Â Â Â Â Â  trace_dma_fence_wait_end(fence);
 Â Â Â Â Â Â Â  return ret;
}

.wait callback seems still a must have?

Thanks,
David Zhou


>
> Christian.
>
>>
>> Thanks,
>> David
>>>
>>> Apart from that looks good to me.
>>>
>>> Christian.
>>>
>>>> +Â Â Â  .release = NULL,
>>>> +};
>>>> +
>>>> Â  /**
>>>> Â Â  * struct drm_syncobj - sync object.
>>>> Â Â  *
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20180822/7c3b8c97/attachment-0001.html>