* [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-09 5:59 ` kernel test robot
` (2 more replies)
2025-07-08 10:40 ` [PATCH 2/8] media: uapi: Convert RkISP1 to " Jacopo Mondi
` (7 subsequent siblings)
8 siblings, 3 replies; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
The header defines two types that all drivers that use the extensible
parameters format for ISP configuration shall use to build their own
parameters format.
The newly introduce type v4l2_params_block represent the
header to be prepend to each ISP configuration block and the
v4l2_params_buffer type represent the base type for the configuration
parameters buffer.
The newly introduced header is not meant to be used directly by
applications which should instead use the platform-specific ones.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
MAINTAINERS | 6 ++
include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
2 files changed, 112 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
F: drivers/media/i2c/vd56g3.c
F: drivers/media/i2c/vgxy61.c
+V4L2 EXTENSIBLE PARAMETERS FORMAT
+M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+F: include/uapi/linux/media/v4l2-extensible-params.h
+
VF610 NAND DRIVER
M: Stefan Agner <stefan@agner.ch>
L: linux-mtd@lists.infradead.org
diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
new file mode 100644
index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
--- /dev/null
+++ b/include/uapi/linux/media/v4l2-extensible-params.h
@@ -0,0 +1,106 @@
+/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
+/*
+ * Video4Linux2 extensible configuration parameters base types
+ *
+ * Copyright (C) 2025 Ideas On Board Oy
+ * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+ */
+
+#ifndef _UAPI_V4L2_PARAMS_H_
+#define _UAPI_V4L2_PARAMS_H_
+
+#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
+/*
+ * Note: each ISP driver exposes a different uAPI, where the types layout
+ * match (more or less strictly) the hardware registers layout.
+ *
+ * This file defines the base types on which each ISP driver can implement its
+ * own types that define its uAPI.
+ *
+ * This file is not meant to be included directly by applications which shall
+ * instead only include the ISP-specific implementation.
+ */
+#error "This file should not be included directly by applications"
+#endif
+
+#include <linux/types.h>
+
+/**
+ * struct v4l2_params_block - V4L2 extensible parameters block header
+ *
+ * This structure represents the common part of all the ISP configuration
+ * blocks. Each parameters block shall embed an instance of this structure type
+ * as its first member, followed by the block-specific configuration data. The
+ * driver inspects this common header to discern the block type and its size and
+ * properly handle the block content by casting it to the correct block-specific
+ * type.
+ *
+ * The @type field is one of the values enumerated by each platform-specific ISP
+ * block types which specifies how the data should be interpreted by the driver.
+ * The @size field specifies the size of the parameters block and is used by the
+ * driver for validation purposes.
+ *
+ * The @flags field is a bitmask of platform-specific control flags.
+ *
+ * Userspace shall never use this type directly but use the platform specific
+ * one with the associated data types.
+ *
+ * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
+ * - Amlogic C3: :c:type:`c3_isp_params_block_type`
+ *
+ * @type: The parameters block type (platform-specific)
+ * @flags: A bitmask of block flags (platform-specific)
+ * @size: Size (in bytes) of the parameters block, including this header
+ */
+struct v4l2_params_block {
+ __u16 type;
+ __u16 flags;
+ __u32 size;
+} __attribute__((aligned(8)));
+
+/**
+ * struct v4l2_params_buffer - V4L2 extensible parameters configuration
+ *
+ * This struct contains the configuration parameters of the ISP algorithms,
+ * serialized by userspace into a data buffer. Each configuration parameter
+ * block is represented by a block-specific structure which contains a
+ * :c:type:`v4l2_params_block` entry as first member. Userspace populates
+ * the @data buffer with configuration parameters for the blocks that it intends
+ * to configure. As a consequence, the data buffer effective size changes
+ * according to the number of ISP blocks that userspace intends to configure and
+ * is set by userspace in the @data_size field.
+ *
+ * The parameters buffer is versioned by the @version field to allow modifying
+ * and extending its definition. Userspace shall populate the @version field to
+ * inform the driver about the version it intends to use. The driver will parse
+ * and handle the @data buffer according to the data layout specific to the
+ * indicated version and return an error if the desired version is not
+ * supported.
+ *
+ * For each ISP block that userspace wants to configure, a block-specific
+ * structure is appended to the @data buffer, one after the other without gaps
+ * in between nor overlaps. Userspace shall populate the @data_size field with
+ * the effective size, in bytes, of the @data buffer.
+ *
+ * Each ISP driver using the extensible parameters format shall define a
+ * type which is type-convertible to this one, with the difference that the
+ * @data member shall actually a memory buffer of platform-specific size and
+ * not a pointer.
+ *
+ * Userspace shall never use this type directly but use the platform specific
+ * one with the associated data types.
+ *
+ * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
+ * - Amlogic C3: :c:type:`c3_isp_params_cfg`
+ *
+ * @version: The parameters buffer version (platform-specific)
+ * @data_size: The configuration data effective size, excluding this header
+ * @data: The configuration data
+ */
+struct v4l2_params_buffer {
+ __u32 version;
+ __u32 data_size;
+ __u8 data[];
+};
+
+#endif /* _UAPI_V4L2_PARAMS_H_ */
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-08 10:40 ` [PATCH 1/8] media: uapi: Introduce V4L2 extensible params Jacopo Mondi
@ 2025-07-09 5:59 ` kernel test robot
2025-07-09 11:33 ` Dan Scally
2025-07-09 18:59 ` Nicolas Dufresne
2 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2025-07-09 5:59 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: llvm, oe-kbuild-all, linux-media, linux-kernel, linux-rockchip,
linux-arm-kernel, Jacopo Mondi
Hi Jacopo,
kernel test robot noticed the following build errors:
[auto build test ERROR on a8598c7de1bcd94461ca54c972efa9b4ea501fb9]
url: https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-uapi-Introduce-V4L2-extensible-params/20250708-184651
base: a8598c7de1bcd94461ca54c972efa9b4ea501fb9
patch link: https://lore.kernel.org/r/20250708-extensible-parameters-validation-v1-1-9fc27c9c728c%40ideasonboard.com
patch subject: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
config: i386-buildonly-randconfig-003-20250709 (https://download.01.org/0day-ci/archive/20250709/202507091305.hPkKkWZJ-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250709/202507091305.hPkKkWZJ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507091305.hPkKkWZJ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <built-in>:1:
>> ./usr/include/linux/media/v4l2-extensible-params.h:23:2: error: "This file should not be included directly by applications"
23 | #error "This file should not be included directly by applications"
| ^
1 error generated.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-08 10:40 ` [PATCH 1/8] media: uapi: Introduce V4L2 extensible params Jacopo Mondi
2025-07-09 5:59 ` kernel test robot
@ 2025-07-09 11:33 ` Dan Scally
2025-07-09 11:53 ` Jacopo Mondi
2025-07-09 18:59 ` Nicolas Dufresne
2 siblings, 1 reply; 27+ messages in thread
From: Dan Scally @ 2025-07-09 11:33 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo - thanks for the patches
On 08/07/2025 11:40, Jacopo Mondi wrote:
> Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
>
> The header defines two types that all drivers that use the extensible
> parameters format for ISP configuration shall use to build their own
> parameters format.
>
> The newly introduce type v4l2_params_block represent the
> header to be prepend to each ISP configuration block and the
> v4l2_params_buffer type represent the base type for the configuration
> parameters buffer.
>
> The newly introduced header is not meant to be used directly by
> applications which should instead use the platform-specific ones.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> MAINTAINERS | 6 ++
> include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> 2 files changed, 112 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
> F: drivers/media/i2c/vd56g3.c
> F: drivers/media/i2c/vgxy61.c
>
> +V4L2 EXTENSIBLE PARAMETERS FORMAT
> +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: include/uapi/linux/media/v4l2-extensible-params.h
> +
> VF610 NAND DRIVER
> M: Stefan Agner <stefan@agner.ch>
> L: linux-mtd@lists.infradead.org
> diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> --- /dev/null
> +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> +/*
> + * Video4Linux2 extensible configuration parameters base types
> + *
> + * Copyright (C) 2025 Ideas On Board Oy
> + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> + */
> +
> +#ifndef _UAPI_V4L2_PARAMS_H_
> +#define _UAPI_V4L2_PARAMS_H_
> +
> +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> +/*
> + * Note: each ISP driver exposes a different uAPI, where the types layout
> + * match (more or less strictly) the hardware registers layout.
> + *
> + * This file defines the base types on which each ISP driver can implement its
> + * own types that define its uAPI.
> + *
> + * This file is not meant to be included directly by applications which shall
> + * instead only include the ISP-specific implementation.
> + */
> +#error "This file should not be included directly by applications"
> +#endif
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct v4l2_params_block - V4L2 extensible parameters block header
struct v4l2_params_block_header would be nicer I think
> + *
> + * This structure represents the common part of all the ISP configuration
> + * blocks. Each parameters block shall embed an instance of this structure type
> + * as its first member, followed by the block-specific configuration data. The
> + * driver inspects this common header to discern the block type and its size and
> + * properly handle the block content by casting it to the correct block-specific
> + * type.
> + *
> + * The @type field is one of the values enumerated by each platform-specific ISP
> + * block types which specifies how the data should be interpreted by the driver.
> + * The @size field specifies the size of the parameters block and is used by the
> + * driver for validation purposes.
> + *
> + * The @flags field is a bitmask of platform-specific control flags.
Since we're including flags in this base struct rather than a platform specific subclass I think
perhaps we should centralise some flags (which I think is supported by the fact that all three
implementations share the same flags so far). Perhaps we could reserve the bottom 8 bits for common
flags (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for platform specific
flags. I think we could then drop the platform specific validation for rkisp1 and c3 and just pass
null to the helpers, since they do the same thing.
> + *
> + * Userspace shall never use this type directly but use the platform specific
> + * one with the associated data types.
Why wouldn't userspace just use these directly? I could see why it might be difficult for the C3 and
Rkisp1 which are merged, but for a new implementation couldn't they just use these objects without
bothering to define their own?
If we end up using these objects directly I think it would be nice to have the example code block
from the platform specific headers documentation here too.
> + *
> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> + *
> + * @type: The parameters block type (platform-specific)
> + * @flags: A bitmask of block flags (platform-specific)
> + * @size: Size (in bytes) of the parameters block, including this header
> + */
> +struct v4l2_params_block {
> + __u16 type;
> + __u16 flags;
> + __u32 size;
> +} __attribute__((aligned(8)));
> +
> +/**
> + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> + *
> + * This struct contains the configuration parameters of the ISP algorithms,
> + * serialized by userspace into a data buffer. Each configuration parameter
> + * block is represented by a block-specific structure which contains a
> + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> + * the @data buffer with configuration parameters for the blocks that it intends
> + * to configure. As a consequence, the data buffer effective size changes
> + * according to the number of ISP blocks that userspace intends to configure and
> + * is set by userspace in the @data_size field.
> + *
> + * The parameters buffer is versioned by the @version field to allow modifying
> + * and extending its definition. Userspace shall populate the @version field to
> + * inform the driver about the version it intends to use. The driver will parse
> + * and handle the @data buffer according to the data layout specific to the
> + * indicated version and return an error if the desired version is not
> + * supported.
> + *
> + * For each ISP block that userspace wants to configure, a block-specific
> + * structure is appended to the @data buffer, one after the other without gaps
> + * in between nor overlaps. Userspace shall populate the @data_size field with
> + * the effective size, in bytes, of the @data buffer.
> + *
> + * Each ISP driver using the extensible parameters format shall define a
> + * type which is type-convertible to this one, with the difference that the
> + * @data member shall actually a memory buffer of platform-specific size and
> + * not a pointer.
Why not just use this object directly? We could provide a helper in v4l2-extensible-params.h that
calculates the size of the buffer with a given data array size for the driver's convenience
Thanks
Dan
> + *
> + * Userspace shall never use this type directly but use the platform specific
> + * one with the associated data types.
> + *
> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> + *
> + * @version: The parameters buffer version (platform-specific)
> + * @data_size: The configuration data effective size, excluding this header
> + * @data: The configuration data
> + */
> +struct v4l2_params_buffer {
> + __u32 version;
> + __u32 data_size;
> + __u8 data[];
> +};
> +
> +#endif /* _UAPI_V4L2_PARAMS_H_ */
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-09 11:33 ` Dan Scally
@ 2025-07-09 11:53 ` Jacopo Mondi
2025-07-09 13:18 ` Dan Scally
0 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-09 11:53 UTC (permalink / raw)
To: Dan Scally
Cc: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Dan,
thanks for the comments
On Wed, Jul 09, 2025 at 12:33:17PM +0100, Dan Scally wrote:
> Hi Jacopo - thanks for the patches
>
> On 08/07/2025 11:40, Jacopo Mondi wrote:
> > Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
> >
> > The header defines two types that all drivers that use the extensible
> > parameters format for ISP configuration shall use to build their own
> > parameters format.
> >
> > The newly introduce type v4l2_params_block represent the
> > header to be prepend to each ISP configuration block and the
> > v4l2_params_buffer type represent the base type for the configuration
> > parameters buffer.
> >
> > The newly introduced header is not meant to be used directly by
> > applications which should instead use the platform-specific ones.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > MAINTAINERS | 6 ++
> > include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> > 2 files changed, 112 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
> > F: drivers/media/i2c/vd56g3.c
> > F: drivers/media/i2c/vgxy61.c
> > +V4L2 EXTENSIBLE PARAMETERS FORMAT
> > +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > +L: linux-media@vger.kernel.org
> > +S: Maintained
> > +F: include/uapi/linux/media/v4l2-extensible-params.h
> > +
> > VF610 NAND DRIVER
> > M: Stefan Agner <stefan@agner.ch>
> > L: linux-mtd@lists.infradead.org
> > diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> > --- /dev/null
> > +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> > @@ -0,0 +1,106 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> > +/*
> > + * Video4Linux2 extensible configuration parameters base types
> > + *
> > + * Copyright (C) 2025 Ideas On Board Oy
> > + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > + */
> > +
> > +#ifndef _UAPI_V4L2_PARAMS_H_
> > +#define _UAPI_V4L2_PARAMS_H_
> > +
> > +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > +/*
> > + * Note: each ISP driver exposes a different uAPI, where the types layout
> > + * match (more or less strictly) the hardware registers layout.
> > + *
> > + * This file defines the base types on which each ISP driver can implement its
> > + * own types that define its uAPI.
> > + *
> > + * This file is not meant to be included directly by applications which shall
> > + * instead only include the ISP-specific implementation.
> > + */
> > +#error "This file should not be included directly by applications"
> > +#endif
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct v4l2_params_block - V4L2 extensible parameters block header
>
> struct v4l2_params_block_header would be nicer I think
>
That's what I had started with :)
I'm debated between a longer but more explicative name, or a shorter
one.
> > + *
> > + * This structure represents the common part of all the ISP configuration
> > + * blocks. Each parameters block shall embed an instance of this structure type
> > + * as its first member, followed by the block-specific configuration data. The
> > + * driver inspects this common header to discern the block type and its size and
> > + * properly handle the block content by casting it to the correct block-specific
> > + * type.
> > + *
> > + * The @type field is one of the values enumerated by each platform-specific ISP
> > + * block types which specifies how the data should be interpreted by the driver.
> > + * The @size field specifies the size of the parameters block and is used by the
> > + * driver for validation purposes.
> > + *
> > + * The @flags field is a bitmask of platform-specific control flags.
> Since we're including flags in this base struct rather than a platform
> specific subclass I think perhaps we should centralise some flags (which I
> think is supported by the fact that all three implementations share the same
> flags so far). Perhaps we could reserve the bottom 8 bits for common flags
> (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for
> platform specific flags. I think we could then drop the platform specific
> validation for rkisp1 and c3 and just pass null to the helpers, since they
> do the same thing.
Yes, that's one of the things I was not sure about... if we should
centralize flags definition as well or not...
Knowing that Mali will use the same flags that the two existing
implementations already have is a good indication that we can probably
centralize at least the ENABLE/DISABLE ones
> > + *
> > + * Userspace shall never use this type directly but use the platform specific
> > + * one with the associated data types.
>
> Why wouldn't userspace just use these directly? I could see why it might be
> difficult for the C3 and Rkisp1 which are merged, but for a new
> implementation couldn't they just use these objects without bothering to
> define their own?
>
mmm, my thinking was that each driver implementation shall define
their own types because I would expect that they will have to define
their own meta image format... For v4l2_params_buffer see below, for
the blocks it might be totally possible to use these type most
probably..
>
> If we end up using these objects directly I think it would be nice to have
> the example code block from the platform specific headers documentation here
> too.
>
> > + *
> > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> > + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> > + *
> > + * @type: The parameters block type (platform-specific)
> > + * @flags: A bitmask of block flags (platform-specific)
> > + * @size: Size (in bytes) of the parameters block, including this header
> > + */
> > +struct v4l2_params_block {
> > + __u16 type;
> > + __u16 flags;
> > + __u32 size;
> > +} __attribute__((aligned(8)));
> > +
> > +/**
> > + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> > + *
> > + * This struct contains the configuration parameters of the ISP algorithms,
> > + * serialized by userspace into a data buffer. Each configuration parameter
> > + * block is represented by a block-specific structure which contains a
> > + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> > + * the @data buffer with configuration parameters for the blocks that it intends
> > + * to configure. As a consequence, the data buffer effective size changes
> > + * according to the number of ISP blocks that userspace intends to configure and
> > + * is set by userspace in the @data_size field.
> > + *
> > + * The parameters buffer is versioned by the @version field to allow modifying
> > + * and extending its definition. Userspace shall populate the @version field to
> > + * inform the driver about the version it intends to use. The driver will parse
> > + * and handle the @data buffer according to the data layout specific to the
> > + * indicated version and return an error if the desired version is not
> > + * supported.
> > + *
> > + * For each ISP block that userspace wants to configure, a block-specific
> > + * structure is appended to the @data buffer, one after the other without gaps
> > + * in between nor overlaps. Userspace shall populate the @data_size field with
> > + * the effective size, in bytes, of the @data buffer.
> > + *
> > + * Each ISP driver using the extensible parameters format shall define a
> > + * type which is type-convertible to this one, with the difference that the
> > + * @data member shall actually a memory buffer of platform-specific size and
> > + * not a pointer.
>
> Why not just use this object directly? We could provide a helper in
> v4l2-extensible-params.h that calculates the size of the buffer with a given
> data array size for the driver's convenience
The main reason I thought v4l2_params_buffer cannot be used is because
of the flexible-array at the end of the type
struct v4l2_params_buffer {
__u32 version;
__u32 data_size;
__u8 data[];
};
vs
struct rkisp1_ext_params_cfg {
__u32 version;
__u32 data_size;
__u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
};
I might have missed what you're suggesting here with the helper in
v4l2-extensible-params.h :)
>
>
> Thanks
>
> Dan
>
> > + *
> > + * Userspace shall never use this type directly but use the platform specific
> > + * one with the associated data types.
> > + *
> > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> > + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> > + *
> > + * @version: The parameters buffer version (platform-specific)
> > + * @data_size: The configuration data effective size, excluding this header
> > + * @data: The configuration data
> > + */
> > +struct v4l2_params_buffer {
> > + __u32 version;
> > + __u32 data_size;
> > + __u8 data[];
> > +};
> > +
> > +#endif /* _UAPI_V4L2_PARAMS_H_ */
> >
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-09 11:53 ` Jacopo Mondi
@ 2025-07-09 13:18 ` Dan Scally
2025-07-10 7:15 ` Jacopo Mondi
0 siblings, 1 reply; 27+ messages in thread
From: Dan Scally @ 2025-07-09 13:18 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 09/07/2025 12:53, Jacopo Mondi wrote:
> Hi Dan,
> thanks for the comments
>
> On Wed, Jul 09, 2025 at 12:33:17PM +0100, Dan Scally wrote:
>> Hi Jacopo - thanks for the patches
>>
>> On 08/07/2025 11:40, Jacopo Mondi wrote:
>>> Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
>>>
>>> The header defines two types that all drivers that use the extensible
>>> parameters format for ISP configuration shall use to build their own
>>> parameters format.
>>>
>>> The newly introduce type v4l2_params_block represent the
>>> header to be prepend to each ISP configuration block and the
>>> v4l2_params_buffer type represent the base type for the configuration
>>> parameters buffer.
>>>
>>> The newly introduced header is not meant to be used directly by
>>> applications which should instead use the platform-specific ones.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> ---
>>> MAINTAINERS | 6 ++
>>> include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
>>> 2 files changed, 112 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
>>> F: drivers/media/i2c/vd56g3.c
>>> F: drivers/media/i2c/vgxy61.c
>>> +V4L2 EXTENSIBLE PARAMETERS FORMAT
>>> +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> +L: linux-media@vger.kernel.org
>>> +S: Maintained
>>> +F: include/uapi/linux/media/v4l2-extensible-params.h
>>> +
>>> VF610 NAND DRIVER
>>> M: Stefan Agner <stefan@agner.ch>
>>> L: linux-mtd@lists.infradead.org
>>> diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
>>> --- /dev/null
>>> +++ b/include/uapi/linux/media/v4l2-extensible-params.h
>>> @@ -0,0 +1,106 @@
>>> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
>>> +/*
>>> + * Video4Linux2 extensible configuration parameters base types
>>> + *
>>> + * Copyright (C) 2025 Ideas On Board Oy
>>> + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> + */
>>> +
>>> +#ifndef _UAPI_V4L2_PARAMS_H_
>>> +#define _UAPI_V4L2_PARAMS_H_
>>> +
>>> +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
>>> +/*
>>> + * Note: each ISP driver exposes a different uAPI, where the types layout
>>> + * match (more or less strictly) the hardware registers layout.
>>> + *
>>> + * This file defines the base types on which each ISP driver can implement its
>>> + * own types that define its uAPI.
>>> + *
>>> + * This file is not meant to be included directly by applications which shall
>>> + * instead only include the ISP-specific implementation.
>>> + */
>>> +#error "This file should not be included directly by applications"
>>> +#endif
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +/**
>>> + * struct v4l2_params_block - V4L2 extensible parameters block header
>> struct v4l2_params_block_header would be nicer I think
>>
> That's what I had started with :)
>
> I'm debated between a longer but more explicative name, or a shorter
> one.
I vote for longer here but only because I think the phrase "block" applies more properly to the
likes of struct rkisp1_ext_params_bls_config for example.
>
>>> + *
>>> + * This structure represents the common part of all the ISP configuration
>>> + * blocks. Each parameters block shall embed an instance of this structure type
>>> + * as its first member, followed by the block-specific configuration data. The
>>> + * driver inspects this common header to discern the block type and its size and
>>> + * properly handle the block content by casting it to the correct block-specific
>>> + * type.
>>> + *
>>> + * The @type field is one of the values enumerated by each platform-specific ISP
>>> + * block types which specifies how the data should be interpreted by the driver.
>>> + * The @size field specifies the size of the parameters block and is used by the
>>> + * driver for validation purposes.
>>> + *
>>> + * The @flags field is a bitmask of platform-specific control flags.
>> Since we're including flags in this base struct rather than a platform
>> specific subclass I think perhaps we should centralise some flags (which I
>> think is supported by the fact that all three implementations share the same
>> flags so far). Perhaps we could reserve the bottom 8 bits for common flags
>> (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for
>> platform specific flags. I think we could then drop the platform specific
>> validation for rkisp1 and c3 and just pass null to the helpers, since they
>> do the same thing.
> Yes, that's one of the things I was not sure about... if we should
> centralize flags definition as well or not...
I think probably the ability to have both centralised and platform specific ones would be worthwhile
>
> Knowing that Mali will use the same flags that the two existing
> implementations already have is a good indication that we can probably
> centralize at least the ENABLE/DISABLE ones
Yeah
>
>>> + *
>>> + * Userspace shall never use this type directly but use the platform specific
>>> + * one with the associated data types.
>> Why wouldn't userspace just use these directly? I could see why it might be
>> difficult for the C3 and Rkisp1 which are merged, but for a new
>> implementation couldn't they just use these objects without bothering to
>> define their own?
>>
> mmm, my thinking was that each driver implementation shall define
> their own types because I would expect that they will have to define
> their own meta image format... For v4l2_params_buffer see below, for
> the blocks it might be totally possible to use these type most
> probably..
>
>> If we end up using these objects directly I think it would be nice to have
>> the example code block from the platform specific headers documentation here
>> too.
>>
>>> + *
>>> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
>>> + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
>>> + *
>>> + * @type: The parameters block type (platform-specific)
>>> + * @flags: A bitmask of block flags (platform-specific)
>>> + * @size: Size (in bytes) of the parameters block, including this header
>>> + */
>>> +struct v4l2_params_block {
>>> + __u16 type;
>>> + __u16 flags;
>>> + __u32 size;
>>> +} __attribute__((aligned(8)));
>>> +
>>> +/**
>>> + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
>>> + *
>>> + * This struct contains the configuration parameters of the ISP algorithms,
>>> + * serialized by userspace into a data buffer. Each configuration parameter
>>> + * block is represented by a block-specific structure which contains a
>>> + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
>>> + * the @data buffer with configuration parameters for the blocks that it intends
>>> + * to configure. As a consequence, the data buffer effective size changes
>>> + * according to the number of ISP blocks that userspace intends to configure and
>>> + * is set by userspace in the @data_size field.
>>> + *
>>> + * The parameters buffer is versioned by the @version field to allow modifying
>>> + * and extending its definition. Userspace shall populate the @version field to
>>> + * inform the driver about the version it intends to use. The driver will parse
>>> + * and handle the @data buffer according to the data layout specific to the
>>> + * indicated version and return an error if the desired version is not
>>> + * supported.
>>> + *
>>> + * For each ISP block that userspace wants to configure, a block-specific
>>> + * structure is appended to the @data buffer, one after the other without gaps
>>> + * in between nor overlaps. Userspace shall populate the @data_size field with
>>> + * the effective size, in bytes, of the @data buffer.
>>> + *
>>> + * Each ISP driver using the extensible parameters format shall define a
>>> + * type which is type-convertible to this one, with the difference that the
>>> + * @data member shall actually a memory buffer of platform-specific size and
>>> + * not a pointer.
>> Why not just use this object directly? We could provide a helper in
>> v4l2-extensible-params.h that calculates the size of the buffer with a given
>> data array size for the driver's convenience
> The main reason I thought v4l2_params_buffer cannot be used is because
> of the flexible-array at the end of the type
>
> struct v4l2_params_buffer {
> __u32 version;
> __u32 data_size;
> __u8 data[];
> };
>
> vs
>
> struct rkisp1_ext_params_cfg {
> __u32 version;
> __u32 data_size;
> __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
> };
>
> I might have missed what you're suggesting here with the helper in
> v4l2-extensible-params.h :)
So I think a known size is needed to accomodate operations like "memcpy(dst, src,
sizeof(rkisp1_ext_params_cfg))", but with something like...
#define v4l2_params_buffer_size(max_params_size) \
(offsetof(struct v4l2_params_buffer, data) + max_params_size)
then the above operation can be memcpy(dst,
src, v4l2_params_buffer_size(RKISP1_EXT_PARAMS_MAX_SIZE)) instead
Unless I'm missing something that should be enough to drop the driver specific struct...it seems to
work ok anyway
Dan
>
>
>>
>> Thanks
>>
>> Dan
>>
>>> + *
>>> + * Userspace shall never use this type directly but use the platform specific
>>> + * one with the associated data types.
>>> + *
>>> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
>>> + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
>>> + *
>>> + * @version: The parameters buffer version (platform-specific)
>>> + * @data_size: The configuration data effective size, excluding this header
>>> + * @data: The configuration data
>>> + */
>>> +struct v4l2_params_buffer {
>>> + __u32 version;
>>> + __u32 data_size;
>>> + __u8 data[];
>>> +};
>>> +
>>> +#endif /* _UAPI_V4L2_PARAMS_H_ */
>>>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-09 13:18 ` Dan Scally
@ 2025-07-10 7:15 ` Jacopo Mondi
2025-07-10 9:10 ` Jacopo Mondi
0 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-10 7:15 UTC (permalink / raw)
To: Dan Scally
Cc: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Dan
On Wed, Jul 09, 2025 at 02:18:07PM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 09/07/2025 12:53, Jacopo Mondi wrote:
> > Hi Dan,
> > thanks for the comments
> >
> > On Wed, Jul 09, 2025 at 12:33:17PM +0100, Dan Scally wrote:
> > > Hi Jacopo - thanks for the patches
> > >
> > > On 08/07/2025 11:40, Jacopo Mondi wrote:
> > > > Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
> > > >
> > > > The header defines two types that all drivers that use the extensible
> > > > parameters format for ISP configuration shall use to build their own
> > > > parameters format.
> > > >
> > > > The newly introduce type v4l2_params_block represent the
> > > > header to be prepend to each ISP configuration block and the
> > > > v4l2_params_buffer type represent the base type for the configuration
> > > > parameters buffer.
> > > >
> > > > The newly introduced header is not meant to be used directly by
> > > > applications which should instead use the platform-specific ones.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > ---
> > > > MAINTAINERS | 6 ++
> > > > include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> > > > 2 files changed, 112 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
> > > > F: drivers/media/i2c/vd56g3.c
> > > > F: drivers/media/i2c/vgxy61.c
> > > > +V4L2 EXTENSIBLE PARAMETERS FORMAT
> > > > +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > +L: linux-media@vger.kernel.org
> > > > +S: Maintained
> > > > +F: include/uapi/linux/media/v4l2-extensible-params.h
> > > > +
> > > > VF610 NAND DRIVER
> > > > M: Stefan Agner <stefan@agner.ch>
> > > > L: linux-mtd@lists.infradead.org
> > > > diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > @@ -0,0 +1,106 @@
> > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> > > > +/*
> > > > + * Video4Linux2 extensible configuration parameters base types
> > > > + *
> > > > + * Copyright (C) 2025 Ideas On Board Oy
> > > > + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > + */
> > > > +
> > > > +#ifndef _UAPI_V4L2_PARAMS_H_
> > > > +#define _UAPI_V4L2_PARAMS_H_
> > > > +
> > > > +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > > > +/*
> > > > + * Note: each ISP driver exposes a different uAPI, where the types layout
> > > > + * match (more or less strictly) the hardware registers layout.
> > > > + *
> > > > + * This file defines the base types on which each ISP driver can implement its
> > > > + * own types that define its uAPI.
> > > > + *
> > > > + * This file is not meant to be included directly by applications which shall
> > > > + * instead only include the ISP-specific implementation.
> > > > + */
> > > > +#error "This file should not be included directly by applications"
> > > > +#endif
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +/**
> > > > + * struct v4l2_params_block - V4L2 extensible parameters block header
> > > struct v4l2_params_block_header would be nicer I think
> > >
> > That's what I had started with :)
> >
> > I'm debated between a longer but more explicative name, or a shorter
> > one.
>
>
> I vote for longer here but only because I think the phrase "block" applies
> more properly to the likes of struct rkisp1_ext_params_bls_config for
> example.
Thanks, I've seen you made a patch to change this!
>
> >
> > > > + *
> > > > + * This structure represents the common part of all the ISP configuration
> > > > + * blocks. Each parameters block shall embed an instance of this structure type
> > > > + * as its first member, followed by the block-specific configuration data. The
> > > > + * driver inspects this common header to discern the block type and its size and
> > > > + * properly handle the block content by casting it to the correct block-specific
> > > > + * type.
> > > > + *
> > > > + * The @type field is one of the values enumerated by each platform-specific ISP
> > > > + * block types which specifies how the data should be interpreted by the driver.
> > > > + * The @size field specifies the size of the parameters block and is used by the
> > > > + * driver for validation purposes.
> > > > + *
> > > > + * The @flags field is a bitmask of platform-specific control flags.
> > > Since we're including flags in this base struct rather than a platform
> > > specific subclass I think perhaps we should centralise some flags (which I
> > > think is supported by the fact that all three implementations share the same
> > > flags so far). Perhaps we could reserve the bottom 8 bits for common flags
> > > (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for
> > > platform specific flags. I think we could then drop the platform specific
> > > validation for rkisp1 and c3 and just pass null to the helpers, since they
> > > do the same thing.
> > Yes, that's one of the things I was not sure about... if we should
> > centralize flags definition as well or not...
>
>
> I think probably the ability to have both centralised and platform specific ones would be worthwhile
>
> >
> > Knowing that Mali will use the same flags that the two existing
> > implementations already have is a good indication that we can probably
> > centralize at least the ENABLE/DISABLE ones
>
>
> Yeah
>
> >
> > > > + *
> > > > + * Userspace shall never use this type directly but use the platform specific
> > > > + * one with the associated data types.
> > > Why wouldn't userspace just use these directly? I could see why it might be
> > > difficult for the C3 and Rkisp1 which are merged, but for a new
> > > implementation couldn't they just use these objects without bothering to
> > > define their own?
> > >
> > mmm, my thinking was that each driver implementation shall define
> > their own types because I would expect that they will have to define
> > their own meta image format... For v4l2_params_buffer see below, for
> > the blocks it might be totally possible to use these type most
> > probably..
> >
> > > If we end up using these objects directly I think it would be nice to have
> > > the example code block from the platform specific headers documentation here
> > > too.
> > >
> > > > + *
> > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> > > > + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> > > > + *
> > > > + * @type: The parameters block type (platform-specific)
> > > > + * @flags: A bitmask of block flags (platform-specific)
> > > > + * @size: Size (in bytes) of the parameters block, including this header
> > > > + */
> > > > +struct v4l2_params_block {
> > > > + __u16 type;
> > > > + __u16 flags;
> > > > + __u32 size;
> > > > +} __attribute__((aligned(8)));
> > > > +
> > > > +/**
> > > > + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> > > > + *
> > > > + * This struct contains the configuration parameters of the ISP algorithms,
> > > > + * serialized by userspace into a data buffer. Each configuration parameter
> > > > + * block is represented by a block-specific structure which contains a
> > > > + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> > > > + * the @data buffer with configuration parameters for the blocks that it intends
> > > > + * to configure. As a consequence, the data buffer effective size changes
> > > > + * according to the number of ISP blocks that userspace intends to configure and
> > > > + * is set by userspace in the @data_size field.
> > > > + *
> > > > + * The parameters buffer is versioned by the @version field to allow modifying
> > > > + * and extending its definition. Userspace shall populate the @version field to
> > > > + * inform the driver about the version it intends to use. The driver will parse
> > > > + * and handle the @data buffer according to the data layout specific to the
> > > > + * indicated version and return an error if the desired version is not
> > > > + * supported.
> > > > + *
> > > > + * For each ISP block that userspace wants to configure, a block-specific
> > > > + * structure is appended to the @data buffer, one after the other without gaps
> > > > + * in between nor overlaps. Userspace shall populate the @data_size field with
> > > > + * the effective size, in bytes, of the @data buffer.
> > > > + *
> > > > + * Each ISP driver using the extensible parameters format shall define a
> > > > + * type which is type-convertible to this one, with the difference that the
> > > > + * @data member shall actually a memory buffer of platform-specific size and
> > > > + * not a pointer.
> > > Why not just use this object directly? We could provide a helper in
> > > v4l2-extensible-params.h that calculates the size of the buffer with a given
> > > data array size for the driver's convenience
> > The main reason I thought v4l2_params_buffer cannot be used is because
> > of the flexible-array at the end of the type
> >
> > struct v4l2_params_buffer {
> > __u32 version;
> > __u32 data_size;
> > __u8 data[];
> > };
> >
> > vs
> >
> > struct rkisp1_ext_params_cfg {
> > __u32 version;
> > __u32 data_size;
> > __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
> > };
> >
> > I might have missed what you're suggesting here with the helper in
> > v4l2-extensible-params.h :)
>
> So I think a known size is needed to accomodate operations like "memcpy(dst,
> src, sizeof(rkisp1_ext_params_cfg))", but with something like...
>
>
> #define v4l2_params_buffer_size(max_params_size) \
>
> (offsetof(struct v4l2_params_buffer, data) + max_params_size)
>
>
> then the above operation can be memcpy(dst,
> src, v4l2_params_buffer_size(RKISP1_EXT_PARAMS_MAX_SIZE)) instead
Fine for drivers indeed, my thinking was that we would need to reserve
space for userspace to write configuration blocks in...
However, buffers are allocated by videobuf2 and We could certainly
allocate the appropriate size using the vb2 queue 'buf_struct_size' member...
I can experiment with that indeed, in the meantime I wonder what
maintainers think about this ;)
Thanks
j
>
>
> Unless I'm missing something that should be enough to drop the driver
> specific struct...it seems to work ok anyway
>
>
> Dan
>
> >
> >
> > >
> > > Thanks
> > >
> > > Dan
> > >
> > > > + *
> > > > + * Userspace shall never use this type directly but use the platform specific
> > > > + * one with the associated data types.
> > > > + *
> > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> > > > + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> > > > + *
> > > > + * @version: The parameters buffer version (platform-specific)
> > > > + * @data_size: The configuration data effective size, excluding this header
> > > > + * @data: The configuration data
> > > > + */
> > > > +struct v4l2_params_buffer {
> > > > + __u32 version;
> > > > + __u32 data_size;
> > > > + __u8 data[];
> > > > +};
> > > > +
> > > > +#endif /* _UAPI_V4L2_PARAMS_H_ */
> > > >
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-10 7:15 ` Jacopo Mondi
@ 2025-07-10 9:10 ` Jacopo Mondi
0 siblings, 0 replies; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-10 9:10 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Dan Scally, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Dan
one correction
On Thu, Jul 10, 2025 at 09:15:54AM +0200, Jacopo Mondi wrote:
> Hi Dan
>
> On Wed, Jul 09, 2025 at 02:18:07PM +0100, Dan Scally wrote:
> > Hi Jacopo
> >
> > On 09/07/2025 12:53, Jacopo Mondi wrote:
> > > Hi Dan,
> > > thanks for the comments
> > >
> > > On Wed, Jul 09, 2025 at 12:33:17PM +0100, Dan Scally wrote:
> > > > Hi Jacopo - thanks for the patches
> > > >
> > > > On 08/07/2025 11:40, Jacopo Mondi wrote:
> > > > > Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
> > > > >
> > > > > The header defines two types that all drivers that use the extensible
> > > > > parameters format for ISP configuration shall use to build their own
> > > > > parameters format.
> > > > >
> > > > > The newly introduce type v4l2_params_block represent the
> > > > > header to be prepend to each ISP configuration block and the
> > > > > v4l2_params_buffer type represent the base type for the configuration
> > > > > parameters buffer.
> > > > >
> > > > > The newly introduced header is not meant to be used directly by
> > > > > applications which should instead use the platform-specific ones.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > ---
> > > > > MAINTAINERS | 6 ++
> > > > > include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> > > > > 2 files changed, 112 insertions(+)
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
> > > > > F: drivers/media/i2c/vd56g3.c
> > > > > F: drivers/media/i2c/vgxy61.c
> > > > > +V4L2 EXTENSIBLE PARAMETERS FORMAT
> > > > > +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > +L: linux-media@vger.kernel.org
> > > > > +S: Maintained
> > > > > +F: include/uapi/linux/media/v4l2-extensible-params.h
> > > > > +
> > > > > VF610 NAND DRIVER
> > > > > M: Stefan Agner <stefan@agner.ch>
> > > > > L: linux-mtd@lists.infradead.org
> > > > > diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > > new file mode 100644
> > > > > index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> > > > > --- /dev/null
> > > > > +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > > @@ -0,0 +1,106 @@
> > > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> > > > > +/*
> > > > > + * Video4Linux2 extensible configuration parameters base types
> > > > > + *
> > > > > + * Copyright (C) 2025 Ideas On Board Oy
> > > > > + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > + */
> > > > > +
> > > > > +#ifndef _UAPI_V4L2_PARAMS_H_
> > > > > +#define _UAPI_V4L2_PARAMS_H_
> > > > > +
> > > > > +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > > > > +/*
> > > > > + * Note: each ISP driver exposes a different uAPI, where the types layout
> > > > > + * match (more or less strictly) the hardware registers layout.
> > > > > + *
> > > > > + * This file defines the base types on which each ISP driver can implement its
> > > > > + * own types that define its uAPI.
> > > > > + *
> > > > > + * This file is not meant to be included directly by applications which shall
> > > > > + * instead only include the ISP-specific implementation.
> > > > > + */
> > > > > +#error "This file should not be included directly by applications"
> > > > > +#endif
> > > > > +
> > > > > +#include <linux/types.h>
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_params_block - V4L2 extensible parameters block header
> > > > struct v4l2_params_block_header would be nicer I think
> > > >
> > > That's what I had started with :)
> > >
> > > I'm debated between a longer but more explicative name, or a shorter
> > > one.
> >
> >
> > I vote for longer here but only because I think the phrase "block" applies
> > more properly to the likes of struct rkisp1_ext_params_bls_config for
> > example.
>
> Thanks, I've seen you made a patch to change this!
>
> >
> > >
> > > > > + *
> > > > > + * This structure represents the common part of all the ISP configuration
> > > > > + * blocks. Each parameters block shall embed an instance of this structure type
> > > > > + * as its first member, followed by the block-specific configuration data. The
> > > > > + * driver inspects this common header to discern the block type and its size and
> > > > > + * properly handle the block content by casting it to the correct block-specific
> > > > > + * type.
> > > > > + *
> > > > > + * The @type field is one of the values enumerated by each platform-specific ISP
> > > > > + * block types which specifies how the data should be interpreted by the driver.
> > > > > + * The @size field specifies the size of the parameters block and is used by the
> > > > > + * driver for validation purposes.
> > > > > + *
> > > > > + * The @flags field is a bitmask of platform-specific control flags.
> > > > Since we're including flags in this base struct rather than a platform
> > > > specific subclass I think perhaps we should centralise some flags (which I
> > > > think is supported by the fact that all three implementations share the same
> > > > flags so far). Perhaps we could reserve the bottom 8 bits for common flags
> > > > (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for
> > > > platform specific flags. I think we could then drop the platform specific
> > > > validation for rkisp1 and c3 and just pass null to the helpers, since they
> > > > do the same thing.
> > > Yes, that's one of the things I was not sure about... if we should
> > > centralize flags definition as well or not...
> >
> >
> > I think probably the ability to have both centralised and platform specific ones would be worthwhile
> >
> > >
> > > Knowing that Mali will use the same flags that the two existing
> > > implementations already have is a good indication that we can probably
> > > centralize at least the ENABLE/DISABLE ones
> >
> >
> > Yeah
> >
> > >
> > > > > + *
> > > > > + * Userspace shall never use this type directly but use the platform specific
> > > > > + * one with the associated data types.
> > > > Why wouldn't userspace just use these directly? I could see why it might be
> > > > difficult for the C3 and Rkisp1 which are merged, but for a new
> > > > implementation couldn't they just use these objects without bothering to
> > > > define their own?
> > > >
> > > mmm, my thinking was that each driver implementation shall define
> > > their own types because I would expect that they will have to define
> > > their own meta image format... For v4l2_params_buffer see below, for
> > > the blocks it might be totally possible to use these type most
> > > probably..
> > >
> > > > If we end up using these objects directly I think it would be nice to have
> > > > the example code block from the platform specific headers documentation here
> > > > too.
> > > >
> > > > > + *
> > > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> > > > > + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> > > > > + *
> > > > > + * @type: The parameters block type (platform-specific)
> > > > > + * @flags: A bitmask of block flags (platform-specific)
> > > > > + * @size: Size (in bytes) of the parameters block, including this header
> > > > > + */
> > > > > +struct v4l2_params_block {
> > > > > + __u16 type;
> > > > > + __u16 flags;
> > > > > + __u32 size;
> > > > > +} __attribute__((aligned(8)));
> > > > > +
> > > > > +/**
> > > > > + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> > > > > + *
> > > > > + * This struct contains the configuration parameters of the ISP algorithms,
> > > > > + * serialized by userspace into a data buffer. Each configuration parameter
> > > > > + * block is represented by a block-specific structure which contains a
> > > > > + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> > > > > + * the @data buffer with configuration parameters for the blocks that it intends
> > > > > + * to configure. As a consequence, the data buffer effective size changes
> > > > > + * according to the number of ISP blocks that userspace intends to configure and
> > > > > + * is set by userspace in the @data_size field.
> > > > > + *
> > > > > + * The parameters buffer is versioned by the @version field to allow modifying
> > > > > + * and extending its definition. Userspace shall populate the @version field to
> > > > > + * inform the driver about the version it intends to use. The driver will parse
> > > > > + * and handle the @data buffer according to the data layout specific to the
> > > > > + * indicated version and return an error if the desired version is not
> > > > > + * supported.
> > > > > + *
> > > > > + * For each ISP block that userspace wants to configure, a block-specific
> > > > > + * structure is appended to the @data buffer, one after the other without gaps
> > > > > + * in between nor overlaps. Userspace shall populate the @data_size field with
> > > > > + * the effective size, in bytes, of the @data buffer.
> > > > > + *
> > > > > + * Each ISP driver using the extensible parameters format shall define a
> > > > > + * type which is type-convertible to this one, with the difference that the
> > > > > + * @data member shall actually a memory buffer of platform-specific size and
> > > > > + * not a pointer.
> > > > Why not just use this object directly? We could provide a helper in
> > > > v4l2-extensible-params.h that calculates the size of the buffer with a given
> > > > data array size for the driver's convenience
> > > The main reason I thought v4l2_params_buffer cannot be used is because
> > > of the flexible-array at the end of the type
> > >
> > > struct v4l2_params_buffer {
> > > __u32 version;
> > > __u32 data_size;
> > > __u8 data[];
> > > };
> > >
> > > vs
> > >
> > > struct rkisp1_ext_params_cfg {
> > > __u32 version;
> > > __u32 data_size;
> > > __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
> > > };
> > >
> > > I might have missed what you're suggesting here with the helper in
> > > v4l2-extensible-params.h :)
> >
> > So I think a known size is needed to accomodate operations like "memcpy(dst,
> > src, sizeof(rkisp1_ext_params_cfg))", but with something like...
> >
> >
> > #define v4l2_params_buffer_size(max_params_size) \
> >
> > (offsetof(struct v4l2_params_buffer, data) + max_params_size)
> >
> >
> > then the above operation can be memcpy(dst,
> > src, v4l2_params_buffer_size(RKISP1_EXT_PARAMS_MAX_SIZE)) instead
>
> Fine for drivers indeed, my thinking was that we would need to reserve
> space for userspace to write configuration blocks in...
>
> However, buffers are allocated by videobuf2 and We could certainly
> allocate the appropriate size using the vb2 queue 'buf_struct_size' member...
I was cearly confused. What I should have suggested is using the
queue_setup() vb2 op to specify the desired plane sizes.
>
> I can experiment with that indeed, in the meantime I wonder what
> maintainers think about this ;)
>
> Thanks
> j
>
> >
> >
> > Unless I'm missing something that should be enough to drop the driver
> > specific struct...it seems to work ok anyway
> >
> >
> > Dan
> >
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > > > Dan
> > > >
> > > > > + *
> > > > > + * Userspace shall never use this type directly but use the platform specific
> > > > > + * one with the associated data types.
> > > > > + *
> > > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> > > > > + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> > > > > + *
> > > > > + * @version: The parameters buffer version (platform-specific)
> > > > > + * @data_size: The configuration data effective size, excluding this header
> > > > > + * @data: The configuration data
> > > > > + */
> > > > > +struct v4l2_params_buffer {
> > > > > + __u32 version;
> > > > > + __u32 data_size;
> > > > > + __u8 data[];
> > > > > +};
> > > > > +
> > > > > +#endif /* _UAPI_V4L2_PARAMS_H_ */
> > > > >
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params
2025-07-08 10:40 ` [PATCH 1/8] media: uapi: Introduce V4L2 extensible params Jacopo Mondi
2025-07-09 5:59 ` kernel test robot
2025-07-09 11:33 ` Dan Scally
@ 2025-07-09 18:59 ` Nicolas Dufresne
2 siblings, 0 replies; 27+ messages in thread
From: Nicolas Dufresne @ 2025-07-09 18:59 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 6765 bytes --]
Le mardi 08 juillet 2025 à 12:40 +0200, Jacopo Mondi a écrit :
> Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
>
> The header defines two types that all drivers that use the extensible
> parameters format for ISP configuration shall use to build their own
> parameters format.
>
> The newly introduce type v4l2_params_block represent the
> header to be prepend to each ISP configuration block and the
> v4l2_params_buffer type represent the base type for the configuration
> parameters buffer.
>
> The newly introduced header is not meant to be used directly by
> applications which should instead use the platform-specific ones.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> MAINTAINERS | 6 ++
> include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> 2 files changed, 112 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25968,6 +25968,12 @@ F: drivers/media/i2c/vd55g1.c
> F: drivers/media/i2c/vd56g3.c
> F: drivers/media/i2c/vgxy61.c
>
> +V4L2 EXTENSIBLE PARAMETERS FORMAT
> +M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +F: include/uapi/linux/media/v4l2-extensible-params.h
> +
> VF610 NAND DRIVER
> M: Stefan Agner <stefan@agner.ch>
> L: linux-mtd@lists.infradead.org
> diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> --- /dev/null
> +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> +/*
> + * Video4Linux2 extensible configuration parameters base types
> + *
> + * Copyright (C) 2025 Ideas On Board Oy
> + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> + */
> +
> +#ifndef _UAPI_V4L2_PARAMS_H_
> +#define _UAPI_V4L2_PARAMS_H_
> +
> +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> +/*
> + * Note: each ISP driver exposes a different uAPI, where the types layout
> + * match (more or less strictly) the hardware registers layout.
> + *
> + * This file defines the base types on which each ISP driver can implement its
> + * own types that define its uAPI.
> + *
> + * This file is not meant to be included directly by applications which shall
> + * instead only include the ISP-specific implementation.
> + */
> +#error "This file should not be included directly by applications"
> +#endif
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct v4l2_params_block - V4L2 extensible parameters block header
> + *
> + * This structure represents the common part of all the ISP configuration
> + * blocks. Each parameters block shall embed an instance of this structure type
> + * as its first member, followed by the block-specific configuration data. The
> + * driver inspects this common header to discern the block type and its size and
> + * properly handle the block content by casting it to the correct block-specific
> + * type.
> + *
> + * The @type field is one of the values enumerated by each platform-specific ISP
> + * block types which specifies how the data should be interpreted by the driver.
> + * The @size field specifies the size of the parameters block and is used by the
> + * driver for validation purposes.
> + *
> + * The @flags field is a bitmask of platform-specific control flags.
> + *
> + * Userspace shall never use this type directly but use the platform specific
> + * one with the associated data types.
> + *
> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> + *
> + * @type: The parameters block type (platform-specific)
> + * @flags: A bitmask of block flags (platform-specific)
> + * @size: Size (in bytes) of the parameters block, including this header
> + */
> +struct v4l2_params_block {
> + __u16 type;
> + __u16 flags;
> + __u32 size;
> +} __attribute__((aligned(8)));
> +
> +/**
> + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> + *
> + * This struct contains the configuration parameters of the ISP algorithms,
> + * serialized by userspace into a data buffer. Each configuration parameter
> + * block is represented by a block-specific structure which contains a
> + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> + * the @data buffer with configuration parameters for the blocks that it intends
> + * to configure. As a consequence, the data buffer effective size changes
> + * according to the number of ISP blocks that userspace intends to configure and
> + * is set by userspace in the @data_size field.
> + *
> + * The parameters buffer is versioned by the @version field to allow modifying
> + * and extending its definition. Userspace shall populate the @version field to
> + * inform the driver about the version it intends to use. The driver will parse
> + * and handle the @data buffer according to the data layout specific to the
> + * indicated version and return an error if the desired version is not
> + * supported.
> + *
> + * For each ISP block that userspace wants to configure, a block-specific
> + * structure is appended to the @data buffer, one after the other without gaps
> + * in between nor overlaps. Userspace shall populate the @data_size field with
> + * the effective size, in bytes, of the @data buffer.
> + *
> + * Each ISP driver using the extensible parameters format shall define a
> + * type which is type-convertible to this one, with the difference that the
> + * @data member shall actually a memory buffer of platform-specific size and
There is some word(s) missing here, "shall actually <> a memory buffer".
Nicolas
>
> + * not a pointer.
> + *
> + * Userspace shall never use this type directly but use the platform specific
> + * one with the associated data types.
> + *
> + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> + *
> + * @version: The parameters buffer version (platform-specific)
> + * @data_size: The configuration data effective size, excluding this header
> + * @data: The configuration data
> + */
> +struct v4l2_params_buffer {
> + __u32 version;
> + __u32 data_size;
> + __u8 data[];
> +};
> +
> +#endif /* _UAPI_V4L2_PARAMS_H_ */
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 2/8] media: uapi: Convert RkISP1 to V4L2 extensible params
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
2025-07-08 10:40 ` [PATCH 1/8] media: uapi: Introduce V4L2 extensible params Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-09 10:22 ` kernel test robot
2025-07-08 10:40 ` [PATCH 3/8] media: uapi: Convert Amlogic C3 " Jacopo Mondi
` (6 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
With the introduction of common types for extensible parameters
format, convert the rkisp1-config.h header to use the new types.
Factor-out the documentation that is now part of the common header
and only keep the driver-specific on in place.
The conversion to use common types doesn't impact userspace as the
new types are either identical to the ones already existing in the
RkISP1 uAPI or are 1-to-1 type convertible.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
include/uapi/linux/rkisp1-config.h | 60 ++++++++++++--------------------------
1 file changed, 18 insertions(+), 42 deletions(-)
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 3b060ea6eed71b87d79abc8401eae4e9c9f5323a..fe58ff1aed15b0497a9a9fa9a0bfa1ede3889d3e 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -7,8 +7,12 @@
#ifndef _UAPI_RKISP1_CONFIG_H
#define _UAPI_RKISP1_CONFIG_H
+#include <linux/build_bug.h>
#include <linux/types.h>
+#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
+#include <linux/media/v4l2-extensible-params.h>
+
/* Defect Pixel Cluster Detection */
#define RKISP1_CIF_ISP_MODULE_DPCC (1U << 0)
/* Black Level Subtraction */
@@ -1165,19 +1169,14 @@ enum rkisp1_ext_params_block_type {
#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS (V4L2_CID_USER_RKISP1_BASE + 0x01)
/**
- * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
- * header
+ * rkisp1_ext_params_block_header - RkISP1 extensible parameters block header
*
* This structure represents the common part of all the ISP configuration
- * blocks. Each parameters block shall embed an instance of this structure type
- * as its first member, followed by the block-specific configuration data. The
- * driver inspects this common header to discern the block type and its size and
- * properly handle the block content by casting it to the correct block-specific
- * type.
+ * blocks and is identical to :c:type:`v4l2_params_block`.
*
- * The @type field is one of the values enumerated by
+ * The type field is one of the values enumerated by
* :c:type:`rkisp1_ext_params_block_type` and specifies how the data should be
- * interpreted by the driver. The @size field specifies the size of the
+ * interpreted by the driver. The size field specifies the size of the
* parameters block and is used by the driver for validation purposes.
*
* The @flags field is a bitmask of per-block flags RKISP1_EXT_PARAMS_FL_*.
@@ -1193,14 +1192,14 @@ enum rkisp1_ext_params_block_type {
* If a new configuration of an ISP block has to be applied userspace shall
* fully populate the ISP block configuration and omit setting the
* RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE and RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE bits
- * in the @flags field.
+ * in the flags field.
*
* Setting both the RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE and
- * RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE bits in the @flags field is not allowed
+ * RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE bits in the flags field is not allowed
* and not accepted by the driver.
*
* Userspace is responsible for correctly populating the parameters block header
- * fields (@type, @flags and @size) and the block-specific parameters.
+ * fields (type, flags and size) and the block-specific parameters.
*
* For example:
*
@@ -1220,17 +1219,8 @@ enum rkisp1_ext_params_block_type {
* bls->config.fixed_val.gb = blackLevelGreenB_;
* bls->config.fixed_val.b = blackLevelBlue_;
* }
- *
- * @type: The parameters block type, see
- * :c:type:`rkisp1_ext_params_block_type`
- * @flags: A bitmask of block flags
- * @size: Size (in bytes) of the parameters block, including this header
*/
-struct rkisp1_ext_params_block_header {
- __u16 type;
- __u16 flags;
- __u32 size;
-};
+#define rkisp1_ext_params_block_header v4l2_params_block
/**
* struct rkisp1_ext_params_bls_config - RkISP1 extensible params BLS config
@@ -1594,21 +1584,7 @@ enum rksip1_ext_param_buffer_version {
/**
* struct rkisp1_ext_params_cfg - RkISP1 extensible parameters configuration
*
- * This struct contains the configuration parameters of the RkISP1 ISP
- * algorithms, serialized by userspace into a data buffer. Each configuration
- * parameter block is represented by a block-specific structure which contains a
- * :c:type:`rkisp1_ext_params_block_header` entry as first member. Userspace
- * populates the @data buffer with configuration parameters for the blocks that
- * it intends to configure. As a consequence, the data buffer effective size
- * changes according to the number of ISP blocks that userspace intends to
- * configure and is set by userspace in the @data_size field.
- *
- * The parameters buffer is versioned by the @version field to allow modifying
- * and extending its definition. Userspace shall populate the @version field to
- * inform the driver about the version it intends to use. The driver will parse
- * and handle the @data buffer according to the data layout specific to the
- * indicated version and return an error if the desired version is not
- * supported.
+ * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
*
* Currently the single RKISP1_EXT_PARAM_BUFFER_V1 version is supported.
* When a new format version will be added, a mechanism for userspace to query
@@ -1624,11 +1600,6 @@ enum rksip1_ext_param_buffer_version {
* the maximum value represents the blocks supported by the kernel driver,
* independently of the device instance.
*
- * For each ISP block that userspace wants to configure, a block-specific
- * structure is appended to the @data buffer, one after the other without gaps
- * in between nor overlaps. Userspace shall populate the @data_size field with
- * the effective size, in bytes, of the @data buffer.
- *
* The expected memory layout of the parameters buffer is::
*
* +-------------------- struct rkisp1_ext_params_cfg -------------------+
@@ -1678,4 +1649,9 @@ struct rkisp1_ext_params_cfg {
__u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
};
+/* Make sure the header is type-convertible to the generic v4l2 params one */
+static_assert((sizeof(struct rkisp1_ext_params_cfg) -
+ RKISP1_EXT_PARAMS_MAX_SIZE) ==
+ sizeof(struct v4l2_params_buffer));
+
#endif /* _UAPI_RKISP1_CONFIG_H */
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 2/8] media: uapi: Convert RkISP1 to V4L2 extensible params
2025-07-08 10:40 ` [PATCH 2/8] media: uapi: Convert RkISP1 to " Jacopo Mondi
@ 2025-07-09 10:22 ` kernel test robot
0 siblings, 0 replies; 27+ messages in thread
From: kernel test robot @ 2025-07-09 10:22 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: llvm, oe-kbuild-all, linux-media, linux-kernel, linux-rockchip,
linux-arm-kernel, Jacopo Mondi
Hi Jacopo,
kernel test robot noticed the following build errors:
[auto build test ERROR on a8598c7de1bcd94461ca54c972efa9b4ea501fb9]
url: https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-uapi-Introduce-V4L2-extensible-params/20250708-184651
base: a8598c7de1bcd94461ca54c972efa9b4ea501fb9
patch link: https://lore.kernel.org/r/20250708-extensible-parameters-validation-v1-2-9fc27c9c728c%40ideasonboard.com
patch subject: [PATCH 2/8] media: uapi: Convert RkISP1 to V4L2 extensible params
config: i386-buildonly-randconfig-003-20250709 (https://download.01.org/0day-ci/archive/20250709/202507091859.x8Yx8AZb-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250709/202507091859.x8Yx8AZb-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507091859.x8Yx8AZb-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <built-in>:1:
>> ./usr/include/linux/rkisp1-config.h:10:10: fatal error: 'linux/build_bug.h' file not found
10 | #include <linux/build_bug.h>
| ^~~~~~~~~~~~~~~~~~~
1 error generated.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible params
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
2025-07-08 10:40 ` [PATCH 1/8] media: uapi: Introduce V4L2 extensible params Jacopo Mondi
2025-07-08 10:40 ` [PATCH 2/8] media: uapi: Convert RkISP1 to " Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-10 2:32 ` Keke Li
2025-07-08 10:40 ` [PATCH 4/8] media: Documentation: uapi: Add V4L2 extensible parameters Jacopo Mondi
` (5 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
With the introduction of common types for extensible parameters
format, convert the c3-isp-config.h header to use the new types.
Factor-out the documentation that is now part of the common header
and only keep the driver-specific on in place.
The conversion to use common types doesn't impact userspace as the
new types are either identical to the ones already existing in the
C3 ISP uAPI or are 1-to-1 type convertible.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +++++++-----------------
1 file changed, 12 insertions(+), 33 deletions(-)
diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
index ed085ea62a574932c7ad8d59d34b2c5c74a597d8..203116cdfb89356301c16c98cb40e5b83efe71d6 100644
--- a/include/uapi/linux/media/amlogic/c3-isp-config.h
+++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
@@ -6,8 +6,12 @@
#ifndef _UAPI_C3_ISP_CONFIG_H_
#define _UAPI_C3_ISP_CONFIG_H_
+#include <linux/build_bug.h>
#include <linux/types.h>
+#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
+#include <linux/media/v4l2-extensible-params.h>
+
/*
* Frames are split into zones of almost equal width and height - a zone is a
* rectangular tile of a frame. The metering blocks within the ISP collect
@@ -183,11 +187,6 @@ enum c3_isp_params_block_type {
* struct c3_isp_params_block_header - C3 ISP parameter block header
*
* This structure represents the common part of all the ISP configuration
- * blocks. Each parameters block shall embed an instance of this structure type
- * as its first member, followed by the block-specific configuration data. The
- * driver inspects this common header to discern the block type and its size and
- * properly handle the block content by casting it to the correct block-specific
- * type.
*
* The @type field is one of the values enumerated by
* :c:type:`c3_isp_params_block_type` and specifies how the data should be
@@ -223,15 +222,8 @@ enum c3_isp_params_block_type {
* gamma->pst_gamma_lut[i] = i;
* }
*
- * @type: The parameters block type from :c:type:`c3_isp_params_block_type`
- * @flags: A bitmask of block flags
- * @size: Size (in bytes) of the parameters block, including this header
*/
-struct c3_isp_params_block_header {
- __u16 type;
- __u16 flags;
- __u32 size;
-};
+#define c3_isp_params_block_header v4l2_params_block
/**
* struct c3_isp_params_awb_gains - Gains for auto-white balance
@@ -498,26 +490,9 @@ struct c3_isp_params_blc {
/**
* struct c3_isp_params_cfg - C3 ISP configuration parameters
*
- * This struct contains the configuration parameters of the C3 ISP
- * algorithms, serialized by userspace into an opaque data buffer. Each
- * configuration parameter block is represented by a block-specific structure
- * which contains a :c:type:`c3_isp_param_block_header` entry as first
- * member. Userspace populates the @data buffer with configuration parameters
- * for the blocks that it intends to configure. As a consequence, the data
- * buffer effective size changes according to the number of ISP blocks that
- * userspace intends to configure.
- *
- * The parameters buffer is versioned by the @version field to allow modifying
- * and extending its definition. Userspace should populate the @version field to
- * inform the driver about the version it intends to use. The driver will parse
- * and handle the @data buffer according to the data layout specific to the
- * indicated revision and return an error if the desired revision is not
- * supported.
- *
- * For each ISP block that userspace wants to configure, a block-specific
- * structure is appended to the @data buffer, one after the other without gaps
- * in between nor overlaps. Userspace shall populate the @total_size field with
- * the effective size, in bytes, of the @data buffer.
+ * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
+ *
+ * Currently only C3_ISP_PARAM_BUFFER_V0 is supported.
*
* The expected memory layout of the parameters buffer is::
*
@@ -561,4 +536,8 @@ struct c3_isp_params_cfg {
__u8 data[C3_ISP_PARAMS_MAX_SIZE];
};
+/* Make sure the header is type-convertible to the generic v4l2 params one */
+static_assert((sizeof(struct c3_isp_params_cfg) - C3_ISP_PARAMS_MAX_SIZE) ==
+ sizeof(struct v4l2_params_buffer));
+
#endif
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible params
2025-07-08 10:40 ` [PATCH 3/8] media: uapi: Convert Amlogic C3 " Jacopo Mondi
@ 2025-07-10 2:32 ` Keke Li
2025-07-10 6:44 ` Jacopo Mondi
0 siblings, 1 reply; 27+ messages in thread
From: Keke Li @ 2025-07-10 2:32 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 2025/7/8 18:40, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> With the introduction of common types for extensible parameters
> format, convert the c3-isp-config.h header to use the new types.
>
> Factor-out the documentation that is now part of the common header
> and only keep the driver-specific on in place.
>
> The conversion to use common types doesn't impact userspace as the
> new types are either identical to the ones already existing in the
> C3 ISP uAPI or are 1-to-1 type convertible.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +++++++-----------------
> 1 file changed, 12 insertions(+), 33 deletions(-)
>
> diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
> index ed085ea62a574932c7ad8d59d34b2c5c74a597d8..203116cdfb89356301c16c98cb40e5b83efe71d6 100644
> --- a/include/uapi/linux/media/amlogic/c3-isp-config.h
> +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
> @@ -6,8 +6,12 @@
> #ifndef _UAPI_C3_ISP_CONFIG_H_
> #define _UAPI_C3_ISP_CONFIG_H_
>
> +#include <linux/build_bug.h>
> #include <linux/types.h>
>
> +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> +#include <linux/media/v4l2-extensible-params.h>
> +
> /*
> * Frames are split into zones of almost equal width and height - a zone is a
> * rectangular tile of a frame. The metering blocks within the ISP collect
> @@ -183,11 +187,6 @@ enum c3_isp_params_block_type {
> * struct c3_isp_params_block_header - C3 ISP parameter block header
> *
> * This structure represents the common part of all the ISP configuration
> - * blocks. Each parameters block shall embed an instance of this structure type
> - * as its first member, followed by the block-specific configuration data. The
> - * driver inspects this common header to discern the block type and its size and
> - * properly handle the block content by casting it to the correct block-specific
> - * type.
> *
> * The @type field is one of the values enumerated by
> * :c:type:`c3_isp_params_block_type` and specifies how the data should be
> @@ -223,15 +222,8 @@ enum c3_isp_params_block_type {
> * gamma->pst_gamma_lut[i] = i;
> * }
> *
> - * @type: The parameters block type from :c:type:`c3_isp_params_block_type`
> - * @flags: A bitmask of block flags
> - * @size: Size (in bytes) of the parameters block, including this header
> */
> -struct c3_isp_params_block_header {
> - __u16 type;
> - __u16 flags;
> - __u32 size;
> -};
> +#define c3_isp_params_block_header v4l2_params_block
Why not use v4l2_params_block directly?
Thanks
Keke
>
> /**
> * struct c3_isp_params_awb_gains - Gains for auto-white balance
> @@ -498,26 +490,9 @@ struct c3_isp_params_blc {
> /**
> * struct c3_isp_params_cfg - C3 ISP configuration parameters
> *
> - * This struct contains the configuration parameters of the C3 ISP
> - * algorithms, serialized by userspace into an opaque data buffer. Each
> - * configuration parameter block is represented by a block-specific structure
> - * which contains a :c:type:`c3_isp_param_block_header` entry as first
> - * member. Userspace populates the @data buffer with configuration parameters
> - * for the blocks that it intends to configure. As a consequence, the data
> - * buffer effective size changes according to the number of ISP blocks that
> - * userspace intends to configure.
> - *
> - * The parameters buffer is versioned by the @version field to allow modifying
> - * and extending its definition. Userspace should populate the @version field to
> - * inform the driver about the version it intends to use. The driver will parse
> - * and handle the @data buffer according to the data layout specific to the
> - * indicated revision and return an error if the desired revision is not
> - * supported.
> - *
> - * For each ISP block that userspace wants to configure, a block-specific
> - * structure is appended to the @data buffer, one after the other without gaps
> - * in between nor overlaps. Userspace shall populate the @total_size field with
> - * the effective size, in bytes, of the @data buffer.
> + * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
> + *
> + * Currently only C3_ISP_PARAM_BUFFER_V0 is supported.
> *
> * The expected memory layout of the parameters buffer is::
> *
> @@ -561,4 +536,8 @@ struct c3_isp_params_cfg {
> __u8 data[C3_ISP_PARAMS_MAX_SIZE];
> };
>
> +/* Make sure the header is type-convertible to the generic v4l2 params one */
> +static_assert((sizeof(struct c3_isp_params_cfg) - C3_ISP_PARAMS_MAX_SIZE) ==
> + sizeof(struct v4l2_params_buffer));
> +
> #endif
>
> --
> 2.49.0
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible params
2025-07-10 2:32 ` Keke Li
@ 2025-07-10 6:44 ` Jacopo Mondi
2025-07-10 6:59 ` Keke Li
0 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-10 6:44 UTC (permalink / raw)
To: Keke Li
Cc: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus,
linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Keke
thanks for the comment and for testing
On Thu, Jul 10, 2025 at 10:32:01AM +0800, Keke Li wrote:
> Hi Jacopo
>
> On 2025/7/8 18:40, Jacopo Mondi wrote:
> > [ EXTERNAL EMAIL ]
> >
> > With the introduction of common types for extensible parameters
> > format, convert the c3-isp-config.h header to use the new types.
> >
> > Factor-out the documentation that is now part of the common header
> > and only keep the driver-specific on in place.
> >
> > The conversion to use common types doesn't impact userspace as the
> > new types are either identical to the ones already existing in the
> > C3 ISP uAPI or are 1-to-1 type convertible.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +++++++-----------------
> > 1 file changed, 12 insertions(+), 33 deletions(-)
> >
> > diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
> > index ed085ea62a574932c7ad8d59d34b2c5c74a597d8..203116cdfb89356301c16c98cb40e5b83efe71d6 100644
> > --- a/include/uapi/linux/media/amlogic/c3-isp-config.h
> > +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
> > @@ -6,8 +6,12 @@
> > #ifndef _UAPI_C3_ISP_CONFIG_H_
> > #define _UAPI_C3_ISP_CONFIG_H_
> >
> > +#include <linux/build_bug.h>
> > #include <linux/types.h>
> >
> > +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > +#include <linux/media/v4l2-extensible-params.h>
> > +
> > /*
> > * Frames are split into zones of almost equal width and height - a zone is a
> > * rectangular tile of a frame. The metering blocks within the ISP collect
> > @@ -183,11 +187,6 @@ enum c3_isp_params_block_type {
> > * struct c3_isp_params_block_header - C3 ISP parameter block header
> > *
> > * This structure represents the common part of all the ISP configuration
> > - * blocks. Each parameters block shall embed an instance of this structure type
> > - * as its first member, followed by the block-specific configuration data. The
> > - * driver inspects this common header to discern the block type and its size and
> > - * properly handle the block content by casting it to the correct block-specific
> > - * type.
> > *
> > * The @type field is one of the values enumerated by
> > * :c:type:`c3_isp_params_block_type` and specifies how the data should be
> > @@ -223,15 +222,8 @@ enum c3_isp_params_block_type {
> > * gamma->pst_gamma_lut[i] = i;
> > * }
> > *
> > - * @type: The parameters block type from :c:type:`c3_isp_params_block_type`
> > - * @flags: A bitmask of block flags
> > - * @size: Size (in bytes) of the parameters block, including this header
> > */
> > -struct c3_isp_params_block_header {
> > - __u16 type;
> > - __u16 flags;
> > - __u32 size;
> > -};
> > +#define c3_isp_params_block_header v4l2_params_block
>
> Why not use v4l2_params_block directly?
The types of the Amlogic C3 and RkISP1 headers are already part of the
kernel userspace API and we can't change them without breaking
existing users I'm afraid. In the C3 case the kernel support has been
collected in the v6.16 merge window, and v6.16 has not been released yet,
so one could argue there are no "existing users" yet.
It's however too late in my opinion to make modifications that depend
on this patch series which will, be collected for v6.18 at the
earliest.
Thanks
j
>
> Thanks
>
> Keke
>
> >
> > /**
> > * struct c3_isp_params_awb_gains - Gains for auto-white balance
> > @@ -498,26 +490,9 @@ struct c3_isp_params_blc {
> > /**
> > * struct c3_isp_params_cfg - C3 ISP configuration parameters
> > *
> > - * This struct contains the configuration parameters of the C3 ISP
> > - * algorithms, serialized by userspace into an opaque data buffer. Each
> > - * configuration parameter block is represented by a block-specific structure
> > - * which contains a :c:type:`c3_isp_param_block_header` entry as first
> > - * member. Userspace populates the @data buffer with configuration parameters
> > - * for the blocks that it intends to configure. As a consequence, the data
> > - * buffer effective size changes according to the number of ISP blocks that
> > - * userspace intends to configure.
> > - *
> > - * The parameters buffer is versioned by the @version field to allow modifying
> > - * and extending its definition. Userspace should populate the @version field to
> > - * inform the driver about the version it intends to use. The driver will parse
> > - * and handle the @data buffer according to the data layout specific to the
> > - * indicated revision and return an error if the desired revision is not
> > - * supported.
> > - *
> > - * For each ISP block that userspace wants to configure, a block-specific
> > - * structure is appended to the @data buffer, one after the other without gaps
> > - * in between nor overlaps. Userspace shall populate the @total_size field with
> > - * the effective size, in bytes, of the @data buffer.
> > + * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
> > + *
> > + * Currently only C3_ISP_PARAM_BUFFER_V0 is supported.
> > *
> > * The expected memory layout of the parameters buffer is::
> > *
> > @@ -561,4 +536,8 @@ struct c3_isp_params_cfg {
> > __u8 data[C3_ISP_PARAMS_MAX_SIZE];
> > };
> >
> > +/* Make sure the header is type-convertible to the generic v4l2 params one */
> > +static_assert((sizeof(struct c3_isp_params_cfg) - C3_ISP_PARAMS_MAX_SIZE) ==
> > + sizeof(struct v4l2_params_buffer));
> > +
> > #endif
> >
> > --
> > 2.49.0
> >
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 3/8] media: uapi: Convert Amlogic C3 to V4L2 extensible params
2025-07-10 6:44 ` Jacopo Mondi
@ 2025-07-10 6:59 ` Keke Li
0 siblings, 0 replies; 27+ messages in thread
From: Keke Li @ 2025-07-10 6:59 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Dafna Hirschfeld, Laurent Pinchart, Mauro Carvalho Chehab,
Heiko Stuebner, Dan Scally, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 2025/7/10 14:44, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> Hi Keke
>
> thanks for the comment and for testing
>
> On Thu, Jul 10, 2025 at 10:32:01AM +0800, Keke Li wrote:
>> Hi Jacopo
>>
>> On 2025/7/8 18:40, Jacopo Mondi wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> With the introduction of common types for extensible parameters
>>> format, convert the c3-isp-config.h header to use the new types.
>>>
>>> Factor-out the documentation that is now part of the common header
>>> and only keep the driver-specific on in place.
>>>
>>> The conversion to use common types doesn't impact userspace as the
>>> new types are either identical to the ones already existing in the
>>> C3 ISP uAPI or are 1-to-1 type convertible.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Keke Li <keke.li@amlogic.com>
>>> ---
>>> include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +++++++-----------------
>>> 1 file changed, 12 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/include/uapi/linux/media/amlogic/c3-isp-config.h b/include/uapi/linux/media/amlogic/c3-isp-config.h
>>> index ed085ea62a574932c7ad8d59d34b2c5c74a597d8..203116cdfb89356301c16c98cb40e5b83efe71d6 100644
>>> --- a/include/uapi/linux/media/amlogic/c3-isp-config.h
>>> +++ b/include/uapi/linux/media/amlogic/c3-isp-config.h
>>> @@ -6,8 +6,12 @@
>>> #ifndef _UAPI_C3_ISP_CONFIG_H_
>>> #define _UAPI_C3_ISP_CONFIG_H_
>>>
>>> +#include <linux/build_bug.h>
>>> #include <linux/types.h>
>>>
>>> +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
>>> +#include <linux/media/v4l2-extensible-params.h>
>>> +
>>> /*
>>> * Frames are split into zones of almost equal width and height - a zone is a
>>> * rectangular tile of a frame. The metering blocks within the ISP collect
>>> @@ -183,11 +187,6 @@ enum c3_isp_params_block_type {
>>> * struct c3_isp_params_block_header - C3 ISP parameter block header
>>> *
>>> * This structure represents the common part of all the ISP configuration
>>> - * blocks. Each parameters block shall embed an instance of this structure type
>>> - * as its first member, followed by the block-specific configuration data. The
>>> - * driver inspects this common header to discern the block type and its size and
>>> - * properly handle the block content by casting it to the correct block-specific
>>> - * type.
>>> *
>>> * The @type field is one of the values enumerated by
>>> * :c:type:`c3_isp_params_block_type` and specifies how the data should be
>>> @@ -223,15 +222,8 @@ enum c3_isp_params_block_type {
>>> * gamma->pst_gamma_lut[i] = i;
>>> * }
>>> *
>>> - * @type: The parameters block type from :c:type:`c3_isp_params_block_type`
>>> - * @flags: A bitmask of block flags
>>> - * @size: Size (in bytes) of the parameters block, including this header
>>> */
>>> -struct c3_isp_params_block_header {
>>> - __u16 type;
>>> - __u16 flags;
>>> - __u32 size;
>>> -};
>>> +#define c3_isp_params_block_header v4l2_params_block
>> Why not use v4l2_params_block directly?
> The types of the Amlogic C3 and RkISP1 headers are already part of the
> kernel userspace API and we can't change them without breaking
> existing users I'm afraid. In the C3 case the kernel support has been
> collected in the v6.16 merge window, and v6.16 has not been released yet,
> so one could argue there are no "existing users" yet.
>
> It's however too late in my opinion to make modifications that depend
> on this patch series which will, be collected for v6.18 at the
> earliest.
OK
> Thanks
> j
>
>> Thanks
>>
>> Keke
>>
>>> /**
>>> * struct c3_isp_params_awb_gains - Gains for auto-white balance
>>> @@ -498,26 +490,9 @@ struct c3_isp_params_blc {
>>> /**
>>> * struct c3_isp_params_cfg - C3 ISP configuration parameters
>>> *
>>> - * This struct contains the configuration parameters of the C3 ISP
>>> - * algorithms, serialized by userspace into an opaque data buffer. Each
>>> - * configuration parameter block is represented by a block-specific structure
>>> - * which contains a :c:type:`c3_isp_param_block_header` entry as first
>>> - * member. Userspace populates the @data buffer with configuration parameters
>>> - * for the blocks that it intends to configure. As a consequence, the data
>>> - * buffer effective size changes according to the number of ISP blocks that
>>> - * userspace intends to configure.
>>> - *
>>> - * The parameters buffer is versioned by the @version field to allow modifying
>>> - * and extending its definition. Userspace should populate the @version field to
>>> - * inform the driver about the version it intends to use. The driver will parse
>>> - * and handle the @data buffer according to the data layout specific to the
>>> - * indicated revision and return an error if the desired revision is not
>>> - * supported.
>>> - *
>>> - * For each ISP block that userspace wants to configure, a block-specific
>>> - * structure is appended to the @data buffer, one after the other without gaps
>>> - * in between nor overlaps. Userspace shall populate the @total_size field with
>>> - * the effective size, in bytes, of the @data buffer.
>>> + * This is the driver-specific implementation of :c:type:`v4l2_params_buffer`.
>>> + *
>>> + * Currently only C3_ISP_PARAM_BUFFER_V0 is supported.
>>> *
>>> * The expected memory layout of the parameters buffer is::
>>> *
>>> @@ -561,4 +536,8 @@ struct c3_isp_params_cfg {
>>> __u8 data[C3_ISP_PARAMS_MAX_SIZE];
>>> };
>>>
>>> +/* Make sure the header is type-convertible to the generic v4l2 params one */
>>> +static_assert((sizeof(struct c3_isp_params_cfg) - C3_ISP_PARAMS_MAX_SIZE) ==
>>> + sizeof(struct v4l2_params_buffer));
>>> +
>>> #endif
>>>
>>> --
>>> 2.49.0
>>>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 4/8] media: Documentation: uapi: Add V4L2 extensible parameters
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (2 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 3/8] media: uapi: Convert Amlogic C3 " Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-08 10:40 ` [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c Jacopo Mondi
` (4 subsequent siblings)
8 siblings, 0 replies; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Add documentation for extensible parameters format to the V4L2
userspace API documentation.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
.../media/v4l/extensible-parameters.rst | 89 ++++++++++++++++++++++
.../userspace-api/media/v4l/meta-formats.rst | 1 +
MAINTAINERS | 1 +
3 files changed, 91 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/extensible-parameters.rst b/Documentation/userspace-api/media/v4l/extensible-parameters.rst
new file mode 100644
index 0000000000000000000000000000000000000000..254d4087ae0448d3e545d8533c36d154967e202a
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/extensible-parameters.rst
@@ -0,0 +1,89 @@
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+
+.. _extensible-parameters:
+
+**********************************
+ V4L2 extensible parameters format
+**********************************
+
+ISP configuration
+=================
+
+ISP configuration parameters are computed by userspace and programmed into a
+*parameters buffer* which is queued to the ISP driver on a per-frame basis. The
+layout of the *parameters buffer* generally reflects the ISP peripheral
+registers layout and is, for this reason, platform specific.
+
+The ISP configuration parameters are passed to the ISP driver through a metadata
+output video node, using the :c:type:`v4l2_meta_format` interface. Each ISP
+driver defines a metadata format that implements the configuration parameters
+layout.
+
+Metadata output formats that describe ISP configuration parameters are most of
+the time realized by implementing C structures that reflect the registers layout
+and gets populated by userspace before queueing the buffer to the ISP. Each
+C structure usually corresponds to one ISP *processing block*, with each block
+implementing one of the ISP supported features.
+
+The uAPI/ABI problem
+--------------------
+
+By defining a metadata output format that described the configuration parameters
+layout, driver developers make them part of the Linux kernel ABI. As it
+sometimes happens for most peripherals in Linux, ISP drivers development is
+often an iterative process, where sometimes not all the hardware features are
+supported in the very first driver version that lands in the kernel, and some
+parts of the interface have to later be modified for bug-fixes or improvements.
+
+If any later bug-fix/improvement requires changes to the metadata output format,
+this is considered an ABI-breakage that is strictly forbidden by the Linux
+kernel policies. For this reason, each new iteration of an ISP driver support
+would require defining a new metadata output format, implying drivers have to be
+made ready to handle several different formats.
+
+A new set of metadata output formats has then to be defined, with the design
+goals of being:
+
+- Extensible: new features can be added later on without breaking the existing
+ interface
+- Versioned: different versions of the interface can be defined without
+ breaking the existing interface
+
+The extensible parameters format
+================================
+
+Extensible configuration formats are realized by a defining a single C structure
+that contains a few control parameters and a binary buffer where userspace
+programs a variable number of *ISP block configuration* data.
+
+The generic :c:type:`v4l2_params_buffer` defines a base type that each driver
+shall extend with a type-convertible implementation
+
+Each *ISP block configuration* is identified by an header and contains the
+parameters for that specific block.
+
+The generic :c:type:`v4l2_params_block` defines a base type that each driver can
+re-use as it is or extend appropriately.
+
+Userspace applications program in the control buffer only the parameters of the
+ISP whose configuration has changed for the next frame. The ISP driver parses
+the configuration parameters and apply them to the hardware register.
+
+Any further development that happens after the ISP driver has been merged in
+Linux and which requires supporting new ISP features can be implemented by
+adding new block definitions without invalidating the existing ones. Similarly,
+any change to the existing ISP configuration blocks can be handled by versioning
+them, again without invalidating the existing ones.
+
+Implementations
+---------------
+
+ISP drivers that define an extensible parameters metadata output format:
+
+- :ref:`RkISP1 <v4l2-meta-fmt-rk-isp1-ext-params>`
+- :ref:`Amlogic C3 ISP <v4l2-meta-fmt-c3isp-params>`
+
+V4L2 extensible parameters uAPI data types
+==========================================
+
+.. kernel-doc:: include/uapi/linux/media/v4l2-extensible-params.h
diff --git a/Documentation/userspace-api/media/v4l/meta-formats.rst b/Documentation/userspace-api/media/v4l/meta-formats.rst
index bb6876cfc271e1a0543eee4209d6251e1a6a73cc..58eb3c9c962bee008eee27d9c16678213c47baa9 100644
--- a/Documentation/userspace-api/media/v4l/meta-formats.rst
+++ b/Documentation/userspace-api/media/v4l/meta-formats.rst
@@ -12,6 +12,7 @@ These formats are used for the :ref:`metadata` interface only.
.. toctree::
:maxdepth: 1
+ extensible-parameters
metafmt-c3-isp
metafmt-d4xx
metafmt-generic
diff --git a/MAINTAINERS b/MAINTAINERS
index 49a9329e5fe8874bdbaca13946ea28bd80134cb3..beecac86991d988c48d31366ba5201b09ef25715 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25972,6 +25972,7 @@ V4L2 EXTENSIBLE PARAMETERS FORMAT
M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
L: linux-media@vger.kernel.org
S: Maintained
+F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
F: include/uapi/linux/media/v4l2-extensible-params.h
VF610 NAND DRIVER
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (3 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 4/8] media: Documentation: uapi: Add V4L2 extensible parameters Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-09 6:20 ` kernel test robot
2025-07-10 12:42 ` Dan Scally
2025-07-08 10:40 ` [PATCH 6/8] media: rkisp1: Use v4l2-params for validation Jacopo Mondi
` (3 subsequent siblings)
8 siblings, 2 replies; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Add to the v4l2 framework an helper function to support drivers
when validating a buffer of extensible parameters.
Introduce new types in include/media/v4l2-params.h that driver shall
use in order to comply with the v4l2-params validation procedure, and
add a single helper function to v4l2-params.c.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
MAINTAINERS | 2 +
drivers/media/v4l2-core/Makefile | 3 +-
drivers/media/v4l2-core/v4l2-params.c | 106 ++++++++++++++++++++++
include/media/v4l2-params.h | 166 ++++++++++++++++++++++++++++++++++
4 files changed, 276 insertions(+), 1 deletion(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index beecac86991d988c48d31366ba5201b09ef25715..3d9a8e06c59eb08360d1e8eea85e450a15ee95af 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25973,6 +25973,8 @@ M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
L: linux-media@vger.kernel.org
S: Maintained
F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
+F: drivers/media/v4l2-core/v4l2-params.c
+F: include/media/v4l2-params.h
F: include/uapi/linux/media/v4l2-extensible-params.h
VF610 NAND DRIVER
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 2177b9d63a8ffc1127c5a70118249a2ff63cd759..323330dd359f95c1ae3d0c35bd6fcb8291a33a07 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -11,7 +11,8 @@ tuner-objs := tuner-core.o
videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
v4l2-event.o v4l2-subdev.o v4l2-common.o \
v4l2-ctrls-core.o v4l2-ctrls-api.o \
- v4l2-ctrls-request.o v4l2-ctrls-defs.o
+ v4l2-ctrls-request.o v4l2-ctrls-defs.o \
+ v4l2-params.o
# Please keep it alphabetically sorted by Kconfig name
# (e. g. LC_ALL=C sort Makefile)
diff --git a/drivers/media/v4l2-core/v4l2-params.c b/drivers/media/v4l2-core/v4l2-params.c
new file mode 100644
index 0000000000000000000000000000000000000000..3fb320aec900ee4a05c595f2e14c6ee0d8710669
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-params.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Video4Linux2 extensible parameters helpers
+ *
+ * Copyright (C) 2025 Ideas On Board Oy
+ * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+ */
+
+#include <media/v4l2-params.h>
+
+int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
+ size_t max_size,
+ v4l2_params_validate_buffer buffer_validate)
+{
+ size_t header_size = offsetof(struct v4l2_params_buffer, data);
+ struct v4l2_params_buffer *buffer = vb2_plane_vaddr(vb, 0);
+ size_t payload_size = vb2_get_plane_payload(vb, 0);
+ size_t buffer_size;
+ int ret;
+
+ /* Payload size can't be greater than the destination buffer size */
+ if (payload_size > max_size) {
+ dev_dbg(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(dev, "Payload size is too small: %zu\n", payload_size);
+ return -EINVAL;
+ }
+
+ /* Validate the size reported in the parameter buffer header */
+ buffer_size = header_size + buffer->data_size;
+ if (buffer_size != payload_size) {
+ dev_dbg(dev, "Data size %zu and payload size %zu are different\n",
+ buffer_size, payload_size);
+ return -EINVAL;
+ }
+
+ /* Driver-specific buffer validation. */
+ if (buffer_validate) {
+ ret = buffer_validate(dev, buffer);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_params_buffer_validate);
+
+int v4l2_params_blocks_validate(struct device *dev,
+ const struct v4l2_params_buffer *buffer,
+ const struct v4l2_params_handler *handlers,
+ size_t num_handlers,
+ v4l2_params_validate_block block_validate)
+{
+ size_t block_offset = 0;
+ size_t buffer_size;
+ int ret;
+
+ /* Walk the list of parameter blocks and validate them. */
+ buffer_size = buffer->data_size;
+ while (buffer_size >= sizeof(struct v4l2_params_block)) {
+ const struct v4l2_params_handler *handler;
+ const struct v4l2_params_block *block;
+
+ /* Validate block sizes and types against the handlers. */
+ block = (const struct v4l2_params_block *)
+ (buffer->data + block_offset);
+
+ if (block->type >= num_handlers) {
+ dev_dbg(dev, "Invalid parameters block type\n");
+ return -EINVAL;
+ }
+
+ if (block->size > buffer_size) {
+ dev_dbg(dev, "Premature end of parameters data\n");
+ return -EINVAL;
+ }
+
+ handler = &handlers[block->type];
+ if (block->size != handler->size) {
+ dev_dbg(dev, "Invalid parameters block size\n");
+ return -EINVAL;
+ }
+
+ /* Driver-specific per-block validation. */
+ if (block_validate) {
+ ret = block_validate(dev, block);
+ if (ret)
+ return ret;
+ }
+
+ block_offset += block->size;
+ buffer_size -= block->size;
+ }
+
+ if (buffer_size) {
+ dev_dbg(dev, "Unexpected data after the parameters buffer end\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_params_blocks_validate);
diff --git a/include/media/v4l2-params.h b/include/media/v4l2-params.h
new file mode 100644
index 0000000000000000000000000000000000000000..55f08c646a943fef11eaeddff842fae00b8422d4
--- /dev/null
+++ b/include/media/v4l2-params.h
@@ -0,0 +1,166 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Video4Linux2 extensible parameters helpers
+ *
+ * Copyright (C) 2025 Ideas On Board Oy
+ * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
+ */
+
+#ifndef V4L2_PARAMS_H_
+#define V4L2_PARAMS_H_
+
+#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
+#include <linux/media/v4l2-extensible-params.h>
+
+#include <linux/device.h>
+
+#include <media/videobuf2-core.h>
+
+/**
+ * typedef v4l2_params_block_handler - V4L2 extensible format block handler
+ * @arg: pointer the driver-specific argument
+ * @block: the ISP configuration block to handle
+ *
+ * Defines the function signature of the functions that handle an ISP block
+ * configuration.
+ */
+typedef void (*v4l2_params_block_handler)(void *arg,
+ const struct v4l2_params_block *block);
+
+/**
+ * struct v4l2_params_handler - V4L2 extensible format handler
+ * @size: the block expected size
+ * @handler: the block handler function
+ * @group: the device-specific group id the block belongs to (optional)
+ * @features: the device-specific features flags (optional)
+ *
+ * The v4l2_params_handler defines the type that driver making use of the
+ * V4L2 extensible parameters shall use to define their own ISP block
+ * handlers.
+ *
+ * Drivers shall prepare a list of handlers, one for each supported ISP block
+ * and correctly populate the structure's field with the expected block @size
+ * (used for validation), a pointer to each block @handler function and an
+ * optional @group and @feature flags, the driver can use to differentiate which
+ * ISP blocks are present on the ISP implementation.
+ *
+ * The @group field is intended to be used as a bitmask of driver-specific
+ * flags to allow the driver to setup certain blocks at different times. As an
+ * example an ISP driver can divide its block handlers in "pre-configure" blocks
+ * and "run-time" blocks and use the @group bitmask to identify the ISP blocks
+ * that have to be pre-configured from the ones that only have to be handled at
+ * run-time. The usage and definition of the @group field is totally
+ * driver-specific.
+ *
+ * The @features flag can instead be used to differentiate between blocks
+ * implemented in different revisions of the ISP design. In example some ISP
+ * blocks might be present on more recent revision than others. Populating the
+ * @features bitmask with the ISP/SoC machine identifier allows the driver to
+ * correctly ignore the blocks not supported on the ISP revision it is running
+ * on. As per the @group bitmask, the usage and definition of the @features
+ * field is totally driver-specific.
+ */
+struct v4l2_params_handler {
+ size_t size;
+ v4l2_params_block_handler handler;
+ unsigned int group;
+ unsigned int features;
+};
+
+/**
+ * typedef v4l2_params_validate_buffer - V4L2 extensible parameters buffer
+ * validation callback
+ * @dev: the driver's device pointer (as passed by the driver to
+ * v4l2_params_buffer_validate())
+ * @buffer: the extensible parameters buffer
+ *
+ * Defines the function prototype for the driver's callback to perform
+ * driver-specific validation on the extensible parameters buffer
+ */
+typedef int (*v4l2_params_validate_buffer)(struct device *dev,
+ const struct v4l2_params_buffer *buffer);
+
+/**
+ * v4l2_params_buffer_validate - Validate a V4L2 extensible parameters buffer
+ * @dev: the driver's device pointer
+ * @vb: the videobuf2 buffer
+ * @max_size: the maximum allowed buffer size
+ * @buffer_validate: callback to the driver-specific buffer validation
+ *
+ * Helper function that performs validation of an extensible parameters buffer.
+ *
+ * The helper is meant to be used by drivers to perform validation of the
+ * extensible parameters buffer size correctness.
+ *
+ * The @vb buffer as received from the vb2 .buf_prepare operation is checked
+ * against @max_size and its validated to be large enough to accommodate at
+ * least one ISP configuration block. The effective buffer size is compared
+ * to the data size reported by @cfg to make sure they match.
+ *
+ * If provided, the @buffer_validate callback function is invoked to allow
+ * drivers to perform driver-specific validation (such as checking that the
+ * buffer version is supported).
+ *
+ * Drivers should use this function to validate the buffer size correctness
+ * before performing a copy of the user-provided videobuf2 buffer content into a
+ * kernel-only memory buffer to prevent userspace from modifying the buffer
+ * content after it has been submitted to the driver.
+ *.
+ * Examples of users of this function can be found in
+ * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
+ */
+int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
+ size_t max_size,
+ v4l2_params_validate_buffer buffer_validate);
+
+/**
+ * typedef v4l2_params_validate_block - V4L2 extensible parameters block
+ * validation callback
+ * @dev: the driver's device pointer (as passed by the driver to
+ * v4l2_params_validate())
+ * @block: the ISP configuration block to validate
+ *
+ * Defines the function prototype for the driver's callback to perform
+ * driver-specific validation on each ISP block.
+ */
+typedef int (*v4l2_params_validate_block)(struct device *dev,
+ const struct v4l2_params_block *block);
+
+/**
+ * v4l2_params_blocks_validate - Validate V4L2 extensible parameters ISP
+ * configuration blocks
+ * @dev: the driver's device pointer
+ * @buffer: the extensible parameters configuration buffer
+ * @handlers: the list of block handlers
+ * @num_handlers: the number of block handlers
+ * @block_validate: callback to the driver-specific per-block validation
+ * function
+ *
+ * Helper function that performs validation of the ISP configuration blocks in
+ * an extensible parameters buffer.
+ *
+ * The helper is meant to be used by drivers to perform validation of the
+ * ISP configuration data blocks. For each block in the extensible parameters
+ * buffer, its size and correctness are validated against its associated handler
+ * in the @handlers list. Additionally, if provided, the @block_validate
+ * callback is invoked on each block to allow drivers to perform driver-specific
+ * validation.
+ *
+ * Drivers should to use this function to validate the ISP configuration blocks
+ * after having validated the correctness of the vb2 buffer sizes by using the
+ * v4l2_params_buffer_validate() helper first. Once the buffer size has been
+ * validated, drivers should perform a copy of the user-provided buffer into a
+ * kernel-only memory buffer @cfg to prevent userspace from modifying the buffer
+ * content after it has been submitted to the driver, and then call this
+ * function to perform per-block validation.
+ *
+ * Examples of users of this function can be found in
+ * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
+ */
+int v4l2_params_blocks_validate(struct device *dev,
+ const struct v4l2_params_buffer *buffer,
+ const struct v4l2_params_handler *handlers,
+ size_t num_handlers,
+ v4l2_params_validate_block block_validate);
+
+#endif /* V4L2_PARAMS_H_ */
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c
2025-07-08 10:40 ` [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c Jacopo Mondi
@ 2025-07-09 6:20 ` kernel test robot
2025-07-10 12:42 ` Dan Scally
1 sibling, 0 replies; 27+ messages in thread
From: kernel test robot @ 2025-07-09 6:20 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: llvm, oe-kbuild-all, linux-media, linux-kernel, linux-rockchip,
linux-arm-kernel, Jacopo Mondi
Hi Jacopo,
kernel test robot noticed the following build errors:
[auto build test ERROR on a8598c7de1bcd94461ca54c972efa9b4ea501fb9]
url: https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-uapi-Introduce-V4L2-extensible-params/20250708-184651
base: a8598c7de1bcd94461ca54c972efa9b4ea501fb9
patch link: https://lore.kernel.org/r/20250708-extensible-parameters-validation-v1-5-9fc27c9c728c%40ideasonboard.com
patch subject: [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c
config: i386-buildonly-randconfig-002-20250709 (https://download.01.org/0day-ci/archive/20250709/202507091324.p3wVWAxc-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250709/202507091324.p3wVWAxc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507091324.p3wVWAxc-lkp@intel.com/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: vb2_plane_vaddr
>>> referenced by v4l2-params.c
>>> drivers/media/v4l2-core/v4l2-params.o:(v4l2_params_buffer_validate) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c
2025-07-08 10:40 ` [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c Jacopo Mondi
2025-07-09 6:20 ` kernel test robot
@ 2025-07-10 12:42 ` Dan Scally
2025-07-10 13:04 ` Jacopo Mondi
1 sibling, 1 reply; 27+ messages in thread
From: Dan Scally @ 2025-07-10 12:42 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo, thanks for the patchset
On 08/07/2025 11:40, Jacopo Mondi wrote:
> Add to the v4l2 framework an helper function to support drivers
s/an helper function/helper functions
> when validating a buffer of extensible parameters.
>
> Introduce new types in include/media/v4l2-params.h that driver shall
s/driver/drivers
> use in order to comply with the v4l2-params validation procedure, and
> add a single helper function to v4l2-params.c.
There's two functions?
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> MAINTAINERS | 2 +
> drivers/media/v4l2-core/Makefile | 3 +-
> drivers/media/v4l2-core/v4l2-params.c | 106 ++++++++++++++++++++++
> include/media/v4l2-params.h | 166 ++++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index beecac86991d988c48d31366ba5201b09ef25715..3d9a8e06c59eb08360d1e8eea85e450a15ee95af 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25973,6 +25973,8 @@ M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> L: linux-media@vger.kernel.org
> S: Maintained
> F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
> +F: drivers/media/v4l2-core/v4l2-params.c
> +F: include/media/v4l2-params.h
> F: include/uapi/linux/media/v4l2-extensible-params.h
>
> VF610 NAND DRIVER
> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> index 2177b9d63a8ffc1127c5a70118249a2ff63cd759..323330dd359f95c1ae3d0c35bd6fcb8291a33a07 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -11,7 +11,8 @@ tuner-objs := tuner-core.o
> videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> v4l2-event.o v4l2-subdev.o v4l2-common.o \
> v4l2-ctrls-core.o v4l2-ctrls-api.o \
> - v4l2-ctrls-request.o v4l2-ctrls-defs.o
> + v4l2-ctrls-request.o v4l2-ctrls-defs.o \
> + v4l2-params.o
>
> # Please keep it alphabetically sorted by Kconfig name
> # (e. g. LC_ALL=C sort Makefile)
> diff --git a/drivers/media/v4l2-core/v4l2-params.c b/drivers/media/v4l2-core/v4l2-params.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..3fb320aec900ee4a05c595f2e14c6ee0d8710669
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-params.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Video4Linux2 extensible parameters helpers
> + *
> + * Copyright (C) 2025 Ideas On Board Oy
> + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> + */
> +
> +#include <media/v4l2-params.h>
> +
> +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> + size_t max_size,
> + v4l2_params_validate_buffer buffer_validate)
> +{
> + size_t header_size = offsetof(struct v4l2_params_buffer, data);
> + struct v4l2_params_buffer *buffer = vb2_plane_vaddr(vb, 0);
> + size_t payload_size = vb2_get_plane_payload(vb, 0);
> + size_t buffer_size;
> + int ret;
> +
> + /* Payload size can't be greater than the destination buffer size */
> + if (payload_size > max_size) {
> + dev_dbg(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(dev, "Payload size is too small: %zu\n", payload_size);
> + return -EINVAL;
> + }
> +
> + /* Validate the size reported in the parameter buffer header */
> + buffer_size = header_size + buffer->data_size;
> + if (buffer_size != payload_size) {
> + dev_dbg(dev, "Data size %zu and payload size %zu are different\n",
> + buffer_size, payload_size);
> + return -EINVAL;
> + }
> +
> + /* Driver-specific buffer validation. */
> + if (buffer_validate) {
> + ret = buffer_validate(dev, buffer);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_params_buffer_validate);
> +
> +int v4l2_params_blocks_validate(struct device *dev,
> + const struct v4l2_params_buffer *buffer,
> + const struct v4l2_params_handler *handlers,
> + size_t num_handlers,
> + v4l2_params_validate_block block_validate)
> +{
> + size_t block_offset = 0;
> + size_t buffer_size;
> + int ret;
> +
> + /* Walk the list of parameter blocks and validate them. */
> + buffer_size = buffer->data_size;
> + while (buffer_size >= sizeof(struct v4l2_params_block)) {
> + const struct v4l2_params_handler *handler;
> + const struct v4l2_params_block *block;
> +
> + /* Validate block sizes and types against the handlers. */
> + block = (const struct v4l2_params_block *)
> + (buffer->data + block_offset);
> +
> + if (block->type >= num_handlers) {
> + dev_dbg(dev, "Invalid parameters block type\n");
> + return -EINVAL;
> + }
> +
> + if (block->size > buffer_size) {
> + dev_dbg(dev, "Premature end of parameters data\n");
> + return -EINVAL;
> + }
> +
> + handler = &handlers[block->type];
> + if (block->size != handler->size) {
In the C55 set I also check for just a header here:
if (block->size != handler->size &&
block->size != sizeof(*block) {
That way if all userspace wants to do is disable the block they can just fill in the header with the
flags set appropriately. The handlers of course need to be careful not to try to access past the
header in that case.
> + dev_dbg(dev, "Invalid parameters block size\n");
> + return -EINVAL;
> + }
> +
> + /* Driver-specific per-block validation. */
> + if (block_validate) {
> + ret = block_validate(dev, block);
> + if (ret)
> + return ret;
> + }
> +
> + block_offset += block->size;
> + buffer_size -= block->size;
> + }
> +
> + if (buffer_size) {
> + dev_dbg(dev, "Unexpected data after the parameters buffer end\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_params_blocks_validate);
> diff --git a/include/media/v4l2-params.h b/include/media/v4l2-params.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..55f08c646a943fef11eaeddff842fae00b8422d4
> --- /dev/null
> +++ b/include/media/v4l2-params.h
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Video4Linux2 extensible parameters helpers
> + *
> + * Copyright (C) 2025 Ideas On Board Oy
> + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> + */
> +
> +#ifndef V4L2_PARAMS_H_
> +#define V4L2_PARAMS_H_
> +
> +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> +#include <linux/media/v4l2-extensible-params.h>
> +
> +#include <linux/device.h>
> +
> +#include <media/videobuf2-core.h>
> +
> +/**
> + * typedef v4l2_params_block_handler - V4L2 extensible format block handler
> + * @arg: pointer the driver-specific argument
s/pointer/pointer to
> + * @block: the ISP configuration block to handle
> + *
> + * Defines the function signature of the functions that handle an ISP block
> + * configuration.
> + */
> +typedef void (*v4l2_params_block_handler)(void *arg,
> + const struct v4l2_params_block *block);
> +
> +/**
> + * struct v4l2_params_handler - V4L2 extensible format handler
> + * @size: the block expected size
> + * @handler: the block handler function
> + * @group: the device-specific group id the block belongs to (optional)
> + * @features: the device-specific features flags (optional)
> + *
> + * The v4l2_params_handler defines the type that driver making use of the
> + * V4L2 extensible parameters shall use to define their own ISP block
> + * handlers.
> + *
> + * Drivers shall prepare a list of handlers, one for each supported ISP block
> + * and correctly populate the structure's field with the expected block @size
> + * (used for validation), a pointer to each block @handler function and an
> + * optional @group and @feature flags, the driver can use to differentiate which
> + * ISP blocks are present on the ISP implementation.
> + *
> + * The @group field is intended to be used as a bitmask of driver-specific
> + * flags to allow the driver to setup certain blocks at different times. As an
> + * example an ISP driver can divide its block handlers in "pre-configure" blocks
> + * and "run-time" blocks and use the @group bitmask to identify the ISP blocks
> + * that have to be pre-configured from the ones that only have to be handled at
> + * run-time. The usage and definition of the @group field is totally
> + * driver-specific.
> + *
> + * The @features flag can instead be used to differentiate between blocks
> + * implemented in different revisions of the ISP design. In example some ISP
> + * blocks might be present on more recent revision than others. Populating the
> + * @features bitmask with the ISP/SoC machine identifier allows the driver to
> + * correctly ignore the blocks not supported on the ISP revision it is running
> + * on. As per the @group bitmask, the usage and definition of the @features
> + * field is totally driver-specific.
> + */
> +struct v4l2_params_handler {
> + size_t size;
> + v4l2_params_block_handler handler;
> + unsigned int group;
> + unsigned int features;
> +};
> +
> +/**
> + * typedef v4l2_params_validate_buffer - V4L2 extensible parameters buffer
> + * validation callback
> + * @dev: the driver's device pointer (as passed by the driver to
> + * v4l2_params_buffer_validate())
> + * @buffer: the extensible parameters buffer
> + *
> + * Defines the function prototype for the driver's callback to perform
> + * driver-specific validation on the extensible parameters buffer
> + */
> +typedef int (*v4l2_params_validate_buffer)(struct device *dev,
> + const struct v4l2_params_buffer *buffer);
> +
> +/**
> + * v4l2_params_buffer_validate - Validate a V4L2 extensible parameters buffer
> + * @dev: the driver's device pointer
> + * @vb: the videobuf2 buffer
> + * @max_size: the maximum allowed buffer size
> + * @buffer_validate: callback to the driver-specific buffer validation
> + *
> + * Helper function that performs validation of an extensible parameters buffer.
> + *
> + * The helper is meant to be used by drivers to perform validation of the
> + * extensible parameters buffer size correctness.
> + *
> + * The @vb buffer as received from the vb2 .buf_prepare operation is checked
> + * against @max_size and its validated to be large enough to accommodate at
> + * least one ISP configuration block. The effective buffer size is compared
> + * to the data size reported by @cfg to make sure they match.
> + *
> + * If provided, the @buffer_validate callback function is invoked to allow
> + * drivers to perform driver-specific validation (such as checking that the
> + * buffer version is supported).
> + *
> + * Drivers should use this function to validate the buffer size correctness
> + * before performing a copy of the user-provided videobuf2 buffer content into a
> + * kernel-only memory buffer to prevent userspace from modifying the buffer
> + * content after it has been submitted to the driver.
> + *.
> + * Examples of users of this function can be found in
> + * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
> + */
> +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> + size_t max_size,
> + v4l2_params_validate_buffer buffer_validate);
> +
> +/**
> + * typedef v4l2_params_validate_block - V4L2 extensible parameters block
> + * validation callback
> + * @dev: the driver's device pointer (as passed by the driver to
> + * v4l2_params_validate())
> + * @block: the ISP configuration block to validate
> + *
> + * Defines the function prototype for the driver's callback to perform
> + * driver-specific validation on each ISP block.
> + */
> +typedef int (*v4l2_params_validate_block)(struct device *dev,
> + const struct v4l2_params_block *block);
> +
> +/**
> + * v4l2_params_blocks_validate - Validate V4L2 extensible parameters ISP
> + * configuration blocks
> + * @dev: the driver's device pointer
> + * @buffer: the extensible parameters configuration buffer
> + * @handlers: the list of block handlers
> + * @num_handlers: the number of block handlers
> + * @block_validate: callback to the driver-specific per-block validation
> + * function
> + *
> + * Helper function that performs validation of the ISP configuration blocks in
> + * an extensible parameters buffer.
> + *
> + * The helper is meant to be used by drivers to perform validation of the
> + * ISP configuration data blocks. For each block in the extensible parameters
> + * buffer, its size and correctness are validated against its associated handler
> + * in the @handlers list. Additionally, if provided, the @block_validate
> + * callback is invoked on each block to allow drivers to perform driver-specific
> + * validation.
> + *
> + * Drivers should to use this function to validate the ISP configuration blocks
> + * after having validated the correctness of the vb2 buffer sizes by using the
> + * v4l2_params_buffer_validate() helper first. Once the buffer size has been
> + * validated, drivers should perform a copy of the user-provided buffer into a
> + * kernel-only memory buffer @cfg to prevent userspace from modifying the buffer
> + * content after it has been submitted to the driver, and then call this
> + * function to perform per-block validation.
> + *
> + * Examples of users of this function can be found in
> + * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
> + */
> +int v4l2_params_blocks_validate(struct device *dev,
> + const struct v4l2_params_buffer *buffer,
> + const struct v4l2_params_handler *handlers,
> + size_t num_handlers,
> + v4l2_params_validate_block block_validate);
> +
> +#endif /* V4L2_PARAMS_H_ */
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c
2025-07-10 12:42 ` Dan Scally
@ 2025-07-10 13:04 ` Jacopo Mondi
0 siblings, 0 replies; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-10 13:04 UTC (permalink / raw)
To: Dan Scally
Cc: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus, linux-kernel,
linux-media, linux-rockchip, linux-arm-kernel
Hi Dan
On Thu, Jul 10, 2025 at 01:42:39PM +0100, Dan Scally wrote:
> Hi Jacopo, thanks for the patchset
>
> On 08/07/2025 11:40, Jacopo Mondi wrote:
> > Add to the v4l2 framework an helper function to support drivers
> s/an helper function/helper functions
> > when validating a buffer of extensible parameters.
> >
> > Introduce new types in include/media/v4l2-params.h that driver shall
> s/driver/drivers
> > use in order to comply with the v4l2-params validation procedure, and
> > add a single helper function to v4l2-params.c.
> There's two functions?
Thanks, just a few minutes before I was about to send v2 :)
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> > MAINTAINERS | 2 +
> > drivers/media/v4l2-core/Makefile | 3 +-
> > drivers/media/v4l2-core/v4l2-params.c | 106 ++++++++++++++++++++++
> > include/media/v4l2-params.h | 166 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 276 insertions(+), 1 deletion(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index beecac86991d988c48d31366ba5201b09ef25715..3d9a8e06c59eb08360d1e8eea85e450a15ee95af 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25973,6 +25973,8 @@ M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > L: linux-media@vger.kernel.org
> > S: Maintained
> > F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
> > +F: drivers/media/v4l2-core/v4l2-params.c
> > +F: include/media/v4l2-params.h
> > F: include/uapi/linux/media/v4l2-extensible-params.h
> > VF610 NAND DRIVER
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index 2177b9d63a8ffc1127c5a70118249a2ff63cd759..323330dd359f95c1ae3d0c35bd6fcb8291a33a07 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -11,7 +11,8 @@ tuner-objs := tuner-core.o
> > videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> > v4l2-event.o v4l2-subdev.o v4l2-common.o \
> > v4l2-ctrls-core.o v4l2-ctrls-api.o \
> > - v4l2-ctrls-request.o v4l2-ctrls-defs.o
> > + v4l2-ctrls-request.o v4l2-ctrls-defs.o \
> > + v4l2-params.o
> > # Please keep it alphabetically sorted by Kconfig name
> > # (e. g. LC_ALL=C sort Makefile)
> > diff --git a/drivers/media/v4l2-core/v4l2-params.c b/drivers/media/v4l2-core/v4l2-params.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..3fb320aec900ee4a05c595f2e14c6ee0d8710669
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-params.c
> > @@ -0,0 +1,106 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Video4Linux2 extensible parameters helpers
> > + *
> > + * Copyright (C) 2025 Ideas On Board Oy
> > + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > + */
> > +
> > +#include <media/v4l2-params.h>
> > +
> > +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> > + size_t max_size,
> > + v4l2_params_validate_buffer buffer_validate)
> > +{
> > + size_t header_size = offsetof(struct v4l2_params_buffer, data);
> > + struct v4l2_params_buffer *buffer = vb2_plane_vaddr(vb, 0);
> > + size_t payload_size = vb2_get_plane_payload(vb, 0);
> > + size_t buffer_size;
> > + int ret;
> > +
> > + /* Payload size can't be greater than the destination buffer size */
> > + if (payload_size > max_size) {
> > + dev_dbg(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(dev, "Payload size is too small: %zu\n", payload_size);
> > + return -EINVAL;
> > + }
> > +
> > + /* Validate the size reported in the parameter buffer header */
> > + buffer_size = header_size + buffer->data_size;
> > + if (buffer_size != payload_size) {
> > + dev_dbg(dev, "Data size %zu and payload size %zu are different\n",
> > + buffer_size, payload_size);
> > + return -EINVAL;
> > + }
> > +
> > + /* Driver-specific buffer validation. */
> > + if (buffer_validate) {
> > + ret = buffer_validate(dev, buffer);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_params_buffer_validate);
> > +
> > +int v4l2_params_blocks_validate(struct device *dev,
> > + const struct v4l2_params_buffer *buffer,
> > + const struct v4l2_params_handler *handlers,
> > + size_t num_handlers,
> > + v4l2_params_validate_block block_validate)
> > +{
> > + size_t block_offset = 0;
> > + size_t buffer_size;
> > + int ret;
> > +
> > + /* Walk the list of parameter blocks and validate them. */
> > + buffer_size = buffer->data_size;
> > + while (buffer_size >= sizeof(struct v4l2_params_block)) {
> > + const struct v4l2_params_handler *handler;
> > + const struct v4l2_params_block *block;
> > +
> > + /* Validate block sizes and types against the handlers. */
> > + block = (const struct v4l2_params_block *)
> > + (buffer->data + block_offset);
> > +
> > + if (block->type >= num_handlers) {
> > + dev_dbg(dev, "Invalid parameters block type\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (block->size > buffer_size) {
> > + dev_dbg(dev, "Premature end of parameters data\n");
> > + return -EINVAL;
> > + }
> > +
> > + handler = &handlers[block->type];
> > + if (block->size != handler->size) {
>
> In the C55 set I also check for just a header here:
>
>
> if (block->size != handler->size &&
>
> block->size != sizeof(*block) {
>
>
> That way if all userspace wants to do is disable the block they can just
> fill in the header with the flags set appropriately. The handlers of course
> need to be careful not to try to access past the header in that case.
>
I think that's right. Both the existing implementations in rkisp1 and
c3 allows that
rkisp1
* When userspace simply wants to disable an ISP block the
* RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE bit should be set in @flags field. The
* driver ignores the rest of the block configuration structure in this case.
c3
* When userspace wants to disable an ISP block the
* C3_ISP_PARAMS_BLOCK_FL_DISABLED bit should be set in the @flags field. In
* this case userspace may optionally omit the remainder of the configuration
* block, which will be ignored by the driver.
So that's probably something we want to allow yes.
I'll take the suggestion in, thanks for checking
> > + dev_dbg(dev, "Invalid parameters block size\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* Driver-specific per-block validation. */
> > + if (block_validate) {
> > + ret = block_validate(dev, block);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + block_offset += block->size;
> > + buffer_size -= block->size;
> > + }
> > +
> > + if (buffer_size) {
> > + dev_dbg(dev, "Unexpected data after the parameters buffer end\n");
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_params_blocks_validate);
> > diff --git a/include/media/v4l2-params.h b/include/media/v4l2-params.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..55f08c646a943fef11eaeddff842fae00b8422d4
> > --- /dev/null
> > +++ b/include/media/v4l2-params.h
> > @@ -0,0 +1,166 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Video4Linux2 extensible parameters helpers
> > + *
> > + * Copyright (C) 2025 Ideas On Board Oy
> > + * Author: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > + */
> > +
> > +#ifndef V4L2_PARAMS_H_
> > +#define V4L2_PARAMS_H_
> > +
> > +#define _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > +#include <linux/media/v4l2-extensible-params.h>
> > +
> > +#include <linux/device.h>
> > +
> > +#include <media/videobuf2-core.h>
> > +
> > +/**
> > + * typedef v4l2_params_block_handler - V4L2 extensible format block handler
> > + * @arg: pointer the driver-specific argument
> s/pointer/pointer to
> > + * @block: the ISP configuration block to handle
> > + *
> > + * Defines the function signature of the functions that handle an ISP block
> > + * configuration.
> > + */
> > +typedef void (*v4l2_params_block_handler)(void *arg,
> > + const struct v4l2_params_block *block);
> > +
> > +/**
> > + * struct v4l2_params_handler - V4L2 extensible format handler
> > + * @size: the block expected size
> > + * @handler: the block handler function
> > + * @group: the device-specific group id the block belongs to (optional)
> > + * @features: the device-specific features flags (optional)
> > + *
> > + * The v4l2_params_handler defines the type that driver making use of the
> > + * V4L2 extensible parameters shall use to define their own ISP block
> > + * handlers.
> > + *
> > + * Drivers shall prepare a list of handlers, one for each supported ISP block
> > + * and correctly populate the structure's field with the expected block @size
> > + * (used for validation), a pointer to each block @handler function and an
> > + * optional @group and @feature flags, the driver can use to differentiate which
> > + * ISP blocks are present on the ISP implementation.
> > + *
> > + * The @group field is intended to be used as a bitmask of driver-specific
> > + * flags to allow the driver to setup certain blocks at different times. As an
> > + * example an ISP driver can divide its block handlers in "pre-configure" blocks
> > + * and "run-time" blocks and use the @group bitmask to identify the ISP blocks
> > + * that have to be pre-configured from the ones that only have to be handled at
> > + * run-time. The usage and definition of the @group field is totally
> > + * driver-specific.
> > + *
> > + * The @features flag can instead be used to differentiate between blocks
> > + * implemented in different revisions of the ISP design. In example some ISP
> > + * blocks might be present on more recent revision than others. Populating the
> > + * @features bitmask with the ISP/SoC machine identifier allows the driver to
> > + * correctly ignore the blocks not supported on the ISP revision it is running
> > + * on. As per the @group bitmask, the usage and definition of the @features
> > + * field is totally driver-specific.
> > + */
> > +struct v4l2_params_handler {
> > + size_t size;
> > + v4l2_params_block_handler handler;
> > + unsigned int group;
> > + unsigned int features;
> > +};
> > +
> > +/**
> > + * typedef v4l2_params_validate_buffer - V4L2 extensible parameters buffer
> > + * validation callback
> > + * @dev: the driver's device pointer (as passed by the driver to
> > + * v4l2_params_buffer_validate())
> > + * @buffer: the extensible parameters buffer
> > + *
> > + * Defines the function prototype for the driver's callback to perform
> > + * driver-specific validation on the extensible parameters buffer
> > + */
> > +typedef int (*v4l2_params_validate_buffer)(struct device *dev,
> > + const struct v4l2_params_buffer *buffer);
> > +
> > +/**
> > + * v4l2_params_buffer_validate - Validate a V4L2 extensible parameters buffer
> > + * @dev: the driver's device pointer
> > + * @vb: the videobuf2 buffer
> > + * @max_size: the maximum allowed buffer size
> > + * @buffer_validate: callback to the driver-specific buffer validation
> > + *
> > + * Helper function that performs validation of an extensible parameters buffer.
> > + *
> > + * The helper is meant to be used by drivers to perform validation of the
> > + * extensible parameters buffer size correctness.
> > + *
> > + * The @vb buffer as received from the vb2 .buf_prepare operation is checked
> > + * against @max_size and its validated to be large enough to accommodate at
> > + * least one ISP configuration block. The effective buffer size is compared
> > + * to the data size reported by @cfg to make sure they match.
> > + *
> > + * If provided, the @buffer_validate callback function is invoked to allow
> > + * drivers to perform driver-specific validation (such as checking that the
> > + * buffer version is supported).
> > + *
> > + * Drivers should use this function to validate the buffer size correctness
> > + * before performing a copy of the user-provided videobuf2 buffer content into a
> > + * kernel-only memory buffer to prevent userspace from modifying the buffer
> > + * content after it has been submitted to the driver.
> > + *.
> > + * Examples of users of this function can be found in
> > + * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
> > + */
> > +int v4l2_params_buffer_validate(struct device *dev, struct vb2_buffer *vb,
> > + size_t max_size,
> > + v4l2_params_validate_buffer buffer_validate);
> > +
> > +/**
> > + * typedef v4l2_params_validate_block - V4L2 extensible parameters block
> > + * validation callback
> > + * @dev: the driver's device pointer (as passed by the driver to
> > + * v4l2_params_validate())
> > + * @block: the ISP configuration block to validate
> > + *
> > + * Defines the function prototype for the driver's callback to perform
> > + * driver-specific validation on each ISP block.
> > + */
> > +typedef int (*v4l2_params_validate_block)(struct device *dev,
> > + const struct v4l2_params_block *block);
> > +
> > +/**
> > + * v4l2_params_blocks_validate - Validate V4L2 extensible parameters ISP
> > + * configuration blocks
> > + * @dev: the driver's device pointer
> > + * @buffer: the extensible parameters configuration buffer
> > + * @handlers: the list of block handlers
> > + * @num_handlers: the number of block handlers
> > + * @block_validate: callback to the driver-specific per-block validation
> > + * function
> > + *
> > + * Helper function that performs validation of the ISP configuration blocks in
> > + * an extensible parameters buffer.
> > + *
> > + * The helper is meant to be used by drivers to perform validation of the
> > + * ISP configuration data blocks. For each block in the extensible parameters
> > + * buffer, its size and correctness are validated against its associated handler
> > + * in the @handlers list. Additionally, if provided, the @block_validate
> > + * callback is invoked on each block to allow drivers to perform driver-specific
> > + * validation.
> > + *
> > + * Drivers should to use this function to validate the ISP configuration blocks
> > + * after having validated the correctness of the vb2 buffer sizes by using the
> > + * v4l2_params_buffer_validate() helper first. Once the buffer size has been
> > + * validated, drivers should perform a copy of the user-provided buffer into a
> > + * kernel-only memory buffer @cfg to prevent userspace from modifying the buffer
> > + * content after it has been submitted to the driver, and then call this
> > + * function to perform per-block validation.
> > + *
> > + * Examples of users of this function can be found in
> > + * rkisp1_params_prepare_ext_params() and in c3_isp_params_vb2_buf_prepare().
> > + */
> > +int v4l2_params_blocks_validate(struct device *dev,
> > + const struct v4l2_params_buffer *buffer,
> > + const struct v4l2_params_handler *handlers,
> > + size_t num_handlers,
> > + v4l2_params_validate_block block_validate);
> > +
> > +#endif /* V4L2_PARAMS_H_ */
> >
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 6/8] media: rkisp1: Use v4l2-params for validation
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (4 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 5/8] media: v4l2-common: Introduce v4l2-params.c Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-09 15:09 ` Dan Scally
2025-07-08 10:40 ` [PATCH 7/8] media: amlogic-c3: " Jacopo Mondi
` (2 subsequent siblings)
8 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Convert rkisp1-params.c to use the new types for block handlers
defined in v4l2-params.h and use the new helpers from v4l2-params.c
to remove bolierplate code from the driver.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
.../media/platform/rockchip/rkisp1/rkisp1-params.c | 364 +++++++++------------
1 file changed, 156 insertions(+), 208 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index f1585f8fa0f478304f74317fd9dd09199c94ec82..36840048e97b4557894cf401210d1c135de874cc 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -12,6 +12,7 @@
#include <media/v4l2-common.h>
#include <media/v4l2-event.h>
#include <media/v4l2-ioctl.h>
+#include <media/v4l2-params.h>
#include <media/videobuf2-core.h>
#include <media/videobuf2-vmalloc.h> /* for ISP params */
@@ -40,30 +41,6 @@
#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;
- struct rkisp1_ext_params_compand_bls_config compand_bls;
- struct rkisp1_ext_params_compand_curve_config compand_curve;
- struct rkisp1_ext_params_wdr_config wdr;
-};
-
enum rkisp1_params_formats {
RKISP1_PARAMS_FIXED,
RKISP1_PARAMS_EXTENSIBLE,
@@ -1689,11 +1666,12 @@ 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)
+static void rkisp1_ext_params_bls(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_bls_config *bls = &block->bls;
+ const struct rkisp1_ext_params_bls_config *bls =
+ (const struct rkisp1_ext_params_bls_config *)block;
+ struct rkisp1_params *params = dev;
if (bls->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
@@ -1709,11 +1687,12 @@ rkisp1_ext_params_bls(struct rkisp1_params *params,
RKISP1_CIF_ISP_BLS_ENA);
}
-static void
-rkisp1_ext_params_dpcc(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_dpcc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_dpcc_config *dpcc = &block->dpcc;
+ const struct rkisp1_ext_params_dpcc_config *dpcc =
+ (const struct rkisp1_ext_params_dpcc_config *)block;
+ struct rkisp1_params *params = dev;
if (dpcc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
@@ -1729,11 +1708,12 @@ rkisp1_ext_params_dpcc(struct rkisp1_params *params,
RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
}
-static void
-rkisp1_ext_params_sdg(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_sdg(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_sdg_config *sdg = &block->sdg;
+ const struct rkisp1_ext_params_sdg_config *sdg =
+ (const struct rkisp1_ext_params_sdg_config *)block;
+ struct rkisp1_params *params = dev;
if (sdg->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
@@ -1749,11 +1729,12 @@ rkisp1_ext_params_sdg(struct rkisp1_params *params,
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)
+static void rkisp1_ext_params_lsc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_lsc_config *lsc = &block->lsc;
+ const struct rkisp1_ext_params_lsc_config *lsc =
+ (const struct rkisp1_ext_params_lsc_config *)block;
+ struct rkisp1_params *params = dev;
if (lsc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
@@ -1769,11 +1750,12 @@ rkisp1_ext_params_lsc(struct rkisp1_params *params,
RKISP1_CIF_ISP_LSC_CTRL_ENA);
}
-static void
-rkisp1_ext_params_awbg(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_awbg(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_awb_gain_config *awbg = &block->awbg;
+ const struct rkisp1_ext_params_awb_gain_config *awbg =
+ (const struct rkisp1_ext_params_awb_gain_config *)block;
+ struct rkisp1_params *params = dev;
if (awbg->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
@@ -1789,11 +1771,12 @@ rkisp1_ext_params_awbg(struct rkisp1_params *params,
RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
}
-static void
-rkisp1_ext_params_flt(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_flt(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_flt_config *flt = &block->flt;
+ const struct rkisp1_ext_params_flt_config *flt =
+ (const struct rkisp1_ext_params_flt_config *)block;
+ struct rkisp1_params *params = dev;
if (flt->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE,
@@ -1809,11 +1792,12 @@ rkisp1_ext_params_flt(struct rkisp1_params *params,
RKISP1_CIF_ISP_FLT_ENA);
}
-static void
-rkisp1_ext_params_bdm(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_bdm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_bdm_config *bdm = &block->bdm;
+ const struct rkisp1_ext_params_bdm_config *bdm =
+ (const struct rkisp1_ext_params_bdm_config *)block;
+ struct rkisp1_params *params = dev;
if (bdm->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
@@ -1829,11 +1813,12 @@ rkisp1_ext_params_bdm(struct rkisp1_params *params,
RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
}
-static void
-rkisp1_ext_params_ctk(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_ctk(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_ctk_config *ctk = &block->ctk;
+ const struct rkisp1_ext_params_ctk_config *ctk =
+ (const struct rkisp1_ext_params_ctk_config *)block;
+ struct rkisp1_params *params = dev;
if (ctk->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_ctk_enable(params, false);
@@ -1847,11 +1832,12 @@ rkisp1_ext_params_ctk(struct rkisp1_params *params,
rkisp1_ctk_enable(params, true);
}
-static void
-rkisp1_ext_params_goc(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_goc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_goc_config *goc = &block->goc;
+ const struct rkisp1_ext_params_goc_config *goc =
+ (const struct rkisp1_ext_params_goc_config *)block;
+ struct rkisp1_params *params = dev;
if (goc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
@@ -1869,11 +1855,12 @@ rkisp1_ext_params_goc(struct rkisp1_params *params,
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)
+static void rkisp1_ext_params_dpf(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_dpf_config *dpf = &block->dpf;
+ const struct rkisp1_ext_params_dpf_config *dpf =
+ (const struct rkisp1_ext_params_dpf_config *)block;
+ struct rkisp1_params *params = dev;
if (dpf->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE,
@@ -1889,20 +1876,22 @@ rkisp1_ext_params_dpf(struct rkisp1_params *params,
RKISP1_CIF_ISP_DPF_MODE_EN);
}
-static void
-rkisp1_ext_params_dpfs(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_dpfs(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_dpf_strength_config *dpfs = &block->dpfs;
+ const struct rkisp1_ext_params_dpf_strength_config *dpfs =
+ (const struct rkisp1_ext_params_dpf_strength_config *)block;
+ struct rkisp1_params *params = dev;
rkisp1_dpf_strength_config(params, &dpfs->config);
}
-static void
-rkisp1_ext_params_cproc(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_cproc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_cproc_config *cproc = &block->cproc;
+ const struct rkisp1_ext_params_cproc_config *cproc =
+ (const struct rkisp1_ext_params_cproc_config *)block;
+ struct rkisp1_params *params = dev;
if (cproc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
@@ -1918,11 +1907,12 @@ rkisp1_ext_params_cproc(struct rkisp1_params *params,
RKISP1_CIF_C_PROC_CTR_ENABLE);
}
-static void
-rkisp1_ext_params_ie(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_ie(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_ie_config *ie = &block->ie;
+ const struct rkisp1_ext_params_ie_config *ie =
+ (const struct rkisp1_ext_params_ie_config *)block;
+ struct rkisp1_params *params = dev;
if (ie->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_ie_enable(params, false);
@@ -1936,11 +1926,12 @@ rkisp1_ext_params_ie(struct rkisp1_params *params,
rkisp1_ie_enable(params, true);
}
-static void
-rkisp1_ext_params_awbm(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_awbm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_awb_meas_config *awbm = &block->awbm;
+ const struct rkisp1_ext_params_awb_meas_config *awbm =
+ (const struct rkisp1_ext_params_awb_meas_config *)block;
+ struct rkisp1_params *params = dev;
if (awbm->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
params->ops->awb_meas_enable(params, &awbm->config,
@@ -1956,11 +1947,12 @@ rkisp1_ext_params_awbm(struct rkisp1_params *params,
true);
}
-static void
-rkisp1_ext_params_hstm(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_hstm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_hst_config *hst = &block->hst;
+ const struct rkisp1_ext_params_hst_config *hst =
+ (const struct rkisp1_ext_params_hst_config *)block;
+ struct rkisp1_params *params = dev;
if (hst->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
params->ops->hst_enable(params, &hst->config, false);
@@ -1974,11 +1966,12 @@ rkisp1_ext_params_hstm(struct rkisp1_params *params,
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)
+static void rkisp1_ext_params_aecm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_aec_config *aec = &block->aec;
+ const struct rkisp1_ext_params_aec_config *aec =
+ (const struct rkisp1_ext_params_aec_config *)block;
+ struct rkisp1_params *params = dev;
if (aec->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
@@ -1994,11 +1987,12 @@ rkisp1_ext_params_aecm(struct rkisp1_params *params,
RKISP1_CIF_ISP_EXP_ENA);
}
-static void
-rkisp1_ext_params_afcm(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_afcm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_afc_config *afc = &block->afc;
+ const struct rkisp1_ext_params_afc_config *afc =
+ (const struct rkisp1_ext_params_afc_config *)block;
+ struct rkisp1_params *params = dev;
if (afc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
@@ -2014,11 +2008,12 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
RKISP1_CIF_ISP_AFM_ENA);
}
-static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_compand_bls(void *dev,
+ const struct v4l2_params_block *block)
{
const struct rkisp1_ext_params_compand_bls_config *bls =
- &block->compand_bls;
+ (const struct rkisp1_ext_params_compand_bls_config *)block;
+ struct rkisp1_params *params = dev;
if (bls->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
@@ -2034,11 +2029,13 @@ static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
}
-static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void
+rkisp1_ext_params_compand_expand(void *dev,
+ const struct v4l2_params_block *block)
{
const struct rkisp1_ext_params_compand_curve_config *curve =
- &block->compand_curve;
+ (const struct rkisp1_ext_params_compand_curve_config *)block;
+ struct rkisp1_params *params = dev;
if (curve->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
@@ -2054,11 +2051,13 @@ static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
}
-static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void
+rkisp1_ext_params_compand_compress(void *dev,
+ const struct v4l2_params_block *block)
{
const struct rkisp1_ext_params_compand_curve_config *curve =
- &block->compand_curve;
+ (const struct rkisp1_ext_params_compand_curve_config *)block;
+ struct rkisp1_params *params = dev;
if (curve->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
@@ -2074,10 +2073,12 @@ static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
}
-static void rkisp1_ext_params_wdr(struct rkisp1_params *params,
- const union rkisp1_ext_params_config *block)
+static void rkisp1_ext_params_wdr(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct rkisp1_ext_params_wdr_config *wdr = &block->wdr;
+ const struct rkisp1_ext_params_wdr_config *wdr =
+ (const struct rkisp1_ext_params_wdr_config *)block;
+ struct rkisp1_params *params = dev;
if (wdr->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_WDR_CTRL,
@@ -2093,15 +2094,7 @@ static void rkisp1_ext_params_wdr(struct rkisp1_params *params,
RKISP1_CIF_ISP_WDR_CTRL_ENABLE);
}
-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;
- unsigned int features;
-} rkisp1_ext_params_handlers[] = {
+static const struct v4l2_params_handler rkisp1_ext_params_handlers[] = {
[RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
.size = sizeof(struct rkisp1_ext_params_bls_config),
.handler = rkisp1_ext_params_bls,
@@ -2224,18 +2217,18 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
/* 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;
+ const struct v4l2_params_handler *block_handler;
+ const struct v4l2_params_block *block;
- block = (const union rkisp1_ext_params_config *)
+ block = (const struct v4l2_params_block *)
&cfg->data[block_offset];
- block_offset += block->header.size;
+ block_offset += block->size;
/*
* Make sure the block is supported by the platform and in the
* list of groups to configure.
*/
- block_handler = &rkisp1_ext_params_handlers[block->header.type];
+ block_handler = &rkisp1_ext_params_handlers[block->type];
if (!(block_handler->group & block_group_mask))
continue;
@@ -2245,10 +2238,10 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
block_handler->handler(params, block);
- if (block->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)
- params->enabled_blocks &= ~BIT(block->header.type);
- else if (block->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE)
- params->enabled_blocks |= BIT(block->header.type);
+ if (block->flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)
+ params->enabled_blocks &= ~BIT(block->type);
+ else if (block->flags & RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE)
+ params->enabled_blocks |= BIT(block->type);
}
}
@@ -2641,36 +2634,51 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
spin_unlock_irq(¶ms->config_lock);
}
+static int
+rkisp1_ext_params_validate_buffer(struct device *dev,
+ const struct v4l2_params_buffer *buffer)
+{
+ /* Only v1 is supported at the moment. */
+ if (buffer->version != RKISP1_EXT_PARAM_BUFFER_V1) {
+ dev_dbg(dev, "Unsupported extensible format version: %u\n",
+ buffer->version);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+rkisp1_ext_params_validate_block(struct device *dev,
+ const struct v4l2_params_block *block)
+{
+ if ((block->flags & (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
+ RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) ==
+ (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
+ RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) {
+ dev_dbg(dev, "Invalid parameters block flags\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
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);
- size_t header_size = offsetof(struct rkisp1_ext_params_cfg, data);
struct rkisp1_ext_params_cfg *cfg = params_buf->cfg;
size_t payload_size = vb2_get_plane_payload(vb, 0);
struct rkisp1_ext_params_cfg *usr_cfg =
vb2_plane_vaddr(&vbuf->vb2_buf, 0);
- size_t block_offset = 0;
- size_t cfg_size;
-
- /*
- * Validate the buffer payload size before copying the parameters. The
- * payload has to be smaller than the destination buffer size and larger
- * than the header size.
- */
- if (payload_size > params->metafmt->buffersize) {
- dev_dbg(params->rkisp1->dev,
- "Too large buffer payload size %zu\n", payload_size);
- return -EINVAL;
- }
+ int ret;
- if (payload_size < header_size) {
- dev_dbg(params->rkisp1->dev,
- "Buffer payload %zu smaller than header size %zu\n",
- payload_size, header_size);
- return -EINVAL;
- }
+ ret = v4l2_params_buffer_validate(params->rkisp1->dev, vb,
+ params->metafmt->buffersize,
+ rkisp1_ext_params_validate_buffer);
+ if (ret)
+ return ret;
/*
* Copy the parameters buffer to the internal scratch buffer to avoid
@@ -2678,71 +2686,11 @@ static int rkisp1_params_prepare_ext_params(struct rkisp1_params *params,
*/
memcpy(cfg, usr_cfg, payload_size);
- /* Only v1 is supported at the moment. */
- if (cfg->version != RKISP1_EXT_PARAM_BUFFER_V1) {
- dev_dbg(params->rkisp1->dev,
- "Unsupported extensible format version: %u\n",
- cfg->version);
- return -EINVAL;
- }
-
- /* Validate the size reported in the parameters buffer header. */
- cfg_size = header_size + cfg->data_size;
- if (cfg_size != payload_size) {
- dev_dbg(params->rkisp1->dev,
- "Data size %zu different than buffer payload size %zu\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 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;
- }
-
- if ((block->flags & (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
- RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) ==
- (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
- RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) {
- dev_dbg(params->rkisp1->dev,
- "Invalid parameters block flags\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;
- }
-
- if (cfg_size) {
- dev_dbg(params->rkisp1->dev,
- "Unexpected data after the parameters buffer end\n");
- return -EINVAL;
- }
-
- return 0;
+ return v4l2_params_blocks_validate(params->rkisp1->dev,
+ (struct v4l2_params_buffer *)cfg,
+ rkisp1_ext_params_handlers,
+ ARRAY_SIZE(rkisp1_ext_params_handlers),
+ rkisp1_ext_params_validate_block);
}
static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
@@ -2842,7 +2790,7 @@ static int rkisp1_params_ctrl_init(struct rkisp1_params *params)
v4l2_ctrl_handler_init(¶ms->ctrls, 1);
for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
- const struct rkisp1_ext_params_handler *block_handler;
+ const struct v4l2_params_handler *block_handler;
block_handler = &rkisp1_ext_params_handlers[i];
ctrl_config.max |= BIT(i);
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 6/8] media: rkisp1: Use v4l2-params for validation
2025-07-08 10:40 ` [PATCH 6/8] media: rkisp1: Use v4l2-params for validation Jacopo Mondi
@ 2025-07-09 15:09 ` Dan Scally
0 siblings, 0 replies; 27+ messages in thread
From: Dan Scally @ 2025-07-09 15:09 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 08/07/2025 11:40, Jacopo Mondi wrote:
> Convert rkisp1-params.c to use the new types for block handlers
> defined in v4l2-params.h and use the new helpers from v4l2-params.c
> to remove bolierplate code from the driver.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> .../media/platform/rockchip/rkisp1/rkisp1-params.c | 364 +++++++++------------
> 1 file changed, 156 insertions(+), 208 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index f1585f8fa0f478304f74317fd9dd09199c94ec82..36840048e97b4557894cf401210d1c135de874cc 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -12,6 +12,7 @@
> #include <media/v4l2-common.h>
> #include <media/v4l2-event.h>
> #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-params.h>
> #include <media/videobuf2-core.h>
> #include <media/videobuf2-vmalloc.h> /* for ISP params */
>
> @@ -40,30 +41,6 @@
> #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;
> - struct rkisp1_ext_params_compand_bls_config compand_bls;
> - struct rkisp1_ext_params_compand_curve_config compand_curve;
> - struct rkisp1_ext_params_wdr_config wdr;
> -};
I think that we should keep the union, it makes things a lot tidier. We can replace the first member
with a struct v4l2_params_block[_header] pointer to keep the same effect, and then have a union
variable in each handler that's set like so:
static void mali_c55_params_lsc_selection(void *arg, const struct v4l2_params_block *header)
{
const struct mali_c55_params_mesh_shading_selection *params;
union mali_c55_params_block block;
struct mali_c55 *mali_c55 = arg;
block.header = header;
params = block.shading_selection;
...
}
> -
> enum rkisp1_params_formats {
> RKISP1_PARAMS_FIXED,
> RKISP1_PARAMS_EXTENSIBLE,
> @@ -1689,11 +1666,12 @@ 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)
> +static void rkisp1_ext_params_bls(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_bls_config *bls = &block->bls;
> + const struct rkisp1_ext_params_bls_config *bls =
> + (const struct rkisp1_ext_params_bls_config *)block;
And then we can avoid all the casting like this.
> + struct rkisp1_params *params = dev;
>
> if (bls->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_BLS_CTRL,
> @@ -1709,11 +1687,12 @@ rkisp1_ext_params_bls(struct rkisp1_params *params,
> RKISP1_CIF_ISP_BLS_ENA);
> }
>
> -static void
> -rkisp1_ext_params_dpcc(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_dpcc(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_dpcc_config *dpcc = &block->dpcc;
> + const struct rkisp1_ext_params_dpcc_config *dpcc =
> + (const struct rkisp1_ext_params_dpcc_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (dpcc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPCC_MODE,
> @@ -1729,11 +1708,12 @@ rkisp1_ext_params_dpcc(struct rkisp1_params *params,
> RKISP1_CIF_ISP_DPCC_MODE_DPCC_ENABLE);
> }
>
> -static void
> -rkisp1_ext_params_sdg(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_sdg(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_sdg_config *sdg = &block->sdg;
> + const struct rkisp1_ext_params_sdg_config *sdg =
> + (const struct rkisp1_ext_params_sdg_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (sdg->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1749,11 +1729,12 @@ rkisp1_ext_params_sdg(struct rkisp1_params *params,
> 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)
> +static void rkisp1_ext_params_lsc(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_lsc_config *lsc = &block->lsc;
> + const struct rkisp1_ext_params_lsc_config *lsc =
> + (const struct rkisp1_ext_params_lsc_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (lsc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_LSC_CTRL,
> @@ -1769,11 +1750,12 @@ rkisp1_ext_params_lsc(struct rkisp1_params *params,
> RKISP1_CIF_ISP_LSC_CTRL_ENA);
> }
>
> -static void
> -rkisp1_ext_params_awbg(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_awbg(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_awb_gain_config *awbg = &block->awbg;
> + const struct rkisp1_ext_params_awb_gain_config *awbg =
> + (const struct rkisp1_ext_params_awb_gain_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (awbg->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1789,11 +1771,12 @@ rkisp1_ext_params_awbg(struct rkisp1_params *params,
> RKISP1_CIF_ISP_CTRL_ISP_AWB_ENA);
> }
>
> -static void
> -rkisp1_ext_params_flt(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_flt(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_flt_config *flt = &block->flt;
> + const struct rkisp1_ext_params_flt_config *flt =
> + (const struct rkisp1_ext_params_flt_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (flt->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_FILT_MODE,
> @@ -1809,11 +1792,12 @@ rkisp1_ext_params_flt(struct rkisp1_params *params,
> RKISP1_CIF_ISP_FLT_ENA);
> }
>
> -static void
> -rkisp1_ext_params_bdm(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_bdm(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_bdm_config *bdm = &block->bdm;
> + const struct rkisp1_ext_params_bdm_config *bdm =
> + (const struct rkisp1_ext_params_bdm_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (bdm->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DEMOSAIC,
> @@ -1829,11 +1813,12 @@ rkisp1_ext_params_bdm(struct rkisp1_params *params,
> RKISP1_CIF_ISP_DEMOSAIC_BYPASS);
> }
>
> -static void
> -rkisp1_ext_params_ctk(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_ctk(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_ctk_config *ctk = &block->ctk;
> + const struct rkisp1_ext_params_ctk_config *ctk =
> + (const struct rkisp1_ext_params_ctk_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (ctk->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_ctk_enable(params, false);
> @@ -1847,11 +1832,12 @@ rkisp1_ext_params_ctk(struct rkisp1_params *params,
> rkisp1_ctk_enable(params, true);
> }
>
> -static void
> -rkisp1_ext_params_goc(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_goc(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_goc_config *goc = &block->goc;
> + const struct rkisp1_ext_params_goc_config *goc =
> + (const struct rkisp1_ext_params_goc_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (goc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_CTRL,
> @@ -1869,11 +1855,12 @@ rkisp1_ext_params_goc(struct rkisp1_params *params,
> 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)
> +static void rkisp1_ext_params_dpf(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_dpf_config *dpf = &block->dpf;
> + const struct rkisp1_ext_params_dpf_config *dpf =
> + (const struct rkisp1_ext_params_dpf_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (dpf->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_DPF_MODE,
> @@ -1889,20 +1876,22 @@ rkisp1_ext_params_dpf(struct rkisp1_params *params,
> RKISP1_CIF_ISP_DPF_MODE_EN);
> }
>
> -static void
> -rkisp1_ext_params_dpfs(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_dpfs(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_dpf_strength_config *dpfs = &block->dpfs;
> + const struct rkisp1_ext_params_dpf_strength_config *dpfs =
> + (const struct rkisp1_ext_params_dpf_strength_config *)block;
> + struct rkisp1_params *params = dev;
>
> rkisp1_dpf_strength_config(params, &dpfs->config);
> }
>
> -static void
> -rkisp1_ext_params_cproc(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_cproc(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_cproc_config *cproc = &block->cproc;
> + const struct rkisp1_ext_params_cproc_config *cproc =
> + (const struct rkisp1_ext_params_cproc_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (cproc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_C_PROC_CTRL,
> @@ -1918,11 +1907,12 @@ rkisp1_ext_params_cproc(struct rkisp1_params *params,
> RKISP1_CIF_C_PROC_CTR_ENABLE);
> }
>
> -static void
> -rkisp1_ext_params_ie(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_ie(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_ie_config *ie = &block->ie;
> + const struct rkisp1_ext_params_ie_config *ie =
> + (const struct rkisp1_ext_params_ie_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (ie->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_ie_enable(params, false);
> @@ -1936,11 +1926,12 @@ rkisp1_ext_params_ie(struct rkisp1_params *params,
> rkisp1_ie_enable(params, true);
> }
>
> -static void
> -rkisp1_ext_params_awbm(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_awbm(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_awb_meas_config *awbm = &block->awbm;
> + const struct rkisp1_ext_params_awb_meas_config *awbm =
> + (const struct rkisp1_ext_params_awb_meas_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (awbm->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> params->ops->awb_meas_enable(params, &awbm->config,
> @@ -1956,11 +1947,12 @@ rkisp1_ext_params_awbm(struct rkisp1_params *params,
> true);
> }
>
> -static void
> -rkisp1_ext_params_hstm(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_hstm(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_hst_config *hst = &block->hst;
> + const struct rkisp1_ext_params_hst_config *hst =
> + (const struct rkisp1_ext_params_hst_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (hst->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> params->ops->hst_enable(params, &hst->config, false);
> @@ -1974,11 +1966,12 @@ rkisp1_ext_params_hstm(struct rkisp1_params *params,
> 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)
> +static void rkisp1_ext_params_aecm(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_aec_config *aec = &block->aec;
> + const struct rkisp1_ext_params_aec_config *aec =
> + (const struct rkisp1_ext_params_aec_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (aec->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_EXP_CTRL,
> @@ -1994,11 +1987,12 @@ rkisp1_ext_params_aecm(struct rkisp1_params *params,
> RKISP1_CIF_ISP_EXP_ENA);
> }
>
> -static void
> -rkisp1_ext_params_afcm(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_afcm(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_afc_config *afc = &block->afc;
> + const struct rkisp1_ext_params_afc_config *afc =
> + (const struct rkisp1_ext_params_afc_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (afc->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_AFM_CTRL,
> @@ -2014,11 +2008,12 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
> RKISP1_CIF_ISP_AFM_ENA);
> }
>
> -static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_compand_bls(void *dev,
> + const struct v4l2_params_block *block)
> {
> const struct rkisp1_ext_params_compand_bls_config *bls =
> - &block->compand_bls;
> + (const struct rkisp1_ext_params_compand_bls_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (bls->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> @@ -2034,11 +2029,13 @@ static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
> RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> }
>
> -static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void
> +rkisp1_ext_params_compand_expand(void *dev,
> + const struct v4l2_params_block *block)
> {
> const struct rkisp1_ext_params_compand_curve_config *curve =
> - &block->compand_curve;
> + (const struct rkisp1_ext_params_compand_curve_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (curve->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> @@ -2054,11 +2051,13 @@ static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
> RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> }
>
> -static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void
> +rkisp1_ext_params_compand_compress(void *dev,
> + const struct v4l2_params_block *block)
> {
> const struct rkisp1_ext_params_compand_curve_config *curve =
> - &block->compand_curve;
> + (const struct rkisp1_ext_params_compand_curve_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (curve->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> @@ -2074,10 +2073,12 @@ static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
> RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> }
>
> -static void rkisp1_ext_params_wdr(struct rkisp1_params *params,
> - const union rkisp1_ext_params_config *block)
> +static void rkisp1_ext_params_wdr(void *dev,
> + const struct v4l2_params_block *block)
> {
> - const struct rkisp1_ext_params_wdr_config *wdr = &block->wdr;
> + const struct rkisp1_ext_params_wdr_config *wdr =
> + (const struct rkisp1_ext_params_wdr_config *)block;
> + struct rkisp1_params *params = dev;
>
> if (wdr->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
> rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_WDR_CTRL,
> @@ -2093,15 +2094,7 @@ static void rkisp1_ext_params_wdr(struct rkisp1_params *params,
> RKISP1_CIF_ISP_WDR_CTRL_ENABLE);
> }
>
> -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;
> - unsigned int features;
> -} rkisp1_ext_params_handlers[] = {
> +static const struct v4l2_params_handler rkisp1_ext_params_handlers[] = {
> [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
> .size = sizeof(struct rkisp1_ext_params_bls_config),
> .handler = rkisp1_ext_params_bls,
> @@ -2224,18 +2217,18 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
>
> /* 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;
> + const struct v4l2_params_handler *block_handler;
> + const struct v4l2_params_block *block;
>
> - block = (const union rkisp1_ext_params_config *)
> + block = (const struct v4l2_params_block *)
> &cfg->data[block_offset];
And with the union this can be block.data = &cfg->data[block_offset]
> - block_offset += block->header.size;
> + block_offset += block->size;
>
> /*
> * Make sure the block is supported by the platform and in the
> * list of groups to configure.
> */
> - block_handler = &rkisp1_ext_params_handlers[block->header.type];
> + block_handler = &rkisp1_ext_params_handlers[block->type];
> if (!(block_handler->group & block_group_mask))
> continue;
>
> @@ -2245,10 +2238,10 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
>
> block_handler->handler(params, block);
Perhaps the calling of the handlers could be moved to v4l2-params.c too?
>
> - if (block->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)
> - params->enabled_blocks &= ~BIT(block->header.type);
> - else if (block->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE)
> - params->enabled_blocks |= BIT(block->header.type);
> + if (block->flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)
> + params->enabled_blocks &= ~BIT(block->type);
> + else if (block->flags & RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE)
> + params->enabled_blocks |= BIT(block->type);
> }
> }
>
> @@ -2641,36 +2634,51 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
> spin_unlock_irq(¶ms->config_lock);
> }
>
> +static int
> +rkisp1_ext_params_validate_buffer(struct device *dev,
> + const struct v4l2_params_buffer *buffer)
> +{
> + /* Only v1 is supported at the moment. */
> + if (buffer->version != RKISP1_EXT_PARAM_BUFFER_V1) {
> + dev_dbg(dev, "Unsupported extensible format version: %u\n",
> + buffer->version);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Same with version validation. I like the idea of having a driver-specific validation callback, but
if all that's different between the version validation is the macro I think ideally v4l2-params.c
could check it - perhaps we could mandate that new versions of the buffers must be backwards
compatible, and then we could pass a "latest supported version number" to
v4l2_params_buffer_validate() to check?
> +
> +static int
> +rkisp1_ext_params_validate_block(struct device *dev,
> + const struct v4l2_params_block *block)
> +{
> + if ((block->flags & (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> + RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) ==
> + (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> + RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) {
> + dev_dbg(dev, "Invalid parameters block flags\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
Centrally checking these flags at least would be nice since they're common across all three
implementations.
> +
> 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);
> - size_t header_size = offsetof(struct rkisp1_ext_params_cfg, data);
> struct rkisp1_ext_params_cfg *cfg = params_buf->cfg;
> size_t payload_size = vb2_get_plane_payload(vb, 0);
> struct rkisp1_ext_params_cfg *usr_cfg =
> vb2_plane_vaddr(&vbuf->vb2_buf, 0);
> - size_t block_offset = 0;
> - size_t cfg_size;
> -
> - /*
> - * Validate the buffer payload size before copying the parameters. The
> - * payload has to be smaller than the destination buffer size and larger
> - * than the header size.
> - */
> - if (payload_size > params->metafmt->buffersize) {
> - dev_dbg(params->rkisp1->dev,
> - "Too large buffer payload size %zu\n", payload_size);
> - return -EINVAL;
> - }
> + int ret;
>
> - if (payload_size < header_size) {
> - dev_dbg(params->rkisp1->dev,
> - "Buffer payload %zu smaller than header size %zu\n",
> - payload_size, header_size);
> - return -EINVAL;
> - }
> + ret = v4l2_params_buffer_validate(params->rkisp1->dev, vb,
> + params->metafmt->buffersize,
> + rkisp1_ext_params_validate_buffer);
> + if (ret)
> + return ret;
>
> /*
> * Copy the parameters buffer to the internal scratch buffer to avoid
> @@ -2678,71 +2686,11 @@ static int rkisp1_params_prepare_ext_params(struct rkisp1_params *params,
> */
> memcpy(cfg, usr_cfg, payload_size);
>
> - /* Only v1 is supported at the moment. */
> - if (cfg->version != RKISP1_EXT_PARAM_BUFFER_V1) {
> - dev_dbg(params->rkisp1->dev,
> - "Unsupported extensible format version: %u\n",
> - cfg->version);
> - return -EINVAL;
> - }
> -
> - /* Validate the size reported in the parameters buffer header. */
> - cfg_size = header_size + cfg->data_size;
> - if (cfg_size != payload_size) {
> - dev_dbg(params->rkisp1->dev,
> - "Data size %zu different than buffer payload size %zu\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 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;
> - }
> -
> - if ((block->flags & (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) ==
> - (RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE |
> - RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE)) {
> - dev_dbg(params->rkisp1->dev,
> - "Invalid parameters block flags\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;
> - }
> -
> - if (cfg_size) {
> - dev_dbg(params->rkisp1->dev,
> - "Unexpected data after the parameters buffer end\n");
> - return -EINVAL;
> - }
> -
> - return 0;
> + return v4l2_params_blocks_validate(params->rkisp1->dev,
> + (struct v4l2_params_buffer *)cfg,
> + rkisp1_ext_params_handlers,
> + ARRAY_SIZE(rkisp1_ext_params_handlers),
> + rkisp1_ext_params_validate_block);
> }
The end result is really great - makes the driver much simpler!
>
> static int rkisp1_params_vb2_buf_prepare(struct vb2_buffer *vb)
> @@ -2842,7 +2790,7 @@ static int rkisp1_params_ctrl_init(struct rkisp1_params *params)
> v4l2_ctrl_handler_init(¶ms->ctrls, 1);
>
> for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> - const struct rkisp1_ext_params_handler *block_handler;
> + const struct v4l2_params_handler *block_handler;
>
> block_handler = &rkisp1_ext_params_handlers[i];
> ctrl_config.max |= BIT(i);
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 7/8] media: amlogic-c3: Use v4l2-params for validation
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (5 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 6/8] media: rkisp1: Use v4l2-params for validation Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-10 7:13 ` Keke Li
2025-07-08 10:40 ` [PATCH 8/8] media: Documentation: kapi: Add v4l2 extensible parameters Jacopo Mondi
2025-07-10 2:19 ` [PATCH 0/8] media: Introduce V4L2 " Keke Li
8 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Convert c3-ispa-params.c to use the new types fro block handlers
defined in v4l2-params.h and use the new helpers from v4l2-params.c
to remove boilerplate code from the driver.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
.../media/platform/amlogic/c3/isp/c3-isp-params.c | 272 ++++++++-------------
1 file changed, 103 insertions(+), 169 deletions(-)
diff --git a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
index c80667dd766210d2b2e1ee60c8254a5814b9d81b..9555a657fd93e89c846df029af9de2745d2a5d7c 100644
--- a/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
+++ b/drivers/media/platform/amlogic/c3/isp/c3-isp-params.c
@@ -9,64 +9,25 @@
#include <media/v4l2-ioctl.h>
#include <media/v4l2-mc.h>
+#include <media/v4l2-params.h>
#include <media/videobuf2-vmalloc.h>
#include "c3-isp-common.h"
#include "c3-isp-regs.h"
-/*
- * union c3_isp_params_block - Generalisation of a parameter block
- *
- * This union allows the driver to treat a block as a generic struct to this
- * union and safely access the header and block-specific struct without having
- * to resort to casting. The header member is accessed first, and the type field
- * checked which allows the driver to determine which of the other members
- * should be used.
- *
- * @header: The shared header struct embedded as the first member
- * of all the possible other members. This member would be
- * accessed first and the type field checked to determine
- * which of the other members should be accessed.
- * @awb_gains: For header.type == C3_ISP_PARAMS_BLOCK_AWB_GAINS
- * @awb_cfg: For header.type == C3_ISP_PARAMS_BLOCK_AWB_CONFIG
- * @ae_cfg: For header.type == C3_ISP_PARAMS_BLOCK_AE_CONFIG
- * @af_cfg: For header.type == C3_ISP_PARAMS_BLOCK_AF_CONFIG
- * @pst_gamma: For header.type == C3_ISP_PARAMS_BLOCK_PST_GAMMA
- * @ccm: For header.type == C3_ISP_PARAMS_BLOCK_CCM
- * @csc: For header.type == C3_ISP_PARAMS_BLOCK_CSC
- * @blc: For header.type == C3_ISP_PARAMS_BLOCK_BLC
- */
-union c3_isp_params_block {
- struct c3_isp_params_block_header header;
- struct c3_isp_params_awb_gains awb_gains;
- struct c3_isp_params_awb_config awb_cfg;
- struct c3_isp_params_ae_config ae_cfg;
- struct c3_isp_params_af_config af_cfg;
- struct c3_isp_params_pst_gamma pst_gamma;
- struct c3_isp_params_ccm ccm;
- struct c3_isp_params_csc csc;
- struct c3_isp_params_blc blc;
-};
-
-typedef void (*c3_isp_block_handler)(struct c3_isp_device *isp,
- const union c3_isp_params_block *block);
-
-struct c3_isp_params_handler {
- size_t size;
- c3_isp_block_handler handler;
-};
-
#define to_c3_isp_params_buffer(vbuf) \
container_of(vbuf, struct c3_isp_params_buffer, vb)
/* Hardware configuration */
-static void c3_isp_params_cfg_awb_gains(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_awb_gains(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_awb_gains *awb_gains = &block->awb_gains;
+ const struct c3_isp_params_awb_gains *awb_gains =
+ (const struct c3_isp_params_awb_gains *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_BEO_CTRL,
ISP_TOP_BEO_CTRL_WB_EN_MASK,
ISP_TOP_BEO_CTRL_WB_DIS);
@@ -89,7 +50,7 @@ static void c3_isp_params_cfg_awb_gains(struct c3_isp_device *isp,
ISP_LSWB_WB_GAIN2_IR_GAIN_MASK,
ISP_LSWB_WB_GAIN2_IR_GAIN(awb_gains->gb_gain));
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_BEO_CTRL,
ISP_TOP_BEO_CTRL_WB_EN_MASK,
ISP_TOP_BEO_CTRL_WB_EN);
@@ -151,12 +112,14 @@ static void c3_isp_params_awb_cood(struct c3_isp_device *isp,
ISP_AWB_IDX_DATA_VIDX_DATA(cfg->vert_coord[i]));
}
-static void c3_isp_params_cfg_awb_config(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_awb_config(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_awb_config *awb_cfg = &block->awb_cfg;
+ const struct c3_isp_params_awb_config *awb_cfg =
+ (const struct c3_isp_params_awb_config *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AWB_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AWB_STAT_DIS);
@@ -205,7 +168,7 @@ static void c3_isp_params_cfg_awb_config(struct c3_isp_device *isp,
c3_isp_params_awb_wt(isp, awb_cfg);
c3_isp_params_awb_cood(isp, awb_cfg);
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AWB_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AWB_STAT_EN);
@@ -268,12 +231,14 @@ static void c3_isp_params_ae_cood(struct c3_isp_device *isp,
ISP_AE_IDX_DATA_VIDX_DATA(cfg->vert_coord[i]));
}
-static void c3_isp_params_cfg_ae_config(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_ae_config(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_ae_config *ae_cfg = &block->ae_cfg;
+ const struct c3_isp_params_ae_config *ae_cfg =
+ (const struct c3_isp_params_ae_config *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AE_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AE_STAT_DIS);
@@ -303,7 +268,7 @@ static void c3_isp_params_cfg_ae_config(struct c3_isp_device *isp,
c3_isp_params_ae_wt(isp, ae_cfg);
c3_isp_params_ae_cood(isp, ae_cfg);
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AE_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AE_STAT_EN);
@@ -326,12 +291,14 @@ static void c3_isp_params_af_cood(struct c3_isp_device *isp,
ISP_AF_IDX_DATA_VIDX_DATA(cfg->vert_coord[i]));
}
-static void c3_isp_params_cfg_af_config(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_af_config(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_af_config *af_cfg = &block->af_cfg;
+ const struct c3_isp_params_af_config *af_cfg =
+ (const struct c3_isp_params_af_config *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AF_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AF_STAT_DIS);
@@ -351,20 +318,22 @@ static void c3_isp_params_cfg_af_config(struct c3_isp_device *isp,
c3_isp_params_af_cood(isp, af_cfg);
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_3A_STAT_CRTL,
ISP_TOP_3A_STAT_CRTL_AF_STAT_EN_MASK,
ISP_TOP_3A_STAT_CRTL_AF_STAT_EN);
}
-static void c3_isp_params_cfg_pst_gamma(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_pst_gamma(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_pst_gamma *gm = &block->pst_gamma;
+ const struct c3_isp_params_pst_gamma *gm =
+ (const struct c3_isp_params_pst_gamma *)block;
+ struct c3_isp_device *isp = dev;
unsigned int base;
unsigned int i;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_PST_GAMMA_EN_MASK,
ISP_TOP_BED_CTRL_PST_GAMMA_DIS);
@@ -393,19 +362,21 @@ static void c3_isp_params_cfg_pst_gamma(struct c3_isp_device *isp,
}
}
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_PST_GAMMA_EN_MASK,
ISP_TOP_BED_CTRL_PST_GAMMA_EN);
}
/* Configure 3 x 3 ccm matrix */
-static void c3_isp_params_cfg_ccm(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_ccm(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_ccm *ccm = &block->ccm;
+ const struct c3_isp_params_ccm *ccm =
+ (const struct c3_isp_params_ccm *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_CCM_EN_MASK,
ISP_TOP_BED_CTRL_CCM_DIS);
@@ -442,19 +413,21 @@ static void c3_isp_params_cfg_ccm(struct c3_isp_device *isp,
ISP_CCM_MTX_22_23_RS_MTX_22_MASK,
ISP_CCM_MTX_22_23_RS_MTX_22(ccm->matrix[2][2]));
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_CCM_EN_MASK,
ISP_TOP_BED_CTRL_CCM_EN);
}
/* Configure color space conversion matrix parameters */
-static void c3_isp_params_cfg_csc(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_csc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_csc *csc = &block->csc;
+ const struct c3_isp_params_csc *csc =
+ (const struct c3_isp_params_csc *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_CM0_EN_MASK,
ISP_TOP_BED_CTRL_CM0_DIS);
@@ -491,19 +464,21 @@ static void c3_isp_params_cfg_csc(struct c3_isp_device *isp,
ISP_CM0_COEF22_OUP_OFST0_MTX_22_MASK,
ISP_CM0_COEF22_OUP_OFST0_MTX_22(csc->matrix[2][2]));
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_BED_CTRL,
ISP_TOP_BED_CTRL_CM0_EN_MASK,
ISP_TOP_BED_CTRL_CM0_EN);
}
/* Set blc offset of each color channel */
-static void c3_isp_params_cfg_blc(struct c3_isp_device *isp,
- const union c3_isp_params_block *block)
+static void c3_isp_params_cfg_blc(void *dev,
+ const struct v4l2_params_block *block)
{
- const struct c3_isp_params_blc *blc = &block->blc;
+ const struct c3_isp_params_blc *blc =
+ (const struct c3_isp_params_blc *)block;
+ struct c3_isp_device *isp = dev;
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_DISABLE) {
c3_isp_update_bits(isp, ISP_TOP_BEO_CTRL,
ISP_TOP_BEO_CTRL_BLC_EN_MASK,
ISP_TOP_BEO_CTRL_BLC_DIS);
@@ -517,13 +492,13 @@ static void c3_isp_params_cfg_blc(struct c3_isp_device *isp,
ISP_LSWB_BLC_OFST1_GB_OFST(blc->gb_ofst) |
ISP_LSWB_BLC_OFST1_B_OFST(blc->b_ofst));
- if (block->header.flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
+ if (block->flags & C3_ISP_PARAMS_BLOCK_FL_ENABLE)
c3_isp_update_bits(isp, ISP_TOP_BEO_CTRL,
ISP_TOP_BEO_CTRL_BLC_EN_MASK,
ISP_TOP_BEO_CTRL_BLC_EN);
}
-static const struct c3_isp_params_handler c3_isp_params_handlers[] = {
+static const struct v4l2_params_handler c3_isp_params_handlers[] = {
[C3_ISP_PARAMS_BLOCK_AWB_GAINS] = {
.size = sizeof(struct c3_isp_params_awb_gains),
.handler = c3_isp_params_cfg_awb_gains,
@@ -568,16 +543,16 @@ static void c3_isp_params_cfg_blocks(struct c3_isp_params *params)
/* Walk the list of parameter blocks and process them */
while (block_offset < config->data_size) {
- const struct c3_isp_params_handler *block_handler;
- const union c3_isp_params_block *block;
+ const struct v4l2_params_handler *block_handler;
+ const struct v4l2_params_block *block;
- block = (const union c3_isp_params_block *)
+ block = (const struct v4l2_params_block *)
&config->data[block_offset];
- block_handler = &c3_isp_params_handlers[block->header.type];
+ block_handler = &c3_isp_params_handlers[block->type];
block_handler->handler(params->isp, block);
- block_offset += block->header.size;
+ block_offset += block->size;
}
}
@@ -766,31 +741,49 @@ static void c3_isp_params_vb2_buf_queue(struct vb2_buffer *vb)
list_add_tail(&buf->list, ¶ms->pending);
}
+static int
+c3_isp_params_validate_buffer(struct device *dev,
+ const struct v4l2_params_buffer *buffer)
+{
+ /* Only v0 is supported at the moment */
+ if (buffer->version != C3_ISP_PARAMS_BUFFER_V0) {
+ dev_dbg(dev, "Invalid params buffer version: %u\n",
+ buffer->version);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+c3_isp_params_validate_block(struct device *dev,
+ const struct v4l2_params_block *block)
+{
+ 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(dev, "Invalid parameters block flags\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
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;
- }
+ struct c3_isp_params_cfg *cfg = buf->cfg;
+ int ret;
- /* 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;
- }
+ ret = v4l2_params_buffer_validate(params->isp->dev, vb,
+ params->vfmt.fmt.meta.buffersize,
+ c3_isp_params_validate_buffer);
+ if (ret)
+ return ret;
/*
* Use the internal scratch buffer to avoid userspace modifying
@@ -798,70 +791,11 @@ static int c3_isp_params_vb2_buf_prepare(struct vb2_buffer *vb)
*/
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;
+ return v4l2_params_blocks_validate(params->isp->dev,
+ (struct v4l2_params_buffer *)cfg,
+ c3_isp_params_handlers,
+ ARRAY_SIZE(c3_isp_params_handlers),
+ c3_isp_params_validate_block);
}
static int c3_isp_params_vb2_buf_init(struct vb2_buffer *vb)
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 7/8] media: amlogic-c3: Use v4l2-params for validation
2025-07-08 10:40 ` [PATCH 7/8] media: amlogic-c3: " Jacopo Mondi
@ 2025-07-10 7:13 ` Keke Li
0 siblings, 0 replies; 27+ messages in thread
From: Keke Li @ 2025-07-10 7:13 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
On 2025/7/8 18:40, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> Convert c3-ispa-params.c to use the new types fro block handlers
> defined in v4l2-params.h and use the new helpers from v4l2-params.c
> to remove boilerplate code from the driver.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> .../media/platform/amlogic/c3/isp/c3-isp-params.c | 272 ++++++++-------------
> 1 file changed, 103 insertions(+), 169 deletions(-)
Reviewed-by: Keke Li <keke.li@amlogic.com>
Best regards,
Keke
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 8/8] media: Documentation: kapi: Add v4l2 extensible parameters
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (6 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 7/8] media: amlogic-c3: " Jacopo Mondi
@ 2025-07-08 10:40 ` Jacopo Mondi
2025-07-09 14:59 ` Dan Scally
2025-07-10 2:19 ` [PATCH 0/8] media: Introduce V4L2 " Keke Li
8 siblings, 1 reply; 27+ messages in thread
From: Jacopo Mondi @ 2025-07-08 10:40 UTC (permalink / raw)
To: Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel,
Jacopo Mondi
Add to the driver-api documentation the v4l2-params.h types and
helpers documentation.
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
Documentation/driver-api/media/v4l2-core.rst | 1 +
Documentation/driver-api/media/v4l2-params.rst | 5 +++++
MAINTAINERS | 1 +
3 files changed, 7 insertions(+)
diff --git a/Documentation/driver-api/media/v4l2-core.rst b/Documentation/driver-api/media/v4l2-core.rst
index ad987c34ad2a8460bb95e97adc4d850d624e0b81..2d7793298c6a2046bdd59b185a411e092b659d52 100644
--- a/Documentation/driver-api/media/v4l2-core.rst
+++ b/Documentation/driver-api/media/v4l2-core.rst
@@ -27,3 +27,4 @@ Video4Linux devices
v4l2-common
v4l2-tveeprom
v4l2-jpeg
+ v4l2-params
diff --git a/Documentation/driver-api/media/v4l2-params.rst b/Documentation/driver-api/media/v4l2-params.rst
new file mode 100644
index 0000000000000000000000000000000000000000..8d2a5f004d21dfc3a81255cabbc6b7cce588db71
--- /dev/null
+++ b/Documentation/driver-api/media/v4l2-params.rst
@@ -0,0 +1,5 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+V4L2 extensible parameters kAPI
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+.. kernel-doc:: include/media/v4l2-params.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 3d9a8e06c59eb08360d1e8eea85e450a15ee95af..f03c10092a891a06052484b691409f0c459de87d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25972,6 +25972,7 @@ V4L2 EXTENSIBLE PARAMETERS FORMAT
M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
L: linux-media@vger.kernel.org
S: Maintained
+F: Documentation/driver-api/media/v4l2-params.rst
F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
F: drivers/media/v4l2-core/v4l2-params.c
F: include/media/v4l2-params.h
--
2.49.0
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PATCH 8/8] media: Documentation: kapi: Add v4l2 extensible parameters
2025-07-08 10:40 ` [PATCH 8/8] media: Documentation: kapi: Add v4l2 extensible parameters Jacopo Mondi
@ 2025-07-09 14:59 ` Dan Scally
0 siblings, 0 replies; 27+ messages in thread
From: Dan Scally @ 2025-07-09 14:59 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart, Keke Li,
Mauro Carvalho Chehab, Heiko Stuebner, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 08/07/2025 11:40, Jacopo Mondi wrote:
> Add to the driver-api documentation the v4l2-params.h types and
> helpers documentation.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
> Documentation/driver-api/media/v4l2-core.rst | 1 +
> Documentation/driver-api/media/v4l2-params.rst | 5 +++++
> MAINTAINERS | 1 +
> 3 files changed, 7 insertions(+)
>
> diff --git a/Documentation/driver-api/media/v4l2-core.rst b/Documentation/driver-api/media/v4l2-core.rst
> index ad987c34ad2a8460bb95e97adc4d850d624e0b81..2d7793298c6a2046bdd59b185a411e092b659d52 100644
> --- a/Documentation/driver-api/media/v4l2-core.rst
> +++ b/Documentation/driver-api/media/v4l2-core.rst
> @@ -27,3 +27,4 @@ Video4Linux devices
> v4l2-common
> v4l2-tveeprom
> v4l2-jpeg
> + v4l2-params
> diff --git a/Documentation/driver-api/media/v4l2-params.rst b/Documentation/driver-api/media/v4l2-params.rst
> new file mode 100644
> index 0000000000000000000000000000000000000000..8d2a5f004d21dfc3a81255cabbc6b7cce588db71
> --- /dev/null
> +++ b/Documentation/driver-api/media/v4l2-params.rst
> @@ -0,0 +1,5 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +V4L2 extensible parameters kAPI
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +.. kernel-doc:: include/media/v4l2-params.h
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d9a8e06c59eb08360d1e8eea85e450a15ee95af..f03c10092a891a06052484b691409f0c459de87d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25972,6 +25972,7 @@ V4L2 EXTENSIBLE PARAMETERS FORMAT
> M: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> L: linux-media@vger.kernel.org
> S: Maintained
> +F: Documentation/driver-api/media/v4l2-params.rst
> F: Documentation/userspace-api/media/v4l/extensible-parameters.rst
> F: drivers/media/v4l2-core/v4l2-params.c
> F: include/media/v4l2-params.h
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/8] media: Introduce V4L2 extensible parameters
2025-07-08 10:40 [PATCH 0/8] media: Introduce V4L2 extensible parameters Jacopo Mondi
` (7 preceding siblings ...)
2025-07-08 10:40 ` [PATCH 8/8] media: Documentation: kapi: Add v4l2 extensible parameters Jacopo Mondi
@ 2025-07-10 2:19 ` Keke Li
8 siblings, 0 replies; 27+ messages in thread
From: Keke Li @ 2025-07-10 2:19 UTC (permalink / raw)
To: Jacopo Mondi, Dafna Hirschfeld, Laurent Pinchart,
Mauro Carvalho Chehab, Heiko Stuebner, Dan Scally, Sakari Ailus
Cc: linux-kernel, linux-media, linux-rockchip, linux-arm-kernel
Hi Jacopo
On 2025/7/8 18:40, Jacopo Mondi wrote:
> [ EXTERNAL EMAIL ]
>
> Extensible parameters meta formats have been introduced in the Linux
> kernel v6.12 initially to support different revision of the RkISP1 ISP
> implemented in different SoC. In order to avoid breaking userspace
> everytime an ISP configuration block is added or modified in the uAPI
> these new formats, which are versionated and extensible by their
> definition have been introduced.
>
> See for reference:
> e9d05e9d5db1 ("media: uapi: rkisp1-config: Add extensible params format")
> 6c53a7b68c5d ("media: rkisp1: Implement extensible params support")
>
> The Amlogic C3 ISP driver followed shortly, introducing an extensible
> format for the ISP configuration:
>
> 6d406187ebc0 ("media: uapi: Add stats info and parameters buffer for C3 ISP")
>
> with a very similar, if not identical, implementation of the routines to
> validate and handle the ISP configuration in the ISP driver in the
> c3-isp-params.c file.
>
> fb2e135208f3 ("media: platform: Add C3 ISP driver")
>
> With the recent upstreaming attempt of the Mali C55 ISP driver from Dan,
> a third user of extensible parameters is going to be itroduced in the
> kernel, duplicating again in the driver the procedure for validating and
> handling the ISP configuration blocks
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20250624-c55-v10-15-54f3d4196990@ideasonboard.com/
>
> To avoid duplicating again the validation routines and common types
> definition, this series introduces v4l2-params.c/.h for the kAPI
> and v4l2-extensible-params.h for the uAPI and re-organize the RkISP1
> and Amlogic C3 drivers to use the common types and the helper validation
> routines.
>
> If the here proposed approach is accepted, I propose to rebase the Mali
> C55 driver on top of this series, to use the new common types and
> helpers.
>
> I have been able to test this on RkISP1 but not on C3.
> Keke: would you be able to give this series a try and see what happens ?
I have tested this patch series on C3, the result is OK.
Thanks.
>
> Media CI pipeline:
> https://gitlab.freedesktop.org/linux-media/users/jmondi/-/pipelines/1465950
>
> Thanks
> j
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
> Jacopo Mondi (8):
> media: uapi: Introduce V4L2 extensible params
> media: uapi: Convert RkISP1 to V4L2 extensible params
> media: uapi: Convert Amlogic C3 to V4L2 extensible params
> media: Documentation: uapi: Add V4L2 extensible parameters
> media: v4l2-common: Introduce v4l2-params.c
> media: rkisp1: Use v4l2-params for validation
> media: amlogic-c3: Use v4l2-params for validation
> media: Documentation: kapi: Add v4l2 extensible parameters
>
> Documentation/driver-api/media/v4l2-core.rst | 1 +
> Documentation/driver-api/media/v4l2-params.rst | 5 +
> .../media/v4l/extensible-parameters.rst | 89 +++++
> .../userspace-api/media/v4l/meta-formats.rst | 1 +
> MAINTAINERS | 10 +
> .../media/platform/amlogic/c3/isp/c3-isp-params.c | 272 ++++++---------
> .../media/platform/rockchip/rkisp1/rkisp1-params.c | 364 +++++++++------------
> drivers/media/v4l2-core/Makefile | 3 +-
> drivers/media/v4l2-core/v4l2-params.c | 106 ++++++
> include/media/v4l2-params.h | 166 ++++++++++
> include/uapi/linux/media/amlogic/c3-isp-config.h | 45 +--
> include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++
> include/uapi/linux/rkisp1-config.h | 60 +---
> 13 files changed, 775 insertions(+), 453 deletions(-)
> ---
> base-commit: a8598c7de1bcd94461ca54c972efa9b4ea501fb9
> change-id: 20250701-extensible-parameters-validation-c831f7f5cc0b
>
> Best regards,
> --
> Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 27+ messages in thread