diff mbox series

[RFC,v6,09/11] media: uapi: Add audio rate controls support

Message ID 1697185865-27528-10-git-send-email-shengjiu.wang@nxp.com (mailing list archive)
State Superseded
Headers show
Series Add audio support in v4l2 framework | expand

Commit Message

Shengjiu Wang Oct. 13, 2023, 8:31 a.m. UTC
Fixed point controls are used by the user to configure
the audio sample rate to driver.

Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
new IDs for ASRC rate control.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 .../userspace-api/media/v4l/common.rst        |  1 +
 .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
 .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
 .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
 .../media/videodev2.h.rst.exceptions          |  1 +
 drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
 include/media/v4l2-ctrls.h                    |  2 ++
 include/uapi/linux/v4l2-controls.h            | 13 +++++++
 include/uapi/linux/videodev2.h                |  3 ++
 10 files changed, 76 insertions(+)
 create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst

Comments

Hans Verkuil Oct. 16, 2023, 1:16 p.m. UTC | #1
Hi Shengjiu,

On 13/10/2023 10:31, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> the audio sample rate to driver.
> 
> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> new IDs for ASRC rate control.
> 
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
>  .../userspace-api/media/v4l/common.rst        |  1 +
>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>  .../media/videodev2.h.rst.exceptions          |  1 +
>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>  include/media/v4l2-ctrls.h                    |  2 ++
>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>  include/uapi/linux/videodev2.h                |  3 ++
>  10 files changed, 76 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> 
> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> index ea0435182e44..35707edffb13 100644
> --- a/Documentation/userspace-api/media/v4l/common.rst
> +++ b/Documentation/userspace-api/media/v4l/common.rst
> @@ -52,6 +52,7 @@ applicable to all devices.
>      ext-ctrls-fm-rx
>      ext-ctrls-detect
>      ext-ctrls-colorimetry
> +    ext-ctrls-fixed-point

Rename this to ext-ctrls-audio-m2m.

>      fourcc
>      format
>      planar-apis
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> new file mode 100644
> index 000000000000..2ef6e250580c
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> @@ -0,0 +1,36 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _fixed-point-controls:
> +
> +***************************
> +Fixed Point Control Reference

This is for audio controls. "Fixed Point" is just the type, and it doesn't make
sense to group fixed point controls. But it does make sense to group the audio
controls.

V4L2 controls can be grouped into classes. Basically it is a way to put controls
into categories, and for each category there is also a control that gives a
description of the class (see 2.15.15 in
https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)

If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
they are grouped based on what class of control they are.

So I think it would be a good idea to create a new control class for M2M audio controls,
instead of just adding them to the catch-all 'User Controls' class.

Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
it is done.

M2M_AUDIO would probably be a good name for the class.

> +***************************
> +
> +These controls are intended to support an asynchronous sample
> +rate converter.

Add ' (ASRC).' at the end to indicate the common abbreviation for
that.

> +
> +.. _v4l2-audio-asrc:
> +
> +``V4L2_CID_ASRC_SOURCE_RATE``
> +    sets the resampler source rate.
> +
> +``V4L2_CID_ASRC_DEST_RATE``
> +    sets the resampler destination rate.

Document the unit (Hz) for these two controls.

> +
> +.. c:type:: v4l2_ctrl_fixed_point
> +
> +.. cssclass:: longtable
> +
> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> +
> +.. flat-table:: struct v4l2_ctrl_fixed_point
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       1 1 2
> +
> +    * - __u32

Hmm, shouldn't this be __s32?

> +      - ``integer``
> +      - integer part of fixed point value.
> +    * - __s32

and this __u32?

You want to be able to use this generic type as a signed value.

> +      - ``fractional``
> +      - fractional part of fixed point value, which is Q31.
> 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 f9f73530a6be..1811dabf5c74 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -295,6 +295,10 @@ still cause this situation.
>        - ``p_av1_film_grain``
>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> +      - ``p_fixed_point``
> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>      * - void *
>        - ``ptr``
>        - A pointer to a compound type which can be an N-dimensional array
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 4d38acafe8e1..9285f4f39eed 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> @@ -549,6 +549,13 @@ 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``
> +      - n/a
> +      - n/a
> +      - n/a
> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> +        integer part and fractional part, i.e. audio sample rate.
> +
>  
>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a662fb60f73f..7a616ac91059 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>  		if (!area->width || !area->height)
>  			return -EINVAL;
>  		break;
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> +		break;

Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':

https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/

since min and max values are perfectly fine for a fixed point value.

Even a step value (currently not supported in that patch) would make sense.

But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
why not represent the fixed point value as a Q31.32. Then the standard
minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.

Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
value.

So in enum v4l2_ctrl_type add:

	V4L2_CTRL_TYPE_FIXED_POINT = 10,

(10, because it is no longer a compound type).

>  
>  	default:
>  		return -EINVAL;
> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	case V4L2_CTRL_TYPE_AREA:
>  		elem_size = sizeof(struct v4l2_area);
>  		break;
> +	case V4L2_CTRL_TYPE_FIXED_POINT:
> +		elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> +		break;
>  	default:
>  		if (type < V4L2_CTRL_COMPOUND_TYPES)
>  			elem_size = sizeof(s32);
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 8696eb1cdd61..d8f232df6b6a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>  		*type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>  		break;
> +	case V4L2_CID_ASRC_SOURCE_RATE:
> +	case V4L2_CID_ASRC_DEST_RATE:
> +		*type = V4L2_CTRL_TYPE_FIXED_POINT;
> +		break;
>  	default:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		break;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 59679a42b3e7..645e4cccafc7 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -56,6 +56,7 @@ struct video_device;
>   * @p_av1_tile_group_entry:	Pointer to an AV1 tile group entry structure.
>   * @p_av1_frame:		Pointer to an AV1 frame structure.
>   * @p_av1_film_grain:		Pointer to an AV1 film grain structure.
> + * @p_fixed_point:		Pointer to a struct v4l2_ctrl_fixed_point.
>   * @p:				Pointer to a compound value.
>   * @p_const:			Pointer to a constant compound value.
>   */
> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
>  	struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
>  	struct v4l2_ctrl_av1_frame *p_av1_frame;
>  	struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> +	struct v4l2_ctrl_fixed_point *p_fixed_point;
>  	void *p;
>  	const void *p_const;
>  };
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index c3604a0a3e30..91096259e3ea 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
>  
>  /* last CID + 1 */
>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> +#define V4L2_CID_ASRC_SOURCE_RATE		(V4L2_CID_BASE + 45)
> +#define V4L2_CID_ASRC_DEST_RATE			(V4L2_CID_BASE + 46)

This patch needs to be split in three parts:

1) Add the new M2M_AUDIO control class,
2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
3) Add the new controls.

These are all independent changes, so separating them makes it easier to
review.

>  
>  /* USER-class private control IDs */
>  
> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
>  #endif
>  
> +/**
> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> + *
> + * @rate_integer: integer part of fixed point value.
> + * @rate_fractional: fractional part of fixed point value
> + */
> +struct v4l2_ctrl_fixed_point {
> +	__u32 integer;

__s32?

> +	__u32 fractional;
> +};
> +
>  #endif
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2ac7b989394c..3ef32c09c2fa 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
>  		struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
>  		struct v4l2_ctrl_av1_frame __user *p_av1_frame;
>  		struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> +		struct v4l2_ctrl_fixed_point __user *p_fixed_point;
>  		void __user *ptr;
>  	};
>  } __attribute__ ((packed));
> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>  	V4L2_CTRL_TYPE_AV1_FRAME	    = 0x282,
>  	V4L2_CTRL_TYPE_AV1_FILM_GRAIN	    = 0x283,
> +
> +	V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
>  };
>  
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */

Regards,

	Hans
Shengjiu Wang Oct. 17, 2023, 1:11 p.m. UTC | #2
On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Shengjiu,
>
> On 13/10/2023 10:31, Shengjiu Wang wrote:
> > Fixed point controls are used by the user to configure
> > the audio sample rate to driver.
> >
> > Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> > new IDs for ASRC rate control.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  .../userspace-api/media/v4l/common.rst        |  1 +
> >  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >  .../media/videodev2.h.rst.exceptions          |  1 +
> >  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >  include/media/v4l2-ctrls.h                    |  2 ++
> >  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >  include/uapi/linux/videodev2.h                |  3 ++
> >  10 files changed, 76 insertions(+)
> >  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >
> > diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> > index ea0435182e44..35707edffb13 100644
> > --- a/Documentation/userspace-api/media/v4l/common.rst
> > +++ b/Documentation/userspace-api/media/v4l/common.rst
> > @@ -52,6 +52,7 @@ applicable to all devices.
> >      ext-ctrls-fm-rx
> >      ext-ctrls-detect
> >      ext-ctrls-colorimetry
> > +    ext-ctrls-fixed-point
>
> Rename this to ext-ctrls-audio-m2m.
>
> >      fourcc
> >      format
> >      planar-apis
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> > new file mode 100644
> > index 000000000000..2ef6e250580c
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> > @@ -0,0 +1,36 @@
> > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > +
> > +.. _fixed-point-controls:
> > +
> > +***************************
> > +Fixed Point Control Reference
>
> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> sense to group fixed point controls. But it does make sense to group the audio
> controls.
>
> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> into categories, and for each category there is also a control that gives a
> description of the class (see 2.15.15 in
> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>
> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> they are grouped based on what class of control they are.
>
> So I think it would be a good idea to create a new control class for M2M audio controls,
> instead of just adding them to the catch-all 'User Controls' class.
>
> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> it is done.
>
> M2M_AUDIO would probably be a good name for the class.
>
> > +***************************
> > +
> > +These controls are intended to support an asynchronous sample
> > +rate converter.
>
> Add ' (ASRC).' at the end to indicate the common abbreviation for
> that.
>
> > +
> > +.. _v4l2-audio-asrc:
> > +
> > +``V4L2_CID_ASRC_SOURCE_RATE``
> > +    sets the resampler source rate.
> > +
> > +``V4L2_CID_ASRC_DEST_RATE``
> > +    sets the resampler destination rate.
>
> Document the unit (Hz) for these two controls.
>
> > +
> > +.. c:type:: v4l2_ctrl_fixed_point
> > +
> > +.. cssclass:: longtable
> > +
> > +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> > +
> > +.. flat-table:: struct v4l2_ctrl_fixed_point
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +    :widths:       1 1 2
> > +
> > +    * - __u32
>
> Hmm, shouldn't this be __s32?
>
> > +      - ``integer``
> > +      - integer part of fixed point value.
> > +    * - __s32
>
> and this __u32?
>
> You want to be able to use this generic type as a signed value.
>
> > +      - ``fractional``
> > +      - fractional part of fixed point value, which is Q31.
> > 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 f9f73530a6be..1811dabf5c74 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > @@ -295,6 +295,10 @@ still cause this situation.
> >        - ``p_av1_film_grain``
> >        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> > +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> > +      - ``p_fixed_point``
> > +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> > +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >      * - void *
> >        - ``ptr``
> >        - A pointer to a compound type which can be an N-dimensional array
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > index 4d38acafe8e1..9285f4f39eed 100644
> > --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > @@ -549,6 +549,13 @@ 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``
> > +      - n/a
> > +      - n/a
> > +      - n/a
> > +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> > +        integer part and fractional part, i.e. audio sample rate.
> > +
> >
> >  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > index a662fb60f73f..7a616ac91059 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >               if (!area->width || !area->height)
> >                       return -EINVAL;
> >               break;
> > +     case V4L2_CTRL_TYPE_FIXED_POINT:
> > +             break;
>
> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>
> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>
> since min and max values are perfectly fine for a fixed point value.
>
> Even a step value (currently not supported in that patch) would make sense.
>
> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> why not represent the fixed point value as a Q31.32. Then the standard
> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>
> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> value.
>
> So in enum v4l2_ctrl_type add:
>
>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>
> (10, because it is no longer a compound type).

Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?

The reason I use the 'integer' and 'fractional' is that I want
'integer' to be the normal sample
rate, for example 48kHz.  The 'fractional' is the difference with
normal sample rate.

For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.

So if we use s64 for rate, then in driver need to convert the rate to
the closed normal
sample rate + fractional.

best regards
wang shengjiu

>
> >
> >       default:
> >               return -EINVAL;
> > @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >       case V4L2_CTRL_TYPE_AREA:
> >               elem_size = sizeof(struct v4l2_area);
> >               break;
> > +     case V4L2_CTRL_TYPE_FIXED_POINT:
> > +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> > +             break;
> >       default:
> >               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >                       elem_size = sizeof(s32);
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 8696eb1cdd61..d8f232df6b6a 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >               break;
> > +     case V4L2_CID_ASRC_SOURCE_RATE:
> > +     case V4L2_CID_ASRC_DEST_RATE:
> > +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> > +             break;
> >       default:
> >               *type = V4L2_CTRL_TYPE_INTEGER;
> >               break;
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 59679a42b3e7..645e4cccafc7 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -56,6 +56,7 @@ struct video_device;
> >   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> >   * @p_av1_frame:             Pointer to an AV1 frame structure.
> >   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> > + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> >   * @p:                               Pointer to a compound value.
> >   * @p_const:                 Pointer to a constant compound value.
> >   */
> > @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> >       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> >       struct v4l2_ctrl_av1_frame *p_av1_frame;
> >       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> > +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> >       void *p;
> >       const void *p_const;
> >  };
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index c3604a0a3e30..91096259e3ea 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> >
> >  /* last CID + 1 */
> >  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> > +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> > +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
>
> This patch needs to be split in three parts:
>
> 1) Add the new M2M_AUDIO control class,
> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> 3) Add the new controls.
>
> These are all independent changes, so separating them makes it easier to
> review.
>
> >
> >  /* USER-class private control IDs */
> >
> > @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> >  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> >  #endif
> >
> > +/**
> > + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> > + *
> > + * @rate_integer: integer part of fixed point value.
> > + * @rate_fractional: fractional part of fixed point value
> > + */
> > +struct v4l2_ctrl_fixed_point {
> > +     __u32 integer;
>
> __s32?
>
> > +     __u32 fractional;
> > +};
> > +
> >  #endif
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 2ac7b989394c..3ef32c09c2fa 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> >               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> >               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> >               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> > +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> >               void __user *ptr;
> >       };
> >  } __attribute__ ((packed));
> > @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> >       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> >       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> >       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> > +
> > +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> >  };
> >
> >  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>
> Regards,
>
>         Hans
Hans Verkuil Oct. 17, 2023, 1:37 p.m. UTC | #3
On 17/10/2023 15:11, Shengjiu Wang wrote:
> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Shengjiu,
>>
>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>> Fixed point controls are used by the user to configure
>>> the audio sample rate to driver.
>>>
>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>> new IDs for ASRC rate control.
>>>
>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>> ---
>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>  10 files changed, 76 insertions(+)
>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>
>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>> index ea0435182e44..35707edffb13 100644
>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>      ext-ctrls-fm-rx
>>>      ext-ctrls-detect
>>>      ext-ctrls-colorimetry
>>> +    ext-ctrls-fixed-point
>>
>> Rename this to ext-ctrls-audio-m2m.
>>
>>>      fourcc
>>>      format
>>>      planar-apis
>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>> new file mode 100644
>>> index 000000000000..2ef6e250580c
>>> --- /dev/null
>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>> @@ -0,0 +1,36 @@
>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>> +
>>> +.. _fixed-point-controls:
>>> +
>>> +***************************
>>> +Fixed Point Control Reference
>>
>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>> sense to group fixed point controls. But it does make sense to group the audio
>> controls.
>>
>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>> into categories, and for each category there is also a control that gives a
>> description of the class (see 2.15.15 in
>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>
>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>> they are grouped based on what class of control they are.
>>
>> So I think it would be a good idea to create a new control class for M2M audio controls,
>> instead of just adding them to the catch-all 'User Controls' class.
>>
>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>> it is done.
>>
>> M2M_AUDIO would probably be a good name for the class.
>>
>>> +***************************
>>> +
>>> +These controls are intended to support an asynchronous sample
>>> +rate converter.
>>
>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>> that.
>>
>>> +
>>> +.. _v4l2-audio-asrc:
>>> +
>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>> +    sets the resampler source rate.
>>> +
>>> +``V4L2_CID_ASRC_DEST_RATE``
>>> +    sets the resampler destination rate.
>>
>> Document the unit (Hz) for these two controls.
>>
>>> +
>>> +.. c:type:: v4l2_ctrl_fixed_point
>>> +
>>> +.. cssclass:: longtable
>>> +
>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>> +
>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +    :widths:       1 1 2
>>> +
>>> +    * - __u32
>>
>> Hmm, shouldn't this be __s32?
>>
>>> +      - ``integer``
>>> +      - integer part of fixed point value.
>>> +    * - __s32
>>
>> and this __u32?
>>
>> You want to be able to use this generic type as a signed value.
>>
>>> +      - ``fractional``
>>> +      - fractional part of fixed point value, which is Q31.
>>> 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 f9f73530a6be..1811dabf5c74 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>        - ``p_av1_film_grain``
>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>> +      - ``p_fixed_point``
>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>      * - void *
>>>        - ``ptr``
>>>        - A pointer to a compound type which can be an N-dimensional array
>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> index 4d38acafe8e1..9285f4f39eed 100644
>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>> @@ -549,6 +549,13 @@ 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``
>>> +      - n/a
>>> +      - n/a
>>> +      - n/a
>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>> +        integer part and fractional part, i.e. audio sample rate.
>>> +
>>>
>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> index a662fb60f73f..7a616ac91059 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>               if (!area->width || !area->height)
>>>                       return -EINVAL;
>>>               break;
>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>> +             break;
>>
>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>
>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>
>> since min and max values are perfectly fine for a fixed point value.
>>
>> Even a step value (currently not supported in that patch) would make sense.
>>
>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>> why not represent the fixed point value as a Q31.32. Then the standard
>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>
>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>> value.
>>
>> So in enum v4l2_ctrl_type add:
>>
>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>
>> (10, because it is no longer a compound type).
> 
> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> 
> The reason I use the 'integer' and 'fractional' is that I want
> 'integer' to be the normal sample
> rate, for example 48kHz.  The 'fractional' is the difference with
> normal sample rate.
> 
> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> 
> So if we use s64 for rate, then in driver need to convert the rate to
> the closed normal
> sample rate + fractional.

That wasn't what the documentation said :-)

So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
means in this context) and the offset to the actual sample rate.

