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

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Xavier Roumegue <xavier.roumegue@oss.nxp.com>,
	mchehab@kernel.org, stanimir.varbanov@linaro.org,
	laurent.pinchart@ideasonboard.com,
	tomi.valkeinen@ideasonboard.com, robh+dt@kernel.org,
	nicolas@ndufresne.ca, alexander.stein@ew.tq-group.com
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v4 6/9] media: uapi: Add a control for DW100 driver
Date: Mon, 25 Apr 2022 08:57:07 +0200	[thread overview]
Message-ID: <dba106ac-cee1-2493-13c7-ad9aef556a49@xs4all.nl> (raw)
In-Reply-To: <20220328141309.177611-7-xavier.roumegue@oss.nxp.com>

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.'

> +    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,

	Hans