[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aB0bhkihpY81TrII@valkosipuli.retiisi.eu>
Date: Thu, 8 May 2025 21:00:54 +0000
From: Sakari Ailus <sakari.ailus@....fi>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: keke.li@...ogic.com, 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
Subject: Re: [PATCH v9 08/10] media: platform: Add C3 ISP driver
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
Powered by blists - more mailing lists