| Message ID | 20210610091800.1833-3-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] dma-buf: some dma_fence_chain improvements | expand |
On Thu, Jun 10, 2021 at 11:17:55AM +0200, Christian König wrote: > Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc > and some unused code in the selftest. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/st-dma-fence-chain.c | 16 ++++---------- > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- > drivers/gpu/drm/drm_syncobj.c | 6 ++--- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++--- > drivers/gpu/drm/msm/msm_gem_submit.c | 6 ++--- > include/linux/dma-fence-chain.h | 22 +++++++++++++++++++ > 6 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c > index 9525f7f56119..8ce1ea59d31b 100644 > --- a/drivers/dma-buf/st-dma-fence-chain.c > +++ b/drivers/dma-buf/st-dma-fence-chain.c > @@ -58,28 +58,20 @@ static struct dma_fence *mock_fence(void) > return &f->base; > } > > -static inline struct mock_chain { > - struct dma_fence_chain base; > -} *to_mock_chain(struct dma_fence *f) { > - return container_of(f, struct mock_chain, base.base); > -} > - > static struct dma_fence *mock_chain(struct dma_fence *prev, > struct dma_fence *fence, > u64 seqno) > { > - struct mock_chain *f; > + struct dma_fence_chain *f; > > - f = kmalloc(sizeof(*f), GFP_KERNEL); > + f = dma_fence_chain_alloc(); > if (!f) > return NULL; > > - dma_fence_chain_init(&f->base, > - dma_fence_get(prev), > - dma_fence_get(fence), > + dma_fence_chain_init(f, dma_fence_get(prev), dma_fence_get(fence), > seqno); > > - return &f->base.base; > + return &f->base; > } > > static int sanitycheck(void *arg) > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 90136f9dedd6..325e82621467 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -1124,7 +1124,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p > > dep->chain = NULL; > if (syncobj_deps[i].point) { > - dep->chain = kmalloc(sizeof(*dep->chain), GFP_KERNEL); > + dep->chain = dma_fence_chain_alloc(); > if (!dep->chain) > return -ENOMEM; > } > @@ -1132,7 +1132,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p > dep->syncobj = drm_syncobj_find(p->filp, > syncobj_deps[i].handle); > if (!dep->syncobj) { > - kfree(dep->chain); > + dma_fence_chain_free(dep->chain); > return -EINVAL; > } > dep->point = syncobj_deps[i].point; > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index fdd2ec87cdd1..1c5b9ef6da37 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -861,7 +861,7 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, > &fence); > if (ret) > goto err; > - chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > + chain = dma_fence_chain_alloc(); > if (!chain) { > ret = -ENOMEM; > goto err1; > @@ -1402,10 +1402,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, > goto err_points; > } > for (i = 0; i < args->count_handles; i++) { > - chains[i] = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > + chains[i] = dma_fence_chain_alloc(); > if (!chains[i]) { > for (j = 0; j < i; j++) > - kfree(chains[j]); > + dma_fence_chain_free(chains[j]); > ret = -ENOMEM; > goto err_chains; > } > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 66789111a24b..a22cb86730b3 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -2983,7 +2983,7 @@ __free_fence_array(struct eb_fence *fences, unsigned int n) > while (n--) { > drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2)); > dma_fence_put(fences[n].dma_fence); > - kfree(fences[n].chain_fence); > + dma_fence_chain_free(fences[n].chain_fence); > } > kvfree(fences); > } > @@ -3097,9 +3097,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb, > return -EINVAL; > } > > - f->chain_fence = > - kmalloc(sizeof(*f->chain_fence), > - GFP_KERNEL); > + f->chain_fence = dma_fence_chain_alloc(); > if (!f->chain_fence) { > drm_syncobj_put(syncobj); > dma_fence_put(fence); > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > index 5480852bdeda..6ff6df6c4791 100644 > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > @@ -586,9 +586,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, > break; > } > > - post_deps[i].chain = > - kmalloc(sizeof(*post_deps[i].chain), > - GFP_KERNEL); > + post_deps[i].chain = dma_fence_chain_alloc(); > if (!post_deps[i].chain) { > ret = -ENOMEM; > break; > @@ -605,7 +603,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, > > if (ret) { > for (j = 0; j <= i; ++j) { > - kfree(post_deps[j].chain); > + dma_fence_chain_free(post_deps[j].chain); > if (post_deps[j].syncobj) > drm_syncobj_put(post_deps[j].syncobj); > } > diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h > index 9d6a062be640..5f45689a6d2e 100644 > --- a/include/linux/dma-fence-chain.h > +++ b/include/linux/dma-fence-chain.h > @@ -12,6 +12,7 @@ > > #include <linux/dma-fence.h> > #include <linux/irq_work.h> > +#include <linux/slab.h> > > /** > * struct dma_fence_chain - fence to represent an node of a fence chain > @@ -53,6 +54,27 @@ to_dma_fence_chain(struct dma_fence *fence) > return container_of(fence, struct dma_fence_chain, base); > } > > +/** > + * dma_fence_chain_alloc > + * > + * Returns a new dma_fence_chain object ... or NULL on failure. > + */ > +static inline struct dma_fence_chain *dma_fence_chain_alloc(void) > +{ > + return kmalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > +}; > + > +/** > + * dma_fence_chain_free > + * @chain: chain node to free > + * > + * Frees up an allocated but not used dma_fence_chain node. > + */ > +static inline void dma_fence_chain_free(struct dma_fence_chain *chain) > +{ > + kfree(chain); kfree_rcu, and I guess this means this patch here should be cc: stable. This is kinda why I'm questioning whether this "dma_fence are protected by rcu" cross driver api is really a good idea. We largely get it wrong in the details in a _ton_ of places. With the details fixed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + > /** > * dma_fence_chain_for_each - iterate over all fences in chain > * @iter: current fence > -- > 2.25.1 >
Am 11.06.21 um 09:54 schrieb Daniel Vetter: > On Thu, Jun 10, 2021 at 11:17:55AM +0200, Christian König wrote: >> Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc >> and some unused code in the selftest. >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/dma-buf/st-dma-fence-chain.c | 16 ++++---------- >> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- >> drivers/gpu/drm/drm_syncobj.c | 6 ++--- >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++--- >> drivers/gpu/drm/msm/msm_gem_submit.c | 6 ++--- >> include/linux/dma-fence-chain.h | 22 +++++++++++++++++++ >> 6 files changed, 35 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c >> index 9525f7f56119..8ce1ea59d31b 100644 >> --- a/drivers/dma-buf/st-dma-fence-chain.c >> +++ b/drivers/dma-buf/st-dma-fence-chain.c >> @@ -58,28 +58,20 @@ static struct dma_fence *mock_fence(void) >> return &f->base; >> } >> >> -static inline struct mock_chain { >> - struct dma_fence_chain base; >> -} *to_mock_chain(struct dma_fence *f) { >> - return container_of(f, struct mock_chain, base.base); >> -} >> - >> static struct dma_fence *mock_chain(struct dma_fence *prev, >> struct dma_fence *fence, >> u64 seqno) >> { >> - struct mock_chain *f; >> + struct dma_fence_chain *f; >> >> - f = kmalloc(sizeof(*f), GFP_KERNEL); >> + f = dma_fence_chain_alloc(); >> if (!f) >> return NULL; >> >> - dma_fence_chain_init(&f->base, >> - dma_fence_get(prev), >> - dma_fence_get(fence), >> + dma_fence_chain_init(f, dma_fence_get(prev), dma_fence_get(fence), >> seqno); >> >> - return &f->base.base; >> + return &f->base; >> } >> >> static int sanitycheck(void *arg) >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> index 90136f9dedd6..325e82621467 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c >> @@ -1124,7 +1124,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p >> >> dep->chain = NULL; >> if (syncobj_deps[i].point) { >> - dep->chain = kmalloc(sizeof(*dep->chain), GFP_KERNEL); >> + dep->chain = dma_fence_chain_alloc(); >> if (!dep->chain) >> return -ENOMEM; >> } >> @@ -1132,7 +1132,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p >> dep->syncobj = drm_syncobj_find(p->filp, >> syncobj_deps[i].handle); >> if (!dep->syncobj) { >> - kfree(dep->chain); >> + dma_fence_chain_free(dep->chain); >> return -EINVAL; >> } >> dep->point = syncobj_deps[i].point; >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c >> index fdd2ec87cdd1..1c5b9ef6da37 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -861,7 +861,7 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, >> &fence); >> if (ret) >> goto err; >> - chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); >> + chain = dma_fence_chain_alloc(); >> if (!chain) { >> ret = -ENOMEM; >> goto err1; >> @@ -1402,10 +1402,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, >> goto err_points; >> } >> for (i = 0; i < args->count_handles; i++) { >> - chains[i] = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); >> + chains[i] = dma_fence_chain_alloc(); >> if (!chains[i]) { >> for (j = 0; j < i; j++) >> - kfree(chains[j]); >> + dma_fence_chain_free(chains[j]); >> ret = -ENOMEM; >> goto err_chains; >> } >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> index 66789111a24b..a22cb86730b3 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> @@ -2983,7 +2983,7 @@ __free_fence_array(struct eb_fence *fences, unsigned int n) >> while (n--) { >> drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2)); >> dma_fence_put(fences[n].dma_fence); >> - kfree(fences[n].chain_fence); >> + dma_fence_chain_free(fences[n].chain_fence); >> } >> kvfree(fences); >> } >> @@ -3097,9 +3097,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb, >> return -EINVAL; >> } >> >> - f->chain_fence = >> - kmalloc(sizeof(*f->chain_fence), >> - GFP_KERNEL); >> + f->chain_fence = dma_fence_chain_alloc(); >> if (!f->chain_fence) { >> drm_syncobj_put(syncobj); >> dma_fence_put(fence); >> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c >> index 5480852bdeda..6ff6df6c4791 100644 >> --- a/drivers/gpu/drm/msm/msm_gem_submit.c >> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c >> @@ -586,9 +586,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, >> break; >> } >> >> - post_deps[i].chain = >> - kmalloc(sizeof(*post_deps[i].chain), >> - GFP_KERNEL); >> + post_deps[i].chain = dma_fence_chain_alloc(); >> if (!post_deps[i].chain) { >> ret = -ENOMEM; >> break; >> @@ -605,7 +603,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, >> >> if (ret) { >> for (j = 0; j <= i; ++j) { >> - kfree(post_deps[j].chain); >> + dma_fence_chain_free(post_deps[j].chain); >> if (post_deps[j].syncobj) >> drm_syncobj_put(post_deps[j].syncobj); >> } >> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h >> index 9d6a062be640..5f45689a6d2e 100644 >> --- a/include/linux/dma-fence-chain.h >> +++ b/include/linux/dma-fence-chain.h >> @@ -12,6 +12,7 @@ >> >> #include <linux/dma-fence.h> >> #include <linux/irq_work.h> >> +#include <linux/slab.h> >> >> /** >> * struct dma_fence_chain - fence to represent an node of a fence chain >> @@ -53,6 +54,27 @@ to_dma_fence_chain(struct dma_fence *fence) >> return container_of(fence, struct dma_fence_chain, base); >> } >> >> +/** >> + * dma_fence_chain_alloc >> + * >> + * Returns a new dma_fence_chain object > ... or NULL on failure. > >> + */ >> +static inline struct dma_fence_chain *dma_fence_chain_alloc(void) >> +{ >> + return kmalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); >> +}; >> + >> +/** >> + * dma_fence_chain_free >> + * @chain: chain node to free >> + * >> + * Frees up an allocated but not used dma_fence_chain node. >> + */ >> +static inline void dma_fence_chain_free(struct dma_fence_chain *chain) >> +{ >> + kfree(chain); > kfree_rcu, and I guess this means this patch here should be cc: stable. Nope, kfree() is correct here. This is to free up fences which never been initialized, so an RCU grace period isn't necessary because nobody could potentially have a reference. Christian. > This is kinda why I'm questioning whether this "dma_fence are protected by > rcu" cross driver api is really a good idea. We largely get it wrong in > the details in a _ton_ of places. > > With the details fixed: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> + >> /** >> * dma_fence_chain_for_each - iterate over all fences in chain >> * @iter: current fence >> -- >> 2.25.1 >>
On Fri, Jun 11, 2021 at 1:48 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 11.06.21 um 09:54 schrieb Daniel Vetter: > > On Thu, Jun 10, 2021 at 11:17:55AM +0200, Christian König wrote: > >> Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc > >> and some unused code in the selftest. > >> > >> Signed-off-by: Christian König <christian.koenig@amd.com> > >> --- > >> drivers/dma-buf/st-dma-fence-chain.c | 16 ++++---------- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- > >> drivers/gpu/drm/drm_syncobj.c | 6 ++--- > >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++--- > >> drivers/gpu/drm/msm/msm_gem_submit.c | 6 ++--- > >> include/linux/dma-fence-chain.h | 22 +++++++++++++++++++ > >> 6 files changed, 35 insertions(+), 25 deletions(-) > >> > >> diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c > >> index 9525f7f56119..8ce1ea59d31b 100644 > >> --- a/drivers/dma-buf/st-dma-fence-chain.c > >> +++ b/drivers/dma-buf/st-dma-fence-chain.c > >> @@ -58,28 +58,20 @@ static struct dma_fence *mock_fence(void) > >> return &f->base; > >> } > >> > >> -static inline struct mock_chain { > >> - struct dma_fence_chain base; > >> -} *to_mock_chain(struct dma_fence *f) { > >> - return container_of(f, struct mock_chain, base.base); > >> -} > >> - > >> static struct dma_fence *mock_chain(struct dma_fence *prev, > >> struct dma_fence *fence, > >> u64 seqno) > >> { > >> - struct mock_chain *f; > >> + struct dma_fence_chain *f; > >> > >> - f = kmalloc(sizeof(*f), GFP_KERNEL); > >> + f = dma_fence_chain_alloc(); > >> if (!f) > >> return NULL; > >> > >> - dma_fence_chain_init(&f->base, > >> - dma_fence_get(prev), > >> - dma_fence_get(fence), > >> + dma_fence_chain_init(f, dma_fence_get(prev), dma_fence_get(fence), > >> seqno); > >> > >> - return &f->base.base; > >> + return &f->base; > >> } > >> > >> static int sanitycheck(void *arg) > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> index 90136f9dedd6..325e82621467 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > >> @@ -1124,7 +1124,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p > >> > >> dep->chain = NULL; > >> if (syncobj_deps[i].point) { > >> - dep->chain = kmalloc(sizeof(*dep->chain), GFP_KERNEL); > >> + dep->chain = dma_fence_chain_alloc(); > >> if (!dep->chain) > >> return -ENOMEM; > >> } > >> @@ -1132,7 +1132,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p > >> dep->syncobj = drm_syncobj_find(p->filp, > >> syncobj_deps[i].handle); > >> if (!dep->syncobj) { > >> - kfree(dep->chain); > >> + dma_fence_chain_free(dep->chain); > >> return -EINVAL; > >> } > >> dep->point = syncobj_deps[i].point; > >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > >> index fdd2ec87cdd1..1c5b9ef6da37 100644 > >> --- a/drivers/gpu/drm/drm_syncobj.c > >> +++ b/drivers/gpu/drm/drm_syncobj.c > >> @@ -861,7 +861,7 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, > >> &fence); > >> if (ret) > >> goto err; > >> - chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > >> + chain = dma_fence_chain_alloc(); > >> if (!chain) { > >> ret = -ENOMEM; > >> goto err1; > >> @@ -1402,10 +1402,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, > >> goto err_points; > >> } > >> for (i = 0; i < args->count_handles; i++) { > >> - chains[i] = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > >> + chains[i] = dma_fence_chain_alloc(); > >> if (!chains[i]) { > >> for (j = 0; j < i; j++) > >> - kfree(chains[j]); > >> + dma_fence_chain_free(chains[j]); > >> ret = -ENOMEM; > >> goto err_chains; > >> } > >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >> index 66789111a24b..a22cb86730b3 100644 > >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > >> @@ -2983,7 +2983,7 @@ __free_fence_array(struct eb_fence *fences, unsigned int n) > >> while (n--) { > >> drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2)); > >> dma_fence_put(fences[n].dma_fence); > >> - kfree(fences[n].chain_fence); > >> + dma_fence_chain_free(fences[n].chain_fence); > >> } > >> kvfree(fences); > >> } > >> @@ -3097,9 +3097,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb, > >> return -EINVAL; > >> } > >> > >> - f->chain_fence = > >> - kmalloc(sizeof(*f->chain_fence), > >> - GFP_KERNEL); > >> + f->chain_fence = dma_fence_chain_alloc(); > >> if (!f->chain_fence) { > >> drm_syncobj_put(syncobj); > >> dma_fence_put(fence); > >> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > >> index 5480852bdeda..6ff6df6c4791 100644 > >> --- a/drivers/gpu/drm/msm/msm_gem_submit.c > >> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > >> @@ -586,9 +586,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, > >> break; > >> } > >> > >> - post_deps[i].chain = > >> - kmalloc(sizeof(*post_deps[i].chain), > >> - GFP_KERNEL); > >> + post_deps[i].chain = dma_fence_chain_alloc(); > >> if (!post_deps[i].chain) { > >> ret = -ENOMEM; > >> break; > >> @@ -605,7 +603,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, > >> > >> if (ret) { > >> for (j = 0; j <= i; ++j) { > >> - kfree(post_deps[j].chain); > >> + dma_fence_chain_free(post_deps[j].chain); > >> if (post_deps[j].syncobj) > >> drm_syncobj_put(post_deps[j].syncobj); > >> } > >> diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h > >> index 9d6a062be640..5f45689a6d2e 100644 > >> --- a/include/linux/dma-fence-chain.h > >> +++ b/include/linux/dma-fence-chain.h > >> @@ -12,6 +12,7 @@ > >> > >> #include <linux/dma-fence.h> > >> #include <linux/irq_work.h> > >> +#include <linux/slab.h> > >> > >> /** > >> * struct dma_fence_chain - fence to represent an node of a fence chain > >> @@ -53,6 +54,27 @@ to_dma_fence_chain(struct dma_fence *fence) > >> return container_of(fence, struct dma_fence_chain, base); > >> } > >> > >> +/** > >> + * dma_fence_chain_alloc > >> + * > >> + * Returns a new dma_fence_chain object > > ... or NULL on failure. > > > >> + */ > >> +static inline struct dma_fence_chain *dma_fence_chain_alloc(void) > >> +{ > >> + return kmalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); > >> +}; > >> + > >> +/** > >> + * dma_fence_chain_free > >> + * @chain: chain node to free > >> + * > >> + * Frees up an allocated but not used dma_fence_chain node. > >> + */ > >> +static inline void dma_fence_chain_free(struct dma_fence_chain *chain) > >> +{ > >> + kfree(chain); > > kfree_rcu, and I guess this means this patch here should be cc: stable. > > Nope, kfree() is correct here. > > This is to free up fences which never been initialized, so an RCU grace > period isn't necessary because nobody could potentially have a reference. Hah I got tricked. I see you have the next revision out there already, I'll drop some suggestions on the kerneldoc so it's clearer what this does and why. -Daniel > Christian. > > > This is kinda why I'm questioning whether this "dma_fence are protected by > > rcu" cross driver api is really a good idea. We largely get it wrong in > > the details in a _ton_ of places. > > > > With the details fixed: > > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > >> + > >> /** > >> * dma_fence_chain_for_each - iterate over all fences in chain > >> * @iter: current fence > >> -- > >> 2.25.1 > >> >
diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c index 9525f7f56119..8ce1ea59d31b 100644 --- a/drivers/dma-buf/st-dma-fence-chain.c +++ b/drivers/dma-buf/st-dma-fence-chain.c @@ -58,28 +58,20 @@ static struct dma_fence *mock_fence(void) return &f->base; } -static inline struct mock_chain { - struct dma_fence_chain base; -} *to_mock_chain(struct dma_fence *f) { - return container_of(f, struct mock_chain, base.base); -} - static struct dma_fence *mock_chain(struct dma_fence *prev, struct dma_fence *fence, u64 seqno) { - struct mock_chain *f; + struct dma_fence_chain *f; - f = kmalloc(sizeof(*f), GFP_KERNEL); + f = dma_fence_chain_alloc(); if (!f) return NULL; - dma_fence_chain_init(&f->base, - dma_fence_get(prev), - dma_fence_get(fence), + dma_fence_chain_init(f, dma_fence_get(prev), dma_fence_get(fence), seqno); - return &f->base.base; + return &f->base; } static int sanitycheck(void *arg) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 90136f9dedd6..325e82621467 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1124,7 +1124,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p dep->chain = NULL; if (syncobj_deps[i].point) { - dep->chain = kmalloc(sizeof(*dep->chain), GFP_KERNEL); + dep->chain = dma_fence_chain_alloc(); if (!dep->chain) return -ENOMEM; } @@ -1132,7 +1132,7 @@ static int amdgpu_cs_process_syncobj_timeline_out_dep(struct amdgpu_cs_parser *p dep->syncobj = drm_syncobj_find(p->filp, syncobj_deps[i].handle); if (!dep->syncobj) { - kfree(dep->chain); + dma_fence_chain_free(dep->chain); return -EINVAL; } dep->point = syncobj_deps[i].point; diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index fdd2ec87cdd1..1c5b9ef6da37 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -861,7 +861,7 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, &fence); if (ret) goto err; - chain = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); + chain = dma_fence_chain_alloc(); if (!chain) { ret = -ENOMEM; goto err1; @@ -1402,10 +1402,10 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, goto err_points; } for (i = 0; i < args->count_handles; i++) { - chains[i] = kzalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); + chains[i] = dma_fence_chain_alloc(); if (!chains[i]) { for (j = 0; j < i; j++) - kfree(chains[j]); + dma_fence_chain_free(chains[j]); ret = -ENOMEM; goto err_chains; } diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 66789111a24b..a22cb86730b3 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2983,7 +2983,7 @@ __free_fence_array(struct eb_fence *fences, unsigned int n) while (n--) { drm_syncobj_put(ptr_mask_bits(fences[n].syncobj, 2)); dma_fence_put(fences[n].dma_fence); - kfree(fences[n].chain_fence); + dma_fence_chain_free(fences[n].chain_fence); } kvfree(fences); } @@ -3097,9 +3097,7 @@ add_timeline_fence_array(struct i915_execbuffer *eb, return -EINVAL; } - f->chain_fence = - kmalloc(sizeof(*f->chain_fence), - GFP_KERNEL); + f->chain_fence = dma_fence_chain_alloc(); if (!f->chain_fence) { drm_syncobj_put(syncobj); dma_fence_put(fence); diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 5480852bdeda..6ff6df6c4791 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -586,9 +586,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, break; } - post_deps[i].chain = - kmalloc(sizeof(*post_deps[i].chain), - GFP_KERNEL); + post_deps[i].chain = dma_fence_chain_alloc(); if (!post_deps[i].chain) { ret = -ENOMEM; break; @@ -605,7 +603,7 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev, if (ret) { for (j = 0; j <= i; ++j) { - kfree(post_deps[j].chain); + dma_fence_chain_free(post_deps[j].chain); if (post_deps[j].syncobj) drm_syncobj_put(post_deps[j].syncobj); } diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h index 9d6a062be640..5f45689a6d2e 100644 --- a/include/linux/dma-fence-chain.h +++ b/include/linux/dma-fence-chain.h @@ -12,6 +12,7 @@ #include <linux/dma-fence.h> #include <linux/irq_work.h> +#include <linux/slab.h> /** * struct dma_fence_chain - fence to represent an node of a fence chain @@ -53,6 +54,27 @@ to_dma_fence_chain(struct dma_fence *fence) return container_of(fence, struct dma_fence_chain, base); } +/** + * dma_fence_chain_alloc + * + * Returns a new dma_fence_chain object + */ +static inline struct dma_fence_chain *dma_fence_chain_alloc(void) +{ + return kmalloc(sizeof(struct dma_fence_chain), GFP_KERNEL); +}; + +/** + * dma_fence_chain_free + * @chain: chain node to free + * + * Frees up an allocated but not used dma_fence_chain node. + */ +static inline void dma_fence_chain_free(struct dma_fence_chain *chain) +{ + kfree(chain); +}; + /** * dma_fence_chain_for_each - iterate over all fences in chain * @iter: current fence
Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc and some unused code in the selftest. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/st-dma-fence-chain.c | 16 ++++---------- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 4 ++-- drivers/gpu/drm/drm_syncobj.c | 6 ++--- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 6 ++--- drivers/gpu/drm/msm/msm_gem_submit.c | 6 ++--- include/linux/dma-fence-chain.h | 22 +++++++++++++++++++ 6 files changed, 35 insertions(+), 25 deletions(-)