Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
On Wed, Aug 22, 2018 at 05:56:10PM +0800, zhoucm1 wrote: > > > On 2018年08月22日 17:34, Daniel Vetter wrote: > > On Wed, Aug 22, 2018 at 04:38:56PM +0800, Chunming Zhou wrote: > > > stub fence will be used by timeline syncobj as well. > > > > > > Change-Id: Ia4252f03c07a8105491d2791dc7c8c6976682285 > > > Signed-off-by: Chunming Zhou > > > Cc: Jason Ekstrand > > Please don't expose stuff only used by the drm_syncobj implementation to > > drivers. Gives us a very unclean driver interface. Imo this should all be > > left within drm_syncobj.h. > .c? will fix that. Yup I meant to leave it all in drm_syncobj.c :-) -Daniel > > > > See also my comments for patch 2, you leak all the implemenation details > > to drivers. We need to fix that and have a clear interface. > Yes, I will address them when I do v2. > > Thanks, > David Zhou > > -Daniel > > > > > --- > > > 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(>lock); > > > - dma_fence_init(>base, _syncobj_null_fence_ops, > > > + dma_fence_init(>base, _syncobj_stub_fence_ops, > > > >lock, 0, 0); > > > dma_fence_signal(>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); > > > +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, > > > + .release = NULL, > > > +}; > > > + > > > /** > > >* struct drm_syncobj - sync object. > > >* > > > -- > > > 2.14.1 > > > > > > ___ > > > dri-devel mailing list > > > dri-de...@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
On 2018年08月22日 17:34, Daniel Vetter wrote: On Wed, Aug 22, 2018 at 04:38:56PM +0800, Chunming Zhou wrote: stub fence will be used by timeline syncobj as well. Change-Id: Ia4252f03c07a8105491d2791dc7c8c6976682285 Signed-off-by: Chunming Zhou Cc: Jason Ekstrand Please don't expose stuff only used by the drm_syncobj implementation to drivers. Gives us a very unclean driver interface. Imo this should all be left within drm_syncobj.h. .c? will fix that. See also my comments for patch 2, you leak all the implemenation details to drivers. We need to fix that and have a clear interface. Yes, I will address them when I do v2. Thanks, David Zhou -Daniel --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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); +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, + .release = NULL, +}; + /** * struct drm_syncobj - sync object. * -- 2.14.1 ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
On Wed, Aug 22, 2018 at 04:38:56PM +0800, Chunming Zhou wrote: > stub fence will be used by timeline syncobj as well. > > Change-Id: Ia4252f03c07a8105491d2791dc7c8c6976682285 > Signed-off-by: Chunming Zhou > Cc: Jason Ekstrand Please don't expose stuff only used by the drm_syncobj implementation to drivers. Gives us a very unclean driver interface. Imo this should all be left within drm_syncobj.h. See also my comments for patch 2, you leak all the implemenation details to drivers. We need to fix that and have a clear interface. -Daniel > --- > 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(>lock); > - dma_fence_init(>base, _syncobj_null_fence_ops, > + dma_fence_init(>base, _syncobj_stub_fence_ops, > >lock, 0, 0); > dma_fence_signal(>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); > +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, > + .release = NULL, > +}; > + > /** > * struct drm_syncobj - sync object. > * > -- > 2.14.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
Am 22.08.2018 um 11:10 schrieb 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 Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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 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. That is outdated, when working on common drm stuff you need to work on drm-next or drm-misc-next. 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? See drm-misc-next: trace_dma_fence_wait_start(fence); if (fence->ops->wait) ret = fence->ops->wait(fence, intr, timeout); else ret = dma_fence_default_wait(fence, intr, timeout); trace_dma_fence_wait_end(fence); Regards, Christian. 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
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 Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
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 Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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 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. Christian. Thanks, David Apart from that looks good to me. Christian. + .release = NULL, +}; + /** * struct drm_syncobj - sync object. * ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
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 Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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 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? Thanks, David Apart from that looks good to me. Christian. + .release = NULL, +}; + /** * struct drm_syncobj - sync object. * ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
On Wed, Aug 22, 2018 at 10:52:16AM +0200, 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 > > Cc: Jason Ekstrand > > --- > > 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(>lock); > > - dma_fence_init(>base, _syncobj_null_fence_ops, > > + dma_fence_init(>base, _syncobj_stub_fence_ops, > >>lock, 0, 0); > > dma_fence_signal(>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 from the old implementation, but that is certainly totally > nonsense. > > dma_fence_enable_sw_signaling() is the function who is calling this > callback. > > > +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. > > Apart from that looks good to me. .enable_signaling should probably also be dropped. Same also for wherever you copied this from. -Daniel > > Christian. > > > + .release = NULL, > > +}; > > + > > /** > >* struct drm_syncobj - sync object. > >* > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm: rename null fence to stub fence in syncobj
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 Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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 from the old implementation, but that is certainly totally nonsense. dma_fence_enable_sw_signaling() is the function who is calling this callback. +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. Apart from that looks good to me. Christian. + .release = NULL, +}; + /** * struct drm_syncobj - sync object. * ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/2] drm: rename null fence to stub fence in syncobj
stub fence will be used by timeline syncobj as well. Change-Id: Ia4252f03c07a8105491d2791dc7c8c6976682285 Signed-off-by: Chunming Zhou Cc: Jason Ekstrand --- 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(>lock); - dma_fence_init(>base, _syncobj_null_fence_ops, + dma_fence_init(>base, _syncobj_stub_fence_ops, >lock, 0, 0); dma_fence_signal(>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); +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, + .release = NULL, +}; + /** * struct drm_syncobj - sync object. * -- 2.14.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx