On 09/01/2025 14:17, Christian König wrote:
Am 09.01.25 um 14:20 schrieb Tvrtko Ursulin:
On 09/01/2025 12:49, Christian König wrote:
Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
It is not helping readability nor it is required to keep the line
length
in check.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: Danilo Krummrich <dakr@xxxxxxxxxx>
Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
Cc: Philipp Stanner <pstanner@xxxxxxxxxx>
---
drivers/gpu/drm/scheduler/sched_main.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 1734c17aeea5..01e0d6e686d1 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct
work_struct *w)
container_of(w, struct drm_gpu_scheduler, work_run_job);
struct drm_sched_entity *entity;
struct dma_fence *fence;
- struct drm_sched_fence *s_fence;
struct drm_sched_job *sched_job;
int r;
@@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct
work_struct *w)
return;
}
- s_fence = sched_job->s_fence;
-
atomic_add(sched_job->credits, &sched->credit_count);
drm_sched_job_begin(sched_job);
trace_drm_run_job(sched_job, entity);
fence = sched->ops->run_job(sched_job);
complete_all(&entity->entity_idle);
- drm_sched_fence_scheduled(s_fence, fence);
+ drm_sched_fence_scheduled(sched_job->s_fence, fence);
Originally that was not for readability but for correctness.
As soon as complete_all(&entity->entity_idle); was called the
sched_job could have been released.
And without a comment ouch.
That changed long long time ago and IIRC we did had a comment for that.
But we changed that so that the sched_job is released from a separate
worker instead, so that is most likely not necessary any more.
Very subtle. Especially given some drivers use unordered queue.
Hui? unordered queue? How should that work?
Job submission ordering is a mandatory requirement of the dma_fence.
I think it is fine for submission since it is a single work item which
still runs serialized to itself. But free work can the overtake it on
drivers who pass in unordered queue.
I think Matt promised to document the ordered vs unordered
criteria/requirements some time ago and maybe forgot*.
In any case seems like moving the complete_all() to be last is the
safest option. I'll rework this patch to that effect for v3.
Regards,
Tvrtko
*)
https://lore.kernel.org/all/ZjlmZHBMfK9fld9c@xxxxxxxxxxxxxxxxxxxxxxxx/T/
And for them sched_job is dereferenced a few more times in the block
below so not sure how it is safe.
Move complete_all() to the end of it all?
Most likely good idea, yes.
Regards,
Christian.
Regards,
Tvrtko
if (!IS_ERR_OR_NULL(fence)) {
/* Drop for original kref_init of the fence */
