Re: [Intel-xe] [PATCH v4 09/10] drm/sched: Add helper to queue TDR immediately for current and future jobs

[Luben Tuikov <luben.tuikov@amd.com>]

On 2023-09-19 01:01, Matthew Brost wrote:
> Add helper to queue TDR immediately for current and future jobs. This
> will be used in XE, new Intel GPU driver, to trigger the TDR to cleanup

Please use present tense, "is", in code, comments, commits, etc.

Is it "XE" or is it "Xe"? I always thought it was "Xe".

	This is used in Xe, a new Intel GPU driver, to trigger a TDR to clean up

Code, comments, commits, etc., immediately become history, and it's a bit
ambitious to use future tense in something which immediately becomes
history. It's much better to describe what is happening now, including the patch
in question (any patch, ftm) is considered "now"/"current state" as well.

> a drm_scheduler that encounter error[.]> 
> v2:
>  - Drop timeout args, rename function, use mod delayed work (Luben)
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 19 ++++++++++++++++++-
>  include/drm/gpu_scheduler.h            |  1 +
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index e8a3e6033f66..88ef8be2d3c7 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -435,7 +435,7 @@ static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
>  
>  	if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
>  	    !list_empty(&sched->pending_list))
> -		queue_delayed_work(sched->timeout_wq, &sched->work_tdr, sched->timeout);
> +		mod_delayed_work(sched->timeout_wq, &sched->work_tdr, sched->timeout);
>  }
>  
>  static void drm_sched_start_timeout_unlocked(struct drm_gpu_scheduler *sched)
> @@ -445,6 +445,23 @@ static void drm_sched_start_timeout_unlocked(struct drm_gpu_scheduler *sched)
>  	spin_unlock(&sched->job_list_lock);
>  }
>  
> +/**
> + * drm_sched_tdr_queue_imm: - immediately start timeout handler including future
> + * jobs

Let's not mention "including future jobs", since we don't know the future.
But we can sneak "jobs" into the description like this:

 * drm_sched_tdr_queue_imm - immediately start job timeout handler

:-)

> + *
> + * @sched: scheduler where the timeout handling should be started.

"where" --> "for which"
The former denotes a location, like in space-time, and the latter
denotes an object, like a scheduler, a spaceship, a bicycle, etc.

> + *
> + * Start timeout handling immediately for current and future jobs

 * Start timeout handling immediately for the named scheduler.

> + */
> +void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched)
> +{
> +	spin_lock(&sched->job_list_lock);
> +	sched->timeout = 0;
> +	drm_sched_start_timeout(sched);
> +	spin_unlock(&sched->job_list_lock);
> +}
> +EXPORT_SYMBOL(drm_sched_tdr_queue_imm);
> +
>  /**
>   * drm_sched_fault - immediately start timeout handler
>   *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 7e6c121003ca..27f5778bbd6d 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -568,6 +568,7 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>  				    struct drm_gpu_scheduler **sched_list,
>                                     unsigned int num_sched_list);
>  
> +void drm_sched_tdr_queue_imm(struct drm_gpu_scheduler *sched);
>  void drm_sched_job_cleanup(struct drm_sched_job *job);
>  void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched);
>  bool drm_sched_submit_ready(struct drm_gpu_scheduler *sched);

Looks good!

Fix the above, for an immediate R-B. :-)
-- 
Regards,
Luben