| Message ID | 20250214101944.19390-2-tvrtko.ursulin@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/sched: Job queue peek/pop helpers and struct job re-order | expand |
Thx for the updated version. Overlooked it, I was out on Friday. See below On Fri, 2025-02-14 at 10:19 +0000, Tvrtko Ursulin wrote: > Idea is to add helpers for peeking and popping jobs from entities > with > the goal of decoupling the hidden assumption in the code that > queue_node > is the first element in struct drm_sched_job. > > That assumption usually comes in the form of: > > while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > >job_queue)))) > > Which breaks if the queue_node is re-positioned due to_drm_sched_job > being implemented with a container_of. > > This also allows us to remove duplicate definitions of > to_drm_sched_job. > > 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 | 11 +++--- > drivers/gpu/drm/scheduler/sched_internal.h | 46 > ++++++++++++++++++++++ > drivers/gpu/drm/scheduler/sched_main.c | 7 ++-- > 3 files changed, 54 insertions(+), 10 deletions(-) > create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > b/drivers/gpu/drm/scheduler/sched_entity.c > index 69bcf0e99d57..a171f05ad761 100644 > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > @@ -28,11 +28,10 @@ > #include <drm/drm_print.h> > #include <drm/gpu_scheduler.h> > > +#include "sched_internal.h" > + > #include "gpu_scheduler_trace.h" > > -#define to_drm_sched_job(sched_job) \ > - container_of((sched_job), struct drm_sched_job, > queue_node) > - > /** > * drm_sched_entity_init - Init a context entity used by scheduler > when > * submit to HW ring. > @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct > drm_sched_entity *entity) > /* The entity is guaranteed to not be used by the scheduler > */ > prev = rcu_dereference_check(entity->last_scheduled, true); > dma_fence_get(prev); > - while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > >job_queue)))) { > + while ((job = drm_sched_entity_queue_pop(entity))) { > struct drm_sched_fence *s_fence = job->s_fence; > > dma_fence_get(&s_fence->finished); > @@ -477,7 +476,7 @@ struct drm_sched_job > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > { > struct drm_sched_job *sched_job; > > - sched_job = to_drm_sched_job(spsc_queue_peek(&entity- > >job_queue)); > + sched_job = drm_sched_entity_queue_peek(entity); > if (!sched_job) > return NULL; > > @@ -513,7 +512,7 @@ struct drm_sched_job > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) { > struct drm_sched_job *next; > > - next = to_drm_sched_job(spsc_queue_peek(&entity- > >job_queue)); > + next = drm_sched_entity_queue_peek(entity); > if (next) { > struct drm_sched_rq *rq; > > diff --git a/drivers/gpu/drm/scheduler/sched_internal.h > b/drivers/gpu/drm/scheduler/sched_internal.h > new file mode 100644 > index 000000000000..815d384845a3 > --- /dev/null > +++ b/drivers/gpu/drm/scheduler/sched_internal.h > @@ -0,0 +1,46 @@ > +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_ > +#define _DRM_GPU_SCHEDULER_INTERNAL_H_ DRM maintainer tools complain about a potentially missing license identifier: -:80: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1 #80: FILE: drivers/gpu/drm/scheduler/sched_internal.h:1: The other scheduler files don't have one, either. Still, it might be good to add one for new files. So, shall we make it GPL? Rest of the series looks good. P. > + > +/** > + * drm_sched_entity_queue_pop - Low level helper for popping queued > jobs > + * > + * @entity: scheduler entity > + * > + * Low level helper for popping queued jobs. > + * > + * Returns: The job dequeued or NULL. > + */ > +static inline struct drm_sched_job * > +drm_sched_entity_queue_pop(struct drm_sched_entity *entity) > +{ > + struct spsc_node *node; > + > + node = spsc_queue_pop(&entity->job_queue); > + if (!node) > + return NULL; > + > + return container_of(node, struct drm_sched_job, queue_node); > +} > + > +/** > + * drm_sched_entity_queue_peek - Low level helper for peeking at the > job queue > + * > + * @entity: scheduler entity > + * > + * Low level helper for peeking at the job queue > + * > + * Returns: The job at the head of the queue or NULL. > + */ > +static inline struct drm_sched_job * > +drm_sched_entity_queue_peek(struct drm_sched_entity *entity) > +{ > + struct spsc_node *node; > + > + node = spsc_queue_peek(&entity->job_queue); > + if (!node) > + return NULL; > + > + return container_of(node, struct drm_sched_job, queue_node); > +} > + > +#endif > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > b/drivers/gpu/drm/scheduler/sched_main.c > index 8c36a59afb72..c634993f1346 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -78,6 +78,8 @@ > #include <drm/gpu_scheduler.h> > #include <drm/spsc_queue.h> > > +#include "sched_internal.h" > + > #define CREATE_TRACE_POINTS > #include "gpu_scheduler_trace.h" > > @@ -87,9 +89,6 @@ static struct lockdep_map drm_sched_lockdep_map = { > }; > #endif > > -#define to_drm_sched_job(sched_job) \ > - container_of((sched_job), struct drm_sched_job, > queue_node) > - > int drm_sched_policy = DRM_SCHED_POLICY_FIFO; > > /** > @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct > drm_gpu_scheduler *sched, > { > struct drm_sched_job *s_job; > > - s_job = to_drm_sched_job(spsc_queue_peek(&entity- > >job_queue)); > + s_job = drm_sched_entity_queue_peek(entity); > if (!s_job) > return false; >
On 18/02/2025 12:26, Philipp Stanner wrote: > Thx for the updated version. Overlooked it, I was out on Friday. See > below > > On Fri, 2025-02-14 at 10:19 +0000, Tvrtko Ursulin wrote: >> Idea is to add helpers for peeking and popping jobs from entities >> with >> the goal of decoupling the hidden assumption in the code that >> queue_node >> is the first element in struct drm_sched_job. >> >> That assumption usually comes in the form of: >> >> while ((job = to_drm_sched_job(spsc_queue_pop(&entity- >>> job_queue)))) >> >> Which breaks if the queue_node is re-positioned due to_drm_sched_job >> being implemented with a container_of. >> >> This also allows us to remove duplicate definitions of >> to_drm_sched_job. >> >> 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 | 11 +++--- >> drivers/gpu/drm/scheduler/sched_internal.h | 46 >> ++++++++++++++++++++++ >> drivers/gpu/drm/scheduler/sched_main.c | 7 ++-- >> 3 files changed, 54 insertions(+), 10 deletions(-) >> create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h >> >> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c >> b/drivers/gpu/drm/scheduler/sched_entity.c >> index 69bcf0e99d57..a171f05ad761 100644 >> --- a/drivers/gpu/drm/scheduler/sched_entity.c >> +++ b/drivers/gpu/drm/scheduler/sched_entity.c >> @@ -28,11 +28,10 @@ >> #include <drm/drm_print.h> >> #include <drm/gpu_scheduler.h> >> >> +#include "sched_internal.h" >> + >> #include "gpu_scheduler_trace.h" >> >> -#define to_drm_sched_job(sched_job) \ >> - container_of((sched_job), struct drm_sched_job, >> queue_node) >> - >> /** >> * drm_sched_entity_init - Init a context entity used by scheduler >> when >> * submit to HW ring. >> @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct >> drm_sched_entity *entity) >> /* The entity is guaranteed to not be used by the scheduler >> */ >> prev = rcu_dereference_check(entity->last_scheduled, true); >> dma_fence_get(prev); >> - while ((job = to_drm_sched_job(spsc_queue_pop(&entity- >>> job_queue)))) { >> + while ((job = drm_sched_entity_queue_pop(entity))) { >> struct drm_sched_fence *s_fence = job->s_fence; >> >> dma_fence_get(&s_fence->finished); >> @@ -477,7 +476,7 @@ struct drm_sched_job >> *drm_sched_entity_pop_job(struct drm_sched_entity *entity) >> { >> struct drm_sched_job *sched_job; >> >> - sched_job = to_drm_sched_job(spsc_queue_peek(&entity- >>> job_queue)); >> + sched_job = drm_sched_entity_queue_peek(entity); >> if (!sched_job) >> return NULL; >> >> @@ -513,7 +512,7 @@ struct drm_sched_job >> *drm_sched_entity_pop_job(struct drm_sched_entity *entity) >> if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) { >> struct drm_sched_job *next; >> >> - next = to_drm_sched_job(spsc_queue_peek(&entity- >>> job_queue)); >> + next = drm_sched_entity_queue_peek(entity); >> if (next) { >> struct drm_sched_rq *rq; >> >> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h >> b/drivers/gpu/drm/scheduler/sched_internal.h >> new file mode 100644 >> index 000000000000..815d384845a3 >> --- /dev/null >> +++ b/drivers/gpu/drm/scheduler/sched_internal.h >> @@ -0,0 +1,46 @@ >> +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_ >> +#define _DRM_GPU_SCHEDULER_INTERNAL_H_ > > DRM maintainer tools complain about a potentially missing license > identifier: > > -:80: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1 > #80: FILE: drivers/gpu/drm/scheduler/sched_internal.h:1: > > The other scheduler files don't have one, either. Still, it might be > good to add one for new files. So, shall we make it GPL? Ha, good question. And it is actually good I forgot to do this for this series (I was doing for unit tests last week, I mean adding SPDX lines) because, as sched_internal.h will take parts of gpu_scheduler.h which is not explicitly GPL, nor the other scheduler source files, apart from MODULE_LICENSE which is "GPL and additional rights", question indeed is what copyright blurb to put there. IANAL so not sure. Surely there is some established practice for cases like this one just I don't know what it is. Regards, Tvrtko > Rest of the series looks good. > > P. > >> + >> +/** >> + * drm_sched_entity_queue_pop - Low level helper for popping queued >> jobs >> + * >> + * @entity: scheduler entity >> + * >> + * Low level helper for popping queued jobs. >> + * >> + * Returns: The job dequeued or NULL. >> + */ >> +static inline struct drm_sched_job * >> +drm_sched_entity_queue_pop(struct drm_sched_entity *entity) >> +{ >> + struct spsc_node *node; >> + >> + node = spsc_queue_pop(&entity->job_queue); >> + if (!node) >> + return NULL; >> + >> + return container_of(node, struct drm_sched_job, queue_node); >> +} >> + >> +/** >> + * drm_sched_entity_queue_peek - Low level helper for peeking at the >> job queue >> + * >> + * @entity: scheduler entity >> + * >> + * Low level helper for peeking at the job queue >> + * >> + * Returns: The job at the head of the queue or NULL. >> + */ >> +static inline struct drm_sched_job * >> +drm_sched_entity_queue_peek(struct drm_sched_entity *entity) >> +{ >> + struct spsc_node *node; >> + >> + node = spsc_queue_peek(&entity->job_queue); >> + if (!node) >> + return NULL; >> + >> + return container_of(node, struct drm_sched_job, queue_node); >> +} >> + >> +#endif >> diff --git a/drivers/gpu/drm/scheduler/sched_main.c >> b/drivers/gpu/drm/scheduler/sched_main.c >> index 8c36a59afb72..c634993f1346 100644 >> --- a/drivers/gpu/drm/scheduler/sched_main.c >> +++ b/drivers/gpu/drm/scheduler/sched_main.c >> @@ -78,6 +78,8 @@ >> #include <drm/gpu_scheduler.h> >> #include <drm/spsc_queue.h> >> >> +#include "sched_internal.h" >> + >> #define CREATE_TRACE_POINTS >> #include "gpu_scheduler_trace.h" >> >> @@ -87,9 +89,6 @@ static struct lockdep_map drm_sched_lockdep_map = { >> }; >> #endif >> >> -#define to_drm_sched_job(sched_job) \ >> - container_of((sched_job), struct drm_sched_job, >> queue_node) >> - >> int drm_sched_policy = DRM_SCHED_POLICY_FIFO; >> >> /** >> @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct >> drm_gpu_scheduler *sched, >> { >> struct drm_sched_job *s_job; >> >> - s_job = to_drm_sched_job(spsc_queue_peek(&entity- >>> job_queue)); >> + s_job = drm_sched_entity_queue_peek(entity); >> if (!s_job) >> return false; >> >
On Tue, Feb 18, 2025 at 06:26:15PM +0000, Tvrtko Ursulin wrote: > > On 18/02/2025 12:26, Philipp Stanner wrote: > > Thx for the updated version. Overlooked it, I was out on Friday. See > > below > > > > On Fri, 2025-02-14 at 10:19 +0000, Tvrtko Ursulin wrote: > > > Idea is to add helpers for peeking and popping jobs from entities > > > with > > > the goal of decoupling the hidden assumption in the code that > > > queue_node > > > is the first element in struct drm_sched_job. > > > > > > That assumption usually comes in the form of: > > > > > > while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > > > > job_queue)))) > > > > > > Which breaks if the queue_node is re-positioned due to_drm_sched_job > > > being implemented with a container_of. > > > > > > This also allows us to remove duplicate definitions of > > > to_drm_sched_job. > > > > > > 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 | 11 +++--- > > > drivers/gpu/drm/scheduler/sched_internal.h | 46 > > > ++++++++++++++++++++++ > > > drivers/gpu/drm/scheduler/sched_main.c | 7 ++-- > > > 3 files changed, 54 insertions(+), 10 deletions(-) > > > create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > > > b/drivers/gpu/drm/scheduler/sched_entity.c > > > index 69bcf0e99d57..a171f05ad761 100644 > > > --- a/drivers/gpu/drm/scheduler/sched_entity.c > > > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > > > @@ -28,11 +28,10 @@ > > > #include <drm/drm_print.h> > > > #include <drm/gpu_scheduler.h> > > > +#include "sched_internal.h" > > > + > > > #include "gpu_scheduler_trace.h" > > > -#define to_drm_sched_job(sched_job) \ > > > - container_of((sched_job), struct drm_sched_job, > > > queue_node) > > > - > > > /** > > > * drm_sched_entity_init - Init a context entity used by scheduler > > > when > > > * submit to HW ring. > > > @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct > > > drm_sched_entity *entity) > > > /* The entity is guaranteed to not be used by the scheduler > > > */ > > > prev = rcu_dereference_check(entity->last_scheduled, true); > > > dma_fence_get(prev); > > > - while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > > > > job_queue)))) { > > > + while ((job = drm_sched_entity_queue_pop(entity))) { > > > struct drm_sched_fence *s_fence = job->s_fence; > > > dma_fence_get(&s_fence->finished); > > > @@ -477,7 +476,7 @@ struct drm_sched_job > > > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > > > { > > > struct drm_sched_job *sched_job; > > > - sched_job = to_drm_sched_job(spsc_queue_peek(&entity- > > > > job_queue)); > > > + sched_job = drm_sched_entity_queue_peek(entity); > > > if (!sched_job) > > > return NULL; > > > @@ -513,7 +512,7 @@ struct drm_sched_job > > > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > > > if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) { > > > struct drm_sched_job *next; > > > - next = to_drm_sched_job(spsc_queue_peek(&entity- > > > > job_queue)); > > > + next = drm_sched_entity_queue_peek(entity); > > > if (next) { > > > struct drm_sched_rq *rq; > > > diff --git a/drivers/gpu/drm/scheduler/sched_internal.h > > > b/drivers/gpu/drm/scheduler/sched_internal.h > > > new file mode 100644 > > > index 000000000000..815d384845a3 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/scheduler/sched_internal.h > > > @@ -0,0 +1,46 @@ > > > +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_ > > > +#define _DRM_GPU_SCHEDULER_INTERNAL_H_ > > > > DRM maintainer tools complain about a potentially missing license > > identifier: > > > > -:80: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1 > > #80: FILE: drivers/gpu/drm/scheduler/sched_internal.h:1: > > > > The other scheduler files don't have one, either. Still, it might be > > good to add one for new files. So, shall we make it GPL? > > Ha, good question. And it is actually good I forgot to do this for this > series (I was doing for unit tests last week, I mean adding SPDX lines) > because, as sched_internal.h will take parts of gpu_scheduler.h which is not > explicitly GPL, nor the other scheduler source files, apart from > MODULE_LICENSE which is "GPL and additional rights", question indeed is what > copyright blurb to put there. IANAL so not sure. Surely there is some > established practice for cases like this one just I don't know what it is. > I think GPL or MIT would be the preference given [1]. Also ofc IANAL. Matt [1] https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3852 > Regards, > > Tvrtko > > > Rest of the series looks good. > > > > P. > > > > > + > > > +/** > > > + * drm_sched_entity_queue_pop - Low level helper for popping queued > > > jobs > > > + * > > > + * @entity: scheduler entity > > > + * > > > + * Low level helper for popping queued jobs. > > > + * > > > + * Returns: The job dequeued or NULL. > > > + */ > > > +static inline struct drm_sched_job * > > > +drm_sched_entity_queue_pop(struct drm_sched_entity *entity) > > > +{ > > > + struct spsc_node *node; > > > + > > > + node = spsc_queue_pop(&entity->job_queue); > > > + if (!node) > > > + return NULL; > > > + > > > + return container_of(node, struct drm_sched_job, queue_node); > > > +} > > > + > > > +/** > > > + * drm_sched_entity_queue_peek - Low level helper for peeking at the > > > job queue > > > + * > > > + * @entity: scheduler entity > > > + * > > > + * Low level helper for peeking at the job queue > > > + * > > > + * Returns: The job at the head of the queue or NULL. > > > + */ > > > +static inline struct drm_sched_job * > > > +drm_sched_entity_queue_peek(struct drm_sched_entity *entity) > > > +{ > > > + struct spsc_node *node; > > > + > > > + node = spsc_queue_peek(&entity->job_queue); > > > + if (!node) > > > + return NULL; > > > + > > > + return container_of(node, struct drm_sched_job, queue_node); > > > +} > > > + > > > +#endif > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > index 8c36a59afb72..c634993f1346 100644 > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > @@ -78,6 +78,8 @@ > > > #include <drm/gpu_scheduler.h> > > > #include <drm/spsc_queue.h> > > > +#include "sched_internal.h" > > > + > > > #define CREATE_TRACE_POINTS > > > #include "gpu_scheduler_trace.h" > > > @@ -87,9 +89,6 @@ static struct lockdep_map drm_sched_lockdep_map = { > > > }; > > > #endif > > > -#define to_drm_sched_job(sched_job) \ > > > - container_of((sched_job), struct drm_sched_job, > > > queue_node) > > > - > > > int drm_sched_policy = DRM_SCHED_POLICY_FIFO; > > > /** > > > @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct > > > drm_gpu_scheduler *sched, > > > { > > > struct drm_sched_job *s_job; > > > - s_job = to_drm_sched_job(spsc_queue_peek(&entity- > > > > job_queue)); > > > + s_job = drm_sched_entity_queue_peek(entity); > > > if (!s_job) > > > return false; > > >
On Tue, 2025-02-18 at 14:17 -0800, Matthew Brost wrote: > On Tue, Feb 18, 2025 at 06:26:15PM +0000, Tvrtko Ursulin wrote: > > > > On 18/02/2025 12:26, Philipp Stanner wrote: > > > Thx for the updated version. Overlooked it, I was out on Friday. > > > See > > > below > > > > > > On Fri, 2025-02-14 at 10:19 +0000, Tvrtko Ursulin wrote: > > > > Idea is to add helpers for peeking and popping jobs from > > > > entities > > > > with > > > > the goal of decoupling the hidden assumption in the code that > > > > queue_node > > > > is the first element in struct drm_sched_job. > > > > > > > > That assumption usually comes in the form of: > > > > > > > > while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > > > > > job_queue)))) > > > > > > > > Which breaks if the queue_node is re-positioned due > > > > to_drm_sched_job > > > > being implemented with a container_of. > > > > > > > > This also allows us to remove duplicate definitions of > > > > to_drm_sched_job. > > > > > > > > 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 | 11 +++--- > > > > drivers/gpu/drm/scheduler/sched_internal.h | 46 > > > > ++++++++++++++++++++++ > > > > drivers/gpu/drm/scheduler/sched_main.c | 7 ++-- > > > > 3 files changed, 54 insertions(+), 10 deletions(-) > > > > create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h > > > > > > > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > > > > b/drivers/gpu/drm/scheduler/sched_entity.c > > > > index 69bcf0e99d57..a171f05ad761 100644 > > > > --- a/drivers/gpu/drm/scheduler/sched_entity.c > > > > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > > > > @@ -28,11 +28,10 @@ > > > > #include <drm/drm_print.h> > > > > #include <drm/gpu_scheduler.h> > > > > +#include "sched_internal.h" > > > > + > > > > #include "gpu_scheduler_trace.h" > > > > -#define to_drm_sched_job(sched_job) \ > > > > - container_of((sched_job), struct > > > > drm_sched_job, > > > > queue_node) > > > > - > > > > /** > > > > * drm_sched_entity_init - Init a context entity used by > > > > scheduler > > > > when > > > > * submit to HW ring. > > > > @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct > > > > drm_sched_entity *entity) > > > > /* The entity is guaranteed to not be used by the > > > > scheduler > > > > */ > > > > prev = rcu_dereference_check(entity->last_scheduled, > > > > true); > > > > dma_fence_get(prev); > > > > - while ((job = to_drm_sched_job(spsc_queue_pop(&entity- > > > > > job_queue)))) { > > > > + while ((job = drm_sched_entity_queue_pop(entity))) { > > > > struct drm_sched_fence *s_fence = job- > > > > >s_fence; > > > > dma_fence_get(&s_fence->finished); > > > > @@ -477,7 +476,7 @@ struct drm_sched_job > > > > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > > > > { > > > > struct drm_sched_job *sched_job; > > > > - sched_job = to_drm_sched_job(spsc_queue_peek(&entity- > > > > > job_queue)); > > > > + sched_job = drm_sched_entity_queue_peek(entity); > > > > if (!sched_job) > > > > return NULL; > > > > @@ -513,7 +512,7 @@ struct drm_sched_job > > > > *drm_sched_entity_pop_job(struct drm_sched_entity *entity) > > > > if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) { > > > > struct drm_sched_job *next; > > > > - next = > > > > to_drm_sched_job(spsc_queue_peek(&entity- > > > > > job_queue)); > > > > + next = drm_sched_entity_queue_peek(entity); > > > > if (next) { > > > > struct drm_sched_rq *rq; > > > > diff --git a/drivers/gpu/drm/scheduler/sched_internal.h > > > > b/drivers/gpu/drm/scheduler/sched_internal.h > > > > new file mode 100644 > > > > index 000000000000..815d384845a3 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/scheduler/sched_internal.h > > > > @@ -0,0 +1,46 @@ > > > > +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_ > > > > +#define _DRM_GPU_SCHEDULER_INTERNAL_H_ > > > > > > DRM maintainer tools complain about a potentially missing license > > > identifier: > > > > > > -:80: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX- > > > License-Identifier tag in line 1 > > > #80: FILE: drivers/gpu/drm/scheduler/sched_internal.h:1: > > > > > > The other scheduler files don't have one, either. Still, it might > > > be > > > good to add one for new files. So, shall we make it GPL? > > > > Ha, good question. And it is actually good I forgot to do this for > > this > > series (I was doing for unit tests last week, I mean adding SPDX > > lines) > > because, as sched_internal.h will take parts of gpu_scheduler.h > > which is not > > explicitly GPL, nor the other scheduler source files, apart from > > MODULE_LICENSE which is "GPL and additional rights", question > > indeed is what > > copyright blurb to put there. IANAL so not sure. Surely there is > > some > > established practice for cases like this one just I don't know what > > it is. > > > > I think GPL or MIT would be the preference given [1]. Also ofc IANAL. > > Matt > > [1] https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3852 Well, I briefly synced with Dave – since all our scheduler files do contain the full MIT license header (with AMD's copyright), but no SPDX, we should probably go for MIT with all SPDX identifiers. P. > > > Regards, > > > > Tvrtko > > > > > Rest of the series looks good. > > > > > > P. > > > > > > > + > > > > +/** > > > > + * drm_sched_entity_queue_pop - Low level helper for popping > > > > queued > > > > jobs > > > > + * > > > > + * @entity: scheduler entity > > > > + * > > > > + * Low level helper for popping queued jobs. > > > > + * > > > > + * Returns: The job dequeued or NULL. > > > > + */ > > > > +static inline struct drm_sched_job * > > > > +drm_sched_entity_queue_pop(struct drm_sched_entity *entity) > > > > +{ > > > > + struct spsc_node *node; > > > > + > > > > + node = spsc_queue_pop(&entity->job_queue); > > > > + if (!node) > > > > + return NULL; > > > > + > > > > + return container_of(node, struct drm_sched_job, > > > > queue_node); > > > > +} > > > > + > > > > +/** > > > > + * drm_sched_entity_queue_peek - Low level helper for peeking > > > > at the > > > > job queue > > > > + * > > > > + * @entity: scheduler entity > > > > + * > > > > + * Low level helper for peeking at the job queue > > > > + * > > > > + * Returns: The job at the head of the queue or NULL. > > > > + */ > > > > +static inline struct drm_sched_job * > > > > +drm_sched_entity_queue_peek(struct drm_sched_entity *entity) > > > > +{ > > > > + struct spsc_node *node; > > > > + > > > > + node = spsc_queue_peek(&entity->job_queue); > > > > + if (!node) > > > > + return NULL; > > > > + > > > > + return container_of(node, struct drm_sched_job, > > > > queue_node); > > > > +} > > > > + > > > > +#endif > > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > > > > b/drivers/gpu/drm/scheduler/sched_main.c > > > > index 8c36a59afb72..c634993f1346 100644 > > > > --- a/drivers/gpu/drm/scheduler/sched_main.c > > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c > > > > @@ -78,6 +78,8 @@ > > > > #include <drm/gpu_scheduler.h> > > > > #include <drm/spsc_queue.h> > > > > +#include "sched_internal.h" > > > > + > > > > #define CREATE_TRACE_POINTS > > > > #include "gpu_scheduler_trace.h" > > > > @@ -87,9 +89,6 @@ static struct lockdep_map > > > > drm_sched_lockdep_map = { > > > > }; > > > > #endif > > > > -#define to_drm_sched_job(sched_job) \ > > > > - container_of((sched_job), struct > > > > drm_sched_job, > > > > queue_node) > > > > - > > > > int drm_sched_policy = DRM_SCHED_POLICY_FIFO; > > > > /** > > > > @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct > > > > drm_gpu_scheduler *sched, > > > > { > > > > struct drm_sched_job *s_job; > > > > - s_job = to_drm_sched_job(spsc_queue_peek(&entity- > > > > > job_queue)); > > > > + s_job = drm_sched_entity_queue_peek(entity); > > > > if (!s_job) > > > > return false; > > > > >
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 69bcf0e99d57..a171f05ad761 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -28,11 +28,10 @@ #include <drm/drm_print.h> #include <drm/gpu_scheduler.h> +#include "sched_internal.h" + #include "gpu_scheduler_trace.h" -#define to_drm_sched_job(sched_job) \ - container_of((sched_job), struct drm_sched_job, queue_node) - /** * drm_sched_entity_init - Init a context entity used by scheduler when * submit to HW ring. @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity) /* The entity is guaranteed to not be used by the scheduler */ prev = rcu_dereference_check(entity->last_scheduled, true); dma_fence_get(prev); - while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) { + while ((job = drm_sched_entity_queue_pop(entity))) { struct drm_sched_fence *s_fence = job->s_fence; dma_fence_get(&s_fence->finished); @@ -477,7 +476,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) { struct drm_sched_job *sched_job; - sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue)); + sched_job = drm_sched_entity_queue_peek(entity); if (!sched_job) return NULL; @@ -513,7 +512,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) { struct drm_sched_job *next; - next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue)); + next = drm_sched_entity_queue_peek(entity); if (next) { struct drm_sched_rq *rq; diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h new file mode 100644 index 000000000000..815d384845a3 --- /dev/null +++ b/drivers/gpu/drm/scheduler/sched_internal.h @@ -0,0 +1,46 @@ +#ifndef _DRM_GPU_SCHEDULER_INTERNAL_H_ +#define _DRM_GPU_SCHEDULER_INTERNAL_H_ + +/** + * drm_sched_entity_queue_pop - Low level helper for popping queued jobs + * + * @entity: scheduler entity + * + * Low level helper for popping queued jobs. + * + * Returns: The job dequeued or NULL. + */ +static inline struct drm_sched_job * +drm_sched_entity_queue_pop(struct drm_sched_entity *entity) +{ + struct spsc_node *node; + + node = spsc_queue_pop(&entity->job_queue); + if (!node) + return NULL; + + return container_of(node, struct drm_sched_job, queue_node); +} + +/** + * drm_sched_entity_queue_peek - Low level helper for peeking at the job queue + * + * @entity: scheduler entity + * + * Low level helper for peeking at the job queue + * + * Returns: The job at the head of the queue or NULL. + */ +static inline struct drm_sched_job * +drm_sched_entity_queue_peek(struct drm_sched_entity *entity) +{ + struct spsc_node *node; + + node = spsc_queue_peek(&entity->job_queue); + if (!node) + return NULL; + + return container_of(node, struct drm_sched_job, queue_node); +} + +#endif diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 8c36a59afb72..c634993f1346 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -78,6 +78,8 @@ #include <drm/gpu_scheduler.h> #include <drm/spsc_queue.h> +#include "sched_internal.h" + #define CREATE_TRACE_POINTS #include "gpu_scheduler_trace.h" @@ -87,9 +89,6 @@ static struct lockdep_map drm_sched_lockdep_map = { }; #endif -#define to_drm_sched_job(sched_job) \ - container_of((sched_job), struct drm_sched_job, queue_node) - int drm_sched_policy = DRM_SCHED_POLICY_FIFO; /** @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched, { struct drm_sched_job *s_job; - s_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue)); + s_job = drm_sched_entity_queue_peek(entity); if (!s_job) return false;
Idea is to add helpers for peeking and popping jobs from entities with the goal of decoupling the hidden assumption in the code that queue_node is the first element in struct drm_sched_job. That assumption usually comes in the form of: while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) Which breaks if the queue_node is re-positioned due to_drm_sched_job being implemented with a container_of. This also allows us to remove duplicate definitions of to_drm_sched_job. 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 | 11 +++--- drivers/gpu/drm/scheduler/sched_internal.h | 46 ++++++++++++++++++++++ drivers/gpu/drm/scheduler/sched_main.c | 7 ++-- 3 files changed, 54 insertions(+), 10 deletions(-) create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h