On Fri, Sep 29, 2023 at 06:44:53PM -0400, Luben Tuikov wrote:
> 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".
>
Yea should be 'Xe'.
> This is used in Xe, a new Intel GPU driver, to trigger a TDR to clean up
>
Will fix.
> 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.
>
Got it.
> > a drm_scheduler that encounter error[.]>
> > v2:
> > - Drop timeout args, rename function, use mod delayed work (Luben)
> >
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > ---
> > 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
>
> :-)
>
Will change.
> > + *
> > + * @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.
>
+1
> > + *
> > + * Start timeout handling immediately for current and future jobs
>
> * Start timeout handling immediately for the named scheduler.
>
+1
> > + */
> > +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. :-)
Thanks for the review, will fix all of this.
Matt
> --
> Regards,
> Luben
>