Presumably the 'normal' sample rate is set once, while the offset changes
regularly.

But why do you need the 'normal' sample rate? With audio resampling I assume
you resample from one rate to another, so why do you need a third 'normal'
rate?

Regards,

	Hans

> 
> best regards
> wang shengjiu
> 
>>
>>>
>>>       default:
>>>               return -EINVAL;
>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>       case V4L2_CTRL_TYPE_AREA:
>>>               elem_size = sizeof(struct v4l2_area);
>>>               break;
>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
>>> +             break;
>>>       default:
>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
>>>                       elem_size = sizeof(s32);
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> index 8696eb1cdd61..d8f232df6b6a 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>>>               break;
>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
>>> +     case V4L2_CID_ASRC_DEST_RATE:
>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
>>> +             break;
>>>       default:
>>>               *type = V4L2_CTRL_TYPE_INTEGER;
>>>               break;
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index 59679a42b3e7..645e4cccafc7 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -56,6 +56,7 @@ struct video_device;
>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
>>>   * @p:                               Pointer to a compound value.
>>>   * @p_const:                 Pointer to a constant compound value.
>>>   */
>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
>>>       void *p;
>>>       const void *p_const;
>>>  };
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index c3604a0a3e30..91096259e3ea 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
>>>
>>>  /* last CID + 1 */
>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
>>
>> This patch needs to be split in three parts:
>>
>> 1) Add the new M2M_AUDIO control class,
>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
>> 3) Add the new controls.
>>
>> These are all independent changes, so separating them makes it easier to
>> review.
>>
>>>
>>>  /* USER-class private control IDs */
>>>
>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
>>>  #endif
>>>
>>> +/**
>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
>>> + *
>>> + * @rate_integer: integer part of fixed point value.
>>> + * @rate_fractional: fractional part of fixed point value
>>> + */
>>> +struct v4l2_ctrl_fixed_point {
>>> +     __u32 integer;
>>
>> __s32?
>>
>>> +     __u32 fractional;
>>> +};
>>> +
>>>  #endif
>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>> index 2ac7b989394c..3ef32c09c2fa 100644
>>> --- a/include/uapi/linux/videodev2.h
>>> +++ b/include/uapi/linux/videodev2.h
>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
>>>               void __user *ptr;
>>>       };
>>>  } __attribute__ ((packed));
>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
>>> +
>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
>>>  };
>>>
>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>>
>> Regards,
>>
>>         Hans
Shengjiu Wang Oct. 18, 2023, 2:27 a.m. UTC | #4
On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 17/10/2023 15:11, Shengjiu Wang wrote:
> > On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> Hi Shengjiu,
> >>
> >> On 13/10/2023 10:31, Shengjiu Wang wrote:
> >>> Fixed point controls are used by the user to configure
> >>> the audio sample rate to driver.
> >>>
> >>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> >>> new IDs for ASRC rate control.
> >>>
> >>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>> ---
> >>>  .../userspace-api/media/v4l/common.rst        |  1 +
> >>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >>>  include/media/v4l2-ctrls.h                    |  2 ++
> >>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >>>  include/uapi/linux/videodev2.h                |  3 ++
> >>>  10 files changed, 76 insertions(+)
> >>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>
> >>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> >>> index ea0435182e44..35707edffb13 100644
> >>> --- a/Documentation/userspace-api/media/v4l/common.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> >>> @@ -52,6 +52,7 @@ applicable to all devices.
> >>>      ext-ctrls-fm-rx
> >>>      ext-ctrls-detect
> >>>      ext-ctrls-colorimetry
> >>> +    ext-ctrls-fixed-point
> >>
> >> Rename this to ext-ctrls-audio-m2m.
> >>
> >>>      fourcc
> >>>      format
> >>>      planar-apis
> >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>> new file mode 100644
> >>> index 000000000000..2ef6e250580c
> >>> --- /dev/null
> >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>> @@ -0,0 +1,36 @@
> >>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >>> +
> >>> +.. _fixed-point-controls:
> >>> +
> >>> +***************************
> >>> +Fixed Point Control Reference
> >>
> >> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> >> sense to group fixed point controls. But it does make sense to group the audio
> >> controls.
> >>
> >> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> >> into categories, and for each category there is also a control that gives a
> >> description of the class (see 2.15.15 in
> >> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> >>
> >> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> >> they are grouped based on what class of control they are.
> >>
> >> So I think it would be a good idea to create a new control class for M2M audio controls,
> >> instead of just adding them to the catch-all 'User Controls' class.
> >>
> >> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> >> it is done.
> >>
> >> M2M_AUDIO would probably be a good name for the class.
> >>
> >>> +***************************
> >>> +
> >>> +These controls are intended to support an asynchronous sample
> >>> +rate converter.
> >>
> >> Add ' (ASRC).' at the end to indicate the common abbreviation for
> >> that.
> >>
> >>> +
> >>> +.. _v4l2-audio-asrc:
> >>> +
> >>> +``V4L2_CID_ASRC_SOURCE_RATE``
> >>> +    sets the resampler source rate.
> >>> +
> >>> +``V4L2_CID_ASRC_DEST_RATE``
> >>> +    sets the resampler destination rate.
> >>
> >> Document the unit (Hz) for these two controls.
> >>
> >>> +
> >>> +.. c:type:: v4l2_ctrl_fixed_point
> >>> +
> >>> +.. cssclass:: longtable
> >>> +
> >>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> >>> +
> >>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> >>> +    :header-rows:  0
> >>> +    :stub-columns: 0
> >>> +    :widths:       1 1 2
> >>> +
> >>> +    * - __u32
> >>
> >> Hmm, shouldn't this be __s32?
> >>
> >>> +      - ``integer``
> >>> +      - integer part of fixed point value.
> >>> +    * - __s32
> >>
> >> and this __u32?
> >>
> >> You want to be able to use this generic type as a signed value.
> >>
> >>> +      - ``fractional``
> >>> +      - fractional part of fixed point value, which is Q31.
> >>> 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 f9f73530a6be..1811dabf5c74 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>> @@ -295,6 +295,10 @@ still cause this situation.
> >>>        - ``p_av1_film_grain``
> >>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> >>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> >>> +      - ``p_fixed_point``
> >>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> >>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >>>      * - void *
> >>>        - ``ptr``
> >>>        - A pointer to a compound type which can be an N-dimensional array
> >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> index 4d38acafe8e1..9285f4f39eed 100644
> >>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>> @@ -549,6 +549,13 @@ 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``
> >>> +      - n/a
> >>> +      - n/a
> >>> +      - n/a
> >>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> >>> +        integer part and fractional part, i.e. audio sample rate.
> >>> +
> >>>
> >>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> index a662fb60f73f..7a616ac91059 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>               if (!area->width || !area->height)
> >>>                       return -EINVAL;
> >>>               break;
> >>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>> +             break;
> >>
> >> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> >>
> >> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> >>
> >> since min and max values are perfectly fine for a fixed point value.
> >>
> >> Even a step value (currently not supported in that patch) would make sense.
> >>
> >> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> >> why not represent the fixed point value as a Q31.32. Then the standard
> >> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> >>
> >> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> >> value.
> >>
> >> So in enum v4l2_ctrl_type add:
> >>
> >>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> >>
> >> (10, because it is no longer a compound type).
> >
> > Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> >
> > The reason I use the 'integer' and 'fractional' is that I want
> > 'integer' to be the normal sample
> > rate, for example 48kHz.  The 'fractional' is the difference with
> > normal sample rate.
> >
> > For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> >
> > So if we use s64 for rate, then in driver need to convert the rate to
> > the closed normal
> > sample rate + fractional.
>
> That wasn't what the documentation said :-)
>
> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> means in this context) and the offset to the actual sample rate.
>
> Presumably the 'normal' sample rate is set once, while the offset changes
> regularly.
>
> But why do you need the 'normal' sample rate? With audio resampling I assume
> you resample from one rate to another, so why do you need a third 'normal'
> rate?
>

'Normal' rate is used to select the prefilter table.

Best regards
Wang Shengjiu

> Regards,
>
>         Hans
>
> >
> > best regards
> > wang shengjiu
> >
> >>
> >>>
> >>>       default:
> >>>               return -EINVAL;
> >>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>>       case V4L2_CTRL_TYPE_AREA:
> >>>               elem_size = sizeof(struct v4l2_area);
> >>>               break;
> >>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> >>> +             break;
> >>>       default:
> >>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >>>                       elem_size = sizeof(s32);
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> index 8696eb1cdd61..d8f232df6b6a 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >>>               break;
> >>> +     case V4L2_CID_ASRC_SOURCE_RATE:
> >>> +     case V4L2_CID_ASRC_DEST_RATE:
> >>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> >>> +             break;
> >>>       default:
> >>>               *type = V4L2_CTRL_TYPE_INTEGER;
> >>>               break;
> >>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>> index 59679a42b3e7..645e4cccafc7 100644
> >>> --- a/include/media/v4l2-ctrls.h
> >>> +++ b/include/media/v4l2-ctrls.h
> >>> @@ -56,6 +56,7 @@ struct video_device;
> >>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> >>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
> >>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> >>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> >>>   * @p:                               Pointer to a compound value.
> >>>   * @p_const:                 Pointer to a constant compound value.
> >>>   */
> >>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> >>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> >>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
> >>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> >>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> >>>       void *p;
> >>>       const void *p_const;
> >>>  };
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index c3604a0a3e30..91096259e3ea 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> >>>
> >>>  /* last CID + 1 */
> >>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> >>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> >>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
> >>
> >> This patch needs to be split in three parts:
> >>
> >> 1) Add the new M2M_AUDIO control class,
> >> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> >> 3) Add the new controls.
> >>
> >> These are all independent changes, so separating them makes it easier to
> >> review.
> >>
> >>>
> >>>  /* USER-class private control IDs */
> >>>
> >>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> >>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> >>>  #endif
> >>>
> >>> +/**
> >>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> >>> + *
> >>> + * @rate_integer: integer part of fixed point value.
> >>> + * @rate_fractional: fractional part of fixed point value
> >>> + */
> >>> +struct v4l2_ctrl_fixed_point {
> >>> +     __u32 integer;
> >>
> >> __s32?
> >>
> >>> +     __u32 fractional;
> >>> +};
> >>> +
> >>>  #endif
> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>> index 2ac7b989394c..3ef32c09c2fa 100644
> >>> --- a/include/uapi/linux/videodev2.h
> >>> +++ b/include/uapi/linux/videodev2.h
> >>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> >>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> >>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> >>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> >>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> >>>               void __user *ptr;
> >>>       };
> >>>  } __attribute__ ((packed));
> >>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> >>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> >>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> >>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> >>> +
> >>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> >>>  };
> >>>
> >>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> >>
> >> Regards,
> >>
> >>         Hans
>
Shengjiu Wang Oct. 18, 2023, 7:23 a.m. UTC | #5
On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>
> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 17/10/2023 15:11, Shengjiu Wang wrote:
> > > On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > >>
> > >> Hi Shengjiu,
> > >>
> > >> On 13/10/2023 10:31, Shengjiu Wang wrote:
> > >>> Fixed point controls are used by the user to configure
> > >>> the audio sample rate to driver.
> > >>>
> > >>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> > >>> new IDs for ASRC rate control.
> > >>>
> > >>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > >>> ---
> > >>>  .../userspace-api/media/v4l/common.rst        |  1 +
> > >>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> > >>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> > >>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> > >>>  .../media/videodev2.h.rst.exceptions          |  1 +
> > >>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> > >>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> > >>>  include/media/v4l2-ctrls.h                    |  2 ++
> > >>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> > >>>  include/uapi/linux/videodev2.h                |  3 ++
> > >>>  10 files changed, 76 insertions(+)
> > >>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> > >>>
> > >>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> > >>> index ea0435182e44..35707edffb13 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/common.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> > >>> @@ -52,6 +52,7 @@ applicable to all devices.
> > >>>      ext-ctrls-fm-rx
> > >>>      ext-ctrls-detect
> > >>>      ext-ctrls-colorimetry
> > >>> +    ext-ctrls-fixed-point
> > >>
> > >> Rename this to ext-ctrls-audio-m2m.
> > >>
> > >>>      fourcc
> > >>>      format
> > >>>      planar-apis
> > >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> > >>> new file mode 100644
> > >>> index 000000000000..2ef6e250580c
> > >>> --- /dev/null
> > >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> > >>> @@ -0,0 +1,36 @@
> > >>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > >>> +
> > >>> +.. _fixed-point-controls:
> > >>> +
> > >>> +***************************
> > >>> +Fixed Point Control Reference
> > >>
> > >> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> > >> sense to group fixed point controls. But it does make sense to group the audio
> > >> controls.
> > >>
> > >> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> > >> into categories, and for each category there is also a control that gives a
> > >> description of the class (see 2.15.15 in
> > >> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> > >>
> > >> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> > >> they are grouped based on what class of control they are.
> > >>
> > >> So I think it would be a good idea to create a new control class for M2M audio controls,
> > >> instead of just adding them to the catch-all 'User Controls' class.
> > >>
> > >> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> > >> it is done.
> > >>
> > >> M2M_AUDIO would probably be a good name for the class.
> > >>
> > >>> +***************************
> > >>> +
> > >>> +These controls are intended to support an asynchronous sample
> > >>> +rate converter.
> > >>
> > >> Add ' (ASRC).' at the end to indicate the common abbreviation for
> > >> that.
> > >>
> > >>> +
> > >>> +.. _v4l2-audio-asrc:
> > >>> +
> > >>> +``V4L2_CID_ASRC_SOURCE_RATE``
> > >>> +    sets the resampler source rate.
> > >>> +
> > >>> +``V4L2_CID_ASRC_DEST_RATE``
> > >>> +    sets the resampler destination rate.
> > >>
> > >> Document the unit (Hz) for these two controls.
> > >>
> > >>> +
> > >>> +.. c:type:: v4l2_ctrl_fixed_point
> > >>> +
> > >>> +.. cssclass:: longtable
> > >>> +
> > >>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> > >>> +
> > >>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> > >>> +    :header-rows:  0
> > >>> +    :stub-columns: 0
> > >>> +    :widths:       1 1 2
> > >>> +
> > >>> +    * - __u32
> > >>
> > >> Hmm, shouldn't this be __s32?
> > >>
> > >>> +      - ``integer``
> > >>> +      - integer part of fixed point value.
> > >>> +    * - __s32
> > >>
> > >> and this __u32?
> > >>
> > >> You want to be able to use this generic type as a signed value.
> > >>
> > >>> +      - ``fractional``
> > >>> +      - fractional part of fixed point value, which is Q31.
> > >>> 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 f9f73530a6be..1811dabf5c74 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> > >>> @@ -295,6 +295,10 @@ still cause this situation.
> > >>>        - ``p_av1_film_grain``
> > >>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> > >>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> > >>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> > >>> +      - ``p_fixed_point``
> > >>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> > >>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> > >>>      * - void *
> > >>>        - ``ptr``
> > >>>        - A pointer to a compound type which can be an N-dimensional array
> > >>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > >>> index 4d38acafe8e1..9285f4f39eed 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> > >>> @@ -549,6 +549,13 @@ 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``
> > >>> +      - n/a
> > >>> +      - n/a
> > >>> +      - n/a
> > >>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> > >>> +        integer part and fractional part, i.e. audio sample rate.
> > >>> +
> > >>>
> > >>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > >>> index a662fb60f73f..7a616ac91059 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > >>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > >>>               if (!area->width || !area->height)
> > >>>                       return -EINVAL;
> > >>>               break;
> > >>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> > >>> +             break;
> > >>
> > >> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> > >>
> > >> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> > >>
> > >> since min and max values are perfectly fine for a fixed point value.
> > >>
> > >> Even a step value (currently not supported in that patch) would make sense.
> > >>
> > >> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> > >> why not represent the fixed point value as a Q31.32. Then the standard
> > >> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> > >>
> > >> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> > >> value.
> > >>
> > >> So in enum v4l2_ctrl_type add:
> > >>
> > >>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> > >>
> > >> (10, because it is no longer a compound type).
> > >
> > > Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> > >
> > > The reason I use the 'integer' and 'fractional' is that I want
> > > 'integer' to be the normal sample
> > > rate, for example 48kHz.  The 'fractional' is the difference with
> > > normal sample rate.
> > >
> > > For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> > >
> > > So if we use s64 for rate, then in driver need to convert the rate to
> > > the closed normal
> > > sample rate + fractional.
> >
> > That wasn't what the documentation said :-)
> >
> > So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> > means in this context) and the offset to the actual sample rate.
> >
> > Presumably the 'normal' sample rate is set once, while the offset changes
> > regularly.
> >
> > But why do you need the 'normal' sample rate? With audio resampling I assume
> > you resample from one rate to another, so why do you need a third 'normal'
> > rate?
> >
>
> 'Normal' rate is used to select the prefilter table.
>

Currently I think we may define
V4L2_CID_M2M_AUDIO_SOURCE_RATE
V4L2_CID_M2M_AUDIO_DEST_RATE
V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD

All of them can be V4L2_CTRL_TYPE_INTEGER.

RATIO_MOD was defined in the very beginning version.
I think it is better to let users calculate this value.

The reason is:
if we define the offset for source rate and dest rate in
driver separately,  when offset of source rate is set,
driver don't know if it needs to wait or not the dest rate
offset,  then go to calculate the ratio_mod.

best regards
wang shengjiu

