Re: [PATCH v4 6/9] media: uapi: Add a control for DW100 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 25/04/2022 09:11, Laurent Pinchart wrote:
> On Mon, Apr 25, 2022 at 08:57:07AM +0200, Hans Verkuil wrote:
>> On 28/03/2022 16:13, Xavier Roumegue wrote:
>>> The DW100 driver gets the dewarping mapping as a binary blob from the
>>> userspace application through a custom control.
>>> The blob format is hardware specific so create a dedicated control for
>>> this purpose.
>>>
>>> Signed-off-by: Xavier Roumegue <xavier.roumegue@xxxxxxxxxxx>
>>> ---
>>>  Documentation/userspace-api/media/drivers/dw100.rst | 12 ++++++++++++
>>>  include/uapi/linux/dw100.h                          | 11 +++++++++++
>>>  2 files changed, 23 insertions(+)
>>>  create mode 100644 include/uapi/linux/dw100.h
>>>
>>> diff --git a/Documentation/userspace-api/media/drivers/dw100.rst b/Documentation/userspace-api/media/drivers/dw100.rst
>>> index 4cd55c75628e..f6d684cadf26 100644
>>> --- a/Documentation/userspace-api/media/drivers/dw100.rst
>>> +++ b/Documentation/userspace-api/media/drivers/dw100.rst
>>> @@ -20,4 +20,16 @@ match the expected size inherited from the destination image resolution.
>>>  More details on the DW100 hardware operations can be found in
>>>  *chapter 13.15 DeWarp* of IMX8MP_ reference manuel.
>>>  
>>> +The Vivante DW100 m2m driver implements the following driver-specific control:
>>> +
>>> +``V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP (integer)``
>>
>> (integer) -> (__u32 array)
>>
>> But should this be a __u32 array at all? Wouldn't a __u16 array make more sense?
>>
>>> +    Specifies to DW100 driver its dewarping map (aka LUT) blob as described in
>>> +    *chapter 13.15.2.3 Dewarping Remap* of IMX8MP_ reference manual as an U32
>>> +    dynamic array. The image is divided into many small 16x16 blocks. If the
>>> +    width of the image is not divisible by 16, the size of the rightmost block
>>> +    is the remainder. 
>>
>> Isn't the same true for the height?
>>
>> The dewarping map only saves the vertex coordinates of the
>>> +    block. The dewarping grid map is comprised of vertex coordinates for x and y.
>>> +    Each x, y coordinate register uses 16 bits (UQ12.4) to record the coordinate
>>
>> As mentioned before, UQ12.4 is not necessarily a standard notation. 'unsigned 12.4
>> fixed point' is better, but you also need to specify exactly where the bits are
>> stored inside the __u16. I.e.: 'the integer part is stored in the 12 most significant
>> bits, and the fractional part is stored in the 4 least significant bits of the __u16.'
> 
> Isn't that implied ? I've never seen fixed-point numbers stored the
> other way around.

True, perhaps that's overkill.

> 
> Regarding the Q notation, while it was coined by TI, I think it's
> widespread enough to be used here. I don't mind much though.

I had to look it up :-)

That might say more about me, though...

I think the key phrase that is missing here is "fixed point".

Regards,

	Hans

> 
>>> +    address, with the Y coordinate in the upper bits and X in the lower bits.
>>
>> And with a __u16 array this becomes: 'The array contains pairs of X, Y coordinates.'
>> Or something along those lines.
>>
>>> +
>>>  .. _IMX8MP: https://www.nxp.com/webapp/Download?colCode=IMX8MPRM
>>> diff --git a/include/uapi/linux/dw100.h b/include/uapi/linux/dw100.h
>>> new file mode 100644
>>> index 000000000000..7fdcf2bf42e5
>>> --- /dev/null
>>> +++ b/include/uapi/linux/dw100.h
>>> @@ -0,0 +1,11 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
>>> +/* Copyright 2022 NXP */
>>> +
>>> +#ifndef __UAPI_DW100_H__
>>> +#define __UAPI_DW100_H__
>>> +
>>> +#include <linux/v4l2-controls.h>
>>> +
>>
>> Add a comment referring to the Documentation/userspace-api/media/drivers/dw100.rst
>> documentation so users of this control know where to find the associated
>> documentation.
>>
>>> +#define V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP (V4L2_CID_USER_DW100_BASE + 1)
>>> +
>>> +#endif
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux