* [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe
@ 2025-10-03 20:11 Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator Matthew Brost
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Matthew Brost @ 2025-10-03 20:11 UTC (permalink / raw)
To: intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
At XDC, we discussed that drivers should avoid accessing DRM scheduler
internals, misusing DRM scheduler locks, and adopt a well-defined
pending job list iterator. This series proposes the necessary changes to
the DRM scheduler to bring Xe in line with that agreement and updates Xe
to use the new DRM scheduler API.
This is being sent as an RFC since only Xe is updated in this series. If
consensus is reached, a follow-up series can address other drivers in
the subsystem.
v2:
- Fix checkpatch / naming issues
Matt
Matthew Brost (4):
drm/sched: Add pending job list iterator
drm/sched: Add several job helpers to avoid drivers touching scheduler
state
drm/xe: Add dedicated message lock
drm/xe: Stop abusing DRM scheduler internals
drivers/gpu/drm/xe/xe_gpu_scheduler.c | 3 +-
drivers/gpu/drm/xe/xe_gpu_scheduler.h | 31 ++----
drivers/gpu/drm/xe/xe_gpu_scheduler_types.h | 2 +
drivers/gpu/drm/xe/xe_guc_submit.c | 34 +++---
drivers/gpu/drm/xe/xe_guc_submit_types.h | 1 -
include/drm/gpu_scheduler.h | 116 ++++++++++++++++++++
6 files changed, 142 insertions(+), 45 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator
2025-10-03 20:11 [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe Matthew Brost
@ 2025-10-03 20:11 ` Matthew Brost
2025-10-06 9:19 ` Jani Nikula
2025-10-03 20:11 ` [RFC PATCH v2 2/4] drm/sched: Add several job helpers to avoid drivers touching scheduler state Matthew Brost
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Matthew Brost @ 2025-10-03 20:11 UTC (permalink / raw)
To: intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
Stop open coding pending job list in drivers. Add pending job list
iterator which safely walks DRM scheduler list either locklessly
asserting DRM scheduler is stopped or takes pending job list lock.
v2:
- Fix checkpatch (CI)
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
include/drm/gpu_scheduler.h | 64 +++++++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index fb88301b3c45..bb49d8b715eb 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -698,4 +698,68 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
struct drm_gpu_scheduler **sched_list,
unsigned int num_sched_list);
+/* Inlines */
+
+/**
+ * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
+ * @sched: DRM scheduler associated with pending job iterator
+ * @stopped: DRM scheduler stopped state associated with pending job iterator
+ */
+struct drm_sched_pending_job_iter {
+ struct drm_gpu_scheduler *sched;
+ bool stopped;
+};
+
+/* Drivers should never call this directly */
+static inline struct drm_sched_pending_job_iter
+__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched, bool stopped)
+{
+ struct drm_sched_pending_job_iter iter = {
+ .sched = sched,
+ .stopped = stopped,
+ };
+
+ if (stopped)
+ WARN_ON(!READ_ONCE(sched->pause_submit));
+ else
+ spin_lock(&sched->job_list_lock);
+
+ return iter;
+}
+
+/* Drivers should never call this directly */
+static inline void
+__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
+{
+ if (iter.stopped)
+ WARN_ON(!READ_ONCE(iter.sched->pause_submit));
+ else
+ spin_unlock(&iter.sched->job_list_lock);
+}
+
+DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
+ __drm_sched_pending_job_iter_end(_T),
+ __drm_sched_pending_job_iter_begin(__sched, __stopped),
+ struct drm_gpu_scheduler *__sched, bool __stopped);
+static inline void
+*class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
+{return _T; }
+#define class_drm_sched_pending_job_iter_is_conditional false
+
+/**
+ * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
+ * @__job: Current pending job being iterated over
+ * @__sched: DRM scheduler to iterate over pending jobs
+ * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
+ * @__stopped: DRM scheduler stopped state
+ *
+ * Iterator for each pending job in scheduler, filtering on an entity, and
+ * enforcing locking rules (either scheduler fully stopped or correctly takes
+ * job_list_lock).
+ */
+#define drm_sched_for_each_pending_job(__job, __sched, __entity, __stopped) \
+ scoped_guard(drm_sched_pending_job_iter, (__sched), (__stopped)) \
+ list_for_each_entry((__job), &(__sched)->pending_list, list) \
+ for_each_if(!(__entity) || (__job)->entity == (__entity))
+
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC PATCH v2 2/4] drm/sched: Add several job helpers to avoid drivers touching scheduler state
2025-10-03 20:11 [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator Matthew Brost
@ 2025-10-03 20:11 ` Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 3/4] drm/xe: Add dedicated message lock Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 4/4] drm/xe: Stop abusing DRM scheduler internals Matthew Brost
3 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2025-10-03 20:11 UTC (permalink / raw)
To: intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
Add helpers to find first pending job, pending job count, and a jobs
signaled state. Expected to used driver side on recovery and debug
flows.
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
include/drm/gpu_scheduler.h | 52 +++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index bb49d8b715eb..e1e264c2043e 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -762,4 +762,56 @@ static inline void
list_for_each_entry((__job), &(__sched)->pending_list, list) \
for_each_if(!(__entity) || (__job)->entity == (__entity))
+/**
+ * drm_sched_first_pending_job() - DRM scheduler first pending job
+ * @sched: DRM scheduler
+ *
+ * Find DRM scheduler first pending job, drivers should only call this function
+ * when scheduler is stopped or job can immediately disappear resulting in a
+ * UAF.
+ *
+ * Return: First pending job or NULL
+ */
+static inline struct drm_sched_job *
+drm_sched_first_pending_job(struct drm_gpu_scheduler *sched)
+{
+ WARN_ON(!READ_ONCE(sched->pause_submit));
+ guard(spinlock)(&sched->job_list_lock);
+ return list_first_entry_or_null(&sched->pending_list,
+ struct drm_sched_job, list);
+}
+
+/**
+ * drm_sched_pending_job_count() - DRM scheduler pending job count
+ * @sched: DRM scheduler
+ *
+ * Determine DRM scheduler pending job count. If scheduler if not stopped, this
+ * value can immediately change, thus drivers must guard against using to count
+ * to anything memory unsafe. Use with caution.
+ *
+ * Return: Number of pending jobs
+ */
+static inline int drm_sched_pending_job_count(struct drm_gpu_scheduler *sched)
+{
+ guard(spinlock)(&sched->job_list_lock);
+ return list_count_nodes(&sched->pending_list);
+}
+
+/**
+ * drm_sched_job_is_signaled() - DRM scheduler job is signaled
+ * @job: DRM scheduler job
+ *
+ * Determine if DRM scheduler job is signaled. DRM scheduler should be stopped
+ * to obtain a stable snapshot of state.
+ *
+ * Return: True if job is signaled, False otherwise
+ */
+static inline bool drm_sched_job_is_signaled(struct drm_sched_job *job)
+{
+ struct drm_sched_fence *s_fence = job->s_fence;
+
+ WARN_ON(!READ_ONCE(job->sched->pause_submit));
+ return s_fence->parent && dma_fence_is_signaled(s_fence->parent);
+}
+
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC PATCH v2 3/4] drm/xe: Add dedicated message lock
2025-10-03 20:11 [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 2/4] drm/sched: Add several job helpers to avoid drivers touching scheduler state Matthew Brost
@ 2025-10-03 20:11 ` Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 4/4] drm/xe: Stop abusing DRM scheduler internals Matthew Brost
3 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2025-10-03 20:11 UTC (permalink / raw)
To: intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
Stop abusing DRM scheduler job list lock for messages, add dedicated
message lock.
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
drivers/gpu/drm/xe/xe_gpu_scheduler.c | 3 ++-
drivers/gpu/drm/xe/xe_gpu_scheduler.h | 4 ++--
drivers/gpu/drm/xe/xe_gpu_scheduler_types.h | 2 ++
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
index 455ccaf17314..bfd1aebe70e6 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
@@ -77,6 +77,7 @@ int xe_sched_init(struct xe_gpu_scheduler *sched,
};
sched->ops = xe_ops;
+ spin_lock_init(&sched->msg_lock);
INIT_LIST_HEAD(&sched->msgs);
INIT_WORK(&sched->work_process_msg, xe_sched_process_msg_work);
@@ -130,7 +131,7 @@ void xe_sched_add_msg(struct xe_gpu_scheduler *sched,
void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
struct xe_sched_msg *msg)
{
- lockdep_assert_held(&sched->base.job_list_lock);
+ lockdep_assert_held(&sched->msg_lock);
list_add_tail(&msg->link, &sched->msgs);
xe_sched_process_msg_queue(sched);
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
index e548b2aed95a..04f85c4f7e80 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
@@ -32,12 +32,12 @@ void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
static inline void xe_sched_msg_lock(struct xe_gpu_scheduler *sched)
{
- spin_lock(&sched->base.job_list_lock);
+ spin_lock(&sched->msg_lock);
}
static inline void xe_sched_msg_unlock(struct xe_gpu_scheduler *sched)
{
- spin_unlock(&sched->base.job_list_lock);
+ spin_unlock(&sched->msg_lock);
}
static inline void xe_sched_stop(struct xe_gpu_scheduler *sched)
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
index 6731b13da8bb..63d9bf92583c 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
@@ -47,6 +47,8 @@ struct xe_gpu_scheduler {
const struct xe_sched_backend_ops *ops;
/** @msgs: list of messages to be processed in @work_process_msg */
struct list_head msgs;
+ /** @msg_lock: Message lock */
+ spinlock_t msg_lock;
/** @work_process_msg: processes messages */
struct work_struct work_process_msg;
};
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [RFC PATCH v2 4/4] drm/xe: Stop abusing DRM scheduler internals
2025-10-03 20:11 [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe Matthew Brost
` (2 preceding siblings ...)
2025-10-03 20:11 ` [RFC PATCH v2 3/4] drm/xe: Add dedicated message lock Matthew Brost
@ 2025-10-03 20:11 ` Matthew Brost
3 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2025-10-03 20:11 UTC (permalink / raw)
To: intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
Use new pending job list iterator and new helper functions in Xe to
avoid reaching into DRM scheduler internals.
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
drivers/gpu/drm/xe/xe_gpu_scheduler.h | 27 ++++---------------
drivers/gpu/drm/xe/xe_guc_submit.c | 34 +++++++++++-------------
drivers/gpu/drm/xe/xe_guc_submit_types.h | 1 -
3 files changed, 20 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
index 04f85c4f7e80..e077519abf11 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
@@ -7,7 +7,7 @@
#define _XE_GPU_SCHEDULER_H_
#include "xe_gpu_scheduler_types.h"
-#include "xe_sched_job_types.h"
+#include "xe_sched_job.h"
int xe_sched_init(struct xe_gpu_scheduler *sched,
const struct drm_sched_backend_ops *ops,
@@ -54,13 +54,9 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
{
struct drm_sched_job *s_job;
- list_for_each_entry(s_job, &sched->base.pending_list, list) {
- struct drm_sched_fence *s_fence = s_job->s_fence;
- struct dma_fence *hw_fence = s_fence->parent;
-
- if (hw_fence && !dma_fence_is_signaled(hw_fence))
+ drm_sched_for_each_pending_job(s_job, &sched->base, NULL, true)
+ if (!drm_sched_job_is_signaled(s_job))
sched->base.ops->run_job(s_job);
- }
}
static inline bool
@@ -69,25 +65,12 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
return drm_sched_invalidate_job(&job->drm, threshold);
}
-static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
- struct xe_sched_job *job)
-{
- spin_lock(&sched->base.job_list_lock);
- list_add(&job->drm.list, &sched->base.pending_list);
- spin_unlock(&sched->base.job_list_lock);
-}
-
static inline
struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
{
- struct xe_sched_job *job;
-
- spin_lock(&sched->base.job_list_lock);
- job = list_first_entry_or_null(&sched->base.pending_list,
- struct xe_sched_job, drm.list);
- spin_unlock(&sched->base.job_list_lock);
+ struct drm_sched_job *job = drm_sched_first_pending_job(&sched->base);
- return job;
+ return job ? to_xe_sched_job(job) : NULL;
}
static inline int
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 53024eb5670b..da13c1380cb3 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1217,7 +1217,7 @@ static enum drm_gpu_sched_stat
guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
{
struct xe_sched_job *job = to_xe_sched_job(drm_job);
- struct xe_sched_job *tmp_job;
+ struct drm_sched_job *tmp_job;
struct xe_exec_queue *q = job->q;
struct xe_gpu_scheduler *sched = &q->guc->sched;
struct xe_guc *guc = exec_queue_to_guc(q);
@@ -1226,7 +1226,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
unsigned int fw_ref;
int err = -ETIME;
pid_t pid = -1;
- int i = 0;
bool wedged = false, skip_timeout_check;
/*
@@ -1391,21 +1390,19 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
* Fence state now stable, stop / start scheduler which cleans up any
* fences that are complete
*/
- xe_sched_add_pending_job(sched, job);
+ xe_sched_job_set_error(job, err);
xe_sched_submission_start(sched);
xe_guc_exec_queue_trigger_cleanup(q);
/* Mark all outstanding jobs as bad, thus completing them */
- spin_lock(&sched->base.job_list_lock);
- list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
- xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
- spin_unlock(&sched->base.job_list_lock);
+ drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL, false)
+ xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
/* Start fence signaling */
xe_hw_fence_irq_start(q->fence_irq);
- return DRM_GPU_SCHED_STAT_RESET;
+ return DRM_GPU_SCHED_STAT_NO_HANG;
sched_enable:
enable_scheduling(q);
@@ -2478,30 +2475,30 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
if (snapshot->parallel_execution)
guc_exec_queue_wq_snapshot_capture(q, snapshot);
- spin_lock(&sched->base.job_list_lock);
- snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
+ snapshot->pending_list_size = drm_sched_pending_job_count(&sched->base);
snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
sizeof(struct pending_list_snapshot),
GFP_ATOMIC);
if (snapshot->pending_list) {
struct xe_sched_job *job_iter;
+ struct drm_sched_job *drm_job;
i = 0;
- list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
+ drm_sched_for_each_pending_job(drm_job, &sched->base, NULL, false) {
+ job_iter = to_xe_sched_job(drm_job);
+
+ if (i >= snapshot->pending_list_size)
+ break;
+
snapshot->pending_list[i].seqno =
xe_sched_job_seqno(job_iter);
snapshot->pending_list[i].fence =
dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
- snapshot->pending_list[i].finished =
- dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
- ? 1 : 0;
i++;
}
}
- spin_unlock(&sched->base.job_list_lock);
-
return snapshot;
}
@@ -2562,10 +2559,9 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
i++)
- drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
+ drm_printf(p, "\tJob: seqno=%d, fence=%d\n",
snapshot->pending_list[i].seqno,
- snapshot->pending_list[i].fence,
- snapshot->pending_list[i].finished);
+ snapshot->pending_list[i].fence);
}
/**
diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
index dc7456c34583..59d88dd66e6e 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
@@ -64,7 +64,6 @@ struct guc_submit_parallel_scratch {
struct pending_list_snapshot {
u32 seqno;
bool fence;
- bool finished;
};
/**
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator
2025-10-03 20:11 ` [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator Matthew Brost
@ 2025-10-06 9:19 ` Jani Nikula
2025-10-06 13:17 ` Matthew Brost
0 siblings, 1 reply; 7+ messages in thread
From: Jani Nikula @ 2025-10-06 9:19 UTC (permalink / raw)
To: Matthew Brost, intel-xe, dri-devel; +Cc: christian.koenig, pstanner, dakr
On Fri, 03 Oct 2025, Matthew Brost <matthew.brost@intel.com> wrote:
> Stop open coding pending job list in drivers. Add pending job list
> iterator which safely walks DRM scheduler list either locklessly
> asserting DRM scheduler is stopped or takes pending job list lock.
>
> v2:
> - Fix checkpatch (CI)
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
> include/drm/gpu_scheduler.h | 64 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 64 insertions(+)
>
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index fb88301b3c45..bb49d8b715eb 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -698,4 +698,68 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> struct drm_gpu_scheduler **sched_list,
> unsigned int num_sched_list);
>
> +/* Inlines */
Do they need to be inlines for perf reasons? Otherwise, inlines just
make proper encapsulation harder, proliferate header interdependencies,
and make the incremental builds slower.
Have you tried running the header through the compiler to see if it's
self-contained?
Unfortunately, DRM_HEADER_TEST still depends on BROKEN so we don't get
that check as part of the build. :(
BR,
Jani.
> +
> +/**
> + * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
> + * @sched: DRM scheduler associated with pending job iterator
> + * @stopped: DRM scheduler stopped state associated with pending job iterator
> + */
> +struct drm_sched_pending_job_iter {
> + struct drm_gpu_scheduler *sched;
> + bool stopped;
> +};
> +
> +/* Drivers should never call this directly */
> +static inline struct drm_sched_pending_job_iter
> +__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched, bool stopped)
> +{
> + struct drm_sched_pending_job_iter iter = {
> + .sched = sched,
> + .stopped = stopped,
> + };
> +
> + if (stopped)
> + WARN_ON(!READ_ONCE(sched->pause_submit));
> + else
> + spin_lock(&sched->job_list_lock);
> +
> + return iter;
> +}
> +
> +/* Drivers should never call this directly */
> +static inline void
> +__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
> +{
> + if (iter.stopped)
> + WARN_ON(!READ_ONCE(iter.sched->pause_submit));
> + else
> + spin_unlock(&iter.sched->job_list_lock);
> +}
> +
> +DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
> + __drm_sched_pending_job_iter_end(_T),
> + __drm_sched_pending_job_iter_begin(__sched, __stopped),
> + struct drm_gpu_scheduler *__sched, bool __stopped);
> +static inline void
> +*class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
> +{return _T; }
> +#define class_drm_sched_pending_job_iter_is_conditional false
> +
> +/**
> + * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
> + * @__job: Current pending job being iterated over
> + * @__sched: DRM scheduler to iterate over pending jobs
> + * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
> + * @__stopped: DRM scheduler stopped state
> + *
> + * Iterator for each pending job in scheduler, filtering on an entity, and
> + * enforcing locking rules (either scheduler fully stopped or correctly takes
> + * job_list_lock).
> + */
> +#define drm_sched_for_each_pending_job(__job, __sched, __entity, __stopped) \
> + scoped_guard(drm_sched_pending_job_iter, (__sched), (__stopped)) \
> + list_for_each_entry((__job), &(__sched)->pending_list, list) \
> + for_each_if(!(__entity) || (__job)->entity == (__entity))
> +
> #endif
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator
2025-10-06 9:19 ` Jani Nikula
@ 2025-10-06 13:17 ` Matthew Brost
0 siblings, 0 replies; 7+ messages in thread
From: Matthew Brost @ 2025-10-06 13:17 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-xe, dri-devel, christian.koenig, pstanner, dakr
On Mon, Oct 06, 2025 at 12:19:29PM +0300, Jani Nikula wrote:
> On Fri, 03 Oct 2025, Matthew Brost <matthew.brost@intel.com> wrote:
> > Stop open coding pending job list in drivers. Add pending job list
> > iterator which safely walks DRM scheduler list either locklessly
> > asserting DRM scheduler is stopped or takes pending job list lock.
> >
> > v2:
> > - Fix checkpatch (CI)
> >
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > include/drm/gpu_scheduler.h | 64 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 64 insertions(+)
> >
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index fb88301b3c45..bb49d8b715eb 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -698,4 +698,68 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> > struct drm_gpu_scheduler **sched_list,
> > unsigned int num_sched_list);
> >
> > +/* Inlines */
>
> Do they need to be inlines for perf reasons? Otherwise, inlines just
> make proper encapsulation harder, proliferate header interdependencies,
> and make the incremental builds slower.
>
The iterator needs to b inline as it is a macro. Everything else, no.
All the inlines are in this series are a couple of lines so stuck them
in header, easy enough to move if needed.
> Have you tried running the header through the compiler to see if it's
> self-contained?
>
I would think they are self-contained but I'm not exactly sure what this
means.
Matt
> Unfortunately, DRM_HEADER_TEST still depends on BROKEN so we don't get
> that check as part of the build. :(
>
> BR,
> Jani.
>
>
> > +
> > +/**
> > + * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
> > + * @sched: DRM scheduler associated with pending job iterator
> > + * @stopped: DRM scheduler stopped state associated with pending job iterator
> > + */
> > +struct drm_sched_pending_job_iter {
> > + struct drm_gpu_scheduler *sched;
> > + bool stopped;
> > +};
> > +
> > +/* Drivers should never call this directly */
> > +static inline struct drm_sched_pending_job_iter
> > +__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched, bool stopped)
> > +{
> > + struct drm_sched_pending_job_iter iter = {
> > + .sched = sched,
> > + .stopped = stopped,
> > + };
> > +
> > + if (stopped)
> > + WARN_ON(!READ_ONCE(sched->pause_submit));
> > + else
> > + spin_lock(&sched->job_list_lock);
> > +
> > + return iter;
> > +}
> > +
> > +/* Drivers should never call this directly */
> > +static inline void
> > +__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
> > +{
> > + if (iter.stopped)
> > + WARN_ON(!READ_ONCE(iter.sched->pause_submit));
> > + else
> > + spin_unlock(&iter.sched->job_list_lock);
> > +}
> > +
> > +DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
> > + __drm_sched_pending_job_iter_end(_T),
> > + __drm_sched_pending_job_iter_begin(__sched, __stopped),
> > + struct drm_gpu_scheduler *__sched, bool __stopped);
> > +static inline void
> > +*class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
> > +{return _T; }
> > +#define class_drm_sched_pending_job_iter_is_conditional false
> > +
> > +/**
> > + * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
> > + * @__job: Current pending job being iterated over
> > + * @__sched: DRM scheduler to iterate over pending jobs
> > + * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
> > + * @__stopped: DRM scheduler stopped state
> > + *
> > + * Iterator for each pending job in scheduler, filtering on an entity, and
> > + * enforcing locking rules (either scheduler fully stopped or correctly takes
> > + * job_list_lock).
> > + */
> > +#define drm_sched_for_each_pending_job(__job, __sched, __entity, __stopped) \
> > + scoped_guard(drm_sched_pending_job_iter, (__sched), (__stopped)) \
> > + list_for_each_entry((__job), &(__sched)->pending_list, list) \
> > + for_each_if(!(__entity) || (__job)->entity == (__entity))
> > +
> > #endif
>
> --
> Jani Nikula, Intel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-10-06 13:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-03 20:11 [RFC PATCH v2 0/4] Fix DRM scheduler layering violations in Xe Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator Matthew Brost
2025-10-06 9:19 ` Jani Nikula
2025-10-06 13:17 ` Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 2/4] drm/sched: Add several job helpers to avoid drivers touching scheduler state Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 3/4] drm/xe: Add dedicated message lock Matthew Brost
2025-10-03 20:11 ` [RFC PATCH v2 4/4] drm/xe: Stop abusing DRM scheduler internals Matthew Brost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox