On 18/10/2023 09:23, Shengjiu Wang wrote: > On Wed, Oct 18, 2023 at 10:27 AM Shengjiu Wang <shengjiu.wang@xxxxxxxxx> wrote: >> >> On Tue, Oct 17, 2023 at 9:37 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >>> >>> On 17/10/2023 15:11, Shengjiu Wang wrote: >>>> 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. >>> >>> 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 >>>
