Re: [RFC PATCH v4 09/11] media: uapi: Add V4L2_CID_USER_IMX_ASRC_RATIO_MOD control

[Hans Verkuil]

On 21/09/2023 13:13, Shengjiu Wang wrote:
> On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> On 21/09/2023 08:55, Shengjiu Wang wrote:
>>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>>
>>>> On 20/09/2023 11:32, Shengjiu Wang wrote:
>>>>> The input clock and output clock may not be the accurate
>>>>> rate as the sample rate, there is some drift, so the convert
>>>>> ratio of i.MX ASRC module need to be changed according to
>>>>> actual clock rate.
>>>>>
>>>>> Add V4L2_CID_USER_IMX_ASRC_RATIO_MOD control for user to
>>>>> adjust the ratio.
>>>>>
>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
>>>>> ---
>>>>>  Documentation/userspace-api/media/v4l/control.rst | 5 +++++
>>>>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c         | 1 +
>>>>>  include/uapi/linux/v4l2-controls.h                | 1 +
>>>>>  3 files changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/userspace-api/media/v4l/control.rst b/Documentation/userspace-api/media/v4l/control.rst
>>>>> index 4463fce694b0..2bc175900a34 100644
>>>>> --- a/Documentation/userspace-api/media/v4l/control.rst
>>>>> +++ b/Documentation/userspace-api/media/v4l/control.rst
>>>>> @@ -318,6 +318,11 @@ Control IDs
>>>>>      depending on particular custom controls should check the driver name
>>>>>      and version, see :ref:`querycap`.
>>>>>
>>>>> +.. _v4l2-audio-imx:
>>>>> +
>>>>> +``V4L2_CID_USER_IMX_ASRC_RATIO_MOD``
>>>>> +    sets the rasampler ratio modifier of i.MX asrc module.
>>>>
>>>> rasampler -> resampler (I think?)
>>>>
>>>> This doesn't document at all what the type of the control is or how to interpret it.
>>>>
>>>>> +
>>>>>  Applications can enumerate the available controls with the
>>>>>  :ref:`VIDIOC_QUERYCTRL` and
>>>>>  :ref:`VIDIOC_QUERYMENU <VIDIOC_QUERYCTRL>` ioctls, get and set a
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>> index 8696eb1cdd61..16f66f66198c 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>>>>> @@ -1242,6 +1242,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>>>       case V4L2_CID_COLORIMETRY_CLASS:        return "Colorimetry Controls";
>>>>>       case V4L2_CID_COLORIMETRY_HDR10_CLL_INFO:               return "HDR10 Content Light Info";
>>>>>       case V4L2_CID_COLORIMETRY_HDR10_MASTERING_DISPLAY:      return "HDR10 Mastering Display";
>>>>> +     case V4L2_CID_USER_IMX_ASRC_RATIO_MOD:                  return "ASRC RATIO MOD";
>>>>
>>>> Let's stay consistent with the other control names:
>>>>
>>>> "ASRC Ratio Modifier"
>>>>
>>>> But if this is a driver specific control, then this doesn't belong here.
>>>>
>>>> Driver specific controls are defined in the driver itself, including this
>>>> description.
>>>>
>>>> Same for the control documentation: if it is driver specific, then that
>>>> typically is documented either in a driver-specific public header, or
>>>> possibly in driver-specific documentation (Documentation/admin-guide/media/).
>>>>
>>>> But is this imx specific? Wouldn't other similar devices need this?
>>>
>>> It is imx specific.
>>
>> Why? I'm not opposed to this, but I wonder if you looked at datasheets of
>> similar devices from other vendors: would they use something similar?
> 
> I tried to find some datasheets for other vendors, but failed to find them.
> So I don't know how they implement this part.
> 
> Ratio modification on i.MX is to modify the configured ratio.
> For example, the input rate is 44.1kHz,  output rate is 48kHz,
> configured ratio = 441/480,   the ratio modification is to modify
> the fractional part of (441/480) with small steps.  because the
> input clock or output clock has drift in the real hardware.
> The ratio modification is signed value, it is added to configured
> ratio.
> 
> In our case, we have some sysfs interface for user to get the
> clock from input audio device and output audio device, user
> need to calculate the ratio dynamically , then configure the
> modification to driver

So this ratio modifier comes into play when either the audio input
or audio output (or both) are realtime audio inputs/outputs where
the sample rate is not a perfect 44.1 or 48 kHz, but slightly different?

If you would use this resampler to do offline resampling (i.e. resample
a 44.1 kHz wav file to a 48 kHz wav file), then this wouldn't be needed,
correct?

When dealing with realtime audio, userspace will know how to get the
precise sample rate, but that is out-of-scope of this driver. Here
you just need a knob to slightly tweak the resampling ratio.

If my understanding is correct, then I wonder if it is such a good
idea to put the rate into the v4l2_audio_format: it really has nothing
to do with the audio format as it is stored in memory.

What if you would drop that 'rate' field and instead create just a single
control for the resampling ratio. This can use struct v4l2_fract to represent
a fraction. It would be more work since v4l2_fract is currently not supported
for controls, but it is not hard to add support for that (just a bit tedious)
and I actually think this might be a perfect solution.

That way userspace can quite precisely tweak the ratio on the fly, and
it is a generic solution as well instead of mediatek specific.

Regards,

	Hans

> 
> May be other vendors has similar implementation. or make
> the definition be generic is an option.
> 
> best regards
> wang shengjiu
> 
>>
>> And the very short description you gave in the commit log refers to input
>> and output clock: how would userspace know those clock frequencies? In
>> other words, what information does userspace need in order to set this
>> control correctly? And is that information actually available? How would
>> you use this control?
>>
>> I don't really understand how this is supposed to be used.
>>
>>>
>>> Does this mean that I need to create a header file in include/uapi/linux
>>> folder to put this definition?  I just hesitate if this is necessary.
>>
>> Yes, put it there. There are some examples of this already:
>>
>> include/uapi/linux/aspeed-video.h
>> include/uapi/linux/max2175.h
>>
>>>
>>> There is folder Documentation/userspace-api/media/drivers/ for drivers
>>> Should this document in this folder, not in the
>>> Documentation/admin-guide/media/?
>>
>> Yes, you are correct. For the headers above, the corresponding documentation
>> is in:
>>
>> Documentation/userspace-api/media/drivers/aspeed-video.rst
>> Documentation/userspace-api/media/drivers/max2175.rst
>>
>> So you have some examples as reference.
>>
>> Frankly, what is in admin-guide and in userspace-api is a bit random, it
>> probably could use a cleanup.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Best regards
>>> Wang shengjiu
>>>>
>>>>>       default:
>>>>>               return NULL;
>>>>>       }
>>>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>>>> index c3604a0a3e30..b1c319906d12 100644
>>>>> --- a/include/uapi/linux/v4l2-controls.h
>>>>> +++ b/include/uapi/linux/v4l2-controls.h
>>>>> @@ -162,6 +162,7 @@ enum v4l2_colorfx {
>>>>>  /* The base for the imx driver controls.
>>>>>   * We reserve 16 controls for this driver. */
>>>>>  #define V4L2_CID_USER_IMX_BASE                       (V4L2_CID_USER_BASE + 0x10b0)
>>>>> +#define V4L2_CID_USER_IMX_ASRC_RATIO_MOD     (V4L2_CID_USER_IMX_BASE + 0)
>>>>>
>>>>>  /*
>>>>>   * The base for the atmel isc driver controls.
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>