On Thu, Feb 06, 2025 at 04:40:29PM +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.ursu...@igalia.com>
> Cc: Christian König <christian.koe...@amd.com>
> Cc: Danilo Krummrich <d...@kernel.org>
> Cc: Matthew Brost <matthew.br...@intel.com>
> Cc: Philipp Stanner <pha...@kernel.org>
> ---
> drivers/gpu/drm/scheduler/sched_entity.c | 11 +++---
> drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++++++
> drivers/gpu/drm/scheduler/sched_main.c | 7 ++--
> 3 files changed, 51 insertions(+), 10 deletions(-)
> create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
>
> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h
> b/drivers/gpu/drm/scheduler/sched_internal.h
> new file mode 100644
> index 000000000000..565c83e32371
> --- /dev/null
> +++ b/drivers/gpu/drm/scheduler/sched_internal.h
> @@ -0,0 +1,43 @@
> +
> +
> +/**
> + * __drm_sched_entity_queue_pop - Low level helper for popping queued jobs
Why the '__' prefix?
Could also use this to replace spsc_queue_pop() in drm_sched_entity_pop_job().
> + *
> + * @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
Same here.
Could also use this for drm_sched_entity_is_ready().
> + *
> + * @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);
> +}
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index a48be16ab84f..43ca98e8db5f 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;
>
> --
> 2.48.0
>
