Re: [PATCH v2] drm/imagination: Propagate all errors from KCCB command submission code

[Alessio Belle]

Hi Alexandru,

On Fri, 2026-08-14 at 10:49 +0300, Alexandru Dadu wrote:
> From: Alessio Belle <alessio.belle@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> ---
> Signed-off-by: Alexandru Dadu <alexandru.dadu@xxxxxxxxxx>

Can you add your Signed-off-by line at the end of the patch itself as well? (see
"Developer’s Certificate of Origin 1.1" in [1])

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> ---
> 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@xxxxxxxxxx
> 
> To: Alessio Belle <alessio.belle@xxxxxxxxxx>
> To: Luigi Santivetti <luigi.santivetti@xxxxxxxxxx>
> To: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> To: Maxime Ripard <mripard@xxxxxxxxxx>
> To: Thomas Zimmermann <tzimmermann@xxxxxxx>
> To: David Airlie <airlied@xxxxxxxxx>
> To: Simona Vetter <simona@xxxxxxxx>
> Cc: imagination@xxxxxxxxxxxxxxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> ---
>  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 | 60 +++++++++++++++++++++++----------
>  5 files changed, 89 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index 4accf18e2341..8182babd8ad8 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 sent.
>   * @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 54e88b4208d7..70ad2bcc93b7 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
> @@ -750,6 +750,15 @@ static struct dma_fence *pvr_queue_run_job(struct drm_sched_job *sched_job)
>  	struct pvr_job *job = container_of(sched_job, struct pvr_job, base);
>  	struct pvr_device *pvr_dev = job->pvr_dev;
>  	int err;
> +	bool reserved;
> +
> +	/* Track the KCCB slot reserved status: when reserve_slot succeeds, clears job->kccb_fence.
> +	 * NULL means reservation was taken.
> +	 */
> +	if (job->kccb_fence == NULL)

This is one of the fences checked by pvr_queue_prepare_job(), that have to be
satisfied (be NULL) before this code can run, meaning this is always true.

> +		reserved = true;
> +	else
> +		reserved = false;
>  
>  	/* The fragment job is issued along the geometry job when we use combined
>  	 * geom+frag kicks. When we get there, we should simply return the
> @@ -768,17 +777,21 @@ 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);
> +		if (WARN_ON(err)) {
> +			pvr_job_release_pm_ref(job);

I _think_ it is fine to drop the power reference(s) here, though I would try
exercising/forcing these paths to double check, if you haven't done so already.

Anyway, the early release of the power reference(s) is unrelated to the original
commit, so it should go into its own commit (last) if kept.

> +			goto err_release;
> +		}
>  	}
>  
>  	/* Submit our job to the CCCB */
> @@ -792,25 +805,38 @@ 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);

drm-misc-next (where I think the main patch should land) now contains the whole
of 7.2 but in particular c256bd486855 ("drm/imagination: Update the trace point
pvr_job_submit_fw()") that also updated code around here. Could you rebase the
patch/series?

>  	} 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);
> +	}
> +
> +	if (WARN_ON(err)) {
> +		pvr_job_release_pm_ref(job);
> +		if (job->paired_job)
> +			pvr_job_release_pm_ref(job->paired_job);
> +		goto err_release;
>  	}
>  
>  	return dma_fence_get(job->done_fence);
> +
> +err_release:
> +	if (reserved)
> +		pvr_kccb_release_slot(pvr_dev);

Sashiko correctly reported that pvr_kccb_send_cmd_powered() also has a similar
problem, missed in the original patch. Two approaches I think:

- Both pvr_queue_run_job() and pvr_kccb_send_cmd_powered() release the slot in
their error paths, to balance the reservation done by pvr_queue_prepare_job()
and pvr_kccb_send_cmd_powered() itself respectively.
- pvr_kccb_send_cmd_reserved_powered() is the one ensuring the slot is released
for any KCCB submission attempt. pvr_queue_run_job() still needs to release the
slot for errors not from pvr_kccb_send_cmd_reserved_powered().

First approach sound cleaner to me so I'm leaning towards that.
In any case, any error handling in pvr_queue_run_job() that is not related to
pvr_kccb_send_cmd_reserved_powered() failing should be part of a separate fix,
likely as first commit.

Thanks,
Alessio

> +	return ERR_PTR(err);
>  }
>  
>  static void pvr_queue_stop(struct pvr_queue *queue, struct pvr_job *bad_job)
> 
> ---
> base-commit: e55fead22ff9ee047ab9f1903860c4b43043514e
> change-id: 20260810-b4-upstream-propagate-all-errors-from-kccb-cmd-submission-code-0dae4f05fcb5
> 
> Best regards,
> --  
> Alexandru Dadu <alexandru.dadu@xxxxxxxxxx>
>