On Wed, Jun 12, 2024 at 11:00:37AM +0100, Daniel Scally wrote:
> On 05/06/2024 17:54, Jacopo Mondi wrote:
> > Add a new format definition for the RkISP1 extensible parameters
> > format and document it.
> >
> > Document the usage of the new format in the rkisp1 admin guide.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > ---
> > Documentation/admin-guide/media/rkisp1.rst | 11 +++-
> > .../media/v4l/metafmt-rkisp1.rst | 62 ++++++++++++++++---
> > drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> > include/uapi/linux/videodev2.h | 1 +
> > 4 files changed, 64 insertions(+), 11 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
> > index 6f14d9561fa5..934c25e191df 100644
> > --- a/Documentation/admin-guide/media/rkisp1.rst
> > +++ b/Documentation/admin-guide/media/rkisp1.rst
> > @@ -114,11 +114,18 @@ to be applied to the hardware during a video stream, allowing userspace
> > to dynamically modify values such as black level, cross talk corrections
> > and others.
> >
> > -The buffer format is defined by struct :c:type:`rkisp1_params_cfg`, and
> > -userspace should set
> > +The ISP driver supports two different parameters configuration methods, the
> > +`fixed parameters format` or the `extensible parameters format`.
> > +
> > +When using the `fixed parameters` method the buffer format is defined by struct
> > +:c:type:`rkisp1_params_cfg`, and userspace set
> > :ref:`V4L2_META_FMT_RK_ISP1_PARAMS <v4l2-meta-fmt-rk-isp1-params>` as the
> > dataformat.
> >
> > +When using the fixed parameters method the buffer format is defined by struct
> > +:c:type:`rkisp1_ext_params_cfg`, and userspace set
>
> s/set/should set
Above too.
> > +:ref:`V4L2_META_FMT_RK_ISP1_EXT_PARAMS <v4l2-meta-fmt-rk-isp1-ext-params>` as
> > +the dataformat.
> >
> > Capturing Video Frames Example
> > ==============================
> > diff --git a/Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst b/Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
> > index fa04f00bcd2e..6ff776d071a3 100644
> > --- a/Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
> > +++ b/Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
> > @@ -1,28 +1,72 @@
> > .. SPDX-License-Identifier: GPL-2.0
> >
> > -.. _v4l2-meta-fmt-rk-isp1-params:
> > -
> > .. _v4l2-meta-fmt-rk-isp1-stat-3a:
> >
> > -*****************************************************************************
> > -V4L2_META_FMT_RK_ISP1_PARAMS ('rk1p'), V4L2_META_FMT_RK_ISP1_STAT_3A ('rk1s')
> > -*****************************************************************************
> > +************************************************************************************************************************
> > +V4L2_META_FMT_RK_ISP1_PARAMS ('rk1p'), V4L2_META_FMT_RK_ISP1_STAT_3A ('rk1s'), V4L2_META_FMT_RK_ISP1_EXT_PARAMS ('rk1e')
> > +************************************************************************************************************************
> >
> > +========================
> > Configuration parameters
> > ========================
> >
> > -The configuration parameters are passed to the
> > +The configuration of the RkISP1 ISP is performed by userspace by providing
> > +parameters for the ISP to the driver using the :c:type:`v4l2_meta_format`
> > +interface.
> > +
> > +There are currently two methods that allow to configure the ISP, the `fixed
s/currently //
> > +parameters` configuration format and the `extensible parameters` configuration
> > +format.
> > +
> > +.. _v4l2-meta-fmt-rk-isp1-params:
> > +
> > +Fixed parameters configuration format
> > +=====================================
> > +
> > +When using the fixed configuration format, parameters are passed to the
> > :ref:`rkisp1_params <rkisp1_params>` metadata output video node, using
> > -the :c:type:`v4l2_meta_format` interface. The buffer contains
> > -a single instance of the C structure :c:type:`rkisp1_params_cfg` defined in
> > -``rkisp1-config.h``. So the structure can be obtained from the buffer by:
> > +the `V4L2_META_FMT_RK_ISP1_PARAMS` meta pixel format.
s/meta pixel format/meta format/ I think. Same below.
> > +
> > +The buffer contains a single instance of the C structure
> > +:c:type:`rkisp1_params_cfg` defined in ``rkisp1-config.h``. So the structure can
> > +be obtained from the buffer by:
> >
> > .. code-block:: c
> >
> > struct rkisp1_params_cfg *params = (struct rkisp1_params_cfg*) buffer;
> >
> > +As the members of :c:type:`rkisp1_params_cfg` are defined in the
> > +``rkisp1-config.h`` header, the structure layout is immutable and cannot be
> > +extended further. For this reason the fixed configuration format only allows the
> > +configuration of the ISP blocks supported at the time when the structure had
> > +been defined in the header file, as introducing new parameters or modifying the
> > +existing ones would change the buffer layout and cause breakages in existing
> > +applications.
>
> I'm not sure I'd bother with the last sentence; up to you though.
> Possibly a quick explainer as to why there's two separate methods and
> why the extensible one should be preferred could be in the
> "Configuration parameters" section instead.
I agree. The last sentence here belongs to a commit message more than to
the in-kernel documentation. I would replace it with "This method
supports a subset of the ISP features only, new applications should use
the extensible parameters method." or something similar.
> > +
> > +.. _v4l2-meta-fmt-rk-isp1-ext-params:
> > +
> > +Extensible parameters configuration format
> > +==========================================
> > +
> > +When using the extensible configuration format, parameters are passed to the
> > +:ref:`rkisp1_params <rkisp1_params>` metadata output video node, using
> > +the `V4L2_META_FMT_RK_ISP1_EXT_PARAMS` meta pixel format.
> > +
> > +The buffer contains a single instance of the C structure
> > +:c:type:`rkisp1_ext_params_cfg` defined in ``rkisp1-config.h``. The
> > +:c:type:`rkisp1_ext_params_cfg` structure is designed to allow userspace to
> > +populate the data buffer with only the configuration data for the ISP block it
>
> s/block/block(s)?
>
> > +intends to configure. The extensible parameters format design allows to define
> > +new block types and new configuration parameters and defines a versioning scheme
>
> Perhaps "allows developers to define new block types to support new configuration parameters"?
>
> > +so that it can be extended and versioned without breaking compatibility with
> > +existing applications.
> > +
> > +For these reasons, this configuration method if preferred over the `fixed
> > +parameters` format alternative.
> > +
> > .. rkisp1_stat_buffer
> >
> > +===========================
> > 3A and histogram statistics
> > ===========================
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> > index 4c76d17b4629..aefdc1efd24b 100644
> > --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> > @@ -1456,6 +1456,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> > case V4L2_META_FMT_VIVID: descr = "Vivid Metadata"; break;
> > case V4L2_META_FMT_RK_ISP1_PARAMS: descr = "Rockchip ISP1 3A Parameters"; break;
> > case V4L2_META_FMT_RK_ISP1_STAT_3A: descr = "Rockchip ISP1 3A Statistics"; break;
> > + case V4L2_META_FMT_RK_ISP1_EXT_PARAMS: descr = "Rockchip ISP1 Ext 3A Params"; break;
>
> I think spell out "Extensible" personally; we already allow breaking like length limits here.
Format descriptions are limited to 32 bytes in the UAPI :-(
> Reviewed-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
>
> > case V4L2_PIX_FMT_NV12_8L128: descr = "NV12 (8x128 Linear)"; break;
> > case V4L2_PIX_FMT_NV12M_8L128: descr = "NV12M (8x128 Linear)"; break;
> > case V4L2_PIX_FMT_NV12_10BE_8L128: descr = "10-bit NV12 (8x128 Linear, BE)"; break;
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index fe6b67e83751..7c2a303c6f59 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -840,6 +840,7 @@ struct v4l2_pix_format {
> > /* Vendor specific - used for RK_ISP1 camera sub-system */
> > #define V4L2_META_FMT_RK_ISP1_PARAMS v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> > #define V4L2_META_FMT_RK_ISP1_STAT_3A v4l2_fourcc('R', 'K', '1', 'S') /* Rockchip ISP1 3A Statistics */
> > +#define V4L2_META_FMT_RK_ISP1_EXT_PARAMS v4l2_fourcc('R', 'K', '1', 'E') /* Rockchip ISP1 3a Extensible Parameters */
> >
> > #ifdef __KERNEL__
> > /*
--
Regards,
Laurent Pinchart
