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. 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. > > + 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 -- Regards, Laurent Pinchart
