| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f64bdf69-bc06-4c83-9e7f-5de18fb64d47@ideasonboard.com>
Date: Mon, 6 Oct 2025 09:17:25 +0100
From: Dan Scally <dan.scally@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Dafna Hirschfeld <dafna@...tmail.com>, Keke Li <keke.li@...ogic.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Heiko Stuebner
<heiko@...ech.de>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
Antoine Bouyer <antoine.bouyer@....com>, linux-kernel@...r.kernel.org,
linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v5 2/8] media: uapi: Convert RkISP1 to V4L2 extensible
params
Hi Laurent, Jacopo
On 05/10/2025 01:18, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Sep 15, 2025 at 07:18:11PM +0200, Jacopo Mondi wrote:
>> 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.
>>
>> Reviewed-by: Daniel Scally <dan.scally@...asonboard.com>
>> Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
>
>> ---
>> include/uapi/linux/rkisp1-config.h | 104 ++++++++-----------------------------
>> 1 file changed, 22 insertions(+), 82 deletions(-)
>>
>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
>> index 3b060ea6eed71b87d79abc8401eae4e9c9f5323a..b90d94d3a852fb0af0fe447649487e9e80aca795 100644
>> --- a/include/uapi/linux/rkisp1-config.h
>> +++ b/include/uapi/linux/rkisp1-config.h
>> @@ -7,8 +7,13 @@
>> #ifndef _UAPI_RKISP1_CONFIG_H
>> #define _UAPI_RKISP1_CONFIG_H
>>
>> +#ifdef __KERNEL__
>> +#include <linux/build_bug.h>
>> +#endif /* __KERNEL__ */
>> #include <linux/types.h>
>>
>> +#include <linux/media/v4l2-isp.h>
>> +
>> /* Defect Pixel Cluster Detection */
>> #define RKISP1_CIF_ISP_MODULE_DPCC (1U << 0)
>> /* Black Level Subtraction */
>> @@ -1158,79 +1163,26 @@ enum rkisp1_ext_params_block_type {
>> RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR,
>> };
>>
>> -#define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE (1U << 0)
>> -#define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE (1U << 1)
>> +/* For backward compatibility */
>> +#define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE V4L2_PARAMS_FL_BLOCK_DISABLE
>> +#define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE V4L2_PARAMS_FL_BLOCK_ENABLE
>>
>> /* A bitmask of parameters blocks supported on the current hardware. */
>> #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_header`.
>> *
>> - * 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
>> - * 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_*.
>> - *
>> - * When userspace wants to configure and enable an ISP block it shall fully
>> - * populate the block configuration and set the
>> - * RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE bit in the @flags field.
>> - *
>> - * 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.
>> - *
>> - * 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.
>> - *
>> - * Setting both the RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE and
>> - * 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.
>> - *
>> - * For example:
>> + * interpreted by the driver.
>> *
>> - * .. code-block:: c
>> - *
>> - * void populate_bls(struct rkisp1_ext_params_block_header *block) {
>> - * struct rkisp1_ext_params_bls_config *bls =
>> - * (struct rkisp1_ext_params_bls_config *)block;
>> - *
>> - * bls->header.type = RKISP1_EXT_PARAMS_BLOCK_ID_BLS;
>> - * bls->header.flags = RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE;
>> - * bls->header.size = sizeof(*bls);
>> - *
>> - * bls->config.enable_auto = 0;
>> - * bls->config.fixed_val.r = blackLevelRed_;
>> - * bls->config.fixed_val.gr = blackLevelGreenR_;
>> - * 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
>> + * The flags field is a bitmask of per-block flags RKISP1_EXT_PARAMS_FL_*.
>> */
>> -struct rkisp1_ext_params_block_header {
>> - __u16 type;
>> - __u16 flags;
>> - __u32 size;
>> -};
>> +#define rkisp1_ext_params_block_header v4l2_params_block_header
>>
>> /**
>> * struct rkisp1_ext_params_bls_config - RkISP1 extensible params BLS config
>> @@ -1594,21 +1546,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 +1562,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 +1611,11 @@ struct rkisp1_ext_params_cfg {
>> __u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
>> };
>>
>> +#ifdef __KERNEL__
>> +/* 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 /* __KERNEL__ */
This is where I was thinking v4l2_params_buffer_size could be used:
static assert(sizeof(struct rkisp1_ext_params_cfg) ==
v4l2_params_buffer_size(RKISP1_EXT_PARAMS_MAX_SIZE))
Dan
>> +
>> #endif /* _UAPI_RKISP1_CONFIG_H */
>>
>
Powered by blists - more mailing lists