> Best regards
> Wang Shengjiu
>
> > Regards,
> >
> >         Hans
> >
> > >
> > > best regards
> > > wang shengjiu
> > >
> > >>
> > >>>
> > >>>       default:
> > >>>               return -EINVAL;
> > >>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > >>>       case V4L2_CTRL_TYPE_AREA:
> > >>>               elem_size = sizeof(struct v4l2_area);
> > >>>               break;
> > >>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> > >>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> > >>> +             break;
> > >>>       default:
> > >>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
> > >>>                       elem_size = sizeof(s32);
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> index 8696eb1cdd61..d8f232df6b6a 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> > >>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> > >>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> > >>>               break;
> > >>> +     case V4L2_CID_ASRC_SOURCE_RATE:
> > >>> +     case V4L2_CID_ASRC_DEST_RATE:
> > >>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> > >>> +             break;
> > >>>       default:
> > >>>               *type = V4L2_CTRL_TYPE_INTEGER;
> > >>>               break;
> > >>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > >>> index 59679a42b3e7..645e4cccafc7 100644
> > >>> --- a/include/media/v4l2-ctrls.h
> > >>> +++ b/include/media/v4l2-ctrls.h
> > >>> @@ -56,6 +56,7 @@ struct video_device;
> > >>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> > >>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
> > >>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> > >>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> > >>>   * @p:                               Pointer to a compound value.
> > >>>   * @p_const:                 Pointer to a constant compound value.
> > >>>   */
> > >>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> > >>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> > >>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
> > >>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> > >>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> > >>>       void *p;
> > >>>       const void *p_const;
> > >>>  };
> > >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > >>> index c3604a0a3e30..91096259e3ea 100644
> > >>> --- a/include/uapi/linux/v4l2-controls.h
> > >>> +++ b/include/uapi/linux/v4l2-controls.h
> > >>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> > >>>
> > >>>  /* last CID + 1 */
> > >>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> > >>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> > >>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
> > >>
> > >> This patch needs to be split in three parts:
> > >>
> > >> 1) Add the new M2M_AUDIO control class,
> > >> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> > >> 3) Add the new controls.
> > >>
> > >> These are all independent changes, so separating them makes it easier to
> > >> review.
> > >>
> > >>>
> > >>>  /* USER-class private control IDs */
> > >>>
> > >>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> > >>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> > >>>  #endif
> > >>>
> > >>> +/**
> > >>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> > >>> + *
> > >>> + * @rate_integer: integer part of fixed point value.
> > >>> + * @rate_fractional: fractional part of fixed point value
> > >>> + */
> > >>> +struct v4l2_ctrl_fixed_point {
> > >>> +     __u32 integer;
> > >>
> > >> __s32?
> > >>
> > >>> +     __u32 fractional;
> > >>> +};
> > >>> +
> > >>>  #endif
> > >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > >>> index 2ac7b989394c..3ef32c09c2fa 100644
> > >>> --- a/include/uapi/linux/videodev2.h
> > >>> +++ b/include/uapi/linux/videodev2.h
> > >>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> > >>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> > >>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> > >>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> > >>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> > >>>               void __user *ptr;
> > >>>       };
> > >>>  } __attribute__ ((packed));
> > >>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> > >>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> > >>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> > >>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> > >>> +
> > >>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> > >>>  };
> > >>>
> > >>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> > >>
> > >> Regards,
> > >>
> > >>         Hans
> >
Hans Verkuil Oct. 18, 2023, 7:31 a.m. UTC | #6
On 18/10/2023 09:23, Shengjiu Wang wrote:
> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>>
>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>
>>>>> Hi Shengjiu,
>>>>>
>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>>>>> Fixed point controls are used by the user to configure
>>>>>> the audio sample rate to driver.
>>>>>>
>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>>>>> new IDs for ASRC rate control.
>>>>>>
>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>> ---
>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>>>>  10 files changed, 76 insertions(+)
>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>
>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>>>>> index ea0435182e44..35707edffb13 100644
>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>>>>      ext-ctrls-fm-rx
>>>>>>      ext-ctrls-detect
>>>>>>      ext-ctrls-colorimetry
>>>>>> +    ext-ctrls-fixed-point
>>>>>
>>>>> Rename this to ext-ctrls-audio-m2m.
>>>>>
>>>>>>      fourcc
>>>>>>      format
>>>>>>      planar-apis
>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>> new file mode 100644
>>>>>> index 000000000000..2ef6e250580c
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>> @@ -0,0 +1,36 @@
>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>>> +
>>>>>> +.. _fixed-point-controls:
>>>>>> +
>>>>>> +***************************
>>>>>> +Fixed Point Control Reference
>>>>>
>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>>>>> sense to group fixed point controls. But it does make sense to group the audio
>>>>> controls.
>>>>>
>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>>>>> into categories, and for each category there is also a control that gives a
>>>>> description of the class (see 2.15.15 in
>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>>>>
>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>>>>> they are grouped based on what class of control they are.
>>>>>
>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
>>>>> instead of just adding them to the catch-all 'User Controls' class.
>>>>>
>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>>>>> it is done.
>>>>>
>>>>> M2M_AUDIO would probably be a good name for the class.
>>>>>
>>>>>> +***************************
>>>>>> +
>>>>>> +These controls are intended to support an asynchronous sample
>>>>>> +rate converter.
>>>>>
>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>>>>> that.
>>>>>
>>>>>> +
>>>>>> +.. _v4l2-audio-asrc:
>>>>>> +
>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>>>>> +    sets the resampler source rate.
>>>>>> +
>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
>>>>>> +    sets the resampler destination rate.
>>>>>
>>>>> Document the unit (Hz) for these two controls.
>>>>>
>>>>>> +
>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
>>>>>> +
>>>>>> +.. cssclass:: longtable
>>>>>> +
>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>>>>> +
>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>>>>> +    :header-rows:  0
>>>>>> +    :stub-columns: 0
>>>>>> +    :widths:       1 1 2
>>>>>> +
>>>>>> +    * - __u32
>>>>>
>>>>> Hmm, shouldn't this be __s32?
>>>>>
>>>>>> +      - ``integer``
>>>>>> +      - integer part of fixed point value.
>>>>>> +    * - __s32
>>>>>
>>>>> and this __u32?
>>>>>
>>>>> You want to be able to use this generic type as a signed value.
>>>>>
>>>>>> +      - ``fractional``
>>>>>> +      - fractional part of fixed point value, which is Q31.
>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>>>>        - ``p_av1_film_grain``
>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>>>>> +      - ``p_fixed_point``
>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>>>>      * - void *
>>>>>>        - ``ptr``
>>>>>>        - A pointer to a compound type which can be an N-dimensional array
>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>> @@ -549,6 +549,13 @@ 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``
>>>>>> +      - n/a
>>>>>> +      - n/a
>>>>>> +      - n/a
>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>>>>> +        integer part and fractional part, i.e. audio sample rate.
>>>>>> +
>>>>>>
>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>> index a662fb60f73f..7a616ac91059 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>>>               if (!area->width || !area->height)
>>>>>>                       return -EINVAL;
>>>>>>               break;
>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>> +             break;
>>>>>
>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>>>>
>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>>>>
>>>>> since min and max values are perfectly fine for a fixed point value.
>>>>>
>>>>> Even a step value (currently not supported in that patch) would make sense.
>>>>>
>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>>>>> why not represent the fixed point value as a Q31.32. Then the standard
>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>>>>
>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>>>>> value.
>>>>>
>>>>> So in enum v4l2_ctrl_type add:
>>>>>
>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>>>>
>>>>> (10, because it is no longer a compound type).
>>>>
>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
>>>>
>>>> The reason I use the 'integer' and 'fractional' is that I want
>>>> 'integer' to be the normal sample
>>>> rate, for example 48kHz.  The 'fractional' is the difference with
>>>> normal sample rate.
>>>>
>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
>>>>
>>>> So if we use s64 for rate, then in driver need to convert the rate to
>>>> the closed normal
>>>> sample rate + fractional.
>>>
>>> That wasn't what the documentation said :-)
>>>
>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
>>> means in this context) and the offset to the actual sample rate.
>>>
>>> Presumably the 'normal' sample rate is set once, while the offset changes
>>> regularly.
>>>
>>> But why do you need the 'normal' sample rate? With audio resampling I assume
>>> you resample from one rate to another, so why do you need a third 'normal'
>>> rate?
>>>
>>
>> 'Normal' rate is used to select the prefilter table.
>>
> 
> Currently I think we may define
> V4L2_CID_M2M_AUDIO_SOURCE_RATE
> V4L2_CID_M2M_AUDIO_DEST_RATE

That makes sense.

> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD

OK, can you document this control? Just write it down in the reply, I just want
to understand how the integer value you set here is used.

Regards,

	Hans

> 
> All of them can be V4L2_CTRL_TYPE_INTEGER.
> 
> RATIO_MOD was defined in the very beginning version.
> I think it is better to let users calculate this value.
> 
> The reason is:
> if we define the offset for source rate and dest rate in
> driver separately,  when offset of source rate is set,
> driver don't know if it needs to wait or not the dest rate
> offset,  then go to calculate the ratio_mod.
> 
> best regards
> wang shengjiu
> 
>> Best regards
>> Wang Shengjiu
>>
>>> Regards,
>>>
>>>         Hans
>>>
>>>>
>>>> best regards
>>>> wang shengjiu
>>>>
>>>>>
>>>>>>
>>>>>>       default:
>>>>>>               return -EINVAL;
>>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>>>       case V4L2_CTRL_TYPE_AREA:
>>>>>>               elem_size = sizeof(struct v4l2_area);
>>>>>>               break;
>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
>>>>>> +             break;
>>>>>>       default:
>>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
>>>>>>                       elem_size = sizeof(s32);
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>>>>>>               break;
>>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
>>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
>>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
>>>>>> +             break;
>>>>>>       default:
>>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
>>>>>>               break;
>>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>>>>> index 59679a42b3e7..645e4cccafc7 100644
>>>>>> --- a/include/media/v4l2-ctrls.h
>>>>>> +++ b/include/media/v4l2-ctrls.h
>>>>>> @@ -56,6 +56,7 @@ struct video_device;
>>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
>>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
>>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
>>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
>>>>>>   * @p:                               Pointer to a compound value.
>>>>>>   * @p_const:                 Pointer to a constant compound value.
>>>>>>   */
>>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
>>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
>>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
>>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
>>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
>>>>>>       void *p;
>>>>>>       const void *p_const;
>>>>>>  };
>>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>>> index c3604a0a3e30..91096259e3ea 100644
>>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
>>>>>>
>>>>>>  /* last CID + 1 */
>>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
>>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
>>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
>>>>>
>>>>> This patch needs to be split in three parts:
>>>>>
>>>>> 1) Add the new M2M_AUDIO control class,
>>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
>>>>> 3) Add the new controls.
>>>>>
>>>>> These are all independent changes, so separating them makes it easier to
>>>>> review.
>>>>>
>>>>>>
>>>>>>  /* USER-class private control IDs */
>>>>>>
>>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
>>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
>>>>>>  #endif
>>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
>>>>>> + *
>>>>>> + * @rate_integer: integer part of fixed point value.
>>>>>> + * @rate_fractional: fractional part of fixed point value
>>>>>> + */
>>>>>> +struct v4l2_ctrl_fixed_point {
>>>>>> +     __u32 integer;
>>>>>
>>>>> __s32?
>>>>>
>>>>>> +     __u32 fractional;
>>>>>> +};
>>>>>> +
>>>>>>  #endif
>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
>>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
>>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
>>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
>>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
>>>>>>               void __user *ptr;
>>>>>>       };
>>>>>>  } __attribute__ ((packed));
>>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
>>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
>>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
>>>>>> +
>>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
>>>>>>  };
>>>>>>
>>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>>>>>
>>>>> Regards,
>>>>>
>>>>>         Hans
>>>
Shengjiu Wang Oct. 18, 2023, 7:40 a.m. UTC | #7
On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 18/10/2023 09:23, Shengjiu Wang wrote:
> > On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >>
> >> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>
> >>> On 17/10/2023 15:11, Shengjiu Wang wrote:
> >>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>
> >>>>> Hi Shengjiu,
> >>>>>
> >>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
> >>>>>> Fixed point controls are used by the user to configure
> >>>>>> the audio sample rate to driver.
> >>>>>>
> >>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> >>>>>> new IDs for ASRC rate control.
> >>>>>>
> >>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>> ---
> >>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
> >>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
> >>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >>>>>>  include/uapi/linux/videodev2.h                |  3 ++
> >>>>>>  10 files changed, 76 insertions(+)
> >>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>
> >>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>> index ea0435182e44..35707edffb13 100644
> >>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
> >>>>>>      ext-ctrls-fm-rx
> >>>>>>      ext-ctrls-detect
> >>>>>>      ext-ctrls-colorimetry
> >>>>>> +    ext-ctrls-fixed-point
> >>>>>
> >>>>> Rename this to ext-ctrls-audio-m2m.
> >>>>>
> >>>>>>      fourcc
> >>>>>>      format
> >>>>>>      planar-apis
> >>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..2ef6e250580c
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>> @@ -0,0 +1,36 @@
> >>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >>>>>> +
> >>>>>> +.. _fixed-point-controls:
> >>>>>> +
> >>>>>> +***************************
> >>>>>> +Fixed Point Control Reference
> >>>>>
> >>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> >>>>> sense to group fixed point controls. But it does make sense to group the audio
> >>>>> controls.
> >>>>>
> >>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> >>>>> into categories, and for each category there is also a control that gives a
> >>>>> description of the class (see 2.15.15 in
> >>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> >>>>>
> >>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> >>>>> they are grouped based on what class of control they are.
> >>>>>
> >>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
> >>>>> instead of just adding them to the catch-all 'User Controls' class.
> >>>>>
> >>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> >>>>> it is done.
> >>>>>
> >>>>> M2M_AUDIO would probably be a good name for the class.
> >>>>>
> >>>>>> +***************************
> >>>>>> +
> >>>>>> +These controls are intended to support an asynchronous sample
> >>>>>> +rate converter.
> >>>>>
> >>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
> >>>>> that.
> >>>>>
> >>>>>> +
> >>>>>> +.. _v4l2-audio-asrc:
> >>>>>> +
> >>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
> >>>>>> +    sets the resampler source rate.
> >>>>>> +
> >>>>>> +``V4L2_CID_ASRC_DEST_RATE``
> >>>>>> +    sets the resampler destination rate.
> >>>>>
> >>>>> Document the unit (Hz) for these two controls.
> >>>>>
> >>>>>> +
> >>>>>> +.. c:type:: v4l2_ctrl_fixed_point
> >>>>>> +
> >>>>>> +.. cssclass:: longtable
> >>>>>> +
> >>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> >>>>>> +
> >>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> >>>>>> +    :header-rows:  0
> >>>>>> +    :stub-columns: 0
> >>>>>> +    :widths:       1 1 2
> >>>>>> +
> >>>>>> +    * - __u32
> >>>>>
> >>>>> Hmm, shouldn't this be __s32?
> >>>>>
> >>>>>> +      - ``integer``
> >>>>>> +      - integer part of fixed point value.
> >>>>>> +    * - __s32
> >>>>>
> >>>>> and this __u32?
> >>>>>
> >>>>> You want to be able to use this generic type as a signed value.
> >>>>>
> >>>>>> +      - ``fractional``
> >>>>>> +      - fractional part of fixed point value, which is Q31.
> >>>>>> 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 f9f73530a6be..1811dabf5c74 100644
> >>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>> @@ -295,6 +295,10 @@ still cause this situation.
> >>>>>>        - ``p_av1_film_grain``
> >>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> >>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> >>>>>> +      - ``p_fixed_point``
> >>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> >>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >>>>>>      * - void *
> >>>>>>        - ``ptr``
> >>>>>>        - A pointer to a compound type which can be an N-dimensional array
> >>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>> index 4d38acafe8e1..9285f4f39eed 100644
> >>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>> @@ -549,6 +549,13 @@ 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``
> >>>>>> +      - n/a
> >>>>>> +      - n/a
> >>>>>> +      - n/a
> >>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> >>>>>> +        integer part and fractional part, i.e. audio sample rate.
> >>>>>> +
> >>>>>>
> >>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>> index a662fb60f73f..7a616ac91059 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>>>>               if (!area->width || !area->height)
> >>>>>>                       return -EINVAL;
> >>>>>>               break;
> >>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>> +             break;
> >>>>>
> >>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> >>>>>
> >>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> >>>>>
> >>>>> since min and max values are perfectly fine for a fixed point value.
> >>>>>
> >>>>> Even a step value (currently not supported in that patch) would make sense.
> >>>>>
> >>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> >>>>> why not represent the fixed point value as a Q31.32. Then the standard
> >>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> >>>>>
> >>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> >>>>> value.
> >>>>>
> >>>>> So in enum v4l2_ctrl_type add:
> >>>>>
> >>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> >>>>>
> >>>>> (10, because it is no longer a compound type).
> >>>>
> >>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> >>>>
> >>>> The reason I use the 'integer' and 'fractional' is that I want
> >>>> 'integer' to be the normal sample
> >>>> rate, for example 48kHz.  The 'fractional' is the difference with
> >>>> normal sample rate.
> >>>>
> >>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> >>>>
> >>>> So if we use s64 for rate, then in driver need to convert the rate to
> >>>> the closed normal
> >>>> sample rate + fractional.
> >>>
> >>> That wasn't what the documentation said :-)
> >>>
> >>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> >>> means in this context) and the offset to the actual sample rate.
> >>>
> >>> Presumably the 'normal' sample rate is set once, while the offset changes
> >>> regularly.
> >>>
> >>> But why do you need the 'normal' sample rate? With audio resampling I assume
> >>> you resample from one rate to another, so why do you need a third 'normal'
> >>> rate?
> >>>
> >>
> >> 'Normal' rate is used to select the prefilter table.
> >>
> >
> > Currently I think we may define
> > V4L2_CID_M2M_AUDIO_SOURCE_RATE
> > V4L2_CID_M2M_AUDIO_DEST_RATE
>
> That makes sense.
>
> > V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
>
> OK, can you document this control? Just write it down in the reply, I just want
> to understand how the integer value you set here is used.
>

It is Q31 value.   It is equal to:
in_rate_new / out_rate_new -  in_rate_old / out_rate_old

Best regards
Wang shengjiu

