mbox series

[v4,0/7] media: rkisp1: Implement support for extensible parameters

Message ID 20240703101951.77785-1-jacopo.mondi@ideasonboard.com
Headers show
Series media: rkisp1: Implement support for extensible parameters | expand

Message

Jacopo Mondi July 3, 2024, 10:19 a.m. UTC 
v2->v3:

- Address Laurent's comments on the uAPI:
  - rename $block_config with  just 'config'
  - reduce header size
  - rename a few fields/blocks

- Address Laurent's comment on the params node:
  - Use the plane payload for memcpy() and buffer validation
  - drop buf_out_validate() and use buf_prepare() only
  - validate the total buffer size against the buffer payload
  - use const pointers where possible

v1->v2:

- re-order patches to introduce parameters buffer caching for the existing
  "fixed" format before introducing the "extensible" format
- align all structures to 64-bit boundaries in the uAPI
- remove NO_CHANGE enablement state and cache a bitmask of enabled blocks
- address review comments in documentation

The VeriSilicon ISP8000 IP, supported through the rkisp1 driver in the Linux
kernel, is integrated in several SoCs from different vendors. Different
revisions of the IP differ in the number of supported features and in the
register space location assigned to specific ISP blocks.

The current configuration parameters format, defined in
include/uapi/linux/rkisp1-config.h is realized by a C structure (struct
rkisp1_params_cfg) which wraps other structures that allows to configure
specific ISP blocks. The layout of the parameters buffer is part of the Linux
kernel uAPI and can hardly be extended or modified to adapt it to different
revisions of the same IP.

This series proposes the introduction of a new parameters format for the RkISP1
(without dropping support for the existing one) which is designed with the goals
of being:

1) versioned: can be changed without breaking existing applications
2) extensible: new blocks and parameters can be added without breaking the uABI

To do so, a new 'struct rkisp1_ext_params_cfg' type is introduced. It wraps an
header and a data buffer, which userspace fills with configuration blocks
for each ISP block it intends to configure. The parameters buffer is thus of
different effective sizes, depending on the number of blocks userspace intends
to configure.

The kernel driver parses the data block and decides, based on the versioning
number and the platform it operates on, how to handle each block.

The parameters format is very similar to the parameters format implemented
in the in-review Mali C55 ISP driver [1]

CI pipeline [2]

[1] https://lore.kernel.org/linux-media/20240529152858.183799-15-dan.scally@ideasonboard.com/
[2] https://gitlab.freedesktop.org/linux-media/users/jmondi/-/pipelines/1216065

Jacopo Mondi (7):
  uapi: rkisp1-config: Add extensible parameters format
  uapi: videodev2: Add V4L2_META_FMT_RK_ISP1_EXT_PARAMS
  media: rkisp1: Add struct rkisp1_params_buffer
  media: rkisp1: Copy the parameters buffer
  media: rkisp1: Cache the currently active format
  media: rkisp1: Implement extensible params support
  media: rkisp1: Implement s_fmt/try_fmt

 Documentation/admin-guide/media/rkisp1.rst    |  11 +-
 .../media/v4l/metafmt-rkisp1.rst              |  57 +-
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  31 +-
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 742 ++++++++++++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          |   1 +
 include/uapi/linux/rkisp1-config.h            | 489 ++++++++++++
 include/uapi/linux/videodev2.h                |   1 +
 7 files changed, 1262 insertions(+), 70 deletions(-)

--
2.45.2

Comments

