From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 2/7] dma-buf: add dma_fence_chain_alloc/free
Date: Fri, 11 Jun 2021 13:48:18 +0200 [thread overview]
Message-ID: <43177627-bc11-74b0-f88c-6475dd2904ff@gmail.com> (raw)
In-Reply-To: <YMMW0ROMlgJQyTdU@phenom.ffwll.local>
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
>>
next prev parent reply other threads:[~2021-06-11 11:48 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-10 9:17 Change how amdgpu stores fences in dma_resv objects Christian König
2021-06-10 9:17 ` [PATCH 1/7] dma-buf: some dma_fence_chain improvements Christian König
2021-06-11 7:49 ` Daniel Vetter
2021-06-10 9:17 ` [PATCH 2/7] dma-buf: add dma_fence_chain_alloc/free Christian König
2021-06-11 7:54 ` Daniel Vetter
2021-06-11 11:48 ` Christian König [this message]
2021-06-11 14:40 ` Daniel Vetter
2021-06-10 9:17 ` [PATCH 3/7] dma-buf: add dma_fence_chain_alloc/free self tests Christian König
2021-06-11 7:58 ` Daniel Vetter
2021-06-11 10:04 ` Christian König
2021-06-10 9:17 ` [PATCH 4/7] dma-buf: add dma_fence_chain_garbage_collect Christian König
2021-06-11 8:58 ` Daniel Vetter
2021-06-11 10:07 ` Christian König
2021-06-11 15:11 ` Daniel Vetter
2021-06-16 17:29 ` kernel test robot
2021-06-16 18:50 ` kernel test robot
2021-06-10 9:17 ` [PATCH 5/7] drm/syncobj: drop the manual garbage collection Christian König
2021-06-10 9:17 ` [PATCH 6/7] drm/amdgpu: unwrap fence chains in the explicit sync fence Christian König
2021-06-11 9:07 ` Daniel Vetter
2021-06-11 10:09 ` Christian König
2021-06-11 15:18 ` Daniel Vetter
2021-06-14 7:25 ` Christian König
2021-06-17 16:51 ` Daniel Vetter
2021-06-10 9:18 ` [PATCH 7/7] drm/amdgpu: rework dma_resv handling Christian König
2021-06-11 9:17 ` Daniel Vetter
2021-06-11 10:12 ` Christian König
2021-06-11 15:21 ` Daniel Vetter
2021-06-10 16:34 ` Change how amdgpu stores fences in dma_resv objects Michel Dänzer
2021-06-10 16:44 ` Christian König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=43177627-bc11-74b0-f88c-6475dd2904ff@gmail.com \
--to=ckoenig.leichtzumerken@gmail.com \
--cc=amd-gfx@lists.freedesktop.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).