On Mon, Oct 16, 2023 at 9:16 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxx> > > --- > > .../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@xxxxxxxxxx/ > > 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
