[PATCH 2/2] drm/scheduler: Add a start_timeout_notify() backend function

[Koenig, Christian]

Am 02.11.18 um 11:31 schrieb Sharat Masetty:
> Add an optional backend function op which will let the scheduler clients
> know when the timeout got scheduled on the scheduler instance. This will
> help drivers with multiple schedulers(one per ring) measure time spent on
> the ring accurately, eventually helping with better timeout detection.
>
> Signed-off-by: Sharat Masetty <smasetty at codeaurora.org>

Well, NAK. drm_sched_start_timeout() is called whenever the timer needs 
to run, but that doesn't mean that the timer is started (e.g. it can 
already be running).

So the callback would be called multiple times and not reflect the 
actual job run time.

Additional to that it can be racy, e.g. we can complete multiple jobs at 
a time before the timer is started again.

If you want to accurately count how much time you spend on each job/ring 
you need to do this by measuring the time inside your driver instead.

E.g. for amdgpu I would get the time first in amdgpu_job_run() and then 
again in amdgpu_job_free_cb() and calculate the difference.

Regards,
Christian.

> ---
> Here is an example of how I plan to use this new function callback.
>
> [1] https://patchwork.freedesktop.org/patch/254227/
>
>   drivers/gpu/drm/scheduler/sched_main.c | 7 ++++++-
>   include/drm/gpu_scheduler.h            | 6 ++++++
>   2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index c993d10..afd461e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -192,8 +192,13 @@ bool drm_sched_dependency_optimized(struct dma_fence* fence,
>   static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
>   {
>   	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
> -	    !list_empty(&sched->ring_mirror_list))
> +	    !list_empty(&sched->ring_mirror_list)) {
> +
>   		schedule_delayed_work(&sched->work_tdr, sched->timeout);
> +
> +		if (sched->ops->start_timeout_notify)
> +			sched->ops->start_timeout_notify(sched);
> +	}
>   }
>
>   /* job_finish is called after hw fence signaled
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index d87b268..faf28b4 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -239,6 +239,12 @@ struct drm_sched_backend_ops {
>            * and it's time to clean it up.
>   	 */
>   	void (*free_job)(struct drm_sched_job *sched_job);
> +
> +	/*
> +	 * (Optional) Called to let the driver know that a timeout detection
> +	 * timer has been started.
> +	 */
> +	void (*start_timeout_notify)(struct drm_gpu_scheduler *sched);
>   };
>
>   /**
> --
> 1.9.1
>