Re: [PATCH 1/2] drm/imagination: Collect KCCB initialisation

[Alessio Belle]

On Thu, 2026-09-10 at 10:56 +0300, Alexandru Dadu wrote:
> From: Matt Coster <matt.coster@xxxxxxxxxx>
> 
> There's one FW object attached to KCCB usage (the return buffer) that is
> separately initialised in pvr_fw_init(). Move this initialisation to
> pvr_kccb_init() alongside the initialisation of rest of the members of
> struct pvr_device->kccb.
> 
> Signed-off-by: Matt Coster <matt.coster@xxxxxxxxxx>
> Signed-off-by: Alexandru Dadu <alexandru.dadu@xxxxxxxxxx>

Reviewed-by: Alessio Belle <alessio.belle@xxxxxxxxxx>

Thanks,
Alessio

> ---
>  drivers/gpu/drm/imagination/pvr_ccb.c | 38 +++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/imagination/pvr_fw.c  | 17 +---------------
>  2 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
> index b702d122d791..3b5942b674be 100644
> --- a/drivers/gpu/drm/imagination/pvr_ccb.c
> +++ b/drivers/gpu/drm/imagination/pvr_ccb.c
> @@ -17,6 +17,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/mutex.h>
> +#include <linux/overflow.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  
> @@ -527,6 +528,7 @@ void pvr_kccb_wake_up_waiters(struct pvr_device *pvr_dev)
>   */
>  void pvr_kccb_fini(struct pvr_device *pvr_dev)
>  {
> +	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
>  	pvr_ccb_fini(&pvr_dev->kccb.ccb);
>  	WARN_ON(!list_empty(&pvr_dev->kccb.waiters));
>  	WARN_ON(pvr_dev->kccb.reserved_count);
> @@ -543,14 +545,42 @@ void pvr_kccb_fini(struct pvr_device *pvr_dev)
>  int
>  pvr_kccb_init(struct pvr_device *pvr_dev)
>  {
> -	pvr_dev->kccb.slot_count = 1 << ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
> +	const u32 num_slots_log2 = ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
> +	const u32 num_slots = 1 << num_slots_log2;
> +	u32 rtn_size;
> +	int err;
> +
> +	/*
> +	 * The inputs here are compile-time constants; there's no reason to try
> +	 * to gracefully handle overflow at runtime.
> +	 */
> +	BUILD_BUG_ON(check_mul_overflow(num_slots, sizeof(*pvr_dev->kccb.rtn), &rtn_size));
> +
> +	pvr_dev->kccb.slot_count = num_slots;
>  	INIT_LIST_HEAD(&pvr_dev->kccb.waiters);
>  	pvr_dev->kccb.fence_ctx.id = dma_fence_context_alloc(1);
>  	spin_lock_init(&pvr_dev->kccb.fence_ctx.lock);
>  
> -	return pvr_ccb_init(pvr_dev, &pvr_dev->kccb.ccb,
> -			    ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT,
> -			    sizeof(struct rogue_fwif_kccb_cmd));
> +	err = pvr_ccb_init(pvr_dev, &pvr_dev->kccb.ccb, num_slots_log2,
> +			   sizeof(struct rogue_fwif_kccb_cmd));
> +	if (err)
> +		return err;
> +
> +	/* Allocate memory for KCCB return slots. */
> +	pvr_dev->kccb.rtn = pvr_fw_object_create_and_map(pvr_dev, rtn_size,
> +							 PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
> +							 NULL, NULL, &pvr_dev->kccb.rtn_obj);
> +	if (IS_ERR(pvr_dev->kccb.rtn)) {
> +		err = PTR_ERR(pvr_dev->kccb.rtn);
> +		goto err_ccb_fini;
> +	}
> +
> +	return 0;
> +
> +err_ccb_fini:
> +	pvr_ccb_fini(&pvr_dev->kccb.ccb);
> +
> +	return err;
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
> index 850a3ec8e775..cec17352cf90 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw.c
> @@ -945,8 +945,6 @@ pvr_fw_init(struct pvr_device *pvr_dev)
>  		[PVR_FW_PROCESSOR_TYPE_RISCV] = &pvr_fw_defs_riscv,
>  	};
>  
> -	u32 kccb_size_log2 = ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
> -	u32 kccb_rtn_size = (1 << kccb_size_log2) * sizeof(*pvr_dev->kccb.rtn);
>  	struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
>  	int err;
>  
> @@ -981,18 +979,9 @@ pvr_fw_init(struct pvr_device *pvr_dev)
>  	if (err)
>  		goto err_kccb_fini;
>  
> -	/* Allocate memory for KCCB return slots. */
> -	pvr_dev->kccb.rtn = pvr_fw_object_create_and_map(pvr_dev, kccb_rtn_size,
> -							 PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
> -							 NULL, NULL, &pvr_dev->kccb.rtn_obj);
> -	if (IS_ERR(pvr_dev->kccb.rtn)) {
> -		err = PTR_ERR(pvr_dev->kccb.rtn);
> -		goto err_fwccb_fini;
> -	}
> -
>  	err = pvr_fw_create_structures(pvr_dev);
>  	if (err)
> -		goto err_kccb_rtn_release;
> +		goto err_fwccb_fini;
>  
>  	err = pvr_fw_start(pvr_dev);
>  	if (err)
> @@ -1014,9 +1003,6 @@ pvr_fw_init(struct pvr_device *pvr_dev)
>  err_destroy_structures:
>  	pvr_fw_destroy_structures(pvr_dev);
>  
> -err_kccb_rtn_release:
> -	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
> -
>  err_fwccb_fini:
>  	pvr_ccb_fini(&pvr_dev->fwccb);
>  
> @@ -1047,7 +1033,6 @@ pvr_fw_fini(struct pvr_device *pvr_dev)
>  	WRITE_ONCE(fw_dev->initialised, false);
>  
>  	pvr_fw_destroy_structures(pvr_dev);
> -	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
>  
>  	/*
>  	 * Ensure FWCCB worker has finished executing before destroying FWCCB. The IRQ handler has
>