diff mbox series

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

Message ID 1695202370-24678-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 Sept. 20, 2023, 9:32 a.m. UTC
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@nxp.com>
---
 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(+)

Comments

Hans Verkuil Sept. 20, 2023, 10:19 a.m. UTC | #1
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@nxp.com>
> ---
>  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?

>  	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
Shengjiu Wang Sept. 21, 2023, 6:55 a.m. UTC | #2
On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
> > ---
> >  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.

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.

There is folder Documentation/userspace-api/media/drivers/ for drivers
Should this document in this folder, not in the
Documentation/admin-guide/media/?

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
Hans Verkuil Sept. 21, 2023, 7:11 a.m. UTC | #3
On 21/09/2023 08:55, Shengjiu Wang wrote:
> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
>>> ---
>>>  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?

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
Shengjiu Wang Sept. 21, 2023, 11:13 a.m. UTC | #4
On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 21/09/2023 08:55, Shengjiu Wang wrote:
> > On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
> >>> ---
> >>>  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

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
>
Mark Brown Sept. 21, 2023, 11:45 a.m. UTC | #5
On Thu, Sep 21, 2023 at 07:13:14PM +0800, Shengjiu Wang wrote:

> 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.

It does sound like something other vendors are likely to have to provide
a mechanism to compensate for clock inaccuracies.
Hans Verkuil Sept. 21, 2023, 2:09 p.m. UTC | #6
On 21/09/2023 13:13, Shengjiu Wang wrote:
> On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 21/09/2023 08:55, Shengjiu Wang wrote:
>>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
>>>>> ---
>>>>>  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
>>
Shengjiu Wang Sept. 22, 2023, 2:51 a.m. UTC | #7
On Thu, Sep 21, 2023 at 10:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 21/09/2023 13:13, Shengjiu Wang wrote:
> > On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 21/09/2023 08:55, Shengjiu Wang wrote:
> >>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
> >>>>> ---
> >>>>>  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?

yes.

>
> 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?

yes.

>
> 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.
>

(rate, channel, format) are the basic parameters for audio stream.
For example, if there is decoder/encoder requirement, the rate field is
still needed,  I think the rate shouldn't be removed.

tweak ratio is not always needed by use case. As you said, for
file to file conversion, it is not needed, so keeping 'rate' is necessary.

best regards
wang shengjiu

> 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
> >>
>
Hans Verkuil Sept. 22, 2023, 8:54 a.m. UTC | #8
Hi Shengjiu,

On 22/09/2023 04:51, Shengjiu Wang wrote:
> On Thu, Sep 21, 2023 at 10:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 21/09/2023 13:13, Shengjiu Wang wrote:
>>> On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 21/09/2023 08:55, Shengjiu Wang wrote:
>>>>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
>>>>>>> ---
>>>>>>>  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?
> 
> yes.
> 
>>
>> 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?
> 
> yes.
> 
>>
>> 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.
>>
> 
> (rate, channel, format) are the basic parameters for audio stream.
> For example, if there is decoder/encoder requirement, the rate field is
> still needed,  I think the rate shouldn't be removed.

