* [PATCH v3 1/8] drm/vc4: Move vc4_wait_bo_ioctl() to vc4_bo.c
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:14 ` [PATCH v3 2/8] drm/vc4: Protect perfmon state with a dedicated lock Maíra Canal
` (6 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Move vc4_wait_bo_ioctl() from vc4_gem.c to vc4_bo.c, grouping it with
the other BO-related ioctls. No functional change.
Reviewed-by: Melissa Wen <mwen@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_bo.c | 33 +++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_drv.h | 4 ++--
drivers/gpu/drm/vc4/vc4_gem.c | 33 ---------------------------------
3 files changed, 35 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 2161761b1f22..e2f747c92832 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -831,6 +831,39 @@ int vc4_mmap_bo_ioctl(struct drm_device *dev, void *data,
return 0;
}
+int
+vc4_wait_bo_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ int ret;
+ struct drm_vc4_wait_bo *args = data;
+ unsigned long timeout_jiffies =
+ usecs_to_jiffies(div_u64(args->timeout_ns, 1000));
+ ktime_t start = ktime_get();
+ u64 delta_ns;
+
+ if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
+ return -ENODEV;
+
+ if (args->pad != 0)
+ return -EINVAL;
+
+ ret = drm_gem_dma_resv_wait(file_priv, args->handle,
+ true, timeout_jiffies);
+
+ /* Decrement the user's timeout, in case we got interrupted
+ * such that the ioctl will be restarted.
+ */
+ delta_ns = ktime_to_ns(ktime_sub(ktime_get(), start));
+ if (delta_ns < args->timeout_ns)
+ args->timeout_ns -= delta_ns;
+ else
+ args->timeout_ns = 0;
+
+ return ret;
+}
+
int
vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 0f5958c1e6b6..fe313027a91e 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -906,6 +906,8 @@ int vc4_bo_dumb_create(struct drm_file *file_priv,
struct drm_mode_create_dumb *args);
int vc4_create_bo_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+int vc4_wait_bo_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
int vc4_create_shader_bo_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int vc4_mmap_bo_ioctl(struct drm_device *dev, void *data,
@@ -990,8 +992,6 @@ int vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
int vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
-int vc4_wait_bo_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv);
void vc4_submit_next_bin_job(struct drm_device *dev);
void vc4_submit_next_render_job(struct drm_device *dev);
void vc4_move_job_to_render(struct drm_device *dev, struct vc4_exec_info *exec);
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e231c906709c..f0c711f114cf 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -962,39 +962,6 @@ vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
&args->timeout_ns);
}
-int
-vc4_wait_bo_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- int ret;
- struct drm_vc4_wait_bo *args = data;
- unsigned long timeout_jiffies =
- usecs_to_jiffies(div_u64(args->timeout_ns, 1000));
- ktime_t start = ktime_get();
- u64 delta_ns;
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return -ENODEV;
-
- if (args->pad != 0)
- return -EINVAL;
-
- ret = drm_gem_dma_resv_wait(file_priv, args->handle,
- true, timeout_jiffies);
-
- /* Decrement the user's timeout, in case we got interrupted
- * such that the ioctl will be restarted.
- */
- delta_ns = ktime_to_ns(ktime_sub(ktime_get(), start));
- if (delta_ns < args->timeout_ns)
- args->timeout_ns -= delta_ns;
- else
- args->timeout_ns = 0;
-
- return ret;
-}
-
/**
* vc4_submit_cl_ioctl() - Submits a job (frame) to the VC4.
* @dev: DRM device
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v3 2/8] drm/vc4: Protect perfmon state with a dedicated lock
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
2026-07-12 15:14 ` [PATCH v3 1/8] drm/vc4: Move vc4_wait_bo_ioctl() to vc4_bo.c Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:14 ` [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures Maíra Canal
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
The V3D block exposes a single set of performance counters, so at most
one perfmon can be programmed into the hardware at a time. Until now the
active perfmon was tracked by a bare pointer whose access was serialized
by job_lock, which was sufficient because the legacy job queue started
and stopped perfmons from a single serialized context.
Once job execution moves to the DRM GPU scheduler, a perfmon is started
from the job's run_job() callback but stopped from the IRQ handler and
the reset path, so the shared perfmon state can be touched concurrently
and job_lock no longer covers those accesses.
Introduce a dedicated spinlock next to the active perfmon pointer and
take it in vc4_perfmon_start() and vc4_perfmon_stop(). Snapshot the
counter values under the same lock in vc4_perfmon_get_values_ioctl() so
that a concurrent stop cannot expose a half-updated array to userspace.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_drv.h | 13 ++++++++++---
drivers/gpu/drm/vc4/vc4_gem.c | 3 ++-
drivers/gpu/drm/vc4/vc4_perfmon.c | 34 ++++++++++++++++++++++------------
3 files changed, 34 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fe313027a91e..91c9c7aa8039 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -181,10 +181,17 @@ struct vc4_dev {
wait_queue_head_t job_wait_queue;
struct work_struct job_done_work;
- /* Used to track the active perfmon if any. Access to this field is
- * protected by job_lock.
+ /* Tracks the performance monitor state. The V3D block exposes a single
+ * set of performance counters, so at most one perfmon can be active at
+ * any moment.
*/
- struct vc4_perfmon *active_perfmon;
+ struct {
+ /* Protects @active. */
+ spinlock_t lock;
+
+ /* Perfmon currently programmed in HW (or NULL if none). */
+ struct vc4_perfmon *active;
+ } perfmon_state;
/* The memory used for storing binner tile alloc, tile state,
* and overflow memory allocations. This is freed when V3D
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index f0c711f114cf..f84d82b3bfbb 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -491,7 +491,7 @@ vc4_submit_next_bin_job(struct drm_device *dev)
/* Only start the perfmon if it was not already started by a previous
* job.
*/
- if (exec->perfmon && vc4->active_perfmon != exec->perfmon)
+ if (exec->perfmon && vc4->perfmon_state.active != exec->perfmon)
vc4_perfmon_start(vc4, exec->perfmon);
/* Either put the job in the binner if it uses the binner, or
@@ -1138,6 +1138,7 @@ int vc4_gem_init(struct drm_device *dev)
INIT_LIST_HEAD(&vc4->render_job_list);
INIT_LIST_HEAD(&vc4->job_done_list);
spin_lock_init(&vc4->job_lock);
+ spin_lock_init(&vc4->perfmon_state.lock);
INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
timer_setup(&vc4->hangcheck.timer, vc4_hangcheck_elapsed, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c b/drivers/gpu/drm/vc4/vc4_perfmon.c
index f75dfd156756..2bcf6a7d538f 100644
--- a/drivers/gpu/drm/vc4/vc4_perfmon.c
+++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
@@ -51,16 +51,18 @@ void vc4_perfmon_start(struct vc4_dev *vc4, struct vc4_perfmon *perfmon)
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
- if (WARN_ON_ONCE(!perfmon || vc4->active_perfmon))
+ if (!perfmon)
return;
+ guard(spinlock_irqsave)(&vc4->perfmon_state.lock);
+
for (i = 0; i < perfmon->ncounters; i++)
V3D_WRITE(V3D_PCTRS(i), perfmon->events[i]);
mask = GENMASK(perfmon->ncounters - 1, 0);
V3D_WRITE(V3D_PCTRC, mask);
V3D_WRITE(V3D_PCTRE, V3D_PCTRE_EN | mask);
- vc4->active_perfmon = perfmon;
+ vc4->perfmon_state.active = perfmon;
}
void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon,
@@ -71,8 +73,12 @@ void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon,
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
- if (WARN_ON_ONCE(!vc4->active_perfmon ||
- perfmon != vc4->active_perfmon))
+ if (!perfmon)
+ return;
+
+ guard(spinlock_irqsave)(&vc4->perfmon_state.lock);
+
+ if (perfmon != vc4->perfmon_state.active)
return;
if (capture) {
@@ -81,7 +87,7 @@ void vc4_perfmon_stop(struct vc4_dev *vc4, struct vc4_perfmon *perfmon,
}
V3D_WRITE(V3D_PCTRE, 0);
- vc4->active_perfmon = NULL;
+ vc4->perfmon_state.active = NULL;
}
struct vc4_perfmon *vc4_perfmon_find(struct vc4_file *vc4file, int id)
@@ -116,8 +122,7 @@ static void vc4_perfmon_delete(struct vc4_file *vc4file,
struct vc4_dev *vc4 = vc4file->dev;
/* If the active perfmon is being destroyed, stop it first */
- if (perfmon == vc4->active_perfmon)
- vc4_perfmon_stop(vc4, perfmon, false);
+ vc4_perfmon_stop(vc4, perfmon, false);
vc4_perfmon_put(perfmon);
}
@@ -222,8 +227,10 @@ int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_file *vc4file = file_priv->driver_priv;
struct drm_vc4_perfmon_get_values *req = data;
+ u64 values[DRM_VC4_MAX_PERF_COUNTERS];
struct vc4_perfmon *perfmon;
- int ret;
+ size_t size;
+ int ret = 0;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return -ENODEV;
@@ -237,11 +244,14 @@ int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
if (!perfmon)
return -EINVAL;
- if (copy_to_user(u64_to_user_ptr(req->values_ptr), perfmon->counters,
- perfmon->ncounters * sizeof(u64)))
+ size = perfmon->ncounters * sizeof(u64);
+
+ /* Snapshot the counters under the perfmon lock */
+ scoped_guard(spinlock_irqsave, &vc4->perfmon_state.lock)
+ memcpy(values, perfmon->counters, size);
+
+ if (copy_to_user(u64_to_user_ptr(req->values_ptr), values, size))
ret = -EFAULT;
- else
- ret = 0;
vc4_perfmon_put(perfmon);
return ret;
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
2026-07-12 15:14 ` [PATCH v3 1/8] drm/vc4: Move vc4_wait_bo_ioctl() to vc4_bo.c Maíra Canal
2026-07-12 15:14 ` [PATCH v3 2/8] drm/vc4: Protect perfmon state with a dedicated lock Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:32 ` sashiko-bot
2026-07-12 15:14 ` [PATCH v3 4/8] drm/vc4: Add new job submission implementation Maíra Canal
` (4 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Introduce `struct vc4_job` as the new base structure for GPU jobs, with
`vc4_bin_job` and `vc4_render_job` as specialized variants for BIN and
RENDER jobs, respectively. These structures embed `struct drm_sched_job`
and will be used with the DRM GPU scheduler. The `vc4_exec_info` struct
now holds pointers to these job structs, allowing the existing validation
code to populate job fields during command list processing.
Add the core scheduler infrastructure in the new file "vc4_sched.c",
which implements scheduler operations for BIN and RENDER queues, timeout
handling, and job cleanup. Each queue has its own scheduler instance.
This design is largely inspired by the v3d driver.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_drv.c | 24 +++
drivers/gpu/drm/vc4/vc4_drv.h | 117 +++++++++++++-
drivers/gpu/drm/vc4/vc4_fence.c | 18 +++
drivers/gpu/drm/vc4/vc4_gem.c | 8 +-
drivers/gpu/drm/vc4/vc4_sched.c | 331 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 496 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 616caf9d9915..cacd4cb94259 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -145,7 +145,10 @@ static int vc4_get_param_ioctl(struct drm_device *dev, void *data,
static int vc4_open(struct drm_device *dev, struct drm_file *file)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct drm_gpu_scheduler *sched;
struct vc4_file *vc4file;
+ enum vc4_queue q;
+ int ret;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return -ENODEV;
@@ -155,15 +158,33 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
return -ENOMEM;
vc4file->dev = vc4;
+ for (q = 0; q < VC4_MAX_QUEUES; q++) {
+ sched = &vc4->queue[q].sched;
+ ret = drm_sched_entity_init(&vc4file->sched_entity[q],
+ DRM_SCHED_PRIORITY_NORMAL, &sched,
+ 1, NULL);
+ if (ret)
+ goto err_sched;
+ }
+
vc4_perfmon_open_file(vc4file);
file->driver_priv = vc4file;
+
return 0;
+
+err_sched:
+ for (q--; q >= 0; q--)
+ drm_sched_entity_destroy(&vc4file->sched_entity[q]);
+ kfree(vc4file);
+ return ret;
+
}
static void vc4_close(struct drm_device *dev, struct drm_file *file)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_file *vc4file = file->driver_priv;
+ enum vc4_queue q;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
@@ -171,6 +192,9 @@ static void vc4_close(struct drm_device *dev, struct drm_file *file)
if (vc4file->bin_bo_used)
vc4_v3d_bin_bo_put(vc4);
+ for (q = 0; q < VC4_MAX_QUEUES; q++)
+ drm_sched_entity_destroy(&vc4file->sched_entity[q]);
+
vc4_perfmon_close_file(vc4file);
kfree(vc4file);
}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 91c9c7aa8039..255a95f55586 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -20,6 +20,7 @@
#include <drm/drm_managed.h>
#include <drm/drm_mm.h>
#include <drm/drm_modeset_lock.h>
+#include <drm/gpu_scheduler.h>
#include <kunit/test-bug.h>
@@ -31,6 +32,19 @@ struct drm_gem_object;
extern const struct drm_driver vc4_drm_driver;
extern const struct drm_driver vc5_drm_driver;
+enum vc4_queue {
+ VC4_BIN,
+ VC4_RENDER,
+ VC4_MAX_QUEUES,
+};
+
+struct vc4_queue_state {
+ struct drm_gpu_scheduler sched;
+
+ u64 fence_context;
+ u64 emit_seqno;
+};
+
/* Don't forget to update vc4_bo.c: bo_type_names[] when adding to
* this.
*/
@@ -152,6 +166,12 @@ struct vc4_dev {
*/
uint64_t emit_seqno;
+ struct vc4_queue_state queue[VC4_MAX_QUEUES];
+
+ struct vc4_bin_job *bin_job;
+
+ struct vc4_render_job *render_job;
+
/* Sequence number for the last completed job on the GPU.
* Starts at 0 (no jobs completed).
*/
@@ -174,6 +194,18 @@ struct vc4_dev {
* job_done_work.
*/
struct list_head job_done_list;
+
+ /* Lock taken when resetting the GPU, to keep multiple
+ * processes from trying to park the scheduler threads and
+ * reset at once.
+ */
+ struct mutex reset_lock;
+
+ /* Lock taken when creating and pushing the GPU scheduler
+ * jobs. This lock protects the DRM scheduler arm + push pair.
+ */
+ struct mutex sched_lock;
+
/* Spinlock used to synchronize the job_list and seqno
* accesses between the IRQ handler and GEM ioctls.
*/
@@ -289,8 +321,8 @@ struct vc4_bo {
struct vc4_fence {
struct dma_fence base;
struct drm_device *dev;
- /* vc4 seqno for signaled() test */
uint64_t seqno;
+ enum vc4_queue queue;
};
#define to_vc4_fence(_fence) \
@@ -675,9 +707,84 @@ struct vc4_crtc_state {
#define VC4_REG32(reg) { .name = #reg, .offset = reg }
+struct vc4_job {
+ struct drm_sched_job base;
+
+ struct kref refcount;
+
+ struct vc4_dev *vc4;
+
+ /* Fence to be signaled by IRQ handler when the job is complete. */
+ struct dma_fence *irq_fence;
+
+ /* Scheduler fence for when the job is considered complete and
+ * the BO reservations can be released.
+ */
+ struct dma_fence *done_fence;
+
+ /* Last current and return addresses the hardware was processing when
+ * the job timedout.
+ */
+ u32 timedout_ctca, timedout_ctra;
+
+ /* Pointer to a performance monitor object if the user requested it,
+ * NULL otherwise.
+ */
+ struct vc4_perfmon *perfmon;
+
+ /* Callback for the freeing of the job on refcount going to 0. */
+ void (*free)(struct kref *ref);
+};
+
+struct vc4_bin_job {
+ struct vc4_job base;
+
+ uint32_t ct0ca, ct0ea;
+
+ /* Corresponding render job, for attaching our overflow memory. */
+ struct vc4_render_job *render;
+
+ /* Whether the exec has taken a reference to the binner BO, which should
+ * happen with a VC4_PACKET_TILE_BINNING_MODE_CONFIG packet.
+ */
+ bool bin_bo_used;
+};
+
+struct vc4_render_job {
+ struct vc4_job base;
+
+ uint32_t ct1ca, ct1ea;
+
+ /* This is the array of BOs that were looked up at the start of submission.
+ * Command validation will use indices into this array.
+ */
+ struct drm_gem_object **bo;
+ u32 bo_count;
+
+ /* List of other BOs used in the job that need to be released
+ * once the job is complete.
+ */
+ struct list_head unref_list;
+
+ /* List of BOs that are being written by the RCL. Other than
+ * the binner temporary storage, this is all the BOs written
+ * by the job.
+ */
+ struct drm_gem_dma_object *rcl_write_bo[4];
+ uint32_t rcl_write_bo_count;
+
+ /* Bitmask of which binner slots are freed when this job completes.
+ * Must remain allocated until the render job completes.
+ */
+ uint32_t bin_slots;
+};
+
struct vc4_exec_info {
struct vc4_dev *dev;
+ struct vc4_bin_job *bin;
+ struct vc4_render_job *render;
+
/* Sequence number for this bin/render job. */
uint64_t seqno;
@@ -800,6 +907,8 @@ struct vc4_file {
struct xarray perfmons;
+ struct drm_sched_entity sched_entity[VC4_MAX_QUEUES];
+
bool bin_bo_used;
};
@@ -992,6 +1101,7 @@ extern struct platform_driver vc4_dsi_driver;
/* vc4_fence.c */
extern const struct dma_fence_ops vc4_fence_ops;
+struct dma_fence *vc4_fence_create(struct vc4_dev *vc4, enum vc4_queue queue);
/* vc4_gem.c */
int vc4_gem_init(struct drm_device *dev);
@@ -1007,6 +1117,7 @@ int vc4_wait_for_seqno(struct drm_device *dev, uint64_t seqno,
void vc4_job_handle_completed(struct vc4_dev *vc4);
int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+void vc4_save_hang_state(struct drm_device *dev);
/* vc4_hdmi.c */
extern struct platform_driver vc4_hdmi_driver;
@@ -1105,4 +1216,8 @@ int vc4_perfmon_destroy_ioctl(struct drm_device *dev, void *data,
int vc4_perfmon_get_values_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+/* vc4_sched.c */
+int vc4_sched_init(struct vc4_dev *vc4);
+void vc4_sched_fini(struct vc4_dev *vc4);
+
#endif /* _VC4_DRV_H_ */
diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
index 580214e2158c..18e8c53563d6 100644
--- a/drivers/gpu/drm/vc4/vc4_fence.c
+++ b/drivers/gpu/drm/vc4/vc4_fence.c
@@ -23,6 +23,24 @@
#include "vc4_drv.h"
+struct dma_fence *vc4_fence_create(struct vc4_dev *vc4, enum vc4_queue q)
+{
+ struct vc4_queue_state *queue = &vc4->queue[q];
+ struct vc4_fence *fence;
+
+ fence = kzalloc_obj(*fence);
+ if (!fence)
+ return ERR_PTR(-ENOMEM);
+
+ fence->dev = &vc4->base;
+ fence->queue = q;
+ fence->seqno = ++queue->emit_seqno;
+ dma_fence_init(&fence->base, &vc4_fence_ops, NULL, queue->fence_context,
+ fence->seqno);
+
+ return &fence->base;
+}
+
static const char *vc4_fence_get_driver_name(struct dma_fence *fence)
{
return "vc4";
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index f84d82b3bfbb..5b5cdfd122e2 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -150,7 +150,7 @@ vc4_get_hang_state_ioctl(struct drm_device *dev, void *data,
return ret;
}
-static void
+void
vc4_save_hang_state(struct drm_device *dev)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
@@ -1139,6 +1139,12 @@ int vc4_gem_init(struct drm_device *dev)
INIT_LIST_HEAD(&vc4->job_done_list);
spin_lock_init(&vc4->job_lock);
spin_lock_init(&vc4->perfmon_state.lock);
+ ret = drmm_mutex_init(dev, &vc4->reset_lock);
+ if (ret)
+ return ret;
+ ret = drmm_mutex_init(dev, &vc4->sched_lock);
+ if (ret)
+ return ret;
INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
timer_setup(&vc4->hangcheck.timer, vc4_hangcheck_elapsed, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_sched.c b/drivers/gpu/drm/vc4/vc4_sched.c
new file mode 100644
index 000000000000..179a6b6c61fd
--- /dev/null
+++ b/drivers/gpu/drm/vc4/vc4_sched.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0+
+/* Copyright (C) 2026 Raspberry Pi */
+
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#include <drm/drm_print.h>
+
+#include "vc4_drv.h"
+#include "vc4_regs.h"
+#include "vc4_trace.h"
+
+static struct vc4_job *
+to_vc4_job(struct drm_sched_job *sched_job)
+{
+ return container_of(sched_job, struct vc4_job, base);
+}
+
+static struct vc4_bin_job *
+to_bin_job(struct drm_sched_job *sched_job)
+{
+ return container_of(sched_job, struct vc4_bin_job, base.base);
+}
+
+static struct vc4_render_job *
+to_render_job(struct drm_sched_job *sched_job)
+{
+ return container_of(sched_job, struct vc4_render_job, base.base);
+}
+
+static void
+vc4_flush_caches(struct drm_device *dev)
+{
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+
+ /* Flush the GPU L2 caches. These caches sit on top of system
+ * L3 (the 128kb or so shared with the CPU), and are
+ * non-allocating in the L3.
+ */
+ V3D_WRITE(V3D_L2CACTL, V3D_L2CACTL_L2CCLR);
+
+ V3D_WRITE(V3D_SLCACTL,
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_T1CC) |
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_T0CC) |
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_UCC) |
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_ICC));
+}
+
+static void
+vc4_flush_texture_caches(struct drm_device *dev)
+{
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+
+ V3D_WRITE(V3D_L2CACTL, V3D_L2CACTL_L2CCLR);
+
+ V3D_WRITE(V3D_SLCACTL,
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_T1CC) |
+ VC4_SET_FIELD(0xf, V3D_SLCACTL_T0CC));
+}
+
+static void
+vc4_sched_job_free(struct drm_sched_job *sched_job)
+{
+ struct vc4_job *job = to_vc4_job(sched_job);
+
+ vc4_job_cleanup(job);
+}
+
+static struct dma_fence *vc4_bin_job_run(struct drm_sched_job *sched_job)
+{
+ struct vc4_bin_job *job = to_bin_job(sched_job);
+ struct vc4_dev *vc4 = job->base.vc4;
+ struct drm_device *dev = &vc4->base;
+ struct dma_fence *fence = NULL;
+
+ if (unlikely(job->base.base.s_fence->finished.error))
+ goto out_clean_job;
+
+ /* Lock required around bin_job update vs vc4_overflow_mem_work(). */
+ spin_lock_irq(&vc4->job_lock);
+ vc4->bin_job = job;
+
+ /* Reset the overflow allocation registers to prevent the binner from
+ * writing to a slot that a concurrent render job might free. The first
+ * OUTOMEM interrupt will allocate a fresh slot.
+ */
+ V3D_WRITE(V3D_BPOA, 0);
+ V3D_WRITE(V3D_BPOS, 0);
+ spin_unlock_irq(&vc4->job_lock);
+
+ vc4_flush_caches(dev);
+
+ fence = vc4_fence_create(vc4, VC4_BIN);
+ if (IS_ERR(fence))
+ goto out_clean_job;
+
+ /* Release the previous fence if the scheduler is resubmitting this
+ * job after a GPU reset.
+ */
+ if (job->base.irq_fence)
+ dma_fence_put(job->base.irq_fence);
+ job->base.irq_fence = dma_fence_get(fence);
+
+ trace_vc4_submit_cl(dev, false, to_vc4_fence(fence)->seqno,
+ job->ct0ca, job->ct0ea);
+
+ vc4_perfmon_start(vc4, job->base.perfmon);
+
+ V3D_WRITE(V3D_CTNCA(0), job->ct0ca);
+ V3D_WRITE(V3D_CTNEA(0), job->ct0ea);
+
+ return fence;
+
+out_clean_job:
+ spin_lock_irq(&vc4->job_lock);
+ vc4->bin_job = NULL;
+ spin_unlock_irq(&vc4->job_lock);
+ return fence;
+}
+
+static struct dma_fence *vc4_render_job_run(struct drm_sched_job *sched_job)
+{
+ struct vc4_render_job *job = to_render_job(sched_job);
+ struct vc4_dev *vc4 = job->base.vc4;
+ struct drm_device *dev = &vc4->base;
+ struct dma_fence *fence = NULL;
+
+ if (unlikely(job->base.base.s_fence->finished.error))
+ goto out_clean_job;
+
+ spin_lock_irq(&vc4->job_lock);
+ vc4->render_job = job;
+ spin_unlock_irq(&vc4->job_lock);
+
+ /* A previous RCL may have written to one of our textures, and
+ * our full cache flush at bin time may have occurred before
+ * that RCL completed. Flush the texture cache now, but not
+ * the instructions or uniforms (since we don't write those
+ * from an RCL).
+ */
+ vc4_flush_texture_caches(dev);
+
+ fence = vc4_fence_create(vc4, VC4_RENDER);
+ if (IS_ERR(fence))
+ goto out_clean_job;
+
+ /* Release the previous fence if the scheduler is resubmitting this
+ * job after a GPU reset.
+ */
+ if (job->base.irq_fence)
+ dma_fence_put(job->base.irq_fence);
+ job->base.irq_fence = dma_fence_get(fence);
+
+ trace_vc4_submit_cl(dev, true, to_vc4_fence(fence)->seqno,
+ job->ct1ca, job->ct1ea);
+
+ vc4_perfmon_start(vc4, job->base.perfmon);
+
+ V3D_WRITE(V3D_CTNCA(1), job->ct1ca);
+ V3D_WRITE(V3D_CTNEA(1), job->ct1ea);
+
+ return fence;
+
+out_clean_job:
+ spin_lock_irq(&vc4->job_lock);
+ vc4->render_job = NULL;
+ spin_unlock_irq(&vc4->job_lock);
+
+ return fence;
+}
+
+static void
+vc4_reset(struct drm_device *dev)
+{
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+
+ drm_err(dev, "Resetting GPU.\n");
+
+ mutex_lock(&vc4->power_lock);
+ if (vc4->power_refcount) {
+ /* Power the device off and back on the by dropping the
+ * reference on runtime PM.
+ */
+ pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
+ pm_runtime_get_sync(&vc4->v3d->pdev->dev);
+ }
+ mutex_unlock(&vc4->power_lock);
+
+ vc4_irq_reset(dev);
+}
+
+static enum drm_gpu_sched_stat
+vc4_gpu_reset_for_timeout(struct vc4_dev *vc4, struct drm_sched_job *sched_job)
+{
+ struct vc4_job *job = to_vc4_job(sched_job);
+ enum vc4_queue q;
+
+ mutex_lock(&vc4->reset_lock);
+
+ /* block scheduler */
+ for (q = 0; q < VC4_MAX_QUEUES; q++)
+ drm_sched_stop(&vc4->queue[q].sched, sched_job);
+
+ if (sched_job) {
+ drm_sched_increase_karma(sched_job);
+
+ /* If the guilty job is a BIN job, also increase the karma
+ * of its paired render job. Otherwise, the RENDER job would
+ * be submitted to the GPU without binner output.
+ */
+ if (sched_job->sched == &vc4->queue[VC4_BIN].sched) {
+ struct vc4_bin_job *bin = to_bin_job(sched_job);
+
+ drm_sched_increase_karma(&bin->render->base.base);
+ }
+ }
+
+ vc4_perfmon_stop(vc4, job->perfmon, false);
+ vc4_save_hang_state(&vc4->base);
+
+ /* get the GPU back into the init state */
+ vc4_reset(&vc4->base);
+
+ for (q = 0; q < VC4_MAX_QUEUES; q++)
+ drm_sched_resubmit_jobs(&vc4->queue[q].sched);
+
+ /* Unblock schedulers and restart their jobs. */
+ for (q = 0; q < VC4_MAX_QUEUES; q++)
+ drm_sched_start(&vc4->queue[q].sched, 0);
+
+ mutex_unlock(&vc4->reset_lock);
+
+ return DRM_GPU_SCHED_STAT_RESET;
+}
+
+static enum drm_gpu_sched_stat
+vc4_cl_job_timedout(struct drm_sched_job *sched_job, enum vc4_queue q)
+{
+ struct vc4_job *job = to_vc4_job(sched_job);
+ struct vc4_dev *vc4 = job->vc4;
+ u32 ctca = V3D_READ(V3D_CTNCA(q));
+ u32 ctra = V3D_READ(V3D_CTNRA0(q));
+
+ /* If the current address or return address have changed, then the GPU
+ * has probably made progress and we should delay the reset. This could
+ * fail if the GPU got in an infinite loop in the CL, but that is pretty
+ * unlikely outside of an i-g-t testcase.
+ */
+ if (job->timedout_ctca != ctca || job->timedout_ctra != ctra) {
+ job->timedout_ctca = ctca;
+ job->timedout_ctra = ctra;
+
+ return DRM_GPU_SCHED_STAT_NO_HANG;
+ }
+
+ return vc4_gpu_reset_for_timeout(vc4, sched_job);
+}
+
+static enum drm_gpu_sched_stat
+vc4_bin_job_timedout(struct drm_sched_job *sched_job)
+{
+ return vc4_cl_job_timedout(sched_job, VC4_BIN);
+}
+
+static enum drm_gpu_sched_stat
+vc4_render_job_timedout(struct drm_sched_job *sched_job)
+{
+ return vc4_cl_job_timedout(sched_job, VC4_RENDER);
+}
+
+static const struct drm_sched_backend_ops vc4_bin_sched_ops = {
+ .run_job = vc4_bin_job_run,
+ .timedout_job = vc4_bin_job_timedout,
+ .free_job = vc4_sched_job_free,
+};
+
+static const struct drm_sched_backend_ops vc4_render_sched_ops = {
+ .run_job = vc4_render_job_run,
+ .timedout_job = vc4_render_job_timedout,
+ .free_job = vc4_sched_job_free,
+};
+
+static int
+vc4_queue_sched_init(struct vc4_dev *vc4, const struct drm_sched_backend_ops *ops,
+ enum vc4_queue q, const char *name)
+{
+ struct vc4_queue_state *queue = &vc4->queue[q];
+ struct drm_sched_init_args args = {
+ .credit_limit = 1,
+ .timeout = msecs_to_jiffies(500),
+ .dev = vc4->base.dev,
+ };
+
+ args.ops = ops;
+ args.name = name;
+
+ queue->fence_context = dma_fence_context_alloc(1);
+
+ return drm_sched_init(&queue->sched, &args);
+}
+
+int
+vc4_sched_init(struct vc4_dev *vc4)
+{
+ int ret;
+
+ ret = vc4_queue_sched_init(vc4, &vc4_bin_sched_ops,
+ VC4_BIN, "vc4_bin");
+ if (ret)
+ return ret;
+
+ ret = vc4_queue_sched_init(vc4, &vc4_render_sched_ops,
+ VC4_RENDER, "vc4_render");
+ if (ret) {
+ vc4_sched_fini(vc4);
+ return ret;
+ }
+
+ return 0;
+}
+
+void
+vc4_sched_fini(struct vc4_dev *vc4)
+{
+ enum vc4_queue q;
+
+ for (q = 0; q < VC4_MAX_QUEUES; q++) {
+ if (vc4->queue[q].sched.ready)
+ drm_sched_fini(&vc4->queue[q].sched);
+ }
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures
2026-07-12 15:14 ` [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures Maíra Canal
@ 2026-07-12 15:32 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-12 15:32 UTC (permalink / raw)
To: Maíra Canal; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
New issues:
- [Critical] `vc4_sched_init()` is never called, leading to a zero-initialized scheduler and a kernel panic.
- [High] Unsigned enum used as a reverse loop counter causes an infinite loop and out-of-bounds access.
- [High] Dereferencing `bin->render` without checking for NULL causes a kernel panic.
- [High] Race condition between concurrent multi-queue timeout handlers causes Use-After-Free.
- [High] Missing `dma_fence_put()` for `irq_fence` causes a steady memory leak.
- [High] GFP_KERNEL memory allocations during GPU reset recovery cause deadlocks.
- [Low] Missing definition of `vc4_job_cleanup` causing build failure.
Pre-existing issues:
- [High] Infinite loop in a command list extends the GPU timeout indefinitely (DoS).
- [High] Hardware registers are accessed without guaranteeing the device is successfully powered on.
--
--- Patch [3]: [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures ---
commit 823d1127ab669811483f871897f29bae5ea3a18f
Author: Maíra Canal <mcanal@igalia.com>
drm/vc4: Add DRM GPU scheduler infrastructure and job structures
Introduce struct vc4_job as the new base structure for GPU jobs, embedding
struct drm_sched_job for use with the DRM GPU scheduler. Add core scheduler
infrastructure in vc4_sched.c, implementing queue operations, timeout
handling, and job cleanup.
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
[ ... ]
> @@ -155,15 +158,33 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
> return -ENOMEM;
> vc4file->dev = vc4;
>
> + for (q = 0; q < VC4_MAX_QUEUES; q++) {
> + sched = &vc4->queue[q].sched;
> + ret = drm_sched_entity_init(&vc4file->sched_entity[q],
> + DRM_SCHED_PRIORITY_NORMAL, &sched,
> + 1, NULL);
[Severity: Critical]
Is it intentional that the DRM schedulers are initialized against a
zero-initialized structure?
I see vc4_sched_init() is implemented in vc4_sched.c but it never seems to
be called during driver setup. If vc4->queue[q].sched is completely
zero-initialized, doesn't opening and then closing a file trigger a NULL
pointer dereference in vc4_close() when calling
drm_sched_entity_destroy() due to the uninitialized wait queues?
> + if (ret)
> + goto err_sched;
> + }
> +
> vc4_perfmon_open_file(vc4file);
> file->driver_priv = vc4file;
> +
> return 0;
> +
> +err_sched:
> + for (q--; q >= 0; q--)
> + drm_sched_entity_destroy(&vc4file->sched_entity[q]);
[Severity: High]
Will this loop ever terminate on failure?
Since q is of type enum vc4_queue, the compiler will likely treat it as
an unsigned integer type. If the first allocation fails (q = 0), wouldn't
q-- underflow to the maximum unsigned value, making q >= 0 always true
and leading to an out-of-bounds access and infinite loop?
> diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
> --- a/drivers/gpu/drm/vc4/vc4_fence.c
> +++ b/drivers/gpu/drm/vc4/vc4_fence.c
> @@ -23,6 +23,24 @@
>
> #include "vc4_drv.h"
>
> +struct dma_fence *vc4_fence_create(struct vc4_dev *vc4, enum vc4_queue q)
> +{
> + struct vc4_queue_state *queue = &vc4->queue[q];
> + struct vc4_fence *fence;
> +
> + fence = kzalloc_obj(*fence);
[Severity: High]
Can this memory allocation cause a deadlock during GPU reset?
When the GPU hangs, the timeout handler calls drm_sched_resubmit_jobs()
which invokes the run_job callbacks. If vc4_fence_create() is called in
this path and implicitly uses GFP_KERNEL (via kzalloc_obj), memory
reclaim might block waiting for fences from the already hung GPU,
preventing the reset from ever completing.
> diff --git a/drivers/gpu/drm/vc4/vc4_sched.c b/drivers/gpu/drm/vc4/vc4_sched.c
> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_sched.c
[ ... ]
> +static void
> +vc4_sched_job_free(struct drm_sched_job *sched_job)
> +{
> + struct vc4_job *job = to_vc4_job(sched_job);
> +
> + vc4_job_cleanup(job);
> +}
[Severity: Low]
Will this compile successfully?
The function vc4_job_cleanup() is called here but it does not appear to
be defined anywhere in this patch or the surrounding code.
[ ... ]
> +static struct dma_fence *vc4_bin_job_run(struct drm_sched_job *sched_job)
> +{
[ ... ]
> + /* Release the previous fence if the scheduler is resubmitting this
> + * job after a GPU reset.
> + */
> + if (job->base.irq_fence)
> + dma_fence_put(job->base.irq_fence);
> + job->base.irq_fence = dma_fence_get(fence);
[Severity: High]
Does this code leak the extra dma_fence reference?
We acquire an extra reference here with dma_fence_get() in both the bin
and render job paths, but I don't see a corresponding dma_fence_put() in
the job teardown paths. Doesn't this mean every executed job permanently
leaks a fence?
[ ... ]
> +static void
> +vc4_reset(struct drm_device *dev)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> +
> + drm_err(dev, "Resetting GPU.\n");
> +
> + mutex_lock(&vc4->power_lock);
> + if (vc4->power_refcount) {
> + /* Power the device off and back on the by dropping the
> + * reference on runtime PM.
> + */
> + pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
> + pm_runtime_get_sync(&vc4->v3d->pdev->dev);
[Severity: High]
This is a pre-existing issue, but shouldn't we check the return value of
pm_runtime_get_sync() here?
If the device fails to resume, it remains powered off. The function then
proceeds to call vc4_irq_reset() which performs MMIO writes to V3D_INTCTL
and V3D_INTENA. Writing to unclocked hardware typically triggers bus
errors and a kernel crash.
> + }
> + mutex_unlock(&vc4->power_lock);
> +
> + vc4_irq_reset(dev);
> +}
> +
> +static enum drm_gpu_sched_stat
> +vc4_gpu_reset_for_timeout(struct vc4_dev *vc4, struct drm_sched_job *sched_job)
> +{
> + struct vc4_job *job = to_vc4_job(sched_job);
> + enum vc4_queue q;
> +
> + mutex_lock(&vc4->reset_lock);
[Severity: High]
Can this mutex_lock() race with a concurrent timeout handler?
VC4 has separate schedulers for the BIN and RENDER queues. If jobs on
both queues time out simultaneously, Thread A might acquire the lock,
reset the GPU, restart both schedulers, and unlock.
Thread B would block here. Once Thread A releases the lock, Thread B
wakes up and proceeds to use sched_job. However, since Thread A already
restarted the schedulers, the RENDER scheduler could immediately execute
and complete Thread B's job, freeing it.
Wouldn't this cause a use-after-free when Thread B finally acquires the
lock and dereferences sched_job below?
> +
> + /* block scheduler */
> + for (q = 0; q < VC4_MAX_QUEUES; q++)
> + drm_sched_stop(&vc4->queue[q].sched, sched_job);
> +
> + if (sched_job) {
> + drm_sched_increase_karma(sched_job);
> +
> + /* If the guilty job is a BIN job, also increase the karma
> + * of its paired render job. Otherwise, the RENDER job would
> + * be submitted to the GPU without binner output.
> + */
> + if (sched_job->sched == &vc4->queue[VC4_BIN].sched) {
> + struct vc4_bin_job *bin = to_bin_job(sched_job);
> +
> + drm_sched_increase_karma(&bin->render->base.base);
[Severity: High]
Can bin->render be NULL here?
If userspace submits a job that only has a BIN component, or if the
RENDER job allocation failed prior to submission, won't dereferencing
bin->render->base.base result in a kernel panic?
[ ... ]
> +static enum drm_gpu_sched_stat
> +vc4_cl_job_timedout(struct drm_sched_job *sched_job, enum vc4_queue q)
> +{
> + struct vc4_job *job = to_vc4_job(sched_job);
> + struct vc4_dev *vc4 = job->vc4;
> + u32 ctca = V3D_READ(V3D_CTNCA(q));
> + u32 ctra = V3D_READ(V3D_CTNRA0(q));
> +
> + /* If the current address or return address have changed, then the GPU
> + * has probably made progress and we should delay the reset. This could
> + * fail if the GPU got in an infinite loop in the CL, but that is pretty
> + * unlikely outside of an i-g-t testcase.
> + */
> + if (job->timedout_ctca != ctca || job->timedout_ctra != ctra) {
[Severity: High]
This is a pre-existing issue, but does this logic safely handle infinite
loops in command lists?
It assumes any change in the execution address means forward progress. If
an unprivileged user submits a command list with a backward branch, the
execution address will constantly change. Won't this cause the timeout
handler to infinitely defer the reset, resulting in a permanent GPU
lockup and a denial of service?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 4/8] drm/vc4: Add new job submission implementation
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
` (2 preceding siblings ...)
2026-07-12 15:14 ` [PATCH v3 3/8] drm/vc4: Add DRM GPU scheduler infrastructure and job structures Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:30 ` sashiko-bot
2026-07-12 15:14 ` [PATCH v3 5/8] drm/vc4: Refcount vc4_file for safe access by jobs Maíra Canal
` (3 subsequent siblings)
7 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Introduce vc4_submit.c with the job submission path rewritten to
integrate with the DRM GPU scheduler. Most of this code is adapted from
vc4_gem.c, with key changes concentrated in the job creation and
lifecycle management. This implementation follows the same design as the
v3d driver.
This code coexists with the legacy path until the switchover commit.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_submit.c | 577 +++++++++++++++++++++++++++++++++++++++
1 file changed, 577 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
new file mode 100644
index 000000000000..5e839db2ba4d
--- /dev/null
+++ b/drivers/gpu/drm/vc4/vc4_submit.c
@@ -0,0 +1,577 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright © 2014 Broadcom
+ * Copyright © 2026 Raspberry Pi
+ */
+
+#include <drm/drm_exec.h>
+#include <drm/drm_print.h>
+#include <drm/drm_syncobj.h>
+
+#include "vc4_drv.h"
+#include "vc4_trace.h"
+
+/* Takes the reservation lock on all the BOs being referenced, so that
+ * at queue submit time we can update the reservations.
+ *
+ * We don't lock the RCL the tile alloc/state BOs, or overflow memory
+ * (all of which are on render->unref_list). They're entirely private
+ * to vc4, so we don't attach dma-buf fences to them.
+ */
+static int
+vc4_lock_bo_reservations(struct vc4_exec_info *exec, struct drm_exec *exec_ctx)
+{
+ struct vc4_render_job *render = exec->render;
+ int ret;
+
+ /* Reserve space for our shared (read-only) fence references,
+ * before we commit the CL to the hardware.
+ */
+ drm_exec_init(exec_ctx, DRM_EXEC_INTERRUPTIBLE_WAIT, render->bo_count);
+ drm_exec_until_all_locked(exec_ctx) {
+ ret = drm_exec_prepare_array(exec_ctx, render->bo, render->bo_count, 1);
+ }
+
+ if (ret)
+ drm_exec_fini(exec_ctx);
+
+ return ret;
+}
+
+static int
+vc4_add_implicit_dependencies(struct vc4_exec_info *exec)
+{
+ struct vc4_render_job *render = exec->render;
+ struct vc4_bin_job *bin = exec->bin;
+ struct drm_sched_job *reader;
+ int ret;
+
+ /*
+ * Both the renderer and the binner only read the looked-up BOs, so
+ * they need to wait for prior writes to land before reading them.
+ *
+ * When a BIN job is present, the RENDER job already depends on
+ * the BIN job's completion fence and thus transitively waits for
+ * everything the BIN job waits for.
+ */
+ reader = bin ? &bin->base.base : &render->base.base;
+
+ for (int i = 0; i < render->bo_count; i++) {
+ ret = drm_sched_job_add_implicit_dependencies(reader,
+ render->bo[i], false);
+ if (ret)
+ return ret;
+ }
+
+ /*
+ * The renderer writes the RCL targets (color/Z/MSAA buffers), so it
+ * additionally has to wait for prior readers of those.
+ */
+ for (int i = 0; i < render->rcl_write_bo_count; i++) {
+ ret = drm_sched_job_add_implicit_dependencies(&render->base.base,
+ &render->rcl_write_bo[i]->base,
+ true);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+/**
+ * vc4_lookup_bos() - Sets up job->bo[] with the GEM objects
+ * referenced by the job.
+ * @dev: DRM device
+ * @file_priv: DRM file for this fd
+ * @job: VC4 render job being set up
+ * @bo_handles: GEM handles
+ * @bo_count: Number of GEM handles passed in
+ *
+ * The command validator needs to reference BOs by their index within
+ * the submitted job's BO list. This does the validation of the job's
+ * BO list and reference counting for the lifetime of the job.
+ *
+ * Note that this function doesn't need to unreference the BOs on
+ * failure, because that will happen at `vc4_job_free()`.
+ */
+static int
+vc4_lookup_bos(struct drm_device *dev, struct drm_file *file_priv,
+ struct vc4_render_job *job, u64 bo_handles, u32 bo_count)
+{
+ int i, ret;
+
+ if (!bo_count) {
+ drm_dbg(dev, "Rendering requires BOs to validate\n");
+ return -EINVAL;
+ }
+
+ job->bo_count = bo_count;
+
+ ret = drm_gem_objects_lookup(file_priv, u64_to_user_ptr(bo_handles),
+ job->bo_count, &job->bo);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < job->bo_count; i++) {
+ ret = vc4_bo_inc_usecnt(to_vc4_bo(job->bo[i]));
+ if (ret)
+ goto fail_dec_usecnt;
+ }
+
+ return 0;
+
+fail_dec_usecnt:
+ /* Decrease usecnt on acquired objects */
+ for (i--; i >= 0; i--)
+ vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i]));
+ return ret;
+}
+
+static int
+vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
+{
+ u32 shader_rec_offset, uniforms_offset, exec_size, bin_size;
+ struct drm_vc4_submit_cl *args = exec->args;
+ struct vc4_render_job *render_job = exec->render;
+ struct vc4_bin_job *bin_job = exec->bin;
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct drm_gem_dma_object *exec_bo;
+ struct vc4_bo *bo;
+ void *bin = NULL;
+ int ret;
+
+ shader_rec_offset = roundup(args->bin_cl_size, 16);
+ uniforms_offset = shader_rec_offset + args->shader_rec_size;
+ exec_size = uniforms_offset + args->uniforms_size;
+ bin_size = exec_size + (sizeof(struct vc4_shader_state) * args->shader_rec_count);
+
+ if (shader_rec_offset < args->bin_cl_size ||
+ uniforms_offset < shader_rec_offset ||
+ exec_size < uniforms_offset ||
+ args->shader_rec_count >= (UINT_MAX / sizeof(struct vc4_shader_state)) ||
+ bin_size < exec_size) {
+ drm_dbg(dev, "overflow in exec arguments\n");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Allocate space where we'll store the copied in user command lists
+ * and shader records.
+ *
+ * We don't just copy directly into the BOs because we need to
+ * read the contents back for validation, and I think the
+ * bo->vaddr is uncached access.
+ */
+ bin = kvmalloc(bin_size, GFP_KERNEL);
+ if (!bin) {
+ ret = -ENOMEM;
+ goto fail;
+ }
+ exec->shader_rec_u = bin + shader_rec_offset;
+ exec->uniforms_u = bin + uniforms_offset;
+ exec->shader_state = bin + exec_size;
+ exec->shader_state_size = args->shader_rec_count;
+
+ if (copy_from_user(bin, u64_to_user_ptr(args->bin_cl),
+ args->bin_cl_size)) {
+ ret = -EFAULT;
+ goto fail;
+ }
+
+ if (copy_from_user(exec->shader_rec_u, u64_to_user_ptr(args->shader_rec),
+ args->shader_rec_size)) {
+ ret = -EFAULT;
+ goto fail;
+ }
+
+ if (copy_from_user(exec->uniforms_u, u64_to_user_ptr(args->uniforms),
+ args->uniforms_size)) {
+ ret = -EFAULT;
+ goto fail;
+ }
+
+ bo = vc4_bo_create(dev, exec_size, true, VC4_BO_TYPE_BCL);
+ if (IS_ERR(bo)) {
+ ret = PTR_ERR(bo);
+ goto fail;
+ }
+ exec_bo = &bo->base;
+
+ list_add_tail(&bo->unref_head, &render_job->unref_list);
+
+ bin_job->ct0ca = exec_bo->dma_addr;
+
+ exec->bin_u = bin;
+
+ exec->shader_rec_v = exec_bo->vaddr + shader_rec_offset;
+ exec->shader_rec_p = exec_bo->dma_addr + shader_rec_offset;
+ exec->shader_rec_size = args->shader_rec_size;
+
+ exec->uniforms_v = exec_bo->vaddr + uniforms_offset;
+ exec->uniforms_p = exec_bo->dma_addr + uniforms_offset;
+ exec->uniforms_size = args->uniforms_size;
+
+ ret = vc4_validate_bin_cl(dev, exec_bo->vaddr, bin, exec);
+ if (ret)
+ goto fail;
+
+ ret = vc4_validate_shader_recs(dev, exec);
+ if (ret)
+ goto fail;
+
+ if (exec->found_tile_binning_mode_config_packet) {
+ ret = vc4_v3d_bin_bo_get(vc4, &bin_job->bin_bo_used);
+ if (ret)
+ goto fail;
+ }
+
+fail:
+ kvfree(bin);
+ exec->bin_u = NULL;
+
+ return ret;
+}
+
+static void
+vc4_job_free(struct kref *ref)
+{
+ struct vc4_job *job = container_of(ref, struct vc4_job, refcount);
+
+ dma_fence_put(job->irq_fence);
+ dma_fence_put(job->done_fence);
+
+ if (job->perfmon)
+ vc4_perfmon_put(job->perfmon);
+
+ vc4_v3d_pm_put(job->vc4);
+ kfree(job);
+}
+
+static void
+vc4_bin_job_free(struct kref *ref)
+{
+ struct vc4_bin_job *job = container_of(ref, struct vc4_bin_job,
+ base.refcount);
+ struct vc4_dev *vc4 = job->base.vc4;
+
+ /* Release the reference on the binner BO if needed. */
+ if (job->bin_bo_used)
+ vc4_v3d_bin_bo_put(vc4);
+
+ vc4_job_free(ref);
+}
+
+static void
+vc4_render_job_free(struct kref *ref)
+{
+ struct vc4_render_job *job = container_of(ref, struct vc4_render_job,
+ base.refcount);
+ struct vc4_dev *vc4 = job->base.vc4;
+ struct vc4_bo *bo, *tmp;
+ unsigned long irqflags;
+
+ if (job->bo) {
+ for (int i = 0; i < job->bo_count; i++) {
+ struct vc4_bo *bo = to_vc4_bo(job->bo[i]);
+
+ vc4_bo_dec_usecnt(bo);
+ drm_gem_object_put(job->bo[i]);
+ }
+ kvfree(job->bo);
+ }
+
+ list_for_each_entry_safe(bo, tmp, &job->unref_list, unref_head) {
+ list_del(&bo->unref_head);
+ drm_gem_object_put(&bo->base.base);
+ }
+
+ /* Free up the allocation of any bin slots we used. */
+ spin_lock_irqsave(&vc4->job_lock, irqflags);
+ vc4->bin_alloc_used &= ~job->bin_slots;
+ spin_unlock_irqrestore(&vc4->job_lock, irqflags);
+
+ vc4_job_free(ref);
+}
+
+static void
+vc4_job_put(struct vc4_job *job)
+{
+ if (IS_ERR_OR_NULL(job))
+ return;
+
+ kref_put(&job->refcount, job->free);
+}
+
+void vc4_job_cleanup(struct vc4_job *job)
+{
+ if (IS_ERR_OR_NULL(job))
+ return;
+
+ drm_sched_job_cleanup(&job->base);
+ vc4_job_put(job);
+}
+
+static const struct {
+ size_t size;
+ void (*free)(struct kref *ref);
+} vc4_job_types[] = {
+ [VC4_BIN] = { sizeof(struct vc4_bin_job), vc4_bin_job_free },
+ [VC4_RENDER] = { sizeof(struct vc4_render_job), vc4_render_job_free },
+};
+
+static struct vc4_job *
+vc4_job_alloc(struct vc4_dev *vc4, struct drm_file *file_priv, u32 in_sync,
+ enum vc4_queue queue)
+{
+ struct vc4_file *vc4_priv = file_priv->driver_priv;
+ struct vc4_job *job;
+ int ret;
+
+ if (queue >= VC4_MAX_QUEUES)
+ return ERR_PTR(-EINVAL);
+
+ job = kzalloc(vc4_job_types[queue].size, GFP_KERNEL);
+ if (!job)
+ return ERR_PTR(-ENOMEM);
+
+ job->vc4 = vc4;
+ job->free = vc4_job_types[queue].free;
+
+ ret = drm_sched_job_init(&job->base, &vc4_priv->sched_entity[queue],
+ 1, vc4_priv, file_priv->client_id);
+ if (ret)
+ goto fail_free;
+
+ if (in_sync > 0) {
+ ret = drm_sched_job_add_syncobj_dependency(&job->base, file_priv,
+ in_sync, 0);
+ if (ret)
+ goto fail_deps;
+ }
+
+ ret = vc4_v3d_pm_get(vc4);
+ if (ret)
+ goto fail_deps;
+
+ kref_init(&job->refcount);
+
+ return job;
+
+fail_deps:
+ drm_sched_job_cleanup(&job->base);
+fail_free:
+ kfree(job);
+ return ERR_PTR(ret);
+}
+
+static int
+vc4_push_jobs(struct vc4_exec_info *exec)
+{
+ struct vc4_render_job *render = exec->render;
+ struct vc4_job *jobs[VC4_MAX_QUEUES];
+ int ret, num_jobs = 0;
+
+ if (exec->bin)
+ jobs[num_jobs++] = &exec->bin->base;
+ jobs[num_jobs++] = &render->base;
+
+ for (int i = 0; i < num_jobs; i++) {
+ drm_sched_job_arm(&jobs[i]->base);
+
+ jobs[i]->done_fence = dma_fence_get(&jobs[i]->base.s_fence->finished);
+
+ /* put by scheduler job completion */
+ kref_get(&jobs[i]->refcount);
+ }
+
+ if (exec->bin) {
+ ret = drm_sched_job_add_dependency(&render->base.base,
+ dma_fence_get(exec->bin->base.done_fence));
+ if (ret)
+ goto err;
+ }
+
+ for (int i = 0; i < num_jobs; i++)
+ drm_sched_entity_push_job(&jobs[i]->base);
+
+ return 0;
+
+err:
+ /* Mark every armed job as failed so run_job() skips execution */
+ for (int i = 0; i < num_jobs; i++) {
+ dma_fence_set_error(&jobs[i]->base.s_fence->finished, ret);
+ drm_sched_entity_push_job(&jobs[i]->base);
+ }
+
+ return ret;
+}
+
+static void
+vc4_attach_fences(struct vc4_render_job *job, struct drm_syncobj *sync_out,
+ struct dma_fence *done_fence)
+{
+ struct vc4_bo *bo;
+ int i;
+
+ for (i = 0; i < job->bo_count; i++) {
+ bo = to_vc4_bo(job->bo[i]);
+ dma_resv_add_fence(bo->base.base.resv, job->base.done_fence,
+ DMA_RESV_USAGE_READ);
+ }
+
+ for (i = 0; i < job->rcl_write_bo_count; i++) {
+ bo = to_vc4_bo(&job->rcl_write_bo[i]->base);
+ dma_resv_add_fence(bo->base.base.resv, job->base.done_fence,
+ DMA_RESV_USAGE_WRITE);
+ }
+
+ if (sync_out) {
+ drm_syncobj_replace_fence(sync_out, done_fence);
+ drm_syncobj_put(sync_out);
+ }
+}
+
+/**
+ * vc4_submit_cl_ioctl() - Submits a job (frame) to VC4.
+ * @dev: DRM device
+ * @data: ioctl argument
+ * @file_priv: DRM file for this fd
+ *
+ * This is the main entrypoint for userspace to submit a 3D frame to
+ * the GPU. Userspace provides the binner command list (if
+ * applicable), and the kernel sets up the render command list to draw
+ * to the framebuffer described in the ioctl, using the command lists
+ * that the 3D engine's binner will produce.
+ */
+int
+vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_file *vc4_priv = file_priv->driver_priv;
+ struct drm_vc4_submit_cl *args = data;
+ struct vc4_exec_info exec = {
+ .dev = vc4,
+ .args = args,
+ };
+ struct vc4_bin_job *bin = NULL;
+ struct vc4_render_job *render = NULL;
+ struct drm_syncobj *sync_out = NULL;
+ struct drm_exec exec_ctx;
+ int ret;
+
+ trace_vc4_submit_cl_ioctl(dev, args->bin_cl_size, args->shader_rec_size,
+ args->bo_handle_count);
+
+ if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
+ return -ENODEV;
+
+ if (!vc4->v3d) {
+ drm_dbg(dev, "VC4_SUBMIT_CL with no VC4 V3D probed\n");
+ return -ENODEV;
+ }
+
+ if ((args->flags & ~(VC4_SUBMIT_CL_USE_CLEAR_COLOR |
+ VC4_SUBMIT_CL_FIXED_RCL_ORDER |
+ VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X |
+ VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y)) != 0) {
+ drm_dbg(dev, "Unknown flags: 0x%02x\n", args->flags);
+ return -EINVAL;
+ }
+
+ if (args->pad2 != 0) {
+ drm_dbg(dev, "Invalid pad: 0x%08x\n", args->pad2);
+ return -EINVAL;
+ }
+
+ if (args->out_sync) {
+ sync_out = drm_syncobj_find(file_priv, args->out_sync);
+ if (!sync_out)
+ return -ENOENT;
+ }
+
+ render = (struct vc4_render_job *)vc4_job_alloc(vc4, file_priv,
+ args->in_sync, VC4_RENDER);
+ if (IS_ERR(render)) {
+ ret = PTR_ERR(render);
+ goto fail;
+ }
+
+ exec.render = render;
+ INIT_LIST_HEAD(&render->unref_list);
+
+ ret = vc4_lookup_bos(dev, file_priv, render, args->bo_handles,
+ args->bo_handle_count);
+ if (ret)
+ goto fail;
+
+ if (args->bin_cl_size != 0) {
+ bin = (struct vc4_bin_job *)vc4_job_alloc(vc4, file_priv,
+ args->in_sync, VC4_BIN);
+ if (IS_ERR(bin)) {
+ ret = PTR_ERR(bin);
+ goto fail;
+ }
+
+ exec.bin = bin;
+ bin->render = render;
+
+ ret = vc4_get_bcl(dev, &exec);
+ if (ret)
+ goto fail;
+ }
+
+ ret = vc4_get_rcl(dev, &exec);
+ if (ret)
+ goto fail;
+
+ if (args->perfmonid) {
+ render->base.perfmon = vc4_perfmon_find(vc4_priv, args->perfmonid);
+ if (!render->base.perfmon) {
+ ret = -ENOENT;
+ goto fail;
+ }
+
+ if (bin) {
+ bin->base.perfmon = render->base.perfmon;
+ vc4_perfmon_get(bin->base.perfmon);
+ }
+ }
+
+ ret = vc4_lock_bo_reservations(&exec, &exec_ctx);
+ if (ret)
+ goto fail;
+
+ ret = vc4_add_implicit_dependencies(&exec);
+ if (ret)
+ goto fail_exec;
+
+ scoped_guard(mutex, &vc4->sched_lock) {
+ ret = vc4_push_jobs(&exec);
+ }
+
+ if (!ret) {
+ vc4_attach_fences(render, sync_out, render->base.done_fence);
+ } else if (sync_out) {
+ /* The jobs were never submitted, so release the unpublished syncobj */
+ drm_syncobj_put(sync_out);
+ }
+
+ drm_exec_fini(&exec_ctx);
+
+ vc4_job_put((void *)bin);
+ vc4_job_put((void *)render);
+
+ return ret;
+
+fail_exec:
+ drm_exec_fini(&exec_ctx);
+fail:
+ vc4_job_cleanup((void *)bin);
+ vc4_job_cleanup((void *)render);
+
+ if (sync_out)
+ drm_syncobj_put(sync_out);
+
+ return ret;
+}
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 4/8] drm/vc4: Add new job submission implementation
2026-07-12 15:14 ` [PATCH v3 4/8] drm/vc4: Add new job submission implementation Maíra Canal
@ 2026-07-12 15:30 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-12 15:30 UTC (permalink / raw)
To: Maíra Canal; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Reference count double-decrement and underflow in job submission error path
- [High] ABI Breakage: `seqno` is not returned to userspace upon successful job submission
- [High] Uninitialized `exec->bo` and `exec->rcl_write_bo_count` fields break job validation and dependency tracking
--
--- Patch [4]: [PATCH v3 4/8] drm/vc4: Add new job submission implementation ---
commit 394b85fa6f0403b5520bcd58bae0ec277c6cabc7
Author: Maíra Canal <mcanal@igalia.com>
drm/vc4: Add new job submission implementation
Introduce vc4_submit.c with the job submission path rewritten to
integrate with the DRM GPU scheduler. Most of this code is adapted from
vc4_gem.c, with key changes concentrated in the job creation and
lifecycle management.
> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c
[ ... ]
> +static int
> +vc4_add_implicit_dependencies(struct vc4_exec_info *exec)
> +{
> + struct vc4_render_job *render = exec->render;
[ ... ]
> + /*
> + * The renderer writes the RCL targets (color/Z/MSAA buffers), so it
> + * additionally has to wait for prior readers of those.
> + */
> + for (int i = 0; i < render->rcl_write_bo_count; i++) {
> + ret = drm_sched_job_add_implicit_dependencies(&render->base.base,
> + &render->rcl_write_bo[i]->base,
> + true);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
[Severity: High]
Does this loop skip adding implicit dependencies?
The validation code in vc4_get_rcl() appears to populate
exec->rcl_write_bo_count, but this loop iterates over
render->rcl_write_bo_count, which remains 0.
[ ... ]
> +static int
> +vc4_lookup_bos(struct drm_device *dev, struct drm_file *file_priv,
> + struct vc4_render_job *job, u64 bo_handles, u32 bo_count)
> +{
[ ... ]
> + for (i = 0; i < job->bo_count; i++) {
> + ret = vc4_bo_inc_usecnt(to_vc4_bo(job->bo[i]));
> + if (ret)
> + goto fail_dec_usecnt;
> + }
> +
> + return 0;
> +
> +fail_dec_usecnt:
> + /* Decrease usecnt on acquired objects */
> + for (i--; i >= 0; i--)
> + vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i]));
> + return ret;
> +}
[ ... ]
> +static void
> +vc4_render_job_free(struct kref *ref)
> +{
> + struct vc4_render_job *job = container_of(ref, struct vc4_render_job,
> + base.refcount);
> + struct vc4_dev *vc4 = job->base.vc4;
> + struct vc4_bo *bo, *tmp;
> + unsigned long irqflags;
> +
> + if (job->bo) {
> + for (int i = 0; i < job->bo_count; i++) {
> + struct vc4_bo *bo = to_vc4_bo(job->bo[i]);
> +
> + vc4_bo_dec_usecnt(bo);
[Severity: High]
If vc4_bo_inc_usecnt() fails in vc4_lookup_bos(), the error path rolls back
the increments but leaves job->bo fully populated.
When the error propagates up and triggers vc4_render_job_free(), the cleanup
code iterates over job->bo and unconditionally decrements the usecnt again.
Could this cause a double-decrement and underflow for the BOs?
[ ... ]
> +int
> +vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
[ ... ]
> + exec.render = render;
> + INIT_LIST_HEAD(&render->unref_list);
> +
> + ret = vc4_lookup_bos(dev, file_priv, render, args->bo_handles,
> + args->bo_handle_count);
> + if (ret)
> + goto fail;
[Severity: High]
This call populates render->bo, but does it leave exec.bo uninitialized and
exec.bo_count as 0?
If validation functions like vc4_get_bcl() and vc4_get_rcl() still rely on
the legacy exec->bo array via vc4_use_bo(), this could cause shader
validation to fail with -EINVAL.
[ ... ]
> + scoped_guard(mutex, &vc4->sched_lock) {
> + ret = vc4_push_jobs(&exec);
> + }
> +
> + if (!ret) {
> + vc4_attach_fences(render, sync_out, render->base.done_fence);
> + } else if (sync_out) {
> + /* The jobs were never submitted, so release the unpublished syncobj */
> + drm_syncobj_put(sync_out);
> + }
[Severity: High]
Are we missing the sequence number assignment to userspace here?
The drm_vc4_submit_cl structure expects the kernel to return the sequence
number of the submitted job in the seqno field.
Since args->seqno is not populated before returning success, could this
prevent userspace from properly waiting on the submitted job?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 5/8] drm/vc4: Refcount vc4_file for safe access by jobs
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
` (3 preceding siblings ...)
2026-07-12 15:14 ` [PATCH v3 4/8] drm/vc4: Add new job submission implementation Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:14 ` [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking Maíra Canal
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
This is a preparation commit for per-file descriptor seqno tracking,
which will require the struct vc4_render_job to store a pointer to
vc4_file. Since the scheduler's free_job callback runs asynchronously,
it can execute after vc4_close() has already freed the vc4_file, causing
a use-after-free.
Add kref-based reference counting to vc4_file so that jobs can safely
take a reference at submit time via vc4_file_get() and drop it in their
free callback via vc4_file_put().
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_drv.c | 12 ++++++++++--
drivers/gpu/drm/vc4/vc4_drv.h | 17 +++++++++++++++++
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index cacd4cb94259..6c49304301c9 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -167,6 +167,7 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
goto err_sched;
}
+ kref_init(&vc4file->refcount);
vc4_perfmon_open_file(vc4file);
file->driver_priv = vc4file;
@@ -180,6 +181,14 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
}
+void vc4_file_release(struct kref *ref)
+{
+ struct vc4_file *vc4file = container_of(ref, struct vc4_file, refcount);
+
+ vc4_perfmon_close_file(vc4file);
+ kfree(vc4file);
+}
+
static void vc4_close(struct drm_device *dev, struct drm_file *file)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
@@ -195,8 +204,7 @@ static void vc4_close(struct drm_device *dev, struct drm_file *file)
for (q = 0; q < VC4_MAX_QUEUES; q++)
drm_sched_entity_destroy(&vc4file->sched_entity[q]);
- vc4_perfmon_close_file(vc4file);
- kfree(vc4file);
+ vc4_file_put(vc4file);
}
DEFINE_DRM_GEM_FOPS(vc4_drm_fops);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 255a95f55586..4f452ea3b4a6 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -905,6 +905,8 @@ struct vc4_exec_info {
struct vc4_file {
struct vc4_dev *dev;
+ struct kref refcount;
+
struct xarray perfmons;
struct drm_sched_entity sched_entity[VC4_MAX_QUEUES];
@@ -1092,6 +1094,21 @@ static inline void vc4_debugfs_add_regset32(struct drm_device *drm,
/* vc4_drv.c */
void __iomem *vc4_ioremap_regs(struct platform_device *dev, int index);
int vc4_dumb_fixup_args(struct drm_mode_create_dumb *args);
+void vc4_file_release(struct kref *ref);
+
+static inline
+struct vc4_file *vc4_file_get(struct vc4_file *vc4file)
+{
+ kref_get(&vc4file->refcount);
+ return vc4file;
+}
+
+static inline
+void vc4_file_put(struct vc4_file *vc4file)
+{
+ kref_put(&vc4file->refcount, vc4_file_release);
+}
+
/* vc4_dpi.c */
extern struct platform_driver vc4_dpi_driver;
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
` (4 preceding siblings ...)
2026-07-12 15:14 ` [PATCH v3 5/8] drm/vc4: Refcount vc4_file for safe access by jobs Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:28 ` sashiko-bot
2026-07-12 15:14 ` [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
2026-07-12 15:14 ` [PATCH v3 8/8] drm/vc4: Use unique fence timeline names per queue Maíra Canal
7 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Implement per-file descriptor seqno tracking using an xarray, allowing
userspace to wait on specific job completions via vc4_wait_seqno_ioctl.
While this interface should ideally be deprecated in favor of syncobjs,
it has long been exposed to userspace and therefore must continue to be
supported.
Lay the groundwork to replace the existing `finished_seqno` logic with
DMA fence-based tracking. Each allocated seqno is associated with the
job's done_fence in a per-fd xarray. This allows vc4_wait_seqno_ioctl()
to simply look up the corresponding fence and wait on it.
This changes seqno semantics from global to per-file descriptor.
However, this doesn't affect userspace because a client can only wait
on seqnos returned from its own submissions.
Having per-fd seqnos is an intentional choice, as job ordering with the
DRM scheduler can only be guaranteed within an entity, which is per-fd.
Jobs from different file descriptors can complete out of order, so a
global seqno would break user-space expectations. Therefore, using
per-fd seqnos guarantees that the seqno monotonicity contract holds.
This design is inspired by the user fence handling in the Etnaviv and
msm drivers.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_drv.c | 2 ++
drivers/gpu/drm/vc4/vc4_drv.h | 11 ++++++
drivers/gpu/drm/vc4/vc4_submit.c | 77 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 88 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 6c49304301c9..3b0551da4dd0 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,6 +168,7 @@ static int vc4_open(struct drm_device *dev, struct drm_file *file)
}
kref_init(&vc4file->refcount);
+ xa_init_flags(&vc4file->seqno_xa, XA_FLAGS_ALLOC1);
vc4_perfmon_open_file(vc4file);
file->driver_priv = vc4file;
@@ -185,6 +186,7 @@ void vc4_file_release(struct kref *ref)
{
struct vc4_file *vc4file = container_of(ref, struct vc4_file, refcount);
+ xa_destroy(&vc4file->seqno_xa);
vc4_perfmon_close_file(vc4file);
kfree(vc4file);
}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4f452ea3b4a6..fc37410aef9e 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -10,6 +10,7 @@
#include <linux/of.h>
#include <linux/refcount.h>
#include <linux/uaccess.h>
+#include <linux/xarray.h>
#include <drm/drm_atomic.h>
#include <drm/drm_debugfs.h>
@@ -777,6 +778,10 @@ struct vc4_render_job {
* Must remain allocated until the render job completes.
*/
uint32_t bin_slots;
+
+ /* For userspace fence tracking. */
+ struct vc4_file *file;
+ u32 seqno;
};
struct vc4_exec_info {
@@ -911,6 +916,12 @@ struct vc4_file {
struct drm_sched_entity sched_entity[VC4_MAX_QUEUES];
+ /* Mapping of seqno to dma_fence for job completion tracking.
+ * Allows userspace to wait on specific submissions.
+ */
+ struct xarray seqno_xa;
+ u32 next_seqno;
+
bool bin_bo_used;
};
diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
index 5e839db2ba4d..0e43f31fb813 100644
--- a/drivers/gpu/drm/vc4/vc4_submit.c
+++ b/drivers/gpu/drm/vc4/vc4_submit.c
@@ -232,6 +232,57 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
return ret;
}
+int
+vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv)
+{
+ struct vc4_file *vc4_priv = file_priv->driver_priv;
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct drm_vc4_wait_seqno *args = data;
+ unsigned long timeout_jiffies = nsecs_to_jiffies(args->timeout_ns);
+ unsigned long start = jiffies;
+ struct dma_fence *fence;
+ long ret;
+
+ if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
+ return -ENODEV;
+
+ /*
+ * While RCU guarantees the xarray entry won't be freed during the
+ * lookup, it does not prevent the fence's refcount from being
+ * concurrently dropped to zero from the IRQ context.
+ *
+ * dma_fence_get_rcu() pretends we didn't find a fence in that case.
+ */
+ rcu_read_lock();
+ fence = xa_load(&vc4_priv->seqno_xa, args->seqno);
+ if (fence)
+ fence = dma_fence_get_rcu(fence);
+ rcu_read_unlock();
+
+ if (!fence)
+ return 0;
+
+ trace_vc4_wait_for_seqno_begin(dev, args->seqno, args->timeout_ns);
+ ret = dma_fence_wait_timeout(fence, true, timeout_jiffies);
+ trace_vc4_wait_for_seqno_end(dev, args->seqno);
+
+ dma_fence_put(fence);
+
+ if (ret == -ERESTARTSYS) {
+ u64 delta = jiffies_to_nsecs(jiffies - start);
+
+ if (args->timeout_ns >= delta)
+ args->timeout_ns -= delta;
+ else
+ args->timeout_ns = 0;
+
+ return ret;
+ }
+
+ return ret > 0 ? 0 : -ETIME;
+}
+
static void
vc4_job_free(struct kref *ref)
{
@@ -290,6 +341,10 @@ vc4_render_job_free(struct kref *ref)
vc4->bin_alloc_used &= ~job->bin_slots;
spin_unlock_irqrestore(&vc4->job_lock, irqflags);
+ if (job->seqno)
+ xa_erase(&job->file->seqno_xa, job->seqno);
+
+ vc4_file_put(job->file);
vc4_job_free(ref);
}
@@ -365,7 +420,7 @@ vc4_job_alloc(struct vc4_dev *vc4, struct drm_file *file_priv, u32 in_sync,
}
static int
-vc4_push_jobs(struct vc4_exec_info *exec)
+vc4_push_jobs(struct vc4_file *vc4_priv, struct vc4_exec_info *exec)
{
struct vc4_render_job *render = exec->render;
struct vc4_job *jobs[VC4_MAX_QUEUES];
@@ -391,6 +446,14 @@ vc4_push_jobs(struct vc4_exec_info *exec)
goto err;
}
+ /*
+ * The slot was already reserved by xa_alloc_cyclic() (which allocates
+ * the node), so storing the fence into the same index reuses that slot
+ * and cannot fail.
+ */
+ xa_store(&vc4_priv->seqno_xa, render->seqno, render->base.done_fence,
+ GFP_KERNEL);
+
for (int i = 0; i < num_jobs; i++)
drm_sched_entity_push_job(&jobs[i]->base);
@@ -498,6 +561,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
}
exec.render = render;
+ render->file = vc4_file_get(vc4_priv);
INIT_LIST_HEAD(&render->unref_list);
ret = vc4_lookup_bos(dev, file_priv, render, args->bo_handles,
@@ -547,11 +611,20 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
goto fail_exec;
scoped_guard(mutex, &vc4->sched_lock) {
- ret = vc4_push_jobs(&exec);
+ ret = xa_alloc_cyclic(&vc4_priv->seqno_xa, &render->seqno,
+ NULL, xa_limit_32b, &vc4_priv->next_seqno,
+ GFP_KERNEL);
+ if (ret < 0)
+ goto fail_exec;
+
+ ret = vc4_push_jobs(vc4_priv, &exec);
}
if (!ret) {
vc4_attach_fences(render, sync_out, render->base.done_fence);
+
+ /* Return the seqno for our job. */
+ args->seqno = render->seqno;
} else if (sync_out) {
/* The jobs were never submitted, so release the unpublished syncobj */
drm_syncobj_put(sync_out);
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking
2026-07-12 15:14 ` [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking Maíra Canal
@ 2026-07-12 15:28 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-12 15:28 UTC (permalink / raw)
To: Maíra Canal; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Integer truncation on 32-bit platforms breaks infinite waits.
- [High] xa_alloc_cyclic() error path causes data corruption by erasing valid fences.
- [High] GFP_KERNEL memory allocation under dma_resv locks causes deadlocks.
- [Medium] Legitimate negative error codes from dma_fence_wait_timeout are masked to -ETIME.
--
--- Patch [6]: [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking ---
commit fcd8c204d7bfbab0abf15f0e12e9b8db05263bcc
Author: Maíra Canal <mcanal@igalia.com>
drm/vc4: Add per-file descriptor seqno tracking
Implement per-file descriptor seqno tracking using an xarray, allowing
userspace to wait on specific job completions via vc4_wait_seqno_ioctl.
> diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
> index 5e839db2ba4d3..0e43f31fb813f 100644
> --- a/drivers/gpu/drm/vc4/vc4_submit.c
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c
[ ... ]
> @@ -232,6 +232,57 @@ vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
> return ret;
> }
>
> +int
> +vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct vc4_file *vc4_priv = file_priv->driver_priv;
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + struct drm_vc4_wait_seqno *args = data;
> + unsigned long timeout_jiffies = nsecs_to_jiffies(args->timeout_ns);
[Severity: High]
On 32-bit architectures, could this truncate the 64-bit timeout_ns?
If userspace passes ~0ULL for an infinite wait, this would truncate to a
32-bit unsigned long, which then evaluates to a negative signed long when
passed to dma_fence_wait_timeout(), potentially causing the wait to fail
instantly.
> + unsigned long start = jiffies;
> + struct dma_fence *fence;
> + long ret;
> +
[ ... ]
> + if (!fence)
> + return 0;
> +
> + trace_vc4_wait_for_seqno_begin(dev, args->seqno, args->timeout_ns);
> + ret = dma_fence_wait_timeout(fence, true, timeout_jiffies);
> + trace_vc4_wait_for_seqno_end(dev, args->seqno);
> +
> + dma_fence_put(fence);
> +
> + if (ret == -ERESTARTSYS) {
> + u64 delta = jiffies_to_nsecs(jiffies - start);
> +
> + if (args->timeout_ns >= delta)
> + args->timeout_ns -= delta;
> + else
> + args->timeout_ns = 0;
> +
> + return ret;
> + }
> +
> + return ret > 0 ? 0 : -ETIME;
[Severity: Medium]
Does this mask legitimate negative error codes returned by
dma_fence_wait_timeout()?
If dma_fence_wait_timeout() returns an error like -EINVAL for invalid
timeouts or -EDEADLK, this translates it to -ETIME, which might mislead
userspace debugging and error handling.
> +}
> +
[ ... ]
> @@ -290,6 +341,10 @@ vc4_render_job_free(struct kref *ref)
> vc4->bin_alloc_used &= ~job->bin_slots;
> spin_unlock_irqrestore(&vc4->job_lock, irqflags);
>
> + if (job->seqno)
> + xa_erase(&job->file->seqno_xa, job->seqno);
[Severity: High]
Could this unconditionally erase a valid fence belonging to a concurrent
submission if xa_alloc_cyclic() previously failed?
In vc4_submit_cl_ioctl(), xa_alloc_cyclic() can populate render->seqno with
a proposed ID before returning -ENOMEM. The error path would then call this
function, which erases the ID from the xarray. If another thread successfully
used that same ID, its completion fence would be incorrectly removed.
> +
> + vc4_file_put(job->file);
> vc4_job_free(ref);
> }
>
[ ... ]
> @@ -547,11 +611,20 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
> goto fail_exec;
>
> scoped_guard(mutex, &vc4->sched_lock) {
> - ret = vc4_push_jobs(&exec);
> + ret = xa_alloc_cyclic(&vc4_priv->seqno_xa, &render->seqno,
> + NULL, xa_limit_32b, &vc4_priv->next_seqno,
> + GFP_KERNEL);
[Severity: High]
Is it safe to use GFP_KERNEL here and in xa_store() while dma_resv locks are
held?
vc4_submit_cl_ioctl() calls vc4_lock_bo_reservations() before reaching this
code. Using GFP_KERNEL allows direct memory reclaim, which could invoke DRM
shrinkers. If a shrinker attempts to acquire the same dma_resv lock to evict
a buffer object, it could result in an AB-BA deadlock.
> + if (ret < 0)
> + goto fail_exec;
> +
> + ret = vc4_push_jobs(vc4_priv, &exec);
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=6
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
` (5 preceding siblings ...)
2026-07-12 15:14 ` [PATCH v3 6/8] drm/vc4: Add per-file descriptor seqno tracking Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
2026-07-12 15:29 ` sashiko-bot
2026-07-12 15:14 ` [PATCH v3 8/8] drm/vc4: Use unique fence timeline names per queue Maíra Canal
7 siblings, 1 reply; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Wire up the DRM GPU scheduler to the VC4 submission path, replacing the
legacy hand-rolled job queue implementation. The scheduler now handles
job ordering, dependency tracking, and timeout detection, so the legacy
job queues, hangcheck timer, and the custom fence signaling logic can be
removed.
With this change, the `struct vc4_exec_info` is reduced to a container
used only during ioctl validation. Fields that moved to vc4_bin_job and
vc4_render_job (BOs, command list addresses, bin slots, etc.) are
removed from it.
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/Kconfig | 1 +
drivers/gpu/drm/vc4/Makefile | 2 +
drivers/gpu/drm/vc4/vc4_drv.h | 132 +-----
drivers/gpu/drm/vc4/vc4_fence.c | 9 -
drivers/gpu/drm/vc4/vc4_gem.c | 919 ++----------------------------------
drivers/gpu/drm/vc4/vc4_irq.c | 140 ++----
drivers/gpu/drm/vc4/vc4_render_cl.c | 17 +-
drivers/gpu/drm/vc4/vc4_v3d.c | 24 +-
drivers/gpu/drm/vc4/vc4_validate.c | 21 +-
9 files changed, 107 insertions(+), 1158 deletions(-)
diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
index bb8c40be3250..c9839e0dd408 100644
--- a/drivers/gpu/drm/vc4/Kconfig
+++ b/drivers/gpu/drm/vc4/Kconfig
@@ -23,6 +23,7 @@ config DRM_VC4
select SND_SOC_GENERIC_DMAENGINE_PCM
select SND_SOC_HDMI_CODEC
select DRM_MIPI_DSI
+ select DRM_SCHED
help
Choose this option if you have a system that has a Broadcom
VC4 GPU, such as the Raspberry Pi or other BCM2708/BCM2835.
diff --git a/drivers/gpu/drm/vc4/Makefile b/drivers/gpu/drm/vc4/Makefile
index c41f89a15a55..259bbb76c4af 100644
--- a/drivers/gpu/drm/vc4/Makefile
+++ b/drivers/gpu/drm/vc4/Makefile
@@ -19,6 +19,8 @@ vc4-y := \
vc4_perfmon.o \
vc4_plane.o \
vc4_render_cl.o \
+ vc4_sched.o \
+ vc4_submit.o \
vc4_trace_points.o \
vc4_txp.o \
vc4_v3d.o \
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fc37410aef9e..3e96d3601855 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -160,42 +160,12 @@ struct vc4_dev {
struct mutex lock;
} purgeable;
- uint64_t dma_fence_context;
-
- /* Sequence number for the last job queued in bin_job_list.
- * Starts at 0 (no jobs emitted).
- */
- uint64_t emit_seqno;
-
struct vc4_queue_state queue[VC4_MAX_QUEUES];
struct vc4_bin_job *bin_job;
struct vc4_render_job *render_job;
- /* Sequence number for the last completed job on the GPU.
- * Starts at 0 (no jobs completed).
- */
- uint64_t finished_seqno;
-
- /* List of all struct vc4_exec_info for jobs to be executed in
- * the binner. The first job in the list is the one currently
- * programmed into ct0ca for execution.
- */
- struct list_head bin_job_list;
-
- /* List of all struct vc4_exec_info for jobs that have
- * completed binning and are ready for rendering. The first
- * job in the list is the one currently programmed into ct1ca
- * for execution.
- */
- struct list_head render_job_list;
-
- /* List of the finished vc4_exec_infos waiting to be freed by
- * job_done_work.
- */
- struct list_head job_done_list;
-
/* Lock taken when resetting the GPU, to keep multiple
* processes from trying to park the scheduler threads and
* reset at once.
@@ -211,8 +181,6 @@ struct vc4_dev {
* accesses between the IRQ handler and GEM ioctls.
*/
spinlock_t job_lock;
- wait_queue_head_t job_wait_queue;
- struct work_struct job_done_work;
/* Tracks the performance monitor state. The V3D block exposes a single
* set of performance counters, so at most one perfmon can be active at
@@ -260,11 +228,6 @@ struct vc4_dev {
/* Mutex controlling the power refcount. */
struct mutex power_lock;
- struct {
- struct timer_list timer;
- struct work_struct reset_work;
- } hangcheck;
-
struct drm_modeset_lock ctm_state_lock;
struct drm_private_obj ctm_manager;
struct drm_private_obj hvs_channels;
@@ -790,50 +753,14 @@ struct vc4_exec_info {
struct vc4_bin_job *bin;
struct vc4_render_job *render;
- /* Sequence number for this bin/render job. */
- uint64_t seqno;
-
- struct dma_fence *fence;
-
- /* Last current addresses the hardware was processing when the
- * hangcheck timer checked on us.
- */
- uint32_t last_ct0ca, last_ct1ca;
-
/* Kernel-space copy of the ioctl arguments */
struct drm_vc4_submit_cl *args;
- /* This is the array of BOs that were looked up at the start of exec.
- * Command validation will use indices into this array.
- */
- struct drm_gem_object **bo;
- uint32_t bo_count;
-
- /* List of BOs that are being written by the RCL. Other than
- * the binner temporary storage, this is all the BOs written
- * by the job.
- */
- struct drm_gem_dma_object *rcl_write_bo[4];
- uint32_t rcl_write_bo_count;
-
- /* Pointers for our position in vc4->job_list */
- struct list_head head;
-
- /* List of other BOs used in the job that need to be released
- * once the job is complete.
- */
- struct list_head unref_list;
-
/* Current unvalidated indices into @bo loaded by the non-hardware
* VC4_PACKET_GEM_HANDLES.
*/
uint32_t bo_index[2];
- /* This is the BO where we store the validated command lists, shader
- * records, and uniforms.
- */
- struct drm_gem_dma_object *exec_bo;
-
/**
* This tracks the per-shader-record state (packet 64) that
* determines the length of the shader record and the offset
@@ -862,15 +789,6 @@ struct vc4_exec_info {
* (where each tile's binned CL will start)
*/
uint32_t tile_alloc_offset;
- /* Bitmask of which binner slots are freed when this job completes. */
- uint32_t bin_slots;
-
- /**
- * Computed addresses pointing into exec_bo where we start the
- * bin thread (ct0) and render thread (ct1).
- */
- uint32_t ct0ca, ct0ea;
- uint32_t ct1ca, ct1ea;
/* Pointer to the unvalidated bin CL (if present). */
void *bin_u;
@@ -892,16 +810,6 @@ struct vc4_exec_info {
void *uniforms_v;
uint32_t uniforms_p;
uint32_t uniforms_size;
-
- /* Pointer to a performance monitor object if the user requested it,
- * NULL otherwise.
- */
- struct vc4_perfmon *perfmon;
-
- /* Whether the exec has taken a reference to the binner BO, which should
- * happen with a VC4_PACKET_TILE_BINNING_MODE_CONFIG packet.
- */
- bool bin_bo_used;
};
/* Per-open file private data. Any driver-specific resource that has to be
@@ -925,29 +833,6 @@ struct vc4_file {
bool bin_bo_used;
};
-static inline struct vc4_exec_info *
-vc4_first_bin_job(struct vc4_dev *vc4)
-{
- return list_first_entry_or_null(&vc4->bin_job_list,
- struct vc4_exec_info, head);
-}
-
-static inline struct vc4_exec_info *
-vc4_first_render_job(struct vc4_dev *vc4)
-{
- return list_first_entry_or_null(&vc4->render_job_list,
- struct vc4_exec_info, head);
-}
-
-static inline struct vc4_exec_info *
-vc4_last_render_job(struct vc4_dev *vc4)
-{
- if (list_empty(&vc4->render_job_list))
- return NULL;
- return list_last_entry(&vc4->render_job_list,
- struct vc4_exec_info, head);
-}
-
/**
* struct vc4_texture_sample_info - saves the offsets into the UBO for texture
* setup parameters.
@@ -1133,20 +1018,17 @@ struct dma_fence *vc4_fence_create(struct vc4_dev *vc4, enum vc4_queue queue);
/* vc4_gem.c */
int vc4_gem_init(struct drm_device *dev);
-int vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv);
-int vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv);
-void vc4_submit_next_bin_job(struct drm_device *dev);
-void vc4_submit_next_render_job(struct drm_device *dev);
-void vc4_move_job_to_render(struct drm_device *dev, struct vc4_exec_info *exec);
-int vc4_wait_for_seqno(struct drm_device *dev, uint64_t seqno,
- uint64_t timeout_ns, bool interruptible);
-void vc4_job_handle_completed(struct vc4_dev *vc4);
int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv);
void vc4_save_hang_state(struct drm_device *dev);
+/* vc4_submit.c */
+void vc4_job_cleanup(struct vc4_job *job);
+int vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
+int vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
+ struct drm_file *file_priv);
+
/* vc4_hdmi.c */
extern struct platform_driver vc4_hdmi_driver;
diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
index 18e8c53563d6..12f164983238 100644
--- a/drivers/gpu/drm/vc4/vc4_fence.c
+++ b/drivers/gpu/drm/vc4/vc4_fence.c
@@ -51,16 +51,7 @@ static const char *vc4_fence_get_timeline_name(struct dma_fence *fence)
return "vc4-v3d";
}
-static bool vc4_fence_signaled(struct dma_fence *fence)
-{
- struct vc4_fence *f = to_vc4_fence(fence);
- struct vc4_dev *vc4 = to_vc4_dev(f->dev);
-
- return vc4->finished_seqno >= f->seqno;
-}
-
const struct dma_fence_ops vc4_fence_ops = {
.get_driver_name = vc4_fence_get_driver_name,
.get_timeline_name = vc4_fence_get_timeline_name,
- .signaled = vc4_fence_signaled,
};
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 5b5cdfd122e2..1939965dc5fb 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -21,31 +21,12 @@
* IN THE SOFTWARE.
*/
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/pm_runtime.h>
-#include <linux/device.h>
-#include <linux/io.h>
-#include <linux/sched/signal.h>
-#include <linux/dma-fence-array.h>
-
-#include <drm/drm_exec.h>
#include <drm/drm_print.h>
-#include <drm/drm_syncobj.h>
#include "vc4_drv.h"
#include "vc4_regs.h"
#include "vc4_trace.h"
-static void
-vc4_queue_hangcheck(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
-
- mod_timer(&vc4->hangcheck.timer,
- round_jiffies_up(jiffies + msecs_to_jiffies(100)));
-}
-
struct vc4_hang_state {
struct drm_vc4_get_hang_state user_state;
@@ -156,10 +137,11 @@ vc4_save_hang_state(struct drm_device *dev)
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct drm_vc4_get_hang_state *state;
struct vc4_hang_state *kernel_state;
- struct vc4_exec_info *exec[2];
+ struct vc4_bin_job *bin_job;
+ struct vc4_render_job *render_job;
struct vc4_bo *bo;
unsigned long irqflags;
- unsigned int i, j, k, unref_list_count;
+ unsigned int i, k, unref_list_count;
kernel_state = kzalloc_objs(*kernel_state, 1);
if (!kernel_state)
@@ -168,21 +150,16 @@ vc4_save_hang_state(struct drm_device *dev)
state = &kernel_state->user_state;
spin_lock_irqsave(&vc4->job_lock, irqflags);
- exec[0] = vc4_first_bin_job(vc4);
- exec[1] = vc4_first_render_job(vc4);
- if (!exec[0] && !exec[1])
+ bin_job = vc4->bin_job;
+ render_job = vc4->render_job;
+ if (!bin_job && !render_job)
goto err_free_state;
- /* Get the bos from both binner and renderer into hang state. */
+ /* Get the BOs from the render job into hang state. */
state->bo_count = 0;
- for (i = 0; i < 2; i++) {
- if (!exec[i])
- continue;
-
- unref_list_count = 0;
- list_for_each_entry(bo, &exec[i]->unref_list, unref_head)
- unref_list_count++;
- state->bo_count += exec[i]->bo_count + unref_list_count;
+ if (render_job) {
+ unref_list_count = list_count_nodes(&render_job->unref_list);
+ state->bo_count += render_job->bo_count + unref_list_count;
}
kernel_state->bo = kzalloc_objs(*kernel_state->bo, state->bo_count,
@@ -192,12 +169,9 @@ vc4_save_hang_state(struct drm_device *dev)
goto err_free_state;
k = 0;
- for (i = 0; i < 2; i++) {
- if (!exec[i])
- continue;
-
- for (j = 0; j < exec[i]->bo_count; j++) {
- bo = to_vc4_bo(exec[i]->bo[j]);
+ if (render_job) {
+ for (i = 0; i < render_job->bo_count; i++) {
+ bo = to_vc4_bo(render_job->bo[i]);
/* Retain BOs just in case they were marked purgeable.
* This prevents the BO from being purged before
@@ -205,11 +179,11 @@ vc4_save_hang_state(struct drm_device *dev)
*/
WARN_ON(!refcount_read(&bo->usecnt));
refcount_inc(&bo->usecnt);
- drm_gem_object_get(exec[i]->bo[j]);
- kernel_state->bo[k++] = exec[i]->bo[j];
+ drm_gem_object_get(render_job->bo[i]);
+ kernel_state->bo[k++] = render_job->bo[i];
}
- list_for_each_entry(bo, &exec[i]->unref_list, unref_head) {
+ list_for_each_entry(bo, &render_job->unref_list, unref_head) {
/* No need to retain BOs coming from the ->unref_list
* because they are naturally unpurgeable.
*/
@@ -220,10 +194,10 @@ vc4_save_hang_state(struct drm_device *dev)
WARN_ON_ONCE(k != state->bo_count);
- if (exec[0])
- state->start_bin = exec[0]->ct0ca;
- if (exec[1])
- state->start_render = exec[1]->ct1ca;
+ if (bin_job)
+ state->start_bin = bin_job->ct0ca;
+ if (render_job)
+ state->start_render = render_job->ct1ca;
spin_unlock_irqrestore(&vc4->job_lock, irqflags);
@@ -289,840 +263,6 @@ vc4_save_hang_state(struct drm_device *dev)
kfree(kernel_state);
}
-static void
-vc4_reset(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
-
- DRM_INFO("Resetting GPU.\n");
-
- mutex_lock(&vc4->power_lock);
- if (vc4->power_refcount) {
- /* Power the device off and back on the by dropping the
- * reference on runtime PM.
- */
- pm_runtime_put_sync_suspend(&vc4->v3d->pdev->dev);
- pm_runtime_get_sync(&vc4->v3d->pdev->dev);
- }
- mutex_unlock(&vc4->power_lock);
-
- vc4_irq_reset(dev);
-
- /* Rearm the hangcheck -- another job might have been waiting
- * for our hung one to get kicked off, and vc4_irq_reset()
- * would have started it.
- */
- vc4_queue_hangcheck(dev);
-}
-
-static void
-vc4_reset_work(struct work_struct *work)
-{
- struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, hangcheck.reset_work);
-
- vc4_save_hang_state(&vc4->base);
-
- vc4_reset(&vc4->base);
-}
-
-static void
-vc4_hangcheck_elapsed(struct timer_list *t)
-{
- struct vc4_dev *vc4 = timer_container_of(vc4, t, hangcheck.timer);
- struct drm_device *dev = &vc4->base;
- uint32_t ct0ca, ct1ca;
- unsigned long irqflags;
- struct vc4_exec_info *bin_exec, *render_exec;
-
- spin_lock_irqsave(&vc4->job_lock, irqflags);
-
- bin_exec = vc4_first_bin_job(vc4);
- render_exec = vc4_first_render_job(vc4);
-
- /* If idle, we can stop watching for hangs. */
- if (!bin_exec && !render_exec) {
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
- return;
- }
-
- ct0ca = V3D_READ(V3D_CTNCA(0));
- ct1ca = V3D_READ(V3D_CTNCA(1));
-
- /* If we've made any progress in execution, rearm the timer
- * and wait.
- */
- if ((bin_exec && ct0ca != bin_exec->last_ct0ca) ||
- (render_exec && ct1ca != render_exec->last_ct1ca)) {
- if (bin_exec)
- bin_exec->last_ct0ca = ct0ca;
- if (render_exec)
- render_exec->last_ct1ca = ct1ca;
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
- vc4_queue_hangcheck(dev);
- return;
- }
-
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
-
- /* We've gone too long with no progress, reset. This has to
- * be done from a work struct, since resetting can sleep and
- * this timer hook isn't allowed to.
- */
- schedule_work(&vc4->hangcheck.reset_work);
-}
-
-static void
-submit_cl(struct drm_device *dev, uint32_t thread, uint32_t start, uint32_t end)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
-
- /* Set the current and end address of the control list.
- * Writing the end register is what starts the job.
- */
- V3D_WRITE(V3D_CTNCA(thread), start);
- V3D_WRITE(V3D_CTNEA(thread), end);
-}
-
-int
-vc4_wait_for_seqno(struct drm_device *dev, uint64_t seqno, uint64_t timeout_ns,
- bool interruptible)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- int ret = 0;
- unsigned long timeout_expire;
- DEFINE_WAIT(wait);
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return -ENODEV;
-
- if (vc4->finished_seqno >= seqno)
- return 0;
-
- if (timeout_ns == 0)
- return -ETIME;
-
- timeout_expire = jiffies + nsecs_to_jiffies(timeout_ns);
-
- trace_vc4_wait_for_seqno_begin(dev, seqno, timeout_ns);
- for (;;) {
- prepare_to_wait(&vc4->job_wait_queue, &wait,
- interruptible ? TASK_INTERRUPTIBLE :
- TASK_UNINTERRUPTIBLE);
-
- if (interruptible && signal_pending(current)) {
- ret = -ERESTARTSYS;
- break;
- }
-
- if (vc4->finished_seqno >= seqno)
- break;
-
- if (timeout_ns != ~0ull) {
- if (time_after_eq(jiffies, timeout_expire)) {
- ret = -ETIME;
- break;
- }
- schedule_timeout(timeout_expire - jiffies);
- } else {
- schedule();
- }
- }
-
- finish_wait(&vc4->job_wait_queue, &wait);
- trace_vc4_wait_for_seqno_end(dev, seqno);
-
- return ret;
-}
-
-static void
-vc4_flush_caches(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
-
- /* Flush the GPU L2 caches. These caches sit on top of system
- * L3 (the 128kb or so shared with the CPU), and are
- * non-allocating in the L3.
- */
- V3D_WRITE(V3D_L2CACTL,
- V3D_L2CACTL_L2CCLR);
-
- V3D_WRITE(V3D_SLCACTL,
- VC4_SET_FIELD(0xf, V3D_SLCACTL_T1CC) |
- VC4_SET_FIELD(0xf, V3D_SLCACTL_T0CC) |
- VC4_SET_FIELD(0xf, V3D_SLCACTL_UCC) |
- VC4_SET_FIELD(0xf, V3D_SLCACTL_ICC));
-}
-
-static void
-vc4_flush_texture_caches(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
-
- V3D_WRITE(V3D_L2CACTL,
- V3D_L2CACTL_L2CCLR);
-
- V3D_WRITE(V3D_SLCACTL,
- VC4_SET_FIELD(0xf, V3D_SLCACTL_T1CC) |
- VC4_SET_FIELD(0xf, V3D_SLCACTL_T0CC));
-}
-
-/* Sets the registers for the next job to be actually be executed in
- * the hardware.
- *
- * The job_lock should be held during this.
- */
-void
-vc4_submit_next_bin_job(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *exec;
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return;
-
-again:
- exec = vc4_first_bin_job(vc4);
- if (!exec)
- return;
-
- vc4_flush_caches(dev);
-
- /* Only start the perfmon if it was not already started by a previous
- * job.
- */
- if (exec->perfmon && vc4->perfmon_state.active != exec->perfmon)
- vc4_perfmon_start(vc4, exec->perfmon);
-
- /* Either put the job in the binner if it uses the binner, or
- * immediately move it to the to-be-rendered queue.
- */
- if (exec->ct0ca != exec->ct0ea) {
- trace_vc4_submit_cl(dev, false, exec->seqno, exec->ct0ca,
- exec->ct0ea);
- submit_cl(dev, 0, exec->ct0ca, exec->ct0ea);
- } else {
- struct vc4_exec_info *next;
-
- vc4_move_job_to_render(dev, exec);
- next = vc4_first_bin_job(vc4);
-
- /* We can't start the next bin job if the previous job had a
- * different perfmon instance attached to it. The same goes
- * if one of them had a perfmon attached to it and the other
- * one doesn't.
- */
- if (next && next->perfmon == exec->perfmon)
- goto again;
- }
-}
-
-void
-vc4_submit_next_render_job(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *exec = vc4_first_render_job(vc4);
-
- if (!exec)
- return;
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return;
-
- /* A previous RCL may have written to one of our textures, and
- * our full cache flush at bin time may have occurred before
- * that RCL completed. Flush the texture cache now, but not
- * the instructions or uniforms (since we don't write those
- * from an RCL).
- */
- vc4_flush_texture_caches(dev);
-
- trace_vc4_submit_cl(dev, true, exec->seqno, exec->ct1ca, exec->ct1ea);
- submit_cl(dev, 1, exec->ct1ca, exec->ct1ea);
-}
-
-void
-vc4_move_job_to_render(struct drm_device *dev, struct vc4_exec_info *exec)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- bool was_empty = list_empty(&vc4->render_job_list);
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return;
-
- list_move_tail(&exec->head, &vc4->render_job_list);
- if (was_empty)
- vc4_submit_next_render_job(dev);
-}
-
-static void
-vc4_attach_fences(struct vc4_exec_info *exec)
-{
- struct vc4_bo *bo;
- unsigned i;
-
- for (i = 0; i < exec->bo_count; i++) {
- bo = to_vc4_bo(exec->bo[i]);
- dma_resv_add_fence(bo->base.base.resv, exec->fence,
- DMA_RESV_USAGE_READ);
- }
-
- for (i = 0; i < exec->rcl_write_bo_count; i++) {
- bo = to_vc4_bo(&exec->rcl_write_bo[i]->base);
- dma_resv_add_fence(bo->base.base.resv, exec->fence,
- DMA_RESV_USAGE_WRITE);
- }
-}
-
-/* Takes the reservation lock on all the BOs being referenced, so that
- * at queue submit time we can update the reservations.
- *
- * We don't lock the RCL the tile alloc/state BOs, or overflow memory
- * (all of which are on exec->unref_list). They're entirely private
- * to vc4, so we don't attach dma-buf fences to them.
- */
-static int
-vc4_lock_bo_reservations(struct vc4_exec_info *exec,
- struct drm_exec *exec_ctx)
-{
- int ret;
-
- /* Reserve space for our shared (read-only) fence references,
- * before we commit the CL to the hardware.
- */
- drm_exec_init(exec_ctx, DRM_EXEC_INTERRUPTIBLE_WAIT, exec->bo_count);
- drm_exec_until_all_locked(exec_ctx) {
- ret = drm_exec_prepare_array(exec_ctx, exec->bo,
- exec->bo_count, 1);
- }
-
- if (ret) {
- drm_exec_fini(exec_ctx);
- return ret;
- }
-
- return 0;
-}
-
-/* Queues a struct vc4_exec_info for execution. If no job is
- * currently executing, then submits it.
- *
- * Unlike most GPUs, our hardware only handles one command list at a
- * time. To queue multiple jobs at once, we'd need to edit the
- * previous command list to have a jump to the new one at the end, and
- * then bump the end address. That's a change for a later date,
- * though.
- */
-static int
-vc4_queue_submit(struct drm_device *dev, struct vc4_exec_info *exec,
- struct drm_exec *exec_ctx,
- struct drm_syncobj *out_sync)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *renderjob;
- uint64_t seqno;
- unsigned long irqflags;
- struct vc4_fence *fence;
-
- fence = kzalloc_obj(*fence);
- if (!fence)
- return -ENOMEM;
- fence->dev = dev;
-
- spin_lock_irqsave(&vc4->job_lock, irqflags);
-
- seqno = ++vc4->emit_seqno;
- exec->seqno = seqno;
-
- dma_fence_init(&fence->base, &vc4_fence_ops, &vc4->job_lock,
- vc4->dma_fence_context, exec->seqno);
- fence->seqno = exec->seqno;
- exec->fence = &fence->base;
-
- if (out_sync)
- drm_syncobj_replace_fence(out_sync, exec->fence);
-
- vc4_attach_fences(exec);
-
- drm_exec_fini(exec_ctx);
-
- list_add_tail(&exec->head, &vc4->bin_job_list);
-
- /* If no bin job was executing and if the render job (if any) has the
- * same perfmon as our job attached to it (or if both jobs don't have
- * perfmon activated), then kick ours off. Otherwise, it'll get
- * started when the previous job's flush/render done interrupt occurs.
- */
- renderjob = vc4_first_render_job(vc4);
- if (vc4_first_bin_job(vc4) == exec &&
- (!renderjob || renderjob->perfmon == exec->perfmon)) {
- vc4_submit_next_bin_job(dev);
- vc4_queue_hangcheck(dev);
- }
-
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
-
- return 0;
-}
-
-/**
- * vc4_cl_lookup_bos() - Sets up exec->bo[] with the GEM objects
- * referenced by the job.
- * @dev: DRM device
- * @file_priv: DRM file for this fd
- * @exec: V3D job being set up
- *
- * The command validator needs to reference BOs by their index within
- * the submitted job's BO list. This does the validation of the job's
- * BO list and reference counting for the lifetime of the job.
- */
-static int
-vc4_cl_lookup_bos(struct drm_device *dev,
- struct drm_file *file_priv,
- struct vc4_exec_info *exec)
-{
- struct drm_vc4_submit_cl *args = exec->args;
- int ret = 0;
- int i;
-
- exec->bo_count = args->bo_handle_count;
-
- if (!exec->bo_count) {
- /* See comment on bo_index for why we have to check
- * this.
- */
- DRM_DEBUG("Rendering requires BOs to validate\n");
- return -EINVAL;
- }
-
- ret = drm_gem_objects_lookup(file_priv, u64_to_user_ptr(args->bo_handles),
- exec->bo_count, &exec->bo);
-
- if (ret)
- goto fail_put_bo;
-
- for (i = 0; i < exec->bo_count; i++) {
- ret = vc4_bo_inc_usecnt(to_vc4_bo(exec->bo[i]));
- if (ret)
- goto fail_dec_usecnt;
- }
-
- return 0;
-
-fail_dec_usecnt:
- /* Decrease usecnt on acquired objects.
- * We cannot rely on vc4_complete_exec() to release resources here,
- * because vc4_complete_exec() has no information about which BO has
- * had its ->usecnt incremented.
- * To make things easier we just free everything explicitly and set
- * exec->bo to NULL so that vc4_complete_exec() skips the 'BO release'
- * step.
- */
- for (i-- ; i >= 0; i--)
- vc4_bo_dec_usecnt(to_vc4_bo(exec->bo[i]));
-
-fail_put_bo:
- /* Release any reference to acquired objects. */
- for (i = 0; i < exec->bo_count && exec->bo[i]; i++)
- drm_gem_object_put(exec->bo[i]);
-
- kvfree(exec->bo);
- exec->bo = NULL;
- return ret;
-}
-
-static int
-vc4_get_bcl(struct drm_device *dev, struct vc4_exec_info *exec)
-{
- struct drm_vc4_submit_cl *args = exec->args;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- void *temp = NULL;
- void *bin;
- int ret = 0;
- uint32_t bin_offset = 0;
- uint32_t shader_rec_offset = roundup(bin_offset + args->bin_cl_size,
- 16);
- uint32_t uniforms_offset = shader_rec_offset + args->shader_rec_size;
- uint32_t exec_size = uniforms_offset + args->uniforms_size;
- uint32_t temp_size = exec_size + (sizeof(struct vc4_shader_state) *
- args->shader_rec_count);
- struct vc4_bo *bo;
-
- if (shader_rec_offset < args->bin_cl_size ||
- uniforms_offset < shader_rec_offset ||
- exec_size < uniforms_offset ||
- args->shader_rec_count >= (UINT_MAX /
- sizeof(struct vc4_shader_state)) ||
- temp_size < exec_size) {
- DRM_DEBUG("overflow in exec arguments\n");
- ret = -EINVAL;
- goto fail;
- }
-
- /* Allocate space where we'll store the copied in user command lists
- * and shader records.
- *
- * We don't just copy directly into the BOs because we need to
- * read the contents back for validation, and I think the
- * bo->vaddr is uncached access.
- */
- temp = kvmalloc_array(temp_size, 1, GFP_KERNEL);
- if (!temp) {
- drm_err(dev, "Failed to allocate storage for copying "
- "in bin/render CLs.\n");
- ret = -ENOMEM;
- goto fail;
- }
- bin = temp + bin_offset;
- exec->shader_rec_u = temp + shader_rec_offset;
- exec->uniforms_u = temp + uniforms_offset;
- exec->shader_state = temp + exec_size;
- exec->shader_state_size = args->shader_rec_count;
-
- if (copy_from_user(bin,
- u64_to_user_ptr(args->bin_cl),
- args->bin_cl_size)) {
- ret = -EFAULT;
- goto fail;
- }
-
- if (copy_from_user(exec->shader_rec_u,
- u64_to_user_ptr(args->shader_rec),
- args->shader_rec_size)) {
- ret = -EFAULT;
- goto fail;
- }
-
- if (copy_from_user(exec->uniforms_u,
- u64_to_user_ptr(args->uniforms),
- args->uniforms_size)) {
- ret = -EFAULT;
- goto fail;
- }
-
- bo = vc4_bo_create(dev, exec_size, true, VC4_BO_TYPE_BCL);
- if (IS_ERR(bo)) {
- drm_err(dev, "Couldn't allocate BO for binning\n");
- ret = PTR_ERR(bo);
- goto fail;
- }
- exec->exec_bo = &bo->base;
-
- list_add_tail(&to_vc4_bo(&exec->exec_bo->base)->unref_head,
- &exec->unref_list);
-
- exec->ct0ca = exec->exec_bo->dma_addr + bin_offset;
-
- exec->bin_u = bin;
-
- exec->shader_rec_v = exec->exec_bo->vaddr + shader_rec_offset;
- exec->shader_rec_p = exec->exec_bo->dma_addr + shader_rec_offset;
- exec->shader_rec_size = args->shader_rec_size;
-
- exec->uniforms_v = exec->exec_bo->vaddr + uniforms_offset;
- exec->uniforms_p = exec->exec_bo->dma_addr + uniforms_offset;
- exec->uniforms_size = args->uniforms_size;
-
- ret = vc4_validate_bin_cl(dev,
- exec->exec_bo->vaddr + bin_offset,
- bin,
- exec);
- if (ret)
- goto fail;
-
- ret = vc4_validate_shader_recs(dev, exec);
- if (ret)
- goto fail;
-
- if (exec->found_tile_binning_mode_config_packet) {
- ret = vc4_v3d_bin_bo_get(vc4, &exec->bin_bo_used);
- if (ret)
- goto fail;
- }
-
-fail:
- kvfree(temp);
- return ret;
-}
-
-static void
-vc4_complete_exec(struct drm_device *dev, struct vc4_exec_info *exec)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- unsigned long irqflags;
- unsigned i;
-
- /* If we got force-completed because of GPU reset rather than
- * through our IRQ handler, signal the fence now.
- */
- if (exec->fence) {
- dma_fence_signal(exec->fence);
- dma_fence_put(exec->fence);
- }
-
- if (exec->bo) {
- for (i = 0; i < exec->bo_count; i++) {
- struct vc4_bo *bo = to_vc4_bo(exec->bo[i]);
-
- vc4_bo_dec_usecnt(bo);
- drm_gem_object_put(exec->bo[i]);
- }
- kvfree(exec->bo);
- }
-
- while (!list_empty(&exec->unref_list)) {
- struct vc4_bo *bo = list_first_entry(&exec->unref_list,
- struct vc4_bo, unref_head);
- list_del(&bo->unref_head);
- drm_gem_object_put(&bo->base.base);
- }
-
- /* Free up the allocation of any bin slots we used. */
- spin_lock_irqsave(&vc4->job_lock, irqflags);
- vc4->bin_alloc_used &= ~exec->bin_slots;
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
-
- /* Release the reference on the binner BO if needed. */
- if (exec->bin_bo_used)
- vc4_v3d_bin_bo_put(vc4);
-
- /* Release the reference we had on the perf monitor. */
- vc4_perfmon_put(exec->perfmon);
-
- vc4_v3d_pm_put(vc4);
-
- kfree(exec);
-}
-
-void
-vc4_job_handle_completed(struct vc4_dev *vc4)
-{
- unsigned long irqflags;
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return;
-
- spin_lock_irqsave(&vc4->job_lock, irqflags);
- while (!list_empty(&vc4->job_done_list)) {
- struct vc4_exec_info *exec =
- list_first_entry(&vc4->job_done_list,
- struct vc4_exec_info, head);
- list_del(&exec->head);
-
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
- vc4_complete_exec(&vc4->base, exec);
- spin_lock_irqsave(&vc4->job_lock, irqflags);
- }
-
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
-}
-
-/* Scheduled when any job has been completed, this walks the list of
- * jobs that had completed and unrefs their BOs and frees their exec
- * structs.
- */
-static void
-vc4_job_done_work(struct work_struct *work)
-{
- struct vc4_dev *vc4 =
- container_of(work, struct vc4_dev, job_done_work);
-
- vc4_job_handle_completed(vc4);
-}
-
-static int
-vc4_wait_for_seqno_ioctl_helper(struct drm_device *dev,
- uint64_t seqno,
- uint64_t *timeout_ns)
-{
- unsigned long start = jiffies;
- int ret = vc4_wait_for_seqno(dev, seqno, *timeout_ns, true);
-
- if ((ret == -EINTR || ret == -ERESTARTSYS) && *timeout_ns != ~0ull) {
- uint64_t delta = jiffies_to_nsecs(jiffies - start);
-
- if (*timeout_ns >= delta)
- *timeout_ns -= delta;
- }
-
- return ret;
-}
-
-int
-vc4_wait_seqno_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct drm_vc4_wait_seqno *args = data;
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return -ENODEV;
-
- return vc4_wait_for_seqno_ioctl_helper(dev, args->seqno,
- &args->timeout_ns);
-}
-
-/**
- * vc4_submit_cl_ioctl() - Submits a job (frame) to the VC4.
- * @dev: DRM device
- * @data: ioctl argument
- * @file_priv: DRM file for this fd
- *
- * This is the main entrypoint for userspace to submit a 3D frame to
- * the GPU. Userspace provides the binner command list (if
- * applicable), and the kernel sets up the render command list to draw
- * to the framebuffer described in the ioctl, using the command lists
- * that the 3D engine's binner will produce.
- */
-int
-vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
- struct drm_file *file_priv)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_file *vc4file = file_priv->driver_priv;
- struct drm_vc4_submit_cl *args = data;
- struct drm_syncobj *out_sync = NULL;
- struct vc4_exec_info *exec;
- struct drm_exec exec_ctx;
- struct dma_fence *in_fence;
- int ret = 0;
-
- trace_vc4_submit_cl_ioctl(dev, args->bin_cl_size,
- args->shader_rec_size,
- args->bo_handle_count);
-
- if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
- return -ENODEV;
-
- if (!vc4->v3d) {
- DRM_DEBUG("VC4_SUBMIT_CL with no VC4 V3D probed\n");
- return -ENODEV;
- }
-
- if ((args->flags & ~(VC4_SUBMIT_CL_USE_CLEAR_COLOR |
- VC4_SUBMIT_CL_FIXED_RCL_ORDER |
- VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X |
- VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y)) != 0) {
- DRM_DEBUG("Unknown flags: 0x%02x\n", args->flags);
- return -EINVAL;
- }
-
- if (args->pad2 != 0) {
- DRM_DEBUG("Invalid pad: 0x%08x\n", args->pad2);
- return -EINVAL;
- }
-
- exec = kzalloc_objs(*exec, 1);
- if (!exec)
- return -ENOMEM;
-
- exec->dev = vc4;
-
- ret = vc4_v3d_pm_get(vc4);
- if (ret) {
- kfree(exec);
- return ret;
- }
-
- exec->args = args;
- INIT_LIST_HEAD(&exec->unref_list);
-
- ret = vc4_cl_lookup_bos(dev, file_priv, exec);
- if (ret)
- goto fail;
-
- if (args->perfmonid) {
- exec->perfmon = vc4_perfmon_find(vc4file,
- args->perfmonid);
- if (!exec->perfmon) {
- ret = -ENOENT;
- goto fail;
- }
- }
-
- if (args->in_sync) {
- ret = drm_syncobj_find_fence(file_priv, args->in_sync,
- 0, 0, &in_fence);
- if (ret)
- goto fail;
-
- /* When the fence (or fence array) is exclusively from our
- * context we can skip the wait since jobs are executed in
- * order of their submission through this ioctl and this can
- * only have fences from a prior job.
- */
- if (!dma_fence_match_context(in_fence,
- vc4->dma_fence_context)) {
- ret = dma_fence_wait(in_fence, true);
- if (ret) {
- dma_fence_put(in_fence);
- goto fail;
- }
- }
-
- dma_fence_put(in_fence);
- }
-
- if (exec->args->bin_cl_size != 0) {
- ret = vc4_get_bcl(dev, exec);
- if (ret)
- goto fail;
- } else {
- exec->ct0ca = 0;
- exec->ct0ea = 0;
- }
-
- ret = vc4_get_rcl(dev, exec);
- if (ret)
- goto fail;
-
- ret = vc4_lock_bo_reservations(exec, &exec_ctx);
- if (ret)
- goto fail;
-
- if (args->out_sync) {
- out_sync = drm_syncobj_find(file_priv, args->out_sync);
- if (!out_sync) {
- ret = -EINVAL;
- goto fail_unreserve;
- }
-
- /* We replace the fence in out_sync in vc4_queue_submit since
- * the render job could execute immediately after that call.
- * If it finishes before our ioctl processing resumes the
- * render job fence could already have been freed.
- */
- }
-
- /* Clear this out of the struct we'll be putting in the queue,
- * since it's part of our stack.
- */
- exec->args = NULL;
-
- ret = vc4_queue_submit(dev, exec, &exec_ctx, out_sync);
-
- /* The syncobj isn't part of the exec data and we need to free our
- * reference even if job submission failed.
- */
- if (out_sync)
- drm_syncobj_put(out_sync);
-
- if (ret)
- goto fail_unreserve;
-
- /* Return the seqno for our job. */
- args->seqno = vc4->emit_seqno;
-
- return 0;
-
-fail_unreserve:
- drm_exec_fini(&exec_ctx);
-fail:
- vc4_complete_exec(&vc4->base, exec);
-
- return ret;
-}
-
static void vc4_gem_destroy(struct drm_device *dev, void *unused);
int vc4_gem_init(struct drm_device *dev)
{
@@ -1132,11 +272,6 @@ int vc4_gem_init(struct drm_device *dev)
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return -ENODEV;
- vc4->dma_fence_context = dma_fence_context_alloc(1);
-
- INIT_LIST_HEAD(&vc4->bin_job_list);
- INIT_LIST_HEAD(&vc4->render_job_list);
- INIT_LIST_HEAD(&vc4->job_done_list);
spin_lock_init(&vc4->job_lock);
spin_lock_init(&vc4->perfmon_state.lock);
ret = drmm_mutex_init(dev, &vc4->reset_lock);
@@ -1146,11 +281,6 @@ int vc4_gem_init(struct drm_device *dev)
if (ret)
return ret;
- INIT_WORK(&vc4->hangcheck.reset_work, vc4_reset_work);
- timer_setup(&vc4->hangcheck.timer, vc4_hangcheck_elapsed, 0);
-
- INIT_WORK(&vc4->job_done_work, vc4_job_done_work);
-
ret = drmm_mutex_init(dev, &vc4->power_lock);
if (ret)
return ret;
@@ -1161,6 +291,10 @@ int vc4_gem_init(struct drm_device *dev)
if (ret)
return ret;
+ ret = vc4_sched_init(vc4);
+ if (ret)
+ return ret;
+
return drmm_add_action_or_reset(dev, vc4_gem_destroy, NULL);
}
@@ -1171,7 +305,8 @@ static void vc4_gem_destroy(struct drm_device *dev, void *unused)
/* Waiting for exec to finish would need to be done before
* unregistering V3D.
*/
- WARN_ON(vc4->emit_seqno != vc4->finished_seqno);
+ WARN_ON(vc4->bin_job);
+ WARN_ON(vc4->render_job);
/* V3D should already have disabled its interrupt and cleared
* the overflow allocation registers. Now free the object.
@@ -1183,6 +318,8 @@ static void vc4_gem_destroy(struct drm_device *dev, void *unused)
if (vc4->hang_state)
vc4_free_hang_state(dev, vc4->hang_state);
+
+ vc4_sched_fini(vc4);
}
int vc4_gem_madvise_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 8e5141bb5075..28b6820291ef 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -64,7 +64,6 @@ vc4_overflow_mem_work(struct work_struct *work)
container_of(work, struct vc4_dev, overflow_mem_work);
struct vc4_bo *bo;
int bin_bo_slot;
- struct vc4_exec_info *exec;
unsigned long irqflags;
mutex_lock(&vc4->bin_bo_lock);
@@ -84,16 +83,14 @@ vc4_overflow_mem_work(struct work_struct *work)
if (vc4->bin_alloc_overflow) {
/* If we had overflow memory allocated previously,
- * then that chunk will free when the current bin job
- * is done. If we don't have a bin job running, then
- * the chunk will be done whenever the list of render
- * jobs has drained.
+ * then that chunk will free when the current render job
+ * is done. If we don't have a render job running, then
+ * the chunk is free immediately.
*/
- exec = vc4_first_bin_job(vc4);
- if (!exec)
- exec = vc4_last_render_job(vc4);
- if (exec) {
- exec->bin_slots |= vc4->bin_alloc_overflow;
+ if (vc4->bin_job) {
+ vc4->bin_job->render->bin_slots |= vc4->bin_alloc_overflow;
+ } else if (vc4->render_job) {
+ vc4->render_job->bin_slots |= vc4->bin_alloc_overflow;
} else {
/* There's nothing queued in the hardware, so
* the old slot is free immediately.
@@ -113,92 +110,6 @@ vc4_overflow_mem_work(struct work_struct *work)
mutex_unlock(&vc4->bin_bo_lock);
}
-static void
-vc4_irq_finish_bin_job(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *next, *exec = vc4_first_bin_job(vc4);
-
- if (!exec)
- return;
-
- trace_vc4_bcl_end_irq(dev, exec->seqno);
-
- vc4_move_job_to_render(dev, exec);
- next = vc4_first_bin_job(vc4);
-
- /* Only submit the next job in the bin list if it matches the perfmon
- * attached to the one that just finished (or if both jobs don't have
- * perfmon attached to them).
- */
- if (next && next->perfmon == exec->perfmon)
- vc4_submit_next_bin_job(dev);
-}
-
-static void
-vc4_cancel_bin_job(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *exec = vc4_first_bin_job(vc4);
-
- if (!exec)
- return;
-
- /* Stop the perfmon so that the next bin job can be started. */
- if (exec->perfmon)
- vc4_perfmon_stop(vc4, exec->perfmon, false);
-
- list_move_tail(&exec->head, &vc4->bin_job_list);
- vc4_submit_next_bin_job(dev);
-}
-
-static void
-vc4_irq_finish_render_job(struct drm_device *dev)
-{
- struct vc4_dev *vc4 = to_vc4_dev(dev);
- struct vc4_exec_info *exec = vc4_first_render_job(vc4);
- struct vc4_exec_info *nextbin, *nextrender;
-
- if (!exec)
- return;
-
- trace_vc4_rcl_end_irq(dev, exec->seqno);
-
- vc4->finished_seqno++;
- list_move_tail(&exec->head, &vc4->job_done_list);
-
- nextbin = vc4_first_bin_job(vc4);
- nextrender = vc4_first_render_job(vc4);
-
- /* Only stop the perfmon if following jobs in the queue don't expect it
- * to be enabled.
- */
- if (exec->perfmon && !nextrender &&
- (!nextbin || nextbin->perfmon != exec->perfmon))
- vc4_perfmon_stop(vc4, exec->perfmon, true);
-
- /* If there's a render job waiting, start it. If this is not the case
- * we may have to unblock the binner if it's been stalled because of
- * perfmon (this can be checked by comparing the perfmon attached to
- * the finished renderjob to the one attached to the next bin job: if
- * they don't match, this means the binner is stalled and should be
- * restarted).
- */
- if (nextrender)
- vc4_submit_next_render_job(dev);
- else if (nextbin && nextbin->perfmon != exec->perfmon)
- vc4_submit_next_bin_job(dev);
-
- if (exec->fence) {
- dma_fence_signal_locked(exec->fence);
- dma_fence_put(exec->fence);
- exec->fence = NULL;
- }
-
- wake_up_all(&vc4->job_wait_queue);
- schedule_work(&vc4->job_done_work);
-}
-
static irqreturn_t
vc4_irq(int irq, void *arg)
{
@@ -225,16 +136,42 @@ vc4_irq(int irq, void *arg)
}
if (intctl & V3D_INT_FLDONE) {
+ struct vc4_bin_job *bin_job;
+ struct vc4_fence *fence;
+
spin_lock(&vc4->job_lock);
- vc4_irq_finish_bin_job(dev);
+ bin_job = vc4->bin_job;
+ vc4->bin_job = NULL;
spin_unlock(&vc4->job_lock);
+
+ fence = to_vc4_fence(bin_job->base.irq_fence);
+
+ vc4_perfmon_stop(vc4, bin_job->base.perfmon, true);
+
+ trace_vc4_bcl_end_irq(dev, fence->seqno);
+
+ dma_fence_signal(&fence->base);
+
status = IRQ_HANDLED;
}
if (intctl & V3D_INT_FRDONE) {
+ struct vc4_render_job *render_job;
+ struct vc4_fence *fence;
+
spin_lock(&vc4->job_lock);
- vc4_irq_finish_render_job(dev);
+ render_job = vc4->render_job;
+ vc4->render_job = NULL;
spin_unlock(&vc4->job_lock);
+
+ fence = to_vc4_fence(render_job->base.irq_fence);
+
+ vc4_perfmon_stop(vc4, render_job->base.perfmon, true);
+
+ trace_vc4_rcl_end_irq(dev, fence->seqno);
+
+ dma_fence_signal(&fence->base);
+
status = IRQ_HANDLED;
}
@@ -295,7 +232,6 @@ int vc4_irq_install(struct drm_device *dev, int irq)
if (irq == IRQ_NOTCONNECTED)
return -ENOTCONN;
- init_waitqueue_head(&vc4->job_wait_queue);
INIT_WORK(&vc4->overflow_mem_work, vc4_overflow_mem_work);
/* Clear any pending interrupts someone might have left around
@@ -327,7 +263,6 @@ void vc4_irq_uninstall(struct drm_device *dev)
void vc4_irq_reset(struct drm_device *dev)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
- unsigned long irqflags;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return;
@@ -342,9 +277,4 @@ void vc4_irq_reset(struct drm_device *dev)
* memory yet.
*/
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS);
-
- spin_lock_irqsave(&vc4->job_lock, irqflags);
- vc4_cancel_bin_job(dev);
- vc4_irq_finish_render_job(dev);
- spin_unlock_irqrestore(&vc4->job_lock, irqflags);
}
diff --git a/drivers/gpu/drm/vc4/vc4_render_cl.c b/drivers/gpu/drm/vc4/vc4_render_cl.c
index c264d21bc3fe..b3e351b75c5e 100644
--- a/drivers/gpu/drm/vc4/vc4_render_cl.c
+++ b/drivers/gpu/drm/vc4/vc4_render_cl.c
@@ -254,6 +254,7 @@ static void emit_tile(struct vc4_exec_info *exec,
static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
struct vc4_rcl_setup *setup)
{
+ struct vc4_render_job *render = exec->render;
struct drm_vc4_submit_cl *args = exec->args;
bool has_bin = args->bin_cl_size != 0;
uint8_t min_x_tile = args->min_x_tile;
@@ -333,7 +334,7 @@ static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
if (IS_ERR(setup->rcl))
return PTR_ERR(setup->rcl);
list_add_tail(&to_vc4_bo(&setup->rcl->base)->unref_head,
- &exec->unref_list);
+ &render->unref_list);
/* The tile buffer gets cleared when the previous tile is stored. If
* the clear values changed between frames, then the tile buffer has
@@ -375,8 +376,8 @@ static int vc4_create_rcl_bo(struct drm_device *dev, struct vc4_exec_info *exec,
}
BUG_ON(setup->next_offset != size);
- exec->ct1ca = setup->rcl->dma_addr;
- exec->ct1ea = setup->rcl->dma_addr + setup->next_offset;
+ render->ct1ca = setup->rcl->dma_addr;
+ render->ct1ea = setup->rcl->dma_addr + setup->next_offset;
return 0;
}
@@ -411,6 +412,8 @@ static int vc4_rcl_msaa_surface_setup(struct vc4_exec_info *exec,
struct drm_gem_dma_object **obj,
struct drm_vc4_submit_rcl_surface *surf)
{
+ struct vc4_render_job *render = exec->render;
+
if (surf->flags != 0 || surf->bits != 0) {
DRM_DEBUG("MSAA surface had nonzero flags/bits\n");
return -EINVAL;
@@ -423,7 +426,7 @@ static int vc4_rcl_msaa_surface_setup(struct vc4_exec_info *exec,
if (!*obj)
return -EINVAL;
- exec->rcl_write_bo[exec->rcl_write_bo_count++] = *obj;
+ render->rcl_write_bo[render->rcl_write_bo_count++] = *obj;
if (surf->offset & 0xf) {
DRM_DEBUG("MSAA write must be 16b aligned.\n");
@@ -438,6 +441,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info *exec,
struct drm_vc4_submit_rcl_surface *surf,
bool is_write)
{
+ struct vc4_render_job *render = exec->render;
uint8_t tiling = VC4_GET_FIELD(surf->bits,
VC4_LOADSTORE_TILE_BUFFER_TILING);
uint8_t buffer = VC4_GET_FIELD(surf->bits,
@@ -460,7 +464,7 @@ static int vc4_rcl_surface_setup(struct vc4_exec_info *exec,
return -EINVAL;
if (is_write)
- exec->rcl_write_bo[exec->rcl_write_bo_count++] = *obj;
+ render->rcl_write_bo[render->rcl_write_bo_count++] = *obj;
if (surf->flags & VC4_SUBMIT_RCL_SURFACE_READ_IS_FULL_RES) {
if (surf == &exec->args->zs_write) {
@@ -537,6 +541,7 @@ vc4_rcl_render_config_surface_setup(struct vc4_exec_info *exec,
struct drm_gem_dma_object **obj,
struct drm_vc4_submit_rcl_surface *surf)
{
+ struct vc4_render_job *render = exec->render;
uint8_t tiling = VC4_GET_FIELD(surf->bits,
VC4_RENDER_CONFIG_MEMORY_FORMAT);
uint8_t format = VC4_GET_FIELD(surf->bits,
@@ -564,7 +569,7 @@ vc4_rcl_render_config_surface_setup(struct vc4_exec_info *exec,
if (!*obj)
return -EINVAL;
- exec->rcl_write_bo[exec->rcl_write_bo_count++] = *obj;
+ render->rcl_write_bo[render->rcl_write_bo_count++] = *obj;
if (tiling > VC4_TILING_FORMAT_LT) {
DRM_DEBUG("Bad tiling format\n");
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index d31b906cb8e7..4a3de002ad84 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -174,11 +174,10 @@ static void vc4_v3d_init_hw(struct drm_device *dev)
int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
{
- struct drm_device *dev = &vc4->base;
+ struct dma_fence *fence;
unsigned long irqflags;
int slot;
- uint64_t seqno = 0;
- struct vc4_exec_info *exec;
+ long ret;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return -ENODEV;
@@ -194,21 +193,20 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
return slot;
}
- /* Couldn't find an open slot. Wait for render to complete
+ /* Couldn't find an open slot. Wait for render to complete
* and try again.
*/
- exec = vc4_last_render_job(vc4);
- if (exec)
- seqno = exec->seqno;
+ fence = NULL;
+ if (vc4->render_job)
+ fence = dma_fence_get(vc4->render_job->base.done_fence);
spin_unlock_irqrestore(&vc4->job_lock, irqflags);
- if (seqno) {
- int ret = vc4_wait_for_seqno(dev, seqno, ~0ull, true);
-
- if (ret == 0)
+ if (fence) {
+ ret = dma_fence_wait_timeout(fence, true, MAX_SCHEDULE_TIMEOUT);
+ dma_fence_put(fence);
+ if (ret > 0)
goto try_again;
-
- return ret;
+ return ret < 0 ? ret : -ETIMEDOUT;
}
return -ENOMEM;
diff --git a/drivers/gpu/drm/vc4/vc4_validate.c b/drivers/gpu/drm/vc4/vc4_validate.c
index 7f2fadfde7a8..f2a2391b8601 100644
--- a/drivers/gpu/drm/vc4/vc4_validate.c
+++ b/drivers/gpu/drm/vc4/vc4_validate.c
@@ -107,18 +107,19 @@ struct drm_gem_dma_object *
vc4_use_bo(struct vc4_exec_info *exec, uint32_t hindex)
{
struct vc4_dev *vc4 = exec->dev;
+ struct vc4_render_job *render = exec->render;
struct drm_gem_dma_object *obj;
struct vc4_bo *bo;
if (WARN_ON_ONCE(vc4->gen > VC4_GEN_4))
return NULL;
- if (hindex >= exec->bo_count) {
+ if (hindex >= render->bo_count) {
DRM_DEBUG("BO index %d greater than BO count %d\n",
- hindex, exec->bo_count);
+ hindex, render->bo_count);
return NULL;
}
- obj = to_drm_gem_dma_obj(exec->bo[hindex]);
+ obj = to_drm_gem_dma_obj(render->bo[hindex]);
bo = to_vc4_bo(&obj->base);
if (bo->validated_shader) {
@@ -354,8 +355,8 @@ validate_gl_shader_state(VALIDATE_ARGS)
static int
validate_tile_binning_config(VALIDATE_ARGS)
{
- struct drm_device *dev = exec->exec_bo->base.dev;
- struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_dev *vc4 = exec->dev;
+ struct drm_device *dev = &vc4->base;
uint8_t flags;
uint32_t tile_state_size;
uint32_t tile_count, bin_addr;
@@ -397,7 +398,7 @@ validate_tile_binning_config(VALIDATE_ARGS)
/* The slot we allocated will only be used by this job, and is
* free when the job completes rendering.
*/
- exec->bin_slots |= BIT(bin_slot);
+ exec->render->bin_slots |= BIT(bin_slot);
bin_addr = vc4->bin_bo->base.dma_addr + bin_slot * vc4->bin_alloc_size;
/* The tile state data array is 48 bytes per tile, and we put it at
@@ -488,6 +489,7 @@ vc4_validate_bin_cl(struct drm_device *dev,
void *unvalidated,
struct vc4_exec_info *exec)
{
+ struct vc4_bin_job *bin = exec->bin;
struct vc4_dev *vc4 = to_vc4_dev(dev);
uint32_t len = exec->args->bin_cl_size;
uint32_t dst_offset = 0;
@@ -544,7 +546,7 @@ vc4_validate_bin_cl(struct drm_device *dev,
break;
}
- exec->ct0ea = exec->ct0ca + dst_offset;
+ bin->ct0ea = bin->ct0ca + dst_offset;
if (!exec->found_start_tile_binning_packet) {
DRM_DEBUG("Bin CL missing VC4_PACKET_START_TILE_BINNING\n");
@@ -760,6 +762,7 @@ validate_gl_shader_rec(struct drm_device *dev,
uint32_t shader_reloc_count = ARRAY_SIZE(shader_reloc_offsets);
struct drm_gem_dma_object *bo[ARRAY_SIZE(shader_reloc_offsets) + 8];
uint32_t nr_attributes, nr_relocs, packet_size;
+ struct vc4_render_job *render = exec->render;
int i;
nr_attributes = state->addr & 0x7;
@@ -798,12 +801,12 @@ validate_gl_shader_rec(struct drm_device *dev,
exec->shader_rec_size -= packet_size;
for (i = 0; i < shader_reloc_count; i++) {
- if (src_handles[i] > exec->bo_count) {
+ if (src_handles[i] > render->bo_count) {
DRM_DEBUG("Shader handle %d too big\n", src_handles[i]);
return -EINVAL;
}
- bo[i] = to_drm_gem_dma_obj(exec->bo[src_handles[i]]);
+ bo[i] = to_drm_gem_dma_obj(render->bo[src_handles[i]]);
if (!bo[i])
return -EINVAL;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler
2026-07-12 15:14 ` [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
@ 2026-07-12 15:29 ` sashiko-bot
0 siblings, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-07-12 15:29 UTC (permalink / raw)
To: Maíra Canal; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] Missing NULL check for `bin_job` and `render_job` in IRQ handler causes kernel panic on spurious interrupts.
- [High] `vc4_v3d_get_bin_slot` prematurely returns `-ENOMEM` when bin slots are exhausted but the hardware is momentarily idle.
- [High] Unbalanced `vc4_bo_dec_usecnt()` in error path during BO lookup causes usecnt underflow.
- [Medium] `vc4_save_hang_state()` fails to capture BOs for the active bin job, capturing only the render job's BOs.
- [Medium] Unconditional `vc4_perfmon_stop` in IRQ handlers breaks performance monitoring for pipelined or concurrent jobs.
Pre-existing issues:
- [High] An off-by-one error in shader record validation allows userspace to trigger an out-of-bounds read past the end of the `render->bo` array.
--
--- Patch [7]: [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler ---
commit 873ba817e912575f3a2ceffbc30604b80b1e73ac
Author: Maíra Canal <mcanal@igalia.com>
drm/vc4: Switch to DRM GPU scheduler
Wire up the DRM GPU scheduler to the VC4 submission path, replacing the
legacy hand-rolled job queue implementation. The scheduler now handles
job ordering, dependency tracking, and timeout detection.
> diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
> --- a/drivers/gpu/drm/vc4/vc4_irq.c
> +++ b/drivers/gpu/drm/vc4/vc4_irq.c
> @@ -225,16 +136,42 @@ vc4_irq(int irq, void *arg)
> if (intctl & V3D_INT_FLDONE) {
> + struct vc4_bin_job *bin_job;
> + struct vc4_fence *fence;
> +
> spin_lock(&vc4->job_lock);
> - vc4_irq_finish_bin_job(dev);
> + bin_job = vc4->bin_job;
> + vc4->bin_job = NULL;
> spin_unlock(&vc4->job_lock);
> +
> + fence = to_vc4_fence(bin_job->base.irq_fence);
[Severity: High]
Could this cause a NULL pointer dereference on spurious interrupts?
If the hardware generates a spurious V3D_INT_FLDONE interrupt, or if the
interrupt fires after the job state has been reset (e.g., due to a timeout
or GPU reset), vc4->bin_job will be NULL. Unconditionally dereferencing
bin_job->base.irq_fence here would lead to an immediate kernel panic in the
interrupt handler.
> +
> + vc4_perfmon_stop(vc4, bin_job->base.perfmon, true);
[Severity: Medium]
Is it safe to unconditionally stop the perfmon here?
Under the legacy implementation, the driver left the perfmon running across
the gap between a job's bin and render phases, only stopping it if the next
job didn't use the same perfmon. By calling vc4_perfmon_stop() at the end
of every bin job, the hardware counters might be prematurely disabled while
the corresponding render job is still actively executing, which would corrupt
the gathered statistics.
> +
> + trace_vc4_bcl_end_irq(dev, fence->seqno);
> +
> + dma_fence_signal(&fence->base);
> +
> status = IRQ_HANDLED;
> }
>
> if (intctl & V3D_INT_FRDONE) {
> + struct vc4_render_job *render_job;
> + struct vc4_fence *fence;
> +
> spin_lock(&vc4->job_lock);
> - vc4_irq_finish_render_job(dev);
> + render_job = vc4->render_job;
> + vc4->render_job = NULL;
> spin_unlock(&vc4->job_lock);
> +
> + fence = to_vc4_fence(render_job->base.irq_fence);
[Severity: High]
Similar to the FLDONE case above, is it possible for render_job to be NULL
here if a spurious V3D_INT_FRDONE interrupt occurs, leading to a panic?
> diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
> --- a/drivers/gpu/drm/vc4/vc4_v3d.c
> +++ b/drivers/gpu/drm/vc4/vc4_v3d.c
> @@ -194,21 +193,20 @@ int vc4_v3d_get_bin_slot(struct vc4_dev *vc4)
> return slot;
> }
>
> - /* Couldn't find an open slot. Wait for render to complete
> + /* Couldn't find an open slot. Wait for render to complete
> * and try again.
> */
> - exec = vc4_last_render_job(vc4);
> - if (exec)
> - seqno = exec->seqno;
> + fence = NULL;
> + if (vc4->render_job)
> + fence = dma_fence_get(vc4->render_job->base.done_fence);
> spin_unlock_irqrestore(&vc4->job_lock, irqflags);
[Severity: High]
Will this prematurely return -ENOMEM when the hardware is momentarily idle?
Bin slots are allocated during vc4_submit_cl_ioctl before jobs are queued
in the DRM scheduler. If the hardware is momentarily idle while jobs are
waiting in the DRM scheduler queue holding bin slots, vc4->render_job
will be NULL. This bypasses the wait and immediately returns -ENOMEM,
which could cause spurious ioctl failures under load.
> diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> --- a/drivers/gpu/drm/vc4/vc4_gem.c
> +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> @@ -168,21 +150,16 @@ vc4_save_hang_state(struct drm_device *dev)
> spin_lock_irqsave(&vc4->job_lock, irqflags);
> - exec[0] = vc4_first_bin_job(vc4);
> - exec[1] = vc4_first_render_job(vc4);
> - if (!exec[0] && !exec[1])
> + bin_job = vc4->bin_job;
> + render_job = vc4->render_job;
> + if (!bin_job && !render_job)
> goto err_free_state;
>
> - /* Get the bos from both binner and renderer into hang state. */
> + /* Get the BOs from the render job into hang state. */
> state->bo_count = 0;
> - for (i = 0; i < 2; i++) {
> - if (!exec[i])
> - continue;
> -
> - unref_list_count = 0;
> - list_for_each_entry(bo, &exec[i]->unref_list, unref_head)
> - unref_list_count++;
> - state->bo_count += exec[i]->bo_count + unref_list_count;
> + if (render_job) {
> + unref_list_count = list_count_nodes(&render_job->unref_list);
> + state->bo_count += render_job->bo_count + unref_list_count;
> }
[Severity: Medium]
Does this unintentionally skip capturing BOs for the active bin job?
Because the new job structures moved BO tracking exclusively into
vc4_render_job, this only iterates over the bo_count and unref_list of
vc4->render_job. If a hang happens during a binning operation while the
render queue is idle, it seems no BOs will be captured at all.
> diff --git a/drivers/gpu/drm/vc4/vc4_validate.c b/drivers/gpu/drm/vc4/vc4_validate.c
> --- a/drivers/gpu/drm/vc4/vc4_validate.c
> +++ b/drivers/gpu/drm/vc4/vc4_validate.c
> @@ -798,12 +801,12 @@ validate_gl_shader_rec(struct drm_device *dev,
> exec->shader_rec_size -= packet_size;
>
> for (i = 0; i < shader_reloc_count; i++) {
> - if (src_handles[i] > exec->bo_count) {
> + if (src_handles[i] > render->bo_count) {
> DRM_DEBUG("Shader handle %d too big\n", src_handles[i]);
> return -EINVAL;
> }
>
> - bo[i] = to_drm_gem_dma_obj(exec->bo[src_handles[i]]);
> + bo[i] = to_drm_gem_dma_obj(render->bo[src_handles[i]]);
[Severity: High]
This is a pre-existing issue, but does this bounds check contain an
off-by-one error?
Since the render->bo array has bo_count elements, valid indices are from
0 to bo_count - 1. If userspace provides an index exactly equal to
bo_count, the check incorrectly succeeds. The kernel then dereferences
render->bo[bo_count], resulting in an out-of-bounds read past the end
of the array.
> diff --git a/drivers/gpu/drm/vc4/vc4_submit.c b/drivers/gpu/drm/vc4/vc4_submit.c
> --- /dev/null
> +++ b/drivers/gpu/drm/vc4/vc4_submit.c
[ ... ]
> +fail_dec_usecnt:
> + /* Decrease usecnt on acquired objects */
> + for (i--; i >= 0; i--)
> + vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i]));
[Severity: High]
Are the reference counts getting underflowed during the error path?
If an increment fails, the error path loops backwards decrementing the
usecnt. However, because the BO array remains fully populated in the job
struct, the subsequent failure cleanup in vc4_render_job_free() iterates
over the entire array again:
vc4_submit.c:vc4_render_job_free() {
if (job->bo) {
for (int i = 0; i < job->bo_count; i++) {
struct vc4_bo *bo = to_vc4_bo(job->bo[i]);
vc4_bo_dec_usecnt(bo);
...
}
}
}
This would double-decrement the BOs that were unwound here.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-vc4-drm-scheduler-v3-0-ed6dc4defa53@igalia.com?part=7
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 8/8] drm/vc4: Use unique fence timeline names per queue
2026-07-12 15:14 [PATCH v3 0/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
` (6 preceding siblings ...)
2026-07-12 15:14 ` [PATCH v3 7/8] drm/vc4: Switch to DRM GPU scheduler Maíra Canal
@ 2026-07-12 15:14 ` Maíra Canal
7 siblings, 0 replies; 13+ messages in thread
From: Maíra Canal @ 2026-07-12 15:14 UTC (permalink / raw)
To: Tvrtko Ursulin, Melissa Wen, Maxime Ripard, Dave Stevenson,
Raspberry Pi Kernel Maintenance, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: kernel-dev, dri-devel, Maíra Canal
Return distinct timeline names ("vc4-bin" and "vc4-render") from
vc4_fence_get_timeline_name() based on the fence's queue, instead of
the generic "vc4-v3d" for all fences. This improves debuggability when
inspecting fence state.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
drivers/gpu/drm/vc4/vc4_fence.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_fence.c b/drivers/gpu/drm/vc4/vc4_fence.c
index 12f164983238..3b30107bbc4a 100644
--- a/drivers/gpu/drm/vc4/vc4_fence.c
+++ b/drivers/gpu/drm/vc4/vc4_fence.c
@@ -48,7 +48,16 @@ static const char *vc4_fence_get_driver_name(struct dma_fence *fence)
static const char *vc4_fence_get_timeline_name(struct dma_fence *fence)
{
- return "vc4-v3d";
+ struct vc4_fence *f = to_vc4_fence(fence);
+
+ switch (f->queue) {
+ case VC4_BIN:
+ return "vc4-bin";
+ case VC4_RENDER:
+ return "vc4-render";
+ default:
+ return "vc4-unknown";
+ }
}
const struct dma_fence_ops vc4_fence_ops = {
--
2.54.0
^ permalink raw reply related [flat|nested] 13+ messages in thread