| Message ID | 20180919195544.1511-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/40] drm: Use default dma_fence hooks where possible for null syncobj | expand |
On 19/09/2018 20:55, Chris Wilson wrote: > Both the .enable_signaling and .release of the null syncobj fence > can be replaced by the default callbacks for a small reduction in code > size. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/drm_syncobj.c | 11 ----------- > 1 file changed, 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 497729202bfe..e254f97fed7d 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) > return "syncobjstub"; > } > > -static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence) > -{ > - return !dma_fence_is_signaled(fence); > -} > - > -static void drm_syncobj_stub_fence_release(struct dma_fence *f) > -{ > - kfree(f); > -} With the default implementation this becomes kfree_rcu - so theoretically a change in behaviour after all. Since there are RCU usages in syncobj and the magical null/stub handle is not explained (or I did not find it), was the code fine with a plain kfree? Regards, Tvrtko > static 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, > - .release = drm_syncobj_stub_fence_release, > }; > > >
Quoting Tvrtko Ursulin (2018-09-20 14:34:29) > > On 19/09/2018 20:55, Chris Wilson wrote: > > Both the .enable_signaling and .release of the null syncobj fence > > can be replaced by the default callbacks for a small reduction in code > > size. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/drm_syncobj.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > > index 497729202bfe..e254f97fed7d 100644 > > --- a/drivers/gpu/drm/drm_syncobj.c > > +++ b/drivers/gpu/drm/drm_syncobj.c > > @@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) > > return "syncobjstub"; > > } > > > > -static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence) > > -{ > > - return !dma_fence_is_signaled(fence); > > -} > > - > > -static void drm_syncobj_stub_fence_release(struct dma_fence *f) > > -{ > > - kfree(f); > > -} > > With the default implementation this becomes kfree_rcu - so > theoretically a change in behaviour after all. A correction, since dma_fence are required to be RCU safe. > Since there are RCU usages in syncobj and the magical null/stub handle > is not explained (or I did not find it), was the code fine with a plain > kfree? No :) It's the third parties who may be doing RCU lookups, the only argument would be that it would never be exposed... Except it was. -Chris
On 20/09/2018 14:40, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2018-09-20 14:34:29) >> >> On 19/09/2018 20:55, Chris Wilson wrote: >>> Both the .enable_signaling and .release of the null syncobj fence >>> can be replaced by the default callbacks for a small reduction in code >>> size. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/drm_syncobj.c | 11 ----------- >>> 1 file changed, 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c >>> index 497729202bfe..e254f97fed7d 100644 >>> --- a/drivers/gpu/drm/drm_syncobj.c >>> +++ b/drivers/gpu/drm/drm_syncobj.c >>> @@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) >>> return "syncobjstub"; >>> } >>> >>> -static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence) >>> -{ >>> - return !dma_fence_is_signaled(fence); >>> -} >>> - >>> -static void drm_syncobj_stub_fence_release(struct dma_fence *f) >>> -{ >>> - kfree(f); >>> -} >> >> With the default implementation this becomes kfree_rcu - so >> theoretically a change in behaviour after all. > > A correction, since dma_fence are required to be RCU safe. > >> Since there are RCU usages in syncobj and the magical null/stub handle >> is not explained (or I did not find it), was the code fine with a plain >> kfree? > > No :) It's the third parties who may be doing RCU lookups, the only > argument would be that it would never be exposed... Except it was. Update the commit then to reflect the bug fixing nature and: Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Add Fixes: perhaps.. e28bd10? Claims to be renaming, but it did add the kfree release callback as well. Regards, Tvrtko
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 497729202bfe..e254f97fed7d 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -66,20 +66,9 @@ static const char *drm_syncobj_stub_fence_get_name(struct dma_fence *fence) return "syncobjstub"; } -static bool drm_syncobj_stub_fence_enable_signaling(struct dma_fence *fence) -{ - return !dma_fence_is_signaled(fence); -} - -static void drm_syncobj_stub_fence_release(struct dma_fence *f) -{ - kfree(f); -} static 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, - .release = drm_syncobj_stub_fence_release, };
Both the .enable_signaling and .release of the null syncobj fence can be replaced by the default callbacks for a small reduction in code size. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_syncobj.c | 11 ----------- 1 file changed, 11 deletions(-)