On 14/01/2026 17:48, Danilo Krummrich wrote:
On Fri Dec 19, 2025 at 2:53 PM CET, Tvrtko Ursulin wrote:
+/**
+ * drm_sched_entity_stats_alloc - Allocate a new struct drm_sched_entity_stats
object
+ *
+ * Return: Pointer to newly allocated struct drm_sched_entity_stats object.
+ */
+static struct drm_sched_entity_stats *drm_sched_entity_stats_alloc(void)
NIT: This function allocates and initializes the structure, hence something
along the lines of *_new() is a better fit.
Can do.
+{
+ struct drm_sched_entity_stats *stats;
+
+ stats = kzalloc(sizeof(*stats), GFP_KERNEL);
+ if (!stats)
+ return NULL;
+
+ kref_init(&stats->kref);
+ spin_lock_init(&stats->lock);
+
+ return stats;
+}
<snip>
+/**
+ * struct drm_sched_entity_stats - execution stats for an entity.
+ * @kref: reference count for the object.
+ * @lock: lock guarding the @runtime updates.
+ * @runtime: time entity spent on the GPU.
+ *
+ * Because jobs and entities have decoupled lifetimes, ie. we cannot access the
The beginning of this sentence seems slightly broken.
Suggest me an alternative because I don't see it?
+ * entity once the job is completed and we know how much time it took on the
+ * GPU, we need to track these stats in a separate object which is then
+ * reference counted by both entities and jobs.
+ */
+struct drm_sched_entity_stats {
+ struct kref kref;
+ spinlock_t lock;
+ ktime_t runtime;
We can avoid the lock entirely by using a atomic64_t instead. ktime_t is just a
typedef for s64.
Later in the series lock is needed (more members get added) so I wanted
to avoid the churn of converting the atomic64_t to ktime_t in the fair
policy patch.
+};
<snip>
+/**
+ * drm_sched_entity_stats_job_add_gpu_time - Account job execution time to
entity
+ * @job: Scheduler job to account.
+ *
+ * Accounts the execution time of @job to its respective entity stats object.
+ */
+static inline void
+drm_sched_entity_stats_job_add_gpu_time(struct drm_sched_job *job)
+{
+ struct drm_sched_entity_stats *stats = job->entity_stats;
+ struct drm_sched_fence *s_fence = job->s_fence;
+ ktime_t start, end;
+
+ start = dma_fence_timestamp(&s_fence->scheduled);
+ end = dma_fence_timestamp(&s_fence->finished);
+
+ spin_lock(&stats->lock);
+ stats->runtime = ktime_add(stats->runtime, ktime_sub(end, start));
+ spin_unlock(&stats->lock);
+}
This shouldn't be an inline function in the header, please move to
sched_entity.c.
It is not super pretty for a static inline but it was a pragmatic choice
because it doesn't really belong to sched_entity.c. The whole entity
stats object that is. Jobs and entities have only an association
relationship to struct drm_sched_entity_stats. The only caller for this
is even in sched_main.c while other updates are done in and from sched_rq.c.
So if pragmatic approach is not acceptable I would even rather create a
new file along the lines of sched_entity_stats.h|c. Unless that turns
out would have some other design wart of leaking knowledge of some other
part of the scheduler (ie wouldn't be fully standalone).
+
#endif
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index f825ad9e2260..4c10c7ba6704 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -660,6 +660,7 @@ void drm_sched_job_arm(struct drm_sched_job *job)
job->sched = sched;
job->s_priority = entity->priority;
+ job->entity_stats = drm_sched_entity_stats_get(entity->stats);
drm_sched_fence_init(job->s_fence, job->entity);
}
@@ -849,6 +850,7 @@ void drm_sched_job_cleanup(struct drm_sched_job *job)
* been called.
*/
dma_fence_put(&job->s_fence->finished);
+ drm_sched_entity_stats_put(job->entity_stats);
} else {
/* The job was aborted before it has been committed to be run;
* notably, drm_sched_job_arm() has not been called.
@@ -1000,8 +1002,10 @@ static void drm_sched_free_job_work(struct work_struct
*w)
container_of(w, struct drm_gpu_scheduler, work_free_job);
struct drm_sched_job *job;
- while ((job = drm_sched_get_finished_job(sched)))
+ while ((job = drm_sched_get_finished_job(sched))) {
+ drm_sched_entity_stats_job_add_gpu_time(job);
Is it really always OK to update this value in the free job work? What if a new
job gets scheduled concurrently. Doesn't this hurt accuracy, since the entity
value has not been updated yet?
What exactly you mean by entity value?
If a new job gets scheduled concurrently then it is either just about to
run, still running, both of which are not relevant for this finished
job, and once finished will also end up here to have it's duration
accounted against the stats.
Regards,
Tvrtko
sched->ops->free_job(job);
+ }
