On 10/10/2025 13:22, Philipp Stanner wrote:
On Wed, 2025-10-08 at 09:53 +0100, Tvrtko Ursulin wrote:
To implement fair scheduling we need a view into the GPU time consumed by
entities. Problem we have is that jobs and entities objects have decoupled
lifetimes, where at the point we have a view into accurate GPU time, we
cannot link back to the entity any longer.
Solve this by adding a light weight entity stats object which is reference
counted by both entity and the job and hence can safely be used from
either side.
With that, the only other thing we need is to add a helper for adding the
job's GPU time into the respective entity stats object, and call it once
the accurate GPU time has been calculated.
Signed-off-by: Tvrtko Ursulin <[email protected]>
Cc: Christian König <[email protected]>
Cc: Danilo Krummrich <[email protected]>
Cc: Matthew Brost <[email protected]>
Cc: Philipp Stanner <[email protected]>
---
drivers/gpu/drm/scheduler/sched_entity.c | 39 ++++++++++++
drivers/gpu/drm/scheduler/sched_internal.h | 71 ++++++++++++++++++++++
drivers/gpu/drm/scheduler/sched_main.c | 6 +-
include/drm/gpu_scheduler.h | 12 ++++
4 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 7a0a52ba87bf..04ce8b7d436b 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -32,6 +32,39 @@
#include "gpu_scheduler_trace.h"
+
+/**
+ * drm_sched_entity_stats_release - Entity stats kref release function
+ *
+ * @kref: Entity stats embedded kref pointer
We've got fractured docstring style throughout drm_sched. What I'd like
us to move to is no empty lines between first line and first parameter
for the function docstrings.
Applies to all the other functions, too.
Done here and throughout the series.
+ */
+void drm_sched_entity_stats_release(struct kref *kref)
+{
+ struct drm_sched_entity_stats *stats =
+ container_of(kref, typeof(*stats), kref);
+
+ kfree(stats);
+}
+
+/**
+ * drm_sched_entity_stats_alloc - Allocate a new struct drm_sched_entity_stats
object
+ *
+ * Returns: Pointer to newly allocated struct drm_sched_entity_stats object.
s/Returns/Return
That's at least how it's documented in the official docstring docu, and
we have fractured style here, too. Unifying that mid-term will be good.
Ditto.
+ */
+static struct drm_sched_entity_stats *drm_sched_entity_stats_alloc(void)
+{
+ 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;
+}
+
/**
* drm_sched_entity_init - Init a context entity used by scheduler when
* submit to HW ring.
@@ -65,6 +98,11 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
return -EINVAL;
memset(entity, 0, sizeof(struct drm_sched_entity));
+
+ entity->stats = drm_sched_entity_stats_alloc();
+ if (!entity->stats)
+ return -ENOMEM;
+
INIT_LIST_HEAD(&entity->list);
entity->rq = NULL;
entity->guilty = guilty;
@@ -338,6 +376,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
dma_fence_put(rcu_dereference_check(entity->last_scheduled, true));
RCU_INIT_POINTER(entity->last_scheduled, NULL);
+ drm_sched_entity_stats_put(entity->stats);
}
EXPORT_SYMBOL(drm_sched_entity_fini);
diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
index 5a8984e057e5..1132a771aa37 100644
--- a/drivers/gpu/drm/scheduler/sched_internal.h
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -3,6 +3,27 @@
#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_
#define _DRM_GPU_SCHEDULER_INTERNAL_H_
+#include <linux/ktime.h>
+#include <linux/kref.h>
+#include <linux/spinlock.h>
+
+/**
+ * struct drm_sched_entity_stats - execution stats for an entity.
+ *
+ * Because jobs and entities have decoupled lifetimes, ie. we cannot access the
+ * 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.
+ *
+ * @kref: reference count for the object.
+ * @lock: lock guarding the @runtime updates.
+ * @runtime: time entity spent on the GPU.
Same here, let's follow the official style
https://docs.kernel.org/doc-guide/kernel-doc.html#members
Yep.
+ */
+struct drm_sched_entity_stats {
+ struct kref kref;
+ spinlock_t lock;
+ ktime_t runtime;
+};
/* Used to choose between FIFO and RR job-scheduling */
extern int drm_sched_policy;
@@ -93,4 +114,54 @@ drm_sched_entity_is_ready(struct drm_sched_entity *entity)
return true;
}
+void drm_sched_entity_stats_release(struct kref *kref);
+
+/**
+ * drm_sched_entity_stats_get - Obtain a reference count on struct
drm_sched_entity_stats object
If you want to cross-link it you need a '&struct'
Done.
+ *
+ * @stats: struct drm_sched_entity_stats pointer
+ *
+ * Returns: struct drm_sched_entity_stats pointer
+ */
+static inline struct drm_sched_entity_stats *
+drm_sched_entity_stats_get(struct drm_sched_entity_stats *stats)
+{
+ kref_get(&stats->kref);
+
+ return stats;
+}
+
+/**
+ * drm_sched_entity_stats_put - Release a reference count on struct
drm_sched_entity_stats object
Same
+ *
+ * @stats: struct drm_sched_entity_stats pointer
+ */
+static inline void
+drm_sched_entity_stats_put(struct drm_sched_entity_stats *stats)
+{
+ kref_put(&stats->kref, drm_sched_entity_stats_release);
+}
+
+/**
+ * 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);
+}
+
#endif
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 41e076fdcb0d..f180d292bf66 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -658,6 +658,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);
}
@@ -846,6 +847,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);
Maybe you want to comment on this patch here:
https://lore.kernel.org/dri-devel/[email protected]/
I submitted it becausue of this change you make here.
I see there was some discussion. I'll try to form an opinion and reply
next week but feel free to ping me if I forget.
Regards,
Tvrtko
} else {
/* The job was aborted before it has been committed to be run;
* notably, drm_sched_job_arm() has not been called.
@@ -997,8 +999,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);
sched->ops->free_job(job);
+ }
drm_sched_run_job_queue(sched);
}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 8992393ed200..93d0b7224a57 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -71,6 +71,8 @@ enum drm_sched_priority {
DRM_SCHED_PRIORITY_COUNT
};
+struct drm_sched_entity_stats;
+
/**
* struct drm_sched_entity - A wrapper around a job queue (typically
* attached to the DRM file_priv).
@@ -110,6 +112,11 @@ struct drm_sched_entity {
*/
struct drm_sched_rq *rq;
+ /**
+ * @stats: Stats object reference held by the entity and jobs.
+ */
+ struct drm_sched_entity_stats *stats;
+
/**
* @sched_list:
*
@@ -365,6 +372,11 @@ struct drm_sched_job {
struct drm_sched_fence *s_fence;
struct drm_sched_entity *entity;
+ /**
+ * @entity_stats: Stats object reference held by the job and entity.
+ */
+ struct drm_sched_entity_stats *entity_stats;
+
enum drm_sched_priority s_priority;
u32 credits;
/** @last_dependency: tracks @dependencies as they signal */
Code itself looks correct and very nice and clean to me.
P.
