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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@oss.nxp.com>
> > ---
> >  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