Re: [PATCH v3 05/13] drm/sched: Split free_job into own work item
On Tue, Sep 12, 2023 at 04:53:00PM +0200, Boris Brezillon wrote: > On Tue, 12 Sep 2023 14:37:57 + > Matthew Brost wrote: > > > > Looks like you are changing the behavior here (unconditional -> > > > conditional timestamp update)? Probably something that should go in a > > > separate patch. > > > > > > > This patch creates a race so this check isn't need before this patch. > > With that I think it makes sense to have all in a single patch. If you > > feel strongly about this, I can break this change out into a patch prior > > to this one. > > It's probably fine to keep it in this patch, but we should > definitely have a comment explaining why this check is needed. Sure, will add comment in next rev. Matt
Re: [PATCH v3 05/13] drm/sched: Split free_job into own work item
On Tue, 12 Sep 2023 14:37:57 + Matthew Brost wrote: > > Looks like you are changing the behavior here (unconditional -> > > conditional timestamp update)? Probably something that should go in a > > separate patch. > > > > This patch creates a race so this check isn't need before this patch. > With that I think it makes sense to have all in a single patch. If you > feel strongly about this, I can break this change out into a patch prior > to this one. It's probably fine to keep it in this patch, but we should definitely have a comment explaining why this check is needed.
Re: [PATCH v3 05/13] drm/sched: Split free_job into own work item
On Tue, Sep 12, 2023 at 10:08:33AM +0200, Boris Brezillon wrote:
> On Mon, 11 Sep 2023 19:16:07 -0700
> Matthew Brost wrote:
>
> > Rather than call free_job and run_job in same work item have a dedicated
> > work item for each. This aligns with the design and intended use of work
> > queues.
> >
> > v2:
> >- Test for DMA_FENCE_FLAG_TIMESTAMP_BIT before setting
> > timestamp in free_job() work item (Danilo)
> >
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c | 143 ++---
> > include/drm/gpu_scheduler.h| 8 +-
> > 2 files changed, 110 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 3820e9ae12c8..d28b6751256e 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq
> > *rq,
> > * drm_sched_rq_select_entity_rr - Select an entity which could provide a
> > job to run
> > *
> > * @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
> > *
> > * Try to find a ready entity, returns NULL if none found.
> > */
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct drm_sched_entity *entity;
> >
> > @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > if (entity) {
> > list_for_each_entry_continue(entity, >entities, list) {
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > spin_unlock(>lock);
> > return entity;
> > }
> > @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > list_for_each_entry(entity, >entities, list) {
> >
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > spin_unlock(>lock);
> > return entity;
> > }
> > @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> > * drm_sched_rq_select_entity_fifo - Select an entity which provides a job
> > to run
> > *
> > * @rq: scheduler run queue to check.
> > + * @dequeue: dequeue selected entity
> > *
> > * Find oldest waiting ready entity, returns NULL if none found.
> > */
> > static struct drm_sched_entity *
> > -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> > +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
> > {
> > struct rb_node *rb;
> >
> > @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
> > *rq)
> >
> > entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> > if (drm_sched_entity_is_ready(entity)) {
> > - rq->current_entity = entity;
> > - reinit_completion(>entity_idle);
> > + if (dequeue) {
> > + rq->current_entity = entity;
> > + reinit_completion(>entity_idle);
> > + }
> > break;
> > }
> > }
> > @@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq
> > *rq)
> > }
> >
> > /**
> > - * drm_sched_submit_queue - scheduler queue submission
> > + * drm_sched_run_job_queue - queue job submission
> > * @sched: scheduler instance
> > */
> > -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> > +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> > {
> > if (!READ_ONCE(sched->pause_submit))
> > - queue_work(sched->submit_wq, >work_submit);
> > + queue_work(sched->submit_wq, >work_run_job);
> > +}
> > +
> > +static struct drm_sched_entity *
> > +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
>
> Nit: Can you drop this forward declaration and move the function here?
>
Sure. Will likely move this function in a seperate patch though.
> > +
> > +/**
> > + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> > + * @sched: scheduler instance
> > + */
> > +static void
Re: [PATCH v3 05/13] drm/sched: Split free_job into own work item
On Mon, 11 Sep 2023 19:16:07 -0700
Matthew Brost wrote:
> Rather than call free_job and run_job in same work item have a dedicated
> work item for each. This aligns with the design and intended use of work
> queues.
>
> v2:
>- Test for DMA_FENCE_FLAG_TIMESTAMP_BIT before setting
> timestamp in free_job() work item (Danilo)
>
> Signed-off-by: Matthew Brost
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 143 ++---
> include/drm/gpu_scheduler.h| 8 +-
> 2 files changed, 110 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 3820e9ae12c8..d28b6751256e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> * drm_sched_rq_select_entity_rr - Select an entity which could provide a
> job to run
> *
> * @rq: scheduler run queue to check.
> + * @dequeue: dequeue selected entity
> *
> * Try to find a ready entity, returns NULL if none found.
> */
> static struct drm_sched_entity *
> -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
> {
> struct drm_sched_entity *entity;
>
> @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> if (entity) {
> list_for_each_entry_continue(entity, >entities, list) {
> if (drm_sched_entity_is_ready(entity)) {
> - rq->current_entity = entity;
> - reinit_completion(>entity_idle);
> + if (dequeue) {
> + rq->current_entity = entity;
> + reinit_completion(>entity_idle);
> + }
> spin_unlock(>lock);
> return entity;
> }
> @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> list_for_each_entry(entity, >entities, list) {
>
> if (drm_sched_entity_is_ready(entity)) {
> - rq->current_entity = entity;
> - reinit_completion(>entity_idle);
> + if (dequeue) {
> + rq->current_entity = entity;
> + reinit_completion(>entity_idle);
> + }
> spin_unlock(>lock);
> return entity;
> }
> @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
> * drm_sched_rq_select_entity_fifo - Select an entity which provides a job
> to run
> *
> * @rq: scheduler run queue to check.
> + * @dequeue: dequeue selected entity
> *
> * Find oldest waiting ready entity, returns NULL if none found.
> */
> static struct drm_sched_entity *
> -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
> {
> struct rb_node *rb;
>
> @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>
> entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> if (drm_sched_entity_is_ready(entity)) {
> - rq->current_entity = entity;
> - reinit_completion(>entity_idle);
> + if (dequeue) {
> + rq->current_entity = entity;
> + reinit_completion(>entity_idle);
> + }
> break;
> }
> }
> @@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
> }
>
> /**
> - * drm_sched_submit_queue - scheduler queue submission
> + * drm_sched_run_job_queue - queue job submission
> * @sched: scheduler instance
> */
> -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
> +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> {
> if (!READ_ONCE(sched->pause_submit))
> - queue_work(sched->submit_wq, >work_submit);
> + queue_work(sched->submit_wq, >work_run_job);
> +}
> +
> +static struct drm_sched_entity *
> +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
Nit: Can you drop this forward declaration and move the function here?
> +
> +/**
> + * drm_sched_run_job_queue_if_ready - queue job submission if ready
> + * @sched: scheduler instance
> + */
> +static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler *sched)
> +{
> + if (drm_sched_select_entity(sched, false))
> + drm_sched_run_job_queue(sched);
> +}
> +
> +/**
> + * drm_sched_free_job_queue - queue free job
> + *
> + * @sched: scheduler instance to queue free job
> + */
> +static void
