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

[Sakari Ailus <sakari.ailus@iki.fi>]

Hi Jacopo,

On Thu, May 08, 2025 at 06:28:41PM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Thu, May 08, 2025 at 03:44:47PM +0000, Sakari Ailus wrote:
> > 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
> 
> slightly similar, yes :)
> 
> > 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?
> >
> 
> Yeah, that's the plan.
> 
> We have more drivers in the pipeline using extensible parameters and this
> code (and possibily other parts) will certainly be factored out.
> 
> My plan is to add at one more user in and the do move common parts to
> the framework. Would this work for you ?

How about refactoring the two users to use a common framework first and
then using it in the third? :-) The more copies there are, the greater the
task to unify them.

-- 
Regards,

Sakari Ailus