| Message ID | 20250207145104.60455-5-tvrtko.ursulin@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/sched: Job queue peek/pop helpers and struct job re-order | expand |
btw. I still believe that it would be helpful (and congruent with the established norm) to have the version in all patch titles. I do use threaded view, but inboxes are huge, and everything that helps you orient yourself is welcome On Fri, 2025-02-07 at 14:51 +0000, Tvrtko Ursulin wrote: > Helper is for scheduler internal use so lets hide it from DRM drivers > completely. > > At the same time we change the method of checking whethere there is > anything in the queue from peeking to looking at the node count. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Matthew Brost <matthew.brost@intel.com> > Cc: Philipp Stanner <phasta@kernel.org> > --- > drivers/gpu/drm/scheduler/sched_entity.c | 12 ------------ > drivers/gpu/drm/scheduler/sched_internal.h | 13 +++++++++++++ > include/drm/gpu_scheduler.h | 1 - > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > b/drivers/gpu/drm/scheduler/sched_entity.c > index a171f05ad761..87f88259ddf6 100644 > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > @@ -151,18 +151,6 @@ static bool drm_sched_entity_is_idle(struct > drm_sched_entity *entity) > return false; > } > > -/* Return true if entity could provide a job. */ > -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity) > -{ > - if (spsc_queue_peek(&entity->job_queue) == NULL) > - return false; > - > - if (READ_ONCE(entity->dependency)) > - return false; > - > - return true; > -} > - > /** > * drm_sched_entity_error - return error of last scheduled job > * @entity: scheduler entity to check > diff --git a/drivers/gpu/drm/scheduler/sched_internal.h > b/drivers/gpu/drm/scheduler/sched_internal.h > index 25ac62ac2bf3..9ff5cddb5708 100644 > --- a/drivers/gpu/drm/scheduler/sched_internal.h > +++ b/drivers/gpu/drm/scheduler/sched_internal.h > @@ -43,4 +43,17 @@ drm_sched_entity_queue_peek(struct > drm_sched_entity *entity) > return container_of(node, struct drm_sched_job, queue_node); > } > > +/* Return true if entity could provide a job. */ > +static inline bool > +drm_sched_entity_is_ready(struct drm_sched_entity *entity) > +{ > + if (!spsc_queue_count(&entity->job_queue)) > + return false; > + > + if (READ_ONCE(entity->dependency)) > + return false; > + > + return true; > +} Is there any particular reason why you want the code in an header instead of a new .c file? We might want to consider this, since I'd expect that the number of internal helpers increases over the years. P. > + > #endif > diff --git a/include/drm/gpu_scheduler.h > b/include/drm/gpu_scheduler.h > index 5188c7f3bd3b..962825613596 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -606,7 +606,6 @@ struct drm_sched_job > *drm_sched_entity_pop_job(struct drm_sched_entity *entity); > void drm_sched_entity_push_job(struct drm_sched_job *sched_job); > void drm_sched_entity_set_priority(struct drm_sched_entity *entity, > enum drm_sched_priority > priority); > -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); > int drm_sched_entity_error(struct drm_sched_entity *entity); > > struct drm_sched_fence *drm_sched_fence_alloc(
On 12/02/2025 09:02, Philipp Stanner wrote: > btw. I still believe that it would be helpful (and congruent with the > established norm) to have the version in all patch titles. I do use > threaded view, but inboxes are huge, and everything that helps you > orient yourself is welcome > > On Fri, 2025-02-07 at 14:51 +0000, Tvrtko Ursulin wrote: >> Helper is for scheduler internal use so lets hide it from DRM drivers >> completely. >> >> At the same time we change the method of checking whethere there is >> anything in the queue from peeking to looking at the node count. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Danilo Krummrich <dakr@kernel.org> >> Cc: Matthew Brost <matthew.brost@intel.com> >> Cc: Philipp Stanner <phasta@kernel.org> >> --- >> drivers/gpu/drm/scheduler/sched_entity.c | 12 ------------ >> drivers/gpu/drm/scheduler/sched_internal.h | 13 +++++++++++++ >> include/drm/gpu_scheduler.h | 1 - >> 3 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c >> b/drivers/gpu/drm/scheduler/sched_entity.c >> index a171f05ad761..87f88259ddf6 100644 >> --- a/drivers/gpu/drm/scheduler/sched_entity.c >> +++ b/drivers/gpu/drm/scheduler/sched_entity.c >> @@ -151,18 +151,6 @@ static bool drm_sched_entity_is_idle(struct >> drm_sched_entity *entity) >> return false; >> } >> >> -/* Return true if entity could provide a job. */ >> -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity) >> -{ >> - if (spsc_queue_peek(&entity->job_queue) == NULL) >> - return false; >> - >> - if (READ_ONCE(entity->dependency)) >> - return false; >> - >> - return true; >> -} >> - >> /** >> * drm_sched_entity_error - return error of last scheduled job >> * @entity: scheduler entity to check >> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h >> b/drivers/gpu/drm/scheduler/sched_internal.h >> index 25ac62ac2bf3..9ff5cddb5708 100644 >> --- a/drivers/gpu/drm/scheduler/sched_internal.h >> +++ b/drivers/gpu/drm/scheduler/sched_internal.h >> @@ -43,4 +43,17 @@ drm_sched_entity_queue_peek(struct >> drm_sched_entity *entity) >> return container_of(node, struct drm_sched_job, queue_node); >> } >> >> +/* Return true if entity could provide a job. */ >> +static inline bool >> +drm_sched_entity_is_ready(struct drm_sched_entity *entity) >> +{ >> + if (!spsc_queue_count(&entity->job_queue)) >> + return false; >> + >> + if (READ_ONCE(entity->dependency)) >> + return false; >> + >> + return true; >> +} > > Is there any particular reason why you want the code in an header > instead of a new .c file? IMO it is too trivial to warrant emitting a function. > We might want to consider this, since I'd expect that the number of > internal helpers increases over the years. I actually hope the scheduler code gets smaller and simpler in the future. In any case the things hidden in sched_internal.h will be easy to refactor since it will be impossible for drivers to grow a dependency on them. Regards, Tvrtko >> + >> #endif >> diff --git a/include/drm/gpu_scheduler.h >> b/include/drm/gpu_scheduler.h >> index 5188c7f3bd3b..962825613596 100644 >> --- a/include/drm/gpu_scheduler.h >> +++ b/include/drm/gpu_scheduler.h >> @@ -606,7 +606,6 @@ struct drm_sched_job >> *drm_sched_entity_pop_job(struct drm_sched_entity *entity); >> void drm_sched_entity_push_job(struct drm_sched_job *sched_job); >> void drm_sched_entity_set_priority(struct drm_sched_entity *entity, >> enum drm_sched_priority >> priority); >> -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); >> int drm_sched_entity_error(struct drm_sched_entity *entity); >> >> struct drm_sched_fence *drm_sched_fence_alloc( >
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index a171f05ad761..87f88259ddf6 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -151,18 +151,6 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) return false; } -/* Return true if entity could provide a job. */ -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity) -{ - if (spsc_queue_peek(&entity->job_queue) == NULL) - return false; - - if (READ_ONCE(entity->dependency)) - return false; - - return true; -} - /** * drm_sched_entity_error - return error of last scheduled job * @entity: scheduler entity to check diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h index 25ac62ac2bf3..9ff5cddb5708 100644 --- a/drivers/gpu/drm/scheduler/sched_internal.h +++ b/drivers/gpu/drm/scheduler/sched_internal.h @@ -43,4 +43,17 @@ drm_sched_entity_queue_peek(struct drm_sched_entity *entity) return container_of(node, struct drm_sched_job, queue_node); } +/* Return true if entity could provide a job. */ +static inline bool +drm_sched_entity_is_ready(struct drm_sched_entity *entity) +{ + if (!spsc_queue_count(&entity->job_queue)) + return false; + + if (READ_ONCE(entity->dependency)) + return false; + + return true; +} + #endif diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 5188c7f3bd3b..962825613596 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -606,7 +606,6 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity); void drm_sched_entity_push_job(struct drm_sched_job *sched_job); void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); -bool drm_sched_entity_is_ready(struct drm_sched_entity *entity); int drm_sched_entity_error(struct drm_sched_entity *entity); struct drm_sched_fence *drm_sched_fence_alloc(
Helper is for scheduler internal use so lets hide it from DRM drivers completely. At the same time we change the method of checking whethere there is anything in the queue from peeking to looking at the node count. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Cc: Christian König <christian.koenig@amd.com> Cc: Danilo Krummrich <dakr@kernel.org> Cc: Matthew Brost <matthew.brost@intel.com> Cc: Philipp Stanner <phasta@kernel.org> --- drivers/gpu/drm/scheduler/sched_entity.c | 12 ------------ drivers/gpu/drm/scheduler/sched_internal.h | 13 +++++++++++++ include/drm/gpu_scheduler.h | 1 - 3 files changed, 13 insertions(+), 13 deletions(-)