> Regards,
>
>         Hans
>
> >
> > All of them can be V4L2_CTRL_TYPE_INTEGER.
> >
> > RATIO_MOD was defined in the very beginning version.
> > I think it is better to let users calculate this value.
> >
> > The reason is:
> > if we define the offset for source rate and dest rate in
> > driver separately,  when offset of source rate is set,
> > driver don't know if it needs to wait or not the dest rate
> > offset,  then go to calculate the ratio_mod.
> >
> > best regards
> > wang shengjiu
> >
> >> Best regards
> >> Wang Shengjiu
> >>
> >>> Regards,
> >>>
> >>>         Hans
> >>>
> >>>>
> >>>> best regards
> >>>> wang shengjiu
> >>>>
> >>>>>
> >>>>>>
> >>>>>>       default:
> >>>>>>               return -EINVAL;
> >>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>>>>>       case V4L2_CTRL_TYPE_AREA:
> >>>>>>               elem_size = sizeof(struct v4l2_area);
> >>>>>>               break;
> >>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> >>>>>> +             break;
> >>>>>>       default:
> >>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >>>>>>                       elem_size = sizeof(s32);
> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >>>>>>               break;
> >>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
> >>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
> >>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> >>>>>> +             break;
> >>>>>>       default:
> >>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
> >>>>>>               break;
> >>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>>> index 59679a42b3e7..645e4cccafc7 100644
> >>>>>> --- a/include/media/v4l2-ctrls.h
> >>>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>>> @@ -56,6 +56,7 @@ struct video_device;
> >>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> >>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
> >>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> >>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> >>>>>>   * @p:                               Pointer to a compound value.
> >>>>>>   * @p_const:                 Pointer to a constant compound value.
> >>>>>>   */
> >>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> >>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> >>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
> >>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> >>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> >>>>>>       void *p;
> >>>>>>       const void *p_const;
> >>>>>>  };
> >>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>>>>> index c3604a0a3e30..91096259e3ea 100644
> >>>>>> --- a/include/uapi/linux/v4l2-controls.h
> >>>>>> +++ b/include/uapi/linux/v4l2-controls.h
> >>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> >>>>>>
> >>>>>>  /* last CID + 1 */
> >>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> >>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> >>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
> >>>>>
> >>>>> This patch needs to be split in three parts:
> >>>>>
> >>>>> 1) Add the new M2M_AUDIO control class,
> >>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> >>>>> 3) Add the new controls.
> >>>>>
> >>>>> These are all independent changes, so separating them makes it easier to
> >>>>> review.
> >>>>>
> >>>>>>
> >>>>>>  /* USER-class private control IDs */
> >>>>>>
> >>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> >>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> >>>>>>  #endif
> >>>>>>
> >>>>>> +/**
> >>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> >>>>>> + *
> >>>>>> + * @rate_integer: integer part of fixed point value.
> >>>>>> + * @rate_fractional: fractional part of fixed point value
> >>>>>> + */
> >>>>>> +struct v4l2_ctrl_fixed_point {
> >>>>>> +     __u32 integer;
> >>>>>
> >>>>> __s32?
> >>>>>
> >>>>>> +     __u32 fractional;
> >>>>>> +};
> >>>>>> +
> >>>>>>  #endif
> >>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
> >>>>>> --- a/include/uapi/linux/videodev2.h
> >>>>>> +++ b/include/uapi/linux/videodev2.h
> >>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> >>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> >>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> >>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> >>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> >>>>>>               void __user *ptr;
> >>>>>>       };
> >>>>>>  } __attribute__ ((packed));
> >>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> >>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> >>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> >>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> >>>>>> +
> >>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> >>>>>>  };
> >>>>>>
> >>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> >>>>>
> >>>>> Regards,
> >>>>>
> >>>>>         Hans
> >>>
>
Hans Verkuil Oct. 18, 2023, 7:58 a.m. UTC | #8
On 18/10/2023 09:40, Shengjiu Wang wrote:
> On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 18/10/2023 09:23, Shengjiu Wang wrote:
>>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>>>>
>>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>
>>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
>>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>
>>>>>>> Hi Shengjiu,
>>>>>>>
>>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>> the audio sample rate to driver.
>>>>>>>>
>>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>>>>>>> new IDs for ASRC rate control.
>>>>>>>>
>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>> ---
>>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>>>>>>  10 files changed, 76 insertions(+)
>>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>
>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>> index ea0435182e44..35707edffb13 100644
>>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>>>>>>      ext-ctrls-fm-rx
>>>>>>>>      ext-ctrls-detect
>>>>>>>>      ext-ctrls-colorimetry
>>>>>>>> +    ext-ctrls-fixed-point
>>>>>>>
>>>>>>> Rename this to ext-ctrls-audio-m2m.
>>>>>>>
>>>>>>>>      fourcc
>>>>>>>>      format
>>>>>>>>      planar-apis
>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..2ef6e250580c
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>>>>> +
>>>>>>>> +.. _fixed-point-controls:
>>>>>>>> +
>>>>>>>> +***************************
>>>>>>>> +Fixed Point Control Reference
>>>>>>>
>>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>>>>>>> sense to group fixed point controls. But it does make sense to group the audio
>>>>>>> controls.
>>>>>>>
>>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>>>>>>> into categories, and for each category there is also a control that gives a
>>>>>>> description of the class (see 2.15.15 in
>>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>>>>>>
>>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>>>>>>> they are grouped based on what class of control they are.
>>>>>>>
>>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
>>>>>>> instead of just adding them to the catch-all 'User Controls' class.
>>>>>>>
>>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>>>>>>> it is done.
>>>>>>>
>>>>>>> M2M_AUDIO would probably be a good name for the class.
>>>>>>>
>>>>>>>> +***************************
>>>>>>>> +
>>>>>>>> +These controls are intended to support an asynchronous sample
>>>>>>>> +rate converter.
>>>>>>>
>>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>>>>>>> that.
>>>>>>>
>>>>>>>> +
>>>>>>>> +.. _v4l2-audio-asrc:
>>>>>>>> +
>>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>>>>>>> +    sets the resampler source rate.
>>>>>>>> +
>>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
>>>>>>>> +    sets the resampler destination rate.
>>>>>>>
>>>>>>> Document the unit (Hz) for these two controls.
>>>>>>>
>>>>>>>> +
>>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
>>>>>>>> +
>>>>>>>> +.. cssclass:: longtable
>>>>>>>> +
>>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>>>>>>> +
>>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>>>>>>> +    :header-rows:  0
>>>>>>>> +    :stub-columns: 0
>>>>>>>> +    :widths:       1 1 2
>>>>>>>> +
>>>>>>>> +    * - __u32
>>>>>>>
>>>>>>> Hmm, shouldn't this be __s32?
>>>>>>>
>>>>>>>> +      - ``integer``
>>>>>>>> +      - integer part of fixed point value.
>>>>>>>> +    * - __s32
>>>>>>>
>>>>>>> and this __u32?
>>>>>>>
>>>>>>> You want to be able to use this generic type as a signed value.
>>>>>>>
>>>>>>>> +      - ``fractional``
>>>>>>>> +      - fractional part of fixed point value, which is Q31.
>>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>>>>>>        - ``p_av1_film_grain``
>>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>>>>>>> +      - ``p_fixed_point``
>>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>>>>>>      * - void *
>>>>>>>>        - ``ptr``
>>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>> @@ -549,6 +549,13 @@ 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``
>>>>>>>> +      - n/a
>>>>>>>> +      - n/a
>>>>>>>> +      - n/a
>>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
>>>>>>>> +
>>>>>>>>
>>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>> index a662fb60f73f..7a616ac91059 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>>>>>               if (!area->width || !area->height)
>>>>>>>>                       return -EINVAL;
>>>>>>>>               break;
>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>> +             break;
>>>>>>>
>>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>>>>>>
>>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>>>>>>
>>>>>>> since min and max values are perfectly fine for a fixed point value.
>>>>>>>
>>>>>>> Even a step value (currently not supported in that patch) would make sense.
>>>>>>>
>>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
>>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>>>>>>
>>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>>>>>>> value.
>>>>>>>
>>>>>>> So in enum v4l2_ctrl_type add:
>>>>>>>
>>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>>>>>>
>>>>>>> (10, because it is no longer a compound type).
>>>>>>
>>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
>>>>>>
>>>>>> The reason I use the 'integer' and 'fractional' is that I want
>>>>>> 'integer' to be the normal sample
>>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
>>>>>> normal sample rate.
>>>>>>
>>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
>>>>>>
>>>>>> So if we use s64 for rate, then in driver need to convert the rate to
>>>>>> the closed normal
>>>>>> sample rate + fractional.
>>>>>
>>>>> That wasn't what the documentation said :-)
>>>>>
>>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
>>>>> means in this context) and the offset to the actual sample rate.
>>>>>
>>>>> Presumably the 'normal' sample rate is set once, while the offset changes
>>>>> regularly.
>>>>>
>>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
>>>>> you resample from one rate to another, so why do you need a third 'normal'
>>>>> rate?
>>>>>
>>>>
>>>> 'Normal' rate is used to select the prefilter table.
>>>>
>>>
>>> Currently I think we may define
>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
>>> V4L2_CID_M2M_AUDIO_DEST_RATE
>>
>> That makes sense.
>>
>>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
>>
>> OK, can you document this control? Just write it down in the reply, I just want
>> to understand how the integer value you set here is used.
>>
> 
> It is Q31 value.   It is equal to:
> in_rate_new / out_rate_new -  in_rate_old / out_rate_old

So that's not an integer. Also, Q31 is limited to -1...1, and I think
that's too limiting.

For this having a Q31.32 fixed point type still makes a lot of sense.

I still feel this is a overly complicated API.

See more below...

> 
> Best regards
> Wang shengjiu
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>> All of them can be V4L2_CTRL_TYPE_INTEGER.
>>>
>>> RATIO_MOD was defined in the very beginning version.
>>> I think it is better to let users calculate this value.
>>>
>>> The reason is:
>>> if we define the offset for source rate and dest rate in
>>> driver separately,  when offset of source rate is set,
>>> driver don't know if it needs to wait or not the dest rate
>>> offset,  then go to calculate the ratio_mod.

Ah, in order to update the ratio mod userspace needs to set both source and
dest rate at the same time to avoid race conditions.

That is perfectly possible in the V4L2 control framework. See:

https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters

In practice, isn't it likely that you would fix either the source or
destination rate, and let the other rate fluctuate? It kind of feels weird
to me that both source AND destination rates can fluctuate over time.

In any case, with a control cluster it doesn't really matter, you can set
one rate or both rates, and it will be handled atomically.

I feel that the RATIO_MOD control is too hardware specific. This is something
that should be hidden in the driver.

Regards,

	Hans

>>>
>>> best regards
>>> wang shengjiu
>>>
>>>> Best regards
>>>> Wang Shengjiu
>>>>
>>>>> Regards,
>>>>>
>>>>>         Hans
>>>>>
>>>>>>
>>>>>> best regards
>>>>>> wang shengjiu
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>       default:
>>>>>>>>               return -EINVAL;
>>>>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>>>>>       case V4L2_CTRL_TYPE_AREA:
>>>>>>>>               elem_size = sizeof(struct v4l2_area);
>>>>>>>>               break;
>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
>>>>>>>> +             break;
>>>>>>>>       default:
>>>>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
>>>>>>>>                       elem_size = sizeof(s32);
>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>>>>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>>>>>>>>               break;
>>>>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
>>>>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
>>>>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
>>>>>>>> +             break;
>>>>>>>>       default:
>>>>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
>>>>>>>>               break;
>>>>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>>>>>>> index 59679a42b3e7..645e4cccafc7 100644
>>>>>>>> --- a/include/media/v4l2-ctrls.h
>>>>>>>> +++ b/include/media/v4l2-ctrls.h
>>>>>>>> @@ -56,6 +56,7 @@ struct video_device;
>>>>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
>>>>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
>>>>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
>>>>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
>>>>>>>>   * @p:                               Pointer to a compound value.
>>>>>>>>   * @p_const:                 Pointer to a constant compound value.
>>>>>>>>   */
>>>>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
>>>>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
>>>>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
>>>>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
>>>>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
>>>>>>>>       void *p;
>>>>>>>>       const void *p_const;
>>>>>>>>  };
>>>>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>>>>> index c3604a0a3e30..91096259e3ea 100644
>>>>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
>>>>>>>>
>>>>>>>>  /* last CID + 1 */
>>>>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
>>>>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
>>>>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
>>>>>>>
>>>>>>> This patch needs to be split in three parts:
>>>>>>>
>>>>>>> 1) Add the new M2M_AUDIO control class,
>>>>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
>>>>>>> 3) Add the new controls.
>>>>>>>
>>>>>>> These are all independent changes, so separating them makes it easier to
>>>>>>> review.
>>>>>>>
>>>>>>>>
>>>>>>>>  /* USER-class private control IDs */
>>>>>>>>
>>>>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
>>>>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
>>>>>>>>  #endif
>>>>>>>>
>>>>>>>> +/**
>>>>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
>>>>>>>> + *
>>>>>>>> + * @rate_integer: integer part of fixed point value.
>>>>>>>> + * @rate_fractional: fractional part of fixed point value
>>>>>>>> + */
>>>>>>>> +struct v4l2_ctrl_fixed_point {
>>>>>>>> +     __u32 integer;
>>>>>>>
>>>>>>> __s32?
>>>>>>>
>>>>>>>> +     __u32 fractional;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  #endif
>>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
>>>>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
>>>>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
>>>>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
>>>>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
>>>>>>>>               void __user *ptr;
>>>>>>>>       };
>>>>>>>>  } __attribute__ ((packed));
>>>>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
>>>>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>>>>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
>>>>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
>>>>>>>> +
>>>>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>>         Hans
>>>>>
>>
Shengjiu Wang Oct. 18, 2023, 12:52 p.m. UTC | #9
On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 18/10/2023 09:40, Shengjiu Wang wrote:
> > On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 18/10/2023 09:23, Shengjiu Wang wrote:
> >>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >>>>
> >>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>
> >>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
> >>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>>>
> >>>>>>> Hi Shengjiu,
> >>>>>>>
> >>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
> >>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>> the audio sample rate to driver.
> >>>>>>>>
> >>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> >>>>>>>> new IDs for ASRC rate control.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>> ---
> >>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
> >>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
> >>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
> >>>>>>>>  10 files changed, 76 insertions(+)
> >>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> index ea0435182e44..35707edffb13 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
> >>>>>>>>      ext-ctrls-fm-rx
> >>>>>>>>      ext-ctrls-detect
> >>>>>>>>      ext-ctrls-colorimetry
> >>>>>>>> +    ext-ctrls-fixed-point
> >>>>>>>
> >>>>>>> Rename this to ext-ctrls-audio-m2m.
> >>>>>>>
> >>>>>>>>      fourcc
> >>>>>>>>      format
> >>>>>>>>      planar-apis
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..2ef6e250580c
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>> @@ -0,0 +1,36 @@
> >>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >>>>>>>> +
> >>>>>>>> +.. _fixed-point-controls:
> >>>>>>>> +
> >>>>>>>> +***************************
> >>>>>>>> +Fixed Point Control Reference
> >>>>>>>
> >>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> >>>>>>> sense to group fixed point controls. But it does make sense to group the audio
> >>>>>>> controls.
> >>>>>>>
> >>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> >>>>>>> into categories, and for each category there is also a control that gives a
> >>>>>>> description of the class (see 2.15.15 in
> >>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> >>>>>>>
> >>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> >>>>>>> they are grouped based on what class of control they are.
> >>>>>>>
> >>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
> >>>>>>> instead of just adding them to the catch-all 'User Controls' class.
> >>>>>>>
> >>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> >>>>>>> it is done.
> >>>>>>>
> >>>>>>> M2M_AUDIO would probably be a good name for the class.
> >>>>>>>
> >>>>>>>> +***************************
> >>>>>>>> +
> >>>>>>>> +These controls are intended to support an asynchronous sample
> >>>>>>>> +rate converter.
> >>>>>>>
> >>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
> >>>>>>> that.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +.. _v4l2-audio-asrc:
> >>>>>>>> +
> >>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
> >>>>>>>> +    sets the resampler source rate.
> >>>>>>>> +
> >>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
> >>>>>>>> +    sets the resampler destination rate.
> >>>>>>>
> >>>>>>> Document the unit (Hz) for these two controls.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
> >>>>>>>> +
> >>>>>>>> +.. cssclass:: longtable
> >>>>>>>> +
> >>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> >>>>>>>> +
> >>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> >>>>>>>> +    :header-rows:  0
> >>>>>>>> +    :stub-columns: 0
> >>>>>>>> +    :widths:       1 1 2
> >>>>>>>> +
> >>>>>>>> +    * - __u32
> >>>>>>>
> >>>>>>> Hmm, shouldn't this be __s32?
> >>>>>>>
> >>>>>>>> +      - ``integer``
> >>>>>>>> +      - integer part of fixed point value.
> >>>>>>>> +    * - __s32
> >>>>>>>
> >>>>>>> and this __u32?
> >>>>>>>
> >>>>>>> You want to be able to use this generic type as a signed value.
> >>>>>>>
> >>>>>>>> +      - ``fractional``
> >>>>>>>> +      - fractional part of fixed point value, which is Q31.
> >>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
> >>>>>>>>        - ``p_av1_film_grain``
> >>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> >>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> >>>>>>>> +      - ``p_fixed_point``
> >>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> >>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >>>>>>>>      * - void *
> >>>>>>>>        - ``ptr``
> >>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> @@ -549,6 +549,13 @@ 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``
> >>>>>>>> +      - n/a
> >>>>>>>> +      - n/a
> >>>>>>>> +      - n/a
> >>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> >>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
> >>>>>>>> +
> >>>>>>>>
> >>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> index a662fb60f73f..7a616ac91059 100644
> >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>>>>>>               if (!area->width || !area->height)
> >>>>>>>>                       return -EINVAL;
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>>>> +             break;
> >>>>>>>
> >>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> >>>>>>>
> >>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> >>>>>>>
> >>>>>>> since min and max values are perfectly fine for a fixed point value.
> >>>>>>>
> >>>>>>> Even a step value (currently not supported in that patch) would make sense.
> >>>>>>>
> >>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> >>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
> >>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> >>>>>>>
> >>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> >>>>>>> value.
> >>>>>>>
> >>>>>>> So in enum v4l2_ctrl_type add:
> >>>>>>>
> >>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> >>>>>>>
> >>>>>>> (10, because it is no longer a compound type).
> >>>>>>
> >>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> >>>>>>
> >>>>>> The reason I use the 'integer' and 'fractional' is that I want
> >>>>>> 'integer' to be the normal sample
> >>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
> >>>>>> normal sample rate.
> >>>>>>
> >>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> >>>>>>
> >>>>>> So if we use s64 for rate, then in driver need to convert the rate to
> >>>>>> the closed normal
> >>>>>> sample rate + fractional.
> >>>>>
> >>>>> That wasn't what the documentation said :-)
> >>>>>
> >>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> >>>>> means in this context) and the offset to the actual sample rate.
> >>>>>
> >>>>> Presumably the 'normal' sample rate is set once, while the offset changes
> >>>>> regularly.
> >>>>>
> >>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
> >>>>> you resample from one rate to another, so why do you need a third 'normal'
> >>>>> rate?
> >>>>>
> >>>>
> >>>> 'Normal' rate is used to select the prefilter table.
> >>>>
> >>>
> >>> Currently I think we may define
> >>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
> >>> V4L2_CID_M2M_AUDIO_DEST_RATE
> >>
> >> That makes sense.
> >>
> >>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
> >>
> >> OK, can you document this control? Just write it down in the reply, I just want
> >> to understand how the integer value you set here is used.
> >>
> >
> > It is Q31 value.   It is equal to:
> > in_rate_new / out_rate_new -  in_rate_old / out_rate_old
>
> So that's not an integer. Also, Q31 is limited to -1...1, and I think
> that's too limiting.
>
> For this having a Q31.32 fixed point type still makes a lot of sense.
>
> I still feel this is a overly complicated API.
>
> See more below...
>
> >
> > Best regards
> > Wang shengjiu
> >
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> All of them can be V4L2_CTRL_TYPE_INTEGER.
> >>>
> >>> RATIO_MOD was defined in the very beginning version.
> >>> I think it is better to let users calculate this value.
> >>>
> >>> The reason is:
> >>> if we define the offset for source rate and dest rate in
> >>> driver separately,  when offset of source rate is set,
> >>> driver don't know if it needs to wait or not the dest rate
> >>> offset,  then go to calculate the ratio_mod.
>
> Ah, in order to update the ratio mod userspace needs to set both source and
> dest rate at the same time to avoid race conditions.
>
> That is perfectly possible in the V4L2 control framework. See:
>
> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
>
> In practice, isn't it likely that you would fix either the source or
> destination rate, and let the other rate fluctuate? It kind of feels weird
> to me that both source AND destination rates can fluctuate over time.
>
Right, the source and dest rates needn't change in same time.

