Hi Hans, Laurent,
On Tue, Jul 30, 2024 at 02:37:04PM +0200, Hans Verkuil wrote:
> On 30/07/2024 14:18, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Tue, Jul 30, 2024 at 02:11:12PM +0200, Hans Verkuil wrote:
> >> On 24/07/2024 10:49, Jacopo Mondi wrote:
> >>> Add to the rkisp1-config.h header data types and documentation of
> >>> the extensible parameters format.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >>> Reviewed-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> >>> ---
> >>> include/uapi/linux/rkisp1-config.h | 489 +++++++++++++++++++++++++++++
> >>> 1 file changed, 489 insertions(+)
> >>>
> >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >>> index 6eeaf8bf2362..00b09c92cca7 100644
> >>> --- a/include/uapi/linux/rkisp1-config.h
> >>> +++ b/include/uapi/linux/rkisp1-config.h
> >>> @@ -996,4 +996,493 @@ struct rkisp1_stat_buffer {
> >>> struct rkisp1_cif_isp_stat params;
> >>> };
> >>>
> >>> +/*---------- PART3: Extensible Configuration Parameters ------------*/
> >>> +
> >>> +/**
> >>> + * enum rkisp1_ext_params_block_type - RkISP1 extensible params block type
> >>> + *
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS: Black level subtraction
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC: Defect pixel cluster correction
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG: Sensor de-gamma
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAIN: Auto white balance gains
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT: ISP filtering
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM: Bayer de-mosaic
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK: Cross-talk correction
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC: Gamma out correction
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF: De-noise pre-filter
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGTH: De-noise pre-filter strength
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC: Color processing
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_IE: Image effects
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC: Lens shading correction
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS: Auto white balance statistics
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics
> >>> + */
> >>> +enum rkisp1_ext_params_block_type {
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_SDG,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_GAIN,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_FLT,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_BDM,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_CTK,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_GOC,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_DPF_STRENGTH,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_CPROC,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_IE,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_LSC,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_AWB_MEAS,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS,
> >>> + RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS,
> >>> +};
> >>> +
> >>> +/**
> >>> + * enum rkisp1_ext_params_block_enable - RkISP1 extensible parameter block
> >>> + * enable flags
> >>> + *
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_DISABLE: Disable the HW block
> >>> + * @RKISP1_EXT_PARAMS_BLOCK_ENABLE: Enable the HW block
> >>> + */
> >>> +enum rkisp1_ext_params_block_enable {
> >>> + RKISP1_EXT_PARAMS_BLOCK_DISABLE,
> >>> + RKISP1_EXT_PARAMS_BLOCK_ENABLE,
> >>> +};
> >>> +
> >>> +/**
> >>> + * struct rkisp1_ext_params_block_header - RkISP1 extensible 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:`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 @enable field specifies the ISP block enablement state. The possible
> >>> + * enablement states are enumerated by :c:type:`rkisp1_ext_params_block_enable`.
> >>> + * When userspace needs to configure and enable an ISP block it shall fully
> >>> + * populate the block configuration and the @enable flag shall be set to
> >>> + * RKISP1_EXT_PARAMS_BLOCK_ENABLE. When userspace simply wants to disable the
> >>> + * ISP block the @enable flag shall be set to RKISP1_EXT_PARAMS_BLOCK_DISABLE.
> >>> + * 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 set the @enable flag to
> >>> + * RKISP1_EXT_PARAMS_BLOCK_ENABLE.
> >>> + *
> >>> + * Userspace is responsible for correctly populating the parameters block header
> >>> + * fields (@type, @enable and @size) and the block-specific parameters.
> >>> + *
> >>> + * For example:
> >>> + *
> >>> + * .. 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.enable = RKISP1_EXT_PARAMS_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`
> >>> + * @enable: The block enable flag, see
> >>> + * :c:type:`rkisp1_ext_params_block_enable`
> >>> + * @size: Size (in bytes) of the parameters block, including this header
> >>> + */
> >>> +struct rkisp1_ext_params_block_header {
> >>> + __u16 type;
> >>> + __u16 enable;
> >>> + __u16 size;
> >>
> >> I would suggest changing this to '__u32 size;'. It ensures the header is8 bytes
> >> long (much nicer than 6), and if there is ever a block > 65535, then it is supported.
> >
> > I'm pretty confident we will never need a block size larger than 64kB.
>
> Hmm, famous last words :-)
>
> > That would mean more than 64kB of data written to hardware
> > registers/SRAM for a single processing block, and it would be incredibly
> > expensive in terms of hardware. Keeping size a __u16 means we have two
> > bytes of reserved space we could possibly use later, which may come
> > handy.
>
> i would prefer to change the size to a u32, but rename the 'enable' field
> to 'flags', and assign bit 0 to the enable/disable bit. This is a bit
> more flexible IMHO and allows for 15 bits to encode additional data.
>
> Blocks > 64kB could still be supported in the future by defining
> > a new version of the parameters format (RKISP1_EXT_PARAM_BUFFER_V2)
> > without needing a different 4CC.
...or making of use the existing padding. Shouldn't that be a reserved
field btw.?
I'm fine either approach, perhaps leaning slightly towards u32 size.
For the series:
Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >
> > This being said, the opposite argument can be made, that a 32-bit size
> > could come handy if we ever have larger blocks, and a new version of the
> > parameters format could be used if we ever need to add more fields to
> > the block header. I won't insist either way.
> >
> >> i wonder if, with this change, the 'aligned(8)' attributes are even needed, but
> >> I didn't dig into that.
> >
> > The header would become 8-bytes long, but its larger field would still
> > be 4-bytes long, so the compiler would only enforce 4-bytes aligned
> > AFAIK.
>
> Normally the actual data blocks (in the non-extensible format) are already aligned
> to either 4 or 8 bytes (depending on whether u64 values are used). So an 8 byte
> header won't mess up the alignment.
--
Kind regards,
Sakari Ailus
