[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aBzRb8ZKuGI3E_cu@valkosipuli.retiisi.eu>
Date: Thu, 8 May 2025 15:44:47 +0000
From: Sakari Ailus <sakari.ailus@....fi>
To: keke.li@...ogic.com
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
kieran.bingham@...asonboard.com, laurent.pinchart@...asonboard.com,
dan.scally@...asonboard.com, jacopo.mondi@...asonboard.com
Subject: Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver
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
Powered by blists - more mailing lists