Re: [PATCH v9 7/8] media: dw100: Add i.MX8MP dw100 dewarper driver

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

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@oss.nxp.com>
>> Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   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