[v9,10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

Message ID	1699595289-25773-11-git-send-email-shengjiu.wang@nxp.com (mailing list archive)
State	Superseded
Headers	show Return-Path: <alsa-devel-bounces@alsa-project.org> From: Shengjiu Wang <shengjiu.wang@nxp.com> To: hverkuil@xs4all.nl, sakari.ailus@iki.fi, tfiga@chromium.org, m.szyprowski@samsung.com, mchehab@kernel.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, shengjiu.wang@gmail.com, Xiubo.Lee@gmail.com, festevam@gmail.com, nicoleotsuka@gmail.com, lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org, linuxppc-dev@lists.ozlabs.org Subject: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT Date: Fri, 10 Nov 2023 13:48:04 +0800 Message-Id: <1699595289-25773-11-git-send-email-shengjiu.wang@nxp.com> In-Reply-To: <1699595289-25773-1-git-send-email-shengjiu.wang@nxp.com> References: <1699595289-25773-1-git-send-email-shengjiu.wang@nxp.com> Message-ID-Hash: 3PX23ESCE5GZKG5QGBDLJF2TPCAT5ASJ Precedence: list Archived-At: <https://mailman.alsa-project.org/hyperkitty/list/alsa-devel@alsa-project.org/message/3PX23ESCE5GZKG5QGBDLJF2TPCAT5ASJ/>
Series	Add audio support in v4l2 framework \| expand [v9,00/15] Add audio support in v4l2 framework [v9,01/15] ASoC: fsl_asrc: define functions for memory to memory usage [v9,02/15] ASoC: fsl_easrc: define functions for memory to memory usage [v9,03/15] ASoC: fsl_asrc: move fsl_asrc_common.h to include/sound [v9,04/15] ASoC: fsl_asrc: register m2m platform device [v9,05/15] ASoC: fsl_easrc: register m2m platform device [v9,06/15] media: uapi: Add V4L2_CAP_AUDIO_M2M capability flag [v9,07/15] media: v4l2: Add audio capture and output support [v9,08/15] media: uapi: Define audio sample format fourcc type [v9,09/15] media: uapi: Add V4L2_CTRL_CLASS_M2M_AUDIO [v9,10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT [v9,11/15] media: uapi: Add audio rate controls support [v9,12/15] media: uapi: Declare interface types for Audio [v9,13/15] media: uapi: Add an entity type for audio resampler [v9,14/15] media: imx-asrc: Add memory to memory driver [v9,15/15] media: vim2m-audio: add virtual driver for audio memory to memory

Shengjiu Wang Nov. 10, 2023, 5:48 a.m. UTC

Fixed point controls are used by the user to configure
a fixed point value in 64bits, which Q31.32 format.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
 .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
 .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
 drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
 drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
 include/uapi/linux/videodev2.h                      |  1 +
 6 files changed, 23 insertions(+), 8 deletions(-)

Hans Verkuil Nov. 13, 2023, 10:29 a.m. UTC | #1

Hi Shengjiu,

On 10/11/2023 06:48, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> a fixed point value in 64bits, which Q31.32 format.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>

This patch adds a new control type. This is something that also needs to be
tested by v4l2-compliance, and for that we need to add support for this to
one of the media test-drivers. The best place for that is the vivid driver,
since that has already a bunch of test controls for other control types.

See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.

Can you add a patch adding a fixed point test control to vivid?

Thanks!

	Hans

> ---
>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
>  .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
>  drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
>  include/uapi/linux/videodev2.h                      |  1 +
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index e8475f9fd2cf..e7e5d78dc11e 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -162,13 +162,13 @@ still cause this situation.
>      * - __s32
>        - ``value``
>        - New value or current value. Valid if this control is not of type
> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> -	not set.
> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>      * - __s64
>        - ``value64``
>        - New value or current value. Valid if this control is of type
> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> -	not set.
> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>      * - char *
>        - ``string``
>        - A pointer to a string. Valid if this control is of type
> @@ -193,8 +193,9 @@ still cause this situation.
>      * - __s64 *
>        - ``p_s64``
>        - A pointer to a matrix control of signed 64-bit values. Valid if
> -        this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> -        ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> +        this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> +        ``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> +        is set.
>      * - struct :c:type:`v4l2_area` *
>        - ``p_area``
>        - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 4d38acafe8e1..f3995ec57044 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
>        - ``default_value``
>        - The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
>  	``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> -	or ``_U16`` control. Not valid for other types of controls.
> +	``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> +	controls.
>  
>  	.. note::
>  
> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
>        - n/a
>        - A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film Grain
>          parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_FIXED_POINT``
> +      - any
> +      - any
> +      - any
> +      - A 64-bit integer valued control, containing parameter which is
> +        Q31.32 format.
>  
>  .. raw:: latex
>  
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index e61152bb80d1..2faa5a2015eb 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
>  
>  # V4L2 capability defines
>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..e6a0fb8d6791 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -57,6 +57,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
>  		return copy_to_user(c->string, ptr.p_char, len + 1) ?
>  		       -EFAULT : 0;
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		c->value64 = *ptr.p_s64;
>  		break;
>  	default:
> @@ -132,6 +133,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>  
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		*ctrl->p_new.p_s64 = c->value64;
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
> @@ -540,7 +542,8 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
>  		 */
>  		if (ctrl->is_ptr)
>  			continue;
> -		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> +		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64 ||
> +		    ctrl->type == V4L2_CTRL_TYPE_FIXED_POINT)
>  			p_new.p_s64 = &cs->controls[i].value64;
>  		else
>  			p_new.p_s32 = &cs->controls[i].value;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..9d50df0d9874 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1187,6 +1187,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_INTEGER:
>  		return ROUND_TO_RANGE(ptr.p_s32[idx], u32, ctrl);
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		/*
>  		 * We can't use the ROUND_TO_RANGE define here due to
>  		 * the u64 divide that needs special care.
> @@ -1779,6 +1780,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	/* Prefill elem_size for all types handled by std_type_ops */
>  	switch ((u32)type) {
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		elem_size = sizeof(s64);
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index cf8c44595a1d..9482ac66a675 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1903,6 +1903,7 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_STRING        = 7,
>  	V4L2_CTRL_TYPE_BITMASK       = 8,
>  	V4L2_CTRL_TYPE_INTEGER_MENU  = 9,
> +	V4L2_CTRL_TYPE_FIXED_POINT   = 10,
>  
>  	/* Compound types are >= 0x0100 */
>  	V4L2_CTRL_COMPOUND_TYPES     = 0x0100,

Laurent Pinchart Nov. 13, 2023, 10:42 a.m. UTC | #2

On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> Hi Shengjiu,
> 
> On 10/11/2023 06:48, Shengjiu Wang wrote:
> > Fixed point controls are used by the user to configure
> > a fixed point value in 64bits, which Q31.32 format.
> > 
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> 
> This patch adds a new control type. This is something that also needs to be
> tested by v4l2-compliance, and for that we need to add support for this to
> one of the media test-drivers. The best place for that is the vivid driver,
> since that has already a bunch of test controls for other control types.
> 
> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> 
> Can you add a patch adding a fixed point test control to vivid?

I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
relate more to units than control types. We have lots of fixed-point
values in controls already, using the 32-bit and 64-bit integer control
types. They use various locations for the decimal point, depending on
the control. If we want to make this more explicit to users, we should
work on adding unit support to the V4L2 controls.

> > ---
> >  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
> >  .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
> >  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
> >  drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
> >  drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
> >  include/uapi/linux/videodev2.h                      |  1 +
> >  6 files changed, 23 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > index e8475f9fd2cf..e7e5d78dc11e 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > @@ -162,13 +162,13 @@ still cause this situation.
> >      * - __s32
> >        - ``value``
> >        - New value or current value. Valid if this control is not of type
> > -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> > -	not set.
> > +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> > +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >      * - __s64
> >        - ``value64``
> >        - New value or current value. Valid if this control is of type
> > -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> > -	not set.
> > +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> > +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >      * - char *
> >        - ``string``
> >        - A pointer to a string. Valid if this control is of type
> > @@ -193,8 +193,9 @@ still cause this situation.
> >      * - __s64 *
> >        - ``p_s64``
> >        - A pointer to a matrix control of signed 64-bit values. Valid if
> > -        this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> > -        ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> > +        this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> > +        ``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> > +        is set.
> >      * - struct :c:type:`v4l2_area` *
> >        - ``p_area``
> >        - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > index 4d38acafe8e1..f3995ec57044 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
> >        - ``default_value``
> >        - The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
> >  	``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> > -	or ``_U16`` control. Not valid for other types of controls.
> > +	``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> > +	controls.
> >  
> >  	.. note::
> >  
> > @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
> >        - n/a
> >        - A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film Grain
> >          parameters for stateless video decoders.
> > +    * - ``V4L2_CTRL_TYPE_FIXED_POINT``
> > +      - any
> > +      - any
> > +      - any
> > +      - A 64-bit integer valued control, containing parameter which is
> > +        Q31.32 format.
> >  
> >  .. raw:: latex
> >  
> > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > index e61152bb80d1..2faa5a2015eb 100644
> > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> > @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE :c:type:`v4l2_ctrl_type`
> >  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
> >  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
> >  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
> > +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
> >  
> >  # V4L2 capability defines
> >  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> > index 002ea6588edf..e6a0fb8d6791 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> > @@ -57,6 +57,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
> >  		return copy_to_user(c->string, ptr.p_char, len + 1) ?
> >  		       -EFAULT : 0;
> >  	case V4L2_CTRL_TYPE_INTEGER64:
> > +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >  		c->value64 = *ptr.p_s64;
> >  		break;
> >  	default:
> > @@ -132,6 +133,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
> >  
> >  	switch (ctrl->type) {
> >  	case V4L2_CTRL_TYPE_INTEGER64:
> > +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >  		*ctrl->p_new.p_s64 = c->value64;
> >  		break;
> >  	case V4L2_CTRL_TYPE_STRING:
> > @@ -540,7 +542,8 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
> >  		 */
> >  		if (ctrl->is_ptr)
> >  			continue;
> > -		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> > +		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64 ||
> > +		    ctrl->type == V4L2_CTRL_TYPE_FIXED_POINT)
> >  			p_new.p_s64 = &cs->controls[i].value64;
> >  		else
> >  			p_new.p_s32 = &cs->controls[i].value;
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > index a662fb60f73f..9d50df0d9874 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > @@ -1187,6 +1187,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
> >  	case V4L2_CTRL_TYPE_INTEGER:
> >  		return ROUND_TO_RANGE(ptr.p_s32[idx], u32, ctrl);
> >  	case V4L2_CTRL_TYPE_INTEGER64:
> > +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >  		/*
> >  		 * We can't use the ROUND_TO_RANGE define here due to
> >  		 * the u64 divide that needs special care.
> > @@ -1779,6 +1780,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >  	/* Prefill elem_size for all types handled by std_type_ops */
> >  	switch ((u32)type) {
> >  	case V4L2_CTRL_TYPE_INTEGER64:
> > +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >  		elem_size = sizeof(s64);
> >  		break;
> >  	case V4L2_CTRL_TYPE_STRING:
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index cf8c44595a1d..9482ac66a675 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1903,6 +1903,7 @@ enum v4l2_ctrl_type {
> >  	V4L2_CTRL_TYPE_STRING        = 7,
> >  	V4L2_CTRL_TYPE_BITMASK       = 8,
> >  	V4L2_CTRL_TYPE_INTEGER_MENU  = 9,
> > +	V4L2_CTRL_TYPE_FIXED_POINT   = 10,
> >  
> >  	/* Compound types are >= 0x0100 */
> >  	V4L2_CTRL_COMPOUND_TYPES     = 0x0100,

Hans Verkuil Nov. 13, 2023, 10:56 a.m. UTC | #3

On 13/11/2023 11:42, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>> Hi Shengjiu,
>>
>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>> Fixed point controls are used by the user to configure
>>> a fixed point value in 64bits, which Q31.32 format.
>>>
>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>
>> This patch adds a new control type. This is something that also needs to be
>> tested by v4l2-compliance, and for that we need to add support for this to
>> one of the media test-drivers. The best place for that is the vivid driver,
>> since that has already a bunch of test controls for other control types.
>>
>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>
>> Can you add a patch adding a fixed point test control to vivid?
> 
> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> relate more to units than control types. We have lots of fixed-point
> values in controls already, using the 32-bit and 64-bit integer control
> types. They use various locations for the decimal point, depending on
> the control. If we want to make this more explicit to users, we should
> work on adding unit support to the V4L2 controls.

"Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.

A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
only shows a single driver specific control (dw100.rst).

I'm not aware of other controls in mainline that use fixed point.

Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
min/max/step you can easily map that to just about any QN.M format where
N <= 31 and M <= 32.

In the case of dw100 it is a bit different in that it is quite specialized
and it had to fit in 16 bits.

Regards,

	Hans

> 
>>> ---
>>>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
>>>  .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
>>>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
>>>  drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
>>>  include/uapi/linux/videodev2.h                      |  1 +
>>>  6 files changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> index e8475f9fd2cf..e7e5d78dc11e 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> @@ -162,13 +162,13 @@ still cause this situation.
>>>      * - __s32
>>>        - ``value``
>>>        - New value or current value. Valid if this control is not of type
>>> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
>>> -	not set.
>>> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
>>> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>>>      * - __s64
>>>        - ``value64``
>>>        - New value or current value. Valid if this control is of type
>>> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
>>> -	not set.
>>> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
>>> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>>>      * - char *
>>>        - ``string``
>>>        - A pointer to a string. Valid if this control is of type
>>> @@ -193,8 +193,9 @@ still cause this situation.
>>>      * - __s64 *
>>>        - ``p_s64``
>>>        - A pointer to a matrix control of signed 64-bit values. Valid if
>>> -        this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
>>> -        ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
>>> +        this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
>>> +        ``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
>>> +        is set.
>>>      * - struct :c:type:`v4l2_area` *
>>>        - ``p_area``
>>>        - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> index 4d38acafe8e1..f3995ec57044 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
>>>        - ``default_value``
>>>        - The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
>>>  	``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
>>> -	or ``_U16`` control. Not valid for other types of controls.
>>> +	``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
>>> +	controls.
>>>  
>>>  	.. note::
>>>  
>>> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
>>>        - n/a
>>>        - A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film Grain
>>>          parameters for stateless video decoders.
>>> +    * - ``V4L2_CTRL_TYPE_FIXED_POINT``
>>> +      - any
>>> +      - any
>>> +      - any
>>> +      - A 64-bit integer valued control, containing parameter which is
>>> +        Q31.32 format.
>>>  
>>>  .. raw:: latex
>>>  
>>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> index e61152bb80d1..2faa5a2015eb 100644
>>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>>> @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE :c:type:`v4l2_ctrl_type`
>>>  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
>>>  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
>>>  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
>>> +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
>>>  
>>>  # V4L2 capability defines
>>>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>>> index 002ea6588edf..e6a0fb8d6791 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>>> @@ -57,6 +57,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
>>>  		return copy_to_user(c->string, ptr.p_char, len + 1) ?
>>>  		       -EFAULT : 0;
>>>  	case V4L2_CTRL_TYPE_INTEGER64:
>>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>>>  		c->value64 = *ptr.p_s64;
>>>  		break;
>>>  	default:
>>> @@ -132,6 +133,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>>>  
>>>  	switch (ctrl->type) {
>>>  	case V4L2_CTRL_TYPE_INTEGER64:
>>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>>>  		*ctrl->p_new.p_s64 = c->value64;
>>>  		break;
>>>  	case V4L2_CTRL_TYPE_STRING:
>>> @@ -540,7 +542,8 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
>>>  		 */
>>>  		if (ctrl->is_ptr)
>>>  			continue;
>>> -		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
>>> +		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64 ||
>>> +		    ctrl->type == V4L2_CTRL_TYPE_FIXED_POINT)
>>>  			p_new.p_s64 = &cs->controls[i].value64;
>>>  		else
>>>  			p_new.p_s32 = &cs->controls[i].value;
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> index a662fb60f73f..9d50df0d9874 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> @@ -1187,6 +1187,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
>>>  	case V4L2_CTRL_TYPE_INTEGER:
>>>  		return ROUND_TO_RANGE(ptr.p_s32[idx], u32, ctrl);
>>>  	case V4L2_CTRL_TYPE_INTEGER64:
>>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>>>  		/*
>>>  		 * We can't use the ROUND_TO_RANGE define here due to
>>>  		 * the u64 divide that needs special care.
>>> @@ -1779,6 +1780,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>  	/* Prefill elem_size for all types handled by std_type_ops */
>>>  	switch ((u32)type) {
>>>  	case V4L2_CTRL_TYPE_INTEGER64:
>>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>>>  		elem_size = sizeof(s64);
>>>  		break;
>>>  	case V4L2_CTRL_TYPE_STRING:
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index cf8c44595a1d..9482ac66a675 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1903,6 +1903,7 @@ enum v4l2_ctrl_type {
>>>  	V4L2_CTRL_TYPE_STRING        = 7,
>>>  	V4L2_CTRL_TYPE_BITMASK       = 8,
>>>  	V4L2_CTRL_TYPE_INTEGER_MENU  = 9,
>>> +	V4L2_CTRL_TYPE_FIXED_POINT   = 10,
>>>  
>>>  	/* Compound types are >= 0x0100 */
>>>  	V4L2_CTRL_COMPOUND_TYPES     = 0x0100,
>

Laurent Pinchart Nov. 13, 2023, 11:07 a.m. UTC | #4

On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >> Hi Shengjiu,
> >>
> >> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>> Fixed point controls are used by the user to configure
> >>> a fixed point value in 64bits, which Q31.32 format.
> >>>
> >>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>
> >> This patch adds a new control type. This is something that also needs to be
> >> tested by v4l2-compliance, and for that we need to add support for this to
> >> one of the media test-drivers. The best place for that is the vivid driver,
> >> since that has already a bunch of test controls for other control types.
> >>
> >> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>
> >> Can you add a patch adding a fixed point test control to vivid?
> > 
> > I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > relate more to units than control types. We have lots of fixed-point
> > values in controls already, using the 32-bit and 64-bit integer control
> > types. They use various locations for the decimal point, depending on
> > the control. If we want to make this more explicit to users, we should
> > work on adding unit support to the V4L2 controls.
> 
> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.

It's not a unit, but I think it's related to units. My point is that,
without units support, I don't see why we need a formal definition of
fixed-point types, and why this series couldn't just use
VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
values as they see fit.

> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> only shows a single driver specific control (dw100.rst).
> 
> I'm not aware of other controls in mainline that use fixed point.

The analog gain control for sensors for instance.

> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> min/max/step you can easily map that to just about any QN.M format where
> N <= 31 and M <= 32.
> 
> In the case of dw100 it is a bit different in that it is quite specialized
> and it had to fit in 16 bits.
> 
> >>> ---
> >>>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
> >>>  .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
> >>>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
> >>>  drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
> >>>  drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
> >>>  include/uapi/linux/videodev2.h                      |  1 +
> >>>  6 files changed, 23 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> index e8475f9fd2cf..e7e5d78dc11e 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> @@ -162,13 +162,13 @@ still cause this situation.
> >>>      * - __s32
> >>>        - ``value``
> >>>        - New value or current value. Valid if this control is not of type
> >>> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> >>> -	not set.
> >>> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> >>> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >>>      * - __s64
> >>>        - ``value64``
> >>>        - New value or current value. Valid if this control is of type
> >>> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> >>> -	not set.
> >>> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> >>> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
> >>>      * - char *
> >>>        - ``string``
> >>>        - A pointer to a string. Valid if this control is of type
> >>> @@ -193,8 +193,9 @@ still cause this situation.
> >>>      * - __s64 *
> >>>        - ``p_s64``
> >>>        - A pointer to a matrix control of signed 64-bit values. Valid if
> >>> -        this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> >>> -        ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> >>> +        this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> >>> +        ``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> >>> +        is set.
> >>>      * - struct :c:type:`v4l2_area` *
> >>>        - ``p_area``
> >>>        - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> index 4d38acafe8e1..f3995ec57044 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
> >>>        - ``default_value``
> >>>        - The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
> >>>  	``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> >>> -	or ``_U16`` control. Not valid for other types of controls.
> >>> +	``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> >>> +	controls.
> >>>  
> >>>  	.. note::
> >>>  
> >>> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
> >>>        - n/a
> >>>        - A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film Grain
> >>>          parameters for stateless video decoders.
> >>> +    * - ``V4L2_CTRL_TYPE_FIXED_POINT``
> >>> +      - any
> >>> +      - any
> >>> +      - any
> >>> +      - A 64-bit integer valued control, containing parameter which is
> >>> +        Q31.32 format.
> >>>  
> >>>  .. raw:: latex
> >>>  
> >>> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> index e61152bb80d1..2faa5a2015eb 100644
> >>> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >>> @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE :c:type:`v4l2_ctrl_type`
> >>>  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
> >>>  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
> >>>  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
> >>> +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
> >>>  
> >>>  # V4L2 capability defines
> >>>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> >>> index 002ea6588edf..e6a0fb8d6791 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> >>> @@ -57,6 +57,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
> >>>  		return copy_to_user(c->string, ptr.p_char, len + 1) ?
> >>>  		       -EFAULT : 0;
> >>>  	case V4L2_CTRL_TYPE_INTEGER64:
> >>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>  		c->value64 = *ptr.p_s64;
> >>>  		break;
> >>>  	default:
> >>> @@ -132,6 +133,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
> >>>  
> >>>  	switch (ctrl->type) {
> >>>  	case V4L2_CTRL_TYPE_INTEGER64:
> >>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>  		*ctrl->p_new.p_s64 = c->value64;
> >>>  		break;
> >>>  	case V4L2_CTRL_TYPE_STRING:
> >>> @@ -540,7 +542,8 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
> >>>  		 */
> >>>  		if (ctrl->is_ptr)
> >>>  			continue;
> >>> -		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> >>> +		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64 ||
> >>> +		    ctrl->type == V4L2_CTRL_TYPE_FIXED_POINT)
> >>>  			p_new.p_s64 = &cs->controls[i].value64;
> >>>  		else
> >>>  			p_new.p_s32 = &cs->controls[i].value;
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> index a662fb60f73f..9d50df0d9874 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> @@ -1187,6 +1187,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>  	case V4L2_CTRL_TYPE_INTEGER:
> >>>  		return ROUND_TO_RANGE(ptr.p_s32[idx], u32, ctrl);
> >>>  	case V4L2_CTRL_TYPE_INTEGER64:
> >>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>  		/*
> >>>  		 * We can't use the ROUND_TO_RANGE define here due to
> >>>  		 * the u64 divide that needs special care.
> >>> @@ -1779,6 +1780,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>>  	/* Prefill elem_size for all types handled by std_type_ops */
> >>>  	switch ((u32)type) {
> >>>  	case V4L2_CTRL_TYPE_INTEGER64:
> >>> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>  		elem_size = sizeof(s64);
> >>>  		break;
> >>>  	case V4L2_CTRL_TYPE_STRING:
> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>> index cf8c44595a1d..9482ac66a675 100644
> >>> --- a/include/uapi/linux/videodev2.h
> >>> +++ b/include/uapi/linux/videodev2.h
> >>> @@ -1903,6 +1903,7 @@ enum v4l2_ctrl_type {
> >>>  	V4L2_CTRL_TYPE_STRING        = 7,
> >>>  	V4L2_CTRL_TYPE_BITMASK       = 8,
> >>>  	V4L2_CTRL_TYPE_INTEGER_MENU  = 9,
> >>> +	V4L2_CTRL_TYPE_FIXED_POINT   = 10,
> >>>  
> >>>  	/* Compound types are >= 0x0100 */
> >>>  	V4L2_CTRL_COMPOUND_TYPES     = 0x0100,

Hans Verkuil Nov. 13, 2023, 11:24 a.m. UTC | #5

On 13/11/2023 12:07, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>> Hi Shengjiu,
>>>>
>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>> Fixed point controls are used by the user to configure
>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>
>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>
>>>> This patch adds a new control type. This is something that also needs to be
>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>> since that has already a bunch of test controls for other control types.
>>>>
>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>
>>>> Can you add a patch adding a fixed point test control to vivid?
>>>
>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>> relate more to units than control types. We have lots of fixed-point
>>> values in controls already, using the 32-bit and 64-bit integer control
>>> types. They use various locations for the decimal point, depending on
>>> the control. If we want to make this more explicit to users, we should
>>> work on adding unit support to the V4L2 controls.
>>
>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> 
> It's not a unit, but I think it's related to units. My point is that,
> without units support, I don't see why we need a formal definition of
> fixed-point types, and why this series couldn't just use
> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> values as they see fit.

They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
(I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
is always interpreted as a 64 bit integer and nothing else. As it should.

And while we do not have support for units (other than the documentation),
we do have type support in the form of V4L2_CTRL_TYPE_*.

> 
>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>> only shows a single driver specific control (dw100.rst).
>>
>> I'm not aware of other controls in mainline that use fixed point.
> 
> The analog gain control for sensors for instance.

Not really. The documentation is super vague:

V4L2_CID_ANALOGUE_GAIN (integer)

	Analogue gain is gain affecting all colour components in the pixel matrix. The
	gain operation is performed in the analogue domain before A/D conversion.

And the integer is just a range. Internally it might map to some fixed
point value, but userspace won't see that, it's hidden in the driver AFAICT.

In the case of this particular series the control type is really a fixed point
value with a documented unit (Hz). It really is not something you want to
use type INTEGER64 for.

> 
>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>> min/max/step you can easily map that to just about any QN.M format where
>> N <= 31 and M <= 32.
>>
>> In the case of dw100 it is a bit different in that it is quite specialized
>> and it had to fit in 16 bits.

Regards,

	Hans

Sakari Ailus Nov. 13, 2023, 11:28 a.m. UTC | #6

Hi Hans,

On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> On 13/11/2023 12:07, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>> Hi Shengjiu,
> >>>>
> >>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>> Fixed point controls are used by the user to configure
> >>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>
> >>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>
> >>>> This patch adds a new control type. This is something that also needs to be
> >>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>> since that has already a bunch of test controls for other control types.
> >>>>
> >>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>
> >>>> Can you add a patch adding a fixed point test control to vivid?
> >>>
> >>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>> relate more to units than control types. We have lots of fixed-point
> >>> values in controls already, using the 32-bit and 64-bit integer control
> >>> types. They use various locations for the decimal point, depending on
> >>> the control. If we want to make this more explicit to users, we should
> >>> work on adding unit support to the V4L2 controls.
> >>
> >> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > 
> > It's not a unit, but I think it's related to units. My point is that,
> > without units support, I don't see why we need a formal definition of
> > fixed-point types, and why this series couldn't just use
> > VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > values as they see fit.
> 
> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> is always interpreted as a 64 bit integer and nothing else. As it should.
> 
> And while we do not have support for units (other than the documentation),
> we do have type support in the form of V4L2_CTRL_TYPE_*.
> 
> > 
> >> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >> only shows a single driver specific control (dw100.rst).
> >>
> >> I'm not aware of other controls in mainline that use fixed point.
> > 
> > The analog gain control for sensors for instance.
> 
> Not really. The documentation is super vague:
> 
> V4L2_CID_ANALOGUE_GAIN (integer)
> 
> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
> 	gain operation is performed in the analogue domain before A/D conversion.
> 
> And the integer is just a range. Internally it might map to some fixed
> point value, but userspace won't see that, it's hidden in the driver AFAICT.

I wonder if Laurent meant digital gain.

Those are often Q numbers. The practice there has been that the default
value yields gain of 1.

There are probably many other examples in controls where something being
controlled isn't actually an integer while integer controls are still being
used for the purpose.

Instead of this patch, I'd prefer to have a way to express the meaning of
the control value, be it a Q number or something else, and do that
independently of the type of the control.

> 
> In the case of this particular series the control type is really a fixed point
> value with a documented unit (Hz). It really is not something you want to
> use type INTEGER64 for.
> 
> > 
> >> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >> min/max/step you can easily map that to just about any QN.M format where
> >> N <= 31 and M <= 32.
> >>
> >> In the case of dw100 it is a bit different in that it is quite specialized
> >> and it had to fit in 16 bits.

Laurent Pinchart Nov. 13, 2023, 11:43 a.m. UTC | #7

On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> > On 13/11/2023 12:07, Laurent Pinchart wrote:
> > > On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> > >> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > >>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> > >>>> Hi Shengjiu,
> > >>>>
> > >>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> > >>>>> Fixed point controls are used by the user to configure
> > >>>>> a fixed point value in 64bits, which Q31.32 format.
> > >>>>>
> > >>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > >>>>
> > >>>> This patch adds a new control type. This is something that also needs to be
> > >>>> tested by v4l2-compliance, and for that we need to add support for this to
> > >>>> one of the media test-drivers. The best place for that is the vivid driver,
> > >>>> since that has already a bunch of test controls for other control types.
> > >>>>
> > >>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> > >>>>
> > >>>> Can you add a patch adding a fixed point test control to vivid?
> > >>>
> > >>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > >>> relate more to units than control types. We have lots of fixed-point
> > >>> values in controls already, using the 32-bit and 64-bit integer control
> > >>> types. They use various locations for the decimal point, depending on
> > >>> the control. If we want to make this more explicit to users, we should
> > >>> work on adding unit support to the V4L2 controls.
> > >>
> > >> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > > 
> > > It's not a unit, but I think it's related to units. My point is that,
> > > without units support, I don't see why we need a formal definition of
> > > fixed-point types, and why this series couldn't just use
> > > VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > > values as they see fit.
> > 
> > They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> > (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it

Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)

> > is always interpreted as a 64 bit integer and nothing else. As it should.

The most common case for control handling in drivers is taking the
integer value and converting it to a register value, using
device-specific encoding of the register value. It can be a fixed-point
format or something else, depending on the device. My point is that
drivers routinely convert a "plain" integer to something else, and that
has never been considered as a cause of concern. I don't see why it
would be different in this series.

> > And while we do not have support for units (other than the documentation),
> > we do have type support in the form of V4L2_CTRL_TYPE_*.
> > 
> > >> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> > >> only shows a single driver specific control (dw100.rst).
> > >>
> > >> I'm not aware of other controls in mainline that use fixed point.
> > > 
> > > The analog gain control for sensors for instance.
> > 
> > Not really. The documentation is super vague:
> > 
> > V4L2_CID_ANALOGUE_GAIN (integer)
> > 
> > 	Analogue gain is gain affecting all colour components in the pixel matrix. The
> > 	gain operation is performed in the analogue domain before A/D conversion.
> > 
> > And the integer is just a range. Internally it might map to some fixed
> > point value, but userspace won't see that, it's hidden in the driver AFAICT.

It's hidden so well that libcamera has a database of the sensor it
supports, with formulas to map a real gain value to the
V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
matter, and the kernel doesn't expose it. We may or may not consider
that as a shortcoming of the V4L2 control API, but in any case it's the
situation we have today.

> I wonder if Laurent meant digital gain.

No, I meant analog. It applies to digital gain too though.

> Those are often Q numbers. The practice there has been that the default
> value yields gain of 1.
> 
> There are probably many other examples in controls where something being
> controlled isn't actually an integer while integer controls are still being
> used for the purpose.

A good summary of my opinion :-)

> Instead of this patch, I'd prefer to have a way to express the meaning of
> the control value, be it a Q number or something else, and do that
> independently of the type of the control.

Agreed.

> > In the case of this particular series the control type is really a fixed point
> > value with a documented unit (Hz). It really is not something you want to
> > use type INTEGER64 for.
> > 
> > >> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> > >> min/max/step you can easily map that to just about any QN.M format where
> > >> N <= 31 and M <= 32.
> > >>
> > >> In the case of dw100 it is a bit different in that it is quite specialized
> > >> and it had to fit in 16 bits.

Hans Verkuil Nov. 13, 2023, 12:05 p.m. UTC | #8

On 13/11/2023 12:43, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
>> Hi Hans,
>>
>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>>>>> Hi Shengjiu,
>>>>>>>
>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>>>>
>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>
>>>>>>> This patch adds a new control type. This is something that also needs to be
>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>>>>> since that has already a bunch of test controls for other control types.
>>>>>>>
>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>>>>
>>>>>>> Can you add a patch adding a fixed point test control to vivid?
>>>>>>
>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>>>>> relate more to units than control types. We have lots of fixed-point
>>>>>> values in controls already, using the 32-bit and 64-bit integer control
>>>>>> types. They use various locations for the decimal point, depending on
>>>>>> the control. If we want to make this more explicit to users, we should
>>>>>> work on adding unit support to the V4L2 controls.
>>>>>
>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
>>>>
>>>> It's not a unit, but I think it's related to units. My point is that,
>>>> without units support, I don't see why we need a formal definition of
>>>> fixed-point types, and why this series couldn't just use
>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>>>> values as they see fit.
>>>
>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> 
> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> 
>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> 
> The most common case for control handling in drivers is taking the
> integer value and converting it to a register value, using
> device-specific encoding of the register value. It can be a fixed-point
> format or something else, depending on the device. My point is that
> drivers routinely convert a "plain" integer to something else, and that
> has never been considered as a cause of concern. I don't see why it
> would be different in this series.
> 
>>> And while we do not have support for units (other than the documentation),
>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>
>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>>>>> only shows a single driver specific control (dw100.rst).
>>>>>
>>>>> I'm not aware of other controls in mainline that use fixed point.
>>>>
>>>> The analog gain control for sensors for instance.
>>>
>>> Not really. The documentation is super vague:
>>>
>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>
>>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
>>> 	gain operation is performed in the analogue domain before A/D conversion.
>>>
>>> And the integer is just a range. Internally it might map to some fixed
>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> 
> It's hidden so well that libcamera has a database of the sensor it
> supports, with formulas to map a real gain value to the
> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> matter, and the kernel doesn't expose it. We may or may not consider
> that as a shortcoming of the V4L2 control API, but in any case it's the
> situation we have today.
> 
>> I wonder if Laurent meant digital gain.
> 
> No, I meant analog. It applies to digital gain too though.
> 
>> Those are often Q numbers. The practice there has been that the default
>> value yields gain of 1.
>>
>> There are probably many other examples in controls where something being
>> controlled isn't actually an integer while integer controls are still being
>> used for the purpose.
> 
> A good summary of my opinion :-)

And that works fine as long as userspace doesn't need to know what the value
actually means.

That's not the case here. The control is really a fractional Hz value:

+``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
+    Sets the offset from the audio source sample rate, unit is Hz.
+    The offset compensates for any clock drift. The actual source audio sample
+    rate is the ideal source audio sample rate from
+    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.

> 
>> Instead of this patch, I'd prefer to have a way to express the meaning of
>> the control value, be it a Q number or something else, and do that
>> independently of the type of the control.

Huh? How is that different from the type of the control? You have integers
(one type) and fixed point (another type).

Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
explicitly?

I think the main reason why we use integer controls for gain is that we
never had a fixed point control type and you could get away with that in
user space for that particular use-case.

Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
be calculated as (value / default_value), but that won't work for a rate offset
control as above, or for e.g. CSC matrices for color converters.

Regards,

	Hans

> 
> Agreed.
> 
>>> In the case of this particular series the control type is really a fixed point
>>> value with a documented unit (Hz). It really is not something you want to
>>> use type INTEGER64 for.
>>>
>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>>>>> min/max/step you can easily map that to just about any QN.M format where
>>>>> N <= 31 and M <= 32.
>>>>>
>>>>> In the case of dw100 it is a bit different in that it is quite specialized
>>>>> and it had to fit in 16 bits.
>

Laurent Pinchart Nov. 13, 2023, 12:44 p.m. UTC | #9

Hi Hans,

On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> On 13/11/2023 12:43, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> >> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> >>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>
> >>>>>>> This patch adds a new control type. This is something that also needs to be
> >>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>>>>> since that has already a bunch of test controls for other control types.
> >>>>>>>
> >>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>>>>
> >>>>>>> Can you add a patch adding a fixed point test control to vivid?
> >>>>>>
> >>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>>>>> relate more to units than control types. We have lots of fixed-point
> >>>>>> values in controls already, using the 32-bit and 64-bit integer control
> >>>>>> types. They use various locations for the decimal point, depending on
> >>>>>> the control. If we want to make this more explicit to users, we should
> >>>>>> work on adding unit support to the V4L2 controls.
> >>>>>
> >>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> >>>>
> >>>> It's not a unit, but I think it's related to units. My point is that,
> >>>> without units support, I don't see why we need a formal definition of
> >>>> fixed-point types, and why this series couldn't just use
> >>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> >>>> values as they see fit.
> >>>
> >>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> > 
> > Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> > 
> >>> is always interpreted as a 64 bit integer and nothing else. As it should.
> > 
> > The most common case for control handling in drivers is taking the
> > integer value and converting it to a register value, using
> > device-specific encoding of the register value. It can be a fixed-point
> > format or something else, depending on the device. My point is that
> > drivers routinely convert a "plain" integer to something else, and that
> > has never been considered as a cause of concern. I don't see why it
> > would be different in this series.
> > 
> >>> And while we do not have support for units (other than the documentation),
> >>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>
> >>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >>>>> only shows a single driver specific control (dw100.rst).
> >>>>>
> >>>>> I'm not aware of other controls in mainline that use fixed point.
> >>>>
> >>>> The analog gain control for sensors for instance.
> >>>
> >>> Not really. The documentation is super vague:
> >>>
> >>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>
> >>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
> >>> 	gain operation is performed in the analogue domain before A/D conversion.
> >>>
> >>> And the integer is just a range. Internally it might map to some fixed
> >>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> > 
> > It's hidden so well that libcamera has a database of the sensor it
> > supports, with formulas to map a real gain value to the
> > V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> > matter, and the kernel doesn't expose it. We may or may not consider
> > that as a shortcoming of the V4L2 control API, but in any case it's the
> > situation we have today.
> > 
> >> I wonder if Laurent meant digital gain.
> > 
> > No, I meant analog. It applies to digital gain too though.
> > 
> >> Those are often Q numbers. The practice there has been that the default
> >> value yields gain of 1.
> >>
> >> There are probably many other examples in controls where something being
> >> controlled isn't actually an integer while integer controls are still being
> >> used for the purpose.
> > 
> > A good summary of my opinion :-)
> 
> And that works fine as long as userspace doesn't need to know what the value
> actually means.
> 
> That's not the case here. The control is really a fractional Hz value:
> 
> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> +    Sets the offset from the audio source sample rate, unit is Hz.
> +    The offset compensates for any clock drift. The actual source audio sample
> +    rate is the ideal source audio sample rate from
> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.

I don't see why this would require a new type, you can use
V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
fixed-point values in Q31.32 format.

> >> Instead of this patch, I'd prefer to have a way to express the meaning of
> >> the control value, be it a Q number or something else, and do that
> >> independently of the type of the control.
> 
> Huh? How is that different from the type of the control? You have integers
> (one type) and fixed point (another type).
> 
> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> explicitly?
> 
> I think the main reason why we use integer controls for gain is that we
> never had a fixed point control type and you could get away with that in
> user space for that particular use-case.
> 
> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> be calculated as (value / default_value),

Typically, but not always. Some sensor have an exponential gain model,
and some have weird gain representation, such as 1/x. That's getting out
of scope though.

> but that won't work for a rate offset
> control as above, or for e.g. CSC matrices for color converters.
> 
> > Agreed.
> > 
> >>> In the case of this particular series the control type is really a fixed point
> >>> value with a documented unit (Hz). It really is not something you want to
> >>> use type INTEGER64 for.
> >>>
> >>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >>>>> min/max/step you can easily map that to just about any QN.M format where
> >>>>> N <= 31 and M <= 32.
> >>>>>
> >>>>> In the case of dw100 it is a bit different in that it is quite specialized
> >>>>> and it had to fit in 16 bits.

Hans Verkuil Nov. 15, 2023, 8:09 a.m. UTC | #10

Hi Laurent,

On 13/11/2023 13:44, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
>> On 13/11/2023 12:43, Laurent Pinchart wrote:
>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>
>>>>>>>>> This patch adds a new control type. This is something that also needs to be
>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>>>>>>> since that has already a bunch of test controls for other control types.
>>>>>>>>>
>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>>>>>>
>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
>>>>>>>>
>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>>>>>>> relate more to units than control types. We have lots of fixed-point
>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
>>>>>>>> types. They use various locations for the decimal point, depending on
>>>>>>>> the control. If we want to make this more explicit to users, we should
>>>>>>>> work on adding unit support to the V4L2 controls.
>>>>>>>
>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
>>>>>>
>>>>>> It's not a unit, but I think it's related to units. My point is that,
>>>>>> without units support, I don't see why we need a formal definition of
>>>>>> fixed-point types, and why this series couldn't just use
>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>>>>>> values as they see fit.
>>>>>
>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
>>>
>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
>>>
>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
>>>
>>> The most common case for control handling in drivers is taking the
>>> integer value and converting it to a register value, using
>>> device-specific encoding of the register value. It can be a fixed-point
>>> format or something else, depending on the device. My point is that
>>> drivers routinely convert a "plain" integer to something else, and that
>>> has never been considered as a cause of concern. I don't see why it
>>> would be different in this series.
>>>
>>>>> And while we do not have support for units (other than the documentation),
>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>>>
>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>>>>>>> only shows a single driver specific control (dw100.rst).
>>>>>>>
>>>>>>> I'm not aware of other controls in mainline that use fixed point.
>>>>>>
>>>>>> The analog gain control for sensors for instance.
>>>>>
>>>>> Not really. The documentation is super vague:
>>>>>
>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>>>
>>>>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
>>>>> 	gain operation is performed in the analogue domain before A/D conversion.
>>>>>
>>>>> And the integer is just a range. Internally it might map to some fixed
>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
>>>
>>> It's hidden so well that libcamera has a database of the sensor it
>>> supports, with formulas to map a real gain value to the
>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
>>> matter, and the kernel doesn't expose it. We may or may not consider
>>> that as a shortcoming of the V4L2 control API, but in any case it's the
>>> situation we have today.
>>>
>>>> I wonder if Laurent meant digital gain.
>>>
>>> No, I meant analog. It applies to digital gain too though.
>>>
>>>> Those are often Q numbers. The practice there has been that the default
>>>> value yields gain of 1.
>>>>
>>>> There are probably many other examples in controls where something being
>>>> controlled isn't actually an integer while integer controls are still being
>>>> used for the purpose.
>>>
>>> A good summary of my opinion :-)
>>
>> And that works fine as long as userspace doesn't need to know what the value
>> actually means.
>>
>> That's not the case here. The control is really a fractional Hz value:
>>
>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
>> +    Sets the offset from the audio source sample rate, unit is Hz.
>> +    The offset compensates for any clock drift. The actual source audio sample
>> +    rate is the ideal source audio sample rate from
>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> 
> I don't see why this would require a new type, you can use
> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> fixed-point values in Q31.32 format.

Why would you want to do this? I can store a double in a long long int,
and just document that the variable is really a double, but why would you?

The cost of adding a FIXED_POINT type is minimal, and having this type
makes it easy to work with fixed point controls (think about proper reporting
and setting of the value in v4l2-ctl and user applications in general that
deal with controls).

If this would add a thousand lines of complex code, then this would be a
consideration, but this is just a few lines.

Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
and it reports the value 13958643712, would you be able to see that that is
really 3.25 in fixed point format? With the right type it would be printed
like that. Much easier to work work.

Regards,

	Hans

> 
>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
>>>> the control value, be it a Q number or something else, and do that
>>>> independently of the type of the control.
>>
>> Huh? How is that different from the type of the control? You have integers
>> (one type) and fixed point (another type).
>>
>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
>> explicitly?
>>
>> I think the main reason why we use integer controls for gain is that we
>> never had a fixed point control type and you could get away with that in
>> user space for that particular use-case.
>>
>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
>> be calculated as (value / default_value),
> 
> Typically, but not always. Some sensor have an exponential gain model,
> and some have weird gain representation, such as 1/x. That's getting out
> of scope though.
> 
>> but that won't work for a rate offset
>> control as above, or for e.g. CSC matrices for color converters.
>>
>>> Agreed.
>>>
>>>>> In the case of this particular series the control type is really a fixed point
>>>>> value with a documented unit (Hz). It really is not something you want to
>>>>> use type INTEGER64 for.
>>>>>
>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>>>>>>> min/max/step you can easily map that to just about any QN.M format where
>>>>>>> N <= 31 and M <= 32.
>>>>>>>
>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
>>>>>>> and it had to fit in 16 bits.
>

Hans Verkuil Nov. 15, 2023, 8:22 a.m. UTC | #11

On 10/11/2023 06:48, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> a fixed point value in 64bits, which Q31.32 format.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  .../userspace-api/media/v4l/vidioc-g-ext-ctrls.rst  | 13 +++++++------
>  .../userspace-api/media/v4l/vidioc-queryctrl.rst    |  9 ++++++++-
>  .../userspace-api/media/videodev2.h.rst.exceptions  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-api.c            |  5 ++++-
>  drivers/media/v4l2-core/v4l2-ctrls-core.c           |  2 ++
>  include/uapi/linux/videodev2.h                      |  1 +
>  6 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index e8475f9fd2cf..e7e5d78dc11e 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -162,13 +162,13 @@ still cause this situation.
>      * - __s32
>        - ``value``
>        - New value or current value. Valid if this control is not of type
> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> -	not set.
> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>      * - __s64
>        - ``value64``
>        - New value or current value. Valid if this control is of type
> -	``V4L2_CTRL_TYPE_INTEGER64`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is
> -	not set.
> +	``V4L2_CTRL_TYPE_INTEGER64``, ``V4L2_CTRL_TYPE_FIXED_POINT`` and
> +	``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is not set.
>      * - char *
>        - ``string``
>        - A pointer to a string. Valid if this control is of type
> @@ -193,8 +193,9 @@ still cause this situation.
>      * - __s64 *
>        - ``p_s64``
>        - A pointer to a matrix control of signed 64-bit values. Valid if
> -        this control is of type ``V4L2_CTRL_TYPE_INTEGER64`` and
> -        ``V4L2_CTRL_FLAG_HAS_PAYLOAD`` is set.
> +        this control is of type ``V4L2_CTRL_TYPE_INTEGER64``,
> +        ``V4L2_CTRL_TYPE_FIXED_POINT`` and ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> +        is set.
>      * - struct :c:type:`v4l2_area` *
>        - ``p_area``
>        - A pointer to a struct :c:type:`v4l2_area`. Valid if this control is
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 4d38acafe8e1..f3995ec57044 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -235,7 +235,8 @@ See also the examples in :ref:`control`.
>        - ``default_value``
>        - The default value of a ``V4L2_CTRL_TYPE_INTEGER``, ``_INTEGER64``,
>  	``_BOOLEAN``, ``_BITMASK``, ``_MENU``, ``_INTEGER_MENU``, ``_U8``
> -	or ``_U16`` control. Not valid for other types of controls.
> +	``_FIXED_POINT`` or ``_U16`` control. Not valid for other types of
> +	controls.
>  
>  	.. note::
>  
> @@ -549,6 +550,12 @@ See also the examples in :ref:`control`.
>        - n/a
>        - A struct :c:type:`v4l2_ctrl_av1_film_grain`, containing AV1 Film Grain
>          parameters for stateless video decoders.
> +    * - ``V4L2_CTRL_TYPE_FIXED_POINT``
> +      - any
> +      - any
> +      - any
> +      - A 64-bit integer valued control, containing parameter which is
> +        Q31.32 format.
>  
>  .. raw:: latex
>  
> diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> index e61152bb80d1..2faa5a2015eb 100644
> --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> @@ -167,6 +167,7 @@ replace symbol V4L2_CTRL_TYPE_AV1_SEQUENCE :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FRAME :c:type:`v4l2_ctrl_type`
>  replace symbol V4L2_CTRL_TYPE_AV1_FILM_GRAIN :c:type:`v4l2_ctrl_type`
> +replace symbol V4L2_CTRL_TYPE_FIXED_POINT :c:type:`v4l2_ctrl_type`
>  
>  # V4L2 capability defines
>  replace define V4L2_CAP_VIDEO_CAPTURE device-capabilities
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..e6a0fb8d6791 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -57,6 +57,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
>  		return copy_to_user(c->string, ptr.p_char, len + 1) ?
>  		       -EFAULT : 0;
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:n
>  		c->value64 = *ptr.p_s64;
>  		break;
>  	default:
> @@ -132,6 +133,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>  
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		*ctrl->p_new.p_s64 = c->value64;
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
> @@ -540,7 +542,8 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
>  		 */
>  		if (ctrl->is_ptr)
>  			continue;
> -		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64)
> +		if (ctrl->type == V4L2_CTRL_TYPE_INTEGER64 ||
> +		    ctrl->type == V4L2_CTRL_TYPE_FIXED_POINT)
>  			p_new.p_s64 = &cs->controls[i].value64;
>  		else
>  			p_new.p_s32 = &cs->controls[i].value;

You missed a few more: everywhere in this source where V4L2_CTRL_TYPE_INTEGER64
is used you need to add a check for FIXED_POINT as well.

> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..9d50df0d9874 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c

Same here for this source.

Note that v4l2_ctrl_type_op_log() needs a separate case for FIXED_POINT so that it
can be logged properly.

> @@ -1187,6 +1187,7 @@ static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_INTEGER:
>  		return ROUND_TO_RANGE(ptr.p_s32[idx], u32, ctrl);
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		/*
>  		 * We can't use the ROUND_TO_RANGE define here due to
>  		 * the u64 divide that needs special care.
> @@ -1779,6 +1780,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	/* Prefill elem_size for all types handled by std_type_ops */
>  	switch ((u32)type) {
>  	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
>  		elem_size = sizeof(s64);
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:

There is one more place where V4L2_CTRL_TYPE_INTEGER64 is used:
drivers/media/v4l2-core/v4l2-ioctl.c. That should report a fixed point
value in the same way as v4l2_ctrl_type_op_log().

> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index cf8c44595a1d..9482ac66a675 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1903,6 +1903,7 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_STRING        = 7,
>  	V4L2_CTRL_TYPE_BITMASK       = 8,
>  	V4L2_CTRL_TYPE_INTEGER_MENU  = 9,
> +	V4L2_CTRL_TYPE_FIXED_POINT   = 10,
>  
>  	/* Compound types are >= 0x0100 */
>  	V4L2_CTRL_COMPOUND_TYPES     = 0x0100,

We will probably need a helper to create a fixed point value from the integer
and fractional part, and one to extract them.

Regards,

	Hans

Tomasz Figa Nov. 15, 2023, 8:45 a.m. UTC | #12

On Wed, Nov 15, 2023 at 5:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Laurent,
>
> On 13/11/2023 13:44, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> >> On 13/11/2023 12:43, Laurent Pinchart wrote:
> >>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> >>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> >>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>>>
> >>>>>>>>> This patch adds a new control type. This is something that also needs to be
> >>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>>>>>>> since that has already a bunch of test controls for other control types.
> >>>>>>>>>
> >>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>>>>>>
> >>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> >>>>>>>>
> >>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>>>>>>> relate more to units than control types. We have lots of fixed-point
> >>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> >>>>>>>> types. They use various locations for the decimal point, depending on
> >>>>>>>> the control. If we want to make this more explicit to users, we should
> >>>>>>>> work on adding unit support to the V4L2 controls.
> >>>>>>>
> >>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> >>>>>>
> >>>>>> It's not a unit, but I think it's related to units. My point is that,
> >>>>>> without units support, I don't see why we need a formal definition of
> >>>>>> fixed-point types, and why this series couldn't just use
> >>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> >>>>>> values as they see fit.
> >>>>>
> >>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> >>>
> >>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> >>>
> >>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> >>>
> >>> The most common case for control handling in drivers is taking the
> >>> integer value and converting it to a register value, using
> >>> device-specific encoding of the register value. It can be a fixed-point
> >>> format or something else, depending on the device. My point is that
> >>> drivers routinely convert a "plain" integer to something else, and that
> >>> has never been considered as a cause of concern. I don't see why it
> >>> would be different in this series.
> >>>
> >>>>> And while we do not have support for units (other than the documentation),
> >>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>>>
> >>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >>>>>>> only shows a single driver specific control (dw100.rst).
> >>>>>>>
> >>>>>>> I'm not aware of other controls in mainline that use fixed point.
> >>>>>>
> >>>>>> The analog gain control for sensors for instance.
> >>>>>
> >>>>> Not really. The documentation is super vague:
> >>>>>
> >>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>>>
> >>>>>   Analogue gain is gain affecting all colour components in the pixel matrix. The
> >>>>>   gain operation is performed in the analogue domain before A/D conversion.
> >>>>>
> >>>>> And the integer is just a range. Internally it might map to some fixed
> >>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> >>>
> >>> It's hidden so well that libcamera has a database of the sensor it
> >>> supports, with formulas to map a real gain value to the
> >>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> >>> matter, and the kernel doesn't expose it. We may or may not consider
> >>> that as a shortcoming of the V4L2 control API, but in any case it's the
> >>> situation we have today.
> >>>
> >>>> I wonder if Laurent meant digital gain.
> >>>
> >>> No, I meant analog. It applies to digital gain too though.
> >>>
> >>>> Those are often Q numbers. The practice there has been that the default
> >>>> value yields gain of 1.
> >>>>
> >>>> There are probably many other examples in controls where something being
> >>>> controlled isn't actually an integer while integer controls are still being
> >>>> used for the purpose.
> >>>
> >>> A good summary of my opinion :-)
> >>
> >> And that works fine as long as userspace doesn't need to know what the value
> >> actually means.
> >>
> >> That's not the case here. The control is really a fractional Hz value:
> >>
> >> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> >> +    Sets the offset from the audio source sample rate, unit is Hz.
> >> +    The offset compensates for any clock drift. The actual source audio sample
> >> +    rate is the ideal source audio sample rate from
> >> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> >
> > I don't see why this would require a new type, you can use
> > V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> > fixed-point values in Q31.32 format.
>
> Why would you want to do this? I can store a double in a long long int,
> and just document that the variable is really a double, but why would you?
>
> The cost of adding a FIXED_POINT type is minimal, and having this type
> makes it easy to work with fixed point controls (think about proper reporting
> and setting of the value in v4l2-ctl and user applications in general that
> deal with controls).

I can see one potential drawback of adding a new type - userspace
would have to be made aware of it, although arguably with brand new
controls, userspace would have to be aware of them anyway. Not sure if
we have some kind of userspace that can handle any controls purely
based on their type - if yes, they would not be able to handle the new
controls.

>
> If this would add a thousand lines of complex code, then this would be a
> consideration, but this is just a few lines.
>
> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> and it reports the value 13958643712, would you be able to see that that is
> really 3.25 in fixed point format? With the right type it would be printed
> like that. Much easier to work work.
>
> Regards,
>
>         Hans
>
> >
> >>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> >>>> the control value, be it a Q number or something else, and do that
> >>>> independently of the type of the control.
> >>
> >> Huh? How is that different from the type of the control? You have integers
> >> (one type) and fixed point (another type).
> >>
> >> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> >> explicitly?
> >>
> >> I think the main reason why we use integer controls for gain is that we
> >> never had a fixed point control type and you could get away with that in
> >> user space for that particular use-case.
> >>
> >> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> >> be calculated as (value / default_value),
> >
> > Typically, but not always. Some sensor have an exponential gain model,
> > and some have weird gain representation, such as 1/x. That's getting out
> > of scope though.
> >
> >> but that won't work for a rate offset
> >> control as above, or for e.g. CSC matrices for color converters.
> >>
> >>> Agreed.
> >>>
> >>>>> In the case of this particular series the control type is really a fixed point
> >>>>> value with a documented unit (Hz). It really is not something you want to
> >>>>> use type INTEGER64 for.
> >>>>>
> >>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >>>>>>> min/max/step you can easily map that to just about any QN.M format where
> >>>>>>> N <= 31 and M <= 32.
> >>>>>>>
> >>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> >>>>>>> and it had to fit in 16 bits.
> >
>

Hans Verkuil Nov. 15, 2023, 9:13 a.m. UTC | #13

On 15/11/2023 09:45, Tomasz Figa wrote:
> On Wed, Nov 15, 2023 at 5:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Laurent,
>>
>> On 13/11/2023 13:44, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
>>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
>>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
>>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>>>
>>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
>>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>>>>>>>>> since that has already a bunch of test controls for other control types.
>>>>>>>>>>>
>>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>>>>>>>>
>>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
>>>>>>>>>>
>>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>>>>>>>>> relate more to units than control types. We have lots of fixed-point
>>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
>>>>>>>>>> types. They use various locations for the decimal point, depending on
>>>>>>>>>> the control. If we want to make this more explicit to users, we should
>>>>>>>>>> work on adding unit support to the V4L2 controls.
>>>>>>>>>
>>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
>>>>>>>>
>>>>>>>> It's not a unit, but I think it's related to units. My point is that,
>>>>>>>> without units support, I don't see why we need a formal definition of
>>>>>>>> fixed-point types, and why this series couldn't just use
>>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>>>>>>>> values as they see fit.
>>>>>>>
>>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
>>>>>
>>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
>>>>>
>>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
>>>>>
>>>>> The most common case for control handling in drivers is taking the
>>>>> integer value and converting it to a register value, using
>>>>> device-specific encoding of the register value. It can be a fixed-point
>>>>> format or something else, depending on the device. My point is that
>>>>> drivers routinely convert a "plain" integer to something else, and that
>>>>> has never been considered as a cause of concern. I don't see why it
>>>>> would be different in this series.
>>>>>
>>>>>>> And while we do not have support for units (other than the documentation),
>>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>>>>>
>>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>>>>>>>>> only shows a single driver specific control (dw100.rst).
>>>>>>>>>
>>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
>>>>>>>>
>>>>>>>> The analog gain control for sensors for instance.
>>>>>>>
>>>>>>> Not really. The documentation is super vague:
>>>>>>>
>>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>>>>>
>>>>>>>   Analogue gain is gain affecting all colour components in the pixel matrix. The
>>>>>>>   gain operation is performed in the analogue domain before A/D conversion.
>>>>>>>
>>>>>>> And the integer is just a range. Internally it might map to some fixed
>>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
>>>>>
>>>>> It's hidden so well that libcamera has a database of the sensor it
>>>>> supports, with formulas to map a real gain value to the
>>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
>>>>> matter, and the kernel doesn't expose it. We may or may not consider
>>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
>>>>> situation we have today.
>>>>>
>>>>>> I wonder if Laurent meant digital gain.
>>>>>
>>>>> No, I meant analog. It applies to digital gain too though.
>>>>>
>>>>>> Those are often Q numbers. The practice there has been that the default
>>>>>> value yields gain of 1.
>>>>>>
>>>>>> There are probably many other examples in controls where something being
>>>>>> controlled isn't actually an integer while integer controls are still being
>>>>>> used for the purpose.
>>>>>
>>>>> A good summary of my opinion :-)
>>>>
>>>> And that works fine as long as userspace doesn't need to know what the value
>>>> actually means.
>>>>
>>>> That's not the case here. The control is really a fractional Hz value:
>>>>
>>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
>>>> +    Sets the offset from the audio source sample rate, unit is Hz.
>>>> +    The offset compensates for any clock drift. The actual source audio sample
>>>> +    rate is the ideal source audio sample rate from
>>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
>>>
>>> I don't see why this would require a new type, you can use
>>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
>>> fixed-point values in Q31.32 format.
>>
>> Why would you want to do this? I can store a double in a long long int,
>> and just document that the variable is really a double, but why would you?
>>
>> The cost of adding a FIXED_POINT type is minimal, and having this type
>> makes it easy to work with fixed point controls (think about proper reporting
>> and setting of the value in v4l2-ctl and user applications in general that
>> deal with controls).
> 
> I can see one potential drawback of adding a new type - userspace
> would have to be made aware of it, although arguably with brand new
> controls, userspace would have to be aware of them anyway. Not sure if
> we have some kind of userspace that can handle any controls purely
> based on their type - if yes, they would not be able to handle the new
> controls.

The generic userspace apps are all in v4l-utils (v4l2-ctl/compliance, qv4l2),
so that is not a problem. We add new types all the time, this is nothing
new.

Regards,

	Hans

> 
>>
>> If this would add a thousand lines of complex code, then this would be a
>> consideration, but this is just a few lines.
>>
>> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
>> and it reports the value 13958643712, would you be able to see that that is
>> really 3.25 in fixed point format? With the right type it would be printed
>> like that. Much easier to work work.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
>>>>>> the control value, be it a Q number or something else, and do that
>>>>>> independently of the type of the control.
>>>>
>>>> Huh? How is that different from the type of the control? You have integers
>>>> (one type) and fixed point (another type).
>>>>
>>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
>>>> explicitly?
>>>>
>>>> I think the main reason why we use integer controls for gain is that we
>>>> never had a fixed point control type and you could get away with that in
>>>> user space for that particular use-case.
>>>>
>>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
>>>> be calculated as (value / default_value),
>>>
>>> Typically, but not always. Some sensor have an exponential gain model,
>>> and some have weird gain representation, such as 1/x. That's getting out
>>> of scope though.
>>>
>>>> but that won't work for a rate offset
>>>> control as above, or for e.g. CSC matrices for color converters.
>>>>
>>>>> Agreed.
>>>>>
>>>>>>> In the case of this particular series the control type is really a fixed point
>>>>>>> value with a documented unit (Hz). It really is not something you want to
>>>>>>> use type INTEGER64 for.
>>>>>>>
>>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
>>>>>>>>> N <= 31 and M <= 32.
>>>>>>>>>
>>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
>>>>>>>>> and it had to fit in 16 bits.
>>>
>>

Laurent Pinchart Nov. 15, 2023, 10:55 a.m. UTC | #14

Hi Hans,

On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
> On 13/11/2023 13:44, Laurent Pinchart wrote:
> > On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> >> On 13/11/2023 12:43, Laurent Pinchart wrote:
> >>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> >>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> >>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>>>
> >>>>>>>>> This patch adds a new control type. This is something that also needs to be
> >>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>>>>>>> since that has already a bunch of test controls for other control types.
> >>>>>>>>>
> >>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>>>>>>
> >>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> >>>>>>>>
> >>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>>>>>>> relate more to units than control types. We have lots of fixed-point
> >>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> >>>>>>>> types. They use various locations for the decimal point, depending on
> >>>>>>>> the control. If we want to make this more explicit to users, we should
> >>>>>>>> work on adding unit support to the V4L2 controls.
> >>>>>>>
> >>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> >>>>>>
> >>>>>> It's not a unit, but I think it's related to units. My point is that,
> >>>>>> without units support, I don't see why we need a formal definition of
> >>>>>> fixed-point types, and why this series couldn't just use
> >>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> >>>>>> values as they see fit.
> >>>>>
> >>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> >>>
> >>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> >>>
> >>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> >>>
> >>> The most common case for control handling in drivers is taking the
> >>> integer value and converting it to a register value, using
> >>> device-specific encoding of the register value. It can be a fixed-point
> >>> format or something else, depending on the device. My point is that
> >>> drivers routinely convert a "plain" integer to something else, and that
> >>> has never been considered as a cause of concern. I don't see why it
> >>> would be different in this series.
> >>>
> >>>>> And while we do not have support for units (other than the documentation),
> >>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>>>
> >>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >>>>>>> only shows a single driver specific control (dw100.rst).
> >>>>>>>
> >>>>>>> I'm not aware of other controls in mainline that use fixed point.
> >>>>>>
> >>>>>> The analog gain control for sensors for instance.
> >>>>>
> >>>>> Not really. The documentation is super vague:
> >>>>>
> >>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>>>
> >>>>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
> >>>>> 	gain operation is performed in the analogue domain before A/D conversion.
> >>>>>
> >>>>> And the integer is just a range. Internally it might map to some fixed
> >>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> >>>
> >>> It's hidden so well that libcamera has a database of the sensor it
> >>> supports, with formulas to map a real gain value to the
> >>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> >>> matter, and the kernel doesn't expose it. We may or may not consider
> >>> that as a shortcoming of the V4L2 control API, but in any case it's the
> >>> situation we have today.
> >>>
> >>>> I wonder if Laurent meant digital gain.
> >>>
> >>> No, I meant analog. It applies to digital gain too though.
> >>>
> >>>> Those are often Q numbers. The practice there has been that the default
> >>>> value yields gain of 1.
> >>>>
> >>>> There are probably many other examples in controls where something being
> >>>> controlled isn't actually an integer while integer controls are still being
> >>>> used for the purpose.
> >>>
> >>> A good summary of my opinion :-)
> >>
> >> And that works fine as long as userspace doesn't need to know what the value
> >> actually means.
> >>
> >> That's not the case here. The control is really a fractional Hz value:
> >>
> >> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> >> +    Sets the offset from the audio source sample rate, unit is Hz.
> >> +    The offset compensates for any clock drift. The actual source audio sample
> >> +    rate is the ideal source audio sample rate from
> >> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> > 
> > I don't see why this would require a new type, you can use
> > V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> > fixed-point values in Q31.32 format.
> 
> Why would you want to do this? I can store a double in a long long int,
> and just document that the variable is really a double, but why would you?

I'm happy we have no floating point control types ;-)

> The cost of adding a FIXED_POINT type is minimal, and having this type
> makes it easy to work with fixed point controls (think about proper reporting
> and setting of the value in v4l2-ctl and user applications in general that
> deal with controls).

The next thing you know is that someone will want a FIXED_POINT_Q15_16
type as 64-bit would be too large to store in a large array. And then
Q7.8. And Q3.12. And a bunch of other type. I really don't see what
added value they bring compared to using the 32-bit and 64-bit integer
types we already have. Every new type that is added adds complexity to
userspace that will need to deal with the type.

> If this would add a thousand lines of complex code, then this would be a
> consideration, but this is just a few lines.
> 
> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> and it reports the value 13958643712, would you be able to see that that is
> really 3.25 in fixed point format? With the right type it would be printed
> like that. Much easier to work work.

The same is true for analog gains, where x1.23 or +12dB is nicer to read
than raw values. If we care about printing values in command line tools
(which is nice to have, but certainly not the majority of use cases),
then I would recommand working on units support for V4L2 controls, to
convey how values are encoded, and in what unit they are expressed.

> >>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> >>>> the control value, be it a Q number or something else, and do that
> >>>> independently of the type of the control.
> >>
> >> Huh? How is that different from the type of the control? You have integers
> >> (one type) and fixed point (another type).
> >>
> >> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> >> explicitly?
> >>
> >> I think the main reason why we use integer controls for gain is that we
> >> never had a fixed point control type and you could get away with that in
> >> user space for that particular use-case.
> >>
> >> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> >> be calculated as (value / default_value),
> > 
> > Typically, but not always. Some sensor have an exponential gain model,
> > and some have weird gain representation, such as 1/x. That's getting out
> > of scope though.
> > 
> >> but that won't work for a rate offset
> >> control as above, or for e.g. CSC matrices for color converters.
> >>
> >>> Agreed.
> >>>
> >>>>> In the case of this particular series the control type is really a fixed point
> >>>>> value with a documented unit (Hz). It really is not something you want to
> >>>>> use type INTEGER64 for.
> >>>>>
> >>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >>>>>>> min/max/step you can easily map that to just about any QN.M format where
> >>>>>>> N <= 31 and M <= 32.
> >>>>>>>
> >>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> >>>>>>> and it had to fit in 16 bits.

Hans Verkuil Nov. 15, 2023, 11:19 a.m. UTC | #15

On 11/15/23 11:55, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
>> On 13/11/2023 13:44, Laurent Pinchart wrote:
>>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
>>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
>>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
>>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>>>
>>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
>>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>>>>>>>>> since that has already a bunch of test controls for other control types.
>>>>>>>>>>>
>>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>>>>>>>>
>>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
>>>>>>>>>>
>>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>>>>>>>>> relate more to units than control types. We have lots of fixed-point
>>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
>>>>>>>>>> types. They use various locations for the decimal point, depending on
>>>>>>>>>> the control. If we want to make this more explicit to users, we should
>>>>>>>>>> work on adding unit support to the V4L2 controls.
>>>>>>>>>
>>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
>>>>>>>>
>>>>>>>> It's not a unit, but I think it's related to units. My point is that,
>>>>>>>> without units support, I don't see why we need a formal definition of
>>>>>>>> fixed-point types, and why this series couldn't just use
>>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>>>>>>>> values as they see fit.
>>>>>>>
>>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
>>>>>
>>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
>>>>>
>>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
>>>>>
>>>>> The most common case for control handling in drivers is taking the
>>>>> integer value and converting it to a register value, using
>>>>> device-specific encoding of the register value. It can be a fixed-point
>>>>> format or something else, depending on the device. My point is that
>>>>> drivers routinely convert a "plain" integer to something else, and that
>>>>> has never been considered as a cause of concern. I don't see why it
>>>>> would be different in this series.
>>>>>
>>>>>>> And while we do not have support for units (other than the documentation),
>>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>>>>>
>>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>>>>>>>>> only shows a single driver specific control (dw100.rst).
>>>>>>>>>
>>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
>>>>>>>>
>>>>>>>> The analog gain control for sensors for instance.
>>>>>>>
>>>>>>> Not really. The documentation is super vague:
>>>>>>>
>>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>>>>>
>>>>>>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
>>>>>>> 	gain operation is performed in the analogue domain before A/D conversion.
>>>>>>>
>>>>>>> And the integer is just a range. Internally it might map to some fixed
>>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
>>>>>
>>>>> It's hidden so well that libcamera has a database of the sensor it
>>>>> supports, with formulas to map a real gain value to the
>>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
>>>>> matter, and the kernel doesn't expose it. We may or may not consider
>>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
>>>>> situation we have today.
>>>>>
>>>>>> I wonder if Laurent meant digital gain.
>>>>>
>>>>> No, I meant analog. It applies to digital gain too though.
>>>>>
>>>>>> Those are often Q numbers. The practice there has been that the default
>>>>>> value yields gain of 1.
>>>>>>
>>>>>> There are probably many other examples in controls where something being
>>>>>> controlled isn't actually an integer while integer controls are still being
>>>>>> used for the purpose.
>>>>>
>>>>> A good summary of my opinion :-)
>>>>
>>>> And that works fine as long as userspace doesn't need to know what the value
>>>> actually means.
>>>>
>>>> That's not the case here. The control is really a fractional Hz value:
>>>>
>>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
>>>> +    Sets the offset from the audio source sample rate, unit is Hz.
>>>> +    The offset compensates for any clock drift. The actual source audio sample
>>>> +    rate is the ideal source audio sample rate from
>>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
>>>
>>> I don't see why this would require a new type, you can use
>>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
>>> fixed-point values in Q31.32 format.
>>
>> Why would you want to do this? I can store a double in a long long int,
>> and just document that the variable is really a double, but why would you?
> 
> I'm happy we have no floating point control types ;-)
> 
>> The cost of adding a FIXED_POINT type is minimal, and having this type
>> makes it easy to work with fixed point controls (think about proper reporting
>> and setting of the value in v4l2-ctl and user applications in general that
>> deal with controls).
> 
> The next thing you know is that someone will want a FIXED_POINT_Q15_16
> type as 64-bit would be too large to store in a large array. And then
> Q7.8. And Q3.12. And a bunch of other type. I really don't see what
> added value they bring compared to using the 32-bit and 64-bit integer
> types we already have. Every new type that is added adds complexity to
> userspace that will need to deal with the type.
> 
>> If this would add a thousand lines of complex code, then this would be a
>> consideration, but this is just a few lines.
>>
>> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
>> and it reports the value 13958643712, would you be able to see that that is
>> really 3.25 in fixed point format? With the right type it would be printed
>> like that. Much easier to work work.
> 
> The same is true for analog gains, where x1.23 or +12dB is nicer to read
> than raw values. If we care about printing values in command line tools
> (which is nice to have, but certainly not the majority of use cases),
> then I would recommand working on units support for V4L2 controls, to
> convey how values are encoded, and in what unit they are expressed.

So you prefer to have a way to specify the N value in QM.N as part
of the control information?

E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
in QM.N.

I can go along with that. This would be valid for INTEGER, INTEGER64,
U8, U16 and U32 controls (the last three are only used in control arrays).

A better name for 'fraction_bits' is welcome, I took it from the wikipedia
article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic

Reporting unit names is certainly possible, but should perhaps be done
with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
needed for applications, unless they need to report values. In theory
it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
4 of the reserved fields for a 'char unit[16];' field. But I feel a
bit uncomfortable taking reserved fields for something that is rarely
needed.

Regards,

	Hans

> 
>>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
>>>>>> the control value, be it a Q number or something else, and do that
>>>>>> independently of the type of the control.
>>>>
>>>> Huh? How is that different from the type of the control? You have integers
>>>> (one type) and fixed point (another type).
>>>>
>>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
>>>> explicitly?
>>>>
>>>> I think the main reason why we use integer controls for gain is that we
>>>> never had a fixed point control type and you could get away with that in
>>>> user space for that particular use-case.
>>>>
>>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
>>>> be calculated as (value / default_value),
>>>
>>> Typically, but not always. Some sensor have an exponential gain model,
>>> and some have weird gain representation, such as 1/x. That's getting out
>>> of scope though.
>>>
>>>> but that won't work for a rate offset
>>>> control as above, or for e.g. CSC matrices for color converters.
>>>>
>>>>> Agreed.
>>>>>
>>>>>>> In the case of this particular series the control type is really a fixed point
>>>>>>> value with a documented unit (Hz). It really is not something you want to
>>>>>>> use type INTEGER64 for.
>>>>>>>
>>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
>>>>>>>>> N <= 31 and M <= 32.
>>>>>>>>>
>>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
>>>>>>>>> and it had to fit in 16 bits.
>

Laurent Pinchart Nov. 15, 2023, 11:49 a.m. UTC | #16

Hi Hans,

On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote:
> On 11/15/23 11:55, Laurent Pinchart wrote:
> > On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
> >> On 13/11/2023 13:44, Laurent Pinchart wrote:
> >>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> >>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
> >>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> >>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> >>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>>>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>>>>>
> >>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
> >>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>>>>>>>>> since that has already a bunch of test controls for other control types.
> >>>>>>>>>>>
> >>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>>>>>>>>
> >>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> >>>>>>>>>>
> >>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>>>>>>>>> relate more to units than control types. We have lots of fixed-point
> >>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> >>>>>>>>>> types. They use various locations for the decimal point, depending on
> >>>>>>>>>> the control. If we want to make this more explicit to users, we should
> >>>>>>>>>> work on adding unit support to the V4L2 controls.
> >>>>>>>>>
> >>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> >>>>>>>>
> >>>>>>>> It's not a unit, but I think it's related to units. My point is that,
> >>>>>>>> without units support, I don't see why we need a formal definition of
> >>>>>>>> fixed-point types, and why this series couldn't just use
> >>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> >>>>>>>> values as they see fit.
> >>>>>>>
> >>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> >>>>>
> >>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> >>>>>
> >>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> >>>>>
> >>>>> The most common case for control handling in drivers is taking the
> >>>>> integer value and converting it to a register value, using
> >>>>> device-specific encoding of the register value. It can be a fixed-point
> >>>>> format or something else, depending on the device. My point is that
> >>>>> drivers routinely convert a "plain" integer to something else, and that
> >>>>> has never been considered as a cause of concern. I don't see why it
> >>>>> would be different in this series.
> >>>>>
> >>>>>>> And while we do not have support for units (other than the documentation),
> >>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>>>>>
> >>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >>>>>>>>> only shows a single driver specific control (dw100.rst).
> >>>>>>>>>
> >>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
> >>>>>>>>
> >>>>>>>> The analog gain control for sensors for instance.
> >>>>>>>
> >>>>>>> Not really. The documentation is super vague:
> >>>>>>>
> >>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>>>>>
> >>>>>>> 	Analogue gain is gain affecting all colour components in the pixel matrix. The
> >>>>>>> 	gain operation is performed in the analogue domain before A/D conversion.
> >>>>>>>
> >>>>>>> And the integer is just a range. Internally it might map to some fixed
> >>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> >>>>>
> >>>>> It's hidden so well that libcamera has a database of the sensor it
> >>>>> supports, with formulas to map a real gain value to the
> >>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> >>>>> matter, and the kernel doesn't expose it. We may or may not consider
> >>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
> >>>>> situation we have today.
> >>>>>
> >>>>>> I wonder if Laurent meant digital gain.
> >>>>>
> >>>>> No, I meant analog. It applies to digital gain too though.
> >>>>>
> >>>>>> Those are often Q numbers. The practice there has been that the default
> >>>>>> value yields gain of 1.
> >>>>>>
> >>>>>> There are probably many other examples in controls where something being
> >>>>>> controlled isn't actually an integer while integer controls are still being
> >>>>>> used for the purpose.
> >>>>>
> >>>>> A good summary of my opinion :-)
> >>>>
> >>>> And that works fine as long as userspace doesn't need to know what the value
> >>>> actually means.
> >>>>
> >>>> That's not the case here. The control is really a fractional Hz value:
> >>>>
> >>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> >>>> +    Sets the offset from the audio source sample rate, unit is Hz.
> >>>> +    The offset compensates for any clock drift. The actual source audio sample
> >>>> +    rate is the ideal source audio sample rate from
> >>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> >>>
> >>> I don't see why this would require a new type, you can use
> >>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> >>> fixed-point values in Q31.32 format.
> >>
> >> Why would you want to do this? I can store a double in a long long int,
> >> and just document that the variable is really a double, but why would you?
> > 
> > I'm happy we have no floating point control types ;-)
> > 
> >> The cost of adding a FIXED_POINT type is minimal, and having this type
> >> makes it easy to work with fixed point controls (think about proper reporting
> >> and setting of the value in v4l2-ctl and user applications in general that
> >> deal with controls).
> > 
> > The next thing you know is that someone will want a FIXED_POINT_Q15_16
> > type as 64-bit would be too large to store in a large array. And then
> > Q7.8. And Q3.12. And a bunch of other type. I really don't see what
> > added value they bring compared to using the 32-bit and 64-bit integer
> > types we already have. Every new type that is added adds complexity to
> > userspace that will need to deal with the type.
> > 
> >> If this would add a thousand lines of complex code, then this would be a
> >> consideration, but this is just a few lines.
> >>
> >> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> >> and it reports the value 13958643712, would you be able to see that that is
> >> really 3.25 in fixed point format? With the right type it would be printed
> >> like that. Much easier to work work.
> > 
> > The same is true for analog gains, where x1.23 or +12dB is nicer to read
> > than raw values. If we care about printing values in command line tools
> > (which is nice to have, but certainly not the majority of use cases),
> > then I would recommand working on units support for V4L2 controls, to
> > convey how values are encoded, and in what unit they are expressed.
> 
> So you prefer to have a way to specify the N value in QM.N as part
> of the control information?
> 
> E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
> and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
> in QM.N.
> 
> I can go along with that. This would be valid for INTEGER, INTEGER64,
> U8, U16 and U32 controls (the last three are only used in control arrays).

I think that would be nicer. Not only is it more flexible, but it also
allows applications to ignore that information, and still operate on
integer controls without any modification.

> A better name for 'fraction_bits' is welcome, I took it from the wikipedia
> article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic
> 
> Reporting unit names is certainly possible, but should perhaps be done
> with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
> needed for applications, unless they need to report values. In theory
> it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
> 4 of the reserved fields for a 'char unit[16];' field. But I feel a
> bit uncomfortable taking reserved fields for something that is rarely
> needed.

I would make the unit an enumerated integer value. If it's a string, it
gets more difficult to operate on. Having to standardize a unit means
that the unit will get reviewed.

> >>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> >>>>>> the control value, be it a Q number or something else, and do that
> >>>>>> independently of the type of the control.
> >>>>
> >>>> Huh? How is that different from the type of the control? You have integers
> >>>> (one type) and fixed point (another type).
> >>>>
> >>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> >>>> explicitly?
> >>>>
> >>>> I think the main reason why we use integer controls for gain is that we
> >>>> never had a fixed point control type and you could get away with that in
> >>>> user space for that particular use-case.
> >>>>
> >>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> >>>> be calculated as (value / default_value),
> >>>
> >>> Typically, but not always. Some sensor have an exponential gain model,
> >>> and some have weird gain representation, such as 1/x. That's getting out
> >>> of scope though.
> >>>
> >>>> but that won't work for a rate offset
> >>>> control as above, or for e.g. CSC matrices for color converters.
> >>>>
> >>>>> Agreed.
> >>>>>
> >>>>>>> In the case of this particular series the control type is really a fixed point
> >>>>>>> value with a documented unit (Hz). It really is not something you want to
> >>>>>>> use type INTEGER64 for.
> >>>>>>>
> >>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
> >>>>>>>>> N <= 31 and M <= 32.
> >>>>>>>>>
> >>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> >>>>>>>>> and it had to fit in 16 bits.

Tomasz Figa Nov. 16, 2023, 7:31 a.m. UTC | #17

On Wed, Nov 15, 2023 at 8:49 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hans,
>
> On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote:
> > On 11/15/23 11:55, Laurent Pinchart wrote:
> > > On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
> > >> On 13/11/2023 13:44, Laurent Pinchart wrote:
> > >>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> > >>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
> > >>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> > >>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> > >>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> > >>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> > >>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > >>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> > >>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> > >>>>>>>>>>>> Fixed point controls are used by the user to configure
> > >>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> > >>>>>>>>>>>>
> > >>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > >>>>>>>>>>>
> > >>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
> > >>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> > >>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> > >>>>>>>>>>> since that has already a bunch of test controls for other control types.
> > >>>>>>>>>>>
> > >>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> > >>>>>>>>>>
> > >>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > >>>>>>>>>> relate more to units than control types. We have lots of fixed-point
> > >>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> > >>>>>>>>>> types. They use various locations for the decimal point, depending on
> > >>>>>>>>>> the control. If we want to make this more explicit to users, we should
> > >>>>>>>>>> work on adding unit support to the V4L2 controls.
> > >>>>>>>>>
> > >>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > >>>>>>>>
> > >>>>>>>> It's not a unit, but I think it's related to units. My point is that,
> > >>>>>>>> without units support, I don't see why we need a formal definition of
> > >>>>>>>> fixed-point types, and why this series couldn't just use
> > >>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > >>>>>>>> values as they see fit.
> > >>>>>>>
> > >>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> > >>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> > >>>>>
> > >>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> > >>>>>
> > >>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> > >>>>>
> > >>>>> The most common case for control handling in drivers is taking the
> > >>>>> integer value and converting it to a register value, using
> > >>>>> device-specific encoding of the register value. It can be a fixed-point
> > >>>>> format or something else, depending on the device. My point is that
> > >>>>> drivers routinely convert a "plain" integer to something else, and that
> > >>>>> has never been considered as a cause of concern. I don't see why it
> > >>>>> would be different in this series.
> > >>>>>
> > >>>>>>> And while we do not have support for units (other than the documentation),
> > >>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> > >>>>>>>
> > >>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> > >>>>>>>>> only shows a single driver specific control (dw100.rst).
> > >>>>>>>>>
> > >>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
> > >>>>>>>>
> > >>>>>>>> The analog gain control for sensors for instance.
> > >>>>>>>
> > >>>>>>> Not really. The documentation is super vague:
> > >>>>>>>
> > >>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> > >>>>>>>
> > >>>>>>>       Analogue gain is gain affecting all colour components in the pixel matrix. The
> > >>>>>>>       gain operation is performed in the analogue domain before A/D conversion.
> > >>>>>>>
> > >>>>>>> And the integer is just a range. Internally it might map to some fixed
> > >>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> > >>>>>
> > >>>>> It's hidden so well that libcamera has a database of the sensor it
> > >>>>> supports, with formulas to map a real gain value to the
> > >>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> > >>>>> matter, and the kernel doesn't expose it. We may or may not consider
> > >>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
> > >>>>> situation we have today.
> > >>>>>
> > >>>>>> I wonder if Laurent meant digital gain.
> > >>>>>
> > >>>>> No, I meant analog. It applies to digital gain too though.
> > >>>>>
> > >>>>>> Those are often Q numbers. The practice there has been that the default
> > >>>>>> value yields gain of 1.
> > >>>>>>
> > >>>>>> There are probably many other examples in controls where something being
> > >>>>>> controlled isn't actually an integer while integer controls are still being
> > >>>>>> used for the purpose.
> > >>>>>
> > >>>>> A good summary of my opinion :-)
> > >>>>
> > >>>> And that works fine as long as userspace doesn't need to know what the value
> > >>>> actually means.
> > >>>>
> > >>>> That's not the case here. The control is really a fractional Hz value:
> > >>>>
> > >>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> > >>>> +    Sets the offset from the audio source sample rate, unit is Hz.
> > >>>> +    The offset compensates for any clock drift. The actual source audio sample
> > >>>> +    rate is the ideal source audio sample rate from
> > >>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> > >>>
> > >>> I don't see why this would require a new type, you can use
> > >>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> > >>> fixed-point values in Q31.32 format.
> > >>
> > >> Why would you want to do this? I can store a double in a long long int,
> > >> and just document that the variable is really a double, but why would you?
> > >
> > > I'm happy we have no floating point control types ;-)
> > >
> > >> The cost of adding a FIXED_POINT type is minimal, and having this type
> > >> makes it easy to work with fixed point controls (think about proper reporting
> > >> and setting of the value in v4l2-ctl and user applications in general that
> > >> deal with controls).
> > >
> > > The next thing you know is that someone will want a FIXED_POINT_Q15_16
> > > type as 64-bit would be too large to store in a large array. And then
> > > Q7.8. And Q3.12. And a bunch of other type. I really don't see what
> > > added value they bring compared to using the 32-bit and 64-bit integer
> > > types we already have. Every new type that is added adds complexity to
> > > userspace that will need to deal with the type.
> > >
> > >> If this would add a thousand lines of complex code, then this would be a
> > >> consideration, but this is just a few lines.
> > >>
> > >> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> > >> and it reports the value 13958643712, would you be able to see that that is
> > >> really 3.25 in fixed point format? With the right type it would be printed
> > >> like that. Much easier to work work.
> > >
> > > The same is true for analog gains, where x1.23 or +12dB is nicer to read
> > > than raw values. If we care about printing values in command line tools
> > > (which is nice to have, but certainly not the majority of use cases),
> > > then I would recommand working on units support for V4L2 controls, to
> > > convey how values are encoded, and in what unit they are expressed.
> >
> > So you prefer to have a way to specify the N value in QM.N as part
> > of the control information?
> >
> > E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
> > and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
> > in QM.N.
> >
> > I can go along with that. This would be valid for INTEGER, INTEGER64,
> > U8, U16 and U32 controls (the last three are only used in control arrays).
>
> I think that would be nicer. Not only is it more flexible, but it also
> allows applications to ignore that information, and still operate on
> integer controls without any modification.
>
> > A better name for 'fraction_bits' is welcome, I took it from the wikipedia
> > article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic
> >

I like the idea and the name sounds fine to me too.

> > Reporting unit names is certainly possible, but should perhaps be done
> > with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
> > needed for applications, unless they need to report values. In theory
> > it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
> > 4 of the reserved fields for a 'char unit[16];' field. But I feel a
> > bit uncomfortable taking reserved fields for something that is rarely
> > needed.
>
> I would make the unit an enumerated integer value. If it's a string, it
> gets more difficult to operate on. Having to standardize a unit means
> that the unit will get reviewed.
>

What usage do we envision for units? Could one give some examples? My
impression is that we already defined most of the controls with
explicit units.

> > >>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> > >>>>>> the control value, be it a Q number or something else, and do that
> > >>>>>> independently of the type of the control.
> > >>>>
> > >>>> Huh? How is that different from the type of the control? You have integers
> > >>>> (one type) and fixed point (another type).
> > >>>>
> > >>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> > >>>> explicitly?
> > >>>>
> > >>>> I think the main reason why we use integer controls for gain is that we
> > >>>> never had a fixed point control type and you could get away with that in
> > >>>> user space for that particular use-case.
> > >>>>
> > >>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> > >>>> be calculated as (value / default_value),
> > >>>
> > >>> Typically, but not always. Some sensor have an exponential gain model,
> > >>> and some have weird gain representation, such as 1/x. That's getting out
> > >>> of scope though.
> > >>>
> > >>>> but that won't work for a rate offset
> > >>>> control as above, or for e.g. CSC matrices for color converters.
> > >>>>
> > >>>>> Agreed.
> > >>>>>
> > >>>>>>> In the case of this particular series the control type is really a fixed point
> > >>>>>>> value with a documented unit (Hz). It really is not something you want to
> > >>>>>>> use type INTEGER64 for.
> > >>>>>>>
> > >>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> > >>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
> > >>>>>>>>> N <= 31 and M <= 32.
> > >>>>>>>>>
> > >>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> > >>>>>>>>> and it had to fit in 16 bits.
>
> --
> Regards,
>
> Laurent Pinchart

Hans Verkuil Nov. 16, 2023, 8:01 a.m. UTC | #18

Shengjiu,

FYI: I started work on adding the fraction_bits field. I hope to have a
patch for that early next week.

Regards,

	Hans

On 16/11/2023 08:31, Tomasz Figa wrote:
> On Wed, Nov 15, 2023 at 8:49 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Hans,
>>
>> On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote:
>>> On 11/15/23 11:55, Laurent Pinchart wrote:
>>>> On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
>>>>> On 13/11/2023 13:44, Laurent Pinchart wrote:
>>>>>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
>>>>>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
>>>>>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
>>>>>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
>>>>>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
>>>>>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>>>>>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>>>>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>>>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>>>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
>>>>>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>>>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>>>>>>>>>>>> since that has already a bunch of test controls for other control types.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
>>>>>>>>>>>>>
>>>>>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>>>>>>>>>>>> relate more to units than control types. We have lots of fixed-point
>>>>>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
>>>>>>>>>>>>> types. They use various locations for the decimal point, depending on
>>>>>>>>>>>>> the control. If we want to make this more explicit to users, we should
>>>>>>>>>>>>> work on adding unit support to the V4L2 controls.
>>>>>>>>>>>>
>>>>>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
>>>>>>>>>>>
>>>>>>>>>>> It's not a unit, but I think it's related to units. My point is that,
>>>>>>>>>>> without units support, I don't see why we need a formal definition of
>>>>>>>>>>> fixed-point types, and why this series couldn't just use
>>>>>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
>>>>>>>>>>> values as they see fit.
>>>>>>>>>>
>>>>>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
>>>>>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
>>>>>>>>
>>>>>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
>>>>>>>>
>>>>>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
>>>>>>>>
>>>>>>>> The most common case for control handling in drivers is taking the
>>>>>>>> integer value and converting it to a register value, using
>>>>>>>> device-specific encoding of the register value. It can be a fixed-point
>>>>>>>> format or something else, depending on the device. My point is that
>>>>>>>> drivers routinely convert a "plain" integer to something else, and that
>>>>>>>> has never been considered as a cause of concern. I don't see why it
>>>>>>>> would be different in this series.
>>>>>>>>
>>>>>>>>>> And while we do not have support for units (other than the documentation),
>>>>>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
>>>>>>>>>>
>>>>>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>>>>>>>>>>>> only shows a single driver specific control (dw100.rst).
>>>>>>>>>>>>
>>>>>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
>>>>>>>>>>>
>>>>>>>>>>> The analog gain control for sensors for instance.
>>>>>>>>>>
>>>>>>>>>> Not really. The documentation is super vague:
>>>>>>>>>>
>>>>>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
>>>>>>>>>>
>>>>>>>>>>       Analogue gain is gain affecting all colour components in the pixel matrix. The
>>>>>>>>>>       gain operation is performed in the analogue domain before A/D conversion.
>>>>>>>>>>
>>>>>>>>>> And the integer is just a range. Internally it might map to some fixed
>>>>>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
>>>>>>>>
>>>>>>>> It's hidden so well that libcamera has a database of the sensor it
>>>>>>>> supports, with formulas to map a real gain value to the
>>>>>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
>>>>>>>> matter, and the kernel doesn't expose it. We may or may not consider
>>>>>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
>>>>>>>> situation we have today.
>>>>>>>>
>>>>>>>>> I wonder if Laurent meant digital gain.
>>>>>>>>
>>>>>>>> No, I meant analog. It applies to digital gain too though.
>>>>>>>>
>>>>>>>>> Those are often Q numbers. The practice there has been that the default
>>>>>>>>> value yields gain of 1.
>>>>>>>>>
>>>>>>>>> There are probably many other examples in controls where something being
>>>>>>>>> controlled isn't actually an integer while integer controls are still being
>>>>>>>>> used for the purpose.
>>>>>>>>
>>>>>>>> A good summary of my opinion :-)
>>>>>>>
>>>>>>> And that works fine as long as userspace doesn't need to know what the value
>>>>>>> actually means.
>>>>>>>
>>>>>>> That's not the case here. The control is really a fractional Hz value:
>>>>>>>
>>>>>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
>>>>>>> +    Sets the offset from the audio source sample rate, unit is Hz.
>>>>>>> +    The offset compensates for any clock drift. The actual source audio sample
>>>>>>> +    rate is the ideal source audio sample rate from
>>>>>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
>>>>>>
>>>>>> I don't see why this would require a new type, you can use
>>>>>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
>>>>>> fixed-point values in Q31.32 format.
>>>>>
>>>>> Why would you want to do this? I can store a double in a long long int,
>>>>> and just document that the variable is really a double, but why would you?
>>>>
>>>> I'm happy we have no floating point control types ;-)
>>>>
>>>>> The cost of adding a FIXED_POINT type is minimal, and having this type
>>>>> makes it easy to work with fixed point controls (think about proper reporting
>>>>> and setting of the value in v4l2-ctl and user applications in general that
>>>>> deal with controls).
>>>>
>>>> The next thing you know is that someone will want a FIXED_POINT_Q15_16
>>>> type as 64-bit would be too large to store in a large array. And then
>>>> Q7.8. And Q3.12. And a bunch of other type. I really don't see what
>>>> added value they bring compared to using the 32-bit and 64-bit integer
>>>> types we already have. Every new type that is added adds complexity to
>>>> userspace that will need to deal with the type.
>>>>
>>>>> If this would add a thousand lines of complex code, then this would be a
>>>>> consideration, but this is just a few lines.
>>>>>
>>>>> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
>>>>> and it reports the value 13958643712, would you be able to see that that is
>>>>> really 3.25 in fixed point format? With the right type it would be printed
>>>>> like that. Much easier to work work.
>>>>
>>>> The same is true for analog gains, where x1.23 or +12dB is nicer to read
>>>> than raw values. If we care about printing values in command line tools
>>>> (which is nice to have, but certainly not the majority of use cases),
>>>> then I would recommand working on units support for V4L2 controls, to
>>>> convey how values are encoded, and in what unit they are expressed.
>>>
>>> So you prefer to have a way to specify the N value in QM.N as part
>>> of the control information?
>>>
>>> E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
>>> and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
>>> in QM.N.
>>>
>>> I can go along with that. This would be valid for INTEGER, INTEGER64,
>>> U8, U16 and U32 controls (the last three are only used in control arrays).
>>
>> I think that would be nicer. Not only is it more flexible, but it also
>> allows applications to ignore that information, and still operate on
>> integer controls without any modification.
>>
>>> A better name for 'fraction_bits' is welcome, I took it from the wikipedia
>>> article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic
>>>
> 
> I like the idea and the name sounds fine to me too.
> 
>>> Reporting unit names is certainly possible, but should perhaps be done
>>> with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
>>> needed for applications, unless they need to report values. In theory
>>> it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
>>> 4 of the reserved fields for a 'char unit[16];' field. But I feel a
>>> bit uncomfortable taking reserved fields for something that is rarely
>>> needed.
>>
>> I would make the unit an enumerated integer value. If it's a string, it
>> gets more difficult to operate on. Having to standardize a unit means
>> that the unit will get reviewed.
>>
> 
> What usage do we envision for units? Could one give some examples? My
> impression is that we already defined most of the controls with
> explicit units.
> 
>>>>>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
>>>>>>>>> the control value, be it a Q number or something else, and do that
>>>>>>>>> independently of the type of the control.
>>>>>>>
>>>>>>> Huh? How is that different from the type of the control? You have integers
>>>>>>> (one type) and fixed point (another type).
>>>>>>>
>>>>>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
>>>>>>> explicitly?
>>>>>>>
>>>>>>> I think the main reason why we use integer controls for gain is that we
>>>>>>> never had a fixed point control type and you could get away with that in
>>>>>>> user space for that particular use-case.
>>>>>>>
>>>>>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
>>>>>>> be calculated as (value / default_value),
>>>>>>
>>>>>> Typically, but not always. Some sensor have an exponential gain model,
>>>>>> and some have weird gain representation, such as 1/x. That's getting out
>>>>>> of scope though.
>>>>>>
>>>>>>> but that won't work for a rate offset
>>>>>>> control as above, or for e.g. CSC matrices for color converters.
>>>>>>>
>>>>>>>> Agreed.
>>>>>>>>
>>>>>>>>>> In the case of this particular series the control type is really a fixed point
>>>>>>>>>> value with a documented unit (Hz). It really is not something you want to
>>>>>>>>>> use type INTEGER64 for.
>>>>>>>>>>
>>>>>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>>>>>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
>>>>>>>>>>>> N <= 31 and M <= 32.
>>>>>>>>>>>>
>>>>>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
>>>>>>>>>>>> and it had to fit in 16 bits.
>>
>> --
>> Regards,
>>
>> Laurent Pinchart

Shengjiu Wang Nov. 16, 2023, 9:15 a.m. UTC | #19

On Thu, Nov 16, 2023 at 4:01 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Shengjiu,
>
> FYI: I started work on adding the fraction_bits field. I hope to have a
> patch for that early next week.
>
Thanks.  I will wait for your patch to be ready.

best regards
wang shengjiu

> Regards,
>
>         Hans
>
> On 16/11/2023 08:31, Tomasz Figa wrote:
> > On Wed, Nov 15, 2023 at 8:49 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>
> >> Hi Hans,
> >>
> >> On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote:
> >>> On 11/15/23 11:55, Laurent Pinchart wrote:
> >>>> On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
> >>>>> On 13/11/2023 13:44, Laurent Pinchart wrote:
> >>>>>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> >>>>>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
> >>>>>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> >>>>>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> >>>>>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> >>>>>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> >>>>>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> >>>>>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> >>>>>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> >>>>>>>>>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> >>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
> >>>>>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> >>>>>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> >>>>>>>>>>>>>> since that has already a bunch of test controls for other control types.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> >>>>>>>>>>>>> relate more to units than control types. We have lots of fixed-point
> >>>>>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> >>>>>>>>>>>>> types. They use various locations for the decimal point, depending on
> >>>>>>>>>>>>> the control. If we want to make this more explicit to users, we should
> >>>>>>>>>>>>> work on adding unit support to the V4L2 controls.
> >>>>>>>>>>>>
> >>>>>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> >>>>>>>>>>>
> >>>>>>>>>>> It's not a unit, but I think it's related to units. My point is that,
> >>>>>>>>>>> without units support, I don't see why we need a formal definition of
> >>>>>>>>>>> fixed-point types, and why this series couldn't just use
> >>>>>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> >>>>>>>>>>> values as they see fit.
> >>>>>>>>>>
> >>>>>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> >>>>>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> >>>>>>>>
> >>>>>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> >>>>>>>>
> >>>>>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> >>>>>>>>
> >>>>>>>> The most common case for control handling in drivers is taking the
> >>>>>>>> integer value and converting it to a register value, using
> >>>>>>>> device-specific encoding of the register value. It can be a fixed-point
> >>>>>>>> format or something else, depending on the device. My point is that
> >>>>>>>> drivers routinely convert a "plain" integer to something else, and that
> >>>>>>>> has never been considered as a cause of concern. I don't see why it
> >>>>>>>> would be different in this series.
> >>>>>>>>
> >>>>>>>>>> And while we do not have support for units (other than the documentation),
> >>>>>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> >>>>>>>>>>
> >>>>>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> >>>>>>>>>>>> only shows a single driver specific control (dw100.rst).
> >>>>>>>>>>>>
> >>>>>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
> >>>>>>>>>>>
> >>>>>>>>>>> The analog gain control for sensors for instance.
> >>>>>>>>>>
> >>>>>>>>>> Not really. The documentation is super vague:
> >>>>>>>>>>
> >>>>>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> >>>>>>>>>>
> >>>>>>>>>>       Analogue gain is gain affecting all colour components in the pixel matrix. The
> >>>>>>>>>>       gain operation is performed in the analogue domain before A/D conversion.
> >>>>>>>>>>
> >>>>>>>>>> And the integer is just a range. Internally it might map to some fixed
> >>>>>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> >>>>>>>>
> >>>>>>>> It's hidden so well that libcamera has a database of the sensor it
> >>>>>>>> supports, with formulas to map a real gain value to the
> >>>>>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> >>>>>>>> matter, and the kernel doesn't expose it. We may or may not consider
> >>>>>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
> >>>>>>>> situation we have today.
> >>>>>>>>
> >>>>>>>>> I wonder if Laurent meant digital gain.
> >>>>>>>>
> >>>>>>>> No, I meant analog. It applies to digital gain too though.
> >>>>>>>>
> >>>>>>>>> Those are often Q numbers. The practice there has been that the default
> >>>>>>>>> value yields gain of 1.
> >>>>>>>>>
> >>>>>>>>> There are probably many other examples in controls where something being
> >>>>>>>>> controlled isn't actually an integer while integer controls are still being
> >>>>>>>>> used for the purpose.
> >>>>>>>>
> >>>>>>>> A good summary of my opinion :-)
> >>>>>>>
> >>>>>>> And that works fine as long as userspace doesn't need to know what the value
> >>>>>>> actually means.
> >>>>>>>
> >>>>>>> That's not the case here. The control is really a fractional Hz value:
> >>>>>>>
> >>>>>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> >>>>>>> +    Sets the offset from the audio source sample rate, unit is Hz.
> >>>>>>> +    The offset compensates for any clock drift. The actual source audio sample
> >>>>>>> +    rate is the ideal source audio sample rate from
> >>>>>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> >>>>>>
> >>>>>> I don't see why this would require a new type, you can use
> >>>>>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> >>>>>> fixed-point values in Q31.32 format.
> >>>>>
> >>>>> Why would you want to do this? I can store a double in a long long int,
> >>>>> and just document that the variable is really a double, but why would you?
> >>>>
> >>>> I'm happy we have no floating point control types ;-)
> >>>>
> >>>>> The cost of adding a FIXED_POINT type is minimal, and having this type
> >>>>> makes it easy to work with fixed point controls (think about proper reporting
> >>>>> and setting of the value in v4l2-ctl and user applications in general that
> >>>>> deal with controls).
> >>>>
> >>>> The next thing you know is that someone will want a FIXED_POINT_Q15_16
> >>>> type as 64-bit would be too large to store in a large array. And then
> >>>> Q7.8. And Q3.12. And a bunch of other type. I really don't see what
> >>>> added value they bring compared to using the 32-bit and 64-bit integer
> >>>> types we already have. Every new type that is added adds complexity to
> >>>> userspace that will need to deal with the type.
> >>>>
> >>>>> If this would add a thousand lines of complex code, then this would be a
> >>>>> consideration, but this is just a few lines.
> >>>>>
> >>>>> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> >>>>> and it reports the value 13958643712, would you be able to see that that is
> >>>>> really 3.25 in fixed point format? With the right type it would be printed
> >>>>> like that. Much easier to work work.
> >>>>
> >>>> The same is true for analog gains, where x1.23 or +12dB is nicer to read
> >>>> than raw values. If we care about printing values in command line tools
> >>>> (which is nice to have, but certainly not the majority of use cases),
> >>>> then I would recommand working on units support for V4L2 controls, to
> >>>> convey how values are encoded, and in what unit they are expressed.
> >>>
> >>> So you prefer to have a way to specify the N value in QM.N as part
> >>> of the control information?
> >>>
> >>> E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
> >>> and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
> >>> in QM.N.
> >>>
> >>> I can go along with that. This would be valid for INTEGER, INTEGER64,
> >>> U8, U16 and U32 controls (the last three are only used in control arrays).
> >>
> >> I think that would be nicer. Not only is it more flexible, but it also
> >> allows applications to ignore that information, and still operate on
> >> integer controls without any modification.
> >>
> >>> A better name for 'fraction_bits' is welcome, I took it from the wikipedia
> >>> article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic
> >>>
> >
> > I like the idea and the name sounds fine to me too.
> >
> >>> Reporting unit names is certainly possible, but should perhaps be done
> >>> with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
> >>> needed for applications, unless they need to report values. In theory
> >>> it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
> >>> 4 of the reserved fields for a 'char unit[16];' field. But I feel a
> >>> bit uncomfortable taking reserved fields for something that is rarely
> >>> needed.
> >>
> >> I would make the unit an enumerated integer value. If it's a string, it
> >> gets more difficult to operate on. Having to standardize a unit means
> >> that the unit will get reviewed.
> >>
> >
> > What usage do we envision for units? Could one give some examples? My
> > impression is that we already defined most of the controls with
> > explicit units.
> >
> >>>>>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> >>>>>>>>> the control value, be it a Q number or something else, and do that
> >>>>>>>>> independently of the type of the control.
> >>>>>>>
> >>>>>>> Huh? How is that different from the type of the control? You have integers
> >>>>>>> (one type) and fixed point (another type).
> >>>>>>>
> >>>>>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> >>>>>>> explicitly?
> >>>>>>>
> >>>>>>> I think the main reason why we use integer controls for gain is that we
> >>>>>>> never had a fixed point control type and you could get away with that in
> >>>>>>> user space for that particular use-case.
> >>>>>>>
> >>>>>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> >>>>>>> be calculated as (value / default_value),
> >>>>>>
> >>>>>> Typically, but not always. Some sensor have an exponential gain model,
> >>>>>> and some have weird gain representation, such as 1/x. That's getting out
> >>>>>> of scope though.
> >>>>>>
> >>>>>>> but that won't work for a rate offset
> >>>>>>> control as above, or for e.g. CSC matrices for color converters.
> >>>>>>>
> >>>>>>>> Agreed.
> >>>>>>>>
> >>>>>>>>>> In the case of this particular series the control type is really a fixed point
> >>>>>>>>>> value with a documented unit (Hz). It really is not something you want to
> >>>>>>>>>> use type INTEGER64 for.
> >>>>>>>>>>
> >>>>>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> >>>>>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
> >>>>>>>>>>>> N <= 31 and M <= 32.
> >>>>>>>>>>>>
> >>>>>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> >>>>>>>>>>>> and it had to fit in 16 bits.
> >>
> >> --
> >> Regards,
> >>
> >> Laurent Pinchart
>

Hans Verkuil Nov. 17, 2023, 12:07 p.m. UTC | #20

Here is an RFC patch adding support for 'fraction_bits'. It's lacking
documentation, but it can be used for testing.

It was rather a pain logging fixed point number in a reasonable format,
but I think it is OK.

In userspace (where you can use floating point) it is a lot easier:

printf("%.*g\n", fraction_bits, (double)v * (1.0 / (1ULL << fraction_bits)));

I decided to only expose fraction_bits in struct v4l2_query_ext_ctrl.
I could add it to struct v4l2_queryctrl, but I did not think that was
necessary. Other opinions are welcome.

In the meantime, let me know if this works for your patch series. If it
does, then I can clean this up.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-ctrls-api.c  |  1 +
 drivers/media/v4l2-core/v4l2-ctrls-core.c | 72 +++++++++++++++++++----
 include/media/v4l2-ctrls.h                |  7 ++-
 include/uapi/linux/videodev2.h            | 20 ++++++-
 4 files changed, 85 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
index 002ea6588edf..3132df315b17 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
@@ -1101,6 +1101,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 	qc->elems = ctrl->elems;
 	qc->nr_of_dims = ctrl->nr_of_dims;
 	memcpy(qc->dims, ctrl->dims, qc->nr_of_dims * sizeof(qc->dims[0]));
+	qc->fraction_bits = ctrl->fraction_bits;
 	qc->minimum = ctrl->minimum;
 	qc->maximum = ctrl->maximum;
 	qc->default_value = ctrl->default_value;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index a662fb60f73f..0e08a371af5c 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -252,12 +252,42 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
 }
 EXPORT_SYMBOL(v4l2_ctrl_type_op_init);

+static void v4l2_ctrl_log_fp(s64 v, unsigned int fraction_bits)
+{
+	s64 i = v4l2_fp_integer(v, fraction_bits);
+	s64 f = v4l2_fp_fraction(v, fraction_bits);
+
+	if (!f) {
+		pr_cont("%lld", i);
+	} else if (fraction_bits < 20) {
+		u64 div = 1ULL << fraction_bits;
+
+		if (!i && f < 0)
+			pr_cont("-%lld/%llu", -f, div);
+		else if (!i)
+			pr_cont("%lld/%llu", f, div);
+		else if (i < 0 || f < 0)
+			pr_cont("-%lld-%llu/%llu", -i, -f, div);
+		else
+			pr_cont("%lld+%llu/%llu", i, f, div);
+	} else {
+		if (!i && f < 0)
+			pr_cont("-%lld/(2^%u)", -f, fraction_bits);
+		else if (!i)
+			pr_cont("%lld/(2^%u)", f, fraction_bits);
+		else if (i < 0 || f < 0)
+			pr_cont("-%lld-%llu/(2^%u)", -i, -f, fraction_bits);
+		else
+			pr_cont("%lld+%llu/(2^%u)", i, f, fraction_bits);
+	}
+}
+
 void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
 {
 	union v4l2_ctrl_ptr ptr = ctrl->p_cur;

 	if (ctrl->is_array) {
-		unsigned i;
+		unsigned int i;

 		for (i = 0; i < ctrl->nr_of_dims; i++)
 			pr_cont("[%u]", ctrl->dims[i]);
@@ -266,7 +296,10 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)

 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER:
-		pr_cont("%d", *ptr.p_s32);
+		if (!ctrl->fraction_bits)
+			pr_cont("%d", *ptr.p_s32);
+		else
+			v4l2_ctrl_log_fp(*ptr.p_s32, ctrl->fraction_bits);
 		break;
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		pr_cont("%s", *ptr.p_s32 ? "true" : "false");
@@ -281,19 +314,31 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
 		pr_cont("0x%08x", *ptr.p_s32);
 		break;
 	case V4L2_CTRL_TYPE_INTEGER64:
-		pr_cont("%lld", *ptr.p_s64);
+		if (!ctrl->fraction_bits)
+			pr_cont("%lld", *ptr.p_s64);
+		else
+			v4l2_ctrl_log_fp(*ptr.p_s64, ctrl->fraction_bits);
 		break;
 	case V4L2_CTRL_TYPE_STRING:
 		pr_cont("%s", ptr.p_char);
 		break;
 	case V4L2_CTRL_TYPE_U8:
-		pr_cont("%u", (unsigned)*ptr.p_u8);
+		if (!ctrl->fraction_bits)
+			pr_cont("%u", (unsigned int)*ptr.p_u8);
+		else
+			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u8, ctrl->fraction_bits);
 		break;
 	case V4L2_CTRL_TYPE_U16:
-		pr_cont("%u", (unsigned)*ptr.p_u16);
+		if (!ctrl->fraction_bits)
+			pr_cont("%u", (unsigned int)*ptr.p_u16);
+		else
+			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u16, ctrl->fraction_bits);
 		break;
 	case V4L2_CTRL_TYPE_U32:
-		pr_cont("%u", (unsigned)*ptr.p_u32);
+		if (!ctrl->fraction_bits)
+			pr_cont("%u", (unsigned int)*ptr.p_u32);
+		else
+			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u32, ctrl->fraction_bits);
 		break;
 	case V4L2_CTRL_TYPE_H264_SPS:
 		pr_cont("H264_SPS");
@@ -1752,7 +1797,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			u32 id, const char *name, enum v4l2_ctrl_type type,
 			s64 min, s64 max, u64 step, s64 def,
 			const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
-			u32 flags, const char * const *qmenu,
+			u32 fraction_bits, u32 flags, const char * const *qmenu,
 			const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
 			void *priv)
 {
@@ -1939,6 +1984,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->name = name;
 	ctrl->type = type;
 	ctrl->flags = flags;
+	ctrl->fraction_bits = fraction_bits;
 	ctrl->minimum = min;
 	ctrl->maximum = max;
 	ctrl->step = step;
@@ -2037,7 +2083,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 	ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name,
 			type, min, max,
 			is_menu ? cfg->menu_skip_mask : step, def,
-			cfg->dims, cfg->elem_size,
+			cfg->dims, cfg->elem_size, cfg->fraction_bits,
 			flags, qmenu, qmenu_int, cfg->p_def, priv);
 	if (ctrl)
 		ctrl->is_private = cfg->is_private;
@@ -2062,7 +2108,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
-			     min, max, step, def, NULL, 0,
+			     min, max, step, def, NULL, 0, 0,
 			     flags, NULL, NULL, ptr_null, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2095,7 +2141,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
-			     0, max, mask, def, NULL, 0,
+			     0, max, mask, def, NULL, 0, 0,
 			     flags, qmenu, qmenu_int, ptr_null, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2127,7 +2173,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
-			     0, max, mask, def, NULL, 0,
+			     0, max, mask, def, NULL, 0, 0,
 			     flags, qmenu, NULL, ptr_null, NULL);

 }
@@ -2149,7 +2195,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
-			     min, max, step, def, NULL, 0,
+			     min, max, step, def, NULL, 0, 0,
 			     flags, NULL, NULL, p_def, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
@@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
-			     0, max, 0, def, NULL, 0,
+			     0, max, 0, def, NULL, 0, 0,
 			     flags, NULL, qmenu_int, ptr_null, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 59679a42b3e7..c35514c5bf88 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		except for dynamic arrays. In that case it is in the range of
  *		1 to @p_array_alloc_elems.
  * @dims:	The size of each dimension.
- * @nr_of_dims:The number of dimensions in @dims.
+ * @nr_of_dims: The number of dimensions in @dims.
+ * @fraction_bits: The number of fraction bits for fixed point values.
  * @menu_skip_mask: The control's skip mask for menu controls. This makes it
  *		easy to skip menu items that are not valid. If bit X is set,
  *		then menu item X is skipped. Of course, this only works for
@@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
  *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
  * @flags:	The control's flags.
+ * @fraction_bits: The number of fraction bits for fixed point values.
  * @priv:	The control's private pointer. For use by the driver. It is
  *		untouched by the control framework. Note that this pointer is
  *		not freed when the control is deleted. Should this be needed
@@ -286,6 +288,7 @@ struct v4l2_ctrl {
 	u32 new_elems;
 	u32 dims[V4L2_CTRL_MAX_DIMS];
 	u32 nr_of_dims;
+	u32 fraction_bits;
 	union {
 		u64 step;
 		u64 menu_skip_mask;
@@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
  * @dims:	The size of each dimension.
  * @elem_size:	The size in bytes of the control.
  * @flags:	The control's flags.
+ * @fraction_bits: The number of fraction bits for fixed point values.
  * @menu_skip_mask: The control's skip mask for menu controls. This makes it
  *		easy to skip menu items that are not valid. If bit X is set,
  *		then menu item X is skipped. Of course, this only works for
@@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
 	u32 dims[V4L2_CTRL_MAX_DIMS];
 	u32 elem_size;
 	u32 flags;
+	u32 fraction_bits;
 	u64 menu_skip_mask;
 	const char * const *qmenu;
 	const s64 *qmenu_int;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c3d4e490ce7c..26ecac19722a 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
 	__u32                elems;
 	__u32                nr_of_dims;
 	__u32                dims[V4L2_CTRL_MAX_DIMS];
-	__u32		     reserved[32];
+	__u32                fraction_bits;
+	__u32		     reserved[31];
 };

+static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int fraction_bits)
+{
+	return (i << fraction_bits) + f;
+}
+
+static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
+{
+	return v / (1LL << fraction_bits);
+}
+
+static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
+{
+	__u64 mask = (1ULL << fraction_bits) - 1;
+
+	return v < 0 ? -((-v) & mask) : (v & mask);
+}
+
 /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
 struct v4l2_querymenu {
 	__u32		id;

Laurent Pinchart Nov. 17, 2023, 12:37 p.m. UTC | #21

Hi Tomasz,

On Thu, Nov 16, 2023 at 04:31:41PM +0900, Tomasz Figa wrote:
> On Wed, Nov 15, 2023 at 8:49 PM Laurent Pinchart wrote:
> > On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote:
> > > On 11/15/23 11:55, Laurent Pinchart wrote:
> > > > On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote:
> > > >> On 13/11/2023 13:44, Laurent Pinchart wrote:
> > > >>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote:
> > > >>>> On 13/11/2023 12:43, Laurent Pinchart wrote:
> > > >>>>> On Mon, Nov 13, 2023 at 11:28:51AM +0000, Sakari Ailus wrote:
> > > >>>>>> On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote:
> > > >>>>>>> On 13/11/2023 12:07, Laurent Pinchart wrote:
> > > >>>>>>>> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
> > > >>>>>>>>> On 13/11/2023 11:42, Laurent Pinchart wrote:
> > > >>>>>>>>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
> > > >>>>>>>>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
> > > >>>>>>>>>>>> Fixed point controls are used by the user to configure
> > > >>>>>>>>>>>> a fixed point value in 64bits, which Q31.32 format.
> > > >>>>>>>>>>>>
> > > >>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> This patch adds a new control type. This is something that also needs to be
> > > >>>>>>>>>>> tested by v4l2-compliance, and for that we need to add support for this to
> > > >>>>>>>>>>> one of the media test-drivers. The best place for that is the vivid driver,
> > > >>>>>>>>>>> since that has already a bunch of test controls for other control types.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
> > > >>>>>>>>>>>
> > > >>>>>>>>>>> Can you add a patch adding a fixed point test control to vivid?
> > > >>>>>>>>>>
> > > >>>>>>>>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
> > > >>>>>>>>>> relate more to units than control types. We have lots of fixed-point
> > > >>>>>>>>>> values in controls already, using the 32-bit and 64-bit integer control
> > > >>>>>>>>>> types. They use various locations for the decimal point, depending on
> > > >>>>>>>>>> the control. If we want to make this more explicit to users, we should
> > > >>>>>>>>>> work on adding unit support to the V4L2 controls.
> > > >>>>>>>>>
> > > >>>>>>>>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> > > >>>>>>>>
> > > >>>>>>>> It's not a unit, but I think it's related to units. My point is that,
> > > >>>>>>>> without units support, I don't see why we need a formal definition of
> > > >>>>>>>> fixed-point types, and why this series couldn't just use
> > > >>>>>>>> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> > > >>>>>>>> values as they see fit.
> > > >>>>>>>
> > > >>>>>>> They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
> > > >>>>>>> (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
> > > >>>>>
> > > >>>>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-)
> > > >>>>>
> > > >>>>>>> is always interpreted as a 64 bit integer and nothing else. As it should.
> > > >>>>>
> > > >>>>> The most common case for control handling in drivers is taking the
> > > >>>>> integer value and converting it to a register value, using
> > > >>>>> device-specific encoding of the register value. It can be a fixed-point
> > > >>>>> format or something else, depending on the device. My point is that
> > > >>>>> drivers routinely convert a "plain" integer to something else, and that
> > > >>>>> has never been considered as a cause of concern. I don't see why it
> > > >>>>> would be different in this series.
> > > >>>>>
> > > >>>>>>> And while we do not have support for units (other than the documentation),
> > > >>>>>>> we do have type support in the form of V4L2_CTRL_TYPE_*.
> > > >>>>>>>
> > > >>>>>>>>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
> > > >>>>>>>>> only shows a single driver specific control (dw100.rst).
> > > >>>>>>>>>
> > > >>>>>>>>> I'm not aware of other controls in mainline that use fixed point.
> > > >>>>>>>>
> > > >>>>>>>> The analog gain control for sensors for instance.
> > > >>>>>>>
> > > >>>>>>> Not really. The documentation is super vague:
> > > >>>>>>>
> > > >>>>>>> V4L2_CID_ANALOGUE_GAIN (integer)
> > > >>>>>>>
> > > >>>>>>>       Analogue gain is gain affecting all colour components in the pixel matrix. The
> > > >>>>>>>       gain operation is performed in the analogue domain before A/D conversion.
> > > >>>>>>>
> > > >>>>>>> And the integer is just a range. Internally it might map to some fixed
> > > >>>>>>> point value, but userspace won't see that, it's hidden in the driver AFAICT.
> > > >>>>>
> > > >>>>> It's hidden so well that libcamera has a database of the sensor it
> > > >>>>> supports, with formulas to map a real gain value to the
> > > >>>>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does
> > > >>>>> matter, and the kernel doesn't expose it. We may or may not consider
> > > >>>>> that as a shortcoming of the V4L2 control API, but in any case it's the
> > > >>>>> situation we have today.
> > > >>>>>
> > > >>>>>> I wonder if Laurent meant digital gain.
> > > >>>>>
> > > >>>>> No, I meant analog. It applies to digital gain too though.
> > > >>>>>
> > > >>>>>> Those are often Q numbers. The practice there has been that the default
> > > >>>>>> value yields gain of 1.
> > > >>>>>>
> > > >>>>>> There are probably many other examples in controls where something being
> > > >>>>>> controlled isn't actually an integer while integer controls are still being
> > > >>>>>> used for the purpose.
> > > >>>>>
> > > >>>>> A good summary of my opinion :-)
> > > >>>>
> > > >>>> And that works fine as long as userspace doesn't need to know what the value
> > > >>>> actually means.
> > > >>>>
> > > >>>> That's not the case here. The control is really a fractional Hz value:
> > > >>>>
> > > >>>> +``V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET (fixed point)``
> > > >>>> +    Sets the offset from the audio source sample rate, unit is Hz.
> > > >>>> +    The offset compensates for any clock drift. The actual source audio sample
> > > >>>> +    rate is the ideal source audio sample rate from
> > > >>>> +    ``V4L2_CID_M2M_AUDIO_SOURCE_RATE`` plus this fixed point offset.
> > > >>>
> > > >>> I don't see why this would require a new type, you can use
> > > >>> V4L2_CTRL_TYPE_INTEGER64, and document the control as containing
> > > >>> fixed-point values in Q31.32 format.
> > > >>
> > > >> Why would you want to do this? I can store a double in a long long int,
> > > >> and just document that the variable is really a double, but why would you?
> > > >
> > > > I'm happy we have no floating point control types ;-)
> > > >
> > > >> The cost of adding a FIXED_POINT type is minimal, and having this type
> > > >> makes it easy to work with fixed point controls (think about proper reporting
> > > >> and setting of the value in v4l2-ctl and user applications in general that
> > > >> deal with controls).
> > > >
> > > > The next thing you know is that someone will want a FIXED_POINT_Q15_16
> > > > type as 64-bit would be too large to store in a large array. And then
> > > > Q7.8. And Q3.12. And a bunch of other type. I really don't see what
> > > > added value they bring compared to using the 32-bit and 64-bit integer
> > > > types we already have. Every new type that is added adds complexity to
> > > > userspace that will need to deal with the type.
> > > >
> > > >> If this would add a thousand lines of complex code, then this would be a
> > > >> consideration, but this is just a few lines.
> > > >>
> > > >> Just to give an example, if you use 'v4l2-ctl -l' to list a int64 control
> > > >> and it reports the value 13958643712, would you be able to see that that is
> > > >> really 3.25 in fixed point format? With the right type it would be printed
> > > >> like that. Much easier to work work.
> > > >
> > > > The same is true for analog gains, where x1.23 or +12dB is nicer to read
> > > > than raw values. If we care about printing values in command line tools
> > > > (which is nice to have, but certainly not the majority of use cases),
> > > > then I would recommand working on units support for V4L2 controls, to
> > > > convey how values are encoded, and in what unit they are expressed.
> > >
> > > So you prefer to have a way to specify the N value in QM.N as part
> > > of the control information?
> > >
> > > E.g. add a '__u8 fraction_bits' field to structs v4l2_query_ext_ctrl
> > > and v4l2_queryctrl. If 0, then it is an integer, otherwise it is the N
> > > in QM.N.
> > >
> > > I can go along with that. This would be valid for INTEGER, INTEGER64,
> > > U8, U16 and U32 controls (the last three are only used in control arrays).
> >
> > I think that would be nicer. Not only is it more flexible, but it also
> > allows applications to ignore that information, and still operate on
> > integer controls without any modification.
> >
> > > A better name for 'fraction_bits' is welcome, I took it from the wikipedia
> > > article: https://en.wikipedia.org/wiki/Fixed-point_arithmetic
> 
> I like the idea and the name sounds fine to me too.
> 
> > > Reporting unit names is certainly possible, but should perhaps be done
> > > with a separate ioctl? E.g. VIDIOC_QUERY_CTRL_UNIT. It is not typically
> > > needed for applications, unless they need to report values. In theory
> > > it can also be reported through VIDIOC_QUERY_EXT_CTRL by using, say,
> > > 4 of the reserved fields for a 'char unit[16];' field. But I feel a
> > > bit uncomfortable taking reserved fields for something that is rarely
> > > needed.
> >
> > I would make the unit an enumerated integer value. If it's a string, it
> > gets more difficult to operate on. Having to standardize a unit means
> > that the unit will get reviewed.
> 
> What usage do we envision for units? Could one give some examples? My
> impression is that we already defined most of the controls with
> explicit units.

Many controls are defined with explicit units in the API specification.
Drivers may or may not comply with that, and there's no way for
applications to query unit-related information dynamically. As a result,
we end up hardcoding in libcamera the unit and scale of the analogue
gain control for each sensor. I'm not saying this necessarily has to
change, but it would be one use case.

Another use case would be to pretty-print values, but I don't think we
should design the API with this use case as an important target. If I
were to redesign the V4L2 control API, I would drop the control name,
and certainly drop things like V4L2_CTRL_FLAG_SLIDER, and probably
V4L2_CTRL_FLAG_GRABBED too. If there are valid use cases for units, then
we can also use them for pretty-printing, but let's not add them
otherwise.

> > > >>>>>> Instead of this patch, I'd prefer to have a way to express the meaning of
> > > >>>>>> the control value, be it a Q number or something else, and do that
> > > >>>>>> independently of the type of the control.
> > > >>>>
> > > >>>> Huh? How is that different from the type of the control? You have integers
> > > >>>> (one type) and fixed point (another type).
> > > >>>>
> > > >>>> Or do you want a more general V4L2_CTRL_TYPE_ that specifies the N.M values
> > > >>>> explicitly?
> > > >>>>
> > > >>>> I think the main reason why we use integer controls for gain is that we
> > > >>>> never had a fixed point control type and you could get away with that in
> > > >>>> user space for that particular use-case.
> > > >>>>
> > > >>>> Based on the V4L2_CID_NOTIFY_GAINS documentation the gain value can typically
> > > >>>> be calculated as (value / default_value),
> > > >>>
> > > >>> Typically, but not always. Some sensor have an exponential gain model,
> > > >>> and some have weird gain representation, such as 1/x. That's getting out
> > > >>> of scope though.
> > > >>>
> > > >>>> but that won't work for a rate offset
> > > >>>> control as above, or for e.g. CSC matrices for color converters.
> > > >>>>
> > > >>>>> Agreed.
> > > >>>>>
> > > >>>>>>> In the case of this particular series the control type is really a fixed point
> > > >>>>>>> value with a documented unit (Hz). It really is not something you want to
> > > >>>>>>> use type INTEGER64 for.
> > > >>>>>>>
> > > >>>>>>>>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
> > > >>>>>>>>> min/max/step you can easily map that to just about any QN.M format where
> > > >>>>>>>>> N <= 31 and M <= 32.
> > > >>>>>>>>>
> > > >>>>>>>>> In the case of dw100 it is a bit different in that it is quite specialized
> > > >>>>>>>>> and it had to fit in 16 bits.

Laurent Pinchart Nov. 17, 2023, 12:55 p.m. UTC | #22

Hi Hans,

Thank you for the patch.

On Fri, Nov 17, 2023 at 01:07:44PM +0100, Hans Verkuil wrote:
> Here is an RFC patch adding support for 'fraction_bits'. It's lacking
> documentation, but it can be used for testing.
> 
> It was rather a pain logging fixed point number in a reasonable format,
> but I think it is OK.
> 
> In userspace (where you can use floating point) it is a lot easier:
> 
> printf("%.*g\n", fraction_bits, (double)v * (1.0 / (1ULL << fraction_bits)));
> 
> I decided to only expose fraction_bits in struct v4l2_query_ext_ctrl.
> I could add it to struct v4l2_queryctrl, but I did not think that was
> necessary. Other opinions are welcome.

Agreed.

> In the meantime, let me know if this works for your patch series. If it
> does, then I can clean this up.
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-api.c  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 72 +++++++++++++++++++----
>  include/media/v4l2-ctrls.h                |  7 ++-
>  include/uapi/linux/videodev2.h            | 20 ++++++-
>  4 files changed, 85 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..3132df315b17 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -1101,6 +1101,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>  	qc->elems = ctrl->elems;
>  	qc->nr_of_dims = ctrl->nr_of_dims;
>  	memcpy(qc->dims, ctrl->dims, qc->nr_of_dims * sizeof(qc->dims[0]));
> +	qc->fraction_bits = ctrl->fraction_bits;
>  	qc->minimum = ctrl->minimum;
>  	qc->maximum = ctrl->maximum;
>  	qc->default_value = ctrl->default_value;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..0e08a371af5c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -252,12 +252,42 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_type_op_init);
> 
> +static void v4l2_ctrl_log_fp(s64 v, unsigned int fraction_bits)
> +{
> +	s64 i = v4l2_fp_integer(v, fraction_bits);
> +	s64 f = v4l2_fp_fraction(v, fraction_bits);
> +
> +	if (!f) {
> +		pr_cont("%lld", i);
> +	} else if (fraction_bits < 20) {
> +		u64 div = 1ULL << fraction_bits;
> +
> +		if (!i && f < 0)
> +			pr_cont("-%lld/%llu", -f, div);
> +		else if (!i)
> +			pr_cont("%lld/%llu", f, div);
> +		else if (i < 0 || f < 0)
> +			pr_cont("-%lld-%llu/%llu", -i, -f, div);
> +		else
> +			pr_cont("%lld+%llu/%llu", i, f, div);
> +	} else {
> +		if (!i && f < 0)
> +			pr_cont("-%lld/(2^%u)", -f, fraction_bits);
> +		else if (!i)
> +			pr_cont("%lld/(2^%u)", f, fraction_bits);
> +		else if (i < 0 || f < 0)
> +			pr_cont("-%lld-%llu/(2^%u)", -i, -f, fraction_bits);
> +		else
> +			pr_cont("%lld+%llu/(2^%u)", i, f, fraction_bits);
> +	}
> +}
> +
>  void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  {
>  	union v4l2_ctrl_ptr ptr = ctrl->p_cur;
> 
>  	if (ctrl->is_array) {
> -		unsigned i;
> +		unsigned int i;
> 
>  		for (i = 0; i < ctrl->nr_of_dims; i++)
>  			pr_cont("[%u]", ctrl->dims[i]);
> @@ -266,7 +296,10 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
> 
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER:
> -		pr_cont("%d", *ptr.p_s32);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%d", *ptr.p_s32);
> +		else
> +			v4l2_ctrl_log_fp(*ptr.p_s32, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_BOOLEAN:
>  		pr_cont("%s", *ptr.p_s32 ? "true" : "false");
> @@ -281,19 +314,31 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  		pr_cont("0x%08x", *ptr.p_s32);
>  		break;
>  	case V4L2_CTRL_TYPE_INTEGER64:
> -		pr_cont("%lld", *ptr.p_s64);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%lld", *ptr.p_s64);
> +		else
> +			v4l2_ctrl_log_fp(*ptr.p_s64, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
>  		pr_cont("%s", ptr.p_char);
>  		break;
>  	case V4L2_CTRL_TYPE_U8:
> -		pr_cont("%u", (unsigned)*ptr.p_u8);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u8);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u8, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_U16:
> -		pr_cont("%u", (unsigned)*ptr.p_u16);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u16);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u16, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_U32:
> -		pr_cont("%u", (unsigned)*ptr.p_u32);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u32);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u32, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_H264_SPS:
>  		pr_cont("H264_SPS");
> @@ -1752,7 +1797,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  			u32 id, const char *name, enum v4l2_ctrl_type type,
>  			s64 min, s64 max, u64 step, s64 def,
>  			const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> -			u32 flags, const char * const *qmenu,
> +			u32 fraction_bits, u32 flags, const char * const *qmenu,
>  			const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>  			void *priv)
>  {
> @@ -1939,6 +1984,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	ctrl->name = name;
>  	ctrl->type = type;
>  	ctrl->flags = flags;
> +	ctrl->fraction_bits = fraction_bits;
>  	ctrl->minimum = min;
>  	ctrl->maximum = max;
>  	ctrl->step = step;
> @@ -2037,7 +2083,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>  	ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name,
>  			type, min, max,
>  			is_menu ? cfg->menu_skip_mask : step, def,
> -			cfg->dims, cfg->elem_size,
> +			cfg->dims, cfg->elem_size, cfg->fraction_bits,
>  			flags, qmenu, qmenu_int, cfg->p_def, priv);
>  	if (ctrl)
>  		ctrl->is_private = cfg->is_private;
> @@ -2062,7 +2108,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     min, max, step, def, NULL, 0,
> +			     min, max, step, def, NULL, 0, 0,
>  			     flags, NULL, NULL, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
> @@ -2095,7 +2141,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, mask, def, NULL, 0,
> +			     0, max, mask, def, NULL, 0, 0,
>  			     flags, qmenu, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
> @@ -2127,7 +2173,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, mask, def, NULL, 0,
> +			     0, max, mask, def, NULL, 0, 0,
>  			     flags, qmenu, NULL, ptr_null, NULL);
> 
>  }
> @@ -2149,7 +2195,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     min, max, step, def, NULL, 0,
> +			     min, max, step, def, NULL, 0, 0,
>  			     flags, NULL, NULL, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> @@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, 0, def, NULL, 0,
> +			     0, max, 0, def, NULL, 0, 0,
>  			     flags, NULL, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 59679a42b3e7..c35514c5bf88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *		except for dynamic arrays. In that case it is in the range of
>   *		1 to @p_array_alloc_elems.
>   * @dims:	The size of each dimension.
> - * @nr_of_dims:The number of dimensions in @dims.
> + * @nr_of_dims: The number of dimensions in @dims.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *		easy to skip menu items that are not valid. If bit X is set,
>   *		then menu item X is skipped. Of course, this only works for
> @@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
>   *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
>   * @flags:	The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @priv:	The control's private pointer. For use by the driver. It is
>   *		untouched by the control framework. Note that this pointer is
>   *		not freed when the control is deleted. Should this be needed
> @@ -286,6 +288,7 @@ struct v4l2_ctrl {
>  	u32 new_elems;
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 nr_of_dims;
> +	u32 fraction_bits;
>  	union {
>  		u64 step;
>  		u64 menu_skip_mask;
> @@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
>   * @dims:	The size of each dimension.
>   * @elem_size:	The size in bytes of the control.
>   * @flags:	The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *		easy to skip menu items that are not valid. If bit X is set,
>   *		then menu item X is skipped. Of course, this only works for
> @@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 elem_size;
>  	u32 flags;
> +	u32 fraction_bits;
>  	u64 menu_skip_mask;
>  	const char * const *qmenu;
>  	const s64 *qmenu_int;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..26ecac19722a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
>  	__u32                elems;
>  	__u32                nr_of_dims;
>  	__u32                dims[V4L2_CTRL_MAX_DIMS];
> -	__u32		     reserved[32];
> +	__u32                fraction_bits;

Thinking forward, what if the control uses a different type of
quantization, for instance if the control is exponential instead of
linear ? Is this something we want to plan for already (without
necessarily implementing it yet) ? For instance, the CCS spec defines a
linear gain model where the gain is expressed as

	gain = (m0 * x + c0) / (m1 * x + c1)

where x is the control value, gain is the real gain, and m0, c0, m1 and
c1 are device-specific 16-bit integer constants (with the constraint
that one of m0 and m1 has to be zero, but not both).

There's also an exponential model in CCS, with

	gain = linear_gain * 2 ^ exponential_gain

where linear_gain and exponential_gain are U8.8 values.

> +	__u32		     reserved[31];
>  };
> 
> +static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int fraction_bits)
> +{
> +	return (i << fraction_bits) + f;
> +}
> +
> +static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
> +{
> +	return v / (1LL << fraction_bits);
> +}
> +
> +static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
> +{
> +	__u64 mask = (1ULL << fraction_bits) - 1;
> +
> +	return v < 0 ? -((-v) & mask) : (v & mask);
> +}

I woudln't add static inline functions to the UAPI. They cause licensing
issues, and increase the UAPI surface, thus making UAPI evolutions more
difficult.

> +
>  /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
>  struct v4l2_querymenu {
>  	__u32		id;

Sakari Ailus Nov. 17, 2023, 1:07 p.m. UTC | #23

Hi Hans,

Thank you for the patch.

On Fri, Nov 17, 2023 at 01:07:44PM +0100, Hans Verkuil wrote:
> Here is an RFC patch adding support for 'fraction_bits'. It's lacking
> documentation, but it can be used for testing.
> 
> It was rather a pain logging fixed point number in a reasonable format,
> but I think it is OK.
> 
> In userspace (where you can use floating point) it is a lot easier:
> 
> printf("%.*g\n", fraction_bits, (double)v * (1.0 / (1ULL << fraction_bits)));

I wonder if we could add a printk() format specifier for this. Doesn't need
to be done right now though, just an idea.

> 
> I decided to only expose fraction_bits in struct v4l2_query_ext_ctrl.
> I could add it to struct v4l2_queryctrl, but I did not think that was
> necessary. Other opinions are welcome.
> 
> In the meantime, let me know if this works for your patch series. If it
> does, then I can clean this up.
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-api.c  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 72 +++++++++++++++++++----
>  include/media/v4l2-ctrls.h                |  7 ++-
>  include/uapi/linux/videodev2.h            | 20 ++++++-
>  4 files changed, 85 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..3132df315b17 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -1101,6 +1101,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>  	qc->elems = ctrl->elems;
>  	qc->nr_of_dims = ctrl->nr_of_dims;
>  	memcpy(qc->dims, ctrl->dims, qc->nr_of_dims * sizeof(qc->dims[0]));
> +	qc->fraction_bits = ctrl->fraction_bits;
>  	qc->minimum = ctrl->minimum;
>  	qc->maximum = ctrl->maximum;
>  	qc->default_value = ctrl->default_value;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..0e08a371af5c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -252,12 +252,42 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_type_op_init);
> 
> +static void v4l2_ctrl_log_fp(s64 v, unsigned int fraction_bits)
> +{
> +	s64 i = v4l2_fp_integer(v, fraction_bits);
> +	s64 f = v4l2_fp_fraction(v, fraction_bits);
> +
> +	if (!f) {
> +		pr_cont("%lld", i);
> +	} else if (fraction_bits < 20) {
> +		u64 div = 1ULL << fraction_bits;
> +
> +		if (!i && f < 0)
> +			pr_cont("-%lld/%llu", -f, div);
> +		else if (!i)
> +			pr_cont("%lld/%llu", f, div);
> +		else if (i < 0 || f < 0)
> +			pr_cont("-%lld-%llu/%llu", -i, -f, div);
> +		else
> +			pr_cont("%lld+%llu/%llu", i, f, div);
> +	} else {
> +		if (!i && f < 0)
> +			pr_cont("-%lld/(2^%u)", -f, fraction_bits);
> +		else if (!i)
> +			pr_cont("%lld/(2^%u)", f, fraction_bits);
> +		else if (i < 0 || f < 0)
> +			pr_cont("-%lld-%llu/(2^%u)", -i, -f, fraction_bits);
> +		else
> +			pr_cont("%lld+%llu/(2^%u)", i, f, fraction_bits);
> +	}
> +}
> +
>  void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  {
>  	union v4l2_ctrl_ptr ptr = ctrl->p_cur;
> 
>  	if (ctrl->is_array) {
> -		unsigned i;
> +		unsigned int i;
> 
>  		for (i = 0; i < ctrl->nr_of_dims; i++)
>  			pr_cont("[%u]", ctrl->dims[i]);
> @@ -266,7 +296,10 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
> 
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER:
> -		pr_cont("%d", *ptr.p_s32);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%d", *ptr.p_s32);
> +		else
> +			v4l2_ctrl_log_fp(*ptr.p_s32, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_BOOLEAN:
>  		pr_cont("%s", *ptr.p_s32 ? "true" : "false");
> @@ -281,19 +314,31 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  		pr_cont("0x%08x", *ptr.p_s32);
>  		break;
>  	case V4L2_CTRL_TYPE_INTEGER64:
> -		pr_cont("%lld", *ptr.p_s64);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%lld", *ptr.p_s64);
> +		else
> +			v4l2_ctrl_log_fp(*ptr.p_s64, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
>  		pr_cont("%s", ptr.p_char);
>  		break;
>  	case V4L2_CTRL_TYPE_U8:
> -		pr_cont("%u", (unsigned)*ptr.p_u8);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u8);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u8, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_U16:
> -		pr_cont("%u", (unsigned)*ptr.p_u16);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u16);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u16, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_U32:
> -		pr_cont("%u", (unsigned)*ptr.p_u32);
> +		if (!ctrl->fraction_bits)
> +			pr_cont("%u", (unsigned int)*ptr.p_u32);
> +		else
> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u32, ctrl->fraction_bits);
>  		break;
>  	case V4L2_CTRL_TYPE_H264_SPS:
>  		pr_cont("H264_SPS");
> @@ -1752,7 +1797,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  			u32 id, const char *name, enum v4l2_ctrl_type type,
>  			s64 min, s64 max, u64 step, s64 def,
>  			const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> -			u32 flags, const char * const *qmenu,
> +			u32 fraction_bits, u32 flags, const char * const *qmenu,
>  			const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>  			void *priv)
>  {
> @@ -1939,6 +1984,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	ctrl->name = name;
>  	ctrl->type = type;
>  	ctrl->flags = flags;
> +	ctrl->fraction_bits = fraction_bits;
>  	ctrl->minimum = min;
>  	ctrl->maximum = max;
>  	ctrl->step = step;
> @@ -2037,7 +2083,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>  	ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name,
>  			type, min, max,
>  			is_menu ? cfg->menu_skip_mask : step, def,
> -			cfg->dims, cfg->elem_size,
> +			cfg->dims, cfg->elem_size, cfg->fraction_bits,
>  			flags, qmenu, qmenu_int, cfg->p_def, priv);
>  	if (ctrl)
>  		ctrl->is_private = cfg->is_private;
> @@ -2062,7 +2108,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     min, max, step, def, NULL, 0,
> +			     min, max, step, def, NULL, 0, 0,
>  			     flags, NULL, NULL, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
> @@ -2095,7 +2141,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, mask, def, NULL, 0,
> +			     0, max, mask, def, NULL, 0, 0,
>  			     flags, qmenu, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
> @@ -2127,7 +2173,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, mask, def, NULL, 0,
> +			     0, max, mask, def, NULL, 0, 0,
>  			     flags, qmenu, NULL, ptr_null, NULL);
> 
>  }
> @@ -2149,7 +2195,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     min, max, step, def, NULL, 0,
> +			     min, max, step, def, NULL, 0, 0,
>  			     flags, NULL, NULL, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> @@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>  		return NULL;
>  	}
>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -			     0, max, 0, def, NULL, 0,
> +			     0, max, 0, def, NULL, 0, 0,
>  			     flags, NULL, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 59679a42b3e7..c35514c5bf88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *		except for dynamic arrays. In that case it is in the range of
>   *		1 to @p_array_alloc_elems.
>   * @dims:	The size of each dimension.
> - * @nr_of_dims:The number of dimensions in @dims.
> + * @nr_of_dims: The number of dimensions in @dims.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *		easy to skip menu items that are not valid. If bit X is set,
>   *		then menu item X is skipped. Of course, this only works for
> @@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
>   *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
>   * @flags:	The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @priv:	The control's private pointer. For use by the driver. It is
>   *		untouched by the control framework. Note that this pointer is
>   *		not freed when the control is deleted. Should this be needed
> @@ -286,6 +288,7 @@ struct v4l2_ctrl {
>  	u32 new_elems;
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 nr_of_dims;
> +	u32 fraction_bits;
>  	union {
>  		u64 step;
>  		u64 menu_skip_mask;
> @@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
>   * @dims:	The size of each dimension.
>   * @elem_size:	The size in bytes of the control.
>   * @flags:	The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *		easy to skip menu items that are not valid. If bit X is set,
>   *		then menu item X is skipped. Of course, this only works for
> @@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 elem_size;
>  	u32 flags;
> +	u32 fraction_bits;
>  	u64 menu_skip_mask;
>  	const char * const *qmenu;
>  	const s64 *qmenu_int;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..26ecac19722a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
>  	__u32                elems;
>  	__u32                nr_of_dims;
>  	__u32                dims[V4L2_CTRL_MAX_DIMS];
> -	__u32		     reserved[32];
> +	__u32                fraction_bits;

u8 would suffice. Not that we'd be short of space but still...

> +	__u32		     reserved[31];
>  };
> 
> +static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int fraction_bits)
> +{
> +	return (i << fraction_bits) + f;
> +}
> +
> +static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
> +{
> +	return v / (1LL << fraction_bits);

Why not just:

	return v >> fraction_bits;

I'd use macros so you could use whatever control types with this without
casting. E.g.

#define V4L2_FP_INTEGER(v, fraction_bits) ((v) >> fraction_bits)

A more generic way to expose this could be to have base and exponent, the
base being 2 in this case. Just an idea. This would of course be a little
bit more difficult to use.

> +}
> +
> +static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
> +{
> +	__u64 mask = (1ULL << fraction_bits) - 1;
> +
> +	return v < 0 ? -((-v) & mask) : (v & mask);
> +}
> +
>  /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
>  struct v4l2_querymenu {
>  	__u32		id;

Hans Verkuil Nov. 17, 2023, 2:05 p.m. UTC | #24

On 17/11/2023 14:07, Sakari Ailus wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Fri, Nov 17, 2023 at 01:07:44PM +0100, Hans Verkuil wrote:
>> Here is an RFC patch adding support for 'fraction_bits'. It's lacking
>> documentation, but it can be used for testing.
>>
>> It was rather a pain logging fixed point number in a reasonable format,
>> but I think it is OK.
>>
>> In userspace (where you can use floating point) it is a lot easier:
>>
>> printf("%.*g\n", fraction_bits, (double)v * (1.0 / (1ULL << fraction_bits)));
> 
> I wonder if we could add a printk() format specifier for this. Doesn't need
> to be done right now though, just an idea.
> 
>>
>> I decided to only expose fraction_bits in struct v4l2_query_ext_ctrl.
>> I could add it to struct v4l2_queryctrl, but I did not think that was
>> necessary. Other opinions are welcome.
>>
>> In the meantime, let me know if this works for your patch series. If it
>> does, then I can clean this up.
>>
>> Regards,
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls-api.c  |  1 +
>>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 72 +++++++++++++++++++----
>>  include/media/v4l2-ctrls.h                |  7 ++-
>>  include/uapi/linux/videodev2.h            | 20 ++++++-
>>  4 files changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> index 002ea6588edf..3132df315b17 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
>> @@ -1101,6 +1101,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>>  	qc->elems = ctrl->elems;
>>  	qc->nr_of_dims = ctrl->nr_of_dims;
>>  	memcpy(qc->dims, ctrl->dims, qc->nr_of_dims * sizeof(qc->dims[0]));
>> +	qc->fraction_bits = ctrl->fraction_bits;
>>  	qc->minimum = ctrl->minimum;
>>  	qc->maximum = ctrl->maximum;
>>  	qc->default_value = ctrl->default_value;
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> index a662fb60f73f..0e08a371af5c 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>> @@ -252,12 +252,42 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_type_op_init);
>>
>> +static void v4l2_ctrl_log_fp(s64 v, unsigned int fraction_bits)
>> +{
>> +	s64 i = v4l2_fp_integer(v, fraction_bits);
>> +	s64 f = v4l2_fp_fraction(v, fraction_bits);
>> +
>> +	if (!f) {
>> +		pr_cont("%lld", i);
>> +	} else if (fraction_bits < 20) {
>> +		u64 div = 1ULL << fraction_bits;
>> +
>> +		if (!i && f < 0)
>> +			pr_cont("-%lld/%llu", -f, div);
>> +		else if (!i)
>> +			pr_cont("%lld/%llu", f, div);
>> +		else if (i < 0 || f < 0)
>> +			pr_cont("-%lld-%llu/%llu", -i, -f, div);
>> +		else
>> +			pr_cont("%lld+%llu/%llu", i, f, div);
>> +	} else {
>> +		if (!i && f < 0)
>> +			pr_cont("-%lld/(2^%u)", -f, fraction_bits);
>> +		else if (!i)
>> +			pr_cont("%lld/(2^%u)", f, fraction_bits);
>> +		else if (i < 0 || f < 0)
>> +			pr_cont("-%lld-%llu/(2^%u)", -i, -f, fraction_bits);
>> +		else
>> +			pr_cont("%lld+%llu/(2^%u)", i, f, fraction_bits);
>> +	}
>> +}
>> +
>>  void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>>  {
>>  	union v4l2_ctrl_ptr ptr = ctrl->p_cur;
>>
>>  	if (ctrl->is_array) {
>> -		unsigned i;
>> +		unsigned int i;
>>
>>  		for (i = 0; i < ctrl->nr_of_dims; i++)
>>  			pr_cont("[%u]", ctrl->dims[i]);
>> @@ -266,7 +296,10 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>>
>>  	switch (ctrl->type) {
>>  	case V4L2_CTRL_TYPE_INTEGER:
>> -		pr_cont("%d", *ptr.p_s32);
>> +		if (!ctrl->fraction_bits)
>> +			pr_cont("%d", *ptr.p_s32);
>> +		else
>> +			v4l2_ctrl_log_fp(*ptr.p_s32, ctrl->fraction_bits);
>>  		break;
>>  	case V4L2_CTRL_TYPE_BOOLEAN:
>>  		pr_cont("%s", *ptr.p_s32 ? "true" : "false");
>> @@ -281,19 +314,31 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>>  		pr_cont("0x%08x", *ptr.p_s32);
>>  		break;
>>  	case V4L2_CTRL_TYPE_INTEGER64:
>> -		pr_cont("%lld", *ptr.p_s64);
>> +		if (!ctrl->fraction_bits)
>> +			pr_cont("%lld", *ptr.p_s64);
>> +		else
>> +			v4l2_ctrl_log_fp(*ptr.p_s64, ctrl->fraction_bits);
>>  		break;
>>  	case V4L2_CTRL_TYPE_STRING:
>>  		pr_cont("%s", ptr.p_char);
>>  		break;
>>  	case V4L2_CTRL_TYPE_U8:
>> -		pr_cont("%u", (unsigned)*ptr.p_u8);
>> +		if (!ctrl->fraction_bits)
>> +			pr_cont("%u", (unsigned int)*ptr.p_u8);
>> +		else
>> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u8, ctrl->fraction_bits);
>>  		break;
>>  	case V4L2_CTRL_TYPE_U16:
>> -		pr_cont("%u", (unsigned)*ptr.p_u16);
>> +		if (!ctrl->fraction_bits)
>> +			pr_cont("%u", (unsigned int)*ptr.p_u16);
>> +		else
>> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u16, ctrl->fraction_bits);
>>  		break;
>>  	case V4L2_CTRL_TYPE_U32:
>> -		pr_cont("%u", (unsigned)*ptr.p_u32);
>> +		if (!ctrl->fraction_bits)
>> +			pr_cont("%u", (unsigned int)*ptr.p_u32);
>> +		else
>> +			v4l2_ctrl_log_fp((unsigned int)*ptr.p_u32, ctrl->fraction_bits);
>>  		break;
>>  	case V4L2_CTRL_TYPE_H264_SPS:
>>  		pr_cont("H264_SPS");
>> @@ -1752,7 +1797,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>  			u32 id, const char *name, enum v4l2_ctrl_type type,
>>  			s64 min, s64 max, u64 step, s64 def,
>>  			const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
>> -			u32 flags, const char * const *qmenu,
>> +			u32 fraction_bits, u32 flags, const char * const *qmenu,
>>  			const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>>  			void *priv)
>>  {
>> @@ -1939,6 +1984,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>  	ctrl->name = name;
>>  	ctrl->type = type;
>>  	ctrl->flags = flags;
>> +	ctrl->fraction_bits = fraction_bits;
>>  	ctrl->minimum = min;
>>  	ctrl->maximum = max;
>>  	ctrl->step = step;
>> @@ -2037,7 +2083,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>  	ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name,
>>  			type, min, max,
>>  			is_menu ? cfg->menu_skip_mask : step, def,
>> -			cfg->dims, cfg->elem_size,
>> +			cfg->dims, cfg->elem_size, cfg->fraction_bits,
>>  			flags, qmenu, qmenu_int, cfg->p_def, priv);
>>  	if (ctrl)
>>  		ctrl->is_private = cfg->is_private;
>> @@ -2062,7 +2108,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>>  		return NULL;
>>  	}
>>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> -			     min, max, step, def, NULL, 0,
>> +			     min, max, step, def, NULL, 0, 0,
>>  			     flags, NULL, NULL, ptr_null, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
>> @@ -2095,7 +2141,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>>  		return NULL;
>>  	}
>>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> -			     0, max, mask, def, NULL, 0,
>> +			     0, max, mask, def, NULL, 0, 0,
>>  			     flags, qmenu, qmenu_int, ptr_null, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
>> @@ -2127,7 +2173,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>  		return NULL;
>>  	}
>>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> -			     0, max, mask, def, NULL, 0,
>> +			     0, max, mask, def, NULL, 0, 0,
>>  			     flags, qmenu, NULL, ptr_null, NULL);
>>
>>  }
>> @@ -2149,7 +2195,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>>  		return NULL;
>>  	}
>>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> -			     min, max, step, def, NULL, 0,
>> +			     min, max, step, def, NULL, 0, 0,
>>  			     flags, NULL, NULL, p_def, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
>> @@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>  		return NULL;
>>  	}
>>  	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> -			     0, max, 0, def, NULL, 0,
>> +			     0, max, 0, def, NULL, 0, 0,
>>  			     flags, NULL, qmenu_int, ptr_null, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index 59679a42b3e7..c35514c5bf88 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>   *		except for dynamic arrays. In that case it is in the range of
>>   *		1 to @p_array_alloc_elems.
>>   * @dims:	The size of each dimension.
>> - * @nr_of_dims:The number of dimensions in @dims.
>> + * @nr_of_dims: The number of dimensions in @dims.
>> + * @fraction_bits: The number of fraction bits for fixed point values.
>>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>>   *		easy to skip menu items that are not valid. If bit X is set,
>>   *		then menu item X is skipped. Of course, this only works for
>> @@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>   *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
>>   *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
>>   * @flags:	The control's flags.
>> + * @fraction_bits: The number of fraction bits for fixed point values.
>>   * @priv:	The control's private pointer. For use by the driver. It is
>>   *		untouched by the control framework. Note that this pointer is
>>   *		not freed when the control is deleted. Should this be needed
>> @@ -286,6 +288,7 @@ struct v4l2_ctrl {
>>  	u32 new_elems;
>>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>>  	u32 nr_of_dims;
>> +	u32 fraction_bits;
>>  	union {
>>  		u64 step;
>>  		u64 menu_skip_mask;
>> @@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
>>   * @dims:	The size of each dimension.
>>   * @elem_size:	The size in bytes of the control.
>>   * @flags:	The control's flags.
>> + * @fraction_bits: The number of fraction bits for fixed point values.
>>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>>   *		easy to skip menu items that are not valid. If bit X is set,
>>   *		then menu item X is skipped. Of course, this only works for
>> @@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
>>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>>  	u32 elem_size;
>>  	u32 flags;
>> +	u32 fraction_bits;
>>  	u64 menu_skip_mask;
>>  	const char * const *qmenu;
>>  	const s64 *qmenu_int;
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index c3d4e490ce7c..26ecac19722a 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
>>  	__u32                elems;
>>  	__u32                nr_of_dims;
>>  	__u32                dims[V4L2_CTRL_MAX_DIMS];
>> -	__u32		     reserved[32];
>> +	__u32                fraction_bits;
> 
> u8 would suffice. Not that we'd be short of space but still...
> 
>> +	__u32		     reserved[31];
>>  };
>>
>> +static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int fraction_bits)
>> +{
>> +	return (i << fraction_bits) + f;
>> +}
>> +
>> +static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
>> +{
>> +	return v / (1LL << fraction_bits);
> 
> Why not just:
> 
> 	return v >> fraction_bits;

That works if v >= 0, but not for v < 0. Getting this right for negative
fixed point values was rather tricky. Actually, it is still wrong if
fraction_bits == 63. This works:

static inline long long v4l2_fp_integer(long long v, unsigned int fraction_bits)
{
        if (fraction_bits >= 63)
                return v < 0 ? -1 : 0;
        return v / (1ULL << fraction_bits);
}

> 
> I'd use macros so you could use whatever control types with this without
> casting. E.g.
> 
> #define V4L2_FP_INTEGER(v, fraction_bits) ((v) >> fraction_bits)
> 
> A more generic way to expose this could be to have base and exponent, the
> base being 2 in this case. Just an idea. This would of course be a little
> bit more difficult to use.

To be honest, I am not at all certain this should be in a public header.
I am inclined to drop it, especially since in userspace you can just use
floating point operations which makes working with fixed point a lot easier.

The code to extract the integer and fraction part is really only relevant
when logging the fixed point value.

Regards,

	Hans

> 
>> +}
>> +
>> +static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
>> +{
>> +	__u64 mask = (1ULL << fraction_bits) - 1;
>> +
>> +	return v < 0 ? -((-v) & mask) : (v & mask);
>> +}
>> +
>>  /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
>>  struct v4l2_querymenu {
>>  	__u32		id;
>

Shengjiu Wang Nov. 20, 2023, 8:30 a.m. UTC | #25

On Fri, Nov 17, 2023 at 8:07 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Here is an RFC patch adding support for 'fraction_bits'. It's lacking
> documentation, but it can be used for testing.
>
> It was rather a pain logging fixed point number in a reasonable format,
> but I think it is OK.
>
> In userspace (where you can use floating point) it is a lot easier:
>
> printf("%.*g\n", fraction_bits, (double)v * (1.0 / (1ULL << fraction_bits)));
>
> I decided to only expose fraction_bits in struct v4l2_query_ext_ctrl.
> I could add it to struct v4l2_queryctrl, but I did not think that was
> necessary. Other opinions are welcome.
>
> In the meantime, let me know if this works for your patch series. If it
> does, then I can clean this up.

Thanks.  It works for me.  What I have done are:
1. drop FIXED_POINT
2. use v4l2_ctrl_new_custom

Best regards
Wang shengjiu

>
> Regards,
>
>         Hans
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls-api.c  |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c | 72 +++++++++++++++++++----
>  include/media/v4l2-ctrls.h                |  7 ++-
>  include/uapi/linux/videodev2.h            | 20 ++++++-
>  4 files changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 002ea6588edf..3132df315b17 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -1101,6 +1101,7 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>         qc->elems = ctrl->elems;
>         qc->nr_of_dims = ctrl->nr_of_dims;
>         memcpy(qc->dims, ctrl->dims, qc->nr_of_dims * sizeof(qc->dims[0]));
> +       qc->fraction_bits = ctrl->fraction_bits;
>         qc->minimum = ctrl->minimum;
>         qc->maximum = ctrl->maximum;
>         qc->default_value = ctrl->default_value;
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..0e08a371af5c 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -252,12 +252,42 @@ void v4l2_ctrl_type_op_init(const struct v4l2_ctrl *ctrl, u32 from_idx,
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_type_op_init);
>
> +static void v4l2_ctrl_log_fp(s64 v, unsigned int fraction_bits)
> +{
> +       s64 i = v4l2_fp_integer(v, fraction_bits);
> +       s64 f = v4l2_fp_fraction(v, fraction_bits);
> +
> +       if (!f) {
> +               pr_cont("%lld", i);
> +       } else if (fraction_bits < 20) {
> +               u64 div = 1ULL << fraction_bits;
> +
> +               if (!i && f < 0)
> +                       pr_cont("-%lld/%llu", -f, div);
> +               else if (!i)
> +                       pr_cont("%lld/%llu", f, div);
> +               else if (i < 0 || f < 0)
> +                       pr_cont("-%lld-%llu/%llu", -i, -f, div);
> +               else
> +                       pr_cont("%lld+%llu/%llu", i, f, div);
> +       } else {
> +               if (!i && f < 0)
> +                       pr_cont("-%lld/(2^%u)", -f, fraction_bits);
> +               else if (!i)
> +                       pr_cont("%lld/(2^%u)", f, fraction_bits);
> +               else if (i < 0 || f < 0)
> +                       pr_cont("-%lld-%llu/(2^%u)", -i, -f, fraction_bits);
> +               else
> +                       pr_cont("%lld+%llu/(2^%u)", i, f, fraction_bits);
> +       }
> +}
> +
>  void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>  {
>         union v4l2_ctrl_ptr ptr = ctrl->p_cur;
>
>         if (ctrl->is_array) {
> -               unsigned i;
> +               unsigned int i;
>
>                 for (i = 0; i < ctrl->nr_of_dims; i++)
>                         pr_cont("[%u]", ctrl->dims[i]);
> @@ -266,7 +296,10 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>
>         switch (ctrl->type) {
>         case V4L2_CTRL_TYPE_INTEGER:
> -               pr_cont("%d", *ptr.p_s32);
> +               if (!ctrl->fraction_bits)
> +                       pr_cont("%d", *ptr.p_s32);
> +               else
> +                       v4l2_ctrl_log_fp(*ptr.p_s32, ctrl->fraction_bits);
>                 break;
>         case V4L2_CTRL_TYPE_BOOLEAN:
>                 pr_cont("%s", *ptr.p_s32 ? "true" : "false");
> @@ -281,19 +314,31 @@ void v4l2_ctrl_type_op_log(const struct v4l2_ctrl *ctrl)
>                 pr_cont("0x%08x", *ptr.p_s32);
>                 break;
>         case V4L2_CTRL_TYPE_INTEGER64:
> -               pr_cont("%lld", *ptr.p_s64);
> +               if (!ctrl->fraction_bits)
> +                       pr_cont("%lld", *ptr.p_s64);
> +               else
> +                       v4l2_ctrl_log_fp(*ptr.p_s64, ctrl->fraction_bits);
>                 break;
>         case V4L2_CTRL_TYPE_STRING:
>                 pr_cont("%s", ptr.p_char);
>                 break;
>         case V4L2_CTRL_TYPE_U8:
> -               pr_cont("%u", (unsigned)*ptr.p_u8);
> +               if (!ctrl->fraction_bits)
> +                       pr_cont("%u", (unsigned int)*ptr.p_u8);
> +               else
> +                       v4l2_ctrl_log_fp((unsigned int)*ptr.p_u8, ctrl->fraction_bits);
>                 break;
>         case V4L2_CTRL_TYPE_U16:
> -               pr_cont("%u", (unsigned)*ptr.p_u16);
> +               if (!ctrl->fraction_bits)
> +                       pr_cont("%u", (unsigned int)*ptr.p_u16);
> +               else
> +                       v4l2_ctrl_log_fp((unsigned int)*ptr.p_u16, ctrl->fraction_bits);
>                 break;
>         case V4L2_CTRL_TYPE_U32:
> -               pr_cont("%u", (unsigned)*ptr.p_u32);
> +               if (!ctrl->fraction_bits)
> +                       pr_cont("%u", (unsigned int)*ptr.p_u32);
> +               else
> +                       v4l2_ctrl_log_fp((unsigned int)*ptr.p_u32, ctrl->fraction_bits);
>                 break;
>         case V4L2_CTRL_TYPE_H264_SPS:
>                 pr_cont("H264_SPS");
> @@ -1752,7 +1797,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>                         u32 id, const char *name, enum v4l2_ctrl_type type,
>                         s64 min, s64 max, u64 step, s64 def,
>                         const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> -                       u32 flags, const char * const *qmenu,
> +                       u32 fraction_bits, u32 flags, const char * const *qmenu,
>                         const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>                         void *priv)
>  {
> @@ -1939,6 +1984,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>         ctrl->name = name;
>         ctrl->type = type;
>         ctrl->flags = flags;
> +       ctrl->fraction_bits = fraction_bits;
>         ctrl->minimum = min;
>         ctrl->maximum = max;
>         ctrl->step = step;
> @@ -2037,7 +2083,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>         ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name,
>                         type, min, max,
>                         is_menu ? cfg->menu_skip_mask : step, def,
> -                       cfg->dims, cfg->elem_size,
> +                       cfg->dims, cfg->elem_size, cfg->fraction_bits,
>                         flags, qmenu, qmenu_int, cfg->p_def, priv);
>         if (ctrl)
>                 ctrl->is_private = cfg->is_private;
> @@ -2062,7 +2108,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>                 return NULL;
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -                            min, max, step, def, NULL, 0,
> +                            min, max, step, def, NULL, 0, 0,
>                              flags, NULL, NULL, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
> @@ -2095,7 +2141,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>                 return NULL;
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -                            0, max, mask, def, NULL, 0,
> +                            0, max, mask, def, NULL, 0, 0,
>                              flags, qmenu, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
> @@ -2127,7 +2173,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>                 return NULL;
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -                            0, max, mask, def, NULL, 0,
> +                            0, max, mask, def, NULL, 0, 0,
>                              flags, qmenu, NULL, ptr_null, NULL);
>
>  }
> @@ -2149,7 +2195,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>                 return NULL;
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -                            min, max, step, def, NULL, 0,
> +                            min, max, step, def, NULL, 0, 0,
>                              flags, NULL, NULL, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> @@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>                 return NULL;
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> -                            0, max, 0, def, NULL, 0,
> +                            0, max, 0, def, NULL, 0, 0,
>                              flags, NULL, qmenu_int, ptr_null, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 59679a42b3e7..c35514c5bf88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *             except for dynamic arrays. In that case it is in the range of
>   *             1 to @p_array_alloc_elems.
>   * @dims:      The size of each dimension.
> - * @nr_of_dims:The number of dimensions in @dims.
> + * @nr_of_dims: The number of dimensions in @dims.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *             easy to skip menu items that are not valid. If bit X is set,
>   *             then menu item X is skipped. Of course, this only works for
> @@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *             :math:`ceil(\frac{maximum - minimum}{step}) + 1`.
>   *             Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
>   * @flags:     The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @priv:      The control's private pointer. For use by the driver. It is
>   *             untouched by the control framework. Note that this pointer is
>   *             not freed when the control is deleted. Should this be needed
> @@ -286,6 +288,7 @@ struct v4l2_ctrl {
>         u32 new_elems;
>         u32 dims[V4L2_CTRL_MAX_DIMS];
>         u32 nr_of_dims;
> +       u32 fraction_bits;
>         union {
>                 u64 step;
>                 u64 menu_skip_mask;
> @@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
>   * @dims:      The size of each dimension.
>   * @elem_size: The size in bytes of the control.
>   * @flags:     The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
>   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
>   *             easy to skip menu items that are not valid. If bit X is set,
>   *             then menu item X is skipped. Of course, this only works for
> @@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
>         u32 dims[V4L2_CTRL_MAX_DIMS];
>         u32 elem_size;
>         u32 flags;
> +       u32 fraction_bits;
>         u64 menu_skip_mask;
>         const char * const *qmenu;
>         const s64 *qmenu_int;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..26ecac19722a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
>         __u32                elems;
>         __u32                nr_of_dims;
>         __u32                dims[V4L2_CTRL_MAX_DIMS];
> -       __u32                reserved[32];
> +       __u32                fraction_bits;
> +       __u32                reserved[31];
>  };
>
> +static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int fraction_bits)
> +{
> +       return (i << fraction_bits) + f;
> +}
> +
> +static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
> +{
> +       return v / (1LL << fraction_bits);
> +}
> +
> +static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
> +{
> +       __u64 mask = (1ULL << fraction_bits) - 1;
> +
> +       return v < 0 ? -((-v) & mask) : (v & mask);
> +}
> +
>  /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
>  struct v4l2_querymenu {
>         __u32           id;
> --
> 2.42.0
>
>

[v9,10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

Commit Message

Comments

Patch