> In any case, with a control cluster it doesn't really matter, you can set
> one rate or both rates, and it will be handled atomically.
>
> I feel that the RATIO_MOD control is too hardware specific. This is something
> that should be hidden in the driver.
>

I will use:

V4L2_CID_M2M_AUDIO_SOURCE_RATE
V4L2_CID_M2M_AUDIO_DEST_RATE
V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET
V4L2_CID_M2M_AUDIO_DEST_RATE_OFFSET

'OFFSET' is V4L2_CTRL_TYPE_FIXED_POINT, which is Q31.32.

best regards
wang shengjiu

> Regards,
>
>         Hans
>
> >>>
> >>> best regards
> >>> wang shengjiu
> >>>
> >>>> Best regards
> >>>> Wang Shengjiu
> >>>>
> >>>>> Regards,
> >>>>>
> >>>>>         Hans
> >>>>>
> >>>>>>
> >>>>>> best regards
> >>>>>> wang shengjiu
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>>       default:
> >>>>>>>>               return -EINVAL;
> >>>>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>>>>>>>       case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>               elem_size = sizeof(struct v4l2_area);
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> >>>>>>>> +             break;
> >>>>>>>>       default:
> >>>>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >>>>>>>>                       elem_size = sizeof(s32);
> >>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
> >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >>>>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
> >>>>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
> >>>>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> >>>>>>>> +             break;
> >>>>>>>>       default:
> >>>>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
> >>>>>>>>               break;
> >>>>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>>>>> index 59679a42b3e7..645e4cccafc7 100644
> >>>>>>>> --- a/include/media/v4l2-ctrls.h
> >>>>>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>>>>> @@ -56,6 +56,7 @@ struct video_device;
> >>>>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> >>>>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
> >>>>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> >>>>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> >>>>>>>>   * @p:                               Pointer to a compound value.
> >>>>>>>>   * @p_const:                 Pointer to a constant compound value.
> >>>>>>>>   */
> >>>>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> >>>>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> >>>>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
> >>>>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> >>>>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> >>>>>>>>       void *p;
> >>>>>>>>       const void *p_const;
> >>>>>>>>  };
> >>>>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>>>>>>> index c3604a0a3e30..91096259e3ea 100644
> >>>>>>>> --- a/include/uapi/linux/v4l2-controls.h
> >>>>>>>> +++ b/include/uapi/linux/v4l2-controls.h
> >>>>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> >>>>>>>>
> >>>>>>>>  /* last CID + 1 */
> >>>>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> >>>>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> >>>>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
> >>>>>>>
> >>>>>>> This patch needs to be split in three parts:
> >>>>>>>
> >>>>>>> 1) Add the new M2M_AUDIO control class,
> >>>>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> >>>>>>> 3) Add the new controls.
> >>>>>>>
> >>>>>>> These are all independent changes, so separating them makes it easier to
> >>>>>>> review.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>  /* USER-class private control IDs */
> >>>>>>>>
> >>>>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> >>>>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> >>>>>>>>  #endif
> >>>>>>>>
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> >>>>>>>> + *
> >>>>>>>> + * @rate_integer: integer part of fixed point value.
> >>>>>>>> + * @rate_fractional: fractional part of fixed point value
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_ctrl_fixed_point {
> >>>>>>>> +     __u32 integer;
> >>>>>>>
> >>>>>>> __s32?
> >>>>>>>
> >>>>>>>> +     __u32 fractional;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>  #endif
> >>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>>>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
> >>>>>>>> --- a/include/uapi/linux/videodev2.h
> >>>>>>>> +++ b/include/uapi/linux/videodev2.h
> >>>>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> >>>>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> >>>>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> >>>>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> >>>>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> >>>>>>>>               void __user *ptr;
> >>>>>>>>       };
> >>>>>>>>  } __attribute__ ((packed));
> >>>>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> >>>>>>>> +
> >>>>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> >>>>>>>>  };
> >>>>>>>>
> >>>>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>>         Hans
> >>>>>
> >>
>
Hans Verkuil Oct. 18, 2023, 1:09 p.m. UTC | #10
On 18/10/2023 14:52, Shengjiu Wang wrote:
> On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 18/10/2023 09:40, Shengjiu Wang wrote:
>>> On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 18/10/2023 09:23, Shengjiu Wang wrote:
>>>>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>
>>>>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
>>>>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>>>
>>>>>>>>> Hi Shengjiu,
>>>>>>>>>
>>>>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>> the audio sample rate to driver.
>>>>>>>>>>
>>>>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>>>>>>>>> new IDs for ASRC rate control.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>>>>>>>>  10 files changed, 76 insertions(+)
>>>>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> index ea0435182e44..35707edffb13 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>>>>>>>>      ext-ctrls-fm-rx
>>>>>>>>>>      ext-ctrls-detect
>>>>>>>>>>      ext-ctrls-colorimetry
>>>>>>>>>> +    ext-ctrls-fixed-point
>>>>>>>>>
>>>>>>>>> Rename this to ext-ctrls-audio-m2m.
>>>>>>>>>
>>>>>>>>>>      fourcc
>>>>>>>>>>      format
>>>>>>>>>>      planar-apis
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..2ef6e250580c
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>>>>>>> +
>>>>>>>>>> +.. _fixed-point-controls:
>>>>>>>>>> +
>>>>>>>>>> +***************************
>>>>>>>>>> +Fixed Point Control Reference
>>>>>>>>>
>>>>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>>>>>>>>> sense to group fixed point controls. But it does make sense to group the audio
>>>>>>>>> controls.
>>>>>>>>>
>>>>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>>>>>>>>> into categories, and for each category there is also a control that gives a
>>>>>>>>> description of the class (see 2.15.15 in
>>>>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>>>>>>>>
>>>>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>>>>>>>>> they are grouped based on what class of control they are.
>>>>>>>>>
>>>>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
>>>>>>>>> instead of just adding them to the catch-all 'User Controls' class.
>>>>>>>>>
>>>>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>>>>>>>>> it is done.
>>>>>>>>>
>>>>>>>>> M2M_AUDIO would probably be a good name for the class.
>>>>>>>>>
>>>>>>>>>> +***************************
>>>>>>>>>> +
>>>>>>>>>> +These controls are intended to support an asynchronous sample
>>>>>>>>>> +rate converter.
>>>>>>>>>
>>>>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>>>>>>>>> that.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +.. _v4l2-audio-asrc:
>>>>>>>>>> +
>>>>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>>>>>>>>> +    sets the resampler source rate.
>>>>>>>>>> +
>>>>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
>>>>>>>>>> +    sets the resampler destination rate.
>>>>>>>>>
>>>>>>>>> Document the unit (Hz) for these two controls.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
>>>>>>>>>> +
>>>>>>>>>> +.. cssclass:: longtable
>>>>>>>>>> +
>>>>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>>>>>>>>> +
>>>>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>>>>>>>>> +    :header-rows:  0
>>>>>>>>>> +    :stub-columns: 0
>>>>>>>>>> +    :widths:       1 1 2
>>>>>>>>>> +
>>>>>>>>>> +    * - __u32
>>>>>>>>>
>>>>>>>>> Hmm, shouldn't this be __s32?
>>>>>>>>>
>>>>>>>>>> +      - ``integer``
>>>>>>>>>> +      - integer part of fixed point value.
>>>>>>>>>> +    * - __s32
>>>>>>>>>
>>>>>>>>> and this __u32?
>>>>>>>>>
>>>>>>>>> You want to be able to use this generic type as a signed value.
>>>>>>>>>
>>>>>>>>>> +      - ``fractional``
>>>>>>>>>> +      - fractional part of fixed point value, which is Q31.
>>>>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>>>>>>>>        - ``p_av1_film_grain``
>>>>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>>>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>>>>>>>>> +      - ``p_fixed_point``
>>>>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>>>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>>>>>>>>      * - void *
>>>>>>>>>>        - ``ptr``
>>>>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> @@ -549,6 +549,13 @@ 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``
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>>>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> index a662fb60f73f..7a616ac91059 100644
>>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>>>>>>>               if (!area->width || !area->height)
>>>>>>>>>>                       return -EINVAL;
>>>>>>>>>>               break;
>>>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>>>> +             break;
>>>>>>>>>
>>>>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>>>>>>>>
>>>>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>>>>>>>>
>>>>>>>>> since min and max values are perfectly fine for a fixed point value.
>>>>>>>>>
>>>>>>>>> Even a step value (currently not supported in that patch) would make sense.
>>>>>>>>>
>>>>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>>>>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
>>>>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>>>>>>>>
>>>>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> So in enum v4l2_ctrl_type add:
>>>>>>>>>
>>>>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>>>>>>>>
>>>>>>>>> (10, because it is no longer a compound type).
>>>>>>>>
>>>>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
>>>>>>>>
>>>>>>>> The reason I use the 'integer' and 'fractional' is that I want
>>>>>>>> 'integer' to be the normal sample
>>>>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
>>>>>>>> normal sample rate.
>>>>>>>>
>>>>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
>>>>>>>>
>>>>>>>> So if we use s64 for rate, then in driver need to convert the rate to
>>>>>>>> the closed normal
>>>>>>>> sample rate + fractional.
>>>>>>>
>>>>>>> That wasn't what the documentation said :-)
>>>>>>>
>>>>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
>>>>>>> means in this context) and the offset to the actual sample rate.
>>>>>>>
>>>>>>> Presumably the 'normal' sample rate is set once, while the offset changes
>>>>>>> regularly.
>>>>>>>
>>>>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
>>>>>>> you resample from one rate to another, so why do you need a third 'normal'
>>>>>>> rate?
>>>>>>>
>>>>>>
>>>>>> 'Normal' rate is used to select the prefilter table.
>>>>>>
>>>>>
>>>>> Currently I think we may define
>>>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
>>>>> V4L2_CID_M2M_AUDIO_DEST_RATE
>>>>
>>>> That makes sense.
>>>>
>>>>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
>>>>
>>>> OK, can you document this control? Just write it down in the reply, I just want
>>>> to understand how the integer value you set here is used.
>>>>
>>>
>>> It is Q31 value.   It is equal to:
>>> in_rate_new / out_rate_new -  in_rate_old / out_rate_old
>>
>> So that's not an integer. Also, Q31 is limited to -1...1, and I think
>> that's too limiting.
>>
>> For this having a Q31.32 fixed point type still makes a lot of sense.
>>
>> I still feel this is a overly complicated API.
>>
>> See more below...
>>
>>>
>>> Best regards
>>> Wang shengjiu
>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>
>>>>> All of them can be V4L2_CTRL_TYPE_INTEGER.
>>>>>
>>>>> RATIO_MOD was defined in the very beginning version.
>>>>> I think it is better to let users calculate this value.
>>>>>
>>>>> The reason is:
>>>>> if we define the offset for source rate and dest rate in
>>>>> driver separately,  when offset of source rate is set,
>>>>> driver don't know if it needs to wait or not the dest rate
>>>>> offset,  then go to calculate the ratio_mod.
>>
>> Ah, in order to update the ratio mod userspace needs to set both source and
>> dest rate at the same time to avoid race conditions.
>>
>> That is perfectly possible in the V4L2 control framework. See:
>>
>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
>>
>> In practice, isn't it likely that you would fix either the source or
>> destination rate, and let the other rate fluctuate? It kind of feels weird
>> to me that both source AND destination rates can fluctuate over time.
>>
> Right, the source and dest rates needn't change in same time.
> 
>> In any case, with a control cluster it doesn't really matter, you can set
>> one rate or both rates, and it will be handled atomically.
>>
>> I feel that the RATIO_MOD control is too hardware specific. This is something
>> that should be hidden in the driver.
>>
> 
> I will use:
> 
> V4L2_CID_M2M_AUDIO_SOURCE_RATE
> V4L2_CID_M2M_AUDIO_DEST_RATE
> V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET
> V4L2_CID_M2M_AUDIO_DEST_RATE_OFFSET
> 
> 'OFFSET' is V4L2_CTRL_TYPE_FIXED_POINT, which is Q31.32.

So now I come back to my original question: why do you need both
the rate and the offset? Isn't it enough to set just the rates,
as long as that is in fixed point format?

Why does the driver need both the 'ideal' rate + the offset?

I'm not opposed to this, I'm just trying to understand whether this
makes sense.

Can't you take e.g. the source and dest rate as starting points
when you start streaming? And every time userspace updates one or both
of these rates you calculate the ratio_mod compared to the previous rates?

Or is there a reason why you need the ideal rates as well? E.g. 48000 or
44100, etc.

Regards,

	Hans
