Hello Hans,
On 7/30/22 12:49, Hans Verkuil wrote:
Hi Xavier,
On 30/07/2022 12:24, Xavier Roumegue wrote:
Add a V4L2 mem-to-mem driver for the Vivante DW100 Dewarp Processor IP
core found on i.MX8MP SoC.
The processor core applies a programmable geometrical transformation on
input images to correct distorsion introduced by lenses.
The transformation function is exposed as a grid map with 16x16 pixel
macroblocks indexed using X, Y vertex coordinates.
The dewarping map can be set from application through a dedicated v4l2
control. If not set or invalid, the driver computes an identity map
prior to starting the processing engine.
The driver supports scaling, cropping and pixel format conversion.
Signed-off-by: Xavier Roumegue <xavier.roumegue@xxxxxxxxxxx>
Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
---
drivers/media/platform/nxp/Kconfig | 1 +
drivers/media/platform/nxp/Makefile | 1 +
drivers/media/platform/nxp/dw100/Kconfig | 17 +
drivers/media/platform/nxp/dw100/Makefile | 3 +
drivers/media/platform/nxp/dw100/dw100.c | 1712 +++++++++++++++++
drivers/media/platform/nxp/dw100/dw100_regs.h | 117 ++
6 files changed, 1851 insertions(+)
create mode 100644 drivers/media/platform/nxp/dw100/Kconfig
create mode 100644 drivers/media/platform/nxp/dw100/Makefile
create mode 100644 drivers/media/platform/nxp/dw100/dw100.c
create mode 100644 drivers/media/platform/nxp/dw100/dw100_regs.h
<snip>
+/*
+ * Initialize the dewarping map with an identity mapping.
+ *
+ * A 16 pixels cell size grid is mapped on the destination image.
+ * The last cells width/height might be lesser than 16 if the destination image
+ * width/height is not divisible by 16. This dewarping grid map specifies the
+ * source image pixel location (x, y) on each grid intersection point.
+ * Bilinear interpolation is used to compute inner cell points locations.
+ *
+ * The coordinates are saved in UQ12.4 fixed point format.
+ */
+static void dw100_ctrl_dewarping_map_init(const struct v4l2_ctrl *ctrl,
+ u32 from_idx, u32 elems,
+ union v4l2_ctrl_ptr ptr)
+{
+ struct dw100_ctx *ctx =
+ container_of(ctrl->handler, struct dw100_ctx, hdl);
+
+ u32 sw, sh, dw, dh, mw, mh, i, j;
+ u16 qx, qy, qdx, qdy, qsh, qsw;
+ u32 *map = ctrl->p_cur.p_u32;
+
+ sw = ctx->q_data[DW100_QUEUE_SRC].pix_fmt.width;
+ dw = ctx->q_data[DW100_QUEUE_DST].pix_fmt.width;
+ sh = ctx->q_data[DW100_QUEUE_SRC].pix_fmt.height;
+ dh = ctx->q_data[DW100_QUEUE_DST].pix_fmt.height;
+
+ mw = dw100_get_n_vertices_from_length(dw);
+ mh = dw100_get_n_vertices_from_length(dh);
Note that ctrl->dims[] contains the array dimensions: use that rather than
calculating from dw/dh since using dims[] is more robust.
+
+ qsw = dw100_map_convert_to_uq12_4(sw);
+ qsh = dw100_map_convert_to_uq12_4(sh);
+ qdx = qsw / (mw - 1);
+ qdy = qsh / (mh - 1);
+
+ ctx->map_width = mw;
+ ctx->map_height = mh;
+ ctx->map_size = mh * mw * sizeof(u32);
+
+ for (i = 0, qy = 0; i < mh; i++, qy += qdy) {
This isn't correct: you actually start from 'from_idx', which is almost always 0,
except if userspace only sets the first N elements of an array, in that case
those N elements are copied to the control array and the remainder is initialized.
I admit that it doesn't make much sense in this particular case, but you still
need to take it into account.
Right, but it was done on purpose.It's incorrect from the driver
perspective to initialize partially the dewarping map as this can lead
to hardware hang up. Strictly speaking, accepting partial update would
force the driver to verify each individual pixel has been properly set
prior to start the streaming - which is not reasonable. API/semantic
wise, if init() is intended to be used as "set_range()", this might
rather makes more sense to add a dedicated set_range() callback, and
limit init() use for initializing the entire control range.
Having said that, I can implement as per your suggestions but I will not
implement sanity checks and error handling in case v4l2-ctrl api and/or
userspace have not initialized properly the mapping, i.e. partial
update, assuming that v4l2-ctrl api will always call init() for the
whole range on its first call.
I also would rename i and j to y and x, that makes more sense here.
Indeed, but as from_idx/elems involves a 1D handling, the loop index
will become idx.
+ if (qy > qsh)
+ qy = qsh;
+ for (j = 0, qx = 0; j < mw; j++, qx += qdx) {
+ if (qx > qsw)
+ qx = qsw;
+ *map++ = dw100_map_format_coordinates(qx, qy);
+ }
+ }
+
+ ctx->user_map_is_set = false;
+}
Regards,
Hans
Regards.
Xavier
