[2/8] uapi: videodev2: Add V4L2_META_FMT_RK_ISP1_EXT_PARAMS

Message ID 20240605165434.432230-3-jacopo.mondi@ideasonboard.com (mailing list archive)
State New
Headers
Series media: rkisp1: Implement support for extensible parameters |

Commit Message

Jacopo Mondi June 5, 2024, 4:54 p.m. UTC
  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@ideasonboard.com>
---
 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(-)
  

Comments

Dan Scally June 12, 2024, 10 a.m. UTC | #1
Hi Jacopo

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@ideasonboard.com>
> ---
>   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
> +: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
> +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.
> +
> +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.

> +
> +.. _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.


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   	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__
>   /*
  
Laurent Pinchart June 12, 2024, 2:35 p.m. UTC | #2
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@ideasonboard.com>
> > ---
> >   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@ideasonboard.com>
> 
> >   	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__
> >   /*
  
Dan Scally June 12, 2024, 3:09 p.m. UTC | #3
On 12/06/2024 15:35, Laurent Pinchart wrote:
> 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@ideasonboard.com>
>>> ---
>>>    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 :-(

Today I learned!
>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>>>    	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__
>>>    /*
  
Paul Elder June 20, 2024, 9:31 a.m. UTC | #4
On Wed, Jun 05, 2024 at 06:54:21PM +0200, 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@ideasonboard.com>
> ---
>  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

s/fixed/extensible/ ?

> +:c:type:`rkisp1_ext_params_cfg`, and userspace set
> +: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
> +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.
> +
> +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.
> +
> +.. _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
> +intends to configure. The extensible parameters format design allows to define

s/allows to define/allows defining/

> +new block types and new configuration parameters and defines a versioning scheme

s/parameters/parameters,/


Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> +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;
>  	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__
>  /*
> -- 
> 2.45.1
>
  

Patch

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
+: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
+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.
+
+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.
+
+.. _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
+intends to configure. The extensible parameters format design allows to define
+new block types and new configuration parameters and defines a versioning scheme
+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;
 	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__
 /*