Shengjiu Wang Oct. 18, 2023, 1:50 p.m. UTC | #11
On Wed, Oct 18, 2023 at 9:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 18/10/2023 14:52, Shengjiu Wang wrote:
> > On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 18/10/2023 09:40, Shengjiu Wang wrote:
> >>> On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 18/10/2023 09:23, Shengjiu Wang wrote:
> >>>>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >>>>>>
> >>>>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>>>
> >>>>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
> >>>>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Shengjiu,
> >>>>>>>>>
> >>>>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
> >>>>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>>>> the audio sample rate to driver.
> >>>>>>>>>>
> >>>>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> >>>>>>>>>> new IDs for ASRC rate control.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>>>> ---
> >>>>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
> >>>>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >>>>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >>>>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >>>>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >>>>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
> >>>>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >>>>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
> >>>>>>>>>>  10 files changed, 76 insertions(+)
> >>>>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>>>> index ea0435182e44..35707edffb13 100644
> >>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
> >>>>>>>>>>      ext-ctrls-fm-rx
> >>>>>>>>>>      ext-ctrls-detect
> >>>>>>>>>>      ext-ctrls-colorimetry
> >>>>>>>>>> +    ext-ctrls-fixed-point
> >>>>>>>>>
> >>>>>>>>> Rename this to ext-ctrls-audio-m2m.
> >>>>>>>>>
> >>>>>>>>>>      fourcc
> >>>>>>>>>>      format
> >>>>>>>>>>      planar-apis
> >>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>>>> new file mode 100644
> >>>>>>>>>> index 000000000000..2ef6e250580c
> >>>>>>>>>> --- /dev/null
> >>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>>>> @@ -0,0 +1,36 @@
> >>>>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >>>>>>>>>> +
> >>>>>>>>>> +.. _fixed-point-controls:
> >>>>>>>>>> +
> >>>>>>>>>> +***************************
> >>>>>>>>>> +Fixed Point Control Reference
> >>>>>>>>>
> >>>>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> >>>>>>>>> sense to group fixed point controls. But it does make sense to group the audio
> >>>>>>>>> controls.
> >>>>>>>>>
> >>>>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> >>>>>>>>> into categories, and for each category there is also a control that gives a
> >>>>>>>>> description of the class (see 2.15.15 in
> >>>>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> >>>>>>>>>
> >>>>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> >>>>>>>>> they are grouped based on what class of control they are.
> >>>>>>>>>
> >>>>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
> >>>>>>>>> instead of just adding them to the catch-all 'User Controls' class.
> >>>>>>>>>
> >>>>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> >>>>>>>>> it is done.
> >>>>>>>>>
> >>>>>>>>> M2M_AUDIO would probably be a good name for the class.
> >>>>>>>>>
> >>>>>>>>>> +***************************
> >>>>>>>>>> +
> >>>>>>>>>> +These controls are intended to support an asynchronous sample
> >>>>>>>>>> +rate converter.
> >>>>>>>>>
> >>>>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
> >>>>>>>>> that.
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +.. _v4l2-audio-asrc:
> >>>>>>>>>> +
> >>>>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
> >>>>>>>>>> +    sets the resampler source rate.
> >>>>>>>>>> +
> >>>>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
> >>>>>>>>>> +    sets the resampler destination rate.
> >>>>>>>>>
> >>>>>>>>> Document the unit (Hz) for these two controls.
> >>>>>>>>>
> >>>>>>>>>> +
> >>>>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
> >>>>>>>>>> +
> >>>>>>>>>> +.. cssclass:: longtable
> >>>>>>>>>> +
> >>>>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> >>>>>>>>>> +
> >>>>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> >>>>>>>>>> +    :header-rows:  0
> >>>>>>>>>> +    :stub-columns: 0
> >>>>>>>>>> +    :widths:       1 1 2
> >>>>>>>>>> +
> >>>>>>>>>> +    * - __u32
> >>>>>>>>>
> >>>>>>>>> Hmm, shouldn't this be __s32?
> >>>>>>>>>
> >>>>>>>>>> +      - ``integer``
> >>>>>>>>>> +      - integer part of fixed point value.
> >>>>>>>>>> +    * - __s32
> >>>>>>>>>
> >>>>>>>>> and this __u32?
> >>>>>>>>>
> >>>>>>>>> You want to be able to use this generic type as a signed value.
> >>>>>>>>>
> >>>>>>>>>> +      - ``fractional``
> >>>>>>>>>> +      - fractional part of fixed point value, which is Q31.
> >>>>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
> >>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
> >>>>>>>>>>        - ``p_av1_film_grain``
> >>>>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >>>>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> >>>>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> >>>>>>>>>> +      - ``p_fixed_point``
> >>>>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> >>>>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >>>>>>>>>>      * - void *
> >>>>>>>>>>        - ``ptr``
> >>>>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
> >>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
> >>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>>>> @@ -549,6 +549,13 @@ 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``
> >>>>>>>>>> +      - n/a
> >>>>>>>>>> +      - n/a
> >>>>>>>>>> +      - n/a
> >>>>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> >>>>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
> >>>>>>>>>> +
> >>>>>>>>>>
> >>>>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>>>> index a662fb60f73f..7a616ac91059 100644
> >>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>>>>>>>>               if (!area->width || !area->height)
> >>>>>>>>>>                       return -EINVAL;
> >>>>>>>>>>               break;
> >>>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>>>>>> +             break;
> >>>>>>>>>
> >>>>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> >>>>>>>>>
> >>>>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> >>>>>>>>>
> >>>>>>>>> since min and max values are perfectly fine for a fixed point value.
> >>>>>>>>>
> >>>>>>>>> Even a step value (currently not supported in that patch) would make sense.
> >>>>>>>>>
> >>>>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> >>>>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
> >>>>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> >>>>>>>>>
> >>>>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> >>>>>>>>> value.
> >>>>>>>>>
> >>>>>>>>> So in enum v4l2_ctrl_type add:
> >>>>>>>>>
> >>>>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> >>>>>>>>>
> >>>>>>>>> (10, because it is no longer a compound type).
> >>>>>>>>
> >>>>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> >>>>>>>>
> >>>>>>>> The reason I use the 'integer' and 'fractional' is that I want
> >>>>>>>> 'integer' to be the normal sample
> >>>>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
> >>>>>>>> normal sample rate.
> >>>>>>>>
> >>>>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> >>>>>>>>
> >>>>>>>> So if we use s64 for rate, then in driver need to convert the rate to
> >>>>>>>> the closed normal
> >>>>>>>> sample rate + fractional.
> >>>>>>>
> >>>>>>> That wasn't what the documentation said :-)
> >>>>>>>
> >>>>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> >>>>>>> means in this context) and the offset to the actual sample rate.
> >>>>>>>
> >>>>>>> Presumably the 'normal' sample rate is set once, while the offset changes
> >>>>>>> regularly.
> >>>>>>>
> >>>>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
> >>>>>>> you resample from one rate to another, so why do you need a third 'normal'
> >>>>>>> rate?
> >>>>>>>
> >>>>>>
> >>>>>> 'Normal' rate is used to select the prefilter table.
> >>>>>>
> >>>>>
> >>>>> Currently I think we may define
> >>>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
> >>>>> V4L2_CID_M2M_AUDIO_DEST_RATE
> >>>>
> >>>> That makes sense.
> >>>>
> >>>>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
> >>>>
> >>>> OK, can you document this control? Just write it down in the reply, I just want
> >>>> to understand how the integer value you set here is used.
> >>>>
> >>>
> >>> It is Q31 value.   It is equal to:
> >>> in_rate_new / out_rate_new -  in_rate_old / out_rate_old
> >>
> >> So that's not an integer. Also, Q31 is limited to -1...1, and I think
> >> that's too limiting.
> >>
> >> For this having a Q31.32 fixed point type still makes a lot of sense.
> >>
> >> I still feel this is a overly complicated API.
> >>
> >> See more below...
> >>
> >>>
> >>> Best regards
> >>> Wang shengjiu
> >>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>> All of them can be V4L2_CTRL_TYPE_INTEGER.
> >>>>>
> >>>>> RATIO_MOD was defined in the very beginning version.
> >>>>> I think it is better to let users calculate this value.
> >>>>>
> >>>>> The reason is:
> >>>>> if we define the offset for source rate and dest rate in
> >>>>> driver separately,  when offset of source rate is set,
> >>>>> driver don't know if it needs to wait or not the dest rate
> >>>>> offset,  then go to calculate the ratio_mod.
> >>
> >> Ah, in order to update the ratio mod userspace needs to set both source and
> >> dest rate at the same time to avoid race conditions.
> >>
> >> That is perfectly possible in the V4L2 control framework. See:
> >>
> >> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
> >>
> >> In practice, isn't it likely that you would fix either the source or
> >> destination rate, and let the other rate fluctuate? It kind of feels weird
> >> to me that both source AND destination rates can fluctuate over time.
> >>
> > Right, the source and dest rates needn't change in same time.
> >
> >> In any case, with a control cluster it doesn't really matter, you can set
> >> one rate or both rates, and it will be handled atomically.
> >>
> >> I feel that the RATIO_MOD control is too hardware specific. This is something
> >> that should be hidden in the driver.
> >>
> >
> > I will use:
> >
> > V4L2_CID_M2M_AUDIO_SOURCE_RATE
> > V4L2_CID_M2M_AUDIO_DEST_RATE
> > V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET
> > V4L2_CID_M2M_AUDIO_DEST_RATE_OFFSET
> >
> > 'OFFSET' is V4L2_CTRL_TYPE_FIXED_POINT, which is Q31.32.
>
> So now I come back to my original question: why do you need both
> the rate and the offset? Isn't it enough to set just the rates,
> as long as that is in fixed point format?
>
> Why does the driver need both the 'ideal' rate + the offset?
>
> I'm not opposed to this, I'm just trying to understand whether this
> makes sense.
>
> Can't you take e.g. the source and dest rate as starting points
> when you start streaming? And every time userspace updates one or both
> of these rates you calculate the ratio_mod compared to the previous rates?
>
> Or is there a reason why you need the ideal rates as well? E.g. 48000 or
> 44100, etc.
>

ideal rates is used to select prefilter table. the prefilter table is indexed by
ideal rates.

The asrc has two part: prefilter and resampler filter.  The convert ratio is
applied to rasampler filter part.

best regards
wang shengjiu
Hans Verkuil Oct. 18, 2023, 2:17 p.m. UTC | #12
On 18/10/2023 15:50, Shengjiu Wang wrote:
> On Wed, Oct 18, 2023 at 9:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 18/10/2023 14:52, Shengjiu Wang wrote:
>>> On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 18/10/2023 09:40, Shengjiu Wang wrote:
>>>>> On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>
>>>>>> On 18/10/2023 09:23, Shengjiu Wang wrote:
>>>>>>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>>>
>>>>>>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
>>>>>>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>>>>>
>>>>>>>>>>> Hi Shengjiu,
>>>>>>>>>>>
>>>>>>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>>>> the audio sample rate to driver.
>>>>>>>>>>>>
>>>>>>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>>>>>>>>>>> new IDs for ASRC rate control.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>>>>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>>>>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>>>>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>>>>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>>>>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>>>>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>>>>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>>>>>>>>>>  10 files changed, 76 insertions(+)
>>>>>>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>>>> index ea0435182e44..35707edffb13 100644
>>>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>>>>>>>>>>      ext-ctrls-fm-rx
>>>>>>>>>>>>      ext-ctrls-detect
>>>>>>>>>>>>      ext-ctrls-colorimetry
>>>>>>>>>>>> +    ext-ctrls-fixed-point
>>>>>>>>>>>
>>>>>>>>>>> Rename this to ext-ctrls-audio-m2m.
>>>>>>>>>>>
>>>>>>>>>>>>      fourcc
>>>>>>>>>>>>      format
>>>>>>>>>>>>      planar-apis
>>>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>>>> new file mode 100644
>>>>>>>>>>>> index 000000000000..2ef6e250580c
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. _fixed-point-controls:
>>>>>>>>>>>> +
>>>>>>>>>>>> +***************************
>>>>>>>>>>>> +Fixed Point Control Reference
>>>>>>>>>>>
>>>>>>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>>>>>>>>>>> sense to group fixed point controls. But it does make sense to group the audio
>>>>>>>>>>> controls.
>>>>>>>>>>>
>>>>>>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>>>>>>>>>>> into categories, and for each category there is also a control that gives a
>>>>>>>>>>> description of the class (see 2.15.15 in
>>>>>>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>>>>>>>>>>
>>>>>>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>>>>>>>>>>> they are grouped based on what class of control they are.
>>>>>>>>>>>
>>>>>>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
>>>>>>>>>>> instead of just adding them to the catch-all 'User Controls' class.
>>>>>>>>>>>
>>>>>>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>>>>>>>>>>> it is done.
>>>>>>>>>>>
>>>>>>>>>>> M2M_AUDIO would probably be a good name for the class.
>>>>>>>>>>>
>>>>>>>>>>>> +***************************
>>>>>>>>>>>> +
>>>>>>>>>>>> +These controls are intended to support an asynchronous sample
>>>>>>>>>>>> +rate converter.
>>>>>>>>>>>
>>>>>>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>>>>>>>>>>> that.
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. _v4l2-audio-asrc:
>>>>>>>>>>>> +
>>>>>>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>>>>>>>>>>> +    sets the resampler source rate.
>>>>>>>>>>>> +
>>>>>>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
>>>>>>>>>>>> +    sets the resampler destination rate.
>>>>>>>>>>>
>>>>>>>>>>> Document the unit (Hz) for these two controls.
>>>>>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. cssclass:: longtable
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>>>>>>>>>>> +
>>>>>>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>>>>>>>>>>> +    :header-rows:  0
>>>>>>>>>>>> +    :stub-columns: 0
>>>>>>>>>>>> +    :widths:       1 1 2
>>>>>>>>>>>> +
>>>>>>>>>>>> +    * - __u32
>>>>>>>>>>>
>>>>>>>>>>> Hmm, shouldn't this be __s32?
>>>>>>>>>>>
>>>>>>>>>>>> +      - ``integer``
>>>>>>>>>>>> +      - integer part of fixed point value.
>>>>>>>>>>>> +    * - __s32
>>>>>>>>>>>
>>>>>>>>>>> and this __u32?
>>>>>>>>>>>
>>>>>>>>>>> You want to be able to use this generic type as a signed value.
>>>>>>>>>>>
>>>>>>>>>>>> +      - ``fractional``
>>>>>>>>>>>> +      - fractional part of fixed point value, which is Q31.
>>>>>>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
>>>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>>>>>>>>>>        - ``p_av1_film_grain``
>>>>>>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>>>>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>>>>>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>>>>>>>>>>> +      - ``p_fixed_point``
>>>>>>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>>>>>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>>>>>>>>>>      * - void *
>>>>>>>>>>>>        - ``ptr``
>>>>>>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
>>>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
>>>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>>>> @@ -549,6 +549,13 @@ 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``
>>>>>>>>>>>> +      - n/a
>>>>>>>>>>>> +      - n/a
>>>>>>>>>>>> +      - n/a
>>>>>>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>>>>>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>>>> index a662fb60f73f..7a616ac91059 100644
>>>>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>>>>>>>>>               if (!area->width || !area->height)
>>>>>>>>>>>>                       return -EINVAL;
>>>>>>>>>>>>               break;
>>>>>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>>>>>> +             break;
>>>>>>>>>>>
>>>>>>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>>>>>>>>>>
>>>>>>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>>>>>>>>>>
>>>>>>>>>>> since min and max values are perfectly fine for a fixed point value.
>>>>>>>>>>>
>>>>>>>>>>> Even a step value (currently not supported in that patch) would make sense.
>>>>>>>>>>>
>>>>>>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>>>>>>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
>>>>>>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>>>>>>>>>>
>>>>>>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>>>>>>>>>>> value.
>>>>>>>>>>>
>>>>>>>>>>> So in enum v4l2_ctrl_type add:
>>>>>>>>>>>
>>>>>>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>>>>>>>>>>
>>>>>>>>>>> (10, because it is no longer a compound type).
>>>>>>>>>>
>>>>>>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
>>>>>>>>>>
>>>>>>>>>> The reason I use the 'integer' and 'fractional' is that I want
>>>>>>>>>> 'integer' to be the normal sample
>>>>>>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
>>>>>>>>>> normal sample rate.
>>>>>>>>>>
>>>>>>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
>>>>>>>>>>
>>>>>>>>>> So if we use s64 for rate, then in driver need to convert the rate to
>>>>>>>>>> the closed normal
>>>>>>>>>> sample rate + fractional.
>>>>>>>>>
>>>>>>>>> That wasn't what the documentation said :-)
>>>>>>>>>
>>>>>>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
>>>>>>>>> means in this context) and the offset to the actual sample rate.
>>>>>>>>>
>>>>>>>>> Presumably the 'normal' sample rate is set once, while the offset changes
>>>>>>>>> regularly.
>>>>>>>>>
>>>>>>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
>>>>>>>>> you resample from one rate to another, so why do you need a third 'normal'
>>>>>>>>> rate?
>>>>>>>>>
>>>>>>>>
>>>>>>>> 'Normal' rate is used to select the prefilter table.
>>>>>>>>
>>>>>>>
>>>>>>> Currently I think we may define
>>>>>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
>>>>>>> V4L2_CID_M2M_AUDIO_DEST_RATE
>>>>>>
>>>>>> That makes sense.
>>>>>>
>>>>>>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
>>>>>>
>>>>>> OK, can you document this control? Just write it down in the reply, I just want
>>>>>> to understand how the integer value you set here is used.
>>>>>>
>>>>>
>>>>> It is Q31 value.   It is equal to:
>>>>> in_rate_new / out_rate_new -  in_rate_old / out_rate_old
>>>>
>>>> So that's not an integer. Also, Q31 is limited to -1...1, and I think
>>>> that's too limiting.
>>>>
>>>> For this having a Q31.32 fixed point type still makes a lot of sense.
>>>>
>>>> I still feel this is a overly complicated API.
>>>>
>>>> See more below...
>>>>
>>>>>
>>>>> Best regards
>>>>> Wang shengjiu
>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>         Hans
>>>>>>
>>>>>>>
>>>>>>> All of them can be V4L2_CTRL_TYPE_INTEGER.
>>>>>>>
>>>>>>> RATIO_MOD was defined in the very beginning version.
>>>>>>> I think it is better to let users calculate this value.
>>>>>>>
>>>>>>> The reason is:
>>>>>>> if we define the offset for source rate and dest rate in
>>>>>>> driver separately,  when offset of source rate is set,
>>>>>>> driver don't know if it needs to wait or not the dest rate
>>>>>>> offset,  then go to calculate the ratio_mod.
>>>>
>>>> Ah, in order to update the ratio mod userspace needs to set both source and
>>>> dest rate at the same time to avoid race conditions.
>>>>
>>>> That is perfectly possible in the V4L2 control framework. See:
>>>>
>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
>>>>
>>>> In practice, isn't it likely that you would fix either the source or
>>>> destination rate, and let the other rate fluctuate? It kind of feels weird
>>>> to me that both source AND destination rates can fluctuate over time.
>>>>
>>> Right, the source and dest rates needn't change in same time.
>>>
>>>> In any case, with a control cluster it doesn't really matter, you can set
>>>> one rate or both rates, and it will be handled atomically.
>>>>
>>>> I feel that the RATIO_MOD control is too hardware specific. This is something
>>>> that should be hidden in the driver.
>>>>
>>>
>>> I will use:
>>>
>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
>>> V4L2_CID_M2M_AUDIO_DEST_RATE
>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE_OFFSET
>>> V4L2_CID_M2M_AUDIO_DEST_RATE_OFFSET
>>>
>>> 'OFFSET' is V4L2_CTRL_TYPE_FIXED_POINT, which is Q31.32.
>>
>> So now I come back to my original question: why do you need both
>> the rate and the offset? Isn't it enough to set just the rates,
>> as long as that is in fixed point format?
>>
>> Why does the driver need both the 'ideal' rate + the offset?
>>
>> I'm not opposed to this, I'm just trying to understand whether this
>> makes sense.
>>
>> Can't you take e.g. the source and dest rate as starting points
>> when you start streaming? And every time userspace updates one or both
>> of these rates you calculate the ratio_mod compared to the previous rates?
>>
>> Or is there a reason why you need the ideal rates as well? E.g. 48000 or
>> 44100, etc.
>>
> 
> ideal rates is used to select prefilter table. the prefilter table is indexed by
> ideal rates.
> 
> The asrc has two part: prefilter and resampler filter.  The convert ratio is
> applied to rasampler filter part.

Ah! But doesn't this also imply that there are only a limited number of supported
ideal rates? I see the supported_asrc_rate array in sound/soc/fsl/fsl_asrc.c, which
I suspect is in fact that list?

That means that the SOURCE/DEST_RATE controls aren't integers, but instead should
be of type V4L2_CTRL_TYPE_INTEGER_MENU.

How flexible is the resampler? E.g. I see that 12 and 16 kHz are supported, so can I
select 12 kHz as the ideal rate, and an offset of 2000 Hz to convert from 14 kHz to
something else? Or is it much more limited?

I ask, because if it is really flexible, then it is easy to just select 14 kHz and let
the driver find the closest ideal rate.

If, however, it only can support a small range around each ideal rate (e.g. 11900-12100
Hz around the 12000 Hz ideal rate), then userspace needs to know those ideal rates,
and that needs to be exposed through an INTEGER_MENU control.

Regards,

	Hans
