Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver

[Sakari Ailus]

Hi Keke, Jacopo,

On Sun, Apr 27, 2025 at 02:27:16PM +0800, Keke Li via B4 Relay wrote:
> diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
> new file mode 100644
> index 000000000000..0e0b5d61654a
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c

...

> +static int c3_isp_params_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct c3_isp_params_buffer *buf = to_c3_isp_params_buffer(vbuf);
> +	struct c3_isp_params *params = vb2_get_drv_priv(vb->vb2_queue);
> +	struct c3_isp_params_cfg *cfg = buf->cfg;
> +	struct c3_isp_params_cfg *usr_cfg = vb2_plane_vaddr(vb, 0);
> +	size_t payload_size = vb2_get_plane_payload(vb, 0);
> +	size_t header_size = offsetof(struct c3_isp_params_cfg, data);
> +	size_t block_offset = 0;
> +	size_t cfg_size;
> +
> +	/* Payload size can't be greater than the destination buffer size */
> +	if (payload_size > params->vfmt.fmt.meta.buffersize) {
> +		dev_dbg(params->isp->dev,
> +			"Payload size is too large: %zu\n", payload_size);
> +		return -EINVAL;
> +	}
> +
> +	/* Payload size can't be smaller than the header size */
> +	if (payload_size < header_size) {
> +		dev_dbg(params->isp->dev,
> +			"Payload size is too small: %zu\n", payload_size);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Use the internal scratch buffer to avoid userspace modifying
> +	 * the buffer content while the driver is processing it.
> +	 */
> +	memcpy(cfg, usr_cfg, payload_size);
> +
> +	/* Only v0 is supported at the moment */
> +	if (cfg->version != C3_ISP_PARAMS_BUFFER_V0) {
> +		dev_dbg(params->isp->dev,
> +			"Invalid params buffer version: %u\n", cfg->version);
> +		return -EINVAL;
> +	}
> +
> +	/* Validate the size reported in the parameter buffer header */
> +	cfg_size = header_size + cfg->data_size;
> +	if (cfg_size != payload_size) {
> +		dev_dbg(params->isp->dev,
> +			"Data size %zu and payload size %zu are different\n",
> +			cfg_size, payload_size);
> +		return -EINVAL;
> +	}
> +
> +	/* Walk the list of parameter blocks and validate them */
> +	cfg_size = cfg->data_size;
> +	while (cfg_size >= sizeof(struct c3_isp_params_block_header)) {
> +		const struct c3_isp_params_block_header *block;
> +		const struct c3_isp_params_handler *handler;
> +
> +		block = (struct c3_isp_params_block_header *)
> +			&cfg->data[block_offset];
> +
> +		if (block->type >= ARRAY_SIZE(c3_isp_params_handlers)) {
> +			dev_dbg(params->isp->dev,
> +				"Invalid params block type\n");
> +			return -EINVAL;
> +		}
> +
> +		if (block->size > cfg_size) {
> +			dev_dbg(params->isp->dev,
> +				"Block size is greater than cfg size\n");
> +			return -EINVAL;
> +		}
> +
> +		if ((block->flags & (C3_ISP_PARAMS_BLOCK_FL_ENABLE |
> +				     C3_ISP_PARAMS_BLOCK_FL_DISABLE)) ==
> +		    (C3_ISP_PARAMS_BLOCK_FL_ENABLE |
> +		     C3_ISP_PARAMS_BLOCK_FL_DISABLE)) {
> +			dev_dbg(params->isp->dev,
> +				"Invalid parameters block flags\n");
> +			return -EINVAL;
> +		}
> +
> +		handler = &c3_isp_params_handlers[block->type];
> +		if (block->size != handler->size) {
> +			dev_dbg(params->isp->dev,
> +				"Invalid params block size\n");
> +			return -EINVAL;
> +		}
> +
> +		block_offset += block->size;
> +		cfg_size -= block->size;
> +	}
> +
> +	if (cfg_size) {
> +		dev_dbg(params->isp->dev,
> +			"Unexpected data after the params buffer end\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

The above looks very much like rkisp1_params_prepare_ext_params() in the
Rockchip ISP driver. Instead of copying all this non-trivial code in
verbatim here, could you instead refactor this so both the drivers could
use the same implementation?

The types are different so macros will be likely needed.

-- 
Regards,

Sakari Ailus