Am 08.06.23 um 08:55 schrieb Boris Brezillon:
If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed
to wait on all the external dependencies (those added to
drm_sched_job::dependencies) before signaling the job finished fence.
This is done this way to prevent jobs depending on these canceled jobs
from considering the resources they want to access as ready, when
they're actually still used by other components, thus leading to
potential data corruptions.
The problem is, the kill_jobs logic is omitting the last fence popped
from the dependencies array that was waited upon before
drm_sched_entity_kill() was called (drm_sched_entity::dependency field),
so we're basically waiting for all dependencies except one.
This is an attempt at fixing that, but also an opportunity to make sure
I understood the drm_sched_entity_kill(), because I'm not 100% sure if
skipping this currently popped dependency was intentional or not. I can't
see a good reason why we'd want to skip that one, but I might be missing
something.
Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
Cc: Frank Binns <frank.binns@xxxxxxxxxx>
Cc: Sarah Walker <sarah.walker@xxxxxxxxxx>
Cc: Donald Robson <donald.robson@xxxxxxxxxx>
Cc: Luben Tuikov <luben.tuikov@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
---
Stumbled across this issue while working on native dependency support
with Donald (which will be posted soon). Flagged as RFC, because I'm
not sure this is legit, and also not sure we want to fix it this way.
I tried re-using drm_sched_entity::dependency, but it's a bit of a mess
because of the asynchronousity of the wait, and the fact we use
drm_sched_entity::dependency to know if we have a clear_dep()
callback registered, so we can easily reset it without removing the
callback.
Well yes, that's a known problem. But this is really not the right
approach to fixing this.
Trying to wait for all the dependencies before killing jobs was added
because of the way we kept track of dma_fences in dma_resv objects.
Basically adding exclusive fences removed all other fences leading to a
bit fragile memory management.
This handling was removed by now and so the workaround for waiting for
dependencies is not really necessary any more, but I think it is still
better to do so.
The right approach of getting this waiting for dependencies completely
straight is also not to touch entity->dependency in any way, but to stop
removing them from the XA in drm_sched_job_dependency(). Otherwise you
don't catch the pipeline optimized ones either.
Regards,
Christian.
---
drivers/gpu/drm/scheduler/sched_entity.c | 40 ++++++++++++++++++------
drivers/gpu/drm/scheduler/sched_main.c | 3 ++
include/drm/gpu_scheduler.h | 12 +++++++
3 files changed, 45 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 68e807ae136a..3821f9adf7bd 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -176,20 +176,35 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
{
struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
finish_cb);
- int r;
dma_fence_put(f);
- /* Wait for all dependencies to avoid data corruptions */
- while (!xa_empty(&job->dependencies)) {
- f = xa_erase(&job->dependencies, job->last_dependency++);
- r = dma_fence_add_callback(f, &job->finish_cb,
- drm_sched_entity_kill_jobs_cb);
- if (!r)
+ /* Wait for all remaining dependencies to avoid data corruptions.
+ *
+ * We first check the last dependency popped from job->dependencies,
+ * and then walk job->dependencies.
+ *
+ * Note that we don't wait on the last fence returned by
+ * drm_gpu_scheduler_ops::prepare_job(), nor do we call
+ * drm_gpu_scheduler_ops::prepare_job() to empty the list of potential
+ * internal dependencies the driver might want to wait on before
+ * scheduling the job. We simply assume skipping internal dependencies
+ * can't cause data corruption on resources passed to the job.
+ */
+ do {
+ f = job->cur_dep;
+
+ if (!xa_empty(&job->dependencies))
+ job->cur_dep = xa_erase(&job->dependencies, job->last_dependency++);
+ else
+ job->cur_dep = NULL;
+
+ if (f &&
+ !dma_fence_add_callback(f, &job->finish_cb, drm_sched_entity_kill_jobs_cb))
return;
dma_fence_put(f);
- }
+ } while (job->cur_dep);
INIT_WORK(&job->work, drm_sched_entity_kill_jobs_work);
schedule_work(&job->work);
@@ -415,8 +430,13 @@ static struct dma_fence *
drm_sched_job_dependency(struct drm_sched_job *job,
struct drm_sched_entity *entity)
{
- if (!xa_empty(&job->dependencies))
- return xa_erase(&job->dependencies, job->last_dependency++);
+ dma_fence_put(job->cur_dep);
+ job->cur_dep = NULL;
+
+ if (!xa_empty(&job->dependencies)) {
+ job->cur_dep = xa_erase(&job->dependencies, job->last_dependency++);
+ return dma_fence_get(job->cur_dep);
+ }
if (job->sched->ops->prepare_job)
return job->sched->ops->prepare_job(job, entity);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 394010a60821..70ab60e5760c 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -839,6 +839,9 @@ void drm_sched_job_cleanup(struct drm_sched_job *job)
job->s_fence = NULL;
+ if (WARN_ON(job->cur_dep))
+ dma_fence_put(job->cur_dep);
+
xa_for_each(&job->dependencies, index, fence) {
dma_fence_put(fence);
}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index e95b4837e5a3..8e8e3decdc80 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -366,6 +366,18 @@ struct drm_sched_job {
/** @last_dependency: tracks @dependencies as they signal */
unsigned long last_dependency;
+ /*
+ * @cur_dep:
+ *
+ * Last dependency popped from @dependencies. Dependencies returned by
+ * drm_gpu_scheculer_ops::prepare_job() are not recorded here.
+ *
+ * We need to keep track of the last dependencies popped from
+ * @dependencies so we can wait on the already popped dependency when
+ * drm_sched_entity_kill_jobs_cb() is called.
+ */
+ struct dma_fence *cur_dep;
+
/**
* @submit_ts:
*