Shengjiu Wang Oct. 19, 2023, 10:44 a.m. UTC | #13
On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 18/10/2023 09:40, Shengjiu Wang wrote:
> > On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 18/10/2023 09:23, Shengjiu Wang wrote:
> >>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
> >>>>
> >>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>
> >>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
> >>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>>>>
> >>>>>>> Hi Shengjiu,
> >>>>>>>
> >>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
> >>>>>>>> Fixed point controls are used by the user to configure
> >>>>>>>> the audio sample rate to driver.
> >>>>>>>>
> >>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> >>>>>>>> new IDs for ASRC rate control.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> >>>>>>>> ---
> >>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
> >>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
> >>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
> >>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
> >>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
> >>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
> >>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
> >>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
> >>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
> >>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
> >>>>>>>>  10 files changed, 76 insertions(+)
> >>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>>
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> index ea0435182e44..35707edffb13 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
> >>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
> >>>>>>>>      ext-ctrls-fm-rx
> >>>>>>>>      ext-ctrls-detect
> >>>>>>>>      ext-ctrls-colorimetry
> >>>>>>>> +    ext-ctrls-fixed-point
> >>>>>>>
> >>>>>>> Rename this to ext-ctrls-audio-m2m.
> >>>>>>>
> >>>>>>>>      fourcc
> >>>>>>>>      format
> >>>>>>>>      planar-apis
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>> new file mode 100644
> >>>>>>>> index 000000000000..2ef6e250580c
> >>>>>>>> --- /dev/null
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> >>>>>>>> @@ -0,0 +1,36 @@
> >>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> >>>>>>>> +
> >>>>>>>> +.. _fixed-point-controls:
> >>>>>>>> +
> >>>>>>>> +***************************
> >>>>>>>> +Fixed Point Control Reference
> >>>>>>>
> >>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
> >>>>>>> sense to group fixed point controls. But it does make sense to group the audio
> >>>>>>> controls.
> >>>>>>>
> >>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
> >>>>>>> into categories, and for each category there is also a control that gives a
> >>>>>>> description of the class (see 2.15.15 in
> >>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
> >>>>>>>
> >>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
> >>>>>>> they are grouped based on what class of control they are.
> >>>>>>>
> >>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
> >>>>>>> instead of just adding them to the catch-all 'User Controls' class.
> >>>>>>>
> >>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
> >>>>>>> it is done.
> >>>>>>>
> >>>>>>> M2M_AUDIO would probably be a good name for the class.
> >>>>>>>
> >>>>>>>> +***************************
> >>>>>>>> +
> >>>>>>>> +These controls are intended to support an asynchronous sample
> >>>>>>>> +rate converter.
> >>>>>>>
> >>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
> >>>>>>> that.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +.. _v4l2-audio-asrc:
> >>>>>>>> +
> >>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
> >>>>>>>> +    sets the resampler source rate.
> >>>>>>>> +
> >>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
> >>>>>>>> +    sets the resampler destination rate.
> >>>>>>>
> >>>>>>> Document the unit (Hz) for these two controls.
> >>>>>>>
> >>>>>>>> +
> >>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
> >>>>>>>> +
> >>>>>>>> +.. cssclass:: longtable
> >>>>>>>> +
> >>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> >>>>>>>> +
> >>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
> >>>>>>>> +    :header-rows:  0
> >>>>>>>> +    :stub-columns: 0
> >>>>>>>> +    :widths:       1 1 2
> >>>>>>>> +
> >>>>>>>> +    * - __u32
> >>>>>>>
> >>>>>>> Hmm, shouldn't this be __s32?
> >>>>>>>
> >>>>>>>> +      - ``integer``
> >>>>>>>> +      - integer part of fixed point value.
> >>>>>>>> +    * - __s32
> >>>>>>>
> >>>>>>> and this __u32?
> >>>>>>>
> >>>>>>> You want to be able to use this generic type as a signed value.
> >>>>>>>
> >>>>>>>> +      - ``fractional``
> >>>>>>>> +      - fractional part of fixed point value, which is Q31.
> >>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> >>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
> >>>>>>>>        - ``p_av1_film_grain``
> >>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
> >>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> >>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
> >>>>>>>> +      - ``p_fixed_point``
> >>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
> >>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> >>>>>>>>      * - void *
> >>>>>>>>        - ``ptr``
> >>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
> >>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
> >>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> >>>>>>>> @@ -549,6 +549,13 @@ 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``
> >>>>>>>> +      - n/a
> >>>>>>>> +      - n/a
> >>>>>>>> +      - n/a
> >>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
> >>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
> >>>>>>>> +
> >>>>>>>>
> >>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> index a662fb60f73f..7a616ac91059 100644
> >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> >>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>>>>>>>               if (!area->width || !area->height)
> >>>>>>>>                       return -EINVAL;
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>>>> +             break;
> >>>>>>>
> >>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
> >>>>>>>
> >>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
> >>>>>>>
> >>>>>>> since min and max values are perfectly fine for a fixed point value.
> >>>>>>>
> >>>>>>> Even a step value (currently not supported in that patch) would make sense.
> >>>>>>>
> >>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
> >>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
> >>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
> >>>>>>>
> >>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
> >>>>>>> value.
> >>>>>>>
> >>>>>>> So in enum v4l2_ctrl_type add:
> >>>>>>>
> >>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
> >>>>>>>
> >>>>>>> (10, because it is no longer a compound type).
> >>>>>>
> >>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
> >>>>>>
> >>>>>> The reason I use the 'integer' and 'fractional' is that I want
> >>>>>> 'integer' to be the normal sample
> >>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
> >>>>>> normal sample rate.
> >>>>>>
> >>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
> >>>>>>
> >>>>>> So if we use s64 for rate, then in driver need to convert the rate to
> >>>>>> the closed normal
> >>>>>> sample rate + fractional.
> >>>>>
> >>>>> That wasn't what the documentation said :-)
> >>>>>
> >>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
> >>>>> means in this context) and the offset to the actual sample rate.
> >>>>>
> >>>>> Presumably the 'normal' sample rate is set once, while the offset changes
> >>>>> regularly.
> >>>>>
> >>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
> >>>>> you resample from one rate to another, so why do you need a third 'normal'
> >>>>> rate?
> >>>>>
> >>>>
> >>>> 'Normal' rate is used to select the prefilter table.
> >>>>
> >>>
> >>> Currently I think we may define
> >>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
> >>> V4L2_CID_M2M_AUDIO_DEST_RATE
> >>
> >> That makes sense.
> >>
> >>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
> >>
> >> OK, can you document this control? Just write it down in the reply, I just want
> >> to understand how the integer value you set here is used.
> >>
> >
> > It is Q31 value.   It is equal to:
> > in_rate_new / out_rate_new -  in_rate_old / out_rate_old
>
> So that's not an integer. Also, Q31 is limited to -1...1, and I think
> that's too limiting.
>
> For this having a Q31.32 fixed point type still makes a lot of sense.

Can we use V4L2_CTRL_TYPE_INTEGER64 for Q31.32?
or still need to define V4L2_CTRL_TYPE_FIXED_POINT?

