Re: [PATCH 3/5] dma-buf: add dma_fence_chain_alloc/free v2

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@amd.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

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@ffwll.ch

• */

+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