<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 2022-04-26 22:31, Hangyu Hua wrote:<br>
</div>
<blockquote type="cite" cite="mid:88dd5d67-7dd5-2f58-5254-adaa941deb0f@gmail.com">On
2022/4/26 22:55, Andrey Grodzovsky wrote:
<br>
<blockquote type="cite">
<br>
On 2022-04-25 22:54, Hangyu Hua wrote:
<br>
<blockquote type="cite">On 2022/4/25 23:42, Andrey Grodzovsky
wrote:
<br>
<blockquote type="cite">On 2022-04-25 04:36, Hangyu Hua wrote:
<br>
<br>
<blockquote type="cite">When drm_sched_job_add_dependency()
fails, dma_fence_put() will be called
<br>
internally. Calling it again after
drm_sched_job_add_dependency() finishes
<br>
may result in a dangling pointer.
<br>
<br>
Fix this by removing redundant dma_fence_put().
<br>
<br>
Signed-off-by: Hangyu Hua <a class="moz-txt-link-rfc2396E" href="mailto:hbh25y@gmail.com"><hbh25y@gmail.com></a>
<br>
---
<br>
drivers/gpu/drm/lima/lima_gem.c | 1 -
<br>
drivers/gpu/drm/scheduler/sched_main.c | 1 -
<br>
2 files changed, 2 deletions(-)
<br>
<br>
diff --git a/drivers/gpu/drm/lima/lima_gem.c
b/drivers/gpu/drm/lima/lima_gem.c
<br>
index 55bb1ec3c4f7..99c8e7f6bb1c 100644
<br>
--- a/drivers/gpu/drm/lima/lima_gem.c
<br>
+++ b/drivers/gpu/drm/lima/lima_gem.c
<br>
@@ -291,7 +291,6 @@ static int lima_gem_add_deps(struct
drm_file *file, struct lima_submit *submit)
<br>
err =
drm_sched_job_add_dependency(&submit->task->base,
fence);
<br>
if (err) {
<br>
- dma_fence_put(fence);
<br>
return err;
<br>
</blockquote>
<br>
<br>
Makes sense here
<br>
<br>
<br>
<blockquote type="cite"> }
<br>
}
<br>
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
<br>
index b81fceb0b8a2..ebab9eca37a8 100644
<br>
--- a/drivers/gpu/drm/scheduler/sched_main.c
<br>
+++ b/drivers/gpu/drm/scheduler/sched_main.c
<br>
@@ -708,7 +708,6 @@ int
drm_sched_job_add_implicit_dependencies(struct
drm_sched_job *job,
<br>
dma_fence_get(fence);
<br>
ret = drm_sched_job_add_dependency(job, fence);
<br>
if (ret) {
<br>
- dma_fence_put(fence);
<br>
</blockquote>
<br>
<br>
<br>
Not sure about this one since if you look at the relevant
commits -
<br>
'drm/scheduler: fix drm_sched_job_add_implicit_dependencies'
and
<br>
'drm/scheduler: fix drm_sched_job_add_implicit_dependencies
harder'
<br>
You will see that the dma_fence_put here balances the extra
dma_fence_get
<br>
above
<br>
<br>
Andrey
<br>
<br>
</blockquote>
<br>
I don't think so. I checked the call chain and found no
additional dma_fence_get(). But dma_fence_get() needs to be
called before drm_sched_job_add_dependency() to keep the
counter balanced. </blockquote>
<br>
<br>
I don't say there is an additional get, I just say that
drm_sched_job_add_dependency doesn't grab an extra reference to
the fences it stores so this needs to be done outside and for
that
<br>
drm_sched_job_add_implicit_dependencies->dma_fence_get is
called and, if this addition fails you just call dma_fence_put
to keep the counter balanced.
<br>
<br>
</blockquote>
<br>
drm_sched_job_add_implicit_dependencies() will call
drm_sched_job_add_dependency(). And drm_sched_job_add_dependency()
already call dma_fence_put() when it fails. Calling
dma_fence_put() twice doesn't make sense.
<br>
<br>
dma_fence_get() is in [2]. But dma_fence_put() will be called in
[1] and [3] when xa_alloc() fails.
<br>
</blockquote>
<p><br>
</p>
<p>The way I see it, [2] and [3] are mat matching <b>get</b> and <b>put</b>
respectively. [1] <b>put</b> is against the original
dma_fence_init->kref_init of the fence which always set the
refcount at 1.<br>
Also in support of this see commit 'drm/scheduler: fix
drm_sched_job_add_implicit_dependencies harder' - it says there
"drm_sched_job_add_dependency() could drop the last ref" - this
last ref is the original refcount set by dma_fence_init->kref <br>
</p>
<p>Andrey</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:88dd5d67-7dd5-2f58-5254-adaa941deb0f@gmail.com">
<br>
<br>
int drm_sched_job_add_dependency(struct drm_sched_job *job,
<br>
struct dma_fence *fence)
<br>
{
<br>
...
<br>
ret = xa_alloc(&job->dependencies, &id, fence,
xa_limit_32b, GFP_KERNEL);
<br>
if (ret != 0)
<br>
dma_fence_put(fence); <--- [1]
<br>
<br>
return ret;
<br>
}
<br>
EXPORT_SYMBOL(drm_sched_job_add_dependency);
<br>
<br>
<br>
int drm_sched_job_add_implicit_dependencies(struct drm_sched_job
*job,
<br>
struct drm_gem_object *obj,
<br>
bool write)
<br>
{
<br>
struct dma_resv_iter cursor;
<br>
struct dma_fence *fence;
<br>
int ret;
<br>
<br>
dma_resv_for_each_fence(&cursor, obj->resv, write,
fence) {
<br>
/* Make sure to grab an additional ref on the added fence
*/
<br>
dma_fence_get(fence); <--- [2]
<br>
ret = drm_sched_job_add_dependency(job, fence);
<br>
if (ret) {
<br>
dma_fence_put(fence); <--- [3]
<br>
return ret;
<br>
}
<br>
}
<br>
return 0;
<br>
}
<br>
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">On the other hand, dma_fence_get() and
dma_fence_put() are meaningless here if threre is an extra
dma_fence_get() beacause counter will not decrease to 0 during
drm_sched_job_add_dependency().
<br>
<br>
I check the call chain as follows:
<br>
<br>
msm_ioctl_gem_submit()
<br>
-> submit_fence_sync()
<br>
-> drm_sched_job_add_implicit_dependencies()
<br>
</blockquote>
<br>
<br>
Can you maybe trace or print one such example of problematic
refcount that you are trying to fix ? I still don't see where is
the problem.
<br>
<br>
Andrey
<br>
<br>
</blockquote>
<br>
I also wish I could. System logs can make this easy. But i don't
have a corresponding GPU physical device.
drm_sched_job_add_implicit_dependencies is only used in a few
devices.
<br>
<br>
Thanks.
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">
<br>
Thanks,
<br>
Hangyu
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite"> return ret;
<br>
}
<br>
}
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>