best regards
wang shengjiu
>
> I still feel this is a overly complicated API.
>
> See more below...
>
> >
> > Best regards
> > Wang shengjiu
> >
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> All of them can be V4L2_CTRL_TYPE_INTEGER.
> >>>
> >>> RATIO_MOD was defined in the very beginning version.
> >>> I think it is better to let users calculate this value.
> >>>
> >>> The reason is:
> >>> if we define the offset for source rate and dest rate in
> >>> driver separately,  when offset of source rate is set,
> >>> driver don't know if it needs to wait or not the dest rate
> >>> offset,  then go to calculate the ratio_mod.
>
> Ah, in order to update the ratio mod userspace needs to set both source and
> dest rate at the same time to avoid race conditions.
>
> That is perfectly possible in the V4L2 control framework. See:
>
> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
>
> In practice, isn't it likely that you would fix either the source or
> destination rate, and let the other rate fluctuate? It kind of feels weird
> to me that both source AND destination rates can fluctuate over time.
>
> In any case, with a control cluster it doesn't really matter, you can set
> one rate or both rates, and it will be handled atomically.
>
> I feel that the RATIO_MOD control is too hardware specific. This is something
> that should be hidden in the driver.
>
> Regards,
>
>         Hans
>
> >>>
> >>> best regards
> >>> wang shengjiu
> >>>
> >>>> Best regards
> >>>> Wang Shengjiu
> >>>>
> >>>>> Regards,
> >>>>>
> >>>>>         Hans
> >>>>>
> >>>>>>
> >>>>>> best regards
> >>>>>> wang shengjiu
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>>       default:
> >>>>>>>>               return -EINVAL;
> >>>>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>>>>>>>       case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>               elem_size = sizeof(struct v4l2_area);
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
> >>>>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
> >>>>>>>> +             break;
> >>>>>>>>       default:
> >>>>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
> >>>>>>>>                       elem_size = sizeof(s32);
> >>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
> >>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> >>>>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
> >>>>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
> >>>>>>>>               break;
> >>>>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
> >>>>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
> >>>>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
> >>>>>>>> +             break;
> >>>>>>>>       default:
> >>>>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
> >>>>>>>>               break;
> >>>>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >>>>>>>> index 59679a42b3e7..645e4cccafc7 100644
> >>>>>>>> --- a/include/media/v4l2-ctrls.h
> >>>>>>>> +++ b/include/media/v4l2-ctrls.h
> >>>>>>>> @@ -56,6 +56,7 @@ struct video_device;
> >>>>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
> >>>>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
> >>>>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
> >>>>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
> >>>>>>>>   * @p:                               Pointer to a compound value.
> >>>>>>>>   * @p_const:                 Pointer to a constant compound value.
> >>>>>>>>   */
> >>>>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
> >>>>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
> >>>>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
> >>>>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
> >>>>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
> >>>>>>>>       void *p;
> >>>>>>>>       const void *p_const;
> >>>>>>>>  };
> >>>>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>>>>>>> index c3604a0a3e30..91096259e3ea 100644
> >>>>>>>> --- a/include/uapi/linux/v4l2-controls.h
> >>>>>>>> +++ b/include/uapi/linux/v4l2-controls.h
> >>>>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
> >>>>>>>>
> >>>>>>>>  /* last CID + 1 */
> >>>>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
> >>>>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
> >>>>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
> >>>>>>>
> >>>>>>> This patch needs to be split in three parts:
> >>>>>>>
> >>>>>>> 1) Add the new M2M_AUDIO control class,
> >>>>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
> >>>>>>> 3) Add the new controls.
> >>>>>>>
> >>>>>>> These are all independent changes, so separating them makes it easier to
> >>>>>>> review.
> >>>>>>>
> >>>>>>>>
> >>>>>>>>  /* USER-class private control IDs */
> >>>>>>>>
> >>>>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
> >>>>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
> >>>>>>>>  #endif
> >>>>>>>>
> >>>>>>>> +/**
> >>>>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
> >>>>>>>> + *
> >>>>>>>> + * @rate_integer: integer part of fixed point value.
> >>>>>>>> + * @rate_fractional: fractional part of fixed point value
> >>>>>>>> + */
> >>>>>>>> +struct v4l2_ctrl_fixed_point {
> >>>>>>>> +     __u32 integer;
> >>>>>>>
> >>>>>>> __s32?
> >>>>>>>
> >>>>>>>> +     __u32 fractional;
> >>>>>>>> +};
> >>>>>>>> +
> >>>>>>>>  #endif
> >>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>>>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
> >>>>>>>> --- a/include/uapi/linux/videodev2.h
> >>>>>>>> +++ b/include/uapi/linux/videodev2.h
> >>>>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
> >>>>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
> >>>>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
> >>>>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
> >>>>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
> >>>>>>>>               void __user *ptr;
> >>>>>>>>       };
> >>>>>>>>  } __attribute__ ((packed));
> >>>>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
> >>>>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
> >>>>>>>> +
> >>>>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
> >>>>>>>>  };
> >>>>>>>>
> >>>>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>>
> >>>>>>>         Hans
> >>>>>
> >>
>
Hans Verkuil Oct. 19, 2023, 10:51 a.m. UTC | #14
On 19/10/2023 12:44, Shengjiu Wang wrote:
> On Wed, Oct 18, 2023 at 3:58 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 18/10/2023 09:40, Shengjiu Wang wrote:
>>> On Wed, Oct 18, 2023 at 3:31 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 18/10/2023 09:23, Shengjiu Wang wrote:
>>>>> On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@gmail.com> wrote:
>>>>>>
>>>>>> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>
>>>>>>> On 17/10/2023 15:11, Shengjiu Wang wrote:
>>>>>>>> On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>>>>
>>>>>>>>> Hi Shengjiu,
>>>>>>>>>
>>>>>>>>> On 13/10/2023 10:31, Shengjiu Wang wrote:
>>>>>>>>>> Fixed point controls are used by the user to configure
>>>>>>>>>> the audio sample rate to driver.
>>>>>>>>>>
>>>>>>>>>> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
>>>>>>>>>> new IDs for ASRC rate control.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
>>>>>>>>>> ---
>>>>>>>>>>  .../userspace-api/media/v4l/common.rst        |  1 +
>>>>>>>>>>  .../media/v4l/ext-ctrls-fixed-point.rst       | 36 +++++++++++++++++++
>>>>>>>>>>  .../media/v4l/vidioc-g-ext-ctrls.rst          |  4 +++
>>>>>>>>>>  .../media/v4l/vidioc-queryctrl.rst            |  7 ++++
>>>>>>>>>>  .../media/videodev2.h.rst.exceptions          |  1 +
>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-core.c     |  5 +++
>>>>>>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  4 +++
>>>>>>>>>>  include/media/v4l2-ctrls.h                    |  2 ++
>>>>>>>>>>  include/uapi/linux/v4l2-controls.h            | 13 +++++++
>>>>>>>>>>  include/uapi/linux/videodev2.h                |  3 ++
>>>>>>>>>>  10 files changed, 76 insertions(+)
>>>>>>>>>>  create mode 100644 Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> index ea0435182e44..35707edffb13 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/common.rst
>>>>>>>>>> @@ -52,6 +52,7 @@ applicable to all devices.
>>>>>>>>>>      ext-ctrls-fm-rx
>>>>>>>>>>      ext-ctrls-detect
>>>>>>>>>>      ext-ctrls-colorimetry
>>>>>>>>>> +    ext-ctrls-fixed-point
>>>>>>>>>
>>>>>>>>> Rename this to ext-ctrls-audio-m2m.
>>>>>>>>>
>>>>>>>>>>      fourcc
>>>>>>>>>>      format
>>>>>>>>>>      planar-apis
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>> new file mode 100644
>>>>>>>>>> index 000000000000..2ef6e250580c
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>>>>>>>>>> @@ -0,0 +1,36 @@
>>>>>>>>>> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
>>>>>>>>>> +
>>>>>>>>>> +.. _fixed-point-controls:
>>>>>>>>>> +
>>>>>>>>>> +***************************
>>>>>>>>>> +Fixed Point Control Reference
>>>>>>>>>
>>>>>>>>> This is for audio controls. "Fixed Point" is just the type, and it doesn't make
>>>>>>>>> sense to group fixed point controls. But it does make sense to group the audio
>>>>>>>>> controls.
>>>>>>>>>
>>>>>>>>> V4L2 controls can be grouped into classes. Basically it is a way to put controls
>>>>>>>>> into categories, and for each category there is also a control that gives a
>>>>>>>>> description of the class (see 2.15.15 in
>>>>>>>>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
>>>>>>>>>
>>>>>>>>> If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
>>>>>>>>> they are grouped based on what class of control they are.
>>>>>>>>>
>>>>>>>>> So I think it would be a good idea to create a new control class for M2M audio controls,
>>>>>>>>> instead of just adding them to the catch-all 'User Controls' class.
>>>>>>>>>
>>>>>>>>> Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to see how
>>>>>>>>> it is done.
>>>>>>>>>
>>>>>>>>> M2M_AUDIO would probably be a good name for the class.
>>>>>>>>>
>>>>>>>>>> +***************************
>>>>>>>>>> +
>>>>>>>>>> +These controls are intended to support an asynchronous sample
>>>>>>>>>> +rate converter.
>>>>>>>>>
>>>>>>>>> Add ' (ASRC).' at the end to indicate the common abbreviation for
>>>>>>>>> that.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +.. _v4l2-audio-asrc:
>>>>>>>>>> +
>>>>>>>>>> +``V4L2_CID_ASRC_SOURCE_RATE``
>>>>>>>>>> +    sets the resampler source rate.
>>>>>>>>>> +
>>>>>>>>>> +``V4L2_CID_ASRC_DEST_RATE``
>>>>>>>>>> +    sets the resampler destination rate.
>>>>>>>>>
>>>>>>>>> Document the unit (Hz) for these two controls.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +.. c:type:: v4l2_ctrl_fixed_point
>>>>>>>>>> +
>>>>>>>>>> +.. cssclass:: longtable
>>>>>>>>>> +
>>>>>>>>>> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
>>>>>>>>>> +
>>>>>>>>>> +.. flat-table:: struct v4l2_ctrl_fixed_point
>>>>>>>>>> +    :header-rows:  0
>>>>>>>>>> +    :stub-columns: 0
>>>>>>>>>> +    :widths:       1 1 2
>>>>>>>>>> +
>>>>>>>>>> +    * - __u32
>>>>>>>>>
>>>>>>>>> Hmm, shouldn't this be __s32?
>>>>>>>>>
>>>>>>>>>> +      - ``integer``
>>>>>>>>>> +      - integer part of fixed point value.
>>>>>>>>>> +    * - __s32
>>>>>>>>>
>>>>>>>>> and this __u32?
>>>>>>>>>
>>>>>>>>> You want to be able to use this generic type as a signed value.
>>>>>>>>>
>>>>>>>>>> +      - ``fractional``
>>>>>>>>>> +      - fractional part of fixed point value, which is Q31.
>>>>>>>>>> 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 f9f73530a6be..1811dabf5c74 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
>>>>>>>>>> @@ -295,6 +295,10 @@ still cause this situation.
>>>>>>>>>>        - ``p_av1_film_grain``
>>>>>>>>>>        - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
>>>>>>>>>>          of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
>>>>>>>>>> +    * - struct :c:type:`v4l2_ctrl_fixed_point` *
>>>>>>>>>> +      - ``p_fixed_point``
>>>>>>>>>> +      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
>>>>>>>>>> +        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
>>>>>>>>>>      * - void *
>>>>>>>>>>        - ``ptr``
>>>>>>>>>>        - A pointer to a compound type which can be an N-dimensional array
>>>>>>>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> index 4d38acafe8e1..9285f4f39eed 100644
>>>>>>>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>>>>>>>> @@ -549,6 +549,13 @@ 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``
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - n/a
>>>>>>>>>> +      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
>>>>>>>>>> +        integer part and fractional part, i.e. audio sample rate.
>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>  .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> index a662fb60f73f..7a616ac91059 100644
>>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
>>>>>>>>>> @@ -1168,6 +1168,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>>>>>>>               if (!area->width || !area->height)
>>>>>>>>>>                       return -EINVAL;
>>>>>>>>>>               break;
>>>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>>>> +             break;
>>>>>>>>>
>>>>>>>>> Hmm, this would need this patch 'v4l2-ctrls: add support for V4L2_CTRL_WHICH_MIN/MAX_VAL':
>>>>>>>>>
>>>>>>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20231010022136.1504015-7-yunkec@google.com/
>>>>>>>>>
>>>>>>>>> since min and max values are perfectly fine for a fixed point value.
>>>>>>>>>
>>>>>>>>> Even a step value (currently not supported in that patch) would make sense.
>>>>>>>>>
>>>>>>>>> But I wonder if we couldn't simplify this: instead of creating a v4l2_ctrl_fixed_point,
>>>>>>>>> why not represent the fixed point value as a Q31.32. Then the standard
>>>>>>>>> minimum/maximum/step values can be used, and it acts like a regular V4L2_TYPE_INTEGER64.
>>>>>>>>>
>>>>>>>>> Except that both userspace and drivers need to multiply it with 2^-32 to get the actual
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> So in enum v4l2_ctrl_type add:
>>>>>>>>>
>>>>>>>>>         V4L2_CTRL_TYPE_FIXED_POINT = 10,
>>>>>>>>>
>>>>>>>>> (10, because it is no longer a compound type).
>>>>>>>>
>>>>>>>> Seems we don't need V4L2_CTRL_TYPE_FIXED_POINT, just use V4L2_TYPE_INTEGER64?
>>>>>>>>
>>>>>>>> The reason I use the 'integer' and 'fractional' is that I want
>>>>>>>> 'integer' to be the normal sample
>>>>>>>> rate, for example 48kHz.  The 'fractional' is the difference with
>>>>>>>> normal sample rate.
>>>>>>>>
>>>>>>>> For example, the rate = 47998.12345.  so integer = 48000,  fractional= -1.87655.
>>>>>>>>
>>>>>>>> So if we use s64 for rate, then in driver need to convert the rate to
>>>>>>>> the closed normal
>>>>>>>> sample rate + fractional.
>>>>>>>
>>>>>>> That wasn't what the documentation said :-)
>>>>>>>
>>>>>>> So this is really two controls: one for the 'normal sample rate' (whatever 'normal'
>>>>>>> means in this context) and the offset to the actual sample rate.
>>>>>>>
>>>>>>> Presumably the 'normal' sample rate is set once, while the offset changes
>>>>>>> regularly.
>>>>>>>
>>>>>>> But why do you need the 'normal' sample rate? With audio resampling I assume
>>>>>>> you resample from one rate to another, so why do you need a third 'normal'
>>>>>>> rate?
>>>>>>>
>>>>>>
>>>>>> 'Normal' rate is used to select the prefilter table.
>>>>>>
>>>>>
>>>>> Currently I think we may define
>>>>> V4L2_CID_M2M_AUDIO_SOURCE_RATE
>>>>> V4L2_CID_M2M_AUDIO_DEST_RATE
>>>>
>>>> That makes sense.
>>>>
>>>>> V4L2_CID_M2M_AUDIO_ASRC_RATIO_MOD
>>>>
>>>> OK, can you document this control? Just write it down in the reply, I just want
>>>> to understand how the integer value you set here is used.
>>>>
>>>
>>> It is Q31 value.   It is equal to:
>>> in_rate_new / out_rate_new -  in_rate_old / out_rate_old
>>
>> So that's not an integer. Also, Q31 is limited to -1...1, and I think
>> that's too limiting.
>>
>> For this having a Q31.32 fixed point type still makes a lot of sense.
> 
> Can we use V4L2_CTRL_TYPE_INTEGER64 for Q31.32?

No.

> or still need to define V4L2_CTRL_TYPE_FIXED_POINT?

Yes, we need that. For the most part V4L2_CTRL_TYPE_FIXED_POINT will be handled
the same as V4L2_CTRL_TYPE_INTEGER64 internally, so that makes life a lot easier.

Regards,

	Hans

> 
> best regards
> wang shengjiu
>>
>> I still feel this is a overly complicated API.
>>
>> See more below...
>>
>>>
>>> Best regards
>>> Wang shengjiu
>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>
>>>>> All of them can be V4L2_CTRL_TYPE_INTEGER.
>>>>>
>>>>> RATIO_MOD was defined in the very beginning version.
>>>>> I think it is better to let users calculate this value.
>>>>>
>>>>> The reason is:
>>>>> if we define the offset for source rate and dest rate in
>>>>> driver separately,  when offset of source rate is set,
>>>>> driver don't know if it needs to wait or not the dest rate
>>>>> offset,  then go to calculate the ratio_mod.
>>
>> Ah, in order to update the ratio mod userspace needs to set both source and
>> dest rate at the same time to avoid race conditions.
>>
>> That is perfectly possible in the V4L2 control framework. See:
>>
>> https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#control-clusters
>>
>> In practice, isn't it likely that you would fix either the source or
>> destination rate, and let the other rate fluctuate? It kind of feels weird
>> to me that both source AND destination rates can fluctuate over time.
>>
>> In any case, with a control cluster it doesn't really matter, you can set
>> one rate or both rates, and it will be handled atomically.
>>
>> I feel that the RATIO_MOD control is too hardware specific. This is something
>> that should be hidden in the driver.
>>
>> Regards,
>>
>>         Hans
>>
>>>>>
>>>>> best regards
>>>>> wang shengjiu
>>>>>
>>>>>> Best regards
>>>>>> Wang Shengjiu
>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>>         Hans
>>>>>>>
>>>>>>>>
>>>>>>>> best regards
>>>>>>>> wang shengjiu
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>       default:
>>>>>>>>>>               return -EINVAL;
>>>>>>>>>> @@ -1868,6 +1870,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>>>>>>>       case V4L2_CTRL_TYPE_AREA:
>>>>>>>>>>               elem_size = sizeof(struct v4l2_area);
>>>>>>>>>>               break;
>>>>>>>>>> +     case V4L2_CTRL_TYPE_FIXED_POINT:
>>>>>>>>>> +             elem_size = sizeof(struct v4l2_ctrl_fixed_point);
>>>>>>>>>> +             break;
>>>>>>>>>>       default:
>>>>>>>>>>               if (type < V4L2_CTRL_COMPOUND_TYPES)
>>>>>>>>>>                       elem_size = sizeof(s32);
>>>>>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>>>> index 8696eb1cdd61..d8f232df6b6a 100644
>>>>>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>>>>>>> @@ -1602,6 +1602,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>>>>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
>>>>>>>>>>               *type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
>>>>>>>>>>               break;
>>>>>>>>>> +     case V4L2_CID_ASRC_SOURCE_RATE:
>>>>>>>>>> +     case V4L2_CID_ASRC_DEST_RATE:
>>>>>>>>>> +             *type = V4L2_CTRL_TYPE_FIXED_POINT;
>>>>>>>>>> +             break;
>>>>>>>>>>       default:
>>>>>>>>>>               *type = V4L2_CTRL_TYPE_INTEGER;
>>>>>>>>>>               break;
>>>>>>>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>>>>>>>>> index 59679a42b3e7..645e4cccafc7 100644
>>>>>>>>>> --- a/include/media/v4l2-ctrls.h
>>>>>>>>>> +++ b/include/media/v4l2-ctrls.h
>>>>>>>>>> @@ -56,6 +56,7 @@ struct video_device;
>>>>>>>>>>   * @p_av1_tile_group_entry:  Pointer to an AV1 tile group entry structure.
>>>>>>>>>>   * @p_av1_frame:             Pointer to an AV1 frame structure.
>>>>>>>>>>   * @p_av1_film_grain:                Pointer to an AV1 film grain structure.
>>>>>>>>>> + * @p_fixed_point:           Pointer to a struct v4l2_ctrl_fixed_point.
>>>>>>>>>>   * @p:                               Pointer to a compound value.
>>>>>>>>>>   * @p_const:                 Pointer to a constant compound value.
>>>>>>>>>>   */
>>>>>>>>>> @@ -89,6 +90,7 @@ union v4l2_ctrl_ptr {
>>>>>>>>>>       struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
>>>>>>>>>>       struct v4l2_ctrl_av1_frame *p_av1_frame;
>>>>>>>>>>       struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
>>>>>>>>>> +     struct v4l2_ctrl_fixed_point *p_fixed_point;
>>>>>>>>>>       void *p;
>>>>>>>>>>       const void *p_const;
>>>>>>>>>>  };
>>>>>>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>>>>>>> index c3604a0a3e30..91096259e3ea 100644
>>>>>>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>>>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>>>>>>> @@ -112,6 +112,8 @@ enum v4l2_colorfx {
>>>>>>>>>>
>>>>>>>>>>  /* last CID + 1 */
>>>>>>>>>>  #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
>>>>>>>>>> +#define V4L2_CID_ASRC_SOURCE_RATE            (V4L2_CID_BASE + 45)
>>>>>>>>>> +#define V4L2_CID_ASRC_DEST_RATE                      (V4L2_CID_BASE + 46)
>>>>>>>>>
>>>>>>>>> This patch needs to be split in three parts:
>>>>>>>>>
>>>>>>>>> 1) Add the new M2M_AUDIO control class,
>>>>>>>>> 2) Add the new V4L2_CTRL_TYPE_FIXED_POINT type,
>>>>>>>>> 3) Add the new controls.
>>>>>>>>>
>>>>>>>>> These are all independent changes, so separating them makes it easier to
>>>>>>>>> review.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>  /* USER-class private control IDs */
>>>>>>>>>>
>>>>>>>>>> @@ -3488,4 +3490,15 @@ struct v4l2_ctrl_av1_film_grain {
>>>>>>>>>>  #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
>>>>>>>>>>  #endif
>>>>>>>>>>
>>>>>>>>>> +/**
>>>>>>>>>> + * struct v4l2_ctrl_fixed_point - fixed point parameter.
>>>>>>>>>> + *
>>>>>>>>>> + * @rate_integer: integer part of fixed point value.
>>>>>>>>>> + * @rate_fractional: fractional part of fixed point value
>>>>>>>>>> + */
>>>>>>>>>> +struct v4l2_ctrl_fixed_point {
>>>>>>>>>> +     __u32 integer;
>>>>>>>>>
>>>>>>>>> __s32?
>>>>>>>>>
>>>>>>>>>> +     __u32 fractional;
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>  #endif
>>>>>>>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>>>>>>>> index 2ac7b989394c..3ef32c09c2fa 100644
>>>>>>>>>> --- a/include/uapi/linux/videodev2.h
>>>>>>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>>>>>>> @@ -1888,6 +1888,7 @@ struct v4l2_ext_control {
>>>>>>>>>>               struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
>>>>>>>>>>               struct v4l2_ctrl_av1_frame __user *p_av1_frame;
>>>>>>>>>>               struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
>>>>>>>>>> +             struct v4l2_ctrl_fixed_point __user *p_fixed_point;
>>>>>>>>>>               void __user *ptr;
>>>>>>>>>>       };
>>>>>>>>>>  } __attribute__ ((packed));
>>>>>>>>>> @@ -1966,6 +1967,8 @@ enum v4l2_ctrl_type {
>>>>>>>>>>       V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
>>>>>>>>>>       V4L2_CTRL_TYPE_AV1_FRAME            = 0x282,
>>>>>>>>>>       V4L2_CTRL_TYPE_AV1_FILM_GRAIN       = 0x283,
>>>>>>>>>> +
>>>>>>>>>> +     V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
>>>>>>>>>>  };
>>>>>>>>>>
>>>>>>>>>>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>>         Hans
>>>>>>>
>>>>
>>
diff mbox series

Patch

diff --git a/Documentation/userspace-api/media/v4l/common.rst b/Documentation/userspace-api/media/v4l/common.rst
index ea0435182e44..35707edffb13 100644
--- a/Documentation/userspace-api/media/v4l/common.rst
+++ b/Documentation/userspace-api/media/v4l/common.rst
@@ -52,6 +52,7 @@  applicable to all devices.
     ext-ctrls-fm-rx
     ext-ctrls-detect
     ext-ctrls-colorimetry
+    ext-ctrls-fixed-point
     fourcc
     format
     planar-apis
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
new file mode 100644
index 000000000000..2ef6e250580c
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
@@ -0,0 +1,36 @@ 
+.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
+
+.. _fixed-point-controls:
+
+***************************
+Fixed Point Control Reference
+***************************
+
+These controls are intended to support an asynchronous sample
+rate converter.
+
+.. _v4l2-audio-asrc:
+
+``V4L2_CID_ASRC_SOURCE_RATE``
+    sets the resampler source rate.
+
+``V4L2_CID_ASRC_DEST_RATE``
+    sets the resampler destination rate.
+
+.. c:type:: v4l2_ctrl_fixed_point
+
+.. cssclass:: longtable
+
+.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
+
+.. flat-table:: struct v4l2_ctrl_fixed_point
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u32
+      - ``integer``
+      - integer part of fixed point value.
+    * - __s32
+      - ``fractional``
+      - fractional part of fixed point value, which is Q31.
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 f9f73530a6be..1811dabf5c74 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
@@ -295,6 +295,10 @@  still cause this situation.
       - ``p_av1_film_grain``
       - A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if this control is
         of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
+    * - struct :c:type:`v4l2_ctrl_fixed_point` *
+      - ``p_fixed_point``
+      - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this control is
+        of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
     * - void *
       - ``ptr``
       - A pointer to a compound type which can be an N-dimensional array
diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
index 4d38acafe8e1..9285f4f39eed 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
@@ -549,6 +549,13 @@  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``
+      - n/a
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_ctrl_fixed_point`, containing parameter which has
+        integer part and fractional part, i.e. audio sample rate.
+
 
 .. 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-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
index a662fb60f73f..7a616ac91059 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
@@ -1168,6 +1168,8 @@  static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		if (!area->width || !area->height)
 			return -EINVAL;
 		break;
+	case V4L2_CTRL_TYPE_FIXED_POINT:
+		break;
 
 	default:
 		return -EINVAL;
@@ -1868,6 +1870,9 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_AREA:
 		elem_size = sizeof(struct v4l2_area);
 		break;
+	case V4L2_CTRL_TYPE_FIXED_POINT:
+		elem_size = sizeof(struct v4l2_ctrl_fixed_point);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 8696eb1cdd61..d8f232df6b6a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1602,6 +1602,10 @@  void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:
 		*type = V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY;
 		break;
+	case V4L2_CID_ASRC_SOURCE_RATE:
+	case V4L2_CID_ASRC_DEST_RATE:
+		*type = V4L2_CTRL_TYPE_FIXED_POINT;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 59679a42b3e7..645e4cccafc7 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -56,6 +56,7 @@  struct video_device;
  * @p_av1_tile_group_entry:	Pointer to an AV1 tile group entry structure.
  * @p_av1_frame:		Pointer to an AV1 frame structure.
  * @p_av1_film_grain:		Pointer to an AV1 film grain structure.
+ * @p_fixed_point:		Pointer to a struct v4l2_ctrl_fixed_point.
  * @p:				Pointer to a compound value.
  * @p_const:			Pointer to a constant compound value.
  */
@@ -89,6 +90,7 @@  union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_av1_tile_group_entry *p_av1_tile_group_entry;
 	struct v4l2_ctrl_av1_frame *p_av1_frame;
 	struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
+	struct v4l2_ctrl_fixed_point *p_fixed_point;
 	void *p;
 	const void *p_const;
 };
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index c3604a0a3e30..91096259e3ea 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -112,6 +112,8 @@  enum v4l2_colorfx {
 
 /* last CID + 1 */
 #define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)
+#define V4L2_CID_ASRC_SOURCE_RATE		(V4L2_CID_BASE + 45)
+#define V4L2_CID_ASRC_DEST_RATE			(V4L2_CID_BASE + 46)
 
 /* USER-class private control IDs */
 
@@ -3488,4 +3490,15 @@  struct v4l2_ctrl_av1_film_grain {
 #define V4L2_CID_MPEG_MFC51_BASE        V4L2_CID_CODEC_MFC51_BASE
 #endif
 
+/**
+ * struct v4l2_ctrl_fixed_point - fixed point parameter.
+ *
+ * @rate_integer: integer part of fixed point value.
+ * @rate_fractional: fractional part of fixed point value
+ */
+struct v4l2_ctrl_fixed_point {
+	__u32 integer;
+	__u32 fractional;
+};
+
 #endif
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2ac7b989394c..3ef32c09c2fa 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1888,6 +1888,7 @@  struct v4l2_ext_control {
 		struct v4l2_ctrl_av1_tile_group_entry __user *p_av1_tile_group_entry;
 		struct v4l2_ctrl_av1_frame __user *p_av1_frame;
 		struct v4l2_ctrl_av1_film_grain __user *p_av1_film_grain;
+		struct v4l2_ctrl_fixed_point __user *p_fixed_point;
 		void __user *ptr;
 	};
 } __attribute__ ((packed));
@@ -1966,6 +1967,8 @@  enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_AV1_TILE_GROUP_ENTRY = 0x281,
 	V4L2_CTRL_TYPE_AV1_FRAME	    = 0x282,
 	V4L2_CTRL_TYPE_AV1_FILM_GRAIN	    = 0x283,
+
+	V4L2_CTRL_TYPE_FIXED_POINT          = 0x290,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */