Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT

[Hans Verkuil]

On 13/11/2023 12:07, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote:
>> On 13/11/2023 11:42, Laurent Pinchart wrote:
>>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote:
>>>> Hi Shengjiu,
>>>>
>>>> On 10/11/2023 06:48, Shengjiu Wang wrote:
>>>>> Fixed point controls are used by the user to configure
>>>>> a fixed point value in 64bits, which Q31.32 format.
>>>>>
>>>>> Signed-off-by: Shengjiu Wang <shengjiu.wang@xxxxxxx>
>>>>
>>>> This patch adds a new control type. This is something that also needs to be
>>>> tested by v4l2-compliance, and for that we need to add support for this to
>>>> one of the media test-drivers. The best place for that is the vivid driver,
>>>> since that has already a bunch of test controls for other control types.
>>>>
>>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c.
>>>>
>>>> Can you add a patch adding a fixed point test control to vivid?
>>>
>>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to
>>> relate more to units than control types. We have lots of fixed-point
>>> values in controls already, using the 32-bit and 64-bit integer control
>>> types. They use various locations for the decimal point, depending on
>>> the control. If we want to make this more explicit to users, we should
>>> work on adding unit support to the V4L2 controls.
>>
>> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units.
> 
> It's not a unit, but I think it's related to units. My point is that,
> without units support, I don't see why we need a formal definition of
> fixed-point types, and why this series couldn't just use
> VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64
> values as they see fit.

They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64
(I assume you meant that rather than VIVID_CID_INTEGER64) shows that it
is always interpreted as a 64 bit integer and nothing else. As it should.

And while we do not have support for units (other than the documentation),
we do have type support in the form of V4L2_CTRL_TYPE_*.

> 
>> A quick "git grep -i "fixed point" Documentation/userspace-api/media/'
>> only shows a single driver specific control (dw100.rst).
>>
>> I'm not aware of other controls in mainline that use fixed point.
> 
> The analog gain control for sensors for instance.

Not really. The documentation is super vague:

V4L2_CID_ANALOGUE_GAIN (integer)

	Analogue gain is gain affecting all colour components in the pixel matrix. The
	gain operation is performed in the analogue domain before A/D conversion.

And the integer is just a range. Internally it might map to some fixed
point value, but userspace won't see that, it's hidden in the driver AFAICT.

In the case of this particular series the control type is really a fixed point
value with a documented unit (Hz). It really is not something you want to
use type INTEGER64 for.

> 
>> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting
>> min/max/step you can easily map that to just about any QN.M format where
>> N <= 31 and M <= 32.
>>
>> In the case of dw100 it is a bit different in that it is quite specialized
>> and it had to fit in 16 bits.

Regards,

	Hans