Laurent Pinchart July 3, 2024, 1 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Wed, Jul 03, 2024 at 12:19:48PM +0200, Jacopo Mondi wrote:
> The rkisp1-params driver assumes the data buffer format is the only
> currently supported "fixed" one. The usage of the "fixed" format is
> assumed when allocating memory for the scratch buffers and when
> initializing the vb2 queue.
> 
> In order to prepare to support the "extensible" format beside the
> existing "fixed" one, add support in the driver for both formats by
> caching a pointer to the active one in the driver structure and use it
> in the vb2 queue operations and subdev pad operations implementations.
> 
> Do not yet allow userspace to select between the two formats as the
> support for the "extensible" format parsing will be introduced in a later
> patch in the series.
> 
> While at it, document the un-documented ycbcr_encoding field of
> struct rkisp1_params_ops.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  8 ++-
>  .../platform/rockchip/rkisp1/rkisp1-params.c  | 58 +++++++++++--------
>  2 files changed, 40 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 8d520c5c71c3..43cc727a628d 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -255,7 +255,7 @@ struct rkisp1_buffer {
>  struct rkisp1_params_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head queue;
> -	struct rkisp1_params_cfg *cfg;
> +	void *cfg;
>  };
>  
>  static inline struct rkisp1_params_buffer *
> @@ -392,8 +392,9 @@ struct rkisp1_params_ops {
>   * @ops:		pointer to the variant-specific operations
>   * @config_lock:	locks the buffer list 'params'
>   * @params:		queue of rkisp1_buffer
> - * @vdev_fmt:		v4l2_format of the metadata format
> + * @metafmt		the currently enabled metadata format
>   * @quantization:	the quantization configured on the isp's src pad
> + * @ycbcr_encoding	the YCbCr encoding
>   * @raw_type:		the bayer pattern on the isp video sink pad
>   */
>  struct rkisp1_params {
> @@ -403,7 +404,8 @@ struct rkisp1_params {
>  
>  	spinlock_t config_lock; /* locks the buffers list 'params' */
>  	struct list_head params;
> -	struct v4l2_format vdev_fmt;
> +
> +	const struct v4l2_meta_format *metafmt;
>  
>  	enum v4l2_quantization quantization;
>  	enum v4l2_ycbcr_encoding ycbcr_encoding;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 2d49038f8983..9444790c564f 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -35,6 +35,22 @@
>  #define RKISP1_ISP_CC_COEFF(n) \
>  			(RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4)
>  
> +enum rkisp1_params_formats {
> +	RKISP1_PARAMS_FIXED,
> +	RKISP1_PARAMS_EXTENSIBLE,
> +};
> +
> +static const struct v4l2_meta_format rkisp1_params_formats[] = {
> +	[RKISP1_PARAMS_FIXED] = {
> +		.dataformat = V4L2_META_FMT_RK_ISP1_PARAMS,
> +		.buffersize = sizeof(struct rkisp1_params_cfg),
> +	},
> +	[RKISP1_PARAMS_EXTENSIBLE] = {
> +		.dataformat = V4L2_META_FMT_RK_ISP1_EXT_PARAMS,
> +		.buffersize = sizeof(struct rkisp1_ext_params_cfg),
> +	},
> +};
> +
>  static inline void
>  rkisp1_param_set_bits(struct rkisp1_params *params, u32 reg, u32 bit_mask)
>  {
> @@ -1738,7 +1754,7 @@ static int rkisp1_params_enum_fmt_meta_out(struct file *file, void *priv,
>  	if (f->index > 0 || f->type != video->queue->type)
>  		return -EINVAL;
>  
> -	f->pixelformat = params->vdev_fmt.fmt.meta.dataformat;
> +	f->pixelformat = params->metafmt->dataformat;
>  
>  	return 0;
>  }
> @@ -1754,8 +1770,8 @@ static int rkisp1_params_g_fmt_meta_out(struct file *file, void *fh,
>  		return -EINVAL;
>  
>  	memset(meta, 0, sizeof(*meta));
> -	meta->dataformat = params->vdev_fmt.fmt.meta.dataformat;
> -	meta->buffersize = params->vdev_fmt.fmt.meta.buffersize;
> +	meta->dataformat = params->metafmt->dataformat;
> +	meta->buffersize = params->metafmt->buffersize;
>  
>  	return 0;
>  }
> @@ -1798,13 +1814,15 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
>  					 unsigned int sizes[],
>  					 struct device *alloc_devs[])
>  {
> +	struct rkisp1_params *params = vq->drv_priv;
> +
>  	*num_buffers = clamp_t(u32, *num_buffers,
>  			       RKISP1_ISP_PARAMS_REQ_BUFS_MIN,
>  			       RKISP1_ISP_PARAMS_REQ_BUFS_MAX);
>  
>  	*num_planes = 1;
>  
> -	sizes[0] = sizeof(struct rkisp1_params_cfg);
> +	sizes[0] = params->metafmt->buffersize;
>  
>  	return 0;
>  }
> @@ -1813,8 +1831,10 @@ static int rkisp1_params_vb2_buf_init(struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>  	struct rkisp1_params_buffer *params_buf = to_rkisp1_params_buffer(vbuf);
> +	struct rkisp1_params *params = vb->vb2_queue->drv_priv;
>  
> -	params_buf->cfg = kvmalloc(sizeof(*params_buf->cfg), GFP_KERNEL);
> +	params_buf->cfg = kvmalloc(params->metafmt->buffersize,
> +				   GFP_KERNEL);
>  	if (!params_buf->cfg)
>  		return -ENOMEM;
>  
> @@ -1849,16 +1869,14 @@ static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
>  	struct rkisp1_params_cfg *cfg =
>  		vb2_plane_vaddr(&params_buf->vb.vb2_buf, 0);
>  
> -	if (vb2_plane_size(vb, 0) < sizeof(struct rkisp1_params_cfg))
> +	if (vb2_get_plane_payload(vb, 0) < sizeof(struct rkisp1_params_cfg))

sizeof(*cfg)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		return -EINVAL;
>  
> -	vb2_set_plane_payload(vb, 0, sizeof(struct rkisp1_params_cfg));
> -
>  	/*
>  	 * Copy the parameters buffer to the internal scratch buffer to avoid
>  	 * userspace modifying the buffer content while the driver processes it.
>  	 */
> -	memcpy(params_buf->cfg, cfg, sizeof(*cfg));
> +	memcpy(params_buf->cfg, cfg, vb2_get_plane_payload(vb, 0));
>  
>  	return 0;
>  }
> @@ -1921,19 +1939,6 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
>  	return vb2_queue_init(q);
>  }
>  
> -static void rkisp1_init_params(struct rkisp1_params *params)
> -{
> -	params->vdev_fmt.fmt.meta.dataformat =
> -		V4L2_META_FMT_RK_ISP1_PARAMS;
> -	params->vdev_fmt.fmt.meta.buffersize =
> -		sizeof(struct rkisp1_params_cfg);
> -
> -	if (params->rkisp1->info->isp_ver == RKISP1_V12)
> -		params->ops = &rkisp1_v12_params_ops;
> -	else
> -		params->ops = &rkisp1_v10_params_ops;
> -}
> -
>  int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  {
>  	struct rkisp1_params *params = &rkisp1->params;
> @@ -1962,7 +1967,14 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
>  	vdev->vfl_dir = VFL_DIR_TX;
>  	rkisp1_params_init_vb2_queue(vdev->queue, params);
> -	rkisp1_init_params(params);
> +
> +	params->metafmt = &rkisp1_params_formats[RKISP1_PARAMS_FIXED];
> +
> +	if (params->rkisp1->info->isp_ver == RKISP1_V12)
> +		params->ops = &rkisp1_v12_params_ops;
> +	else
> +		params->ops = &rkisp1_v10_params_ops;
> +
>  	video_set_drvdata(vdev, params);
>  
>  	node->pad.flags = MEDIA_PAD_FL_SOURCE;
Laurent Pinchart July 3, 2024, 1:39 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Jul 03, 2024 at 12:19:49PM +0200, Jacopo Mondi wrote:
> Implement support in rkisp1-params for the extensible configuration
> parameters format.
> 
> Create a list of handlers for each ISP block that wraps the existing
> configuration functions and handles the ISP block enablement.
> 
> Parse the configuration parameters buffer in rkisp1_ext_params_config
> and filter the enable blocks by group, to allow setting the 'other'
> groups separately from the 'lsc' group to support the pre/post-configure
> operations.
> 
> Implement parameter buffer validation for the extensible format at
> .buf_prepare() time.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |   3 +
>  .../platform/rockchip/rkisp1/rkisp1-params.c  | 580 +++++++++++++++++-
>  2 files changed, 569 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 43cc727a628d..2f4bf7e97927 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -396,6 +396,7 @@ struct rkisp1_params_ops {
>   * @quantization:	the quantization configured on the isp's src pad
>   * @ycbcr_encoding	the YCbCr encoding
>   * @raw_type:		the bayer pattern on the isp video sink pad
> + * @enabled_blocks:	bitmask of enabled ISP blocks
>   */
>  struct rkisp1_params {
>  	struct rkisp1_vdev_node vnode;
> @@ -410,6 +411,8 @@ struct rkisp1_params {
>  	enum v4l2_quantization quantization;
>  	enum v4l2_ycbcr_encoding ycbcr_encoding;
>  	enum rkisp1_fmt_raw_pat_type raw_type;
> +
> +	u32 enabled_blocks;
>  };
>  
>  /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 9444790c564f..125093d16adb 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -35,6 +35,30 @@
>  #define RKISP1_ISP_CC_COEFF(n) \
>  			(RKISP1_CIF_ISP_CC_COEFF_0 + (n) * 4)
>  
> +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS	BIT(0)
> +#define RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC	BIT(1)
> +
> +union rkisp1_ext_params_config {
> +	struct rkisp1_ext_params_block_header header;
> +	struct rkisp1_ext_params_bls_config bls;
> +	struct rkisp1_ext_params_dpcc_config dpcc;
> +	struct rkisp1_ext_params_sdg_config sdg;
> +	struct rkisp1_ext_params_lsc_config lsc;
> +	struct rkisp1_ext_params_awb_gain_config awbg;
> +	struct rkisp1_ext_params_flt_config flt;
> +	struct rkisp1_ext_params_bdm_config bdm;
> +	struct rkisp1_ext_params_ctk_config ctk;
> +	struct rkisp1_ext_params_goc_config goc;
> +	struct rkisp1_ext_params_dpf_config dpf;
> +	struct rkisp1_ext_params_dpf_strength_config dpfs;
> +	struct rkisp1_ext_params_cproc_config cproc;
> +	struct rkisp1_ext_params_ie_config ie;
> +	struct rkisp1_ext_params_awb_meas_config awbm;
> +	struct rkisp1_ext_params_hst_config hst;
> +	struct rkisp1_ext_params_aec_config aec;
> +	struct rkisp1_ext_params_afc_config afc;
> +};
> +
>  enum rkisp1_params_formats {
>  	RKISP1_PARAMS_FIXED,
>  	RKISP1_PARAMS_EXTENSIBLE,
> @@ -1519,6 +1543,451 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
>  	}
>  }
>  
> +/*------------------------------------------------------------------------------
> + * Extensible parameters format handling
> + */
> +
> +static void
> +rkisp1_ext_params_bls(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_bls_config *bls = &block->bls;
> +
> +	if (bls->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
> +					RKISP1_CIF_ISP_BLS_ENA);
> +		return;
> +	}
> +
> +	rkisp1_bls_config(params, &bls->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
> +				      RKISP1_CIF_ISP_BLS_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_dpcc(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_dpcc_config *dpcc = &block->dpcc;
> +
> +	if (dpcc->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
> +					RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
> +		return;
> +	}
> +
> +	rkisp1_dpcc_config(params, &dpcc->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
> +				      RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
> +}
> +
> +static void
> +rkisp1_ext_params_sdg(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_sdg_config *sdg = &block->sdg;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> +					RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA);
> +		return;
> +	}
> +
> +	rkisp1_sdg_config(params, &sdg->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> +				      RKISP1_CIF_ISP_CTRL_ISP_GAMMA_IN_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_lsc(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_lsc_config *lsc = &block->lsc;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
> +					RKISP1_CIF_ISP_LSC_CTRL_ENA);
> +		return;
> +	}
> +
> +	rkisp1_lsc_config(params, &lsc->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
> +				      RKISP1_CIF_ISP_LSC_CTRL_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_awbg(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_awb_gain_config *awbg = &block->awbg;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> +					RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
> +		return;
> +	}
> +
> +	params->ops->awb_gain_config(params, &awbg->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAIN)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> +				      RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_flt(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_flt_config *flt = &block->flt;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE,
> +					RKISP1_CIF_ISP_FLT_ENA);
> +		return;
> +	}
> +
> +	rkisp1_flt_config(params, &flt->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_FILT_MODE,
> +				      RKISP1_CIF_ISP_FLT_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_bdm(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_bdm_config *bdm = &block->bdm;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
> +					RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
> +		return;
> +	}
> +
> +	rkisp1_bdm_config(params, &bdm->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
> +				      RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
> +}
> +
> +static void
> +rkisp1_ext_params_ctk(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_ctk_config *ctk = &block->ctk;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_ctk_enable(params, false);
> +		return;
> +	}
> +
> +	rkisp1_ctk_config(params, &ctk->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK)))
> +		rkisp1_ctk_enable(params, true);
> +}
> +
> +static void
> +rkisp1_ext_params_goc(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_goc_config *goc = &block->goc;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> +					RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
> +		return;
> +	}
> +
> +	params->ops->goc_config(params, &goc->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> +				      RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_dpf(struct rkisp1_params *params,
> +		      const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_dpf_config *dpf = &block->dpf;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE,
> +					RKISP1_CIF_ISP_DPF_MODE_EN);
> +		return;
> +	}
> +
> +	rkisp1_dpf_config(params, &dpf->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_DPF_MODE,
> +				      RKISP1_CIF_ISP_DPF_MODE_EN);
> +}
> +
> +static void
> +rkisp1_ext_params_dpfs(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_dpf_strength_config *dpfs = &block->dpfs;
> +
> +	rkisp1_dpf_strength_config(params, &dpfs->config);
> +}
> +
> +static void
> +rkisp1_ext_params_cproc(struct rkisp1_params *params,
> +			const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_cproc_config *cproc = &block->cproc;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
> +					RKISP1_CIF_C_PROC_CTR_ENABLE);
> +		return;
> +	}
> +
> +	rkisp1_cproc_config(params, &cproc->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_C_PROC_CTRL,
> +				      RKISP1_CIF_C_PROC_CTR_ENABLE);
> +}
> +
> +static void
> +rkisp1_ext_params_ie(struct rkisp1_params *params,
> +		     const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_ie_config *ie = &block->ie;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_ie_enable(params, false);
> +		return;
> +	}
> +
> +	rkisp1_ie_config(params, &ie->config);
> +
> +	if (!(params->enabled_blocks & BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_IE)))
> +		rkisp1_ie_enable(params, true);
> +}
> +
> +static void
> +rkisp1_ext_params_awbm(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_awb_meas_config *awbm = &block->awbm;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		params->ops->awb_meas_enable(params, &awbm->config,
> +					     false);
> +		return;
> +	}
> +
> +	params->ops->awb_meas_config(params, &awbm->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS)))
> +		params->ops->awb_meas_enable(params, &awbm->config,
> +					     true);
> +}
> +
> +static void
> +rkisp1_ext_params_hstm(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_hst_config *hst = &block->hst;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		params->ops->hst_enable(params, &hst->config, false);
> +		return;
> +	}
> +
> +	params->ops->hst_config(params, &hst->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS)))
> +		params->ops->hst_enable(params, &hst->config, true);
> +}
> +
> +static void
> +rkisp1_ext_params_aecm(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_aec_config *aec = &block->aec;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
> +					RKISP1_CIF_ISP_EXP_ENA);
> +		return;
> +	}
> +
> +	params->ops->aec_config(params, &aec->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
> +				      RKISP1_CIF_ISP_EXP_ENA);
> +}
> +
> +static void
> +rkisp1_ext_params_afcm(struct rkisp1_params *params,
> +		       const union rkisp1_ext_params_config *block)
> +{
> +	const struct rkisp1_ext_params_afc_config *afc = &block->afc;
> +
> +	if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> +		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
> +					RKISP1_CIF_ISP_AFM_ENA);
> +		return;
> +	}
> +
> +	params->ops->afm_config(params, &afc->config);
> +
> +	if (!(params->enabled_blocks &
> +	      BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS)))
> +		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
> +				      RKISP1_CIF_ISP_AFM_ENA);
> +}
> +
> +typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
> +			     const union rkisp1_ext_params_config *config);
> +
> +static const struct rkisp1_ext_params_handler {
> +	size_t size;
> +	rkisp1_block_handler handler;
> +	unsigned int group;
> +} rkisp1_ext_params_handlers[] = {
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
> +		.size		= sizeof(struct rkisp1_ext_params_bls_config),
> +		.handler	= rkisp1_ext_params_bls,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
> +		.size		= sizeof(struct rkisp1_ext_params_dpcc_config),
> +		.handler	= rkisp1_ext_params_dpcc,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG] = {
> +		.size		= sizeof(struct rkisp1_ext_params_sdg_config),
> +		.handler	= rkisp1_ext_params_sdg,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAIN] = {
> +		.size		= sizeof(struct rkisp1_ext_params_awb_gain_config),
> +		.handler	= rkisp1_ext_params_awbg,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT] = {
> +		.size		= sizeof(struct rkisp1_ext_params_flt_config),
> +		.handler	= rkisp1_ext_params_flt,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM] = {
> +		.size		= sizeof(struct rkisp1_ext_params_bdm_config),
> +		.handler	= rkisp1_ext_params_bdm,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK] = {
> +		.size		= sizeof(struct rkisp1_ext_params_ctk_config),
> +		.handler	= rkisp1_ext_params_ctk,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC] = {
> +		.size		= sizeof(struct rkisp1_ext_params_goc_config),
> +		.handler	= rkisp1_ext_params_goc,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF] = {
> +		.size		= sizeof(struct rkisp1_ext_params_dpf_config),
> +		.handler	= rkisp1_ext_params_dpf,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGTH] = {
> +		.size		= sizeof(struct rkisp1_ext_params_dpf_strength_config),
> +		.handler	= rkisp1_ext_params_dpfs,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC] = {
> +		.size		= sizeof(struct rkisp1_ext_params_cproc_config),
> +		.handler	= rkisp1_ext_params_cproc,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_IE] = {
> +		.size		= sizeof(struct rkisp1_ext_params_ie_config),
> +		.handler	= rkisp1_ext_params_ie,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC] = {
> +		.size		= sizeof(struct rkisp1_ext_params_lsc_config),
> +		.handler	= rkisp1_ext_params_lsc,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS] = {
> +		.size		= sizeof(struct rkisp1_ext_params_awb_meas_config),
> +		.handler	= rkisp1_ext_params_awbm,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS] = {
> +		.size		= sizeof(struct rkisp1_ext_params_hst_config),
> +		.handler	= rkisp1_ext_params_hstm,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS] = {
> +		.size		= sizeof(struct rkisp1_ext_params_aec_config),
> +		.handler	= rkisp1_ext_params_aecm,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +	[RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS] = {
> +		.size		= sizeof(struct rkisp1_ext_params_afc_config),
> +		.handler	= rkisp1_ext_params_afcm,
> +		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> +	},
> +};
> +
> +static void rkisp1_ext_params_config(struct rkisp1_params *params,
> +				     struct rkisp1_ext_params_cfg *cfg,
> +				     u32 block_group_mask)
> +{
> +	size_t block_offset = 0;
> +
> +	if (WARN_ON(!cfg))
> +		return;
> +
> +	/* Walk the list of parameter blocks and process them. */
> +	while (block_offset < cfg->data_size) {
> +		const struct rkisp1_ext_params_handler *block_handler;
> +		const union rkisp1_ext_params_config *block;
> +
> +		block = (const union rkisp1_ext_params_config *)
> +			&cfg->data[block_offset];
> +		block_offset += block->header.size;
> +
> +		/* Make sure the block is in the list of groups to configure. */
> +		block_handler = &rkisp1_ext_params_handlers[block->header.type];
> +		if (!(block_handler->group & block_group_mask))
> +			continue;
> +
> +		block_handler->handler(params, block);
> +
> +		if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE)
> +			params->enabled_blocks &= ~BIT(block->header.type);
> +		else
> +			params->enabled_blocks |= BIT(block->header.type);
> +	}
> +}
> +
>  static void rkisp1_params_complete_buffer(struct rkisp1_params *params,
>  					  struct rkisp1_params_buffer *buf,
>  					  unsigned int frame_sequence)
> @@ -1541,9 +2010,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
>  	if (!cur_buf)
>  		goto unlock;
>  
> -	rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> -	rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> -	rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> +	if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> +		rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> +		rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> +		rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> +	} else {
> +		rkisp1_ext_params_config(params, cur_buf->cfg,
> +					 RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> +					 RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
> +	}
>  
>  	/* update shadow register immediately */
>  	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1643,8 +2118,13 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
>  	if (!cur_buf)
>  		goto unlock;
>  
> -	rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> -	rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> +	if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> +		rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> +		rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> +	} else {
> +		rkisp1_ext_params_config(params, cur_buf->cfg,
> +					 RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS);
> +	}
>  
>  	/* update shadow register immediately */
>  	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1673,7 +2153,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params)
>  	if (!cur_buf)
>  		goto unlock;
>  
> -	rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> +	if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> +		rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> +	else
> +		rkisp1_ext_params_config(params, cur_buf->cfg,
> +					 RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
>  
>  	/* update shadow register immediately */
>  	rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1862,23 +2346,90 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
>  	spin_unlock_irq(&params->config_lock);
>  }
>  
> -static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
> +static int rkisp1_params_prepare_ext_params(struct rkisp1_params *params,
> +					    struct vb2_buffer *vb)
>  {
>  	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> -	struct rkisp1_params_buffer *params_buf = to_rkisp1_params_buffer(vbuf);
> -	struct rkisp1_params_cfg *cfg =
> -		vb2_plane_vaddr(&params_buf->vb.vb2_buf, 0);
> +	struct rkisp1_ext_params_cfg *cfg = vb2_plane_vaddr(&vbuf->vb2_buf, 0);

Wrong you're, you're supposed to validate the copy :-)

