* [PATCH v3] drm/imagination: Propagate all errors from KCCB command submission code
@ 2026-09-07 9:16 Alexandru Dadu
2026-09-07 9:29 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: Alexandru Dadu @ 2026-09-07 9:16 UTC (permalink / raw)
To: Alessio Belle, Luigi Santivetti, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter
Cc: imagination, dri-devel, linux-kernel, Alexandru Dadu
From: Alessio Belle <alessio.belle@imgtec.com>
pvr_kccb_send_cmd_reserved_powered() returned void while the other two
variants of pvr_kccb_send_cmd*() returned int.
The error is now propagated all the way to the DRM scheduler's run_job()
callback, which is the only user of pvr_kccb_send_cmd_reserved_powered()
outside of the other variants of pvr_kccb_send_cmd*().
Signed-off-by: Alessio Belle <alessio.belle@imgtec.com>
Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
Changes in v3:
- Dropped the job->kccb_fence check since pvr_queue_prepare_job() makes
sure that the check would always be true.
- Dropped the power references from the patch.
- Link to v2: https://patch.msgid.link/20260814-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v2-1-35355fad50ae@imgtec.com
Changes in v2:
- Provide an error path in pvr_queue_run_job(). The path will use
pvr_kccb_release_slot() to avoid resource leaks.
- Link to v1: https://patch.msgid.link/20260811-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v1-1-ffd55254d6d2@imgtec.com
To: Alessio Belle <alessio.belle@imgtec.com>
To: Luigi Santivetti <luigi.santivetti@imgtec.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
Cc: imagination@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
drivers/gpu/drm/imagination/pvr_ccb.c | 33 +++++++++++++++++++++-------
drivers/gpu/drm/imagination/pvr_ccb.h | 6 ++---
drivers/gpu/drm/imagination/pvr_cccb.c | 12 ++++++----
drivers/gpu/drm/imagination/pvr_cccb.h | 20 ++++++++---------
drivers/gpu/drm/imagination/pvr_queue.c | 39 +++++++++++++++++++--------------
5 files changed, 69 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
index b702d122d791..dfd068270542 100644
--- a/drivers/gpu/drm/imagination/pvr_ccb.c
+++ b/drivers/gpu/drm/imagination/pvr_ccb.c
@@ -257,8 +257,13 @@ pvr_kccb_used_slot_count_locked(struct pvr_device *pvr_dev)
* @pvr_dev: Device pointer.
* @cmd: Command to send.
* @kccb_slot: Address to store the KCCB slot for this command. May be %NULL.
+ *
+ * Returns:
+ * * Zero on success,
+ * * -EIO if the device is lost, or
+ * * -EINVAL if a KCCB slot was not reserved or is not available.
*/
-void
+int
pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
struct rogue_fwif_kccb_cmd *cmd,
u32 *kccb_slot)
@@ -268,19 +273,25 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
struct rogue_fwif_ccb_ctl *ctrl = pvr_ccb->ctrl;
u32 old_write_offset;
u32 new_write_offset;
+ int err;
- WARN_ON(pvr_dev->lost);
+ if (pvr_dev->lost)
+ return -EIO;
mutex_lock(&pvr_ccb->lock);
- if (WARN_ON(!pvr_dev->kccb.reserved_count))
+ if (WARN_ON(!pvr_dev->kccb.reserved_count)) {
+ err = -EINVAL;
goto out_unlock;
+ }
old_write_offset = READ_ONCE(ctrl->write_offset);
/* We reserved the slot, we should have one available. */
- if (WARN_ON(!pvr_ccb_slot_available_locked(pvr_ccb, &new_write_offset)))
+ if (WARN_ON(!pvr_ccb_slot_available_locked(pvr_ccb, &new_write_offset))) {
+ err = -EINVAL;
goto out_unlock;
+ }
memcpy(&kccb[old_write_offset], cmd,
sizeof(struct rogue_fwif_kccb_cmd));
@@ -298,8 +309,14 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
pvr_fw_mts_schedule(pvr_dev,
PVR_FWIF_DM_GP & ~ROGUE_CR_MTS_SCHEDULE_DM_CLRMSK);
+ mutex_unlock(&pvr_ccb->lock);
+
+ return 0;
+
out_unlock:
mutex_unlock(&pvr_ccb->lock);
+
+ return err;
}
/**
@@ -365,8 +382,9 @@ static int pvr_kccb_reserve_slot_sync(struct pvr_device *pvr_dev)
* @kccb_slot: Address to store the KCCB slot for this command. May be %NULL.
*
* Returns:
- * * Zero on success, or
- * * -EBUSY if timeout while waiting for a free KCCB slot.
+ * * Zero on success,
+ * * Any error returned by pvr_kccb_reserve_slot_sync(), or
+ * * Any error returned by pvr_kccb_send_cmd_reserved_powered().
*/
int
pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev, struct rogue_fwif_kccb_cmd *cmd,
@@ -378,8 +396,7 @@ pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev, struct rogue_fwif_kccb_cmd
if (err)
return err;
- pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
- return 0;
+ return pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
}
/**
diff --git a/drivers/gpu/drm/imagination/pvr_ccb.h b/drivers/gpu/drm/imagination/pvr_ccb.h
index 4c8aef31eeb0..8b698206c68b 100644
--- a/drivers/gpu/drm/imagination/pvr_ccb.h
+++ b/drivers/gpu/drm/imagination/pvr_ccb.h
@@ -60,9 +60,9 @@ int pvr_kccb_send_cmd(struct pvr_device *pvr_dev,
int pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev,
struct rogue_fwif_kccb_cmd *cmd,
u32 *kccb_slot);
-void pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
- struct rogue_fwif_kccb_cmd *cmd,
- u32 *kccb_slot);
+int pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
+ struct rogue_fwif_kccb_cmd *cmd,
+ u32 *kccb_slot);
int pvr_kccb_wait_for_completion(struct pvr_device *pvr_dev, u32 slot_nr, u32 timeout,
u32 *rtn_out);
bool pvr_kccb_is_idle(struct pvr_device *pvr_dev);
diff --git a/drivers/gpu/drm/imagination/pvr_cccb.c b/drivers/gpu/drm/imagination/pvr_cccb.c
index 4fabab41bea7..da6e6d94e29f 100644
--- a/drivers/gpu/drm/imagination/pvr_cccb.c
+++ b/drivers/gpu/drm/imagination/pvr_cccb.c
@@ -220,8 +220,12 @@ static void fill_cmd_kick_data(struct pvr_cccb *cccb, u32 ctx_fw_addr,
* You must call pvr_kccb_reserve_slot() and wait for the returned fence to
* signal (if this function didn't return NULL) before calling
* pvr_cccb_send_kccb_kick().
+ *
+ * Returns:
+ * * Zero on success, or
+ * * Any error returned by pvr_kccb_send_cmd_reserved_powered().
*/
-void
+int
pvr_cccb_send_kccb_kick(struct pvr_device *pvr_dev,
struct pvr_cccb *pvr_cccb, u32 cctx_fw_addr,
struct pvr_hwrt_data *hwrt)
@@ -235,10 +239,10 @@ pvr_cccb_send_kccb_kick(struct pvr_device *pvr_dev,
/* Make sure the writes to the CCCB are flushed before sending the KICK. */
wmb();
- pvr_kccb_send_cmd_reserved_powered(pvr_dev, &cmd_kick, NULL);
+ return pvr_kccb_send_cmd_reserved_powered(pvr_dev, &cmd_kick, NULL);
}
-void
+int
pvr_cccb_send_kccb_combined_kick(struct pvr_device *pvr_dev,
struct pvr_cccb *geom_cccb,
struct pvr_cccb *frag_cccb,
@@ -263,5 +267,5 @@ pvr_cccb_send_kccb_combined_kick(struct pvr_device *pvr_dev,
/* Make sure the writes to the CCCB are flushed before sending the KICK. */
wmb();
- pvr_kccb_send_cmd_reserved_powered(pvr_dev, &cmd_kick, NULL);
+ return pvr_kccb_send_cmd_reserved_powered(pvr_dev, &cmd_kick, NULL);
}
diff --git a/drivers/gpu/drm/imagination/pvr_cccb.h b/drivers/gpu/drm/imagination/pvr_cccb.h
index 943fe8f2c963..a2155f732bf1 100644
--- a/drivers/gpu/drm/imagination/pvr_cccb.h
+++ b/drivers/gpu/drm/imagination/pvr_cccb.h
@@ -59,16 +59,16 @@ void pvr_cccb_fini(struct pvr_cccb *cccb);
void pvr_cccb_write_command_with_header(struct pvr_cccb *pvr_cccb,
u32 cmd_type, u32 cmd_size, void *cmd_data,
u32 ext_job_ref, u32 int_job_ref);
-void pvr_cccb_send_kccb_kick(struct pvr_device *pvr_dev,
- struct pvr_cccb *pvr_cccb, u32 cctx_fw_addr,
- struct pvr_hwrt_data *hwrt);
-void pvr_cccb_send_kccb_combined_kick(struct pvr_device *pvr_dev,
- struct pvr_cccb *geom_cccb,
- struct pvr_cccb *frag_cccb,
- u32 geom_ctx_fw_addr,
- u32 frag_ctx_fw_addr,
- struct pvr_hwrt_data *hwrt,
- bool frag_is_pr);
+int pvr_cccb_send_kccb_kick(struct pvr_device *pvr_dev,
+ struct pvr_cccb *pvr_cccb, u32 cctx_fw_addr,
+ struct pvr_hwrt_data *hwrt);
+int pvr_cccb_send_kccb_combined_kick(struct pvr_device *pvr_dev,
+ struct pvr_cccb *geom_cccb,
+ struct pvr_cccb *frag_cccb,
+ u32 geom_ctx_fw_addr,
+ u32 frag_ctx_fw_addr,
+ struct pvr_hwrt_data *hwrt,
+ bool frag_is_pr);
bool pvr_cccb_cmdseq_fits(struct pvr_cccb *pvr_cccb, size_t size);
/**
diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
index 09993e858df8..33ae624177e3 100644
--- a/drivers/gpu/drm/imagination/pvr_queue.c
+++ b/drivers/gpu/drm/imagination/pvr_queue.c
@@ -769,17 +769,19 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
(job->type != DRM_PVR_JOB_TYPE_GEOMETRY ||
job->paired_job->type != DRM_PVR_JOB_TYPE_FRAGMENT ||
job->hwrt != job->paired_job->hwrt ||
- job->ctx != job->paired_job->ctx)))
- return ERR_PTR(-EINVAL);
+ job->ctx != job->paired_job->ctx))) {
+ err = -EINVAL;
+ goto err_release;
+ }
err = pvr_job_get_pm_ref(job);
if (WARN_ON(err))
- return ERR_PTR(err);
+ goto err_release;
if (job->paired_job) {
err = pvr_job_get_pm_ref(job->paired_job);
if (WARN_ON(err))
- return ERR_PTR(err);
+ goto err_release;
}
/* Submit our job to the CCCB */
@@ -793,25 +795,30 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
/* Submit the fragment job along the geometry job and send a combined kick. */
pvr_queue_submit_job_to_cccb(frag_job);
- pvr_cccb_send_kccb_combined_kick(pvr_dev,
- &geom_queue->cccb, &frag_queue->cccb,
- pvr_context_get_fw_addr(geom_job->ctx) +
- geom_queue->ctx_offset,
- pvr_context_get_fw_addr(frag_job->ctx) +
- frag_queue->ctx_offset,
- job->hwrt,
- frag_job->fw_ccb_cmd_type ==
- ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
+ err = pvr_cccb_send_kccb_combined_kick(pvr_dev,
+ &geom_queue->cccb, &frag_queue->cccb,
+ pvr_context_get_fw_addr(geom_job->ctx) +
+ geom_queue->ctx_offset,
+ pvr_context_get_fw_addr(frag_job->ctx) +
+ frag_queue->ctx_offset,
+ job->hwrt,
+ frag_job->fw_ccb_cmd_type ==
+ ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
} else {
struct pvr_queue *queue = container_of(job->base.sched,
struct pvr_queue, scheduler);
- pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
- pvr_context_get_fw_addr(job->ctx) + queue->ctx_offset,
- job->hwrt);
+ err = pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
+ pvr_context_get_fw_addr(job->ctx) +
+ queue->ctx_offset,
+ job->hwrt);
}
return dma_fence_get(job->done_fence);
+
+err_release:
+ pvr_kccb_release_slot(pvr_dev);
+ return ERR_PTR(err);
}
static void pvr_queue_stop(struct pvr_queue *queue, struct pvr_job *bad_job)
---
base-commit: bd4f284df04d76fd65e57141cb1e6e7a49e4c3cb
change-id: 20260810-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-0dae4f05fcb5
Best regards,
--
Alexandru Dadu <alexandru.dadu@imgtec.com>
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v3] drm/imagination: Propagate all errors from KCCB command submission code
2026-09-07 9:16 [PATCH v3] drm/imagination: Propagate all errors from KCCB command submission code Alexandru Dadu
@ 2026-09-07 9:29 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-09-07 9:29 UTC (permalink / raw)
To: Alexandru Dadu; +Cc: dri-devel
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unchecked error assignment in `pvr_queue_run_job()` completely ignores the propagated KCCB error, causing the function to falsely report success to the DRM scheduler and bypass cleanup.
- [High] KCCB slot reservation is leaked in `pvr_kccb_send_cmd_powered()` when `pvr_kccb_send_cmd_reserved_powered()` fails.
--
commit 37f41df35ee17941516339510b33ee61bff243f1
Author: Alessio Belle <alessio.belle@imgtec.com>
drm/imagination: Propagate all errors from KCCB command submission code
This commit updates pvr_kccb_send_cmd_reserved_powered to return an
integer error code instead of void. It propagates these errors up to the
DRM scheduler's run_job callback, adding error paths to
pvr_queue_run_job.
> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
[ ... ]
> @@ -268,19 +273,25 @@ pvr_kccb_send_cmd_reserved_powered(struct pvr_device *pvr_dev,
> struct rogue_fwif_ccb_ctl *ctrl = pvr_ccb->ctrl;
> u32 old_write_offset;
> u32 new_write_offset;
> + int err;
>
> - WARN_ON(pvr_dev->lost);
> + if (pvr_dev->lost)
> + return -EIO;
[ ... ]
> @@ -378,8 +396,7 @@ pvr_kccb_send_cmd_powered(struct pvr_device *pvr_dev, struct rogue_fwif_kccb_cmd
> if (err)
> return err;
>
> - pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
> - return 0;
> + return pvr_kccb_send_cmd_reserved_powered(pvr_dev, cmd, kccb_slot);
> }
[Severity: High]
Does this code leak the KCCB slot reservation when a command fails?
In pvr_kccb_send_cmd_powered(), a slot is successfully reserved via
pvr_kccb_reserve_slot_sync(). However, the patch adds an early return path in
pvr_kccb_send_cmd_reserved_powered() that returns -EIO if pvr_dev->lost is true.
When this occurs, pvr_kccb_send_cmd_powered() directly returns this error but
does not call pvr_kccb_release_slot() to unwind the successful reservation,
permanently leaking the KCCB slot.
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c b/drivers/gpu/drm/imagination/pvr_queue.c
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
[ ... ]
> @@ -793,25 +795,30 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
>
> /* Submit the fragment job along the geometry job and send a combined kick. */
> pvr_queue_submit_job_to_cccb(frag_job);
> - pvr_cccb_send_kccb_combined_kick(pvr_dev,
> - &geom_queue->cccb, &frag_queue->cccb,
> - pvr_context_get_fw_addr(geom_job->ctx) +
> - geom_queue->ctx_offset,
> - pvr_context_get_fw_addr(frag_job->ctx) +
> - frag_queue->ctx_offset,
> - job->hwrt,
> - frag_job->fw_ccb_cmd_type ==
> - ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
> + err = pvr_cccb_send_kccb_combined_kick(pvr_dev,
> + &geom_queue->cccb, &frag_queue->cccb,
> + pvr_context_get_fw_addr(geom_job->ctx) +
> + geom_queue->ctx_offset,
> + pvr_context_get_fw_addr(frag_job->ctx) +
> + frag_queue->ctx_offset,
> + job->hwrt,
> + frag_job->fw_ccb_cmd_type ==
> + ROGUE_FWIF_CCB_CMD_TYPE_FRAG_PR);
> } else {
> struct pvr_queue *queue = container_of(job->base.sched,
> struct pvr_queue, scheduler);
>
> - pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> - pvr_context_get_fw_addr(job->ctx) + queue->ctx_offset,
> - job->hwrt);
> + err = pvr_cccb_send_kccb_kick(pvr_dev, &queue->cccb,
> + pvr_context_get_fw_addr(job->ctx) +
> + queue->ctx_offset,
> + job->hwrt);
> }
>
> return dma_fence_get(job->done_fence);
> +
> +err_release:
> + pvr_kccb_release_slot(pvr_dev);
> + return ERR_PTR(err);
> }
[Severity: High]
Does this code falsely report success to the DRM scheduler when a kick fails?
In pvr_queue_run_job(), the return value from the KCCB kick functions is
assigned to the err variable but is never evaluated. The function proceeds
unconditionally to return success via dma_fence_get(job->done_fence) instead
of checking if err is non-zero and branching to the err_release label.
If a KCCB kick fails, this bypasses the intended cleanup path in err_release
and notifies the DRM scheduler of success for a job that was not sent to the
hardware.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-v3-1-4578a0fadf69@imgtec.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-09-07 9:29 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 9:16 [PATCH v3] drm/imagination: Propagate all errors from KCCB command submission code Alexandru Dadu
2026-09-07 9:29 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.