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

["Xavier Roumegue (OSS)" <xavier.roumegue@oss.nxp.com>]

On 3/8/22 21:28, Nicolas Dufresne wrote:
> Le mardi 08 mars 2022 à 20:42 +0100, Xavier Roumegue (OSS) a écrit :
>> Hello Nicolas,
>>
>> On 3/8/22 20:15, Nicolas Dufresne wrote:
>>> Le mardi 08 mars 2022 à 19:48 +0100, Xavier Roumegue a écrit :
>>>> 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@oss.nxp.com>
>>>> ---
>>>>    Documentation/userspace-api/media/drivers/dw100.rst |  7 +++++++
>>>>    include/uapi/linux/dw100.h                          | 11 +++++++++++
>>>>    2 files changed, 18 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 20aeae63a94f..3abad05849ad 100644
>>>> --- a/Documentation/userspace-api/media/drivers/dw100.rst
>>>> +++ b/Documentation/userspace-api/media/drivers/dw100.rst
>>>> @@ -20,4 +20,11 @@ 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_MAPPING (integer)``
>>>> +    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.
>>>> +
>>>>    .. _IMX8MP: https://www.nxp.com/webapp/Download?colCode=IMX8MPIEC
>>>
>>> This point to a document names "i.MX 8M Plus Applications Processor Datasheet
>>> for Industrial Products" which does not contain that reference.
>> My bad.. Wrong link. :)
>> Will repost with correct link.
> 
> Thanks. What I wanted to check is if it actually made sense to expose the
> synthetized HW LUT. But for this, one need to share the parameters / algo needed
> to generate them.
There is no special dewarping algorithm which strictly depends on the 
dw100 IP, or optimized for the IP capabilities.

  This way we can compare against other popular dewarp
> algorithms / API and see if they have something in common.
The dw100 hw lut description is rather close to a how you implement 
dewarping with openGL taking benefit of the shader pipeline stage.
The main differences with OpenGL implementation are:
- Fixed vertices coordinates (16x16) vs any
- Limited resolution on input (texture) coordinates (UQ12.4) vs float

Standard routines from OpenCV such as initUndistortRectifyMap()
https://docs.opencv.org/4.5.5/d9/d0c/group__calib3d.html#ga7dfb72c9cf9780a347fbe3d1c47e5d5a
can be used to generate the binary blob, with an additional decimation 
processing stage to satisfy the 16x16 macro block vertices grid and the 
fixed point format.

> 
> The issue I see with this control is relate to the message it gives. When adding
> controls for the prosperity, we want these control to actually be usable. This
> is possible if the documentation makes its usage obvious, or if there is Open
> Source userland to support that.
So yes, most famous vision opensource project such OpenCV can be used to 
generate the blob.
> 
> None of this is met, so as a side effect, this looks like NXP sneaking in
> private blob control into a publicly maintained Open Source project.
I then disagree with this statement considering my previous comments.

I plan to release publicly some programming examples on how to generate 
the dewarping map only using openCV library routines and aligned with 
lenses calibration state of the art method.
A dedicated openCV module taking benefit of the DW100 will be published 
as well.

A long term target is to add its support in libcamera, combined with all 
media components (CSI, ISP, ISI) pulled from upstream kernel tree.

  This isn't
> truly aligned with how V4L2 controls are meant to be. Doing trivial lut
> synthesis in the kernel could be fine though.
I am not sure what you meant with this comment.

As part of this patch series, an identity map is generated in the driver 
which should be enough for anyone familiar with dewarping process.
If you meant to generate the remapping table from the lens calibration 
data, I don't think this is a reasonable option considering the 
NP-completeness of the problem.

If this is the idea of binary blob (despite its public format 
description) which hurts you, the map can be exposed to the kernel in a 
more human readable format such Image_in(xin, yin) -> Image_out(xout, 
yout) in UQ1.31 format but will add extra processing at runtime for 
something which has to be done anyway offline, and memory overhead. But 
I don't think we can end with a generic v4l2 control considering the 
hardware restrictions (vertices position, limited fixed point 
resolution, etc..).

Adding a generic dewarping API to V4L2 is possible but this was not the 
scope of this patchset, and anyway missing data on any existing public 
dewarp hardware implementation supported by the kernel is somehow a 
blocker for this.

> 
>>>
>>>> diff --git a/include/uapi/linux/dw100.h b/include/uapi/linux/dw100.h
>>>> new file mode 100644
>>>> index 000000000000..0ef926c61cf0
>>>> --- /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>
>>>> +
>>>> +#define V4L2_CID_DW100_MAPPING		(V4L2_CID_USER_DW100_BASE + 1)
>>>> +
>>>> +#endif
>>>
>