> +	size_t block_offset = 0;
> +	size_t cfg_size;
> +
> +	if (cfg->data_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> +		dev_dbg(params->rkisp1->dev,
> +			"Invalid parameters buffer size %u\n",

s/Invalid/Too larger/ would be more precise.

> +			cfg->data_size);
> +		return -EINVAL;
> +	}
>  
> -	if (vb2_get_plane_payload(vb, 0) < sizeof(struct rkisp1_params_cfg))
> +	cfg_size = offsetof(struct rkisp1_ext_params_cfg, data) +
> +		   cfg->data_size;
> +	if (cfg_size != vb2_get_plane_payload(vb, 0)) {

	if (cfg_size > vb2_get_plane_payload(vb, 0)) {

> +		dev_dbg(params->rkisp1->dev,
> +			"Buffer payload (%lu) larger than data size (%lu)\n",
> +			vb2_get_plane_payload(vb, 0), cfg_size);

The other way around, 

			"Data size %lu larger than buffer payload size %lu\n",
			cfg_size, vb2_get_plane_payload(vb, 0));

>  		return -EINVAL;
> +	}
> +
> +	/* Walk the list of parameter blocks and validate them. */
> +	cfg_size = cfg->data_size;
> +	while (cfg_size >= sizeof(struct rkisp1_ext_params_block_header)) {
> +		const struct rkisp1_ext_params_block_header *block;
> +		const struct rkisp1_ext_params_handler *handler;
> +
> +		block = (const struct rkisp1_ext_params_block_header *)
> +			&cfg->data[block_offset];
> +
> +		if (block->type >= ARRAY_SIZE(rkisp1_ext_params_handlers)) {
> +			dev_dbg(params->rkisp1->dev,
> +				"Invalid parameters block type\n");
> +			return -EINVAL;
> +		}
> +
> +		if (block->size > cfg_size) {
> +			dev_dbg(params->rkisp1->dev,
> +				"Premature end of parameters data\n");
> +			return -EINVAL;
> +		}
> +
> +		handler = &rkisp1_ext_params_handlers[block->type];
> +		if (block->size != handler->size) {
> +			dev_dbg(params->rkisp1->dev,
> +				"Invalid parameters block size\n");
> +			return -EINVAL;
> +		}
> +
> +		block_offset += block->size;
> +		cfg_size -= block->size;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct rkisp1_ext_params_cfg *cfg = vb2_plane_vaddr(&vbuf->vb2_buf, 0);
> +	struct rkisp1_params_buffer *params_buf =
> +		container_of(vbuf, struct rkisp1_params_buffer, vb);

	struct rkisp1_params_buffer *params_buf = to_rkisp1_params_buffer(vbuf);

> +	size_t payload = vb2_get_plane_payload(vb, 0);
> +	struct vb2_queue *vq = vb->vb2_queue;
> +	struct rkisp1_params *params = vq->drv_priv;

You can replace the two above lines with

	struct rkisp1_params *params = vb->vb2_queue->drv_priv;

>  
>  	/*
>  	 * Copy the parameters buffer to the internal scratch buffer to avoid
>  	 * userspace modifying the buffer content while the driver processes it.
>  	 */
> -	memcpy(params_buf->cfg, cfg, vb2_get_plane_payload(vb, 0));
> +	memcpy(params_buf->cfg, cfg, payload);
>  
> -	return 0;
> +	/* Only validate the plane payload size for fixed parameters format. */
> +	if (params->metafmt->dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> +		if (payload < sizeof(struct rkisp1_params_cfg))
> +			return -EINVAL;
> +
> +		return 0;
> +	}
> +
> +	return rkisp1_params_prepare_ext_params(params, vb);
>  }
>  
>  static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
> @@ -1898,6 +2449,8 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
>  
>  	list_for_each_entry(buf, &tmp_list, queue)
>  		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +
> +	params->enabled_blocks = 0;
>  }
>  
>  static const struct vb2_ops rkisp1_params_vb2_ops = {
> @@ -1909,7 +2462,6 @@ static const struct vb2_ops rkisp1_params_vb2_ops = {
>  	.buf_queue = rkisp1_params_vb2_buf_queue,
>  	.buf_prepare = rkisp1_params_vb2_buf_prepare,
>  	.stop_streaming = rkisp1_params_vb2_stop_streaming,
> -
>  };
>  
>  static const struct v4l2_file_operations rkisp1_params_fops = {