The v4l2_format struct is meant to describe the format of the data in memory,
not the rate at which the data has to be processed. It is the same for video:
v4l2_format describes the memory layout of the video data, not the framerate.
That is done through other ioctls (VIDIOC_S/G_PARM, a horrible ioctl, but
that's another story). So for audio the channel and format fields define how
the audio data is laid out in memory, but the rate has nothing to do with
that.

For this resampler you don't even need the rate at all, all you need is the
rate ratio, right? I.e. there is no difference when resampling from 10 kHz to 20 kHz
vs. 30 kHz to 60 kHz, the ratio is the same.

Or is that too simplistic and the hardware needs the actual rates as well?

Remember that I am a video guy, not an audio guy, so apologies if I ask stupid
questions!

Regardless, I don't believe the rate belongs to the audio format struct. It's
not how v4l2_format works. If the rate is needed, then that is probably best
done through controls, one for the source (output queue) and one for the
destination (capture queue).

Regards,

	Hans

> 
> tweak ratio is not always needed by use case. As you said, for
> file to file conversion, it is not needed, so keeping 'rate' is necessary.
> 
> best regards
> wang shengjiu
> 
>> 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
>>>>
>>
Shengjiu Wang Sept. 22, 2023, 10:52 a.m. UTC | #9
On Fri, Sep 22, 2023 at 4:54 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Shengjiu,
>
> On 22/09/2023 04:51, Shengjiu Wang wrote:
> > On Thu, Sep 21, 2023 at 10:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 21/09/2023 13:13, Shengjiu Wang wrote:
> >>> On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 21/09/2023 08:55, Shengjiu Wang wrote:
> >>>>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
> >>>>>>> ---
> >>>>>>>  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?
> >
> > yes.
> >
> >>
> >> 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?
> >
> > yes.
> >
> >>
> >> 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.
> >>
> >
> > (rate, channel, format) are the basic parameters for audio stream.
> > For example, if there is decoder/encoder requirement, the rate field is
> > still needed,  I think the rate shouldn't be removed.
>
> The v4l2_format struct is meant to describe the format of the data in memory,
> not the rate at which the data has to be processed. It is the same for video:
> v4l2_format describes the memory layout of the video data, not the framerate.
> That is done through other ioctls (VIDIOC_S/G_PARM, a horrible ioctl, but
> that's another story). So for audio the channel and format fields define how
> the audio data is laid out in memory, but the rate has nothing to do with
> that.
>
> For this resampler you don't even need the rate at all, all you need is the
> rate ratio, right? I.e. there is no difference when resampling from 10 kHz to 20 kHz
> vs. 30 kHz to 60 kHz, the ratio is the same.
>
> Or is that too simplistic and the hardware needs the actual rates as well?
>
> Remember that I am a video guy, not an audio guy, so apologies if I ask stupid
> questions!
>
> Regardless, I don't believe the rate belongs to the audio format struct. It's
> not how v4l2_format works. If the rate is needed, then that is probably best
> done through controls, one for the source (output queue) and one for the
> destination (capture queue).
>

I am also not familiar with V4L2.  So I try not to touch too many things in
V4L2.

The current asrc memory to memory is implemented based on the alsa
asrc driver,  that asrc alsa driver needs 'rate' parameter.

The struct v4l2_vbi_format has a value: sampling_rate,  is there any reason
here?

if use controls,  do I need to define a new one?

best regards
wang shengjiu

> Regards,
>
>         Hans
>
> >
> > tweak ratio is not always needed by use case. As you said, for
> > file to file conversion, it is not needed, so keeping 'rate' is necessary.
> >
> > best regards
> > wang shengjiu
> >
> >> 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
> >>>>
> >>
>
Hans Verkuil Sept. 22, 2023, 11:18 a.m. UTC | #10
On 22/09/2023 12:52, Shengjiu Wang wrote:
> On Fri, Sep 22, 2023 at 4:54 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Shengjiu,
>>
>> On 22/09/2023 04:51, Shengjiu Wang wrote:
>>> On Thu, Sep 21, 2023 at 10:09 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 21/09/2023 13:13, Shengjiu Wang wrote:
>>>>> On Thu, Sep 21, 2023 at 3:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>
>>>>>> On 21/09/2023 08:55, Shengjiu Wang wrote:
>>>>>>> On Wed, Sep 20, 2023 at 6:19 PM Hans Verkuil <hverkuil@xs4all.nl> 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@nxp.com>
>>>>>>>>> ---
>>>>>>>>>  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?
>>>
>>> yes.
>>>
>>>>
>>>> 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?
>>>
>>> yes.
>>>
>>>>
>>>> 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.
>>>>
>>>
>>> (rate, channel, format) are the basic parameters for audio stream.
>>> For example, if there is decoder/encoder requirement, the rate field is
>>> still needed,  I think the rate shouldn't be removed.
>>
>> The v4l2_format struct is meant to describe the format of the data in memory,
>> not the rate at which the data has to be processed. It is the same for video:
>> v4l2_format describes the memory layout of the video data, not the framerate.
>> That is done through other ioctls (VIDIOC_S/G_PARM, a horrible ioctl, but
>> that's another story). So for audio the channel and format fields define how
>> the audio data is laid out in memory, but the rate has nothing to do with
>> that.
>>
>> For this resampler you don't even need the rate at all, all you need is the
>> rate ratio, right? I.e. there is no difference when resampling from 10 kHz to 20 kHz
>> vs. 30 kHz to 60 kHz, the ratio is the same.
>>
>> Or is that too simplistic and the hardware needs the actual rates as well?
>>
>> Remember that I am a video guy, not an audio guy, so apologies if I ask stupid
>> questions!
>>
>> Regardless, I don't believe the rate belongs to the audio format struct. It's
>> not how v4l2_format works. If the rate is needed, then that is probably best
>> done through controls, one for the source (output queue) and one for the
>> destination (capture queue).
>>
> 
> I am also not familiar with V4L2.  So I try not to touch too many things in
> V4L2.
> 
> The current asrc memory to memory is implemented based on the alsa
> asrc driver,  that asrc alsa driver needs 'rate' parameter.
> 
> The struct v4l2_vbi_format has a value: sampling_rate,  is there any reason
> here?

That has nothing to do with the frame rate. In the case of VBI it is the sampling
rate with which the receiver hardware samples the data, and it is needed in order
to interpret the data. It is an old API, and I suspect that today I would use a
control for this as well.

> 
> if use controls,  do I need to define a new one?

Yes.

"SOURCE_RATE" and "TARGET_RATE" perhaps? Or some variation on that? "DEST_RATE" would
also work.

I'm wondering about the unit, mostly because I really don't like that rate modifier
control. If you would specify the rate in mHz, would that give enough precision to
tweak the audio rate if needed?

And that would also be a generic API, rather than mediatek specific.

Regards,

	Hans

> 
> best regards
> wang shengjiu
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>> tweak ratio is not always needed by use case. As you said, for
>>> file to file conversion, it is not needed, so keeping 'rate' is necessary.
>>>
>>> best regards
>>> wang shengjiu
>>>
>>>> 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
>>>>>>
>>>>
>>
diff mbox series

Patch

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.
+
 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";
 	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.