Re: [PATCH v5 07/28] drm/sched: Account entity GPU time

["Danilo Krummrich" <dakr@kernel.org>]

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.

> +{
> +	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.

> + * 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.

> +};

<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.

> +
>  #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?

>  		sched->ops->free_job(job);
> +	}