On Fri, Jun 11, 2021 at 02:02:59PM +0200, Christian König wrote:
> Add a common allocation helper. Cleaning up the mix of kzalloc/kmalloc
> and some unused code in the selftest.
>
> v2: polish kernel doc a bit
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Given how absolutely wrong I was I'm not sure this r-b here is justified
:-)
> ---
> 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 | 23 +++++++++++++++++++
> 6 files changed, 36 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 c6eb3aa45668..7ec36d850363 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
> @@ -66,6 +67,28 @@ 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.
struct dma_fence_chain for that hyperlink goodness
> + */
> +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. This doesn't need
Same here.
> + * an RCU grace period since the fence was never initialized nor published.
I'd add even more clarification, like:
"After dma_fence_chain_init() has been called the fence must be released
by calling dma_fence_put(), and not through this function."
That's still a notch too strict (in theory as long as the fence isn't
published anywhere it's all fine), but it keeps the door open for some
validation.
With the doc polish:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> + */
> +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
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
