Commit 45ecaea7 authored by Andrey Grodzovsky's avatar Andrey Grodzovsky Committed by Alex Deucher

drm/sched: Partial revert of 'drm/sched: Keep s_fence->parent pointer'

Problem:
This patch caused negative refcount as described in [1] because
for that case parent fence did not signal by the time of drm_sched_stop and hence
kept in pending list the assumption was they will not signal and
so fence was put to account for the s_fence->parent refcount but for
amdgpu which has embedded HW fence (always same parent fence)
drm_sched_fence_release_scheduled was always called and would
still drop the count for parent fence once more. For jobs that
never signaled this imbalance was masked by refcount bug in
amdgpu_fence_driver_clear_job_fences that would not drop
refcount on the fences that were removed from fence drive
fences array (against prevois insertion into the array in
get in amdgpu_fence_emit).

Fix:
Revert this patch and by setting s_job->s_fence->parent to NULL
as before prevent the extra refcount drop in amdgpu when
drm_sched_fence_release_scheduled is called on job release.

Also - align behaviour in drm_sched_resubmit_jobs_ext with that of
drm_sched_main when submitting jobs - take a refcount for the
new parent fence pointer and drop refcount for original kref_init
for new HW fence creation (or fake new HW fence in amdgpu - see next patch).

[1] - https://lore.kernel.org/all/731b7ff1-3cc9-e314-df2a-7c51b76d4db0@amd.com/t/#r00c728fcc069b1276642c325bfa9d82bf8fa21a3Signed-off-by: default avatarAndrey Grodzovsky <andrey.grodzovsky@amd.com>
Tested-by: default avatarYiqing Yao <yiqing.yao@amd.com>
Acked-by: default avatarChristian König <christian.koenig@amd.com>
Signed-off-by: default avatarAlex Deucher <alexander.deucher@amd.com>
parent 9e225fb9
...@@ -419,6 +419,8 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad) ...@@ -419,6 +419,8 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
if (s_job->s_fence->parent && if (s_job->s_fence->parent &&
dma_fence_remove_callback(s_job->s_fence->parent, dma_fence_remove_callback(s_job->s_fence->parent,
&s_job->cb)) { &s_job->cb)) {
dma_fence_put(s_job->s_fence->parent);
s_job->s_fence->parent = NULL;
atomic_dec(&sched->hw_rq_count); atomic_dec(&sched->hw_rq_count);
} else { } else {
/* /*
...@@ -548,7 +550,6 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max) ...@@ -548,7 +550,6 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
if (found_guilty && s_job->s_fence->scheduled.context == guilty_context) if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
dma_fence_set_error(&s_fence->finished, -ECANCELED); dma_fence_set_error(&s_fence->finished, -ECANCELED);
dma_fence_put(s_job->s_fence->parent);
fence = sched->ops->run_job(s_job); fence = sched->ops->run_job(s_job);
i++; i++;
...@@ -558,7 +559,11 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max) ...@@ -558,7 +559,11 @@ void drm_sched_resubmit_jobs_ext(struct drm_gpu_scheduler *sched, int max)
s_job->s_fence->parent = NULL; s_job->s_fence->parent = NULL;
} else { } else {
s_job->s_fence->parent = fence;
s_job->s_fence->parent = dma_fence_get(fence);
/* Drop for orignal kref_init */
dma_fence_put(fence);
} }
} }
} }
...@@ -955,6 +960,9 @@ static int drm_sched_main(void *param) ...@@ -955,6 +960,9 @@ static int drm_sched_main(void *param)
if (!IS_ERR_OR_NULL(fence)) { if (!IS_ERR_OR_NULL(fence)) {
s_fence->parent = dma_fence_get(fence); s_fence->parent = dma_fence_get(fence);
/* Drop for original kref_init of the fence */
dma_fence_put(fence);
r = dma_fence_add_callback(fence, &sched_job->cb, r = dma_fence_add_callback(fence, &sched_job->cb,
drm_sched_job_done_cb); drm_sched_job_done_cb);
if (r == -ENOENT) if (r == -ENOENT)
...@@ -962,7 +970,6 @@ static int drm_sched_main(void *param) ...@@ -962,7 +970,6 @@ static int drm_sched_main(void *param)
else if (r) else if (r)
DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
r); r);
dma_fence_put(fence);
} else { } else {
if (IS_ERR(fence)) if (IS_ERR(fence))
dma_fence_set_error(&s_fence->finished, PTR_ERR(fence)); dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment